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

[Gabriel Krisman Bertazi <krisman@suse.de>]

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