[PATCH] io_uring/net: commit all consumed buffers on bundle recv retry

[PATCH] io_uring/net: commit all consumed buffers on bundle recv retry

[Aohan Mei]

From: Aohan Mei <henrymei@tencent.com>

For a bundle receive (IORING_RECVSEND_BUNDLE) using a provided
buffer ring, the initial issue peeks N buffers and defers the
commit via REQ_F_BUFFERS_COMMIT. If MSG_WAITALL (or a short read
on a streaming socket) yields a partial result,
io_net_kbuf_recyle() commits only the buffers covering that first
partial round and clears REQ_F_BUFFERS_COMMIT. On subsequent poll
armed retries, sel.buf_list is reset to NULL while
REQ_F_BUFFER_RING prevents re-selection, so neither the retry
rounds nor the final io_recv_finish() -> io_put_kbufs() ever
commit the remaining buffers that have already been written to.

This leaves bl->head accounting for only the first partial round
rather than all consumed buffers. Buffers that already hold
received data stay visible in the ring and are handed out again
to later requests, corrupting or dropping application data.

Defer the commit entirely: do not commit in io_net_kbuf_recyle(),
keeping REQ_F_BUFFERS_COMMIT set for the final completion; recover
the buffer list on retry so the final commit has it available; and
derive the number of committed buffers from the total received
bytes rather than the final round, so a short last read cannot
under-count.

Fixes: 41b70df5b38b ("io_uring/net: commit partial buffers on retry")
Reported-by: TencentOS Corvus AI <corvus@tencent.com>
Cc: stable@vger.kernel.org
Assisted-by: CodeBuddy:Kimi-K3
Signed-off-by: Aohan Mei <henrymei@tencent.com>
---
 io_uring/kbuf.c | 4 ++--
 io_uring/kbuf.h | 2 ++
 io_uring/net.c  | 7 ++++---
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/io_uring/kbuf.c b/io_uring/kbuf.c
index 7c309173dd19..c4bae8f00173 100644
--- a/io_uring/kbuf.c
+++ b/io_uring/kbuf.c
@@ -76,8 +76,8 @@ bool io_kbuf_commit(struct io_kiocb *req,
 	return true;
 }
 
-static inline struct io_buffer_list *io_buffer_get_list(struct io_ring_ctx *ctx,
-							unsigned int bgid)
+struct io_buffer_list *io_buffer_get_list(struct io_ring_ctx *ctx,
+					  unsigned int bgid)
 {
 	lockdep_assert_held(&ctx->uring_lock);
 
diff --git a/io_uring/kbuf.h b/io_uring/kbuf.h
index 401773e1ef80..7079f2dec9e0 100644
--- a/io_uring/kbuf.h
+++ b/io_uring/kbuf.h
@@ -71,6 +71,8 @@ struct io_br_sel io_buffer_select(struct io_kiocb *req, size_t *len,
 				  unsigned buf_group, unsigned int issue_flags);
 int io_buffers_select(struct io_kiocb *req, struct buf_sel_arg *arg,
 		      struct io_br_sel *sel, unsigned int issue_flags);
+struct io_buffer_list *io_buffer_get_list(struct io_ring_ctx *ctx,
+					  unsigned int bgid);
 int io_buffers_peek(struct io_kiocb *req, struct buf_sel_arg *arg,
 		    struct io_br_sel *sel);
 void io_destroy_buffers(struct io_ring_ctx *ctx);
diff --git a/io_uring/net.c b/io_uring/net.c
index fbe719d86c46..bff4514c0100 100644
--- a/io_uring/net.c
+++ b/io_uring/net.c
@@ -483,8 +483,6 @@ static int io_net_kbuf_recyle(struct io_kiocb *req, struct io_buffer_list *bl,
 			      struct io_async_msghdr *kmsg, int len)
 {
 	req->flags |= REQ_F_BL_NO_RECYCLE;
-	if (req->flags & REQ_F_BUFFERS_COMMIT)
-		io_kbuf_commit(req, bl, len, io_bundle_nbufs(kmsg, len));
 	return IOU_RETRY;
 }
 
@@ -877,7 +875,8 @@ static inline bool io_recv_finish(struct io_kiocb *req,
 	if (sr->flags & IORING_RECVSEND_BUNDLE) {
 		size_t this_ret = sel->val - sr->done_io;
 
-		cflags |= io_put_kbufs(req, this_ret, sel->buf_list, io_bundle_nbufs(kmsg, this_ret));
+		cflags |= io_put_kbufs(req, this_ret, sel->buf_list,
+				       io_bundle_nbufs(kmsg, sel->val));
 		if (sr->flags & IORING_RECV_RETRY)
 			cflags = req->cqe.flags | (cflags & CQE_F_MASK);
 		if (sr->mshot_len && sel->val >= sr->mshot_len)
@@ -1221,6 +1220,8 @@ int io_recv(struct io_kiocb *req, unsigned int issue_flags)
 			goto out_free;
 		}
 		sr->buf = NULL;
+	} else if (req->flags & REQ_F_BUFFER_RING) {
+		sel.buf_list = io_buffer_get_list(req->ctx, sr->buf_group);
 	}
 
 	kmsg->msg.msg_flags = 0;
-- 
2.43.7

Re: [PATCH] io_uring/net: commit all consumed buffers on bundle recv retry

[Gabriel Krisman Bertazi]

Aohan Mei <ljp1205831794@gmail.com> writes:

> This leaves bl->head accounting for only the first partial round
> rather than all consumed buffers. Buffers that already hold
> received data stay visible in the ring and are handed out again
> to later requests, corrupting or dropping application data.

Hi, Aohan. Do you have a reproducer for this issue?

-- 
Gabriel Krisman Bertazi

Re: [PATCH] io_uring/net: commit all consumed buffers on bundle recv retry

[Jiapeng Lin]

Hi Gabriel,

Yes. The trigger is deterministic (no races involved) and needs no
privileges. Recipe:

- AF_UNIX SOCK_STREAM socketpair; register a provided buffer ring of
  4 x 4KB buffers (bgid=1, IOBL_BUF_RING)
- submit IORING_OP_RECV with len=16KB, IOSQE_BUFFER_SELECT |
  IORING_RECVSEND_BUNDLE, msg_flags=MSG_WAITALL (and no MSG_DONTWAIT,
  no IOSQE_ASYNC)
- peer writes 1KB, then 2KB, then 13KB with ~150ms spacing, forcing
  two positive partial completions (via the io_net_retry WAITALL path)
  and one final completion
- after req1 completes (res=16384), read the ring head via
  IORING_REGISTER_PBUF_STATUS

Unfixed kernel: head=1 (only the first partial round was committed)
while 4 buffers were consumed. A follow-up 4KB RECV then completes
with buf_index=1, i.e. the kernel hands out buf[1] again although it
already holds data from req1.

Patched kernel: head=4, and the follow-up RECV fails with -ENOBUFS,
which is the expected behavior with all buffers correctly committed.

Verified as uid=1000 on v7.2-rc4 and on current master, KASAN builds,
dmesg clean -- consistent with this being a buffer accounting defect,
no memory-safety violation fires.

I have a self-contained C reproducer (~200 lines, raw syscalls, no
liburing dependency) implementing exactly the above. Happy to send it
your way off-list, or to reshape it into an io_uring selftest for
regression coverage if you prefer that on-list.

Thanks,
Aohan


Gabriel Krisman Bertazi <krisman@suse.de> 于2026年9月2日周三 23:31写道：
>
> Aohan Mei <ljp1205831794@gmail.com> writes:
>
> > This leaves bl->head accounting for only the first partial round
> > rather than all consumed buffers. Buffers that already hold
> > received data stay visible in the ring and are handed out again
> > to later requests, corrupting or dropping application data.
>
> Hi, Aohan. Do you have a reproducer for this issue?
>
> --
> Gabriel Krisman Bertazi

Re: [PATCH] io_uring/net: commit all consumed buffers on bundle recv retry

[Gabriel Krisman Bertazi]

Aohan Mei <ljp1205831794@gmail.com> writes:

> From: Aohan Mei <henrymei@tencent.com>
>
> For a bundle receive (IORING_RECVSEND_BUNDLE) using a provided
> buffer ring, the initial issue peeks N buffers and defers the
> commit via REQ_F_BUFFERS_COMMIT. If MSG_WAITALL (or a short read
> on a streaming socket) yields a partial result,
> io_net_kbuf_recyle() commits only the buffers covering that first
> partial round and clears REQ_F_BUFFERS_COMMIT. On subsequent poll
> armed retries, sel.buf_list is reset to NULL while
> REQ_F_BUFFER_RING prevents re-selection, so neither the retry
> rounds nor the final io_recv_finish() -> io_put_kbufs() ever
> commit the remaining buffers that have already been written to.
>
> This leaves bl->head accounting for only the first partial round
> rather than all consumed buffers. Buffers that already hold
> received data stay visible in the ring and are handed out again
> to later requests, corrupting or dropping application data.
>
> Defer the commit entirely: do not commit in io_net_kbuf_recyle(),
> keeping REQ_F_BUFFERS_COMMIT set for the final completion; recover
> the buffer list on retry so the final commit has it available; and
> derive the number of committed buffers from the total received
> bytes rather than the final round, so a short last read cannot
> under-count.
>
> Fixes: 41b70df5b38b ("io_uring/net: commit partial buffers on retry")
> Reported-by: TencentOS Corvus AI <corvus@tencent.com>
> Cc: stable@vger.kernel.org
> Assisted-by: CodeBuddy:Kimi-K3
> Signed-off-by: Aohan Mei <henrymei@tencent.com>
> ---
>  io_uring/kbuf.c | 4 ++--
>  io_uring/kbuf.h | 2 ++
>  io_uring/net.c  | 7 ++++---
>  3 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/io_uring/kbuf.c b/io_uring/kbuf.c
> index 7c309173dd19..c4bae8f00173 100644
> --- a/io_uring/kbuf.c
> +++ b/io_uring/kbuf.c
> @@ -76,8 +76,8 @@ bool io_kbuf_commit(struct io_kiocb *req,
>  	return true;
>  }
>  
> -static inline struct io_buffer_list *io_buffer_get_list(struct io_ring_ctx *ctx,
> -							unsigned int bgid)
> +struct io_buffer_list *io_buffer_get_list(struct io_ring_ctx *ctx,
> +					  unsigned int bgid)
>  {
>  	lockdep_assert_held(&ctx->uring_lock);
>  
> diff --git a/io_uring/kbuf.h b/io_uring/kbuf.h
> index 401773e1ef80..7079f2dec9e0 100644
> --- a/io_uring/kbuf.h
> +++ b/io_uring/kbuf.h
> @@ -71,6 +71,8 @@ struct io_br_sel io_buffer_select(struct io_kiocb *req, size_t *len,
>  				  unsigned buf_group, unsigned int issue_flags);
>  int io_buffers_select(struct io_kiocb *req, struct buf_sel_arg *arg,
>  		      struct io_br_sel *sel, unsigned int issue_flags);
> +struct io_buffer_list *io_buffer_get_list(struct io_ring_ctx *ctx,
> +					  unsigned int bgid);
>  int io_buffers_peek(struct io_kiocb *req, struct buf_sel_arg *arg,
>  		    struct io_br_sel *sel);
>  void io_destroy_buffers(struct io_ring_ctx *ctx);
> diff --git a/io_uring/net.c b/io_uring/net.c
> index fbe719d86c46..bff4514c0100 100644
> --- a/io_uring/net.c
> +++ b/io_uring/net.c
> @@ -483,8 +483,6 @@ static int io_net_kbuf_recyle(struct io_kiocb *req, struct io_buffer_list *bl,
>  			      struct io_async_msghdr *kmsg, int len)
>  {
>  	req->flags |= REQ_F_BL_NO_RECYCLE;
> -	if (req->flags & REQ_F_BUFFERS_COMMIT)
> -		io_kbuf_commit(req, bl, len, io_bundle_nbufs(kmsg, len));
>  	return IOU_RETRY;
>  }

Hi Aohan,

This looks fishy.  This function no longer commits and recycling is now
basically a nop. but io_send also relies on it.  Doesn't it break the
commit for io_send on a short write?


>  
> @@ -877,7 +875,8 @@ static inline bool io_recv_finish(struct io_kiocb *req,
>  	if (sr->flags & IORING_RECVSEND_BUNDLE) {
>  		size_t this_ret = sel->val - sr->done_io;
>  
> -		cflags |= io_put_kbufs(req, this_ret, sel->buf_list, io_bundle_nbufs(kmsg, this_ret));
> +		cflags |= io_put_kbufs(req, this_ret, sel->buf_list,
> +				       io_bundle_nbufs(kmsg, sel->val));
>  		if (sr->flags & IORING_RECV_RETRY)
>  			cflags = req->cqe.flags | (cflags & CQE_F_MASK);
>  		if (sr->mshot_len && sel->val >= sr->mshot_len)
> @@ -1221,6 +1220,8 @@ int io_recv(struct io_kiocb *req, unsigned int issue_flags)
>  			goto out_free;
>  		}
>  		sr->buf = NULL;
> +	} else if (req->flags & REQ_F_BUFFER_RING) {
> +		sel.buf_list = io_buffer_get_list(req->ctx, sr->buf_group);

I'm not sure, but during a retry, you can drop the ctx lock.  are you
guaranteed to get the same buffer you had before?

-- 
Gabriel Krisman Bertazi