[PATCH] io_files_update_prep shouldn't consider all the flags invalid

[PATCH] io_files_update_prep shouldn't consider all the flags invalid

[Daniele Salvatore Albano]

Currently when an IORING_OP_FILES_UPDATE is submitted with the
IOSQE_IO_LINK flag it fails with EINVAL even if it's considered a
valid because the expectation is that there are no flags set for the
sqe.

The patch updates the check to allow IOSQE_IO_LINK and ensure that
EINVAL is returned only for IOSQE_FIXED_FILE and IOSQE_BUFFER_SELECT.

Signed-off-by: Daniele Albano <d.albano@gmail.com>
---
 fs/io_uring.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index ba70dc62f15f..7058b1a0bd39 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -5205,7 +5205,14 @@ static int io_async_cancel(struct io_kiocb *req)
 static int io_files_update_prep(struct io_kiocb *req,
                                const struct io_uring_sqe *sqe)
 {
-       if (sqe->flags || sqe->ioprio || sqe->rw_flags)
+       unsigned flags = 0;
+
+       if (sqe->ioprio || sqe->rw_flags)
+               return -EINVAL;
+
+       flags = READ_ONCE(sqe->flags);
+
+       if (flags & (IOSQE_FIXED_FILE | IOSQE_BUFFER_SELECT))
                return -EINVAL;

        req->files_update.offset = READ_ONCE(sqe->off);
--
2.25.1

Re: [PATCH] io_files_update_prep shouldn't consider all the flags invalid

[Daniele Salvatore Albano]

On Tue, 14 Jul 2020 at 18:32, Daniele Salvatore Albano
<d.albano@gmail.com> wrote:
>
> Currently when an IORING_OP_FILES_UPDATE is submitted with the
> IOSQE_IO_LINK flag it fails with EINVAL even if it's considered a
> valid because the expectation is that there are no flags set for the
> sqe.
>
> The patch updates the check to allow IOSQE_IO_LINK and ensure that
> EINVAL is returned only for IOSQE_FIXED_FILE and IOSQE_BUFFER_SELECT.
>
> Signed-off-by: Daniele Albano <d.albano@gmail.com>
> ---
>  fs/io_uring.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index ba70dc62f15f..7058b1a0bd39 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -5205,7 +5205,14 @@ static int io_async_cancel(struct io_kiocb *req)
>  static int io_files_update_prep(struct io_kiocb *req,
>                                 const struct io_uring_sqe *sqe)
>  {
> -       if (sqe->flags || sqe->ioprio || sqe->rw_flags)
> +       unsigned flags = 0;
> +
> +       if (sqe->ioprio || sqe->rw_flags)
> +               return -EINVAL;
> +
> +       flags = READ_ONCE(sqe->flags);
> +
> +       if (flags & (IOSQE_FIXED_FILE | IOSQE_BUFFER_SELECT))
>                 return -EINVAL;
>
>         req->files_update.offset = READ_ONCE(sqe->off);
> --
> 2.25.1

Hi,

Did you get the chance to review this patch? Would you prefer to get
the flags loaded before the first branching?

Thanks!

Daniele

Re: [PATCH] io_files_update_prep shouldn't consider all the flags invalid

[Jens Axboe]

On 7/17/20 10:13 AM, Daniele Salvatore Albano wrote:
> On Tue, 14 Jul 2020 at 18:32, Daniele Salvatore Albano
> <d.albano@gmail.com> wrote:
>>
>> Currently when an IORING_OP_FILES_UPDATE is submitted with the
>> IOSQE_IO_LINK flag it fails with EINVAL even if it's considered a
>> valid because the expectation is that there are no flags set for the
>> sqe.
>>
>> The patch updates the check to allow IOSQE_IO_LINK and ensure that
>> EINVAL is returned only for IOSQE_FIXED_FILE and IOSQE_BUFFER_SELECT.
>>
>> Signed-off-by: Daniele Albano <d.albano@gmail.com>
>> ---
>>  fs/io_uring.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index ba70dc62f15f..7058b1a0bd39 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -5205,7 +5205,14 @@ static int io_async_cancel(struct io_kiocb *req)
>>  static int io_files_update_prep(struct io_kiocb *req,
>>                                 const struct io_uring_sqe *sqe)
>>  {
>> -       if (sqe->flags || sqe->ioprio || sqe->rw_flags)
>> +       unsigned flags = 0;
>> +
>> +       if (sqe->ioprio || sqe->rw_flags)
>> +               return -EINVAL;
>> +
>> +       flags = READ_ONCE(sqe->flags);
>> +
>> +       if (flags & (IOSQE_FIXED_FILE | IOSQE_BUFFER_SELECT))
>>                 return -EINVAL;
>>
>>         req->files_update.offset = READ_ONCE(sqe->off);
>> --
>> 2.25.1
> 
> Hi,
> 
> Did you get the chance to review this patch? Would you prefer to get
> the flags loaded before the first branching?

I think it looks fine, but looking a bit further, I think we should
extend this kind of checking to also include timeout_prep and cancel_prep
as well. They suffer from the same kind of issue where they disallow all
flags, and they should just fail on the same as the above.

And we should just use req->flags for this checking, and get rid of the
sqe->flags reading in those prep functions. Something like this:


diff --git a/fs/io_uring.c b/fs/io_uring.c
index 74bc4a04befa..5c87b9a686dd 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -4732,7 +4732,9 @@ static int io_timeout_remove_prep(struct io_kiocb *req,
 {
 	if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
 		return -EINVAL;
-	if (sqe->flags || sqe->ioprio || sqe->buf_index || sqe->len)
+	if (req->flags & (REQ_F_FIXED_FILE | REQ_F_BUFFER_SELECT))
+		return -EINVAL;
+	if (sqe->ioprio || sqe->buf_index || sqe->len)
 		return -EINVAL;
 
 	req->timeout.addr = READ_ONCE(sqe->addr);
@@ -4910,8 +4912,9 @@ static int io_async_cancel_prep(struct io_kiocb *req,
 {
 	if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
 		return -EINVAL;
-	if (sqe->flags || sqe->ioprio || sqe->off || sqe->len ||
-	    sqe->cancel_flags)
+	if (req->flags & (REQ_F_FIXED_FILE | REQ_F_BUFFER_SELECT))
+		return -EINVAL;
+	if (sqe->ioprio || sqe->off || sqe->len || sqe->cancel_flags)
 		return -EINVAL;
 
 	req->cancel.addr = READ_ONCE(sqe->addr);
@@ -4929,9 +4932,10 @@ static int io_async_cancel(struct io_kiocb *req)
 static int io_files_update_prep(struct io_kiocb *req,
 				const struct io_uring_sqe *sqe)
 {
-	if (sqe->flags || sqe->ioprio || sqe->rw_flags)
+	if (sqe->ioprio || sqe->rw_flags)
+		return -EINVAL;
+	if (req->flags & (REQ_F_FIXED_FILE | REQ_F_BUFFER_SELECT))
 		return -EINVAL;
-
 	req->files_update.offset = READ_ONCE(sqe->off);
 	req->files_update.nr_args = READ_ONCE(sqe->len);
 	if (!req->files_update.nr_args)

-- 
Jens Axboe

Re: [PATCH] io_files_update_prep shouldn't consider all the flags invalid

[Daniele Salvatore Albano]

Sure thing, tomorrow I will put it together review all the other ops
as well, just in case (although I believe you may already have done
it), and test it.

For the test cases, should I submit a separate patch for liburing or
do you prefer to use pull requests on gh?

On Fri, 17 Jul 2020 at 17:21, Jens Axboe <axboe@kernel.dk> wrote:
>
> On 7/17/20 10:13 AM, Daniele Salvatore Albano wrote:
> > On Tue, 14 Jul 2020 at 18:32, Daniele Salvatore Albano
> > <d.albano@gmail.com> wrote:
> >>
> >> Currently when an IORING_OP_FILES_UPDATE is submitted with the
> >> IOSQE_IO_LINK flag it fails with EINVAL even if it's considered a
> >> valid because the expectation is that there are no flags set for the
> >> sqe.
> >>
> >> The patch updates the check to allow IOSQE_IO_LINK and ensure that
> >> EINVAL is returned only for IOSQE_FIXED_FILE and IOSQE_BUFFER_SELECT.
> >>
> >> Signed-off-by: Daniele Albano <d.albano@gmail.com>
> >> ---
> >>  fs/io_uring.c | 9 ++++++++-
> >>  1 file changed, 8 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/fs/io_uring.c b/fs/io_uring.c
> >> index ba70dc62f15f..7058b1a0bd39 100644
> >> --- a/fs/io_uring.c
> >> +++ b/fs/io_uring.c
> >> @@ -5205,7 +5205,14 @@ static int io_async_cancel(struct io_kiocb *req)
> >>  static int io_files_update_prep(struct io_kiocb *req,
> >>                                 const struct io_uring_sqe *sqe)
> >>  {
> >> -       if (sqe->flags || sqe->ioprio || sqe->rw_flags)
> >> +       unsigned flags = 0;
> >> +
> >> +       if (sqe->ioprio || sqe->rw_flags)
> >> +               return -EINVAL;
> >> +
> >> +       flags = READ_ONCE(sqe->flags);
> >> +
> >> +       if (flags & (IOSQE_FIXED_FILE | IOSQE_BUFFER_SELECT))
> >>                 return -EINVAL;
> >>
> >>         req->files_update.offset = READ_ONCE(sqe->off);
> >> --
> >> 2.25.1
> >
> > Hi,
> >
> > Did you get the chance to review this patch? Would you prefer to get
> > the flags loaded before the first branching?
>
> I think it looks fine, but looking a bit further, I think we should
> extend this kind of checking to also include timeout_prep and cancel_prep
> as well. They suffer from the same kind of issue where they disallow all
> flags, and they should just fail on the same as the above.
>
> And we should just use req->flags for this checking, and get rid of the
> sqe->flags reading in those prep functions. Something like this:
>
>
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 74bc4a04befa..5c87b9a686dd 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -4732,7 +4732,9 @@ static int io_timeout_remove_prep(struct io_kiocb *req,
>  {
>         if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
>                 return -EINVAL;
> -       if (sqe->flags || sqe->ioprio || sqe->buf_index || sqe->len)
> +       if (req->flags & (REQ_F_FIXED_FILE | REQ_F_BUFFER_SELECT))
> +               return -EINVAL;
> +       if (sqe->ioprio || sqe->buf_index || sqe->len)
>                 return -EINVAL;
>
>         req->timeout.addr = READ_ONCE(sqe->addr);
> @@ -4910,8 +4912,9 @@ static int io_async_cancel_prep(struct io_kiocb *req,
>  {
>         if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
>                 return -EINVAL;
> -       if (sqe->flags || sqe->ioprio || sqe->off || sqe->len ||
> -           sqe->cancel_flags)
> +       if (req->flags & (REQ_F_FIXED_FILE | REQ_F_BUFFER_SELECT))
> +               return -EINVAL;
> +       if (sqe->ioprio || sqe->off || sqe->len || sqe->cancel_flags)
>                 return -EINVAL;
>
>         req->cancel.addr = READ_ONCE(sqe->addr);
> @@ -4929,9 +4932,10 @@ static int io_async_cancel(struct io_kiocb *req)
>  static int io_files_update_prep(struct io_kiocb *req,
>                                 const struct io_uring_sqe *sqe)
>  {
> -       if (sqe->flags || sqe->ioprio || sqe->rw_flags)
> +       if (sqe->ioprio || sqe->rw_flags)
> +               return -EINVAL;
> +       if (req->flags & (REQ_F_FIXED_FILE | REQ_F_BUFFER_SELECT))
>                 return -EINVAL;
> -
>         req->files_update.offset = READ_ONCE(sqe->off);
>         req->files_update.nr_args = READ_ONCE(sqe->len);
>         if (!req->files_update.nr_args)
>
> --
> Jens Axboe
>

Re: [PATCH] io_files_update_prep shouldn't consider all the flags invalid

[Jens Axboe]

On 7/17/20 4:39 PM, Daniele Salvatore Albano wrote:
> Sure thing, tomorrow I will put it together review all the other ops
> as well, just in case (although I believe you may already have done
> it), and test it.

I did take a quick look and these were the three I found. There
shouldn't be others, so I think we're good there.

> For the test cases, should I submit a separate patch for liburing or
> do you prefer to use pull requests on gh?

Either one is fine, I can work with either.

-- 
Jens Axboe

Re: [PATCH] io_files_update_prep shouldn't consider all the flags invalid

[Daniele Salvatore Albano]

On Fri, 17 Jul 2020 at 23:48, Jens Axboe <axboe@kernel.dk> wrote:
>
> On 7/17/20 4:39 PM, Daniele Salvatore Albano wrote:
> > Sure thing, tomorrow I will put it together, review all the other ops
> > as well, just in case (although I believe you may already have done
> > it), and test it.
>
> I did take a quick look and these were the three I found. There
> shouldn't be others, so I think we're good there.
>
> > For the test cases, should I submit a separate patch for liburing or
> > do you prefer to use pull requests on gh?
>
> Either one is fine, I can work with either.
>
> --
> Jens Axboe
>

I changed the patch name considering that is now affecting multiple
functions, I will also create the PR for the test cases but it may
take a few days, I wasn't using the other 2 functions and need to do
some testing.

---

[PATCH] allow flags in io_timeout_remove_prep, io_async_cancel_prep
 and io_files_update_prep

io_timeout_remove_prep, io_async_cancel_prep and io_files_update_prep
should allow valid flags.

Signed-off-by: Daniele Albano <d.albano@gmail.com>
---
 fs/io_uring.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index ba70dc62f15f..3101b4a36bc9 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -5010,7 +5010,11 @@ static int io_timeout_remove_prep(struct io_kiocb *req,
 {
        if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
                return -EINVAL;
-       if (sqe->flags || sqe->ioprio || sqe->buf_index || sqe->len)
+
+    if (unlikely(req->flags & (REQ_F_FIXED_FILE | REQ_F_BUFFER_SELECT)))
+        return -EINVAL;
+
+    if (unlikely(sqe->ioprio || sqe->buf_index || sqe->len))
                return -EINVAL;

        req->timeout.addr = READ_ONCE(sqe->addr);
@@ -5186,8 +5190,11 @@ static int io_async_cancel_prep(struct io_kiocb *req,
 {
        if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
                return -EINVAL;
-       if (sqe->flags || sqe->ioprio || sqe->off || sqe->len ||
-           sqe->cancel_flags)
+
+    if (unlikely(req->flags & (REQ_F_FIXED_FILE | REQ_F_BUFFER_SELECT)))
+        return -EINVAL;
+
+    if (unlikely(sqe->ioprio || sqe->off || sqe->len || sqe->cancel_flags))
                return -EINVAL;

        req->cancel.addr = READ_ONCE(sqe->addr);
@@ -5205,7 +5212,10 @@ static int io_async_cancel(struct io_kiocb *req)
 static int io_files_update_prep(struct io_kiocb *req,
                                const struct io_uring_sqe *sqe)
 {
-       if (sqe->flags || sqe->ioprio || sqe->rw_flags)
+    if (unlikely(req->flags & (REQ_F_FIXED_FILE | REQ_F_BUFFER_SELECT)))
+        return -EINVAL;
+
+    if (unlikely(sqe->ioprio || sqe->rw_flags))
                return -EINVAL;

        req->files_update.offset = READ_ONCE(sqe->off);
--
2.25.1

Re: [PATCH] io_files_update_prep shouldn't consider all the flags invalid

[Jens Axboe]

On 7/18/20 11:29 AM, Daniele Salvatore Albano wrote:
> On Fri, 17 Jul 2020 at 23:48, Jens Axboe <axboe@kernel.dk> wrote:
>>
>> On 7/17/20 4:39 PM, Daniele Salvatore Albano wrote:
>>> Sure thing, tomorrow I will put it together, review all the other ops
>>> as well, just in case (although I believe you may already have done
>>> it), and test it.
>>
>> I did take a quick look and these were the three I found. There
>> shouldn't be others, so I think we're good there.
>>
>>> For the test cases, should I submit a separate patch for liburing or
>>> do you prefer to use pull requests on gh?
>>
>> Either one is fine, I can work with either.
>>
>> --
>> Jens Axboe
>>
> 
> I changed the patch name considering that is now affecting multiple
> functions, I will also create the PR for the test cases but it may
> take a few days, I wasn't using the other 2 functions and need to do
> some testing.

Thanks, I applied this one with a modified commit message. Also note
that your mailer is mangling patches, the whitespace is all damaged.
I fixed it up. Here's the end result:

https://git.kernel.dk/cgit/linux-block/commit/?h=io_uring-5.8&id=61710e437f2807e26a3402543bdbb7217a9c8620

-- 
Jens Axboe

Re: [PATCH] io_files_update_prep shouldn't consider all the flags invalid

[Daniele Salvatore Albano]

On Sat, 18 Jul 2020 at 21:23, Jens Axboe <axboe@kernel.dk> wrote:
>
> On 7/18/20 11:29 AM, Daniele Salvatore Albano wrote:
> > On Fri, 17 Jul 2020 at 23:48, Jens Axboe <axboe@kernel.dk> wrote:
> >>
> >> On 7/17/20 4:39 PM, Daniele Salvatore Albano wrote:
> >
> > I changed the patch name considering that is now affecting multiple
> > functions, I will also create the PR for the test cases but it may
> > take a few days, I wasn't using the other 2 functions and need to do
> > some testing.
>
> Thanks, I applied this one with a modified commit message. Also note
> that your mailer is mangling patches, the whitespace is all damaged.
> I fixed it up. Here's the end result:
>
> https://git.kernel.dk/cgit/linux-block/commit/?h=io_uring-5.8&id=61710e437f2807e26a3402543bdbb7217a9c8620
>
> --
> Jens Axboe
>

Oh, I am very sorry about this, it's gmail in plain text mode, I will
double check the settings.

Thanks!
Daniele