* [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 related [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 related [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 related [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, other threads:[~2020-07-18 20:48 UTC | newest] 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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).