IORING_OP_CLOSE fails on fd opened with O_PATH

IORING_OP_CLOSE fails on fd opened with O_PATH

[Clay Harris]

Tested on kernel 5.6.14

$ ./closetest closetest.c

path closetest.c open on fd 3 with O_RDONLY
 ---- io_uring close(3)
 ---- ordinary close(3)
ordinary close(3) failed, errno 9: Bad file descriptor


$ ./closetest closetest.c opath

path closetest.c open on fd 3 with O_PATH
 ---- io_uring close(3)
io_uring close() failed, errno 9: Bad file descriptor
 ---- ordinary close(3)
ordinary close(3) returned 0

Re: IORING_OP_CLOSE fails on fd opened with O_PATH

[Jens Axboe]

On 5/31/20 6:47 AM, Clay Harris wrote:
> Tested on kernel 5.6.14
> 
> $ ./closetest closetest.c
> 
> path closetest.c open on fd 3 with O_RDONLY
>  ---- io_uring close(3)
>  ---- ordinary close(3)
> ordinary close(3) failed, errno 9: Bad file descriptor
> 
> 
> $ ./closetest closetest.c opath
> 
> path closetest.c open on fd 3 with O_PATH
>  ---- io_uring close(3)
> io_uring close() failed, errno 9: Bad file descriptor
>  ---- ordinary close(3)
> ordinary close(3) returned 0

Can you include the test case, please? Should be an easy fix, but no
point rewriting a test case if I can avoid it...

-- 
Jens Axboe

Re: IORING_OP_CLOSE fails on fd opened with O_PATH

[Jens Axboe]

On 5/31/20 8:46 AM, Jens Axboe wrote:
> On 5/31/20 6:47 AM, Clay Harris wrote:
>> Tested on kernel 5.6.14
>>
>> $ ./closetest closetest.c
>>
>> path closetest.c open on fd 3 with O_RDONLY
>>  ---- io_uring close(3)
>>  ---- ordinary close(3)
>> ordinary close(3) failed, errno 9: Bad file descriptor
>>
>>
>> $ ./closetest closetest.c opath
>>
>> path closetest.c open on fd 3 with O_PATH
>>  ---- io_uring close(3)
>> io_uring close() failed, errno 9: Bad file descriptor
>>  ---- ordinary close(3)
>> ordinary close(3) returned 0
> 
> Can you include the test case, please? Should be an easy fix, but no
> point rewriting a test case if I can avoid it...

We just need this ported to stable once it goes into 5.8-rc:

https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.8/io_uring&id=904fbcb115c85090484dfdffaf7f461d96fe8e53

-- 
Jens Axboe

Re: IORING_OP_CLOSE fails on fd opened with O_PATH

[Jann Horn]

On Sun, May 31, 2020 at 10:19 PM Jens Axboe <axboe@kernel.dk> wrote:
> We just need this ported to stable once it goes into 5.8-rc:
>
> https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.8/io_uring&id=904fbcb115c85090484dfdffaf7f461d96fe8e53

How does that work? Who guarantees that the close operation can't drop
the refcount of the uring instance to zero before reaching the fdput()
in io_uring_enter?

Re: IORING_OP_CLOSE fails on fd opened with O_PATH

[Jens Axboe]

On 6/2/20 12:22 PM, Jann Horn wrote:
> On Sun, May 31, 2020 at 10:19 PM Jens Axboe <axboe@kernel.dk> wrote:
>> We just need this ported to stable once it goes into 5.8-rc:
>>
>> https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.8/io_uring&id=904fbcb115c85090484dfdffaf7f461d96fe8e53
> 
> How does that work? Who guarantees that the close operation can't drop
> the refcount of the uring instance to zero before reaching the fdput()
> in io_uring_enter?

Because io_uring_enter() holds a reference to it as well?

-- 
Jens Axboe

Re: IORING_OP_CLOSE fails on fd opened with O_PATH

[Jann Horn]

On Tue, Jun 2, 2020 at 8:42 PM Jens Axboe <axboe@kernel.dk> wrote:
> On 6/2/20 12:22 PM, Jann Horn wrote:
> > On Sun, May 31, 2020 at 10:19 PM Jens Axboe <axboe@kernel.dk> wrote:
> >> We just need this ported to stable once it goes into 5.8-rc:
> >>
> >> https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.8/io_uring&id=904fbcb115c85090484dfdffaf7f461d96fe8e53
> >
> > How does that work? Who guarantees that the close operation can't drop
> > the refcount of the uring instance to zero before reaching the fdput()
> > in io_uring_enter?
>
> Because io_uring_enter() holds a reference to it as well?

Which reference do you mean? fdget() doesn't take a reference if the
calling process is single-threaded, you'd have to use fget() for that.

Re: IORING_OP_CLOSE fails on fd opened with O_PATH

[Jens Axboe]

On 6/2/20 1:16 PM, Jann Horn wrote:
> On Tue, Jun 2, 2020 at 8:42 PM Jens Axboe <axboe@kernel.dk> wrote:
>> On 6/2/20 12:22 PM, Jann Horn wrote:
>>> On Sun, May 31, 2020 at 10:19 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>> We just need this ported to stable once it goes into 5.8-rc:
>>>>
>>>> https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.8/io_uring&id=904fbcb115c85090484dfdffaf7f461d96fe8e53
>>>
>>> How does that work? Who guarantees that the close operation can't drop
>>> the refcount of the uring instance to zero before reaching the fdput()
>>> in io_uring_enter?
>>
>> Because io_uring_enter() holds a reference to it as well?
> 
> Which reference do you mean? fdget() doesn't take a reference if the
> calling process is single-threaded, you'd have to use fget() for that.

I meant the ctx->refs, but that's not enough for the file, good point.
I'll apply the below on top - that should fix the issue with O_PATH
still, while retaining our logic not to allow ring closure. I think we
could make ring closure work, but I don't want to use fget() if I can
avoid it. And it really doesn't seem worth it to go through the trouble
of adding any extra code to allow ring closure.


diff --git a/fs/io_uring.c b/fs/io_uring.c
index 732ec73ec3c0..2ce972d9a49e 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -696,6 +696,8 @@ struct io_op_def {
 	unsigned		needs_mm : 1;
 	/* needs req->file assigned */
 	unsigned		needs_file : 1;
+	/* don't fail if file grab fails */
+	unsigned		needs_file_no_error : 1;
 	/* hash wq insertion if file is a regular file */
 	unsigned		hash_reg_file : 1;
 	/* unbound wq insertion if file is a non-regular file */
@@ -802,6 +804,8 @@ static const struct io_op_def io_op_defs[] = {
 		.needs_fs		= 1,
 	},
 	[IORING_OP_CLOSE] = {
+		.needs_file		= 1,
+		.needs_file_no_error	= 1,
 		.file_table		= 1,
 	},
 	[IORING_OP_FILES_UPDATE] = {
@@ -3424,6 +3428,10 @@ static int io_close_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 		return -EBADF;
 
 	req->close.fd = READ_ONCE(sqe->fd);
+	if ((req->file && req->file->f_op == &io_uring_fops) ||
+	    req->close.fd == req->ctx->ring_fd)
+		return -EBADF;
+
 	return 0;
 }
 
@@ -5437,19 +5445,20 @@ static int io_file_get(struct io_submit_state *state, struct io_kiocb *req,
 			return -EBADF;
 		fd = array_index_nospec(fd, ctx->nr_user_files);
 		file = io_file_from_index(ctx, fd);
-		if (!file)
-			return -EBADF;
-		req->fixed_file_refs = ctx->file_data->cur_refs;
-		percpu_ref_get(req->fixed_file_refs);
+		if (file) {
+			req->fixed_file_refs = ctx->file_data->cur_refs;
+			percpu_ref_get(req->fixed_file_refs);
+		}
 	} else {
 		trace_io_uring_file_get(ctx, fd);
 		file = __io_file_get(state, fd);
-		if (unlikely(!file))
-			return -EBADF;
 	}
 
-	*out_file = file;
-	return 0;
+	if (file || io_op_defs[req->opcode].needs_file_no_error) {
+		*out_file = file;
+		return 0;
+	}
+	return -EBADF;
 }
 
 static int io_req_set_file(struct io_submit_state *state, struct io_kiocb *req,

-- 
Jens Axboe

Re: IORING_OP_CLOSE fails on fd opened with O_PATH

[Clay Harris]

On Sun, May 31 2020 at 08:46:03 -0600, Jens Axboe quoth thus:

> On 5/31/20 6:47 AM, Clay Harris wrote:
> > Tested on kernel 5.6.14
> > 
> > $ ./closetest closetest.c
> > 
> > path closetest.c open on fd 3 with O_RDONLY
> >  ---- io_uring close(3)
> >  ---- ordinary close(3)
> > ordinary close(3) failed, errno 9: Bad file descriptor
> > 
> > 
> > $ ./closetest closetest.c opath
> > 
> > path closetest.c open on fd 3 with O_PATH
> >  ---- io_uring close(3)
> > io_uring close() failed, errno 9: Bad file descriptor
> >  ---- ordinary close(3)
> > ordinary close(3) returned 0
> 
> Can you include the test case, please? Should be an easy fix, but no
> point rewriting a test case if I can avoid it...

Sure.  Here's a cleaned-up test program.
https://claycon.org/software/io_uring/tests/close_opath.c

> -- 
> Jens Axboe

Re: IORING_OP_CLOSE fails on fd opened with O_PATH

[Jens Axboe]

On 6/8/20 5:21 AM, Clay Harris wrote:
> On Sun, May 31 2020 at 08:46:03 -0600, Jens Axboe quoth thus:
> 
>> On 5/31/20 6:47 AM, Clay Harris wrote:
>>> Tested on kernel 5.6.14
>>>
>>> $ ./closetest closetest.c
>>>
>>> path closetest.c open on fd 3 with O_RDONLY
>>>  ---- io_uring close(3)
>>>  ---- ordinary close(3)
>>> ordinary close(3) failed, errno 9: Bad file descriptor
>>>
>>>
>>> $ ./closetest closetest.c opath
>>>
>>> path closetest.c open on fd 3 with O_PATH
>>>  ---- io_uring close(3)
>>> io_uring close() failed, errno 9: Bad file descriptor
>>>  ---- ordinary close(3)
>>> ordinary close(3) returned 0
>>
>> Can you include the test case, please? Should be an easy fix, but no
>> point rewriting a test case if I can avoid it...
> 
> Sure.  Here's a cleaned-up test program.
> https://claycon.org/software/io_uring/tests/close_opath.c

Thanks for sending this - but it's GPL v3, I can't take that. I'll
probably just add an O_PATH test case to the existing open-close test
cases.

-- 
Jens Axboe

Re: IORING_OP_CLOSE fails on fd opened with O_PATH

[Clay Harris]

On Mon, Jun 08 2020 at 14:19:56 -0600, Jens Axboe quoth thus:

> On 6/8/20 5:21 AM, Clay Harris wrote:
> > On Sun, May 31 2020 at 08:46:03 -0600, Jens Axboe quoth thus:
> > 
> >> On 5/31/20 6:47 AM, Clay Harris wrote:
> >>> Tested on kernel 5.6.14
> >>>
> >>> $ ./closetest closetest.c
> >>>
> >>> path closetest.c open on fd 3 with O_RDONLY
> >>>  ---- io_uring close(3)
> >>>  ---- ordinary close(3)
> >>> ordinary close(3) failed, errno 9: Bad file descriptor
> >>>
> >>>
> >>> $ ./closetest closetest.c opath
> >>>
> >>> path closetest.c open on fd 3 with O_PATH
> >>>  ---- io_uring close(3)
> >>> io_uring close() failed, errno 9: Bad file descriptor
> >>>  ---- ordinary close(3)
> >>> ordinary close(3) returned 0
> >>
> >> Can you include the test case, please? Should be an easy fix, but no
> >> point rewriting a test case if I can avoid it...
> > 
> > Sure.  Here's a cleaned-up test program.
> > https://claycon.org/software/io_uring/tests/close_opath.c
> 
> Thanks for sending this - but it's GPL v3, I can't take that. I'll
> probably just add an O_PATH test case to the existing open-close test
> cases.

I didn't realize that would be an issue.
I'll change it.  Would you prefer GPL 2, or should I just delete the
license line altogether?

> -- 
> Jens Axboe

Re: IORING_OP_CLOSE fails on fd opened with O_PATH

[Jens Axboe]

On 6/8/20 7:40 PM, Clay Harris wrote:
> On Mon, Jun 08 2020 at 14:19:56 -0600, Jens Axboe quoth thus:
> 
>> On 6/8/20 5:21 AM, Clay Harris wrote:
>>> On Sun, May 31 2020 at 08:46:03 -0600, Jens Axboe quoth thus:
>>>
>>>> On 5/31/20 6:47 AM, Clay Harris wrote:
>>>>> Tested on kernel 5.6.14
>>>>>
>>>>> $ ./closetest closetest.c
>>>>>
>>>>> path closetest.c open on fd 3 with O_RDONLY
>>>>>  ---- io_uring close(3)
>>>>>  ---- ordinary close(3)
>>>>> ordinary close(3) failed, errno 9: Bad file descriptor
>>>>>
>>>>>
>>>>> $ ./closetest closetest.c opath
>>>>>
>>>>> path closetest.c open on fd 3 with O_PATH
>>>>>  ---- io_uring close(3)
>>>>> io_uring close() failed, errno 9: Bad file descriptor
>>>>>  ---- ordinary close(3)
>>>>> ordinary close(3) returned 0
>>>>
>>>> Can you include the test case, please? Should be an easy fix, but no
>>>> point rewriting a test case if I can avoid it...
>>>
>>> Sure.  Here's a cleaned-up test program.
>>> https://claycon.org/software/io_uring/tests/close_opath.c
>>
>> Thanks for sending this - but it's GPL v3, I can't take that. I'll
>> probably just add an O_PATH test case to the existing open-close test
>> cases.
> 
> I didn't realize that would be an issue.
> I'll change it.  Would you prefer GPL 2, or should I just delete the
> license line altogether?

It's not a huge deal, but at the same time I see no reason to add GPL
v3 unless I absolutely have to (and I don't). So yeah, if you could
just post with MIT (like the other test programs), then that'd be
preferable.

-- 
Jens Axboe

Re: IORING_OP_CLOSE fails on fd opened with O_PATH

[Clay Harris]

On Mon, Jun 08 2020 at 20:14:51 -0600, Jens Axboe quoth thus:

> On 6/8/20 7:40 PM, Clay Harris wrote:
> > On Mon, Jun 08 2020 at 14:19:56 -0600, Jens Axboe quoth thus:
> > 
> >> On 6/8/20 5:21 AM, Clay Harris wrote:
> >>> On Sun, May 31 2020 at 08:46:03 -0600, Jens Axboe quoth thus:
> >>>
> >>>> On 5/31/20 6:47 AM, Clay Harris wrote:
> >>>>> Tested on kernel 5.6.14
> >>>>>
> >>>>> $ ./closetest closetest.c
> >>>>>
> >>>>> path closetest.c open on fd 3 with O_RDONLY
> >>>>>  ---- io_uring close(3)
> >>>>>  ---- ordinary close(3)
> >>>>> ordinary close(3) failed, errno 9: Bad file descriptor
> >>>>>
> >>>>>
> >>>>> $ ./closetest closetest.c opath
> >>>>>
> >>>>> path closetest.c open on fd 3 with O_PATH
> >>>>>  ---- io_uring close(3)
> >>>>> io_uring close() failed, errno 9: Bad file descriptor
> >>>>>  ---- ordinary close(3)
> >>>>> ordinary close(3) returned 0
> >>>>
> >>>> Can you include the test case, please? Should be an easy fix, but no
> >>>> point rewriting a test case if I can avoid it...
> >>>
> >>> Sure.  Here's a cleaned-up test program.
> >>> https://claycon.org/software/io_uring/tests/close_opath.c
> >>
> >> Thanks for sending this - but it's GPL v3, I can't take that. I'll
> >> probably just add an O_PATH test case to the existing open-close test
> >> cases.
> > 
> > I didn't realize that would be an issue.
> > I'll change it.  Would you prefer GPL 2, or should I just delete the
> > license line altogether?
> 
> It's not a huge deal, but at the same time I see no reason to add GPL
> v3 unless I absolutely have to (and I don't). So yeah, if you could
> just post with MIT (like the other test programs), then that'd be
> preferable.

* Change license to MIT.
Done.

> -- 
> Jens Axboe

Re: IORING_OP_CLOSE fails on fd opened with O_PATH

[Jens Axboe]

On 6/8/20 11:16 PM, Clay Harris wrote:
> On Mon, Jun 08 2020 at 20:14:51 -0600, Jens Axboe quoth thus:
> 
>> On 6/8/20 7:40 PM, Clay Harris wrote:
>>> On Mon, Jun 08 2020 at 14:19:56 -0600, Jens Axboe quoth thus:
>>>
>>>> On 6/8/20 5:21 AM, Clay Harris wrote:
>>>>> On Sun, May 31 2020 at 08:46:03 -0600, Jens Axboe quoth thus:
>>>>>
>>>>>> On 5/31/20 6:47 AM, Clay Harris wrote:
>>>>>>> Tested on kernel 5.6.14
>>>>>>>
>>>>>>> $ ./closetest closetest.c
>>>>>>>
>>>>>>> path closetest.c open on fd 3 with O_RDONLY
>>>>>>>  ---- io_uring close(3)
>>>>>>>  ---- ordinary close(3)
>>>>>>> ordinary close(3) failed, errno 9: Bad file descriptor
>>>>>>>
>>>>>>>
>>>>>>> $ ./closetest closetest.c opath
>>>>>>>
>>>>>>> path closetest.c open on fd 3 with O_PATH
>>>>>>>  ---- io_uring close(3)
>>>>>>> io_uring close() failed, errno 9: Bad file descriptor
>>>>>>>  ---- ordinary close(3)
>>>>>>> ordinary close(3) returned 0
>>>>>>
>>>>>> Can you include the test case, please? Should be an easy fix, but no
>>>>>> point rewriting a test case if I can avoid it...
>>>>>
>>>>> Sure.  Here's a cleaned-up test program.
>>>>> https://claycon.org/software/io_uring/tests/close_opath.c
>>>>
>>>> Thanks for sending this - but it's GPL v3, I can't take that. I'll
>>>> probably just add an O_PATH test case to the existing open-close test
>>>> cases.
>>>
>>> I didn't realize that would be an issue.
>>> I'll change it.  Would you prefer GPL 2, or should I just delete the
>>> license line altogether?
>>
>> It's not a huge deal, but at the same time I see no reason to add GPL
>> v3 unless I absolutely have to (and I don't). So yeah, if you could
>> just post with MIT (like the other test programs), then that'd be
>> preferable.
> 
> * Change license to MIT.
> Done.

Thanks, edited it to turn it into something that can be included as
a liburing regression test case.

-- 
Jens Axboe