relative openat dirfd reference on submit

relative openat dirfd reference on submit

[Vito Caputo]

Hello list,

I've been tinkering a bit with some async continuation passing style
IO-oriented code employing liburing.  This exposed a kind of awkward
behavior I suspect could be better from an ergonomics perspective.

Imagine a bunch of OPENAT SQEs have been prepared, and they're all
relative to a common dirfd.  Once io_uring_submit() has consumed all
these SQEs across the syscall boundary, logically it seems the dirfd
should be safe to close, since these dirfd-dependent operations have
all been submitted to the kernel.

But when I attempted this, the subsequent OPENAT CQE results were all
-EBADFD errors.  It appeared the submit didn't add any references to
the dependent dirfd.

To work around this, I resorted to stowing the dirfd and maintaining a
shared refcount in the closures associated with these SQEs and
executed on their CQEs.  This effectively forced replicating the
batched relationship implicit in the shared parent dirfd, where I
otherwise had zero need to.  Just so I could defer closing the dirfd
until once all these closures had run on their respective CQE arrivals
and the refcount for the batch had reached zero.

It doesn't seem right.  If I ensure sufficient queue depth and
explicitly flush all the dependent SQEs beforehand
w/io_uring_submit(), it seems like I should be able to immediately
close(dirfd) and have the close be automagically deferred until the
last dependent CQE removes its reference from the kernel side.

Regards,
Vito Caputo

Re: relative openat dirfd reference on submit

[Jens Axboe]

On 11/2/20 1:52 PM, Vito Caputo wrote:
> Hello list,
> 
> I've been tinkering a bit with some async continuation passing style
> IO-oriented code employing liburing.  This exposed a kind of awkward
> behavior I suspect could be better from an ergonomics perspective.
> 
> Imagine a bunch of OPENAT SQEs have been prepared, and they're all
> relative to a common dirfd.  Once io_uring_submit() has consumed all
> these SQEs across the syscall boundary, logically it seems the dirfd
> should be safe to close, since these dirfd-dependent operations have
> all been submitted to the kernel.
> 
> But when I attempted this, the subsequent OPENAT CQE results were all
> -EBADFD errors.  It appeared the submit didn't add any references to
> the dependent dirfd.
> 
> To work around this, I resorted to stowing the dirfd and maintaining a
> shared refcount in the closures associated with these SQEs and
> executed on their CQEs.  This effectively forced replicating the
> batched relationship implicit in the shared parent dirfd, where I
> otherwise had zero need to.  Just so I could defer closing the dirfd
> until once all these closures had run on their respective CQE arrivals
> and the refcount for the batch had reached zero.
> 
> It doesn't seem right.  If I ensure sufficient queue depth and
> explicitly flush all the dependent SQEs beforehand
> w/io_uring_submit(), it seems like I should be able to immediately
> close(dirfd) and have the close be automagically deferred until the
> last dependent CQE removes its reference from the kernel side.

We pass the 'dfd' straight on, and only the async part acts on it.
Which is why it needs to be kept open. But I wonder if we can get
around it by just pinning the fd for the duration. Since you didn't
include a test case, can you try with this patch applied? Totally
untested...

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 1f555e3c44cd..b3a647dd206b 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -3769,6 +3769,9 @@ static int __io_openat_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe
 		req->open.how.flags |= O_LARGEFILE;
 
 	req->open.dfd = READ_ONCE(sqe->fd);
+	if (req->open.dfd != -1 && req->open.dfd != AT_FDCWD)
+		req->file = fget(req->open.dfd);
+
 	fname = u64_to_user_ptr(READ_ONCE(sqe->addr));
 	req->open.filename = getname(fname);
 	if (IS_ERR(req->open.filename)) {
@@ -3841,6 +3844,8 @@ static int io_openat2(struct io_kiocb *req, bool force_nonblock)
 	}
 err:
 	putname(req->open.filename);
+	if (req->file)
+		fput(req->file);
 	req->flags &= ~REQ_F_NEED_CLEANUP;
 	if (ret < 0)
 		req_set_fail_links(req);
@@ -5876,6 +5881,8 @@ static void __io_clean_op(struct io_kiocb *req)
 		case IORING_OP_OPENAT2:
 			if (req->open.filename)
 				putname(req->open.filename);
+			if (req->file)
+				fput(req->file);
 			break;
 		}
 		req->flags &= ~REQ_F_NEED_CLEANUP;

-- 
Jens Axboe

Re: relative openat dirfd reference on submit

[Pavel Begunkov]

On 03/11/2020 00:05, Jens Axboe wrote:
> On 11/2/20 1:52 PM, Vito Caputo wrote:
>> Hello list,
>>
>> I've been tinkering a bit with some async continuation passing style
>> IO-oriented code employing liburing.  This exposed a kind of awkward
>> behavior I suspect could be better from an ergonomics perspective.
>>
>> Imagine a bunch of OPENAT SQEs have been prepared, and they're all
>> relative to a common dirfd.  Once io_uring_submit() has consumed all
>> these SQEs across the syscall boundary, logically it seems the dirfd
>> should be safe to close, since these dirfd-dependent operations have
>> all been submitted to the kernel.
>>
>> But when I attempted this, the subsequent OPENAT CQE results were all
>> -EBADFD errors.  It appeared the submit didn't add any references to
>> the dependent dirfd.
>>
>> To work around this, I resorted to stowing the dirfd and maintaining a
>> shared refcount in the closures associated with these SQEs and
>> executed on their CQEs.  This effectively forced replicating the
>> batched relationship implicit in the shared parent dirfd, where I
>> otherwise had zero need to.  Just so I could defer closing the dirfd
>> until once all these closures had run on their respective CQE arrivals
>> and the refcount for the batch had reached zero.
>>
>> It doesn't seem right.  If I ensure sufficient queue depth and
>> explicitly flush all the dependent SQEs beforehand
>> w/io_uring_submit(), it seems like I should be able to immediately
>> close(dirfd) and have the close be automagically deferred until the
>> last dependent CQE removes its reference from the kernel side.
> 
> We pass the 'dfd' straight on, and only the async part acts on it.
> Which is why it needs to be kept open. But I wonder if we can get
> around it by just pinning the fd for the duration. Since you didn't
> include a test case, can you try with this patch applied? Totally
> untested...

afaik this doesn't pin an fd in a file table, so the app closes and
dfd right after submit and then do_filp_open() tries to look up
closed dfd. Doesn't seem to work, and we need to pass that struct
file to do_filp_open().

> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 1f555e3c44cd..b3a647dd206b 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -3769,6 +3769,9 @@ static int __io_openat_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe
>  		req->open.how.flags |= O_LARGEFILE;
>  
>  	req->open.dfd = READ_ONCE(sqe->fd);
> +	if (req->open.dfd != -1 && req->open.dfd != AT_FDCWD)
> +		req->file = fget(req->open.dfd);
> +
>  	fname = u64_to_user_ptr(READ_ONCE(sqe->addr));
>  	req->open.filename = getname(fname);
>  	if (IS_ERR(req->open.filename)) {o 
> @@ -3841,6 +3844,8 @@ static int io_openat2(struct io_kiocb *req, bool force_nonblock)
>  	}
>  err:
>  	putname(req->open.filename);
> +	if (req->file)
> +		fput(req->file);
>  	req->flags &= ~REQ_F_NEED_CLEANUP;
>  	if (ret < 0)
>  		req_set_fail_links(req);
> @@ -5876,6 +5881,8 @@ static void __io_clean_op(struct io_kiocb *req)
>  		case IORING_OP_OPENAT2:
>  			if (req->open.filename)
>  				putname(req->open.filename);
> +			if (req->file)
> +				fput(req->file);
>  			break;
>  		}
>  		req->flags &= ~REQ_F_NEED_CLEANUP;
> 

-- 
Pavel Begunkov

Re: relative openat dirfd reference on submit

[Jens Axboe]

On 11/2/20 5:17 PM, Pavel Begunkov wrote:
> On 03/11/2020 00:05, Jens Axboe wrote:
>> On 11/2/20 1:52 PM, Vito Caputo wrote:
>>> Hello list,
>>>
>>> I've been tinkering a bit with some async continuation passing style
>>> IO-oriented code employing liburing.  This exposed a kind of awkward
>>> behavior I suspect could be better from an ergonomics perspective.
>>>
>>> Imagine a bunch of OPENAT SQEs have been prepared, and they're all
>>> relative to a common dirfd.  Once io_uring_submit() has consumed all
>>> these SQEs across the syscall boundary, logically it seems the dirfd
>>> should be safe to close, since these dirfd-dependent operations have
>>> all been submitted to the kernel.
>>>
>>> But when I attempted this, the subsequent OPENAT CQE results were all
>>> -EBADFD errors.  It appeared the submit didn't add any references to
>>> the dependent dirfd.
>>>
>>> To work around this, I resorted to stowing the dirfd and maintaining a
>>> shared refcount in the closures associated with these SQEs and
>>> executed on their CQEs.  This effectively forced replicating the
>>> batched relationship implicit in the shared parent dirfd, where I
>>> otherwise had zero need to.  Just so I could defer closing the dirfd
>>> until once all these closures had run on their respective CQE arrivals
>>> and the refcount for the batch had reached zero.
>>>
>>> It doesn't seem right.  If I ensure sufficient queue depth and
>>> explicitly flush all the dependent SQEs beforehand
>>> w/io_uring_submit(), it seems like I should be able to immediately
>>> close(dirfd) and have the close be automagically deferred until the
>>> last dependent CQE removes its reference from the kernel side.
>>
>> We pass the 'dfd' straight on, and only the async part acts on it.
>> Which is why it needs to be kept open. But I wonder if we can get
>> around it by just pinning the fd for the duration. Since you didn't
>> include a test case, can you try with this patch applied? Totally
>> untested...
> 
> afaik this doesn't pin an fd in a file table, so the app closes and
> dfd right after submit and then do_filp_open() tries to look up
> closed dfd. Doesn't seem to work, and we need to pass that struct
> file to do_filp_open().

Yeah, I just double checked, and it's just referenced, but close() will
still make it NULL in the file table. So won't work... We'll have to
live with it for now, I'm afraid.

-- 
Jens Axboe

Re: relative openat dirfd reference on submit

[Pavel Begunkov]

On 03/11/2020 00:34, Jens Axboe wrote:
> On 11/2/20 5:17 PM, Pavel Begunkov wrote:
>> On 03/11/2020 00:05, Jens Axboe wrote:
>>> On 11/2/20 1:52 PM, Vito Caputo wrote:
>>>> Hello list,
>>>>
>>>> I've been tinkering a bit with some async continuation passing style
>>>> IO-oriented code employing liburing.  This exposed a kind of awkward
>>>> behavior I suspect could be better from an ergonomics perspective.
>>>>
>>>> Imagine a bunch of OPENAT SQEs have been prepared, and they're all
>>>> relative to a common dirfd.  Once io_uring_submit() has consumed all
>>>> these SQEs across the syscall boundary, logically it seems the dirfd
>>>> should be safe to close, since these dirfd-dependent operations have
>>>> all been submitted to the kernel.
>>>>
>>>> But when I attempted this, the subsequent OPENAT CQE results were all
>>>> -EBADFD errors.  It appeared the submit didn't add any references to
>>>> the dependent dirfd.
>>>>
>>>> To work around this, I resorted to stowing the dirfd and maintaining a
>>>> shared refcount in the closures associated with these SQEs and
>>>> executed on their CQEs.  This effectively forced replicating the
>>>> batched relationship implicit in the shared parent dirfd, where I
>>>> otherwise had zero need to.  Just so I could defer closing the dirfd
>>>> until once all these closures had run on their respective CQE arrivals
>>>> and the refcount for the batch had reached zero.
>>>>
>>>> It doesn't seem right.  If I ensure sufficient queue depth and
>>>> explicitly flush all the dependent SQEs beforehand
>>>> w/io_uring_submit(), it seems like I should be able to immediately
>>>> close(dirfd) and have the close be automagically deferred until the
>>>> last dependent CQE removes its reference from the kernel side.
>>>
>>> We pass the 'dfd' straight on, and only the async part acts on it.
>>> Which is why it needs to be kept open. But I wonder if we can get
>>> around it by just pinning the fd for the duration. Since you didn't
>>> include a test case, can you try with this patch applied? Totally
>>> untested...
>>
>> afaik this doesn't pin an fd in a file table, so the app closes and
>> dfd right after submit and then do_filp_open() tries to look up
>> closed dfd. Doesn't seem to work, and we need to pass that struct
>> file to do_filp_open().
> 
> Yeah, I just double checked, and it's just referenced, but close() will
> still make it NULL in the file table. So won't work... We'll have to
> live with it for now, I'm afraid.

Is there a problem with passing in a struct file? Apart from it
being used deep in open callchains?

-- 
Pavel Begunkov

Re: relative openat dirfd reference on submit

[Jens Axboe]

On 11/2/20 5:41 PM, Pavel Begunkov wrote:
> On 03/11/2020 00:34, Jens Axboe wrote:
>> On 11/2/20 5:17 PM, Pavel Begunkov wrote:
>>> On 03/11/2020 00:05, Jens Axboe wrote:
>>>> On 11/2/20 1:52 PM, Vito Caputo wrote:
>>>>> Hello list,
>>>>>
>>>>> I've been tinkering a bit with some async continuation passing style
>>>>> IO-oriented code employing liburing.  This exposed a kind of awkward
>>>>> behavior I suspect could be better from an ergonomics perspective.
>>>>>
>>>>> Imagine a bunch of OPENAT SQEs have been prepared, and they're all
>>>>> relative to a common dirfd.  Once io_uring_submit() has consumed all
>>>>> these SQEs across the syscall boundary, logically it seems the dirfd
>>>>> should be safe to close, since these dirfd-dependent operations have
>>>>> all been submitted to the kernel.
>>>>>
>>>>> But when I attempted this, the subsequent OPENAT CQE results were all
>>>>> -EBADFD errors.  It appeared the submit didn't add any references to
>>>>> the dependent dirfd.
>>>>>
>>>>> To work around this, I resorted to stowing the dirfd and maintaining a
>>>>> shared refcount in the closures associated with these SQEs and
>>>>> executed on their CQEs.  This effectively forced replicating the
>>>>> batched relationship implicit in the shared parent dirfd, where I
>>>>> otherwise had zero need to.  Just so I could defer closing the dirfd
>>>>> until once all these closures had run on their respective CQE arrivals
>>>>> and the refcount for the batch had reached zero.
>>>>>
>>>>> It doesn't seem right.  If I ensure sufficient queue depth and
>>>>> explicitly flush all the dependent SQEs beforehand
>>>>> w/io_uring_submit(), it seems like I should be able to immediately
>>>>> close(dirfd) and have the close be automagically deferred until the
>>>>> last dependent CQE removes its reference from the kernel side.
>>>>
>>>> We pass the 'dfd' straight on, and only the async part acts on it.
>>>> Which is why it needs to be kept open. But I wonder if we can get
>>>> around it by just pinning the fd for the duration. Since you didn't
>>>> include a test case, can you try with this patch applied? Totally
>>>> untested...
>>>
>>> afaik this doesn't pin an fd in a file table, so the app closes and
>>> dfd right after submit and then do_filp_open() tries to look up
>>> closed dfd. Doesn't seem to work, and we need to pass that struct
>>> file to do_filp_open().
>>
>> Yeah, I just double checked, and it's just referenced, but close() will
>> still make it NULL in the file table. So won't work... We'll have to
>> live with it for now, I'm afraid.
> 
> Is there a problem with passing in a struct file? Apart from it
> being used deep in open callchains?

No technical problems as far as I can tell, just needs doing...

-- 
Jens Axboe

Re: relative openat dirfd reference on submit

[Stefan Metzmacher]

[-- Attachment #1.1: Type: text/plain, Size: 3099 bytes --]

Am 05.11.20 um 00:43 schrieb Jens Axboe:
> On 11/2/20 5:41 PM, Pavel Begunkov wrote:
>> On 03/11/2020 00:34, Jens Axboe wrote:
>>> On 11/2/20 5:17 PM, Pavel Begunkov wrote:
>>>> On 03/11/2020 00:05, Jens Axboe wrote:
>>>>> On 11/2/20 1:52 PM, Vito Caputo wrote:
>>>>>> Hello list,
>>>>>>
>>>>>> I've been tinkering a bit with some async continuation passing style
>>>>>> IO-oriented code employing liburing.  This exposed a kind of awkward
>>>>>> behavior I suspect could be better from an ergonomics perspective.
>>>>>>
>>>>>> Imagine a bunch of OPENAT SQEs have been prepared, and they're all
>>>>>> relative to a common dirfd.  Once io_uring_submit() has consumed all
>>>>>> these SQEs across the syscall boundary, logically it seems the dirfd
>>>>>> should be safe to close, since these dirfd-dependent operations have
>>>>>> all been submitted to the kernel.
>>>>>>
>>>>>> But when I attempted this, the subsequent OPENAT CQE results were all
>>>>>> -EBADFD errors.  It appeared the submit didn't add any references to
>>>>>> the dependent dirfd.
>>>>>>
>>>>>> To work around this, I resorted to stowing the dirfd and maintaining a
>>>>>> shared refcount in the closures associated with these SQEs and
>>>>>> executed on their CQEs.  This effectively forced replicating the
>>>>>> batched relationship implicit in the shared parent dirfd, where I
>>>>>> otherwise had zero need to.  Just so I could defer closing the dirfd
>>>>>> until once all these closures had run on their respective CQE arrivals
>>>>>> and the refcount for the batch had reached zero.
>>>>>>
>>>>>> It doesn't seem right.  If I ensure sufficient queue depth and
>>>>>> explicitly flush all the dependent SQEs beforehand
>>>>>> w/io_uring_submit(), it seems like I should be able to immediately
>>>>>> close(dirfd) and have the close be automagically deferred until the
>>>>>> last dependent CQE removes its reference from the kernel side.
>>>>>
>>>>> We pass the 'dfd' straight on, and only the async part acts on it.
>>>>> Which is why it needs to be kept open. But I wonder if we can get
>>>>> around it by just pinning the fd for the duration. Since you didn't
>>>>> include a test case, can you try with this patch applied? Totally
>>>>> untested...
>>>>
>>>> afaik this doesn't pin an fd in a file table, so the app closes and
>>>> dfd right after submit and then do_filp_open() tries to look up
>>>> closed dfd. Doesn't seem to work, and we need to pass that struct
>>>> file to do_filp_open().
>>>
>>> Yeah, I just double checked, and it's just referenced, but close() will
>>> still make it NULL in the file table. So won't work... We'll have to
>>> live with it for now, I'm afraid.
>>
>> Is there a problem with passing in a struct file? Apart from it
>> being used deep in open callchains?
> 
> No technical problems as far as I can tell, just needs doing...

That would also allow fixed files to be used as dirfd, correct?
If that's the case it would be great to have a way to install the resulting
fd also (or maybe only) as fixed file.

metze



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: relative openat dirfd reference on submit

[Jens Axboe]

On 11/5/20 1:45 AM, Stefan Metzmacher wrote:
> Am 05.11.20 um 00:43 schrieb Jens Axboe:
>> On 11/2/20 5:41 PM, Pavel Begunkov wrote:
>>> On 03/11/2020 00:34, Jens Axboe wrote:
>>>> On 11/2/20 5:17 PM, Pavel Begunkov wrote:
>>>>> On 03/11/2020 00:05, Jens Axboe wrote:
>>>>>> On 11/2/20 1:52 PM, Vito Caputo wrote:
>>>>>>> Hello list,
>>>>>>>
>>>>>>> I've been tinkering a bit with some async continuation passing style
>>>>>>> IO-oriented code employing liburing.  This exposed a kind of awkward
>>>>>>> behavior I suspect could be better from an ergonomics perspective.
>>>>>>>
>>>>>>> Imagine a bunch of OPENAT SQEs have been prepared, and they're all
>>>>>>> relative to a common dirfd.  Once io_uring_submit() has consumed all
>>>>>>> these SQEs across the syscall boundary, logically it seems the dirfd
>>>>>>> should be safe to close, since these dirfd-dependent operations have
>>>>>>> all been submitted to the kernel.
>>>>>>>
>>>>>>> But when I attempted this, the subsequent OPENAT CQE results were all
>>>>>>> -EBADFD errors.  It appeared the submit didn't add any references to
>>>>>>> the dependent dirfd.
>>>>>>>
>>>>>>> To work around this, I resorted to stowing the dirfd and maintaining a
>>>>>>> shared refcount in the closures associated with these SQEs and
>>>>>>> executed on their CQEs.  This effectively forced replicating the
>>>>>>> batched relationship implicit in the shared parent dirfd, where I
>>>>>>> otherwise had zero need to.  Just so I could defer closing the dirfd
>>>>>>> until once all these closures had run on their respective CQE arrivals
>>>>>>> and the refcount for the batch had reached zero.
>>>>>>>
>>>>>>> It doesn't seem right.  If I ensure sufficient queue depth and
>>>>>>> explicitly flush all the dependent SQEs beforehand
>>>>>>> w/io_uring_submit(), it seems like I should be able to immediately
>>>>>>> close(dirfd) and have the close be automagically deferred until the
>>>>>>> last dependent CQE removes its reference from the kernel side.
>>>>>>
>>>>>> We pass the 'dfd' straight on, and only the async part acts on it.
>>>>>> Which is why it needs to be kept open. But I wonder if we can get
>>>>>> around it by just pinning the fd for the duration. Since you didn't
>>>>>> include a test case, can you try with this patch applied? Totally
>>>>>> untested...
>>>>>
>>>>> afaik this doesn't pin an fd in a file table, so the app closes and
>>>>> dfd right after submit and then do_filp_open() tries to look up
>>>>> closed dfd. Doesn't seem to work, and we need to pass that struct
>>>>> file to do_filp_open().
>>>>
>>>> Yeah, I just double checked, and it's just referenced, but close() will
>>>> still make it NULL in the file table. So won't work... We'll have to
>>>> live with it for now, I'm afraid.
>>>
>>> Is there a problem with passing in a struct file? Apart from it
>>> being used deep in open callchains?
>>
>> No technical problems as far as I can tell, just needs doing...
> 
> That would also allow fixed files to be used as dirfd, correct?

Correct

> If that's the case it would be great to have a way to install the resulting
> fd also (or maybe only) as fixed file.

That might be handy, yes.

-- 
Jens Axboe