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

[Jens Axboe <axboe@xxxxxxxxx>]

On 11/5/23 3:30 PM, Dylan Yudaken wrote:
> 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@xxxxxxxxx>
> ---
>  io_uring/rw.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/io_uring/rw.c b/io_uring/rw.c
> index 1c76de483ef6..ea86498d8769 100644
> --- a/io_uring/rw.c
> +++ b/io_uring/rw.c
> @@ -111,6 +111,13 @@ int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>  	rw->len = READ_ONCE(sqe->len);
>  	rw->flags = READ_ONCE(sqe->rw_flags);
>  
> +	if (req->opcode == IORING_OP_READ_MULTISHOT) {
> +		if (rw->addr)
> +			return -EINVAL;
> +		if (rw->len)
> +			return -EINVAL;
> +	}

Should we just put these in io_read_mshot_prep() instead? Ala the below.
In general I think it'd be nice to have a core prep_rw, and then each
variant will have its own prep. Then we can get away from random opcode
checking in there.

I do agree with the change in general, just think we can tweak it a bit
to make it a bit cleaner.

diff --git a/io_uring/rw.c b/io_uring/rw.c
index 1c76de483ef6..635a1bf5df70 100644
--- a/io_uring/rw.c
+++ b/io_uring/rw.c
@@ -129,6 +129,7 @@ int io_prep_rw(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 */
@@ -139,6 +140,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;
 }

-- 
Jens Axboe