io_uring: Fix hangs, lost wake ups, missed cancels and ring corner cases
Subsystems (3)
cover io_uring: Fix hangs, lost wake ups, missed cancels and ring corner cases
Hello,
We've been running a large LLM driven audit of the kernel, and these are the io_uring fixes that came out of it. Some of them were found by writing models of the task_work, poll, io-wq and CQ code and running those through a model checker, the rest by reading the code and then writing a reproducer. It's a lot of patches, sorry about that. They're standalone fixes, I put them in one series so I didn't send you 43 separate emails, so take whatever subset you like. The ones that belong together are 7 and 8 (the same lost wake up in two places), 14, 15 and 18 (the same lost cancel, 15 and 18 close what 14 says it leaves open), and 25-27, which touch the same two loops in the resize code.
Patches 1 and 2 are memory safety, a use after free in waitid and a write through LIST_POISON2 in the futexv completion. Patches 3-9 are tasks that hang or spin in the kernel. None of them need any privilege, other than 9 which takes a SCHED_FIFO task, and with 3, 5 and 6 the task can't be killed. 10-12 are io-wq leaving work sitting there with nobody to run it. 13-18 are cancellations that miss their request or report the wrong thing. 19-22 are wake ups that the application never gets. 23-28 are the mixed size CQE/SQE and ring resize corner cases, 29-34 are timeouts and waits, and 35-39 are small things all over the place.
Patches 40-43 are at the end on purpose. They fix real problems, but they change something that somebody could be relying on, so I'd understand if you want these done differently. 40 stops honouring MSG_WAITALL for receives into incremental buffer rings and bundles, since the retry writes into buffers that the ring hands to the next request. 41 fails an ABS update of a multishot timeout with -EINVAL. 42 makes the linked timeout of a futex or waitid request complete with -ETIME like all the others instead of 1, and liburing's waitid.t checks for that 1, so it needs a liburing change to go with it. 43 goes back to looking up the file of an IOSQE_ASYNC request when it's queued, so that io-wq can tell bounded from unbounded work and hash the writes. There are smaller visible changes further up, 3 returns to userspace if the overflow list can't be flushed, 22 allocates the private futex hash at prep time and can fail with -ENOMEM, and 25 and 27 fail a resize with -EOVERFLOW rather than tear a 32b CQE or a 128b SQE in two.
These were found on 7.3-rc5, and the numbers in the changelogs are from there. After rebasing onto for-next I ran the reproducers again, for-next against for-next plus the series, same config, in TCG guests. 30 of the 43 reproduce on the unmodified for-next and are gone with the series. 1, 7, 8, 9 and 11 only reproduce with a delay patched into the window, same delay on both sides. 10 and 12 I can only show in the model, and 25, 33, 34, 36 and 38 are from reading the code. 17 needs a 32-bit kernel, that one I ran on rc5 but not again on for-next. I didn't want to post the reproducers along with the series, but I'm happy to send them to anybody who wants them.
I ran the liburing tests (d5b6007) the same way. Of the 257 for-next passes 240, skips 16, and send-zerocopy.t times out because TCG is slow. With the series the only difference is waitid.t, which is patch 42, see above. iowait.t fails every now and then on both.
Every patch builds on its own with W=1. A matrix of exit, thread exit, exec and ring close against 21 kinds of pending request on plain, DEFER_TASKRUN and SQPOLL rings gives the same results with and without the series, other than the hangs that 4 fixes. KASAN and kmemleak had nothing to say, and lockdep only tripped the assert in signal_wake_up_state() once, for a ring with SQPOLL and exec, which plain 7.3-rc5 does as well.
That matrix did find one thing that's not ours. On for-next, with or without this series, a task that submits an IORING_OP_TIMEOUT with IOSQE_ASYNC and calls exit_group() right away gets stuck in io_uring_cancel_generic() for good in 2 to 27 percent of the attempts, and it can't be killed. 7.3-rc5 doesn't do that. I haven't dug into it, my guess is that the worker arms the timeout after the exiting task has counted the armed timeouts, and then nothing wakes it up. I can send the test for that as well.
A few of these are close to things that are already on the list. Patch 23 is the fix for what Leonid reported in September [1]. Patch 40 is the same bug that Aohan posted a patch for [2], that one defers the commit rather than giving up on the retry and doesn't cover incremental buffers. We also had a fix for the poll retry flag, I dropped it as Juanlu has since posted one [3], for what it's worth we hit the same thing.
This is based on your for-next as of Wednesday. It applies to for-7.4/io_uring as well, except for patch 8, which needs commit fddcf4c5321b ("io_uring/cancel: don't spin on local work the task can't run"). I didn't add Cc: stable to anything, they all have Fixes tags. Thanks,
Josef
[1] https://lore.kernel.org/io-uring/CAECZYD5OMD9vMTzjteFMMP40Dn+SWKj9SFj=43U6uO3XMnxJbw@mail.gmail.com/
[2] https://lore.kernel.org/io-uring/[email protected]/
[3] https://lore.kernel.org/io-uring/[email protected]/
---
Josef Bacik (43):
io_uring/waitid: own the request before clearing ->head io_uring/cmd: don't leave list poison in ->hash_node io_uring/wait: don't spin on a stuck overflow list io_uring/io-wq: don't lose the signal that ends work io_uring/io-wq: don't hide a link from cancellations
io_uring: don't flood a cancelling task with task_work
io_uring: don't lose the DEFER_TASKRUN wake up io_uring/cancel: rearm local work wake up before sleep io_uring/mpscq: disable preemption while pushing io_uring/io-wq: don't go to sleep on pending work io_uring/io-wq: don't strand work when a worker exits io_uring/io-wq: don't exit with a creation pending io_uring/io-wq: let cancel find picked up hashed work io_uring/poll: don't lose a cancel as io-wq arms poll
io_uring: don't lose a cancel as io-wq parks a request io_uring/cancel: don't forget that the work was running io_uring/cancel: recognize "no timeout" on 32-bit io_uring/cancel: let CANCEL_ALL find parked requests
io_uring: wake ring pollers for local task_work io_uring/wait: look at all CQ waiters on wake up io_uring/poll: drop stale double poll entry on rearm io_uring/futex: allocate private hash before waiting
io_uring: drop the CQE cache if a refill fails
io_uring: don't split 128b SQEs at the SQPOLL cap io_uring/register: don't wrap 32b CQEs when resizing io_uring/register: handle index wrap when resizing io_uring/register: keep 128b SQEs intact on resize io_uring/fdinfo: fix CQE dump of CQE32 rings io_uring/timeout: don't skip over reached sequences io_uring/timeout: keep repeat count on multishot update io_uring/timeout: use the right clock for ABS updates io_uring/wait: no early -ETIME if min_wait finds work io_uring/napi: make stale entries time out after 60s io_uring/napi: don't busy poll past an expired timeout io_uring/zcrx: fix buffer offsets in areas above 4GB io_uring/kbuf: handle index wrap in last buffer check io_uring/sqpoll: let the thread sleep on disabled rings io_uring/openclose: keep the lock in io_pipe_fixed()
io_uring: don't pass restart error codes to userspace io_uring/net: don't retry recv on uncommitted buffers io_uring/timeout: no ABS update of multishot timeouts io_uring/timeout: don't end a linked timeout with 1
io_uring: look up the file before queueing to io-wq
include/linux/io_uring_types.h | 3 + io_uring/cancel.c | 43 +++++++++++-- io_uring/cancel.h | 2 + io_uring/fdinfo.c | 30 +++++---- io_uring/futex.c | 32 ++++++++-- io_uring/io-wq.c | 124 +++++++++++++++++++++++++++++++------ io_uring/io_uring.c | 135 +++++++++++++++++++++++++++++------------ io_uring/io_uring.h | 38 +++++++++++- io_uring/kbuf.c | 2 +- io_uring/loop.c | 8 +-- io_uring/mpscq.h | 14 ++++- io_uring/napi.c | 4 +- io_uring/net.c | 24 +++++++- io_uring/openclose.c | 11 ++-- io_uring/poll.c | 21 +++++++ io_uring/register.c | 54 ++++++++++++++++- io_uring/rw.c | 18 ------ io_uring/sqpoll.c | 7 +++ io_uring/timeout.c | 77 ++++++++++++++++------- io_uring/tw.c | 36 ++++++++++- io_uring/uring_cmd.c | 2 +- io_uring/wait.c | 32 +++++++--- io_uring/wait.h | 15 +++++ io_uring/waitid.c | 12 +++- io_uring/zcrx.c | 2 +- 25 files changed, 596 insertions(+), 150 deletions(-)
---
base-commit: 75b36d2f2b09e3bc3294e37e693d6bf0bdf6c5e7
change-id: 20261015-b4-io-uring-fixes-c50136512047
PATCH 1/43 io_uring/waitid: own the request before clearing ->head
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 reusedatomic_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.
--- io_uring/waitid.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
io_uring/waitid.c +8 -2
@@ -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 ;
Signature · 1 line
2.55.0
PATCH 2/43 io_uring/cmd: don't leave list poison in ->hash_node
In reply to Josef Bacikio_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.
--- io_uring/uring_cmd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
io_uring/uring_cmd.c +1 -1
@@ -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); }
Signature · 1 line
2.55.0
PATCH 3/43 io_uring/wait: don't spin on a stuck overflow list
In reply to Josef BacikAs 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.
--- io_uring/wait.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
io_uring/wait.c +11 -2
@@ -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 ;
Signature · 1 line
2.55.0
PATCH 4/43 io_uring/io-wq: don't lose the signal that ends work
In reply to Josef BacikWhen 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.
--- io_uring/io-wq.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
io_uring/io-wq.c +8 -2
@@ -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);
Signature · 1 line
2.55.0
PATCH 5/43 io_uring/io-wq: don't hide a link from cancellations
In reply to Josef BacikA 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.
--- 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(-)
io_uring/io-wq.c +15 -3
@@ -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); }
io_uring/io_uring.c +29 -17
64 lines not on this page · raw message
io_uring/io_uring.h +2 -1
@@ -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);
Signature · 1 line
2.55.0
PATCH 6/43 io_uring: don't flood a cancelling task with task_work
In reply to Josef Bacikio_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.
--- io_uring/io_uring.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
io_uring/io_uring.c +11 -1
@@ -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); }
Signature · 1 line
2.55.0
PATCH 7/43 io_uring: don't lose the DEFER_TASKRUN wake up
In reply to Josef BacikSince 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, returnThe 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.
--- 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(-)
io_uring/loop.c +1 -7
@@ -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) ||
io_uring/tw.c +1 -1
@@ -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.
io_uring/wait.c +1 -2
@@ -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 );
io_uring/wait.h +15 -0
@@ -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;
Signature · 1 line
2.55.0
PATCH 8/43 io_uring/cancel: rearm local work wake up before sleep
In reply to Josef Bacikio_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 Aio_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, returnOnly 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.
--- io_uring/cancel.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
io_uring/cancel.c +12 -2
@@ -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; }/*
Signature · 1 line
2.55.0
PATCH 9/43 io_uring/mpscq: disable preemption while pushing
In reply to Josef BacikA 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) <preempted> 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.
--- io_uring/mpscq.h | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
io_uring/mpscq.h +11 -3
@@ -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; }
Signature · 1 line
2.55.0
PATCH 10/43 io_uring/io-wq: don't go to sleep on pending work
In reply to Josef Bacikio_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 -> 1schedule_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.
--- io_uring/io-wq.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)
io_uring/io-wq.c +14 -3
@@ -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;
Signature · 1 line
2.55.0
PATCH 11/43 io_uring/io-wq: don't strand work when a worker exits
In reply to Josef BacikA 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 createacct->nr_workers--
breakio_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.
--- io_uring/io-wq.c | 36 +++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-)
io_uring/io-wq.c +35 -1
@@ -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);
Signature · 1 line
2.55.0
PATCH 12/43 io_uring/io-wq: don't exit with a creation pending
In reply to Josef BacikA 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 -> 1io_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.
--- io_uring/io-wq.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
io_uring/io-wq.c +6 -2
@@ -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);
Signature · 1 line
2.55.0
PATCH 13/43 io_uring/io-wq: let cancel find picked up hashed work
In reply to Josef Bacikio_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.
--- io_uring/io-wq.c | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-)
io_uring/io-wq.c +29 -6
@@ -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;
Signature · 1 line
2.55.0
PATCH 14/43 io_uring/poll: don't lose a cancel as io-wq arms poll
In reply to Josef BacikA 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.
--- io_uring/poll.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
io_uring/poll.c +12 -0
@@ -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 );
Signature · 1 line
2.55.0
PATCH 15/43 io_uring: don't lose a cancel as io-wq parks a request
In reply to Josef BacikIORING_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_COMPLETENobody 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.
--- io_uring/futex.c | 4 ++++ io_uring/io_uring.h | 17 +++++++++++++++++ io_uring/waitid.c | 2 ++ 3 files changed, 23 insertions(+)
io_uring/futex.c +4 -0
@@ -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 ;
io_uring/io_uring.h +17 -0
@@ -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 */
io_uring/waitid.c +2 -0
@@ -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 ; }
Signature · 1 line
2.55.0
PATCH 16/43 io_uring/cancel: don't forget that the work was running
In reply to Josef BacikIf 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.
--- io_uring/cancel.c | 13 ++++++++++++- io_uring/cancel.h | 2 ++ 2 files changed, 14 insertions(+), 1 deletion(-)
io_uring/cancel.c +12 -1
@@ -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)
io_uring/cancel.h +2 -0
@@ -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);
Signature · 1 line
2.55.0
PATCH 17/43 io_uring/cancel: recognize "no timeout" on 32-bit
In reply to Josef BacikIORING_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.
--- io_uring/cancel.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
io_uring/cancel.c +1 -1
@@ -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
Signature · 1 line
2.55.0
PATCH 18/43 io_uring/cancel: let CANCEL_ALL find parked requests
In reply to Josef BacikA 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.
--- io_uring/cancel.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
io_uring/cancel.c +13 -1
@@ -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)
Signature · 1 line
2.55.0
PATCH 19/43 io_uring: wake ring pollers for local task_work
In reply to Josef BacikWith 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.
--- include/linux/io_uring_types.h | 3 +++ io_uring/tw.c | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+)
include/linux/io_uring_types.h +3 -0
@@ -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;
io_uring/tw.c +34 -0
@@ -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); }/*
Signature · 1 line
2.55.0
PATCH 20/43 io_uring/wait: look at all CQ waiters on wake up
In reply to Josef Bacikio_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.
--- io_uring/wait.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
io_uring/wait.c +2 -1
@@ -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)
Signature · 1 line
2.55.0
PATCH 21/43 io_uring/poll: drop stale double poll entry on rearm
In reply to Josef BacikA 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.
--- io_uring/poll.c | 9 +++++++++ 1 file changed, 9 insertions(+)
io_uring/poll.c +9 -0
@@ -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 );
Signature · 1 line
2.55.0
PATCH 22/43 io_uring/futex: allocate private hash before waiting
In reply to Josef BacikThe 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.
--- io_uring/futex.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-)
io_uring/futex.c +24 -4
@@ -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 ; } }
Signature · 1 line
2.55.0
PATCH 23/43 io_uring: drop the CQE cache if a refill fails
In reply to Josef Bacik->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.
--- io_uring/io_uring.c | 7 +++++++ 1 file changed, 7 insertions(+)
io_uring/io_uring.c +7 -0
@@ -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.
Signature · 1 line
2.55.0
PATCH 24/43 io_uring: don't split 128b SQEs at the SQPOLL cap
In reply to Josef BacikIf 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.
--- io_uring/io_uring.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-)
io_uring/io_uring.c +16 -6
@@ -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 )) {
Signature · 1 line
2.55.0
PATCH 25/43 io_uring/register: don't wrap 32b CQEs when resizing
In reply to Josef BacikOn 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.
--- io_uring/register.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
io_uring/register.c +16 -0
@@ -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);
Signature · 1 line
2.55.0
PATCH 26/43 io_uring/register: handle index wrap when resizing
In reply to Josef BacikThe 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.
--- io_uring/register.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
io_uring/register.c +2 -2
@@ -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;
Signature · 1 line
2.55.0
PATCH 27/43 io_uring/register: keep 128b SQEs intact on resize
In reply to Josef BacikOn 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.
--- io_uring/register.c | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-)
io_uring/register.c +33 -1
@@ -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 ;
Signature · 1 line
2.55.0
PATCH 28/43 io_uring/fdinfo: fix CQE dump of CQE32 rings
In reply to Josef BacikSince 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.
--- io_uring/fdinfo.c | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-)
io_uring/fdinfo.c +18 -12
@@ -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++; }
Signature · 1 line
2.55.0
PATCH 29/43 io_uring/timeout: don't skip over reached sequences
In reply to Josef Bacikio_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.
--- io_uring/timeout.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)
io_uring/timeout.c +14 -7
@@ -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:
Signature · 1 line
2.55.0
PATCH 30/43 io_uring/timeout: keep repeat count on multishot update
In reply to Josef BacikIORING_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.
--- io_uring/timeout.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
io_uring/timeout.c +3 -1
@@ -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);
Signature · 1 line
2.55.0
PATCH 31/43 io_uring/timeout: use the right clock for ABS updates
In reply to Josef Bacikio_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.
--- io_uring/timeout.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
io_uring/timeout.c +10 -1
@@ -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) {
Signature · 1 line
2.55.0
PATCH 32/43 io_uring/wait: no early -ETIME if min_wait finds work
In reply to Josef BacikWhen 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.
--- io_uring/wait.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
io_uring/wait.c +10 -3
@@ -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);
Signature · 1 line
2.55.0
PATCH 33/43 io_uring/napi: make stale entries time out after 60s
In reply to Josef BacikWith 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.
--- io_uring/napi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
io_uring/napi.c +1 -1
@@ -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;
Signature · 1 line
2.55.0
PATCH 34/43 io_uring/napi: don't busy poll past an expired timeout
In reply to Josef Bacik__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.
--- io_uring/napi.c | 2 ++ 1 file changed, 2 insertions(+)
io_uring/napi.c +2 -0
@@ -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); }
Signature · 1 line
2.55.0
PATCH 35/43 io_uring/zcrx: fix buffer offsets in areas above 4GB
In reply to Josef Bacikio_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.
--- io_uring/zcrx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
io_uring/zcrx.c +1 -1
@@ -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 ;
Signature · 1 line
2.55.0
Pavel Begunkov
maintainerRe: [PATCH 35/43] io_uring/zcrx: fix buffer offsets in areas above 4GB
Quoted 10 lines · Josef Bacik
io_zcrx_queue_cqe()computes the offset of the buffer in its area fromnet_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")
I was going to say that zcrx doesn't support >1G areas, but apparently the function it relied on got changed without adjusting zcrx, so the tag should rather refer to.
commit b4e41050b212ca33c82fb0598a7b323d5b18f1bb
Author: Jens Axboe <[email protected]> Date: Mon May 4 05:42:51 2026 -0600
io_uring/rsrc: raise registered buffer 1GB limit
Quoted 20 lines
Assisted-by LLMSigned-off-by Josef Bacik <[email protected]>--- io_uring/zcrx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)io_uring/zcrx.c +1
-1@@ -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 ;
Signature · 1 line
Pavel Begunkov
PATCH 36/43 io_uring/kbuf: handle index wrap in last buffer check
In reply to Josef Bacikio_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.
--- io_uring/kbuf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
io_uring/kbuf.c +1 -1
@@ -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);
Signature · 1 line
2.55.0
PATCH 37/43 io_uring/sqpoll: let the thread sleep on disabled rings
In reply to Josef Bacik__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.
--- io_uring/sqpoll.c | 7 +++++++ 1 file changed, 7 insertions(+)
io_uring/sqpoll.c +7 -0
@@ -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 ;
Signature · 1 line
2.55.0
PATCH 38/43 io_uring/openclose: keep the lock in io_pipe_fixed()
In reply to Josef Bacikio_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.
--- io_uring/openclose.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
io_uring/openclose.c +7 -4
@@ -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 ]);
Signature · 1 line
2.55.0
PATCH 39/43 io_uring: don't pass restart error codes to userspace
In reply to Josef Bacik-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.
--- io_uring/io_uring.c | 5 +++++ io_uring/io_uring.h | 18 ++++++++++++++++++ io_uring/rw.c | 18 ------------------ 3 files changed, 23 insertions(+), 18 deletions(-)
io_uring/io_uring.c +5 -0
@@ -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; }
io_uring/io_uring.h +18 -0
@@ -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) {
io_uring/rw.c +0 -18
@@ -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);
Signature · 1 line
2.55.0
PATCH 40/43 io_uring/net: don't retry recv on uncommitted buffers
In reply to Josef BacikCommit 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.
--- io_uring/net.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-)
io_uring/net.c +22 -2
@@ -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;
Signature · 1 line
2.55.0
PATCH 41/43 io_uring/timeout: no ABS update of multishot timeouts
In reply to Josef BacikIORING_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.
--- io_uring/timeout.c | 38 ++++++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 12 deletions(-)
io_uring/timeout.c +26 -12
@@ -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 */
Signature · 1 line
2.55.0
PATCH 42/43 io_uring/timeout: don't end a linked timeout with 1
In reply to Josef BacikA 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.
--- io_uring/timeout.c | 3 +++ 1 file changed, 3 insertions(+)
io_uring/timeout.c +3 -0
@@ -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 ; }
Signature · 1 line
2.55.0
PATCH 43/43 io_uring: look up the file before queueing to io-wq
In reply to Josef Bacikio_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.
--- io_uring/io_uring.c | 43 +++++++++++++++++++++++++++++-------------- 1 file changed, 29 insertions(+), 14 deletions(-)
io_uring/io_uring.c +29 -14
@@ -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,
Signature · 1 line
2.55.0
Jens Axboe
maintainerQuoted 76 lines · Josef Bacik
Hello,
We've been running a large LLM driven audit of the kernel, and these are the
io_uringfixes that came out of it. Some of them were found by writing models of thetask_work, poll, io-wq and CQ code and running those through a model checker, the rest by reading the code and then writing a reproducer. It's a lot of patches, sorry about that. They're standalone fixes, I put them in one series so I didn't send you 43 separate emails, so take whatever subset you like. The ones that belong together are 7 and 8 (the same lost wake up in two places), 14, 15 and 18 (the same lost cancel, 15 and 18 close what 14 says it leaves open), and 25-27, which touch the same two loops in the resize code.Patches 1 and 2 are memory safety, a use after free in waitid and a write through
LIST_POISON2in the futexv completion. Patches 3-9 are tasks that hang or spin in the kernel. None of them need any privilege, other than 9 which takes aSCHED_FIFOtask, and with 3, 5 and 6 the task can't be killed. 10-12 are io-wq leaving work sitting there with nobody to run it. 13-18 are cancellations that miss their request or report the wrong thing. 19-22 are wake ups that the application never gets. 23-28 are the mixed size CQE/SQE and ring resize corner cases, 29-34 are timeouts and waits, and 35-39 are small things all over the place.Patches 40-43 are at the end on purpose. They fix real problems, but they change something that somebody could be relying on, so I'd understand if you want these done differently. 40 stops honouring
MSG_WAITALLfor receives into incremental buffer rings and bundles, since the retry writes into buffers that the ring hands to the next request. 41 fails an ABS update of a multishot timeout with-EINVAL. 42 makes the linked timeout of a futex or waitid request complete with-ETIMElike all the others instead of 1, and liburing's waitid.t checks for that 1, so it needs a liburing change to go with it. 43 goes back to looking up the file of anIOSQE_ASYNCrequest when it's queued, so that io-wq can tell bounded from unbounded work and hash the writes. There are smaller visible changes further up, 3 returns to userspace if the overflow list can't be flushed, 22 allocates the private futex hash at prep time and can fail with-ENOMEM, and 25 and 27 fail a resize with-EOVERFLOWrather than tear a 32b CQE or a 128b SQE in two.These were found on 7.3-rc5, and the numbers in the changelogs are from there. After rebasing onto for-next I ran the reproducers again, for-next against for-next plus the series, same config, in TCG guests. 30 of the 43 reproduce on the unmodified for-next and are gone with the series. 1, 7, 8, 9 and 11 only reproduce with a delay patched into the window, same delay on both sides. 10 and 12 I can only show in the model, and 25, 33, 34, 36 and 38 are from reading the code. 17 needs a 32-bit kernel, that one I ran on rc5 but not again on for-next. I didn't want to post the reproducers along with the series, but I'm happy to send them to anybody who wants them.
I ran the liburing tests (d5b6007) the same way. Of the 257 for-next passes 240, skips 16, and send-zerocopy.t times out because TCG is slow. With the series the only difference is waitid.t, which is patch 42, see above. iowait.t fails every now and then on both.
Every patch builds on its own with W=1. A matrix of exit, thread exit, exec and ring close against 21 kinds of pending request on plain,
DEFER_TASKRUNand SQPOLL rings gives the same results with and without the series, other than the hangs that 4 fixes. KASAN and kmemleak had nothing to say, and lockdep only tripped the assert insignal_wake_up_state()once, for a ring with SQPOLL and exec, which plain 7.3-rc5 does as well.That matrix did find one thing that's not ours. On for-next, with or without this series, a task that submits an
IORING_OP_TIMEOUTwithIOSQE_ASYNCand callsexit_group()right away gets stuck inio_uring_cancel_generic()for good in 2 to 27 percent of the attempts, and it can't be killed. 7.3-rc5 doesn't do that. I haven't dug into it, my guess is that the worker arms the timeout after the exiting task has counted the armed timeouts, and then nothing wakes it up. I can send the test for that as well.A few of these are close to things that are already on the list. Patch 23 is the fix for what Leonid reported in September [1]. Patch 40 is the same bug that Aohan posted a patch for [2], that one defers the commit rather than giving up on the retry and doesn't cover incremental buffers. We also had a fix for the poll retry flag, I dropped it as Juanlu has since posted one [3], for what it's worth we hit the same thing.
Quoted 5 lines
This is based on your for-next as of Wednesday. It applies to for-7.4/io_uring as well, except for patch 8, which needs commit
fddcf4c5321b("io_uring/cancel: don't spin on local work the task can't run"). I didn't add Cc: stable to anything, they all have Fixes tags. Thanks,
All good fixes, fiddled a few incorrect Fixes tags and added a follow-up fix for one of them. Queued up!
Signature · 1 line
Jens Axboe
Quoted 11 lines · Josef Bacik
We've been running a large LLM driven audit of the kernel, and these are the
io_uringfixes that came out of it. Some of them were found by writing models of thetask_work, poll, io-wq and CQ code and running those through a model checker, the rest by reading the code and then writing a reproducer. It's a lot of patches, sorry about that. They're standalone fixes, I put them in one series so I didn't send you 43 separate emails, so take whatever subset you like. The ones that belong together are 7 and 8 (the same lost wake up in two places), 14, 15 and 18 (the same lost cancel, 15 and 18 close what 14 says it leaves open), and 25-27, which touch the same two loops in the resize code.
Quoted 1 line
[...]
Applied, thanks!
[01/43] io_uring/waitid: own the request before clearing ->head
commit: c254eec4364b9f4b4cbdce43be3fb4df5a651a70
[02/43] io_uring/cmd: don't leave list poison in ->hash_node
commit: 4c83e39f4d1944d7146fd06094f2c8053238d55c
[03/43] io_uring/wait: don't spin on a stuck overflow list
commit: 34f5a664c34d3cb62f6bec7f4156754594a7d25b
[04/43] io_uring/io-wq: don't lose the signal that ends work
commit: 63c103279a95e82280092f2f4232e91e8117a252
[05/43] io_uring/io-wq: don't hide a link from cancellations
commit: 436d48a2ed13112d2d27085325838e5bce6b4507
[06/43] io_uring: don't flood a cancelling task with task_work
commit: 22e643e61b9b6299b42ff7d88bab48bccada8a5d
[07/43] io_uring: don't lose the DEFER_TASKRUN wake up
commit: 8257f6ec45ee07c6ed933c59d8982b03cc7bc1ca
[08/43] io_uring/cancel: rearm local work wake up before sleep
commit: 76a82c666abf155ad52d5f41043eac77dd3e41d8
[09/43] io_uring/mpscq: disable preemption while pushing
commit: 68a95c3968895cd198f42baadb23e635fecc5a66
[10/43] io_uring/io-wq: don't go to sleep on pending work
commit: f19c38feb96802ee53a99649d740d2bf630bcce7
[11/43] io_uring/io-wq: don't strand work when a worker exits
commit: ad41f325be1271b22d99353819c90f0d5e45acc8
[12/43] io_uring/io-wq: don't exit with a creation pending
commit: e231d2e8f80d3649ecd3851b7f2abdfbe606c04b
[13/43] io_uring/io-wq: let cancel find picked up hashed work
commit: e9efeb4fc97b0879512a3c3b995ed448f2487e12
[14/43] io_uring/poll: don't lose a cancel as io-wq arms poll
commit: 6d2d4c2530e95ff6ddeb98195d49d99dbca14369
[15/43] io_uring: don't lose a cancel as io-wq parks a request
commit: 5cb049d2203d7a81a1c1a737c8df4575e71078e3
[16/43] io_uring/cancel: don't forget that the work was running
commit: 8aa0e5db4845fb5ac11abe40d7b1cac0656c3ee3
[17/43] io_uring/cancel: recognize "no timeout" on 32-bit
commit: 3774e4d4c8533da2caae95556c4d635c7c4f45b8
[18/43] io_uring/cancel: let CANCEL_ALL find parked requests
commit: fe6ca13c98b1223f6005f32913ebd32e4df64b77
[19/43] io_uring: wake ring pollers for local task_work
commit: b88902d8745a701d24275d012f0830e3628e2a22
[20/43] io_uring/wait: look at all CQ waiters on wake up
commit: 356f92f2412686f822383b784fa102924407457a
[21/43] io_uring/poll: drop stale double poll entry on rearm
commit: f2dbe87968a3aef74a11c025776d293b80964d3f
[22/43] io_uring/futex: allocate private hash before waiting
commit: 3eff68bce25584cded3fb6ec033ff9a1c9913d04
[23/43] io_uring: drop the CQE cache if a refill fails
commit: 78f9c9cffef02a290b6e97672aaa9c75df0d21da
[24/43] io_uring: don't split 128b SQEs at the SQPOLL cap
commit: 79956bb39b4d372eb146955ea159ecb6fba37f5b
[25/43] io_uring/register: don't wrap 32b CQEs when resizing
commit: 0edf1930b8ed3c398d3cbdbafd8f72dbd1a3d9e4
[26/43] io_uring/register: handle index wrap when resizing
commit: e8dee8d5498c710a22a84076d8ecb661627032b4
[27/43] io_uring/register: keep 128b SQEs intact on resize
commit: 0feb5bcd2d4261d656729892aef4d643461ea5ac
[28/43] io_uring/fdinfo: fix CQE dump of CQE32 rings
commit: e4507a62c247081fffe299622c04c6f7c3d98e92
[29/43] io_uring/timeout: don't skip over reached sequences
commit: c6c0b3eba88c3b13ca11ea112404982e86d58f7a
[30/43] io_uring/timeout: keep repeat count on multishot update
commit: 3dc2a32cd356ee382aa4a8c7d844c53b2c0ad83b
[31/43] io_uring/timeout: use the right clock for ABS updates
commit: dad82eb1805d76c555224e4030319e3844642b64
[32/43] io_uring/wait: no early -ETIME if min_wait finds work
commit: 83cce67298ab1f4d2e9205a49f56975475cdb154
[33/43] io_uring/napi: make stale entries time out after 60s
commit: 9c151648780d89e5446e5f89f3fa2e416c3bb911
[34/43] io_uring/napi: don't busy poll past an expired timeout
commit: 21ed976c12f6b77d7ea028a4fe9a08e69a56179f
[35/43] io_uring/zcrx: fix buffer offsets in areas above 4GB
commit: ab4536bf2e5617b04f2d9396f06393c7bbfb071d
[36/43] io_uring/kbuf: handle index wrap in last buffer check
commit: 6e773b5161506a4d0e739eb3aa4667d28f20219c
[37/43] io_uring/sqpoll: let the thread sleep on disabled rings
commit: d492dcbb3b81c1c2d0aabef41fa1307093f3a556
[38/43] io_uring/openclose: keep the lock in io_pipe_fixed()
commit: a3d7ee11289f657aa9af826d8f0844221292a27a
[39/43] io_uring: don't pass restart error codes to userspace
commit: e1a64bc64897f4c2fe460922c345f2ed8f889b5f
[40/43] io_uring/net: don't retry recv on uncommitted buffers
commit: 8444de681a179e41a4eb92d5bbe770a0677074a6
[41/43] io_uring/timeout: no ABS update of multishot timeouts
commit: b088ae34d4da730dc647261b893a6eb622fe5e1f
[42/43] io_uring/timeout: don't end a linked timeout with 1
commit: c74193430febdd120e35936dda3e5aeec3a54405
[43/43] io_uring: look up the file before queueing to io-wq
commit: 98bc044d5a5374f8d3605fe55a8e2df6d6fc3f82Best regards,
Signature · 1 line
Jens Axboe