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

[Jens Axboe <axboe@xxxxxxxxx>]

On 7/17/20 10:13 AM, Daniele Salvatore Albano wrote:
> On Tue, 14 Jul 2020 at 18:32, Daniele Salvatore Albano
> <d.albano@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>>  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