[PATCH v2 0/3] io_uring: mshot read fix for buffer size changes

[PATCH v2 0/3] io_uring: mshot read fix for buffer size changes

[Dylan Yudaken]

Hi,

This series fixes a bug (see [1] for liburing patch)
where the used buffer size is clamped to the minimum of all the
previous buffers selected.

It also as part of this forces the multishot read API to set addr &
len to 0.
len should probably have some accounting post-processing if it has
meaning to set it to non-zero, but I think for a new API it is simpler
to overly-constrain it upfront?

addr is useful to force to zero as it will allow some more bits to be
used in `struct io_rw`, which is otherwise full.

v2:
 - apply comments
 - add a patch for io_kbuf_recycle to show if it did anything

[1]: https://github.com/axboe/liburing/pull/981

Dylan Yudaken (3):
  io_uring: indicate if io_kbuf_recycle did recycle anything
  io_uring: do not allow multishot read to set addr or len
  io_uring: do not clamp read length for multishot read

 io_uring/kbuf.c |  6 +++---
 io_uring/kbuf.h | 13 ++++++++-----
 io_uring/rw.c   | 13 ++++++++++++-
 3 files changed, 23 insertions(+), 9 deletions(-)


base-commit: f688944cfb810986c626cb13d95bc666e5c8a36c
-- 
2.41.0

[PATCH v2 1/3] io_uring: indicate if io_kbuf_recycle did recycle anything

[Dylan Yudaken]

It can be useful to know if io_kbuf_recycle did actually recycle the
buffer on the request, or if it left the request alone.

Signed-off-by: Dylan Yudaken <dyudaken@gmail.com>
---
 io_uring/kbuf.c |  6 +++---
 io_uring/kbuf.h | 13 ++++++++-----
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/io_uring/kbuf.c b/io_uring/kbuf.c
index fea06810b43d..a1e4239c7d75 100644
--- a/io_uring/kbuf.c
+++ b/io_uring/kbuf.c
@@ -52,7 +52,7 @@ static int io_buffer_add_list(struct io_ring_ctx *ctx,
 	return xa_err(xa_store(&ctx->io_bl_xa, bgid, bl, GFP_KERNEL));
 }
 
-void io_kbuf_recycle_legacy(struct io_kiocb *req, unsigned issue_flags)
+bool io_kbuf_recycle_legacy(struct io_kiocb *req, unsigned issue_flags)
 {
 	struct io_ring_ctx *ctx = req->ctx;
 	struct io_buffer_list *bl;
@@ -65,7 +65,7 @@ void io_kbuf_recycle_legacy(struct io_kiocb *req, unsigned issue_flags)
 	 * multiple use.
 	 */
 	if (req->flags & REQ_F_PARTIAL_IO)
-		return;
+		return false;
 
 	io_ring_submit_lock(ctx, issue_flags);
 
@@ -76,7 +76,7 @@ void io_kbuf_recycle_legacy(struct io_kiocb *req, unsigned issue_flags)
 	req->buf_index = buf->bgid;
 
 	io_ring_submit_unlock(ctx, issue_flags);
-	return;
+	return true;
 }
 
 unsigned int __io_put_kbuf(struct io_kiocb *req, unsigned issue_flags)
diff --git a/io_uring/kbuf.h b/io_uring/kbuf.h
index d14345ef61fc..f2d615236b2c 100644
--- a/io_uring/kbuf.h
+++ b/io_uring/kbuf.h
@@ -53,11 +53,11 @@ int io_unregister_pbuf_ring(struct io_ring_ctx *ctx, void __user *arg);
 
 unsigned int __io_put_kbuf(struct io_kiocb *req, unsigned issue_flags);
 
-void io_kbuf_recycle_legacy(struct io_kiocb *req, unsigned issue_flags);
+bool io_kbuf_recycle_legacy(struct io_kiocb *req, unsigned issue_flags);
 
 void *io_pbuf_get_address(struct io_ring_ctx *ctx, unsigned long bgid);
 
-static inline void io_kbuf_recycle_ring(struct io_kiocb *req)
+static inline bool io_kbuf_recycle_ring(struct io_kiocb *req)
 {
 	/*
 	 * We don't need to recycle for REQ_F_BUFFER_RING, we can just clear
@@ -80,8 +80,10 @@ static inline void io_kbuf_recycle_ring(struct io_kiocb *req)
 		} else {
 			req->buf_index = req->buf_list->bgid;
 			req->flags &= ~REQ_F_BUFFER_RING;
+			return true;
 		}
 	}
+	return false;
 }
 
 static inline bool io_do_buffer_select(struct io_kiocb *req)
@@ -91,12 +93,13 @@ static inline bool io_do_buffer_select(struct io_kiocb *req)
 	return !(req->flags & (REQ_F_BUFFER_SELECTED|REQ_F_BUFFER_RING));
 }
 
-static inline void io_kbuf_recycle(struct io_kiocb *req, unsigned issue_flags)
+static inline bool io_kbuf_recycle(struct io_kiocb *req, unsigned issue_flags)
 {
 	if (req->flags & REQ_F_BUFFER_SELECTED)
-		io_kbuf_recycle_legacy(req, issue_flags);
+		return io_kbuf_recycle_legacy(req, issue_flags);
 	if (req->flags & REQ_F_BUFFER_RING)
-		io_kbuf_recycle_ring(req);
+		return io_kbuf_recycle_ring(req);
+	return false;
 }
 
 static inline unsigned int __io_put_kbuf_list(struct io_kiocb *req,
-- 
2.41.0

[PATCH v2 2/3] io_uring: do not allow multishot read to set addr or len

[Dylan Yudaken]

For addr: this field is not used, since buffer select is forced.
But by forcing it to be zero it leaves open future uses of the field.

len is actually usable, you could imagine that you want to receive
multishot up to a certain length.
However right now this is not how it is implemented, and it seems
safer to force this to be zero.

Fixes: fc68fcda0491 ("io_uring/rw: add support for IORING_OP_READ_MULTISHOT")
Signed-off-by: Dylan Yudaken <dyudaken@gmail.com>
---
 io_uring/rw.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/io_uring/rw.c b/io_uring/rw.c
index 9e3e56b74e35..8321e004ab13 100644
--- a/io_uring/rw.c
+++ b/io_uring/rw.c
@@ -143,6 +143,7 @@ int io_prep_rw_fixed(struct io_kiocb *req, const struct io_uring_sqe *sqe)
  */
 int io_read_mshot_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 {
+	struct io_rw *rw = io_kiocb_to_cmd(req, struct io_rw);
 	int ret;
 
 	/* must be used with provided buffers */
@@ -153,6 +154,9 @@ int io_read_mshot_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 	if (unlikely(ret))
 		return ret;
 
+	if (rw->addr || rw->len)
+		return -EINVAL;
+
 	req->flags |= REQ_F_APOLL_MULTISHOT;
 	return 0;
 }
-- 
2.41.0

[PATCH v2 3/3] io_uring: do not clamp read length for multishot read

[Dylan Yudaken]

When doing a multishot read, the code path reuses the old read
paths. However this breaks an assumption built into those paths,
namely that struct io_rw::len is available for reuse by __io_import_iovec.

For multishot this results in len being set for the first receive
call, and then subsequent calls are clamped to that buffer length
incorrectly.

Instead keep len as zero after recycling buffers, to reuse the full
buffer size of the next selected buffer.

Fixes: fc68fcda0491 ("io_uring/rw: add support for IORING_OP_READ_MULTISHOT")
Signed-off-by: Dylan Yudaken <dyudaken@gmail.com>
---
 io_uring/rw.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/io_uring/rw.c b/io_uring/rw.c
index 8321e004ab13..64390d4e20c1 100644
--- a/io_uring/rw.c
+++ b/io_uring/rw.c
@@ -912,6 +912,7 @@ int io_read(struct io_kiocb *req, unsigned int issue_flags)
 
 int io_read_mshot(struct io_kiocb *req, unsigned int issue_flags)
 {
+	struct io_rw *rw = io_kiocb_to_cmd(req, struct io_rw);
 	unsigned int cflags = 0;
 	int ret;
 
@@ -928,7 +929,12 @@ int io_read_mshot(struct io_kiocb *req, unsigned int issue_flags)
 	 * handling arm it.
 	 */
 	if (ret == -EAGAIN) {
-		io_kbuf_recycle(req, issue_flags);
+		/*
+		 * Reset rw->len to 0 again to avoid clamping future mshot
+		 * reads, in case the buffer size varies.
+		 */
+		if (io_kbuf_recycle(req, issue_flags))
+			rw->len = 0;
 		return -EAGAIN;
 	}
 
@@ -941,6 +947,7 @@ int io_read_mshot(struct io_kiocb *req, unsigned int issue_flags)
 		 * jump to the termination path. This request is then done.
 		 */
 		cflags = io_put_kbuf(req, issue_flags);
+		rw->len = 0; /* similarly to above, reset len to 0 */
 
 		if (io_fill_cqe_req_aux(req,
 					issue_flags & IO_URING_F_COMPLETE_DEFER,
-- 
2.41.0

Re: [PATCH v2 0/3] io_uring: mshot read fix for buffer size changes

[Jens Axboe]

On Mon, 06 Nov 2023 20:39:06 +0000, Dylan Yudaken wrote:
> This series fixes a bug (see [1] for liburing patch)
> where the used buffer size is clamped to the minimum of all the
> previous buffers selected.
> 
> It also as part of this forces the multishot read API to set addr &
> len to 0.
> len should probably have some accounting post-processing if it has
> meaning to set it to non-zero, but I think for a new API it is simpler
> to overly-constrain it upfront?
> 
> [...]

Applied, thanks!

[1/3] io_uring: indicate if io_kbuf_recycle did recycle anything
      commit: 89d528ba2f8281de61163c6b62e598b64d832175
[2/3] io_uring: do not allow multishot read to set addr or len
      commit: 49fbe99486786661994a55ced855c31d966bbdf0
[3/3] io_uring: do not clamp read length for multishot read
      commit: e53759298a7d7e98c3e5c2440d395d19cea7d6bf

Best regards,
-- 
Jens Axboe