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

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

[Dylan Yudaken]

Hi,

This series fixes a bug (will send a liburing patch separately showing
it) 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.

Dylan Yudaken (2):
  io_uring: do not allow multishot read to set addr or len
  io_uring: do not clamp read length for multishot read

 io_uring/rw.c | 9 +++++++++
 1 file changed, 9 insertions(+)

-- 
2.41.0

[PATCH 1/2] 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 | 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;
+	}
+
 	/* Have to do this validation here, as this is in io_read() rw->len might
 	 * have chanaged due to buffer selection
 	 */
-- 
2.41.0

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

[Jens Axboe]

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@gmail.com>
> ---
>  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

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

[Jens Axboe]

On 11/6/23 7:32 AM, Jens Axboe wrote:
> 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@gmail.com>
>> ---
>>  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.

Sent out two cleanups that take it in this direction in general, fwiw.

-- 
Jens Axboe

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

[Dylan Yudaken]

On Mon, Nov 6, 2023 at 2:51 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 11/6/23 7:32 AM, Jens Axboe wrote:
> > 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@gmail.com>
> >> ---
> >>  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.
>
> Sent out two cleanups that take it in this direction in general, fwiw.

Yes - I think this approach is better, will rebase on these

[PATCH 2/2] 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.

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

diff --git a/io_uring/rw.c b/io_uring/rw.c
index ea86498d8769..b7f7fbc28032 100644
--- a/io_uring/rw.c
+++ b/io_uring/rw.c
@@ -417,6 +417,8 @@ static struct iovec *__io_import_iovec(int ddir, struct io_kiocb *req,
 
 	if (!io_issue_defs[opcode].vectored || req->flags & REQ_F_BUFFER_SELECT) {
 		if (io_do_buffer_select(req)) {
+			if (opcode == IORING_OP_READ_MULTISHOT)
+				sqe_len = 0;
 			buf = io_buffer_select(req, &sqe_len, issue_flags);
 			if (!buf)
 				return ERR_PTR(-ENOBUFS);
-- 
2.41.0

Re: [PATCH 2/2] io_uring: do not clamp read length for multishot read

[Jens Axboe]

On 11/5/23 3:30 PM, Dylan Yudaken wrote:
> 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.

Should we just reset this to 0 always in io_read_mshot()? And preferably
with a comment added as well as to why that is necessary to avoid
repeated clamping.

-- 
Jens Axboe

Re: [PATCH 2/2] io_uring: do not clamp read length for multishot read

[Dylan Yudaken]

On Mon, Nov 6, 2023 at 2:46 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 11/5/23 3:30 PM, Dylan Yudaken wrote:
> > 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.
>
> Should we just reset this to 0 always in io_read_mshot()? And preferably
> with a comment added as well as to why that is necessary to avoid
> repeated clamping.

Unfortunately I don't think (without testing) that will work.
Sometimes the request
comes into io_read_mshot with the buffer already selected, and the
length cannot
be touched in that case.

We could check if the buffer is set, and if not clear the length I guess.
I'm a bit unsure which is better - both seem equally ugly to be honest.

Re: [PATCH 2/2] io_uring: do not clamp read length for multishot read

[Jens Axboe]

On 11/6/23 8:33 AM, Dylan Yudaken wrote:
> On Mon, Nov 6, 2023 at 2:46?PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> On 11/5/23 3:30 PM, Dylan Yudaken wrote:
>>> 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.
>>
>> Should we just reset this to 0 always in io_read_mshot()? And preferably
>> with a comment added as well as to why that is necessary to avoid
>> repeated clamping.
> 
> Unfortunately I don't think (without testing) that will work.
> Sometimes the request
> comes into io_read_mshot with the buffer already selected, and the
> length cannot
> be touched in that case.
> 
> We could check if the buffer is set, and if not clear the length I guess.
> I'm a bit unsure which is better - both seem equally ugly to be honest.

I mean do it at the end when we complete it, so it's reset for the next
iteration. But yeah, I'd want to have the test case verify this first
:-)

-- 
Jens Axboe

Re: [PATCH 2/2] io_uring: do not clamp read length for multishot read

[Jens Axboe]

On 11/6/23 8:46 AM, Jens Axboe wrote:
> On 11/6/23 8:33 AM, Dylan Yudaken wrote:
>> On Mon, Nov 6, 2023 at 2:46?PM Jens Axboe <axboe@kernel.dk> wrote:
>>>
>>> On 11/5/23 3:30 PM, Dylan Yudaken wrote:
>>>> 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.
>>>
>>> Should we just reset this to 0 always in io_read_mshot()? And preferably
>>> with a comment added as well as to why that is necessary to avoid
>>> repeated clamping.
>>
>> Unfortunately I don't think (without testing) that will work.
>> Sometimes the request
>> comes into io_read_mshot with the buffer already selected, and the
>> length cannot
>> be touched in that case.
>>
>> We could check if the buffer is set, and if not clear the length I guess.
>> I'm a bit unsure which is better - both seem equally ugly to be honest.
> 
> I mean do it at the end when we complete it, so it's reset for the next
> iteration. But yeah, I'd want to have the test case verify this first
> :-)

Something ala the below?

diff --git a/io_uring/rw.c b/io_uring/rw.c
index 9e3e56b74e35..9121832eadec 100644
--- a/io_uring/rw.c
+++ b/io_uring/rw.c
@@ -932,6 +932,12 @@ int io_read_mshot(struct io_kiocb *req, unsigned int issue_flags)
 	 * Any successful return value will keep the multishot read armed.
 	 */
 	if (ret > 0) {
+		/*
+		 * Reset rw->len to 0 again to avoid clamping future mshot
+		 * reads, in case the buffer size varies.
+		 */
+		io_kiocb_to_cmd(req, struct io_rw)->len = 0;
+
 		/*
 		 * Put our buffer and post a CQE. If we fail to post a CQE, then
 		 * jump to the termination path. This request is then done.

-- 
Jens Axboe

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

[Jens Axboe]

On 11/5/23 3:30 PM, Dylan Yudaken wrote:
> Hi,
> 
> This series fixes a bug (will send a liburing patch separately showing
> it) where the used buffer size is clamped to the minimum of all the
> previous buffers selected.

Oh, if you have time and once we get v2 in, maybe send a patch for
liburing as well to remove 'nbytes' from io_uring_prep_read_multishot()
as well?

-- 
Jens Axboe