Skip to contentSkip to conversation

io_uring fixes for 6.18-stable

PATCH New Jens Axboe 1 participant 12 messages Nov 4 Stable branch

lore
Share & tools
lore

Jens Axboe

author
#0 lore raw

cover io_uring fixes for 6.18-stable

Hi Greg,

Here are io_uring fixes for 6.18-stable.

They apply on top of v6.18.60 with the current queue-6.18 applied (stable-queue 6575ff04f2fd). They don't depend on anything in the queue, and apply to v6.18.60 alone as well.

"io_uring/io-wq: save worker type before dropping its reference" is already in queue-6.18, so it's not included here.

Patches 5, 6, 7 and 9 needed changes to apply to 6.18, the [axboe: ...] note in each says what was changed.

Cheng Yongkang (2):
io_uring/timeout: count cancelled timeouts in cq_timeouts io_uring/net: fix multishot recv length limit accounting

Chris Mason (4):
io_uring/rsrc: charge buffer arrays and nodes to memcg
io_uring: charge tctx nodes and personalities
io_uring: free XArray nodes left by a failed store io_uring/sqpoll: don't wait for the thread to exit

Daan De Meyer (1):
io_uring/net: don't retry a non-blocking connect() that fails with
-EAGAIN

Josef Bacik (1):
io_uring/io-wq: no new workers once the exit has begun

Kyle Zeng (1):
io_uring: defer foreign io-wq enqueues to task_work

Yichong Chen (2):
io_uring/kbuf: advance head when a pickup empties the buffer ring io_uring/net: keep recvmsg buffer select length within MAX_RW_COUNT

 io_uring/io-wq.c    |  6 ++++++
 io_uring/io_uring.c | 30 +++++++++++++++++++++++++++---
 io_uring/kbuf.c     |  4 +++-
 io_uring/net.c      | 23 +++++++++++++++++++----
 io_uring/rsrc.c     |  6 +++---
 io_uring/sqpoll.c   | 28 +++++++++++++++++++++++-----
 io_uring/sqpoll.h   |  3 ++-
 io_uring/tctx.c     |  6 ++++--
 io_uring/timeout.c  |  3 +++
 9 files changed, 90 insertions(+), 19 deletions(-)
Signature · 1 line
2.56.0
author
#1 lore raw

PATCH 1/11 io_uring/kbuf: advance head when a pickup empties the buffer ring

From: Yichong Chen <[email protected]>

Commit 8742f4b5a472c347fe568a73468a2d9e4d630b33 upstream.

io_ring_buffers_peek() flags the request with REQ_F_BL_EMPTY if the buffers it picked were the last ones available, comparing head to tail. When the loop breaks because max_len was exhausted the buffer was picked, but head is left pointing at it, so the flag isn't set if it was the last buffer. A bundle receive then retries, finds no buffer, and the request, which has received data, is marked as failed on account of the -ENOBUFS. That severs a link, and ends a multishot.

Fixes 35c8711c8fc4 ("io_uring/kbuf: add helpers for getting/peeking multiple buffers")
---
 io_uring/kbuf.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
io_uring/kbuf.c +3 -1
index e1f4ee872740..c091c3c462e1 100644
@@ -314,8 +314,10 @@ static int io_ring_buffers_peek(struct io_kiocb *req, struct buf_sel_arg *arg,
 
 		arg->out_len += len;
 		arg->max_len -= len;
-		if (!arg->max_len)
+		if (!arg->max_len) {
+			head++;
 			break;
+		}
 
 		buf = io_ring_head_to_buf(br, ++head, bl->mask);
 	} while (--nr_iovs);
Signature · 1 line
2.56.0
author
#2 lore raw

PATCH 2/11 io_uring/net: keep recvmsg buffer select length within MAX_RW_COUNT

In reply to Jens Axboe

From: Yichong Chen <[email protected]>

Commit 18d3f56eb6e6500a65d81cf265f0fe74fd9ecab6 upstream.

io_recvmsg() takes the transfer length from the ring provided buffer when sqe->len is 0, and such a buffer can be up to 4G. min_ret is an int, so 2G or more makes it negative and the (ret < min_ret) block is skipped, and the oversized length reaches the socket layer unchanged: an empty socket then completes with 0 instead of arming poll, and a short read is reported as complete. Clamp the length to MAX_RW_COUNT, as io_recv() does.

Fixes c7fb19428d67 ("io_uring: add support for ring mapped supplied buffers")
---
 io_uring/net.c | 1 +
 1 file changed, 1 insertion(+)
io_uring/net.c +1 -0
index 163ded47602d..622d40336f5a 100644
@@ -1066,6 +1066,7 @@ int io_recvmsg(struct io_kiocb *req, unsigned int issue_flags)
 		sel = io_buffer_select(req, &len, sr->buf_group, issue_flags);
 		if (!sel.addr)
 			return -ENOBUFS;
+		len = min_t(size_t, len, MAX_RW_COUNT);
 
 		if (req->flags & REQ_F_APOLL_MULTISHOT) {
 			ret = io_recvmsg_prep_multishot(kmsg, sr, &sel.addr, &len);
Signature · 1 line
2.56.0
author
#3 lore raw

PATCH 3/11 io_uring/net: don't retry a non-blocking connect() that fails with -EAGAIN

In reply to Jens Axboe

From: Daan De Meyer <[email protected]>

Commit 71537ed811107c514c0342e98902d489d6d2b7cd upstream.

IORING_OP_CONNECT on an O_NONBLOCK AF_UNIX stream socket retries a connect that fails with -EAGAIN because the listener's backlog is full. Polling the client socket doesn't help: an unconnected stream socket reports EPOLLOUT and EPOLLHUP right away, before the backlog drains. So io_uring retries the connect until the apoll retry limit is hit, then punts the request to io-wq. With a full backlog, io_connect() runs 128 times in the submitting task and once more in a new io-wq worker. That worker stays in the task's thread group, so a later unshare(CLONE_NEWUSER) fails with -EINVAL.

None of this changes the result. __sys_connect_file() adds the socket's own O_NONBLOCK to every attempt, so the attempt from io-wq fails with -EAGAIN as well. That -EAGAIN becomes the CQE result, and connect(2) returns the same -EAGAIN.

Let's complete the request with -EAGAIN right away when the socket has O_NONBLOCK set, like connect(2) does. io_accept() already completes -EAGAIN this way for IORING_ACCEPT_DONTWAIT. -EINPROGRESS still waits for the connection to complete. A blocking socket still ends up in io-wq, where the connect sleeps until the listener accepts.

---
 io_uring/net.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
io_uring/net.c +11 -0
index 622d40336f5a..e78ec89ca5dd 100644
@@ -1864,6 +1864,17 @@ int io_connect(struct io_kiocb *req, unsigned int issue_flags)
 			if (connect->seen_econnaborted)
 				goto out;
 			connect->seen_econnaborted = true;
+		} else if (req->file->f_flags & O_NONBLOCK) {
+			/*
+			 * __sys_connect_file() adds O_NONBLOCK from the socket
+			 * to every attempt, so a retry from io-wq fails with
+			 * -EAGAIN as well. A poll on the socket doesn't wait
+			 * until a retry can succeed. For example, an AF_UNIX
+			 * client reports EPOLLOUT right away while the
+			 * listener's backlog is full. Complete the request
+			 * with -EAGAIN, like connect(2) does.
+			 */
+			goto out;
 		}
 		return -EAGAIN;
 	}
Signature · 1 line
2.56.0
author
#4 lore raw

PATCH 4/11 io_uring/io-wq: no new workers once the exit has begun

In reply to Jens Axboe

From: Josef Bacik <[email protected]>

Commit 47c68f925062a6013235d06add85a59f7d91e0fb upstream.

io_wq_exit_start() sets IO_WQ_BIT_EXIT, but worker creation task_work queued before that stays queued until io_wq_exit_workers() cancels it. In between the task runs its task_work while canceling requests, and a worker can still get created.

That's a problem for exec. begin_new_exec() calls io_uring_task_cancel() after de_thread(), when it's supposedly the only thread left, and io_wq_exit_workers() only waits for ->worker_done, which a worker completes before do_exit(). So the exiting worker is still in the thread group, using the mm, files and sighand, while exec replaces them. Easy to hit with a signal pending when create_io_thread() fails and the retry gets queued right before execve().

Cancel the queued creation task_work as soon as the exit bit is set. Pending work that isn't otherwise canceled now ends with -ECANCELED when the io-wq is destroyed, rather than depending on whether the task happened to run its task_work first.

Fixes 685fe7feedb9 ("io-wq: eliminate the need for a manager thread")

[axboe: the barrier is already upstream, see 0e5663856b69, only the
cancel is new]

---
 io_uring/io-wq.c | 6 ++++++
 1 file changed, 6 insertions(+)
io_uring/io-wq.c +6 -0
index 7d89aa79d06e..fd53297af26e 100644
@@ -1412,6 +1412,12 @@ void io_wq_exit_start(struct io_wq *wq)
 	set_bit(IO_WQ_BIT_EXIT, &wq->state);
 	/* Pairs with task_work_add() in io_queue_worker_create(). */
 	smp_mb__after_atomic();
+	/*
+	 * No more workers from here on. The caller is going to run its task
+	 * work, and on exec a thread that is created now would still be
+	 * around, exiting, by the time that the caller must be on its own.
+	 */
+	io_wq_cancel_tw_create(wq);
 }
 
 static void io_wq_cancel_tw_create(struct io_wq *wq)
Signature · 1 line
2.56.0
author
#5 lore raw

PATCH 5/11 io_uring/rsrc: charge buffer arrays and nodes to memcg

In reply to Jens Axboe

From: Chris Mason <[email protected]>

Commit 075d5837bafc317e796548f721b8fbf7add6821d upstream.

The io_mapped_ubuf of a registered buffer has one bio_vec per page, unless the pages coalesce into huge pages. Huge pages only count against RLIMIT_MEMLOCK once per ring, so a buffer that maps the same 2MB folio over and over in a way that defeats coalescing gets a full bvec array per registration while only the one folio counts as locked. 1GB of that registered in 100 slots took several times a 64MB memory.max, with 2.6MB charged.

Allocate the io_mapped_ubuf and the io_rsrc_node with GFP_KERNEL_ACCOUNT. An object reused from the ring caches stays charged to whoever allocated it first, same as the io_kiocb cache.

Fixes de2939388be5 ("io_uring: improve registered buffer accounting for huge pages")

[axboe: 6.18 uses kvmalloc() + struct_size_t() rather than kvmalloc_flex()]

---
 io_uring/rsrc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
io_uring/rsrc.c +3 -3
index 8d28c6bfebb1..41e5755bd9aa 100644
@@ -110,9 +110,9 @@ static struct io_mapped_ubuf *io_alloc_imu(struct io_ring_ctx *ctx,
 					   int nr_bvecs)
 {
 	if (nr_bvecs <= IO_CACHED_BVECS_SEGS)
-		return io_cache_alloc(&ctx->imu_cache, GFP_KERNEL);
+		return io_cache_alloc(&ctx->imu_cache, GFP_KERNEL_ACCOUNT);
 	return kvmalloc(struct_size_t(struct io_mapped_ubuf, bvec, nr_bvecs),
-			GFP_KERNEL);
+			GFP_KERNEL_ACCOUNT);
 }
 
 static void io_free_imu(struct io_ring_ctx *ctx, struct io_mapped_ubuf *imu)
@@ -140,7 +140,7 @@ struct io_rsrc_node *io_rsrc_node_alloc(struct io_ring_ctx *ctx, int type)
 {
 	struct io_rsrc_node *node;
 
-	node = io_cache_alloc(&ctx->node_cache, GFP_KERNEL);
+	node = io_cache_alloc(&ctx->node_cache, GFP_KERNEL_ACCOUNT);
 	if (node) {
 		node->type = type;
 		node->refs = 1;
Signature · 1 line
2.56.0
author
#6 lore raw

PATCH 6/11 io_uring: charge tctx nodes and personalities

In reply to Jens Axboe

From: Chris Mason <[email protected]>

Commit 0b34e7b20a48710b6f53716bf7030920b69fdbdd upstream.

The io_tctx_node allocated the first time a task touches a ring, and the XArray nodes behind tctx->xa and ctx->personalities, are plain GFP_KERNEL. tctx->xa is indexed by the ring pointer, so nearly every entry needs nodes of its own, and a task gets one entry per ring it has used. 642 threads on 4000 rings held 2.3GB with 59MB charged against a 64MB memory.max.

Allocate the node with GFP_KERNEL_ACCOUNT and set XA_FLAGS_ACCOUNT on both XArrays.

Fixes 0f2122045b94 ("io_uring: don't rely on weak ->files references")

[axboe: 6.18 has kmalloc() for the node, inline in __io_uring_add_tctx_node()]

---
 io_uring/io_uring.c | 2 +-
 io_uring/tctx.c     | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)
io_uring/io_uring.c +1 -1
index 04a90824c190..f1cd0e2b5b68 100644
@@ -332,7 +332,7 @@ static __cold struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
 	if (ret)
 		goto free_ref;
 	init_completion(&ctx->ref_comp);
-	xa_init_flags(&ctx->personalities, XA_FLAGS_ALLOC1);
+	xa_init_flags(&ctx->personalities, XA_FLAGS_ALLOC1 | XA_FLAGS_ACCOUNT);
 	mutex_init(&ctx->uring_lock);
 	init_waitqueue_head(&ctx->cq_wait);
 	init_waitqueue_head(&ctx->poll_wq);
io_uring/tctx.c +2 -2
index 03c278aa5812..bf851ba5ca1a 100644
@@ -91,7 +91,7 @@ __cold int io_uring_alloc_task_context(struct task_struct *task,
 	}
 
 	tctx->task = task;
-	xa_init(&tctx->xa);
+	xa_init_flags(&tctx->xa, XA_FLAGS_ACCOUNT);
 	init_waitqueue_head(&tctx->wait);
 	atomic_set(&tctx->in_cancel, 0);
 	atomic_set(&tctx->inflight_tracked, 0);
@@ -131,7 +131,7 @@ int __io_uring_add_tctx_node(struct io_ring_ctx *ctx)
 	if (tctx->io_wq)
 		io_wq_set_exit_on_idle(tctx->io_wq, false);
 	if (!xa_load(&tctx->xa, (unsigned long)ctx)) {
-		node = kmalloc(sizeof(*node), GFP_KERNEL);
+		node = kmalloc(sizeof(*node), GFP_KERNEL_ACCOUNT);
 		if (!node)
 			return -ENOMEM;
 		node->ctx = ctx;
Signature · 1 line
2.56.0
author
#7 lore raw

PATCH 7/11 io_uring: free XArray nodes left by a failed store

In reply to Jens Axboe

From: Chris Mason <[email protected]>

Commit 51bab09b7ca1a9a6dd9303a3b7ee346f8128c963 upstream.

A failed xa_store() can leave nodes without entries in the tree, and erasing every entry doesn't free those. An XArray needs xa_destroy() before it goes away, even an empty one. That was dropped for tctx->xa in commit ca6484cd308a ("io_uring: no need to call xa_destroy() on empty xarray") over a lockdep report from freeing the tctx in softirq context, and ctx->personalities never had it.

Now that the nodes are charged, a store can fail at memory.max and the leaked nodes stay charged. Call xa_destroy() in io_uring_clean_tctx(), which the task runs itself in process context before the tctx is freed, in the error path of __io_uring_add_tctx_node(), and for ctx->personalities in io_ring_ctx_free().

Fixes ca6484cd308a ("io_uring: no need to call xa_destroy() on empty xarray")

[axboe: 6.18 installs the tctx before adding the node and has no error path that frees it, drop that hunk. There's no ctx->hpage_acct either]

---
 io_uring/io_uring.c | 1 +
 io_uring/tctx.c     | 2 ++
 2 files changed, 3 insertions(+)
io_uring/io_uring.c +1 -0
index f1cd0e2b5b68..ac097a9ac8c0 100644
@@ -3039,6 +3039,7 @@ static __cold void io_ring_ctx_free(struct io_ring_ctx *ctx)
 	io_napi_free(ctx);
 	kvfree(ctx->cancel_table.hbs);
 	xa_destroy(&ctx->io_bl_xa);
+	xa_destroy(&ctx->personalities);
 	kfree(ctx);
 }
 
io_uring/tctx.c +2 -0
index bf851ba5ca1a..0e4445d3a83c 100644
@@ -206,6 +206,8 @@ __cold void io_uring_clean_tctx(struct io_uring_task *tctx)
 		io_uring_del_tctx_node(index);
 		cond_resched();
 	}
+	/* A failed xa_store() leaves nodes with no entry behind. */
+	xa_destroy(&tctx->xa);
 	if (wq) {
 		/*
 		 * Must be after io_uring_del_tctx_node() (removes nodes under
Signature · 1 line
2.56.0
author
#8 lore raw

PATCH 8/11 io_uring/sqpoll: don't wait for the thread to exit

In reply to Jens Axboe

From: Chris Mason <[email protected]>

Commit 360395e8dbf9d23579f74b84a1af11190813b0f8 upstream.

io_sq_thread_stop() wakes the SQPOLL thread and waits for it to exit. The thread belongs to the thread group that created the ring, so a SIGSTOP or the cgroup freezer holds it in get_signal(), and the wake up doesn't change that. io_ring_exit_work() then sleeps in that wait until someone continues or kills the process. iou_exit has 64 slots, so 64 closed SQPOLL rings of a stopped process keep every other ring from being freed, for everyone.

Don't wait. Set the stop bit, wake the thread, and let it exit when it gets to run again. The sqd can now outlive its last ring, so give it a second refcount with one reference for the rings and one for the thread, and free it with the last one. The thread can't reach a freed ring, rings leave ctx_list before dropping their reference, and the io-wq hash is refcounted.

Fixes 16efa4fce3b7 ("io_uring: allow IO worker threads to be frozen")
---
 io_uring/sqpoll.c | 28 +++++++++++++++++++++++-----
 io_uring/sqpoll.h |  3 ++-
 2 files changed, 25 insertions(+), 6 deletions(-)
io_uring/sqpoll.c +23 -5
index 448fa81ed87e..406029c14fbc 100644
@@ -62,6 +62,24 @@ void io_sq_thread_park(struct io_sq_data *sqd)
 	}
 }
 
+/*
+ * The sqd has two holders: its rings, as one, and its thread. Neither waits
+ * for the other to let go. The one that lets go last frees the sqd.
+ */
+static void io_sq_data_release(struct io_sq_data *sqd)
+{
+	if (refcount_dec_and_test(&sqd->free_refs))
+		kfree(sqd);
+}
+
+/*
+ * Ask the thread to exit, and don't wait for it. A thread that is stopped or
+ * frozen exits only when a user lets it run again, and the exit work of the
+ * ring would keep its place in the workqueue until then. All rings have left
+ * ctx_list by now and have no request left, so the thread can reach none of
+ * them. What it still uses is its own or counted: the sqd by free_refs, the
+ * hash of its io-wq by hash->refs.
+ */
 void io_sq_thread_stop(struct io_sq_data *sqd)
 {
 	struct task_struct *tsk;
@@ -76,7 +94,6 @@ void io_sq_thread_stop(struct io_sq_data *sqd)
 		wake_up_process(tsk);
 	}
 	mutex_unlock(&sqd->lock);
-	wait_for_completion(&sqd->exited);
 }
 
 void io_put_sq_data(struct io_sq_data *sqd)
@@ -85,7 +102,7 @@ void io_put_sq_data(struct io_sq_data *sqd)
 		WARN_ON_ONCE(atomic_read(&sqd->park_pending));
 
 		io_sq_thread_stop(sqd);
-		kfree(sqd);
+		io_sq_data_release(sqd);
 	}
 }
 
@@ -159,10 +176,10 @@ static struct io_sq_data *io_get_sq_data(struct io_uring_params *p,
 
 	atomic_set(&sqd->park_pending, 0);
 	refcount_set(&sqd->refs, 1);
+	refcount_set(&sqd->free_refs, 2);
 	INIT_LIST_HEAD(&sqd->ctx_list);
 	mutex_init(&sqd->lock);
 	init_waitqueue_head(&sqd->wait);
-	init_completion(&sqd->exited);
 	return sqd;
 }
 
@@ -429,7 +446,7 @@ static int io_sq_thread(void *data)
 	io_run_task_work();
 	mutex_unlock(&sqd->lock);
 err_out:
-	complete(&sqd->exited);
+	io_sq_data_release(sqd);
 	do_exit(0);
 }
 
@@ -543,7 +560,8 @@ __cold int io_sq_offload_create(struct io_ring_ctx *ctx,
 	}
 	return 0;
 err_sqpoll:
-	complete(&ctx->sq_data->exited);
+	/* no thread was made: let go of its share */
+	io_sq_data_release(ctx->sq_data);
 err:
 	io_sq_thread_finish(ctx);
 	return ret;
io_uring/sqpoll.h +2 -1
index fd2f6f29b516..024a51dc3069 100644
@@ -2,6 +2,8 @@
 
 struct io_sq_data {
 	refcount_t		refs;
+	/* one for all the rings, one for the thread: the last put frees */
+	refcount_t		free_refs;
 	atomic_t		park_pending;
 	struct mutex		lock;
 
@@ -18,7 +20,6 @@ struct io_sq_data {
 
 	u64			work_time;
 	unsigned long		state;
-	struct completion	exited;
 };
 
 int io_sq_offload_create(struct io_ring_ctx *ctx, struct io_uring_params *p);
Signature · 1 line
2.56.0
author
#9 lore raw

PATCH 9/11 io_uring: defer foreign io-wq enqueues to task_work

In reply to Jens Axboe

From: Kyle Zeng <[email protected]>

Commit 3de16279b43dc285ed2222aba06cebdc82fdb28b upstream.

An IOPOLL ring without DEFER_TASKRUN can be reaped by a task other than the one that submitted the I/O. If a polled request is marked REQ_F_REISSUE, io_free_batch_list() calls io_queue_iowq() with the original tctx. Ordinary task exit can tear down tctx->io_wq after draining only the tracked requests. The live request pins the task, but not its io-wq, so the foreign task can race io_wq_put_and_exit() and enqueue through a freed or NULL queue.

Forward foreign userspace enqueues to the owning task before accessing tctx->io_wq. Use a dedicated task_work callback so reissues still go straight to io-wq, and fail them if task_work is running in cancellation mode. Preserve the existing PF_KTHREAD cancellation path. This also covers the other callers of io_queue_iowq() without changing the worker-owned enqueue path.

[axboe: 6.18 lacks 13b0742fd058, which removed io_req_queue_iowq(). Name the new task_work callback io_queue_iowq_foreign_tw() so it doesn't clash with the existing io_req_queue_iowq_tw()]

---
 io_uring/io_uring.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)
io_uring/io_uring.c +25 -2
index ac097a9ac8c0..4a9f3a0fe506 100644
@@ -483,6 +483,19 @@ static __always_inline bool io_assign_file(struct io_kiocb *req,
 	return !!req->file;
 }
 
+static void io_queue_iowq(struct io_kiocb *req);
+
+static void io_queue_iowq_foreign_tw(struct io_tw_req tw_req, io_tw_token_t tw)
+{
+	struct io_kiocb *req = tw_req.req;
+
+	io_tw_lock(req->ctx, tw);
+	if (unlikely(tw.cancel))
+		io_req_defer_failed(req, -ECANCELED);
+	else
+		io_queue_iowq(req);
+}
+
 static void io_queue_iowq(struct io_kiocb *req)
 {
 	struct io_uring_task *tctx = req->tctx;
@@ -490,10 +503,17 @@ static void io_queue_iowq(struct io_kiocb *req)
 	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);
+	if (current->flags & PF_KTHREAD)
+		goto cancel;
+
+	/* Only the submitter can safely access its io-wq during task exit. */
+	if (unlikely(current != tctx->task)) {
+		req->io_task_work.func = io_queue_iowq_foreign_tw;
+		io_req_task_work_add(req);
 		return;
 	}
+	if (!tctx->io_wq)
+		goto cancel;
 
 	/*
 	 * The file is looked up as late as possible, as it may be the result
@@ -522,6 +542,9 @@ static void io_queue_iowq(struct io_kiocb *req)
 
 	trace_io_uring_queue_async_work(req, io_wq_is_hashed(&req->work));
 	io_wq_enqueue(tctx->io_wq, &req->work);
+	return;
+cancel:
+	io_req_task_queue_fail(req, -ECANCELED);
 }
 
 static void io_req_queue_iowq_tw(struct io_tw_req tw_req, io_tw_token_t tw)
Signature · 1 line
2.56.0
author
#10 lore raw

PATCH 10/11 io_uring/timeout: count cancelled timeouts in cq_timeouts

In reply to Jens Axboe

From: Cheng Yongkang <[email protected]>

Commit 1c8b49851543f41fc8bb18bcd1f15cc93d14b530 upstream.

io_timeout_cancel() posts the cancelled timeout's -ECANCELED CQE without advancing ctx->cq_timeouts. io_timeout_fn() and io_kill_timeout() advance it for every timeout CQE they post, and io_flush_timeouts() computes the number of non-timeout completions as

seq = cached_cq_tail - cq_timeouts

to compare against each off timeout's target_seq. The cancelled timeout's CQE is therefore counted as a regular completion, so every cancellation makes an already armed off timeout fire one completion early.

A ring with a keep-alive timeout of off=4 completes it after two NOPs when one off=100 victim is cancelled through IORING_OP_ASYNC_CANCEL, and before any NOP when two victims are cancelled. The cancel CQE should count, but the cancelled timeouts' -ECANCELED CQEs must not: the expected counts are three and two NOPs respectively.

Advance cq_timeouts when io_timeout_extract() succeeds, under timeout_lock and before the -ECANCELED completion is queued, matching io_timeout_fn() and io_kill_timeout(). The -ENOENT and -EALREADY paths post no new timeout CQE and keep their accounting. Both the ASYNC_CANCEL and the TIMEOUT_REMOVE path go through io_timeout_cancel().

Fixes bfe68a221905 ("io_uring: off timeouts based only on completions")
Assisted-by LLM
---
 io_uring/timeout.c | 3 +++
 1 file changed, 3 insertions(+)
io_uring/timeout.c +3 -0
index dbbb9615ec9e..2637c8bb61a0 100644
@@ -327,6 +327,9 @@ int io_timeout_cancel(struct io_ring_ctx *ctx, struct io_cancel_data *cd)
 
 	raw_spin_lock_irq(&ctx->timeout_lock);
 	req = io_timeout_extract(ctx, cd);
+	if (!IS_ERR(req))
+		atomic_set(&ctx->cq_timeouts,
+			   atomic_read(&ctx->cq_timeouts) + 1);
 	raw_spin_unlock_irq(&ctx->timeout_lock);
 
 	if (IS_ERR(req))
Signature · 1 line
2.56.0
author
#11 lore raw

PATCH 11/11 io_uring/net: fix multishot recv length limit accounting

In reply to Jens Axboe

From: Cheng Yongkang <[email protected]>

Commit 3e3aebb7570ccdde5cc49c9bfc8fd3a436b71b0c upstream.

io_recv_finish() debits sr->mshot_total_len with min_t(int, sel->val, sr->mshot_total_len). sel->val is the cumulative byte count of the
request: on a bundle retry the receive path adds sr->done_io back to the return value, and the bundle branch in io_recv_finish() recovers the new bytes with this_ret = sel->val - sr->done_io. The debit therefore uses the wrong value after a bundle retry, and the int cast truncates a limit above INT_MAX.

For an IORING_OP_RECV multishot request with optlen above INT_MAX, the limit becomes negative when cast to int, so the first completion debits the entire limit and ends the multishot with IORING_RECV_MSHOT_DONE while the socket still holds data that covers the limit. With 120 bytes queued, a 64-byte buffer and optlen 0x80000000, the first 64-byte CQE lacks IORING_CQE_F_MORE and 56 bytes remain unread; the same setup with optlen 0x7fffffff posts 64 and 56 bytes with F_MORE.

After a bundle retry, the bytes already debited for the previous attempts are debited again, so the limit is exhausted early. With 1400 bytes queued, optlen 1000 and buffers of 600, 100, 100, 50, 50 and 200 bytes, the first attempt delivers 700 bytes and retries; the second delivers 200 more, but the debit accounts for 900 and finishes the request with a terminal 900-byte CQE while 500 bytes remain unread. The limit could have been reached.

Debit only the bytes received by this invocation, sel->val - sr->done_io, and compare as unsigned so a limit above INT_MAX is accounted correctly. Non-bundle multishot requests keep sr->done_io at zero, so their accounting is unchanged.

Fixes 0ebc9a7ecf6a ("io_uring/net: Support multishot receive len cap")
Assisted-by LLM
[axboe: fix-up near-by incorrect comment while in there]
---
 io_uring/net.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
io_uring/net.c +7 -4
index e78ec89ca5dd..e8d5d17fc754 100644
@@ -879,12 +879,15 @@ static inline bool io_recv_finish(struct io_kiocb *req,
 		cflags |= IORING_CQE_F_SOCK_NONEMPTY;
 
 	if (sel->val > 0 && sr->flags & IORING_RECV_MSHOT_LIM) {
+		unsigned int this_len = sel->val - sr->done_io;
+
 		/*
-		 * If sr->len hits zero, the limit has been reached. Mark
-		 * mshot as finished, and flag MSHOT_DONE as well to prevent
-		 * a potential bundle from being retried.
+		 * If sr->mshot_total_len hits zero, the limit has been reached.
+		 * Mark mshot as finished, and flag MSHOT_DONE as well to
+		 * prevent a potential bundle from being retried.
 		 */
-		sr->mshot_total_len -= min_t(int, sel->val, sr->mshot_total_len);
+		sr->mshot_total_len -= min_t(unsigned int, this_len,
+					     sr->mshot_total_len);
 		if (!sr->mshot_total_len) {
 			sr->flags |= IORING_RECV_MSHOT_DONE;
 			mshot_finished = true;
Signature · 1 line
2.56.0