* IORING_OP_CLOSE fails on fd opened with O_PATH
@ 2020-05-31 12:47 Clay Harris
2020-05-31 14:46 ` Jens Axboe
0 siblings, 1 reply; 13+ messages in thread
From: Clay Harris @ 2020-05-31 12:47 UTC (permalink / raw)
To: io-uring
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: IORING_OP_CLOSE fails on fd opened with O_PATH
2020-05-31 12:47 IORING_OP_CLOSE fails on fd opened with O_PATH Clay Harris
@ 2020-05-31 14:46 ` Jens Axboe
2020-05-31 20:19 ` Jens Axboe
2020-06-08 11:21 ` Clay Harris
0 siblings, 2 replies; 13+ messages in thread
From: Jens Axboe @ 2020-05-31 14:46 UTC (permalink / raw)
To: Clay Harris, io-uring
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: IORING_OP_CLOSE fails on fd opened with O_PATH
2020-05-31 14:46 ` Jens Axboe
@ 2020-05-31 20:19 ` Jens Axboe
2020-06-02 18:22 ` Jann Horn
2020-06-08 11:21 ` Clay Harris
1 sibling, 1 reply; 13+ messages in thread
From: Jens Axboe @ 2020-05-31 20:19 UTC (permalink / raw)
To: Clay Harris, io-uring
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: IORING_OP_CLOSE fails on fd opened with O_PATH
2020-05-31 20:19 ` Jens Axboe
@ 2020-06-02 18:22 ` Jann Horn
2020-06-02 18:42 ` Jens Axboe
0 siblings, 1 reply; 13+ messages in thread
From: Jann Horn @ 2020-06-02 18:22 UTC (permalink / raw)
To: Jens Axboe; +Cc: Clay Harris, io-uring
On Sun, May 31, 2020 at 10:19 PM Jens Axboe <[email protected]> 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?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: IORING_OP_CLOSE fails on fd opened with O_PATH
2020-06-02 18:22 ` Jann Horn
@ 2020-06-02 18:42 ` Jens Axboe
2020-06-02 19:16 ` Jann Horn
0 siblings, 1 reply; 13+ messages in thread
From: Jens Axboe @ 2020-06-02 18:42 UTC (permalink / raw)
To: Jann Horn; +Cc: Clay Harris, io-uring
On 6/2/20 12:22 PM, Jann Horn wrote:
> On Sun, May 31, 2020 at 10:19 PM Jens Axboe <[email protected]> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: IORING_OP_CLOSE fails on fd opened with O_PATH
2020-06-02 18:42 ` Jens Axboe
@ 2020-06-02 19:16 ` Jann Horn
2020-06-02 21:39 ` Jens Axboe
0 siblings, 1 reply; 13+ messages in thread
From: Jann Horn @ 2020-06-02 19:16 UTC (permalink / raw)
To: Jens Axboe; +Cc: Clay Harris, io-uring
On Tue, Jun 2, 2020 at 8:42 PM Jens Axboe <[email protected]> wrote:
> On 6/2/20 12:22 PM, Jann Horn wrote:
> > On Sun, May 31, 2020 at 10:19 PM Jens Axboe <[email protected]> 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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: IORING_OP_CLOSE fails on fd opened with O_PATH
2020-06-02 19:16 ` Jann Horn
@ 2020-06-02 21:39 ` Jens Axboe
0 siblings, 0 replies; 13+ messages in thread
From: Jens Axboe @ 2020-06-02 21:39 UTC (permalink / raw)
To: Jann Horn; +Cc: Clay Harris, io-uring
On 6/2/20 1:16 PM, Jann Horn wrote:
> On Tue, Jun 2, 2020 at 8:42 PM Jens Axboe <[email protected]> wrote:
>> On 6/2/20 12:22 PM, Jann Horn wrote:
>>> On Sun, May 31, 2020 at 10:19 PM Jens Axboe <[email protected]> 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: IORING_OP_CLOSE fails on fd opened with O_PATH
2020-05-31 14:46 ` Jens Axboe
2020-05-31 20:19 ` Jens Axboe
@ 2020-06-08 11:21 ` Clay Harris
2020-06-08 20:19 ` Jens Axboe
1 sibling, 1 reply; 13+ messages in thread
From: Clay Harris @ 2020-06-08 11:21 UTC (permalink / raw)
To: Jens Axboe; +Cc: io-uring
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: IORING_OP_CLOSE fails on fd opened with O_PATH
2020-06-08 11:21 ` Clay Harris
@ 2020-06-08 20:19 ` Jens Axboe
2020-06-09 1:40 ` Clay Harris
0 siblings, 1 reply; 13+ messages in thread
From: Jens Axboe @ 2020-06-08 20:19 UTC (permalink / raw)
To: Clay Harris; +Cc: io-uring
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: IORING_OP_CLOSE fails on fd opened with O_PATH
2020-06-08 20:19 ` Jens Axboe
@ 2020-06-09 1:40 ` Clay Harris
2020-06-09 2:14 ` Jens Axboe
0 siblings, 1 reply; 13+ messages in thread
From: Clay Harris @ 2020-06-09 1:40 UTC (permalink / raw)
To: Jens Axboe; +Cc: io-uring
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: IORING_OP_CLOSE fails on fd opened with O_PATH
2020-06-09 1:40 ` Clay Harris
@ 2020-06-09 2:14 ` Jens Axboe
2020-06-09 5:16 ` Clay Harris
0 siblings, 1 reply; 13+ messages in thread
From: Jens Axboe @ 2020-06-09 2:14 UTC (permalink / raw)
To: Clay Harris; +Cc: io-uring
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: IORING_OP_CLOSE fails on fd opened with O_PATH
2020-06-09 2:14 ` Jens Axboe
@ 2020-06-09 5:16 ` Clay Harris
2020-06-10 1:52 ` Jens Axboe
0 siblings, 1 reply; 13+ messages in thread
From: Clay Harris @ 2020-06-09 5:16 UTC (permalink / raw)
To: Jens Axboe; +Cc: io-uring
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: IORING_OP_CLOSE fails on fd opened with O_PATH
2020-06-09 5:16 ` Clay Harris
@ 2020-06-10 1:52 ` Jens Axboe
0 siblings, 0 replies; 13+ messages in thread
From: Jens Axboe @ 2020-06-10 1:52 UTC (permalink / raw)
To: Clay Harris; +Cc: io-uring
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
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-06-10 1:52 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-05-31 12:47 IORING_OP_CLOSE fails on fd opened with O_PATH Clay Harris
2020-05-31 14:46 ` Jens Axboe
2020-05-31 20:19 ` Jens Axboe
2020-06-02 18:22 ` Jann Horn
2020-06-02 18:42 ` Jens Axboe
2020-06-02 19:16 ` Jann Horn
2020-06-02 21:39 ` Jens Axboe
2020-06-08 11:21 ` Clay Harris
2020-06-08 20:19 ` Jens Axboe
2020-06-09 1:40 ` Clay Harris
2020-06-09 2:14 ` Jens Axboe
2020-06-09 5:16 ` Clay Harris
2020-06-10 1:52 ` Jens Axboe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox