From: Jens Axboe <[email protected]>
To: Daniele Salvatore Albano <[email protected]>,
io-uring <[email protected]>
Subject: Re: [PATCH] io_files_update_prep shouldn't consider all the flags invalid
Date: Fri, 17 Jul 2020 10:21:53 -0600 [thread overview]
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAKq9yRiSyHJu7voNUiXbwm36cRjU+VdcSXYkGPDGWai0w8BG=w@mail.gmail.com>
On 7/17/20 10:13 AM, Daniele Salvatore Albano wrote:
> On Tue, 14 Jul 2020 at 18:32, Daniele Salvatore Albano
> <[email protected]> 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 <[email protected]>
>> ---
>> 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
next prev parent reply other threads:[~2020-07-17 16:21 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
[email protected] \
[email protected] \
[email protected] \
[email protected] \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox