io_waitid_remove_wq() only takes the wait queue lock if ->head is set. io_waitid_wait() clears ->head first thing, before it tries to become the owner of the request by bumping ->refs. If somebody else owns the request at that point, nothing keeps it from completing the request while the wake up handler is still going to touch it: CPU0 (io_waitid_wait) CPU1 (io_waitid, holds a ref) list_del_init(&wait->entry) smp_store_release(&iw->head, NULL) __do_wait() finds another child io_waitid_remove_wq() // ->head == NULL, no locking io_waitid_finish(), IOU_COMPLETE // request freed and reused atomic_fetch_inc(&iw->refs) The same goes for __io_waitid_cancel() if it claims the request in that window. ->refs is part of the per opcode data of the request, so this increments whatever the next user of the request keeps there. If that happens to be zero, the handler concludes that it owns the request, and queues io_waitid_cb() for it. With the window held open by a delay, and the request reused for a receive that is waiting for data: BUG: KASAN: slab-use-after-free in __wake_up_common+0x1ca/0x1f0 __wake_up_sync_key+0x36/0x50 sock_def_readable+0xf1/0x420 unix_stream_sendmsg+0x60b/0x1110 Allocated by task 87: io_arm_apoll+0x1ed/0x940 Freed by task 87: __io_submit_flush_completions+0x118d/0x1a10 tctx_task_work_run+0x327/0x950 Only clear ->head once the handler owns the request. If it doesn't, the owner finds ->head set and has to wait for the lock, which is held by the caller of the handler. Deleting the already deleted entry again is harmless. Fixes: ab673c1bcaf2 ("io_uring/waitid: use io_waitid_remove_wq() consistently") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/waitid.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/io_uring/waitid.c b/io_uring/waitid.c index 2b756017423f..a91491650440 100644 --- a/io_uring/waitid.c +++ b/io_uring/waitid.c @@ -265,12 +265,18 @@ static int io_waitid_wait(struct wait_queue_entry *wait, unsigned mode, return 0; list_del_init(&wait->entry); - smp_store_release(&iw->head, NULL); - /* cancel is in progress */ + /* + * Cancel or issue is in progress. Leave ->head alone, so that whoever + * owns the request has to take the wait queue lock, which we hold, + * before it can complete and free it. + */ if (atomic_fetch_inc(&iw->refs) & IO_WAITID_REF_MASK) return 1; + /* pairs with the smp_load_acquire() in io_waitid_remove_wq() */ + smp_store_release(&iw->head, NULL); + req->io_task_work.func = io_waitid_cb; __io_req_task_work_add(req, IOU_F_TWQ_IN_WAKE); return 1; -- 2.55.0 io_uring_cmd_del_cancelable() uses hlist_del() to take a command off ->cancelable_uring_cmd, which leaves LIST_POISON1/2 behind in ->hash_node. Requests are recycled through the request cache without being reinitialised, and not every user of ->hash_node sets it up before it is used: If a vectored futex wait is woken while futex_wait_multiple_setup() is still queueing the futexes, io_futexv_wait() doesn't add the request to ->futex_list, and leaves the completion to the task_work queued by the wake up. io_futexv_complete() ends in __io_futex_complete(), whose hlist_del_init() is fine for a request that is unhashed, which is how all other users of the node leave it. If the request was a cancelable uring_cmd in its previous life, e.g. a ublk FETCH_REQ, it writes through LIST_POISON2: Oops: general protection fault, probably for non-canonical address KASAN: maybe wild-memory-access in range [0xdead000000000120-...] RIP: 0010:io_futexv_complete+0x219/0x420 Call Trace: tctx_task_work_run+0x804/0x950 tctx_task_work+0x71/0xa0 task_work_run+0x118/0x200 Leave the node unhashed. Fixes: 93b8cc60c37b ("io_uring: cancelable uring_cmd") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/uring_cmd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c index 73b606899833..b5486b56ee0c 100644 --- a/io_uring/uring_cmd.c +++ b/io_uring/uring_cmd.c @@ -87,7 +87,7 @@ static void io_uring_cmd_del_cancelable(struct io_uring_cmd *cmd, cmd->flags &= ~IORING_URING_CMD_CANCELABLE; io_ring_submit_lock(ctx, issue_flags); - hlist_del(&req->hash_node); + hlist_del_init(&req->hash_node); io_ring_submit_unlock(ctx, issue_flags); } -- 2.55.0 As long as IO_CHECK_CQ_OVERFLOW_BIT is set, io_cqring_wait_schedule() returns right away, before it gets to look at signals or arms the timeout, for io_cqring_wait() to flush the overflow list. If that doesn't empty the list, and the CQ ring doesn't hold the number of events the caller wants, the loop starts over, with the same result. The task spins in the kernel, and can't be killed. The assumption is that this can't happen: if CQEs can't be flushed, the ring is full, and min_events is capped to the size of the ring. But: - With IORING_SETUP_CQE_MIXED a 32b CQE needs two contiguous entries. It is stuck on the list if a single one is free, and then the ring holds less than cq_entries events. A single threaded application that asks for as many events as the ring has entries never gets out of io_uring_enter(). - Whether the ring is full is up to the CQ head, which the application can write to. While one thread waits for events on an empty ring, another one can move the head back so that the ring looks full and have a CQE overflow. The waiter can't be stopped any more, not even by SIGKILL. Only the application can make room in the CQ ring. Return to it if the list could not be flushed. Fixes: 3fcf19d592d5 ("io_uring: parse check_cq out of wq waiting") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/wait.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/io_uring/wait.c b/io_uring/wait.c index a48523c2f356..e03b40d9c141 100644 --- a/io_uring/wait.c +++ b/io_uring/wait.c @@ -291,9 +291,18 @@ int io_cqring_wait(struct io_ring_ctx *ctx, int min_events, u32 flags, check_cq = READ_ONCE(ctx->check_cq); if (unlikely(check_cq)) { - /* let the caller flush overflows, retry */ - if (check_cq & BIT(IO_CHECK_CQ_OVERFLOW_BIT)) + if (check_cq & BIT(IO_CHECK_CQ_OVERFLOW_BIT)) { io_cqring_do_overflow_flush(ctx); + /* + * No room for what has overflown. It takes the + * application to change that, so return to it. + */ + if (test_bit(IO_CHECK_CQ_OVERFLOW_BIT, + &ctx->check_cq)) { + ret = 0; + break; + } + } if (check_cq & BIT(IO_CHECK_CQ_DROPPED_BIT)) { ret = -EBADR; break; -- 2.55.0 When a thread exits, only its tracked requests are cancelled. Then its io-wq is torn down. io_wq_exit_workers() signals every worker, to get it out of an interruptible sleep, and waits for all of them to exit. Before they do, they go through the work that is left. Unbounded work is cancelled. Bounded work is run, as it is expected to end by itself. Not all of it does. An open has no file yet, so it counts as bounded, and the open of a FIFO waits for the other end. That is what the signal is for. But the workers are signalled once, and a worker that hasn't started the work by then loses the signal: io_wq_exit_workers() io_worker_handle_work() io_wq_worker_wake() set TIF_NOTIFY_SIGNAL io_assign_current_work() io_run_task_work() clear TIF_NOTIFY_SIGNAL io_wq_submit_work() fifo_open() wait_for_partner() wait_for_completion_timeout() The thread never gets out of do_exit(): IORING_OP_OPENAT of a FIFO without a writer, IOSQE_ASYNC exit of the thread that has submitted it hangs in more than 9 of 10 attempts (4 CPUs, KVM), until the process is killed. Set TIF_NOTIFY_SIGNAL for bounded work that is started with IO_WQ_BIT_EXIT set. The bit is set before the workers are signalled, and it is tested after the flag has been cleared, with clear_notify_signal() providing the barrier. So either the flag stays, or the bit is seen. Work that is started while the io-wq exits is treated like work that is being run at that time: it isn't interrupted unless it waits interruptibly. Fixes: afcc4015d1bf ("io-wq: provide an io_wq_put_and_exit() helper") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/io-wq.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/io_uring/io-wq.c b/io_uring/io-wq.c index 97523ccc7a71..433c003a83e5 100644 --- a/io_uring/io-wq.c +++ b/io_uring/io-wq.c @@ -655,9 +655,15 @@ static void io_worker_handle_work(struct io_wq_acct *acct, next_hashed = wq_next_work(work); - if (do_kill && - (work_flags & IO_WQ_WORK_UNBOUND)) + /* + * io_wq_exit_workers() signals us once, so that we + * don't sleep for ever. If that was before this work + * became the current one, the signal is gone. + */ + if (do_kill && (work_flags & IO_WQ_WORK_UNBOUND)) atomic_or(IO_WQ_WORK_CANCEL, &work->flags); + else if (do_kill) + set_notify_signal(current); req = container_of(work, struct io_kiocb, work); kcov_remote_start_common(req->ctx->kcov_handle); io_wq_submit_work(work); -- 2.55.0 A task that exits cancels its tracked requests and waits until they are gone. io_uring_cancel_generic() looks everywhere a request can be, and goes to sleep if it found nothing to cancel. It is woken up each time one of the requests of the task is completed by task work or freed. When io-wq is done with a request that has a link, there is a time in which the next request of the link can't be found: io_worker_handle_work() io_assign_current_work(worker, NULL) io_wq_free_work(work) io_req_find_next() // takes the link off the request io_free_req() // task work for the task io_assign_current_work(worker, linked) io_run_task_work() cond_resched() worker->cur_work = linked It is not pending, it is not the current work of a worker, and it is not yet on the list that it is about to be parked on. If the pass that the exiting task makes because of io_free_req() falls into that time, it finds nothing and the task goes to sleep. The worker then runs the link, which IO_WQ_BIT_EXIT doesn't prevent for bounded work. If that parks the request nothing is completed or freed, and nobody wakes up the task. If it is the process that exits it can't be killed any more either. NOP, IOSQE_ASYNC | IOSQE_IO_LINK FUTEX_WAIT, FUTEX2_PRIVATE exit of the thread, or of the process, up to 63us later With four of these per round 5 to 8 of 10,000 rounds hang (4 CPUs, KVM): io_uring_cancel_generic do_exit Commit f8b632e89a10 ("io_uring: tighten task exit cancellations") is about the same gap, and makes the task look again for whatever happens before io_free_req(). Free the request once the link is the current work of the worker, or pending. Whoever is woken up by that can find the link, until it is parked where cancellations look as well. To do so split io_wq_free_work() in two. Fixes: 768134d4f481 ("io_uring: don't do flush cancel under inflight_lock") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/io-wq.c | 18 +++++++++++++++--- io_uring/io_uring.c | 46 +++++++++++++++++++++++++++++----------------- io_uring/io_uring.h | 3 ++- 3 files changed, 46 insertions(+), 21 deletions(-) diff --git a/io_uring/io-wq.c b/io_uring/io-wq.c index 433c003a83e5..a5b3d2ba48bf 100644 --- a/io_uring/io-wq.c +++ b/io_uring/io-wq.c @@ -646,12 +646,13 @@ static void io_worker_handle_work(struct io_wq_acct *acct, /* handle a whole dependent link */ do { bool do_kill = test_bit(IO_WQ_BIT_EXIT, &wq->state); - struct io_wq_work *next_hashed, *linked; + struct io_wq_work *next_hashed, *linked, *done = work; unsigned int work_flags = atomic_read(&work->flags); unsigned int hash = __io_wq_is_hashed(work_flags) ? __io_get_work_hash(work_flags) : -1U; struct io_kiocb *req; + bool last; next_hashed = wq_next_work(work); @@ -670,7 +671,7 @@ static void io_worker_handle_work(struct io_wq_acct *acct, kcov_remote_stop(); io_assign_current_work(worker, NULL); - linked = io_wq_free_work(work); + last = io_wq_put_work(work, &linked); work = next_hashed; if (!work && linked && !io_wq_is_hashed(linked)) { work = linked; @@ -679,6 +680,13 @@ static void io_worker_handle_work(struct io_wq_acct *acct, io_assign_current_work(worker, work); if (linked) io_wq_enqueue(wq, linked); + /* + * Not before the link is current or pending work again. + * This may be the last time that a task is woken up + * which exits and looks for requests to cancel. + */ + if (last) + io_wq_free_work(done); if (hash != -1U && !next_hashed) { /* serialize hash clear with wake_up() */ @@ -1008,9 +1016,13 @@ void io_wq_set_exit_on_idle(struct io_wq *wq, bool enable) static void io_run_cancel(struct io_wq_work *work, struct io_wq *wq) { do { + struct io_wq_work *linked; + atomic_or(IO_WQ_WORK_CANCEL, &work->flags); io_wq_submit_work(work); - work = io_wq_free_work(work); + if (io_wq_put_work(work, &linked)) + io_wq_free_work(work); + work = linked; } while (work); } diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c index 0c91d466d825..464d9616234c 100644 --- a/io_uring/io_uring.c +++ b/io_uring/io_uring.c @@ -1486,28 +1486,40 @@ int io_poll_issue(struct io_kiocb *req, io_tw_token_t tw) return ret; } -struct io_wq_work *io_wq_free_work(struct io_wq_work *work) +/* + * io-wq is done with @work. Returns true if it held the last reference, and + * hands out the next request of the link then. The caller has to make sure + * that cancellations can find the latter before it calls io_wq_free_work(). + */ +bool io_wq_put_work(struct io_wq_work *work, struct io_wq_work **linked) { struct io_kiocb *req = container_of(work, struct io_kiocb, work); struct io_kiocb *nxt = NULL; - if (req_ref_put_and_test_atomic(req)) { - /* - * Only continue the link from here if the completion has been - * posted. If it's still pending, io_free_req() completes the - * request and the next link follows from there, after the CQE. - */ - if ((req->flags & IO_REQ_LINK_FLAGS) && - (req->flags & REQ_F_CQE_SKIP)) { - struct io_ring_ctx *ctx = req->ctx; + *linked = NULL; + if (!req_ref_put_and_test_atomic(req)) + return false; - mutex_lock(&ctx->uring_lock); - nxt = io_req_find_next(req); - mutex_unlock(&ctx->uring_lock); - } - io_free_req(req); + /* + * Only continue the link from here if the completion has been posted. + * If it's still pending, io_free_req() completes the request and the + * next link follows from there, after the CQE. + */ + if ((req->flags & IO_REQ_LINK_FLAGS) && (req->flags & REQ_F_CQE_SKIP)) { + struct io_ring_ctx *ctx = req->ctx; + + mutex_lock(&ctx->uring_lock); + nxt = io_req_find_next(req); + mutex_unlock(&ctx->uring_lock); } - return nxt ? &nxt->work : NULL; + if (nxt) + *linked = &nxt->work; + return true; +} + +void io_wq_free_work(struct io_wq_work *work) +{ + io_free_req(container_of(work, struct io_kiocb, work)); } void io_wq_submit_work(struct io_wq_work *work) @@ -1518,7 +1530,7 @@ void io_wq_submit_work(struct io_wq_work *work) bool needs_poll = false; int ret = 0, err = -ECANCELED; - /* one will be dropped by io_wq_free_work() after returning to io-wq */ + /* one will be dropped by io_wq_put_work() after returning to io-wq */ if (!(req->flags & REQ_F_REFCOUNT)) __io_req_set_refcount(req, 2); else diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h index 109261722ca7..c7b340e5d0a8 100644 --- a/io_uring/io_uring.h +++ b/io_uring/io_uring.h @@ -204,7 +204,8 @@ int io_do_iopoll(struct io_ring_ctx *ctx, bool force_nonspin); __cold void io_iopoll_try_reap_events(struct io_ring_ctx *ctx); void __io_submit_flush_completions(struct io_ring_ctx *ctx); -struct io_wq_work *io_wq_free_work(struct io_wq_work *work); +bool io_wq_put_work(struct io_wq_work *work, struct io_wq_work **linked); +void io_wq_free_work(struct io_wq_work *work); void io_wq_submit_work(struct io_wq_work *work); void io_free_req(struct io_kiocb *req); -- 2.55.0 io_ring_exit_work() asks each task which has used the ring to remove its tctx node, by task_work, and repeats that until the list is empty. io_tctx_exit_cb() doesn't touch the node while the task is in io_uring_cancel_generic(), which removes all the nodes at its end. So the work item queues the callback again as soon as it has completed, for nothing. That is more than a waste of time. task_work_run() only returns when it finds the list empty. If the rings are released from inside the cancellation, because requests of other rings held the last references to their files, up to 64 work items keep sending callbacks to the one task which is running them. It never finds the list empty, never gets back to the cancellation, the nodes stay, and so it goes on: a task with 60 pairs of rings which poll each other ran 280000 callbacks in 5 seconds, 20 seconds after exit_group(). It ends when all the senders happen to be late at the same time. Until then the task keeps a CPU and the work items busy. An exiting task can't be killed meanwhile. execve() cancels the requests in the same way, so a live task can get stuck there as well, for minutes. If it takes more than five, the work items run into the WARN_ON_ONCE(time_after(jiffies, timeout)) of io_ring_exit_work(). No privilege is needed for any of this. If the callback has met a task in cancellation, wait a little before it is sent again. Nobody has to wake the work item up for that: either the node is gone by then, or it is the turn of the next round. Fixes: d56d938b4bef ("io_uring: do ctx initiated file note removal") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/io_uring.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c index 464d9616234c..a5fd1845a1d5 100644 --- a/io_uring/io_uring.c +++ b/io_uring/io_uring.c @@ -2348,6 +2348,7 @@ struct io_tctx_exit { struct callback_head task_work; struct completion completion; struct io_ring_ctx *ctx; + bool in_cancel; }; static __cold void io_tctx_exit_cb(struct callback_head *cb) @@ -2362,7 +2363,8 @@ static __cold void io_tctx_exit_cb(struct callback_head *cb) * tctx can be NULL if the queueing of this task_work raced with * work cancelation off the exec path. */ - if (tctx && !atomic_read(&tctx->in_cancel)) + work->in_cancel = tctx && atomic_read(&tctx->in_cancel); + if (tctx && !work->in_cancel) io_uring_del_tctx_node((unsigned long)work->ctx); complete(&work->completion); } @@ -2530,6 +2532,14 @@ static __cold void io_ring_exit_work(struct work_struct *work) * wait is marked as interruptible. */ wait_for_completion_interruptible(&exit.completion); + /* + * The task removes the node when it is done with its + * cancellation. Give it the time for that: it doesn't get back + * to it as long as it finds new task_work each time it has run + * the old one, and we may be one of many who send it. + */ + if (exit.in_cancel) + schedule_timeout_idle(HZ / 20); mutex_lock(&ctx->uring_lock); mutex_lock(&ctx->tctx_lock); } -- 2.55.0 Since local task_work moved to the mpscq, ->cq_wait_nr is a credit: the add that counts it down to zero, or exchanges it for IO_CQ_WAKE_INIT, issues the only wake up of the wait cycle, and every later add returns early as the count is <= 0. An add looks at the count after it has queued its entry, so an add that gets delayed in between can take the credit of a later cycle. That was assumed to cause an early wake up at worst, but the waiter publishes the count before it changes its task state: CPU0 (submitter, io_cqring_wait) CPU1 (io_req_local_work_add) mpscq_push(req A) io_run_local_work() // runs req A atomic_set(&ctx->cq_wait_nr, 1) atomic_read(&ctx->cq_wait_nr) == 1 atomic_xchg(.., IO_CQ_WAKE_INIT) wake_up_state(.., TASK_INTERRUPTIBLE) // still TASK_RUNNING, no-op set_current_state(TASK_INTERRUPTIBLE) io_local_work_pending() == false schedule() mpscq_push(req B) cq_wait_nr <= 0, return The task now sleeps with req B queued, until a signal or a timeout ends the wait. Set the task state first and only then make the count visible, so that whoever takes the credit finds a task it can wake. The full barrier that orders the count against the check of the work list is kept. Do the same for the wait of the BPF driven loop, which copied the sequence. Fixes: d46ab2c98aba ("io_uring: switch local task_work to a mpscq") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/loop.c | 8 +------- io_uring/tw.c | 2 +- io_uring/wait.c | 3 +-- io_uring/wait.h | 15 +++++++++++++++ 4 files changed, 18 insertions(+), 10 deletions(-) diff --git a/io_uring/loop.c b/io_uring/loop.c index 3d472b7827fa..2340459164a1 100644 --- a/io_uring/loop.c +++ b/io_uring/loop.c @@ -10,12 +10,6 @@ static inline int io_loop_nr_cqes(const struct io_ring_ctx *ctx, return lp->cq_wait_idx - READ_ONCE(ctx->rings->cq.tail); } -static inline void io_loop_wait_start(struct io_ring_ctx *ctx, unsigned nr_wait) -{ - atomic_set(&ctx->cq_wait_nr, nr_wait); - set_current_state(TASK_INTERRUPTIBLE); -} - static inline void io_loop_wait_finish(struct io_ring_ctx *ctx) { __set_current_state(TASK_RUNNING); @@ -25,7 +19,7 @@ static inline void io_loop_wait_finish(struct io_ring_ctx *ctx) static void io_loop_wait(struct io_ring_ctx *ctx, struct iou_loop_params *lp, unsigned nr_wait) { - io_loop_wait_start(ctx, nr_wait); + io_cq_wait_arm(ctx, nr_wait); if (unlikely(io_local_work_pending(ctx) || io_loop_nr_cqes(ctx, lp) <= 0) || diff --git a/io_uring/tw.c b/io_uring/tw.c index f9affd68dd26..98e3b1bc61b4 100644 --- a/io_uring/tw.c +++ b/io_uring/tw.c @@ -165,7 +165,7 @@ void io_req_local_work_add(struct io_kiocb *req, unsigned flags) /* * The xchg() in mpscq_push() implies a full barrier, which pairs with - * the barrier in set_current_state() on the io_cqring_wait() side. This + * the barrier in io_cq_wait_arm() on the io_cqring_wait() side. This * ensures that either we see the updated ->cq_wait_nr, or waiters going * to sleep will observe the work added to the list, which is similar to * the wait/wake task state sync. diff --git a/io_uring/wait.c b/io_uring/wait.c index e03b40d9c141..5093365e9eb2 100644 --- a/io_uring/wait.c +++ b/io_uring/wait.c @@ -257,8 +257,7 @@ int io_cqring_wait(struct io_ring_ctx *ctx, int min_events, u32 flags, unsigned long check_cq; if (ctx->flags & IORING_SETUP_DEFER_TASKRUN) { - atomic_set(&ctx->cq_wait_nr, nr_wait); - set_current_state(TASK_INTERRUPTIBLE); + io_cq_wait_arm(ctx, nr_wait); } else { prepare_to_wait_exclusive(&ctx->cq_wait, &iowq.wq, TASK_INTERRUPTIBLE); diff --git a/io_uring/wait.h b/io_uring/wait.h index b7b9c46b1b01..eb5ae8a6fc94 100644 --- a/io_uring/wait.h +++ b/io_uring/wait.h @@ -14,6 +14,21 @@ */ #define IO_CQ_WAKE_INIT (-1) +/* + * Start a wait cycle of the current task. The add that uses up ->cq_wait_nr + * may be one that queued its entry long ago and only now gets to look at the + * count. As nobody else wakes us after that, it has to find us sleeping, hence + * the task state must be visible before the count is. The barrier pairs with + * the xchg() in mpscq_push(), see io_req_local_work_add(). + */ +static inline void io_cq_wait_arm(struct io_ring_ctx *ctx, int nr_wait) +{ + __set_current_state(TASK_INTERRUPTIBLE); + atomic_set_release(&ctx->cq_wait_nr, nr_wait); + /* order the above against looking at the work list */ + smp_mb(); +} + struct ext_arg { size_t argsz; struct timespec64 ts; -- 2.55.0 io_uring_try_cancel_requests() sets ->cq_wait_nr to 1 so that the next io_req_local_work_add() wakes the cancelling task. The add that takes the count to zero is the only one that wakes, later ones see a count <= 0 and return. io_uring_cancel_generic() however only becomes TASK_INTERRUPTIBLE much later, so the one wake up can be spent on a running task. That is fine as long as the entry of that add is still queued, as the io_local_work_pending() check then skips the sleep. It is not if the add was delayed after queueing its entry: CPU0 (submitter in exec or exit) CPU1 (io_req_local_work_add) mpscq_push(req A) io_uring_try_cancel_requests() io_run_local_work() // runs req A io_uring_try_cancel_requests() atomic_set(&ctx->cq_wait_nr, 1) atomic_read(&ctx->cq_wait_nr) == 1 count taken, wake_up_state() // task is running, no-op prepare_to_wait(TASK_INTERRUPTIBLE) io_local_work_pending() == false schedule() mpscq_push(req B) cq_wait_nr <= 0, return Only the sleeping task can run req B, so it is stuck in execve() until a signal happens to wake it. An exiting task doesn't even get that. Arm the count again once the task state is set and before the work list is checked, for the rings whose local work this task is the one to run. Fixes: d46ab2c98aba ("io_uring: switch local task_work to a mpscq") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/cancel.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/io_uring/cancel.c b/io_uring/cancel.c index 9a92a6aa2bcc..64eb4a993c00 100644 --- a/io_uring/cancel.c +++ b/io_uring/cancel.c @@ -694,8 +694,18 @@ __cold void io_uring_cancel_generic(bool cancel_all, struct io_sq_data *sqd) io_uring_drop_tctx_refs(current); /* only skip the sleep for local work this task may run */ xa_for_each(&tctx->xa, index, node) { - if (io_local_work_pending(node->ctx) && - io_allowed_defer_tw_run(node->ctx)) + if (!(node->ctx->flags & IORING_SETUP_DEFER_TASKRUN) || + !io_allowed_defer_tw_run(node->ctx)) + continue; + /* + * The wake up io_uring_try_cancel_requests() asked for + * may have been issued while we were still running. Ask + * again now that it can't be missed. + */ + atomic_set(&node->ctx->cq_wait_nr, 1); + /* pairs with the xchg() in mpscq_push() */ + smp_mb(); + if (io_local_work_pending(node->ctx)) goto end_wait; } /* -- 2.55.0 A push makes the node the new tail, and then links it to its predecessor. In between the queue isn't empty, but mpscq_pop() can't get at the node. The consumers deal with that by trying again, with a cond_resched() in the loop to give a preempted producer the chance to finish. cond_resched() doesn't do that if the consumer has a higher priority than the producer and both have to use the same CPU: producer (SCHED_OTHER) consumer (SCHED_FIFO), same CPU io_req_local_work_add() mpscq_push() prev = xchg(&q->tail, node) io_cqring_wait() __io_run_local_work() again: cond_resched() // nothing to do mpscq_pop() // NULL // work pending, no events yet goto again The producer can be any task that is able to queue task_work for the ring: an io-wq worker, or somebody who sends a message with IORING_OP_MSG_RING. The consumer spins, holding ->uring_lock, until the scheduler takes pity on the fair class, which takes about a second with the default settings of the fair server, and forever without it. The loops in tctx_task_work_run() and io_cancel_local_task_work() have the same problem. Don't allow preemption between the two steps. That bounds the time the window is open on the CPU of the producer by what interrupts take, and a consumer that waits for that is on another CPU. The cost is a preempt_disable()/preempt_enable() pair per push. Fixes: 50cb44bd0d5f ("io_uring/mpscq: add lockless multi-producer, single-consumer FIFO queue") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/mpscq.h | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/io_uring/mpscq.h b/io_uring/mpscq.h index f910526766fd..da9474497cc8 100644 --- a/io_uring/mpscq.h +++ b/io_uring/mpscq.h @@ -25,9 +25,10 @@ * its predecessor. In between, the tail end of the queue is not yet reachable * from the head. mpscq_pop() detects this and returns NULL, while mpscq_empty() * reports false. The consumer must not treat such a NULL as "queue empty" - it - * should retry later. The window is two instructions wide, but a producer can - * be preempted inside it, so the consumer must not spin on it while holding - * resources the producer might need to make progress. + * should retry later. The window is two instructions wide and the producer + * can't be preempted inside it, but it can be interrupted, so the consumer must + * not spin on it while holding resources the producer might need to make + * progress. * * The consumer side only supports a single consumer at a time, callers must * provide their own serialization for it. The stub node is what allows the @@ -69,6 +70,12 @@ static inline bool mpscq_push(struct mpscq *q, struct llist_node *node) struct llist_node *prev; node->next = NULL; + /* + * All the consumer can do about a node that isn't linked yet is to try + * again. If it is what preempted us, and we don't get to run as long + * as it does, that never ends. + */ + preempt_disable(); /* * xchg() implies a full barrier, so the initialization of the * entry (including ->next above) is visible before the node can @@ -77,6 +84,7 @@ static inline bool mpscq_push(struct mpscq *q, struct llist_node *node) */ prev = xchg(&q->tail, node); WRITE_ONCE(prev->next, node); + preempt_enable(); return prev == &q->stub; } -- 2.55.0 io_wq_enqueue() wakes a worker off the free list. If there is none it creates one, unless ->nr_running says that a worker is running. That relies on whoever takes ->nr_running to zero to check for pending work, which io_wq_dec_running() no longer does for a worker without ->cur_work, on the grounds that an idle worker is either woken right away or someone else has created a worker. Neither has to be the case: worker A (idle) worker B (idle) io_wq_enqueue() woken for work 1 woken (e.g. timeout) takes work 1 work list is empty queues work 2 B is first on the free list: wake_up_process(B) leaves the free list work 1 blocks nr_running 2 -> 1 schedule_timeout() nr_running 1 -> 0 ->cur_work == NULL Work 2 isn't looked at until work 1 is done or A's idle timeout fires, five seconds later. ->cur_work is also NULL while a worker blocks on ->uring_lock in io_wq_free_work(), with the same result if it was the only one running. Let the idle worker leave ->nr_running by itself before it takes its last look at the work list. Both that and queueing work are done under acct->lock, so either the worker sees the work, or the enqueue side sees that it can't count on this worker. This doesn't bring back the request for a new worker which the check was meant to avoid, and as idle workers don't get to io_wq_dec_running() any more, the check can go. Fixes: 8343cae362e1 ("io_uring/io-wq: ignore non-busy worker going to sleep") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/io-wq.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/io_uring/io-wq.c b/io_uring/io-wq.c index a5b3d2ba48bf..a3a650813629 100644 --- a/io_uring/io-wq.c +++ b/io_uring/io-wq.c @@ -467,11 +467,9 @@ static void io_wq_dec_running(struct io_worker *worker) if (!atomic_dec_and_test(&acct->nr_running)) return; - if (!worker->cur_work) - return; if (!io_acct_run_queue(acct)) return; - if (io_wq_hash_defer(worker->cur_work, acct)) { + if (worker->cur_work && io_wq_hash_defer(worker->cur_work, acct)) { raw_spin_unlock(&acct->lock); return; } @@ -751,6 +749,19 @@ static int io_wq_worker(void *data) raw_spin_unlock(&acct->workers_lock); if (io_run_task_work()) continue; + /* + * io_wq_enqueue() leaves new work to us for as long as we are + * accounted as running. Get out of ->nr_running before taking + * the last look at the work list, rather than when scheduling + * out. No need to ask for another worker then, we can do it. + */ + clear_bit(IO_WORKER_F_RUNNING, &worker->flags); + atomic_dec(&acct->nr_running); + if (io_acct_run_queue(acct)) { + io_wq_worker_running(current); + io_worker_handle_work(acct, worker); + continue; + } ret = schedule_timeout(WORKER_IDLE_TIMEOUT); if (signal_pending(current)) { struct ksignal ksig; -- 2.55.0 A worker exits if its idle timeout fired and it isn't the last one, if its affinity was changed, or if the wq is marked exit-on-idle. It decides so after it found the work list empty, and until io_worker_exit() is through it stays on the free list and in ->nr_running: worker io_wq_enqueue() io_acct_run_queue() == false queues work io_acct_activate_free_worker() wake_up_process(worker) // found one, nothing to create acct->nr_workers-- break io_worker_exit() Nobody is going to look at the work. If other workers exist, it has to wait until one of them finishes what it is blocked on or hits its idle timeout. If not, which is possible for the affinity and the exit-on-idle case, it sits there until something else is queued. For exit-on-idle it takes a task that closes its last ring and sets up a new one while the workers are on their way out. Make the worker take itself off the free list when it gives up its ->nr_workers slot, then leave ->nr_running, and only then take the final look at the work list. That look and queueing work are both done under acct->lock, so either the worker sees the work and stays, or the enqueue side sees that the worker is gone and wakes or creates another one. If it wants to stay and finds that its slot has been taken, a worker has been created in its place, which is going to look at the work list, and it can leave after all. Fixes: 38aa434ab933 ("io_uring/io-wq: add exit-on-idle state") Fixes: 05c5f4ee4da7 ("io-wq: get rid of FIXED worker flag") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/io-wq.c | 36 +++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/io_uring/io-wq.c b/io_uring/io-wq.c index a3a650813629..89b598b724c4 100644 --- a/io_uring/io-wq.c +++ b/io_uring/io-wq.c @@ -703,6 +703,35 @@ static void io_worker_handle_work(struct io_wq_acct *acct, } while (1); } +/* + * The worker has decided to exit, and has given up its ->nr_workers slot and + * its place on the free list. io_wq_enqueue() may have picked it from that + * list, or seen it running, after it last looked at the work list. Leave + * ->nr_running as well and check again. Returns false if the worker has to + * stay. + */ +static bool io_worker_retire(struct io_wq_acct *acct, struct io_worker *worker) +{ + /* turns io_wq_dec_running() and the scheduler hooks into no-ops */ + clear_bit(IO_WORKER_F_UP, &worker->flags); + atomic_dec(&acct->nr_running); + if (!io_acct_run_queue(acct)) + return true; + raw_spin_unlock(&acct->lock); + + raw_spin_lock(&acct->workers_lock); + if (acct->nr_workers >= acct->max_workers) { + /* a worker was created in our place, the work is its business */ + raw_spin_unlock(&acct->workers_lock); + return true; + } + acct->nr_workers++; + raw_spin_unlock(&acct->workers_lock); + atomic_inc(&acct->nr_running); + set_bit(IO_WORKER_F_UP, &worker->flags); + return false; +} + static int io_wq_worker(void *data) { struct io_worker *worker = data; @@ -740,9 +769,14 @@ static int io_wq_worker(void *data) if ((last_timeout && (exit_mask || acct->nr_workers > 1)) || test_bit(IO_WQ_BIT_EXIT_ON_IDLE, &wq->state)) { acct->nr_workers--; + if (test_and_clear_bit(IO_WORKER_F_FREE, &worker->flags)) + hlist_nulls_del_init_rcu(&worker->nulls_node); raw_spin_unlock(&acct->workers_lock); __set_current_state(TASK_RUNNING); - break; + if (io_worker_retire(acct, worker)) + break; + last_timeout = false; + continue; } last_timeout = false; __io_worker_idle(acct, worker); -- 2.55.0 A worker that is the last one to go to sleep while work is pending asks for another worker. io_wq_dec_running() bumps ->nr_running on behalf of the new worker and queues create_worker_cb() as task_work for the owner of the io-wq. Until that has run, everybody else takes it that a worker is running which is going to look after the work list: io_wq_enqueue() doesn't create one, and other workers that go to sleep don't find themselves to be the last. If the worker that made the request exits, io_worker_exit() cancels the task_work, and io_worker_cancel_cb() drops ->nr_running without a look at the work list: worker A worker B io_wq_enqueue() blocks, work is pending nr_running++ queues create_worker_cb() wakes, handles the work idle timeout, decides to exit picks up work 1 queues work 2 B is on the free list still: wakes B work 1 blocks nr_running 2 -> 1 io_worker_exit() io_worker_cancel_cb() nr_running 1 -> 0 Work 2 has to wait for work 1, although there is room for more workers. It takes an owner that doesn't get around to run its task_work for the time of the idle timeout, less if the worker exits because its affinity was changed or the io-wq is marked exit-on-idle. Let the worker stay around until its request has been dealt with. Only the worker itself sets ->create_state, so it can't be set behind its back once it has decided to leave. Fixes: 1d5f5ea7cb7d ("io-wq: remove worker to owner tw dependency") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/io-wq.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/io_uring/io-wq.c b/io_uring/io-wq.c index 89b598b724c4..31c7d6fc426b 100644 --- a/io_uring/io-wq.c +++ b/io_uring/io-wq.c @@ -765,9 +765,13 @@ static int io_wq_worker(void *data) * idle-exit, drop the worker as well. This is used to avoid * keeping io-wq workers around for tasks that no longer have * any active io_uring instances. + * Not as long as a request of ours for another worker is + * pending, though. It is accounted as a running worker, which + * others rely on, and io_worker_exit() would cancel it. */ - if ((last_timeout && (exit_mask || acct->nr_workers > 1)) || - test_bit(IO_WQ_BIT_EXIT_ON_IDLE, &wq->state)) { + if (((last_timeout && (exit_mask || acct->nr_workers > 1)) || + test_bit(IO_WQ_BIT_EXIT_ON_IDLE, &wq->state)) && + !test_bit(0, &worker->create_state)) { acct->nr_workers--; if (test_and_clear_bit(IO_WORKER_F_FREE, &worker->flags)) hlist_nulls_del_init_rcu(&worker->nulls_node); -- 2.55.0 io_get_next_work() takes all the work items that share a hash off the work list in one go, and the worker runs them one after the other. Only the one it is working on is visible as ->cur_work. The others are neither on the list nor current, so io_wq_cancel_cb() can't find them. IORING_OP_ASYNC_CANCEL and IORING_REGISTER_SYNC_CANCEL return -ENOENT for a request that has not even been started, e.g. a buffered write queued behind another write to the same file that is blocked, and the request is carried out. The cancellation done for a task or ring that goes away gets there in the end, as it keeps trying, but it has to wait for each item to become current. Keep a pointer to the remainder of the chain next to ->cur_work, and have io_wq_worker_cancel() look at it. The items on it belong to the worker, which needs ->lock to move on to the next one. Matching work is flagged with IO_WQ_WORK_CANCEL and fails with -ECANCELED when its turn comes. The worker isn't signalled for it, that would interrupt the unrelated work it is busy with. As the request isn't gone yet, it is reported like running work, with -EALREADY. Fixes: 86f3cd1b589a ("io-wq: handle hashed writes in chains") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/io-wq.c | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/io_uring/io-wq.c b/io_uring/io-wq.c index 31c7d6fc426b..dfdc78612783 100644 --- a/io_uring/io-wq.c +++ b/io_uring/io-wq.c @@ -56,6 +56,8 @@ struct io_worker { struct io_wq_acct *acct; struct io_wq_work *cur_work; + /* rest of the hashed chain ->cur_work was taken off the list with */ + struct io_wq_work *next_work; raw_spinlock_t lock; struct completion ref_done; @@ -581,6 +583,17 @@ static struct io_wq_work *io_get_next_work(struct io_wq_acct *acct, return NULL; } +static void __io_assign_current_work(struct io_worker *worker, + struct io_wq_work *work) +{ + raw_spin_lock(&worker->lock); + worker->cur_work = work; + /* stays valid while in between two items of a chain */ + if (work) + worker->next_work = wq_next_work(work); + raw_spin_unlock(&worker->lock); +} + static void io_assign_current_work(struct io_worker *worker, struct io_wq_work *work) { @@ -589,9 +602,7 @@ static void io_assign_current_work(struct io_worker *worker, cond_resched(); } - raw_spin_lock(&worker->lock); - worker->cur_work = work; - raw_spin_unlock(&worker->lock); + __io_assign_current_work(worker, work); } /* @@ -623,9 +634,7 @@ static void io_worker_handle_work(struct io_wq_acct *acct, * work list, but isn't yet discoverable as the * current work item for this worker. */ - raw_spin_lock(&worker->lock); - worker->cur_work = work; - raw_spin_unlock(&worker->lock); + __io_assign_current_work(worker, work); } raw_spin_unlock(&acct->lock); @@ -1179,6 +1188,7 @@ static bool __io_wq_worker_cancel(struct io_worker *worker, static bool io_wq_worker_cancel(struct io_worker *worker, void *data) { struct io_cb_cancel_data *match = data; + struct io_wq_work *work; /* * Hold the lock to avoid ->cur_work going out of scope, caller @@ -1187,6 +1197,19 @@ static bool io_wq_worker_cancel(struct io_worker *worker, void *data) raw_spin_lock(&worker->lock); if (__io_wq_worker_cancel(worker, match, worker->cur_work)) match->nr_running++; + /* + * Hashed work is taken off the work list a chain at a time, what the + * worker hasn't started yet can only be found here. It doesn't move on + * without the lock. Just mark the work, it fails once it is its turn. + */ + for (work = worker->next_work; work; work = wq_next_work(work)) { + if (match->nr_running && !match->cancel_all) + break; + if (match->fn(work, match->data)) { + atomic_or(IO_WQ_WORK_CANCEL, &work->flags); + match->nr_running++; + } + } raw_spin_unlock(&worker->lock); return match->nr_running && !match->cancel_all; -- 2.55.0 A request on a pollable file that was forced to io-wq is issued without blocking there. If that ends in -EAGAIN, io_wq_submit_work() arms poll for it and returns. It looks at IO_WQ_WORK_CANCEL once, before it issues the request. Somebody who wants to cancel the request in between finds it running: io-wq worker io_try_cancel() io_wq_submit_work() IO_WQ_WORK_CANCEL isn't set io_issue_sqe() = -EAGAIN io_async_cancel_one() sets IO_WQ_WORK_CANCEL signals the worker = -EALREADY io_poll_cancel() = -ENOENT ... = -ENOENT io_arm_poll_handler() = IO_APOLL_OK return Nobody looks at the flag again, and the signal doesn't matter to a worker that doesn't sleep. The request is armed as if nothing had happened: - a linked timeout completes with -ENOENT, and the request it was meant to limit waits for as long as it takes. - IORING_OP_ASYNC_CANCEL returns -EALREADY, and the request stays. - IORING_REGISTER_SYNC_CANCEL waits for the request to complete if it gets -EALREADY. Since commit 5623eb1ed035 ("io_uring/tctx: add separate lock for list of tctx's in ctx") it doesn't sleep but spins, so its next attempt finds the armed poll. The same goes for a multishot poll, recv or accept that is armed from io-wq. With a read of an empty pipe, IOSQE_ASYNC, and a linked timeout of 5ms or a cancel in the same submission, in a guest that is slow enough (TCG, KASAN), the request is lost in 21 of 24 rounds of 40 or 100 attempts. While it arms poll from io-wq, __io_arm_poll_handler() owns the request. Look at the flag once the request is in the hash table. The table is searched and the request is added with ->uring_lock held, and the flag is set before the search. So either the search finds the request, or it came first and the flag is seen. With the request marked as cancelled, the task work that is queued, right away or because ownership can't be released, fails it with -ECANCELED. What the canceller is told doesn't change. In this window a linked timeout still completes with -ENOENT rather than -ETIME, a cancel request with -EALREADY, and one with IORING_ASYNC_CANCEL_ALL or _ANY with a count that doesn't include the request. But the request is cancelled, so what is new is a request that completes with -ECANCELED after such a result. This doesn't help in two cases: - IORING_ASYNC_CANCEL_ALL and _ANY, if the work is flagged after the test added here, while io-wq still has it as the current work of the worker. io_cancel_cb() stamps it with the sequence of the cancel, so the search of the hash table on behalf of the same cancel takes it for done and skips it. - requests that are parked elsewhere by io-wq, like IORING_OP_FUTEX_WAIT and IORING_OP_WAITID. They have the same problem. Fixes: 90fa02883f06 ("io_uring: implement async hybrid mode for pollable requests") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/poll.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/io_uring/poll.c b/io_uring/poll.c index 5447a7c24dce..577ae978bfc9 100644 --- a/io_uring/poll.c +++ b/io_uring/poll.c @@ -617,6 +617,18 @@ static int __io_arm_poll_handler(struct io_kiocb *req, io_poll_add_hash(req, issue_flags); + /* + * Whoever tried to cancel the request while io-wq was running it has + * flagged the work item. If it searched the hash table before the + * request got there it found nothing, and it won't come back. Both + * the search and the insertion are done with ->uring_lock held, and + * the flag is set before the search. If the search came first, + * the flag is visible by now. + */ + if (ipt->owning && + (atomic_read(&req->work.flags) & IO_WQ_WORK_CANCEL)) + io_poll_mark_cancelled(req); + if (mask && (poll->events & EPOLLET) && io_poll_can_finish_inline(req, ipt)) { __io_poll_execute(req, mask, 0); -- 2.55.0 IORING_OP_FUTEX_WAIT, IORING_OP_FUTEX_WAITV and IORING_OP_WAITID don't block. They put the request on a list of the ring, where a wake up or a cancellation finds it, and return IOU_ISSUE_SKIP_COMPLETE. They do so as well if they are run by io-wq, e.g. because of IOSQE_ASYNC. io_wq_submit_work() looks at IO_WQ_WORK_CANCEL once, before it issues the request. Somebody who wants to cancel the request after that, and before it is on the list, finds it running: io-wq worker io_try_cancel() io_wq_submit_work() IO_WQ_WORK_CANCEL isn't set io_async_cancel_one() sets IO_WQ_WORK_CANCEL signals the worker = -EALREADY ... io_futex_cancel() = -ENOENT ... io_futex_wait() adds the request to ->futex_list = IOU_ISSUE_SKIP_COMPLETE Nobody looks at the flag again. IORING_OP_ASYNC_CANCEL returns -EALREADY and the request stays. It is the problem that "io_uring/poll: don't lose a cancel as io-wq arms poll" deals with for poll. Look at the flag once the request is on the list, and cancel it if it is set. The lists are searched and the requests are added with ->uring_lock held, and the flag is set before the search. So either the search finds the request, or it came first and the flag is seen. What the canceller is told doesn't change, see the other patch, also for IORING_ASYNC_CANCEL_ALL and _ANY. Fixes: 194bb58c6090 ("io_uring: add support for futex wake and wait") Fixes: 8f350194d5cf ("io_uring: add support for vectored futex waits") Fixes: f31ecf671ddc ("io_uring: add IORING_OP_WAITID support") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/futex.c | 4 ++++ io_uring/io_uring.h | 17 +++++++++++++++++ io_uring/waitid.c | 2 ++ 3 files changed, 23 insertions(+) diff --git a/io_uring/futex.c b/io_uring/futex.c index eaee14242a3a..64909a2a8c88 100644 --- a/io_uring/futex.c +++ b/io_uring/futex.c @@ -283,6 +283,8 @@ int io_futexv_wait(struct io_kiocb *req, unsigned int issue_flags) */ __set_current_state(TASK_RUNNING); hlist_add_head(&req->hash_node, &ctx->futex_list); + if (io_wq_cancel_seen(req, issue_flags)) + __io_futex_cancel(req); } else { iof->futexv_unqueued = 1; if (woken != -1) @@ -323,6 +325,8 @@ int io_futex_wait(struct io_kiocb *req, unsigned int issue_flags) &ifd->q, NULL, NULL); if (!ret) { hlist_add_head(&req->hash_node, &ctx->futex_list); + if (io_wq_cancel_seen(req, issue_flags)) + __io_futex_cancel(req); io_ring_submit_unlock(ctx, issue_flags); return IOU_ISSUE_SKIP_COMPLETE; diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h index c7b340e5d0a8..36d0d67fcb9f 100644 --- a/io_uring/io_uring.h +++ b/io_uring/io_uring.h @@ -381,6 +381,23 @@ static inline void io_ring_submit_lock(struct io_ring_ctx *ctx, lockdep_assert_held(&ctx->uring_lock); } +/* + * For a handler that is run by io-wq and parks the request on a list where + * cancellations find it, rather than completing it. Whoever tried to cancel + * the request before that found it running. It has flagged the work item, + * has searched the list in vain, and won't come back. To be called with + * ->uring_lock held, which the search needs as well, once the request is on + * the list: if the search came first, the flag is visible. + */ +static inline bool io_wq_cancel_seen(struct io_kiocb *req, + unsigned int issue_flags) +{ + lockdep_assert_held(&req->ctx->uring_lock); + + return (issue_flags & IO_URING_F_IOWQ) && + (atomic_read(&req->work.flags) & IO_WQ_WORK_CANCEL); +} + static inline void io_commit_cqring(struct io_ring_ctx *ctx) { /* order cqe stores with ring update */ diff --git a/io_uring/waitid.c b/io_uring/waitid.c index a91491650440..2ba2369681d0 100644 --- a/io_uring/waitid.c +++ b/io_uring/waitid.c @@ -349,6 +349,8 @@ int io_waitid(struct io_kiocb *req, unsigned int issue_flags) * a waitqueue callback, or if someone cancels it. */ if (!io_waitid_drop_issue_ref(req)) { + if (io_wq_cancel_seen(req, issue_flags)) + __io_waitid_cancel(req, true); io_ring_submit_unlock(ctx, issue_flags); return IOU_ISSUE_SKIP_COMPLETE; } -- 2.55.0 If the request to be cancelled is being run by io-wq, io_async_cancel_one() flags the work, signals the worker and returns -EALREADY. Since commit ccbf726171b7 ("io_uring: perform poll removal even if async work removal is successful") io_try_cancel() goes on regardless, as poll may be armed for the request. If it isn't, and nothing else turns up, the result is the -ENOENT of the last search. __io_async_cancel() makes up for that. On -ENOENT it tries the io-wq of every task that uses the ring, and that of the caller is one of them. So the work is found a second time, and the result is -EALREADY after all. Unless the worker is done with it by then. If the work is flagged before io_wq_submit_work() looks at IO_WQ_WORK_CANCEL, all the worker does is to queue the request to be failed by task work. Then it isn't anywhere to be found until the task work has run, and - IORING_REGISTER_SYNC_CANCEL returns -ENOENT, - IORING_OP_ASYNC_CANCEL completes with -ENOENT, for a request that they have just cancelled, and that completes with -ECANCELED. With a read of an empty pipe, IOSQE_ASYNC, and a sync cancel right behind it, 1 to 3 of 1,000,000 calls return -ENOENT, and 50 to 110 with IORING_SETUP_DEFER_TASKRUN (4 CPUs, KVM). Nothing is written to the pipe, and all of these reads end with -ECANCELED. Remember that the work was found. IORING_OP_ASYNC_CANCEL completes with -EALREADY, as it does if the worker is a bit slower. For IORING_REGISTER_SYNC_CANCEL it means to try again, which ends with 0. The result of a linked timeout, which calls io_try_cancel() by itself, and the number that is returned for IORING_ASYNC_CANCEL_ALL and _ANY don't change. Fixes: ccbf726171b7 ("io_uring: perform poll removal even if async work removal is successful") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/cancel.c | 13 ++++++++++++- io_uring/cancel.h | 2 ++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/io_uring/cancel.c b/io_uring/cancel.c index 64eb4a993c00..290ce804c868 100644 --- a/io_uring/cancel.c +++ b/io_uring/cancel.c @@ -117,6 +117,8 @@ int io_try_cancel(struct io_uring_task *tctx, struct io_cancel_data *cd, */ if (!ret) return 0; + if (ret == -EALREADY) + cd->running = true; ret = io_poll_cancel(ctx, cd, issue_flags); if (ret != -ENOENT) @@ -180,6 +182,7 @@ static int __io_async_cancel(struct io_cancel_data *cd, struct io_tctx_node *node; int ret, nr = 0; + cd->running = false; do { ret = io_try_cancel(tctx, cd, issue_flags); if (ret == -ENOENT) @@ -204,7 +207,15 @@ static int __io_async_cancel(struct io_cancel_data *cd, } mutex_unlock(&ctx->tctx_lock); io_ring_submit_unlock(ctx, issue_flags); - return all ? nr : ret; + if (all) + return nr; + /* + * The worker may be done with the work that was found running, and + * flagged, above. That isn't the same as not having found it. + */ + if (ret == -ENOENT && cd->running) + ret = -EALREADY; + return ret; } int io_async_cancel(struct io_kiocb *req, unsigned int issue_flags) diff --git a/io_uring/cancel.h b/io_uring/cancel.h index e49713a1bc66..484fa5e7ffaa 100644 --- a/io_uring/cancel.h +++ b/io_uring/cancel.h @@ -11,6 +11,8 @@ struct io_cancel_data { u8 opcode; u32 flags; int seq; + /* io_try_cancel() has found it being run by io-wq, and flagged it */ + bool running; }; int io_async_cancel_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe); -- 2.55.0 IORING_REGISTER_SYNC_CANCEL waits without a limit if both fields of io_uring_sync_cancel_reg::timeout are -1. io_sync_cancel() compares them with -1UL. They are of type long long. With a long of 32 bits -1UL is 4294967295, which is what it stays when it is converted for the comparison, and the fields are never equal to it. So { -1, -1 } is taken for a timeout. ktime_set() takes the nanoseconds as an unsigned long, so what becomes of it is -1s + 4294967295ns = 3.29s. If the request is being run by io-wq and can't be interrupted, the call returns -ETIME after that time, rather than waiting for the request to be gone. On i386, with a buffered read from a null_blk device that takes 4s to complete a request, IOSQE_ASYNC, that is what each call does. Fixes: 78a861b94959 ("io_uring: add sync cancelation API through io_uring_register()") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/cancel.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/io_uring/cancel.c b/io_uring/cancel.c index 290ce804c868..14d6ccf57668 100644 --- a/io_uring/cancel.c +++ b/io_uring/cancel.c @@ -319,7 +319,7 @@ int io_sync_cancel(struct io_ring_ctx *ctx, void __user *arg) if (ret != -EALREADY) goto out; - if (sc.timeout.tv_sec != -1UL || sc.timeout.tv_nsec != -1UL) { + if (sc.timeout.tv_sec != -1 || sc.timeout.tv_nsec != -1) { struct timespec64 ts = { .tv_sec = sc.timeout.tv_sec, .tv_nsec = sc.timeout.tv_nsec -- 2.55.0 A cancellation with IORING_ASYNC_CANCEL_ALL or _ANY goes on until it finds nothing more. A request may stay where it was found for a while after it has been cancelled, so each one that matches is stamped with the sequence number of the cancellation, and is skipped from then on. io_cancel_cb() does so for io-wq work as well. There is no need to: pending work is taken off the list, and the workers are looked at once per call. And it does harm. A request that is run by io-wq may be parked while it still is the current work of the worker: poll gets armed for it, or it is put on ->futex_list or ->waitid_list. io-wq worker io_try_cancel() io_wq_submit_work() io_arm_poll_handler() io_poll_add_hash() io_async_cancel_one() io_cancel_cb() stamps the request sets IO_WQ_WORK_CANCEL = -EALREADY io_poll_cancel() skips the request = -ENOENT io_assign_current_work(NULL) The flag comes too late for the worker to see it, and the search that would have found the request takes it for done. The cancellation returns, and the request stays. With a multishot poll, IOSQE_ASYNC, and IORING_OP_ASYNC_CANCEL with IORING_ASYNC_CANCEL_ALL up to 63us later, that happens to 4 of 1,000 requests (4 CPUs, KVM). Take the stamp off again if the work matches. If the request is found on one of the lists it gets a new one, and io_cancel_cb() doesn't match it a second time. __io_async_cancel() tries the io-wq of the caller twice. So far the stamp has kept it from counting work that is running there the second time. Skip it instead, so that the number that is returned stays the same. Fixes: 8e29da69fead ("io_uring: add support for IORING_ASYNC_CANCEL_ALL") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/cancel.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/io_uring/cancel.c b/io_uring/cancel.c index 14d6ccf57668..6f7a6cf356c2 100644 --- a/io_uring/cancel.c +++ b/io_uring/cancel.c @@ -72,7 +72,16 @@ static bool io_cancel_cb(struct io_wq_work *work, void *data) struct io_kiocb *req = container_of(work, struct io_kiocb, work); struct io_cancel_data *cd = data; - return io_cancel_req_match(req, cd); + if (!io_cancel_req_match(req, cd)) + return false; + /* + * Pending work is gone once it has matched. Work that is running may + * park the request where the same cancellation looks next. Don't make + * it skip the request as one that it has dealt with. + */ + if (cd->flags & (IORING_ASYNC_CANCEL_ALL | IORING_ASYNC_CANCEL_ANY)) + req->cancel_seq_set = false; + return true; } static int io_async_cancel_one(struct io_uring_task *tctx, @@ -198,6 +207,9 @@ static int __io_async_cancel(struct io_cancel_data *cd, mutex_lock(&ctx->tctx_lock); ret = -ENOENT; list_for_each_entry(node, &ctx->tctx_list, ctx_node) { + /* has been done above, don't count what is running there */ + if (all && node->task->io_uring == tctx) + continue; ret = io_async_cancel_one(node->task->io_uring, cd); if (ret != -ENOENT) { if (!all) -- 2.55.0 With IORING_SETUP_DEFER_TASKRUN completions are queued as local work, and only become CQEs when the submitter enters the kernel to wait for events. For an application that finds out via poll() or epoll on the ring fd when it is time to do that, io_uring_poll() reports EPOLLIN if local work is pending. That only helps if the work is there at the time of the call. When DEFER_TASKRUN was added, io_req_local_work_add() did io_cqring_wake(), which took care of tasks sleeping in poll() as well. Then polling got a wait queue of its own, ->poll_wq, and the wake up in io_req_local_work_add() was replaced by waking the submitter directly. Since then ->poll_wq is only woken when CQEs are committed, which for such a ring is done by the very task that is waiting to be told to: submitter completion poll(ring_fd) io_uring_poll() // nothing yet schedule() io_req_local_work_add() // ->cq_wait_nr: nobody waits // sleeps on Wake ->poll_wq when the first entry is put on the work list. Like for the eventfd, that must not be done right away if the work is queued from a wake up handler, as the chain of wake ups could get back to the wait queue whose lock is held. Use an RCU callback in that case, with a reference to the ring. Fixes: 130bd686d9be ("io_uring: waitqueue-less cq waiting") Assisted-by: LLM Signed-off-by: Josef Bacik --- include/linux/io_uring_types.h | 3 +++ io_uring/tw.c | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h index 50970b382a9a..3e00012cbfd4 100644 --- a/include/linux/io_uring_types.h +++ b/include/linux/io_uring_types.h @@ -520,6 +520,9 @@ struct io_ring_ctx { u32 iowq_limits[2]; struct callback_head poll_wq_task_work; + /* deferred ->poll_wq wake up for local task_work */ + struct rcu_head poll_wq_rcu; + atomic_t poll_wq_rcu_pending; struct list_head defer_list; unsigned nr_drained; diff --git a/io_uring/tw.c b/io_uring/tw.c index 98e3b1bc61b4..0b5066f97224 100644 --- a/io_uring/tw.c +++ b/io_uring/tw.c @@ -148,6 +148,37 @@ static void io_ctx_mark_taskrun(struct io_ring_ctx *ctx) } } +static void io_poll_wq_wake_rcu(struct rcu_head *rcu) +{ + struct io_ring_ctx *ctx = container_of(rcu, struct io_ring_ctx, + poll_wq_rcu); + + atomic_set(&ctx->poll_wq_rcu_pending, 0); + io_poll_wq_wake(ctx); + percpu_ref_put(&ctx->refs); +} + +/* + * io_uring_poll() reports pending local work as EPOLLIN, as it takes a trip to + * the kernel to turn it into CQEs. Let those know who are asleep already. + */ +static void io_local_work_wake_poll(struct io_ring_ctx *ctx, unsigned int flags) +{ + if (!(flags & IOU_F_TWQ_IN_WAKE)) { + io_poll_wq_wake(ctx); + return; + } + /* + * Called from a wake up handler with some wait queue lock held, this + * one could lead back to it. Do it from a clean context. + */ + if (!wq_has_sleeper(&ctx->poll_wq) || + atomic_xchg(&ctx->poll_wq_rcu_pending, 1)) + return; + percpu_ref_get(&ctx->refs); + call_rcu_hurry(&ctx->poll_wq_rcu, io_poll_wq_wake_rcu); +} + void io_req_local_work_add(struct io_kiocb *req, unsigned flags) { struct io_ring_ctx *ctx = req->ctx; @@ -174,6 +205,9 @@ void io_req_local_work_add(struct io_kiocb *req, unsigned flags) io_ctx_mark_taskrun(ctx); if (data_race(ctx->int_flags) & IO_RING_F_HAS_EVFD) io_eventfd_signal(ctx, false, flags & IOU_F_TWQ_IN_WAKE); + /* whoever sets the flag wakes ->poll_wq afterwards */ + if (data_race(ctx->int_flags) & IO_RING_F_POLL_ACTIVATED) + io_local_work_wake_poll(ctx, flags); } /* -- 2.55.0 io_wake_function() returns -1 if the waiter it is called for doesn't have enough events yet, which makes __wake_up_common() stop walking the wait queue. That assumes that the waiters behind it can't be satisfied either. Each waiter has its own target though, computed from the min_complete it passed in: thread A: io_uring_enter(GETEVENTS, min_complete = 10) // queued first thread B: io_uring_enter(GETEVENTS, min_complete = 1) one CQE is posted: A isn't woken, and B isn't even considered B keeps sleeping with the event it asked for sitting in the CQ ring, until A's demand is met or a timeout or signal ends the wait. Return 0 to carry on with the next waiter. Only waiters whose condition is met are woken, like before. Fixes: bda521624e75 ("io_uring: make CQ ring wakeups be more efficient") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/wait.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/io_uring/wait.c b/io_uring/wait.c index 5093365e9eb2..38425d4cb7bc 100644 --- a/io_uring/wait.c +++ b/io_uring/wait.c @@ -26,7 +26,8 @@ static int io_wake_function(struct wait_queue_entry *curr, unsigned int mode, */ if (io_should_wake(iowq) || io_has_work(iowq->ctx)) return autoremove_wake_function(curr, mode, wake_flags, key); - return -1; + /* keep going, a waiter further down may be content with less */ + return 0; } int io_run_task_work_sig(struct io_ring_ctx *ctx) -- 2.55.0 A file that uses two wait queues for poll, e.g. a tty, gets a second io_poll entry, which IORING_OP_POLL_ADD keeps in ->async_data until the request is freed. io_poll_add() can run more than once for a request though: IORING_OP_POLL_REMOVE with one of the update flags disarms and rearms it, and io_poll_task_func() reissues it if a wake up turns out to be spurious. The second poll_wait() of the new round then finds *poll_ptr set, with ->head cleared by the disarm, and __io_queue_proc() fails what it takes for a third wait queue with -EINVAL. For an update that means that the poll request is completed with -ECANCELED, while the update itself reports success. For a spurious wake up, e.g. tcsetattr() on the polled tty, the poll request fails with -EINVAL. Free the old entry and clear the poll flags before arming, which is what io_arm_apoll() does for internal polling. All entries have been removed from their wait queues at that point. Fixes: b69de288e913 ("io_uring: allow events and user_data update of running poll requests") Fixes: 6e5aedb9324a ("io_uring/poll: attempt request issue after racy poll wakeup") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/poll.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/io_uring/poll.c b/io_uring/poll.c index 577ae978bfc9..7dcb904e8fc3 100644 --- a/io_uring/poll.c +++ b/io_uring/poll.c @@ -918,6 +918,15 @@ int io_poll_add(struct io_kiocb *req, unsigned int issue_flags) ipt.pt._qproc = io_poll_queue_proc; + /* + * A poll update or a spurious wake up arms the request again. Forget + * the entries of the previous round like io_arm_apoll() does, a stale + * second entry would be taken for a third wait queue. + */ + if (req_has_async_data(req)) + io_req_async_data_free(req); + req->flags &= ~(REQ_F_SINGLE_POLL | REQ_F_DOUBLE_POLL); + ret = __io_arm_poll_handler(req, poll, &ipt, poll->events, issue_flags); if (ret > 0) { io_req_set_res(req, ipt.result_mask, 0); -- 2.55.0 The futex code relies on there being no private futex waiters by the time the first private hash of a mm is installed: __futex_pivot_hash() moves the waiters from the previous private hash to the new one, but it does not look for them in the global hash which is used as long as there is no private hash. That is why every clone(CLONE_VM) allocates it up front: a task which is alone in its mm cannot block in futex_wait() and create a thread or invoke prctl(PR_FUTEX_HASH) at the same time. IORING_OP_FUTEX_WAIT and IORING_OP_FUTEX_WAITV do not block the submitter though: io_uring_enter() io_futex_wait() futex_wait_setup() // no private hash: queued in the global hash pthread_create() futex_hash_allocate_default() __futex_pivot_hash() // first private hash, nothing is moved futex(FUTEX_WAKE_PRIVATE) futex_hash() // private hash: no waiter The wakeup is lost and the request only completes when it is cancelled. prctl(PR_FUTEX_HASH, PR_FUTEX_HASH_SET_SLOTS, n) instead of pthread_create() has the same effect. Allocate the default private hash when a wait request on a private futex is prepared, which happens in the context of the submitter. This is a RCU protected pointer check once the hash exists. A failure to allocate it fails the request with -ENOMEM. Fixes: 80367ad01d93 ("futex: Add basic infrastructure for local task local hash") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/futex.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/io_uring/futex.c b/io_uring/futex.c index 64909a2a8c88..9203998654ff 100644 --- a/io_uring/futex.c +++ b/io_uring/futex.c @@ -152,6 +152,24 @@ int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) return 0; } +/* + * A wait on a private futex does not block the submitter. Even a single + * threaded task can go on and have the mm private hash installed by clone() + * or prctl(), which does not pick up waiters from the global hash. Make + * sure the private hash exists before the first waiter is queued. + */ +static int io_futex_prep_private(struct io_kiocb *req) +{ + int ret = futex_hash_allocate_default(); + + if (unlikely(ret)) + return ret; + + /* inflight tracking only needed for mm private hash */ + io_req_track_inflight(req); + return 0; +} + int io_futex_wait_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) { struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex); @@ -161,9 +179,8 @@ int io_futex_wait_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) if (unlikely(ret)) return ret; - /* inflight tracking only needed for mm private hash */ if (!(iof->futex_flags & FLAGS_SHARED)) - io_req_track_inflight(req); + return io_futex_prep_private(req); return 0; } @@ -213,10 +230,13 @@ int io_futexv_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) return ret; } - /* inflight tracking only needed for mm private hash */ for (i = 0; i < iof->futex_nr; i++) { if (!(ifd->futexv[i].w.flags & FLAGS_SHARED)) { - io_req_track_inflight(req); + ret = io_futex_prep_private(req); + if (ret) { + kfree(ifd); + return ret; + } break; } } -- 2.55.0 ->cqe_cached and ->cqe_sentinel delimit the CQ entries which can be handed out without looking at the ring. io_get_cqe_overflow() only calls io_cqe_cache_refill() if that range is too small. Without IORING_SETUP_CQE_MIXED too small means empty. With it, a 32b CQE needs two entries, and one may be left. If the refill then fails, the entry stays in the cache, and the next 16b CQE gets it without any checks. That CQE overtakes the 32b one, which went to the overflow list. The test for IO_CHECK_CQ_OVERFLOW_BIT, which is there to prevent that, is part of the refill: CQ ring with 4 entries, head == 0 16b CQE, 32b CQE tail == 3, one entry cached 32b CQE no room, the CQE overflows 16b CQE written to entry 3, ahead of the 32b CQE Forget about the rest of the cache when it is refilled. Fixes: e26dca67fde1 ("io_uring: add support for IORING_SETUP_CQE_MIXED") Reported-by: Leonid Ovchinnikov Closes: https://lore.kernel.org/io-uring/CAECZYD5OMD9vMTzjteFMMP40Dn+SWKj9SFj=43U6uO3XMnxJbw@mail.gmail.com/ Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/io_uring.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c index a5fd1845a1d5..01187f00943a 100644 --- a/io_uring/io_uring.c +++ b/io_uring/io_uring.c @@ -729,6 +729,13 @@ bool io_cqe_cache_refill(struct io_ring_ctx *ctx, bool overflow, bool cqe32) if (!overflow && (ctx->check_cq & BIT(IO_CHECK_CQ_OVERFLOW_BIT))) return false; + /* + * A single entry may be left if a 32b CQE is asked for. It must not be + * handed out later on if this fails, as the CQE is going to overflow. + */ + ctx->cqe_cached = NULL; + ctx->cqe_sentinel = NULL; + /* * Post dummy CQE if a 32b CQE is needed and there's only room for a * 16b CQE before the ring wraps. -- 2.55.0 If an SQPOLL thread serves more than one ring, __io_sq_thread() submits no more than IORING_SQPOLL_CAP_ENTRIES_VALUE entries per ring and round, for fairness. On a ring with IORING_SETUP_SQE_MIXED the cap may fall between the two entries of a 128b SQE. io_init_req() finds that just one entry is left of what it was asked to submit and fails the request with -EINVAL, although the second entry is sitting in the SQ ring. In the next round the second entry is taken for an SQE of its own, so on top of the failed request the application gets whatever its payload amounts to when it is read as an SQE. The application has done nothing wrong, and it can't know where the cap is going to be applied. For SQPOLL, let the request have its second entry if the SQ ring holds it, in excess of the number of entries that were to be submitted. Fixes: 1cba30bf9fdd ("io_uring: add support for IORING_SETUP_SQE_MIXED") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/io_uring.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c index 01187f00943a..7092efc22dd2 100644 --- a/io_uring/io_uring.c +++ b/io_uring/io_uring.c @@ -1818,17 +1818,27 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req, * A 128b op on a non-128b SQ requires mixed SQE support as * well as 2 contiguous entries. */ - if (!(ctx->flags & IORING_SETUP_SQE_MIXED) || *left < 2 || + if (!(ctx->flags & IORING_SETUP_SQE_MIXED) || (unsigned)(sqe - ctx->sq_sqes) >= ctx->sq_entries - 1) return io_init_fail_req(req, -EINVAL); + if (*left >= 2) { + /* + * A 128b operation on a mixed SQ uses two entries, so + * we have to increment the head and cached refs, and + * decrement what's left. + */ + current->io_uring->cached_refs++; + (*left)--; + } else if (!(ctx->flags & IORING_SETUP_SQPOLL) || + !__io_sqring_entries(ctx)) { + return io_init_fail_req(req, -EINVAL); + } /* - * A 128b operation on a mixed SQ uses two entries, so we have - * to increment the head and cached refs, and decrement what's - * left. + * else: the SQPOLL thread caps the number of entries it submits + * in one go, which is none of the application's business. Take + * the 2nd entry on top. */ - current->io_uring->cached_refs++; ctx->cached_sq_head++; - (*left)--; } if (unlikely(sqe_flags & ~SQE_COMMON_FLAGS)) { -- 2.55.0 On an IORING_SETUP_CQE_MIXED ring, a 32b CQE uses two contiguous CQ entries and is never allowed to wrap the end of the ring. When posting, io_cqe_cache_refill() inserts a dummy IORING_CQE_F_SKIP entry if only one entry is left before the ring wraps, and the application relies on that to read a 32b CQE as one contiguous 32 byte slot. io_register_resize_rings() copies pending CQEs to the new ring one 16b entry at a time, placing entry i at i & (new_entries - 1). When the CQ ring is shrunk, a pending 32b CQE that was contiguous in the old ring can land with its first half on the last entry of the new ring and its second half on entry 0. For example, with 8 old entries, 4 new entries and a 32b CQE at index 3, the halves end up at entries 3 and 0. The application then reads the extra data of that CQE from past the end of the CQ array, and the CQE content it sees is wrong. This was found by code inspection and has not been reproduced. Fix by failing the resize with -EOVERFLOW if a pending 32b CQE would wrap the new ring, the same way the resize fails when the pending CQEs don't fit. Fixes: e26dca67fde1 ("io_uring: add support for IORING_SETUP_CQE_MIXED") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/register.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/io_uring/register.c b/io_uring/register.c index dab3eb4258df..d7e9963ab9b0 100644 --- a/io_uring/register.c +++ b/io_uring/register.c @@ -505,6 +505,7 @@ static int io_register_resize_rings(struct io_ring_ctx *ctx, void __user *arg) struct io_uring_params *p = &config.p; struct io_rings_layout *rl = &config.layout; u32 *o_sq_array, *n_sq_array = NULL; + bool cqe32_tail = false; int ret; memset(&config, 0, sizeof(config)); @@ -654,6 +655,21 @@ static int io_register_resize_rings(struct io_ring_ctx *ctx, void __user *arg) src_mask = (ctx->cq_entries << 1) - 1; dst_mask = (p->cq_entries << 1) - 1; } + /* + * A 32b CQE on a CQE_MIXED ring takes two contiguous entries + * and must not wrap. A smaller ring may place it on the last + * entry, fail the resize if that happens. + */ + if (ctx->flags & IORING_SETUP_CQE_MIXED) { + if (cqe32_tail) { + cqe32_tail = false; + } else if (READ_ONCE(o.rings->cqes[index & src_mask].flags) & + IORING_CQE_F_32) { + if ((index & dst_mask) == dst_mask) + goto overflow; + cqe32_tail = true; + } + } memcpy(&n.rings->cqes[index & dst_mask], &o.rings->cqes[index & src_mask], cq_size); } WRITE_ONCE(n.rings->cq.head, old_head); -- 2.55.0 The ring heads and tails are free running 32-bit counters. io_register_resize_rings() takes that into account when it checks whether the pending entries fit into the new rings, but then copies them with for (i = old_head; i < tail; i++) If the tail has wrapped and the head hasn't, that copies nothing. The new rings get the old head and tail, so the application finds whatever is in the freshly allocated memory, zeroes, in place of the CQEs it hasn't reaped yet, or the kernel in place of the SQEs that have not been submitted yet. A ring that completes a million requests per second gets there after little more than an hour. Loop until the tail is reached, the number of iterations has been bounded by the size of the new ring at that point. Fixes: 79cfe9e59c2a ("io_uring/register: add IORING_REGISTER_RESIZE_RINGS") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/register.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/io_uring/register.c b/io_uring/register.c index d7e9963ab9b0..bce4473dc2dd 100644 --- a/io_uring/register.c +++ b/io_uring/register.c @@ -604,7 +604,7 @@ static int io_register_resize_rings(struct io_ring_ctx *ctx, void __user *arg) old_head = READ_ONCE(o.rings->sq.head); if (tail - old_head > p->sq_entries) goto overflow; - for (i = old_head; i < tail; i++) { + for (i = old_head; i != tail; i++) { unsigned int dst, src; size_t sq_size; @@ -641,7 +641,7 @@ static int io_register_resize_rings(struct io_ring_ctx *ctx, void __user *arg) ret = -EOVERFLOW; goto out; } - for (i = old_head; i < tail; i++) { + for (i = old_head; i != tail; i++) { unsigned index, dst_mask, src_mask; size_t cq_size; -- 2.55.0 On a ring with IORING_SETUP_SQE_MIXED a 128b SQE takes two entries that have to be contiguous in memory, io_init_req() fails it with -EINVAL if it starts on the last entry of the SQ ring. The entry a pending SQE ends up in after IORING_REGISTER_RESIZE_RINGS depends on the size of the new ring, and nothing takes care of that. With four entries instead of eight, a 128b SQE which was fine in entries 3 and 4 is split up between entry 3 and entry 0. The resize succeeds, and the request fails later on. If the ring has an SQ array, the second half is also taken from the wrong place unless the array happens to map the following index to the following entry. The submission side doesn't look at the array for the second half. Do what is done for 32b CQEs on IORING_SETUP_CQE_MIXED rings: fail the resize with -EOVERFLOW if a 128b SQE would wrap. And copy the second half from where it is. Fixes: 1cba30bf9fdd ("io_uring: add support for IORING_SETUP_SQE_MIXED") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/register.c | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/io_uring/register.c b/io_uring/register.c index bce4473dc2dd..2ce36623e25a 100644 --- a/io_uring/register.c +++ b/io_uring/register.c @@ -496,6 +496,17 @@ static void io_register_free_rings(struct io_ring_ctx *ctx, IORING_SETUP_CQE32 | IORING_SETUP_NO_MMAP | \ IORING_SETUP_CQE_MIXED | IORING_SETUP_SQE_MIXED) +static bool io_sqe_is_128(struct io_ring_ctx *ctx, + const struct io_uring_sqe *sqe) +{ + u8 opcode = READ_ONCE(sqe->opcode); + + if (!(ctx->flags & IORING_SETUP_SQE_MIXED) || opcode >= IORING_OP_LAST) + return false; + opcode = array_index_nospec(opcode, IORING_OP_LAST); + return io_issue_defs[opcode].is_128; +} + static int io_register_resize_rings(struct io_ring_ctx *ctx, void __user *arg) { struct io_ctx_config config; @@ -506,6 +517,7 @@ static int io_register_resize_rings(struct io_ring_ctx *ctx, void __user *arg) struct io_rings_layout *rl = &config.layout; u32 *o_sq_array, *n_sq_array = NULL; bool cqe32_tail = false; + unsigned int sqe128_src = 0; int ret; memset(&config, 0, sizeof(config)); @@ -605,12 +617,19 @@ static int io_register_resize_rings(struct io_ring_ctx *ctx, void __user *arg) if (tail - old_head > p->sq_entries) goto overflow; for (i = old_head; i != tail; i++) { + bool sqe128_tail = sqe128_src; unsigned int dst, src; size_t sq_size; dst = i & (p->sq_entries - 1); src = i & (ctx->sq_entries - 1); - if (n_sq_array) { + if (sqe128_tail) { + /* it follows the first half, whatever the array says */ + src = sqe128_src; + sqe128_src = 0; + if (n_sq_array) + WRITE_ONCE(n_sq_array[dst], dst); + } else if (n_sq_array) { src = READ_ONCE(o_sq_array[src]); if (unlikely(src >= ctx->sq_entries)) { WRITE_ONCE(n_sq_array[dst], UINT_MAX); @@ -619,6 +638,19 @@ static int io_register_resize_rings(struct io_ring_ctx *ctx, void __user *arg) WRITE_ONCE(n_sq_array[dst], dst); } + /* + * A 128b SQE on a SQE_MIXED ring takes two contiguous entries + * and must not wrap. A smaller ring may place it on the last + * entry, fail the resize if that happens. + */ + if (!sqe128_tail && i + 1 != tail && + src < ctx->sq_entries - 1 && + io_sqe_is_128(ctx, &o.sq_sqes[src])) { + if (dst == p->sq_entries - 1) + goto overflow; + sqe128_src = src + 1; + } + sq_size = sizeof(struct io_uring_sqe); if (ctx->flags & IORING_SETUP_SQE128) { dst <<= 1; -- 2.55.0 Since the CQ ring dump was changed to deal with IORING_SETUP_CQE_MIXED, a 32b CQE is taken to occupy two entries of the ring. That is how it works for mixed rings. With IORING_SETUP_CQE32 each entry has twice the size: head and tail count 32b CQEs, and entry n is found at cqes[n << 1]. For such rings the dump reads cqes[n], and skips every other entry. It shows half of the pending CQEs at best, and what it shows is taken from the wrong place unless the head happens to be at the start of the ring. The extra data of the last entry isn't shown at all, as the check that keeps the dump of a mixed ring from reading beyond the array is applied to these rings as well. Fixes: 82ceb7fcc5ff ("io_uring/fdinfo: handle mixed sized CQEs") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/fdinfo.c | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/io_uring/fdinfo.c b/io_uring/fdinfo.c index c362f3a81bdc..46e64b041360 100644 --- a/io_uring/fdinfo.c +++ b/io_uring/fdinfo.c @@ -154,20 +154,26 @@ static void __io_uring_show_fdinfo(struct io_ring_ctx *ctx, struct seq_file *m) cq_entries = min(cq_tail - cq_head, ctx->cq_entries); for (i = 0; i < cq_entries; i++) { struct io_uring_cqe *cqe; - bool cqe32 = false; + bool cqe32 = false, two_entries = false; bool is_last_cqarray_slot = (cq_head & cq_mask) == cq_mask; - cqe = &r->cqes[(cq_head & cq_mask)]; - /* - * Userspace can manipulate the last cqarray slot to have - * IORING_CQE_F_32 set, which would cause the second half of - * that CQE to be read out of bounds. - * Ignore the flag for the last cqarray slot, which can't - * legitimately have the flag set. - */ - if ((cqe->flags & IORING_CQE_F_32 || ctx->flags & IORING_SETUP_CQE32) && - !is_last_cqarray_slot) + if (ctx->flags & IORING_SETUP_CQE32) { + /* all entries have twice the size */ + cqe = &r->cqes[(cq_head & cq_mask) << 1]; cqe32 = true; + } else { + cqe = &r->cqes[(cq_head & cq_mask)]; + /* + * Userspace can manipulate the last cqarray slot to + * have IORING_CQE_F_32 set, which would cause the second + * half of that CQE to be read out of bounds. + * Ignore the flag for the last cqarray slot, which can't + * legitimately have the flag set. + */ + two_entries = (cqe->flags & IORING_CQE_F_32) && + !is_last_cqarray_slot; + cqe32 = two_entries; + } seq_printf(m, "%5u: user_data:%llu, res:%d, flags:%x", cq_head & cq_mask, cqe->user_data, cqe->res, cqe->flags); @@ -176,7 +182,7 @@ static void __io_uring_show_fdinfo(struct io_ring_ctx *ctx, struct seq_file *m) cqe->big_cqe[0], cqe->big_cqe[1]); seq_printf(m, "\n"); cq_head++; - if (cqe32) { + if (two_entries) { cq_head++; i++; } -- 2.55.0 io_flush_timeouts() copes with the wrap of the CQ sequence by looking at the distance from ->cq_last_tm_flush. That relies on the target of each pending timeout being at or ahead of the last flush. io_timeout() breaks that. It sets ->cq_last_tm_flush to the current sequence, on the assumption that nothing has been posted since the last flush. That isn't true. A multishot request posts its CQEs right when it is issued or retried, but they are committed, and the timeouts flushed, at the end of the submission or task_work run: X: IORING_OP_TIMEOUT, off == 1 target_seq == N + 1 io_submit_sqes() multishot recv, posts two CQEs sequence == N + 2 Y: IORING_OP_TIMEOUT, off == 100 ->cq_last_tm_flush = N + 2 io_flush_timeouts() X: events_needed = N + 1 - (N + 2) // 0xffffffff events_got = 0 // not yet X isn't completed although the completions it was waiting for have happened, only its timer can end it now. And as the list is sorted and the walk ends at the first timeout that isn't due, it gets in the way of the ones behind it. The insertion sort has the same problem, it measures the distance of the pending targets from the current sequence. ->cq_last_tm_flush only needs to be brought up to date if no timeout is waiting for a sequence, as io_flush_timeouts() isn't called before the first one is set up. Otherwise it is current enough. Timeouts with a sequence come first in the list, so looking at the first entry will do. Use the last flush as the base for the insertion sort as well. Fixes: f010505b78a4 ("io_uring: flush timeouts that should already have expired") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/timeout.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/io_uring/timeout.c b/io_uring/timeout.c index 9c239c0a1715..8b65ba20e143 100644 --- a/io_uring/timeout.c +++ b/io_uring/timeout.c @@ -645,8 +645,9 @@ int io_timeout(struct io_kiocb *req, unsigned int issue_flags) struct io_timeout *timeout = io_kiocb_to_cmd(req, struct io_timeout); struct io_ring_ctx *ctx = req->ctx; struct io_timeout_data *data = req->async_data; + struct io_timeout *first; struct list_head *entry; - u32 tail, off = timeout->off; + u32 tail, base, off = timeout->off; raw_spin_lock_irq(&ctx->timeout_lock); @@ -663,11 +664,17 @@ int io_timeout(struct io_kiocb *req, unsigned int issue_flags) tail = data_race(ctx->cached_cq_tail) - atomic_read(&ctx->cq_timeouts); timeout->target_seq = tail + off; - /* Update the last seq here in case io_flush_timeouts() hasn't. - * This is safe because ->completion_lock is held, and submissions - * and completions are never mixed in the same ->completion_lock section. + /* + * Update the last seq here in case io_flush_timeouts() hasn't, it only + * gets called once sequences are in use. Leave it alone if there are + * timeouts waiting for one. CQEs may have been posted since the last + * flush, and a target that has been reached must not fall behind it. */ - ctx->cq_last_tm_flush = tail; + first = list_first_entry_or_null(&ctx->timeout_list, struct io_timeout, + list); + if (!first || io_is_timeout_noseq(cmd_to_io_kiocb(first))) + ctx->cq_last_tm_flush = tail; + base = ctx->cq_last_tm_flush; /* * Insertion sort, ensuring the first entry in the list is always @@ -679,8 +686,8 @@ int io_timeout(struct io_kiocb *req, unsigned int issue_flags) if (io_is_timeout_noseq(nxt)) continue; - /* nxt.seq is behind @tail, otherwise would've been completed */ - if (off >= nextt->target_seq - tail) + /* no target is behind the last flush, see io_flush_timeouts() */ + if (timeout->target_seq - base >= nextt->target_seq - base) break; } add: -- 2.55.0 IORING_TIMEOUT_UPDATE clears ->off of the timeout it updates, to turn a timeout that waits for a number of completions into a pure one. For a multishot timeout ->off isn't a sequence though, it tells whether the number of repeats is limited: io_timeout_finish() only looks at ->repeats if ->off is set. A multishot timeout that was set up to fire N times hence fires forever once its interval has been updated. Leave ->off alone for those, they are never treated as sequence timeouts anyway. Fixes: ea97f6c8558e ("io_uring: add support for multishot timeouts") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/timeout.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/io_uring/timeout.c b/io_uring/timeout.c index 8b65ba20e143..dbcd0242d8f6 100644 --- a/io_uring/timeout.c +++ b/io_uring/timeout.c @@ -475,8 +475,10 @@ static int io_timeout_update(struct io_ring_ctx *ctx, __u64 user_data, if (IS_ERR(req)) return PTR_ERR(req); - timeout->off = 0; /* noseq */ data = req->async_data; + /* for a multishot request this isn't a sequence, keep the repeats */ + if (!(data->flags & IORING_TIMEOUT_MULTISHOT)) + timeout->off = 0; /* noseq */ data->time = time; list_add_tail(&timeout->list, &ctx->timeout_list); -- 2.55.0 io_parse_user_time() converts an absolute time from the time namespace of the caller to the host's view, for the clock given by the flags. The flags of IORING_TIMEOUT_UPDATE and IORING_LINK_TIMEOUT_UPDATE can't name a clock, the updated timer keeps running on the clock the timeout was set up with. So the time is always converted as if it was CLOCK_MONOTONIC. In a time namespace with a monotonic offset, that moves the new expiry of a CLOCK_REALTIME timeout by that offset, although the clock isn't affected by time namespaces at all, and the one of a CLOCK_BOOTTIME timeout by the difference between the two offsets. Do the conversion when the timeout has been looked up and its clock is known. The update is run by the submitter, its SQPOLL thread or one of its io-wq workers, all of which share the time namespace. Fixes: 9cc6bac1bebf ("io_uring/timeout: honour caller's time namespace for IORING_TIMEOUT_ABS") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/timeout.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/io_uring/timeout.c b/io_uring/timeout.c index dbcd0242d8f6..90d440a75f28 100644 --- a/io_uring/timeout.c +++ b/io_uring/timeout.c @@ -458,6 +458,8 @@ static int io_linked_timeout_update(struct io_ring_ctx *ctx, __u64 user_data, io = req->async_data; if (hrtimer_try_to_cancel(&io->timer) == -1) return -EALREADY; + if (mode == HRTIMER_MODE_ABS) + ts = timens_ktime_to_host(io_timeout_get_clock(io), ts); hrtimer_setup(&io->timer, io_link_timeout_fn, io_timeout_get_clock(io), mode); hrtimer_start(&io->timer, ts, mode); return 0; @@ -479,6 +481,8 @@ static int io_timeout_update(struct io_ring_ctx *ctx, __u64 user_data, /* for a multishot request this isn't a sequence, keep the repeats */ if (!(data->flags & IORING_TIMEOUT_MULTISHOT)) timeout->off = 0; /* noseq */ + if (mode == HRTIMER_MODE_ABS) + time = timens_ktime_to_host(io_timeout_get_clock(data), time); data->time = time; list_add_tail(&timeout->list, &ctx->timeout_list); @@ -511,7 +515,12 @@ int io_timeout_remove_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) IORING_TIMEOUT_ABS | IORING_TIMEOUT_IMMEDIATE_ARG)) return -EINVAL; - ret = io_parse_user_time(&tr->time, READ_ONCE(sqe->addr2), tr->flags); + /* + * The clock is that of the timeout to update, an absolute time + * is converted to the host's view once that has been found. + */ + ret = io_parse_user_time(&tr->time, READ_ONCE(sqe->addr2), + tr->flags & ~IORING_TIMEOUT_ABS); if (ret) return ret; } else if (tr->flags) { -- 2.55.0 When the min wait time is over and there is work to run, which for DEFER_TASKRUN includes task_work that was queued lazily and didn't wake the waiter, io_cqring_min_timer_wakeup() ends the wait via io_cqring_timer_wakeup(). That flags it as timed out. io_cqring_wait() runs the work, leaves the loop, and returns 0 if the CQ ring holds something. Running task_work doesn't have to result in a CQE, though. A poll wake up can turn out to be spurious, for instance if somebody else has read the data by the time the request is retried. The application then gets -ETIME once the min wait time has passed, no matter how far away the timeout it asked for is. Wake the task without declaring a timeout. From now on a single CQE satisfies the wait, like in the case where the timer is switched over to the normal timeout. If running the work doesn't post one, the loop in io_cqring_wait() goes back to sleep with the normal timeout. Fixes: 1100c4a2656d ("io_uring: add support for batch wait timeout") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/wait.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/io_uring/wait.c b/io_uring/wait.c index 38425d4cb7bc..b7a0292767c7 100644 --- a/io_uring/wait.c +++ b/io_uring/wait.c @@ -77,9 +77,16 @@ static enum hrtimer_restart io_cqring_min_timer_wakeup(struct hrtimer *timer) if (iowq->timeout == KTIME_MAX || ktime_compare(iowq->min_timeout, iowq->timeout) >= 0) goto out_wake; - /* work we may need to run, wake function will see if we need to wake */ - if (io_has_work(ctx)) - goto out_wake; + /* + * Work we need to run. It isn't known if that is going to post a CQE, + * so this isn't a timeout. Carry on with the normal one if it doesn't. + */ + if (io_has_work(ctx)) { + iowq->min_timeout = 0; + iowq->cq_tail = iowq->cq_min_tail + 1; + wake_up_process(iowq->wq.private); + return HRTIMER_NORESTART; + } /* got events since we started waiting, min timeout is done */ scoped_guard(rcu) { struct io_rings *rings = io_get_rings(ctx); -- 2.55.0 With dynamic tracking a NAPI id that hasn't been seen for a minute is meant to be dropped from the list the ring busy polls. The timeout is added to jiffies, but it is defined as 60 * SEC_CONVERSION, and SEC_CONVERSION isn't the number of jiffies in a second. It is the internal factor of __timespec64_to_jiffies(), HZ scaled up by SEC_JIFFIE_SC bits. For HZ=1000 that is 1000 << 22, which puts the timeout at about eight years. On 32-bit the multiplication overflows. So entries are never removed, and every id that a socket of the ring ever had keeps being polled on each wait. Fixes: 8d0c12a80cde ("io-uring: add napi busy poll support") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/napi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/io_uring/napi.c b/io_uring/napi.c index ca1c814fa73a..53d7623e4d23 100644 --- a/io_uring/napi.c +++ b/io_uring/napi.c @@ -6,7 +6,7 @@ #ifdef CONFIG_NET_RX_BUSY_POLL /* Timeout for cleanout of stale entries. */ -#define NAPI_TIMEOUT (60 * SEC_CONVERSION) +#define NAPI_TIMEOUT (60 * HZ) struct io_napi_entry { unsigned int napi_id; -- 2.55.0 __io_napi_busy_loop() caps the busy poll time to what is left until the timeout of the wait. The difference is negative if the timeout has passed already, which is what an application gets if it uses IORING_ENTER_ABS_TIMER with a time that isn't in the future any more. The min_t(u64, ...) turns that into a huge value, so the cap has no effect and the wait, which should return right away, spins for the full busy poll time first. Fixes: d5cce407e4f5 ("io_uring/napi: postpone napi timeout adjustment") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/napi.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/io_uring/napi.c b/io_uring/napi.c index 53d7623e4d23..7e698720cd51 100644 --- a/io_uring/napi.c +++ b/io_uring/napi.c @@ -378,6 +378,8 @@ void __io_napi_busy_loop(struct io_ring_ctx *ctx, struct io_wait_queue *iowq) if (iowq->timeout != KTIME_MAX) { ktime_t dt = ktime_sub(iowq->timeout, io_get_time(ctx)); + /* an absolute timeout may have passed already */ + dt = max_t(ktime_t, dt, 0); iowq->napi_busy_poll_dt = min_t(u64, iowq->napi_busy_poll_dt, dt); } -- 2.55.0 io_zcrx_queue_cqe() computes the offset of the buffer in its area from net_iov_idx(), which is an unsigned int, so the shift is done in 32 bits and only the result is widened to the u64 that goes into the CQE. An area can be up to 1TB in size. For every niov beyond the first 4GB the CQE points the application at a buffer 4GB, or a multiple of that, below the one which has the data. It reads the wrong data, and then returns a buffer it doesn't own via the refill queue while the real one is never given back. Fixes: 11ed914bbf94 ("io_uring/zcrx: add io_recvzc request") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/zcrx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/io_uring/zcrx.c b/io_uring/zcrx.c index cc9df7e64a40..6d7abb1636e7 100644 --- a/io_uring/zcrx.c +++ b/io_uring/zcrx.c @@ -1645,7 +1645,7 @@ static bool io_zcrx_queue_cqe(struct io_kiocb *req, struct net_iov *niov, cqe->flags |= IORING_CQE_F_32; area = io_zcrx_iov_to_area(niov); - offset = off + (net_iov_idx(niov) << ifq->niov_shift); + offset = off + ((u64)net_iov_idx(niov) << ifq->niov_shift); rcqe = (struct io_uring_zcrx_cqe *)(cqe + 1); rcqe->off = offset + zcrx_area_id_to_token(area->area_id); rcqe->__pad = 0; -- 2.55.0 io_ring_buffer_select() flags the request with REQ_F_BL_EMPTY if it takes the last buffer of the ring. Head and tail are 16-bit counters, but "head + 1" is computed as an int. If head is 65535 and tail has wrapped to 0, that compares 65536 to 0 and the flag isn't set. A bundle receive relies on the flag to tell that there is no point in trying to get more data. Without it, it goes for another round, finds no buffer, and the request, which has received data, is marked as failed on account of the -ENOBUFS. That severs a link, and ends a multishot. Fixes: c3f9109dbc9e ("io_uring/kbuf: flag request if buffer pool is empty after buffer pick") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/kbuf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/io_uring/kbuf.c b/io_uring/kbuf.c index 6e2facf34bfa..c9641b6ccc8f 100644 --- a/io_uring/kbuf.c +++ b/io_uring/kbuf.c @@ -204,7 +204,7 @@ static struct io_br_sel io_ring_buffer_select(struct io_kiocb *req, size_t *len, if (unlikely(tail == head)) return sel; - if (head + 1 == tail) + if ((__u16)(head + 1) == tail) req->flags |= REQ_F_BL_EMPTY; buf = io_ring_head_to_buf(br, head, bl->mask); -- 2.55.0 __io_sq_thread() doesn't submit anything as long as the ring has IORING_SETUP_R_DISABLED set. The check that keeps the SQPOLL thread from going to sleep while the SQ ring holds entries doesn't know about that. If an application fills in SQEs first and enables the ring later, the thread finds that there is work each time it is about to sleep, goes around, doesn't submit it, and so on. It uses up a CPU until the ring gets enabled. Entries in the SQ ring of a disabled ring are no reason to stay awake. io_register_enable_rings() clears the flag and then wakes the thread, and the flag is looked at after prepare_to_wait(), so the thread can't miss that. Fixes: 7e84e1c7566a ("io_uring: allow disabling rings during the creation") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/sqpoll.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/io_uring/sqpoll.c b/io_uring/sqpoll.c index ad42e8eb1002..ee97808a065a 100644 --- a/io_uring/sqpoll.c +++ b/io_uring/sqpoll.c @@ -385,6 +385,13 @@ static int io_sq_thread(void *data) */ smp_mb__after_atomic(); + /* + * Nothing gets submitted before the ring is + * enabled, which comes with a wake up. + */ + if (READ_ONCE(ctx->flags) & IORING_SETUP_R_DISABLED) + continue; + if (io_sqring_entries(ctx)) { needs_sched = false; break; -- 2.55.0 io_pipe_fixed() installs both ends of the pipe in the fixed file table, drops ->uring_lock, and copies the slot numbers to the application. If that faults, it takes the lock again and removes whatever is in these slots. If the request is run by io-wq the lock really is dropped in between, and the slots are visible to everybody else. By the time the error path gets to them, they may have been closed and reused, by IORING_OP_CLOSE and a direct open or accept, or by IORING_REGISTER_FILES_UPDATE. Then a file is removed from the table that has nothing to do with the failed request. Do the copy with the lock held. Faulting in user memory under ->uring_lock is fine, the registration paths do it all the time. Fixes: 53db8a71ecb4 ("io_uring: add support for IORING_OP_PIPE") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/openclose.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/io_uring/openclose.c b/io_uring/openclose.c index c71242915dad..24cf3ce2e2bc 100644 --- a/io_uring/openclose.c +++ b/io_uring/openclose.c @@ -375,13 +375,16 @@ static int io_pipe_fixed(struct io_kiocb *req, struct file **files, fds[1] = alloc_slot ? ret : slot - 1; files[1] = NULL; - io_ring_submit_unlock(ctx, issue_flags); - - if (!copy_to_user(p->fds, fds, sizeof(fds))) + /* + * Hang on to the lock. If this fails, the slots have to be cleared, and + * they must still hold what was put there. + */ + if (!copy_to_user(p->fds, fds, sizeof(fds))) { + io_ring_submit_unlock(ctx, issue_flags); return 0; + } ret = -EFAULT; - io_ring_submit_lock(ctx, issue_flags); err: if (fds[0] != -1) io_fixed_fd_remove(ctx, fds[0]); -- 2.55.0 -ERESTARTSYS and friends are instructions for the system call exit code, which either restarts the call or turns them into -EINTR. They are not supposed to be seen by applications. A request isn't a system call, nobody looks at what it returns before that is put into a CQE. The read/write and the networking opcodes convert these codes to -EINTR by hand. The others don't. An io-wq worker that is blocked in the open of a FIFO, a tty or some other device, or in a splice from an empty pipe, and that gets interrupted because the request is cancelled, completes it with -512: IORING_OP_OPENAT, IOSQE_ASYNC // FIFO without a writer IORING_OP_ASYNC_CANCEL // -EALREADY, worker is signalled CQE of the open: res == -512 There are more candidates, like whatever a file system or driver returns from ->flush(), ->fsync(), ->fallocate() or ->uring_cmd(), or breaking a lease or delegation. Take care of it in io_issue_sqe(), which all of them go through. The helper is moved from rw.c as is. The cost is a compare and a branch that is not taken per request. Fixes: 15b71abe7b52 ("io_uring: add support for IORING_OP_OPENAT") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/io_uring.c | 5 +++++ io_uring/io_uring.h | 18 ++++++++++++++++++ io_uring/rw.c | 18 ------------------ 3 files changed, 23 insertions(+), 18 deletions(-) diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c index 7092efc22dd2..2fa720160e8f 100644 --- a/io_uring/io_uring.c +++ b/io_uring/io_uring.c @@ -1457,6 +1457,9 @@ static int io_issue_sqe(struct io_kiocb *req, unsigned int issue_flags) ret = __io_issue_sqe(req, issue_flags, def); if (ret == IOU_COMPLETE) { + /* not all handlers take care of the internal restart codes */ + if (unlikely(req->cqe.res <= -ERESTARTSYS)) + req->cqe.res = io_fixup_restart_res(req->cqe.res); if (issue_flags & IO_URING_F_COMPLETE_DEFER) io_req_complete_defer(req); else @@ -1471,6 +1474,8 @@ static int io_issue_sqe(struct io_kiocb *req, unsigned int issue_flags) if (req->flags & REQ_F_IOPOLL) io_iopoll_req_issued(req, issue_flags); } + if (unlikely(ret <= -ERESTARTSYS)) + ret = io_fixup_restart_res(ret); return ret; } diff --git a/io_uring/io_uring.h b/io_uring/io_uring.h index 36d0d67fcb9f..203bf019e931 100644 --- a/io_uring/io_uring.h +++ b/io_uring/io_uring.h @@ -319,6 +319,24 @@ static __always_inline bool io_fill_cqe_req(struct io_ring_ctx *ctx, return true; } +static inline ssize_t io_fixup_restart_res(ssize_t ret) +{ + switch (ret) { + case -ERESTARTSYS: + case -ERESTARTNOINTR: + case -ERESTARTNOHAND: + case -ERESTART_RESTARTBLOCK: + /* + * We can't just restart the syscall, since previously + * submitted sqes may already be in progress. Just fail + * this IO with EINTR. + */ + return -EINTR; + default: + return ret; + } +} + static inline void *io_uring_alloc_async_data(struct io_alloc_cache *cache, struct io_kiocb *req) { diff --git a/io_uring/rw.c b/io_uring/rw.c index d33b925a848e..a65693e39369 100644 --- a/io_uring/rw.c +++ b/io_uring/rw.c @@ -613,24 +613,6 @@ static void io_complete_rw_iopoll(struct kiocb *kiocb, long res) smp_store_release(&req->iopoll_completed, 1); } -static inline ssize_t io_fixup_restart_res(ssize_t ret) -{ - switch (ret) { - case -ERESTARTSYS: - case -ERESTARTNOINTR: - case -ERESTARTNOHAND: - case -ERESTART_RESTARTBLOCK: - /* - * We can't just restart the syscall, since previously - * submitted sqes may already be in progress. Just fail - * this IO with EINTR. - */ - return -EINTR; - default: - return ret; - } -} - static inline void io_rw_done(struct io_kiocb *req, ssize_t ret) { struct io_rw *rw = io_kiocb_to_cmd(req, struct io_rw); -- 2.55.0 Commit 41b70df5b38b ("io_uring/net: commit partial buffers on retry") commits ring provided buffers when a receive is going to be retried after a partial transfer, so that another request using the same buffer group can't be handed the memory the first one keeps receiving into. It commits the number of bytes received so far. That does the job for a single buffer which is consumed as a whole, but the request hangs on to all it has mapped: - For a buffer ring with IOU_PBUF_RING_INC only these bytes are consumed. The rest of the buffer stays at the head of the ring. - For a bundle only the buffers which hold these bytes are consumed, the others that were peeked stay in the ring. In both cases the next receive with that buffer group gets memory that the retried request is still going to write to, and whoever comes last overwrites the data of the other. What the retry fills isn't committed at all, as the buffer list isn't known any more at that point. So the kernel hands it out again later on, while the CQE tells the application that it was used, and for an incremental buffer that it is done with. The remainder can't be committed in advance: if the request ends up receiving less, there is no way to tell the application that more was consumed than the CQE accounts for. Complete the request with what it has got instead of retrying it. MSG_WAITALL is no longer honoured for receives into incremental buffers or multi buffer bundles, they can return a short count, with the request marked as failed like for other short MSG_WAITALL transfers. Fixes: 41b70df5b38b ("io_uring/net: commit partial buffers on retry") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/net.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/io_uring/net.c b/io_uring/net.c index f5f4f3ce6ef3..b1bf7dbec5d2 100644 --- a/io_uring/net.c +++ b/io_uring/net.c @@ -480,6 +480,24 @@ static int io_bundle_nbufs(struct io_async_msghdr *kmsg, int ret) return nbufs; } +/* + * A receive that is retried after a partial transfer keeps writing to what it + * has mapped. io_net_kbuf_recyle() only commits what was filled so far, which + * takes a buffer out of circulation if it's a single one that is consumed as a + * whole. The tail of an incrementally consumed buffer, or the yet unused + * buffers of a bundle, would be up for grabs. They can't be committed in + * advance either, the CQE has no way to tell if they end up not being used. + */ +static bool io_net_kbuf_can_retry(struct io_kiocb *req, + struct io_buffer_list *bl, + struct io_async_msghdr *kmsg) +{ + /* not ring provided, or committed at selection time */ + if (!(req->flags & REQ_F_BUFFERS_COMMIT)) + return true; + return !(bl->flags & IOBL_INC) && iter_is_ubuf(&kmsg->msg.msg_iter); +} + static int io_net_kbuf_recyle(struct io_kiocb *req, struct io_buffer_list *bl, struct io_async_msghdr *kmsg, int len) { @@ -1091,7 +1109,8 @@ int io_recvmsg(struct io_kiocb *req, unsigned int issue_flags) io_kbuf_recycle(req, sel.buf_list, issue_flags); return IOU_RETRY; } - if (ret > 0 && io_net_retry(sock, flags)) { + if (ret > 0 && io_net_retry(sock, flags) && + io_net_kbuf_can_retry(req, sel.buf_list, kmsg)) { sr->done_io += ret; return io_net_kbuf_recyle(req, sel.buf_list, kmsg, ret); } @@ -1252,7 +1271,8 @@ int io_recv(struct io_kiocb *req, unsigned int issue_flags) io_kbuf_recycle(req, sel.buf_list, issue_flags); return IOU_RETRY; } - if (ret > 0 && io_net_retry(sock, flags)) { + if (ret > 0 && io_net_retry(sock, flags) && + io_net_kbuf_can_retry(req, sel.buf_list, kmsg)) { sr->len -= ret; sr->buf += ret; sr->done_io += ret; -- 2.55.0 IORING_TIMEOUT_MULTISHOT can't be combined with IORING_TIMEOUT_ABS, prep rejects that, as the request is armed again with the same value each time it fires. IORING_TIMEOUT_UPDATE doesn't check and takes an absolute time for a multishot timeout. The first expiry is as asked for, but io_timeout_complete() then arms the timer with ->mode of the original request, i.e. it uses the absolute time as the interval. With CLOCK_MONOTONIC the next expiry is an uptime away, with CLOCK_REALTIME more than fifty years. Fail such an update with -EINVAL, and do so before the timeout is touched. Fixes: ea97f6c8558e ("io_uring: add support for multishot timeouts") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/timeout.c | 38 ++++++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/io_uring/timeout.c b/io_uring/timeout.c index 90d440a75f28..1cf59c494769 100644 --- a/io_uring/timeout.c +++ b/io_uring/timeout.c @@ -321,22 +321,29 @@ static enum hrtimer_restart io_timeout_fn(struct hrtimer *timer) return HRTIMER_NORESTART; } -static struct io_kiocb *io_timeout_extract(struct io_ring_ctx *ctx, - struct io_cancel_data *cd) +static struct io_kiocb *io_timeout_find(struct io_ring_ctx *ctx, + struct io_cancel_data *cd) __must_hold(&ctx->timeout_lock) { struct io_timeout *timeout; - struct io_timeout_data *io; - struct io_kiocb *req = NULL; list_for_each_entry(timeout, &ctx->timeout_list, list) { - struct io_kiocb *tmp = cmd_to_io_kiocb(timeout); + struct io_kiocb *req = cmd_to_io_kiocb(timeout); - if (io_cancel_req_match(tmp, cd)) { - req = tmp; - break; - } + if (io_cancel_req_match(req, cd)) + return req; } + return NULL; +} + +static struct io_kiocb *io_timeout_extract(struct io_ring_ctx *ctx, + struct io_cancel_data *cd) + __must_hold(&ctx->timeout_lock) +{ + struct io_kiocb *req = io_timeout_find(ctx, cd); + struct io_timeout *timeout; + struct io_timeout_data *io; + if (!req) return ERR_PTR(-ENOENT); @@ -470,14 +477,21 @@ static int io_timeout_update(struct io_ring_ctx *ctx, __u64 user_data, __must_hold(&ctx->timeout_lock) { struct io_cancel_data cd = { .ctx = ctx, .data = user_data, }; - struct io_kiocb *req = io_timeout_extract(ctx, &cd); + struct io_kiocb *req = io_timeout_find(ctx, &cd); struct io_timeout *timeout = io_kiocb_to_cmd(req, struct io_timeout); struct io_timeout_data *data; - if (IS_ERR(req)) - return PTR_ERR(req); + if (!req) + return -ENOENT; data = req->async_data; + /* multishot requests only make sense with rel values */ + if ((data->flags & IORING_TIMEOUT_MULTISHOT) && mode == HRTIMER_MODE_ABS) + return -EINVAL; + + req = io_timeout_extract(ctx, &cd); + if (IS_ERR(req)) + return PTR_ERR(req); /* for a multishot request this isn't a sequence, keep the repeats */ if (!(data->flags & IORING_TIMEOUT_MULTISHOT)) timeout->off = 0; /* noseq */ -- 2.55.0 A linked timeout that fires cancels the request it is linked to, and completes with -ETIME if that worked out: ret = io_try_cancel(req->tctx, &cd, 0); ... io_req_set_res(req, ret ?: -ETIME, 0); io_try_cancel() returns 0 or an error, except for requests that are parked on ->futex_list or ->waitid_list. io_futex_cancel() and io_waitid_cancel() return the number of requests that they have cancelled, and io_try_cancel() hands it on. So the linked timeout of an IORING_OP_FUTEX_WAIT, IORING_OP_FUTEX_WAITV or IORING_OP_WAITID that it has cancelled completes with 1, each time. The waitid test of liburing checks for that 1, and has to be changed to expect -ETIME, which is what the linked timeout of any other request completes with. Fixes: f31ecf671ddc ("io_uring: add IORING_OP_WAITID support") Fixes: 194bb58c6090 ("io_uring: add support for futex wake and wait") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/timeout.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/io_uring/timeout.c b/io_uring/timeout.c index 1cf59c494769..b219dfdf7af5 100644 --- a/io_uring/timeout.c +++ b/io_uring/timeout.c @@ -393,6 +393,9 @@ static void io_req_task_link_timeout(struct io_tw_req tw_req, io_tw_token_t tw) }; ret = io_try_cancel(req->tctx, &cd, 0); + /* a number of requests for futex and waitid */ + if (ret > 0) + ret = 0; } else { ret = -ECANCELED; } -- 2.55.0 io_prep_async_work() decides by the file of a request what kind of work it is for io-wq. I/O on regular files and block devices is bounded work, limited to min(SQ entries, 4 * CPUs) workers, for anything else that may block indefinitely the limit is RLIMIT_NPROC. Writes to a regular file are hashed by the inode, so that they are run one after the other instead of having a bunch of workers fight for the inode lock. Since the file is only assigned when a request is issued, a request that is queued without an attempt to issue it, REQ_F_FORCE_ASYNC, doesn't have one at that point. io_prep_async_work() takes that for "not a regular file or block device": - Reads and writes end up as unbound work. 32 reads with IOSQE_ASYNC from a slow block device on a ring with 4 SQ entries get 32 workers, not 4. - Writes aren't hashed. Four buffered writes to one file get four workers, three of which do nothing but wait for the lock. The late assignment is for the benefit of links, where a request may use a fixed file that one of its predecessors installs. By the time a request gets to io_queue_iowq() these are done, it is as good as issued. Assign the file there. io_assign_file() moves up and is forced inline. With a third caller the compiler may stop inlining it, and every request that is issued would pay for a call. IORING_OP_SPLICE is left as it is, unbound and not hashed. The file of the request is where it writes to, which says nothing about how long it takes: a splice from an empty pipe or a socket into a regular file waits for its input. As bounded work, four of these on a ring with 4 SQ entries keep all other bounded work of the task from running, and as hashed work one of them blocks all writes to that file. REQ_F_FORCE_ASYNC is not only set for IOSQE_ASYNC. What changes, and brings back what was the case before the file assignment was deferred: - Requests with IOSQE_ASYNC or IOSQE_IO_DRAIN, and all requests that are submitted while a drain is active: reads and writes on regular files and block devices count against the limit for bounded workers, including the one set with IORING_REGISTER_IOWQ_MAX_WORKERS, so fewer of them run at the same time. - Of these, writes to one regular file are run one at a time, unless they are O_DIRECT and the filesystem has FOP_DIO_PARALLEL_WRITE. On a ring with IORING_SETUP_IOPOLL that goes for reads as well. - IORING_OP_FALLOCATE and IORING_OP_FTRUNCATE, which always go to io-wq, are run one at a time per file, and in turn with these writes. - The descriptor or the fixed file slot is looked up when the request is queued, not when a worker gets around to it, so closing or replacing it in between no longer has an effect on the request. Fixes: 6bf9c47a3989 ("io_uring: defer file assignment") Assisted-by: LLM Signed-off-by: Josef Bacik --- io_uring/io_uring.c | 43 +++++++++++++++++++++++++++++-------------- 1 file changed, 29 insertions(+), 14 deletions(-) diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c index 2fa720160e8f..e6569c204917 100644 --- a/io_uring/io_uring.c +++ b/io_uring/io_uring.c @@ -407,17 +407,46 @@ static void io_prep_async_link(struct io_kiocb *req) } } +/* Called for every request that is issued: keep it inline */ +static __always_inline bool io_assign_file(struct io_kiocb *req, + const struct io_issue_def *def, + unsigned int issue_flags) +{ + if (req->file || !def->needs_file) + return true; + + if (req->flags & REQ_F_FIXED_FILE) + req->file = io_file_get_fixed(req, req->cqe.fd, issue_flags); + else + req->file = io_file_get_normal(req, req->cqe.fd); + + return !!req->file; +} + void io_queue_iowq(struct io_kiocb *req) { struct io_uring_task *tctx = req->tctx; BUG_ON(!tctx); + lockdep_assert_held(&req->ctx->uring_lock); if ((current->flags & PF_KTHREAD) || !tctx->io_wq) { io_req_task_queue_fail(req, -ECANCELED); return; } + /* + * The file is looked up as late as possible, as it may be the result + * of an earlier request of the link. This one is up now, and what kind + * of work it is depends on the file. If that fails, leave the error + * handling to the worker. + * + * Not for a splice: whatever it writes to, it may wait for its input + * for ever. It has to remain unbound work that isn't hashed. + */ + if (req->opcode != IORING_OP_SPLICE) + io_assign_file(req, &io_issue_defs[req->opcode], 0); + /* init ->work of the whole link before punting */ io_prep_async_link(req); @@ -1397,20 +1426,6 @@ static __cold void io_drain_req(struct io_kiocb *req) ctx->int_flags &= ~IO_RING_F_DRAIN_ACTIVE; } -static bool io_assign_file(struct io_kiocb *req, const struct io_issue_def *def, - unsigned int issue_flags) -{ - if (req->file || !def->needs_file) - return true; - - if (req->flags & REQ_F_FIXED_FILE) - req->file = io_file_get_fixed(req, req->cqe.fd, issue_flags); - else - req->file = io_file_get_normal(req, req->cqe.fd); - - return !!req->file; -} - #define REQ_ISSUE_SLOW_FLAGS (REQ_F_CREDS | REQ_F_ARM_LTIMEOUT) static inline int __io_issue_sqe(struct io_kiocb *req, -- 2.55.0