[PATCH] io_uring: make OP_CLOSE consistent direct open

[PATCH] io_uring: make OP_CLOSE consistent direct open

[Pavel Begunkov]

From recently open/accept are now able to manipulate fixed file table,
but it's inconsistent that close can't. Close the gap, keep API same as
with open/accept, i.e. via sqe->file_slot.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 8317c360f7a4..ad71c7ef7f6d 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -503,6 +503,7 @@ struct io_poll_update {
 struct io_close {
 	struct file			*file;
 	int				fd;
+	u32				file_slot;
 };
 
 struct io_timeout_data {
@@ -1099,6 +1100,8 @@ static int io_req_prep_async(struct io_kiocb *req);
 
 static int io_install_fixed_file(struct io_kiocb *req, struct file *file,
 				 unsigned int issue_flags, u32 slot_index);
+static int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags);
+
 static enum hrtimer_restart io_link_timeout_fn(struct hrtimer *timer);
 
 static struct kmem_cache *req_cachep;
@@ -4590,12 +4593,16 @@ static int io_close_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 	if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
 		return -EINVAL;
 	if (sqe->ioprio || sqe->off || sqe->addr || sqe->len ||
-	    sqe->rw_flags || sqe->buf_index || sqe->splice_fd_in)
+	    sqe->rw_flags || sqe->buf_index)
 		return -EINVAL;
 	if (req->flags & REQ_F_FIXED_FILE)
 		return -EBADF;
 
 	req->close.fd = READ_ONCE(sqe->fd);
+	req->close.file_slot = READ_ONCE(sqe->file_index);
+	if (req->close.file_slot && req->close.fd)
+		return -EINVAL;
+
 	return 0;
 }
 
@@ -4607,6 +4614,11 @@ static int io_close(struct io_kiocb *req, unsigned int issue_flags)
 	struct file *file = NULL;
 	int ret = -EBADF;
 
+	if (req->close.file_slot) {
+		ret = io_close_fixed(req, issue_flags);
+		goto err;
+	}
+
 	spin_lock(&files->file_lock);
 	fdt = files_fdtable(files);
 	if (close->fd >= fdt->max_fds) {
@@ -8400,6 +8412,44 @@ static int io_install_fixed_file(struct io_kiocb *req, struct file *file,
 	return ret;
 }
 
+static int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags)
+{
+	unsigned int offset = req->close.file_slot - 1;
+	struct io_ring_ctx *ctx = req->ctx;
+	struct io_fixed_file *file_slot;
+	struct file *file;
+	int ret, i;
+
+	io_ring_submit_lock(ctx, !(issue_flags & IO_URING_F_NONBLOCK));
+	ret = -ENXIO;
+	if (unlikely(!ctx->file_data))
+		goto out;
+	ret = -EINVAL;
+	if (offset >= ctx->nr_user_files)
+		goto out;
+	ret = io_rsrc_node_switch_start(ctx);
+	if (ret)
+		goto out;
+
+	i = array_index_nospec(offset, ctx->nr_user_files);
+	file_slot = io_fixed_file_slot(&ctx->file_table, i);
+	ret = -EBADF;
+	if (!file_slot->file_ptr)
+		goto out;
+
+	file = (struct file *)(file_slot->file_ptr & FFS_MASK);
+	ret = io_queue_rsrc_removal(ctx->file_data, offset, ctx->rsrc_node, file);
+	if (ret)
+		goto out;
+
+	file_slot->file_ptr = 0;
+	io_rsrc_node_switch(ctx, ctx->file_data);
+	ret = 0;
+out:
+	io_ring_submit_unlock(ctx, !(issue_flags & IO_URING_F_NONBLOCK));
+	return ret;
+}
+
 static int __io_sqe_files_update(struct io_ring_ctx *ctx,
 				 struct io_uring_rsrc_update2 *up,
 				 unsigned nr_args)
-- 
2.33.0

Re: [PATCH] io_uring: make OP_CLOSE consistent direct open

[Jens Axboe]

On 9/24/21 1:04 PM, Pavel Begunkov wrote:
> From recently open/accept are now able to manipulate fixed file table,
> but it's inconsistent that close can't. Close the gap, keep API same as
> with open/accept, i.e. via sqe->file_slot.

I really think we should do this for 5.15 to make the API a bit more
sane from the user point of view, folks definitely expect being able
to use IORING_OP_CLOSE with a fixed file that they got with IORING_OP_OPEN,
for example.

How about this small tweak, basically making it follow the same rules
as other commands that do fixed files:

1) Require IOSQE_FIXED_FILE to be set for a direct close. sqe->file_index
   will be the descriptor to close in that case. If sqe->fd is set, we
   -EINVAL the request.

2) If IOSQE_FIXED_FILE isn't set, it's a normal close. As before, if
   sqe->file_index is set and IOSQE_FIXED_FILE isn't, then we -EINVAL
   the request.

Basically this incremental on top of yours.


diff --git a/fs/io_uring.c b/fs/io_uring.c
index 82f867983bb3..dc6e3699779d 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -4596,12 +4596,12 @@ static int io_close_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 	if (sqe->ioprio || sqe->off || sqe->addr || sqe->len ||
 	    sqe->rw_flags || sqe->buf_index)
 		return -EINVAL;
-	if (req->flags & REQ_F_FIXED_FILE)
-		return -EBADF;
 
 	req->close.fd = READ_ONCE(sqe->fd);
 	req->close.file_slot = READ_ONCE(sqe->file_index);
-	if (req->close.file_slot && req->close.fd)
+	if (!(req->flags & REQ_F_FIXED_FILE) && req->close.file_slot)
+		return -EINVAL;
+	else if (req->close.fd)
 		return -EINVAL;
 
 	return 0;
@@ -4615,7 +4615,7 @@ static int io_close(struct io_kiocb *req, unsigned int issue_flags)
 	struct file *file = NULL;
 	int ret = -EBADF;
 
-	if (req->close.file_slot) {
+	if (req->flags & REQ_F_FIXED_FILE) {
 		ret = io_close_fixed(req, issue_flags);
 		goto err;
 	}

-- 
Jens Axboe

Re: [PATCH] io_uring: make OP_CLOSE consistent direct open

[Jens Axboe]

On 9/24/21 1:04 PM, Pavel Begunkov wrote:
> From recently open/accept are now able to manipulate fixed file table,
> but it's inconsistent that close can't. Close the gap, keep API same as
> with open/accept, i.e. via sqe->file_slot.

Applied, thanks.

-- 
Jens Axboe

Re: [PATCH] io_uring: make OP_CLOSE consistent direct open

[Jens Axboe]

On 9/24/21 1:58 PM, Jens Axboe wrote:
> On 9/24/21 1:04 PM, Pavel Begunkov wrote:
>> From recently open/accept are now able to manipulate fixed file table,
>> but it's inconsistent that close can't. Close the gap, keep API same as
>> with open/accept, i.e. via sqe->file_slot.
> 
> Applied, thanks.

Wrong message I replied to, that was for the 2 liburing test
improvements!

-- 
Jens Axboe

Re: [PATCH] io_uring: make OP_CLOSE consistent direct open

[Jens Axboe]

On 9/24/21 1:57 PM, Jens Axboe wrote:
> On 9/24/21 1:04 PM, Pavel Begunkov wrote:
>> From recently open/accept are now able to manipulate fixed file table,
>> but it's inconsistent that close can't. Close the gap, keep API same as
>> with open/accept, i.e. via sqe->file_slot.
> 
> I really think we should do this for 5.15 to make the API a bit more
> sane from the user point of view, folks definitely expect being able
> to use IORING_OP_CLOSE with a fixed file that they got with IORING_OP_OPEN,
> for example.
> 
> How about this small tweak, basically making it follow the same rules
> as other commands that do fixed files:
> 
> 1) Require IOSQE_FIXED_FILE to be set for a direct close. sqe->file_index
>    will be the descriptor to close in that case. If sqe->fd is set, we
>    -EINVAL the request.
> 
> 2) If IOSQE_FIXED_FILE isn't set, it's a normal close. As before, if
>    sqe->file_index is set and IOSQE_FIXED_FILE isn't, then we -EINVAL
>    the request.
> 
> Basically this incremental on top of yours.

Hmm, we don't require that for open or accept. Why not? Seems a bit
counter intuitive. But maybe it's better we do this one as-is, and then
do a followup patch that solidifies the fact that IOSQE_FIXED_FILE
should be set for direct open/accept/close.

-- 
Jens Axboe

Re: [PATCH] io_uring: make OP_CLOSE consistent direct open

[Pavel Begunkov]

On 9/24/21 9:06 PM, Jens Axboe wrote:
> On 9/24/21 1:57 PM, Jens Axboe wrote:
>> On 9/24/21 1:04 PM, Pavel Begunkov wrote:
>>> From recently open/accept are now able to manipulate fixed file table,
>>> but it's inconsistent that close can't. Close the gap, keep API same as
>>> with open/accept, i.e. via sqe->file_slot.
>>
>> I really think we should do this for 5.15 to make the API a bit more
>> sane from the user point of view, folks definitely expect being able
>> to use IORING_OP_CLOSE with a fixed file that they got with IORING_OP_OPEN,
>> for example.
>>
>> How about this small tweak, basically making it follow the same rules
>> as other commands that do fixed files:
>>
>> 1) Require IOSQE_FIXED_FILE to be set for a direct close. sqe->file_index
>>    will be the descriptor to close in that case. If sqe->fd is set, we
>>    -EINVAL the request.
>>
>> 2) If IOSQE_FIXED_FILE isn't set, it's a normal close. As before, if
>>    sqe->file_index is set and IOSQE_FIXED_FILE isn't, then we -EINVAL
>>    the request.
>>
>> Basically this incremental on top of yours.
> 
> Hmm, we don't require that for open or accept. Why not? Seems a bit
> counter intuitive. But maybe it's better we do this one as-is, and then

Accept takes a fd as an argument and so IOSQE_FIXED_FILE already applies
to it and can't be used as described. Close is just made consistent with
the rest.


> do a followup patch that solidifies the fact that IOSQE_FIXED_FILE
> should be set for direct open/accept/close.
> 

-- 
Pavel Begunkov

Re: [PATCH] io_uring: make OP_CLOSE consistent direct open

[Jens Axboe]

On 9/24/21 2:11 PM, Pavel Begunkov wrote:
> On 9/24/21 9:06 PM, Jens Axboe wrote:
>> On 9/24/21 1:57 PM, Jens Axboe wrote:
>>> On 9/24/21 1:04 PM, Pavel Begunkov wrote:
>>>> From recently open/accept are now able to manipulate fixed file table,
>>>> but it's inconsistent that close can't. Close the gap, keep API same as
>>>> with open/accept, i.e. via sqe->file_slot.
>>>
>>> I really think we should do this for 5.15 to make the API a bit more
>>> sane from the user point of view, folks definitely expect being able
>>> to use IORING_OP_CLOSE with a fixed file that they got with IORING_OP_OPEN,
>>> for example.
>>>
>>> How about this small tweak, basically making it follow the same rules
>>> as other commands that do fixed files:
>>>
>>> 1) Require IOSQE_FIXED_FILE to be set for a direct close. sqe->file_index
>>>    will be the descriptor to close in that case. If sqe->fd is set, we
>>>    -EINVAL the request.
>>>
>>> 2) If IOSQE_FIXED_FILE isn't set, it's a normal close. As before, if
>>>    sqe->file_index is set and IOSQE_FIXED_FILE isn't, then we -EINVAL
>>>    the request.
>>>
>>> Basically this incremental on top of yours.
>>
>> Hmm, we don't require that for open or accept. Why not? Seems a bit
>> counter intuitive. But maybe it's better we do this one as-is, and then
> 
> Accept takes a fd as an argument and so IOSQE_FIXED_FILE already applies
> to it and can't be used as described. Close is just made consistent with
> the rest.

What I'm saying is why don't we make IOSQE_FIXED_FILE for open/accept
consistent as well?

-- 
Jens Axboe

Re: [PATCH] io_uring: make OP_CLOSE consistent direct open

[Pavel Begunkov]

On 9/24/21 9:19 PM, Jens Axboe wrote:
> On 9/24/21 2:11 PM, Pavel Begunkov wrote:
>> On 9/24/21 9:06 PM, Jens Axboe wrote:
>>> On 9/24/21 1:57 PM, Jens Axboe wrote:
>>>> On 9/24/21 1:04 PM, Pavel Begunkov wrote:
>>>>> From recently open/accept are now able to manipulate fixed file table,
>>>>> but it's inconsistent that close can't. Close the gap, keep API same as
>>>>> with open/accept, i.e. via sqe->file_slot.
>>>>
>>>> I really think we should do this for 5.15 to make the API a bit more
>>>> sane from the user point of view, folks definitely expect being able
>>>> to use IORING_OP_CLOSE with a fixed file that they got with IORING_OP_OPEN,
>>>> for example.
>>>>
>>>> How about this small tweak, basically making it follow the same rules
>>>> as other commands that do fixed files:
>>>>
>>>> 1) Require IOSQE_FIXED_FILE to be set for a direct close. sqe->file_index
>>>>    will be the descriptor to close in that case. If sqe->fd is set, we
>>>>    -EINVAL the request.
>>>>
>>>> 2) If IOSQE_FIXED_FILE isn't set, it's a normal close. As before, if
>>>>    sqe->file_index is set and IOSQE_FIXED_FILE isn't, then we -EINVAL
>>>>    the request.
>>>>
>>>> Basically this incremental on top of yours.
>>>
>>> Hmm, we don't require that for open or accept. Why not? Seems a bit
>>> counter intuitive. But maybe it's better we do this one as-is, and then
>>
>> Accept takes a fd as an argument and so IOSQE_FIXED_FILE already applies
>> to it and can't be used as described. Close is just made consistent with
>> the rest.
> 
> What I'm saying is why don't we make IOSQE_FIXED_FILE for open/accept
> consistent as well?

The flag is already used for accept but for a different purpose 


[IORING_OP_ACCEPT] = {
	.needs_file		= 1,

if (io_op_defs[req->opcode].needs_file) {
	req->file = io_file_get(ctx, req, READ_ONCE(sqe->fd),
				(sqe_flags & IOSQE_FIXED_FILE));

-- 
Pavel Begunkov

Re: [PATCH] io_uring: make OP_CLOSE consistent direct open

[Jens Axboe]

On 9/24/21 2:21 PM, Pavel Begunkov wrote:
> On 9/24/21 9:19 PM, Jens Axboe wrote:
>> On 9/24/21 2:11 PM, Pavel Begunkov wrote:
>>> On 9/24/21 9:06 PM, Jens Axboe wrote:
>>>> On 9/24/21 1:57 PM, Jens Axboe wrote:
>>>>> On 9/24/21 1:04 PM, Pavel Begunkov wrote:
>>>>>> From recently open/accept are now able to manipulate fixed file table,
>>>>>> but it's inconsistent that close can't. Close the gap, keep API same as
>>>>>> with open/accept, i.e. via sqe->file_slot.
>>>>>
>>>>> I really think we should do this for 5.15 to make the API a bit more
>>>>> sane from the user point of view, folks definitely expect being able
>>>>> to use IORING_OP_CLOSE with a fixed file that they got with IORING_OP_OPEN,
>>>>> for example.
>>>>>
>>>>> How about this small tweak, basically making it follow the same rules
>>>>> as other commands that do fixed files:
>>>>>
>>>>> 1) Require IOSQE_FIXED_FILE to be set for a direct close. sqe->file_index
>>>>>    will be the descriptor to close in that case. If sqe->fd is set, we
>>>>>    -EINVAL the request.
>>>>>
>>>>> 2) If IOSQE_FIXED_FILE isn't set, it's a normal close. As before, if
>>>>>    sqe->file_index is set and IOSQE_FIXED_FILE isn't, then we -EINVAL
>>>>>    the request.
>>>>>
>>>>> Basically this incremental on top of yours.
>>>>
>>>> Hmm, we don't require that for open or accept. Why not? Seems a bit
>>>> counter intuitive. But maybe it's better we do this one as-is, and then
>>>
>>> Accept takes a fd as an argument and so IOSQE_FIXED_FILE already applies
>>> to it and can't be used as described. Close is just made consistent with
>>> the rest.
>>
>> What I'm saying is why don't we make IOSQE_FIXED_FILE for open/accept
>> consistent as well?
> 
> The flag is already used for accept but for a different purpose 
> 
> 
> [IORING_OP_ACCEPT] = {
> 	.needs_file		= 1,
> 
> if (io_op_defs[req->opcode].needs_file) {
> 	req->file = io_file_get(ctx, req, READ_ONCE(sqe->fd),
> 				(sqe_flags & IOSQE_FIXED_FILE));

Oh yeah, I guess that won't fly then. Let's just go with this one then,
at least there's an explanation for it and they are consistent in using
->file_index to gate it.

-- 
Jens Axboe