IO-Uring Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] io_files_update_prep shouldn't consider all the flags invalid
@ 2020-07-14 17:32 Daniele Salvatore Albano
  2020-07-17 16:13 ` Daniele Salvatore Albano
  0 siblings, 1 reply; 8+ messages in thread
From: Daniele Salvatore Albano @ 2020-07-14 17:32 UTC (permalink / raw)
  To: io-uring

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] io_files_update_prep shouldn't consider all the flags invalid
  2020-07-14 17:32 [PATCH] io_files_update_prep shouldn't consider all the flags invalid Daniele Salvatore Albano
@ 2020-07-17 16:13 ` Daniele Salvatore Albano
  2020-07-17 16:21   ` Jens Axboe
  0 siblings, 1 reply; 8+ messages in thread
From: Daniele Salvatore Albano @ 2020-07-17 16:13 UTC (permalink / raw)
  To: io-uring

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] io_files_update_prep shouldn't consider all the flags invalid
  2020-07-17 16:13 ` Daniele Salvatore Albano
@ 2020-07-17 16:21   ` Jens Axboe
  2020-07-17 22:39     ` Daniele Salvatore Albano
  0 siblings, 1 reply; 8+ messages in thread
From: Jens Axboe @ 2020-07-17 16:21 UTC (permalink / raw)
  To: Daniele Salvatore Albano, io-uring

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] io_files_update_prep shouldn't consider all the flags invalid
  2020-07-17 16:21   ` Jens Axboe
@ 2020-07-17 22:39     ` Daniele Salvatore Albano
  2020-07-17 22:48       ` Jens Axboe
  0 siblings, 1 reply; 8+ messages in thread
From: Daniele Salvatore Albano @ 2020-07-17 22:39 UTC (permalink / raw)
  To: Jens Axboe; +Cc: io-uring

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
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] io_files_update_prep shouldn't consider all the flags invalid
  2020-07-17 22:39     ` Daniele Salvatore Albano
@ 2020-07-17 22:48       ` Jens Axboe
  2020-07-18 17:29         ` Daniele Salvatore Albano
  0 siblings, 1 reply; 8+ messages in thread
From: Jens Axboe @ 2020-07-17 22:48 UTC (permalink / raw)
  To: Daniele Salvatore Albano; +Cc: io-uring

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] io_files_update_prep shouldn't consider all the flags invalid
  2020-07-17 22:48       ` Jens Axboe
@ 2020-07-18 17:29         ` Daniele Salvatore Albano
  2020-07-18 20:23           ` Jens Axboe
  0 siblings, 1 reply; 8+ messages in thread
From: Daniele Salvatore Albano @ 2020-07-18 17:29 UTC (permalink / raw)
  To: Jens Axboe; +Cc: io-uring

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] io_files_update_prep shouldn't consider all the flags invalid
  2020-07-18 17:29         ` Daniele Salvatore Albano
@ 2020-07-18 20:23           ` Jens Axboe
  2020-07-18 20:48             ` Daniele Salvatore Albano
  0 siblings, 1 reply; 8+ messages in thread
From: Jens Axboe @ 2020-07-18 20:23 UTC (permalink / raw)
  To: Daniele Salvatore Albano; +Cc: io-uring

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] io_files_update_prep shouldn't consider all the flags invalid
  2020-07-18 20:23           ` Jens Axboe
@ 2020-07-18 20:48             ` Daniele Salvatore Albano
  0 siblings, 0 replies; 8+ messages in thread
From: Daniele Salvatore Albano @ 2020-07-18 20:48 UTC (permalink / raw)
  To: Jens Axboe; +Cc: io-uring

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-14 17:32 [PATCH] io_files_update_prep shouldn't consider all the flags invalid Daniele Salvatore Albano
2020-07-17 16:13 ` Daniele Salvatore Albano
2020-07-17 16:21   ` Jens Axboe
2020-07-17 22:39     ` Daniele Salvatore Albano
2020-07-17 22:48       ` Jens Axboe
2020-07-18 17:29         ` Daniele Salvatore Albano
2020-07-18 20:23           ` Jens Axboe
2020-07-18 20:48             ` Daniele Salvatore Albano

IO-Uring Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/io-uring/0 io-uring/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 io-uring io-uring/ https://lore.kernel.org/io-uring \
		io-uring@vger.kernel.org
	public-inbox-index io-uring

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.io-uring


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git