[PATCH 1/1] io_uring: clean up io_uring_register_get_file()

[PATCH 1/1] io_uring: clean up io_uring_register_get_file()

[Pavel Begunkov]

Make it always reference the returned file. It's safer, especially with
unregistrations happening under it. And it makes the api cleaner with no
conditional clean ups by the caller.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/register.c | 6 ++++--
 io_uring/rsrc.c     | 4 ++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/io_uring/register.c b/io_uring/register.c
index 5e48413706ac..a93c979c2f38 100644
--- a/io_uring/register.c
+++ b/io_uring/register.c
@@ -841,6 +841,8 @@ struct file *io_uring_register_get_file(unsigned int fd, bool registered)
 			return ERR_PTR(-EINVAL);
 		fd = array_index_nospec(fd, IO_RINGFD_REG_MAX);
 		file = tctx->registered_rings[fd];
+		if (file)
+			get_file(file);
 	} else {
 		file = fget(fd);
 	}
@@ -907,7 +909,7 @@ SYSCALL_DEFINE4(io_uring_register, unsigned int, fd, unsigned int, opcode,
 	trace_io_uring_register(ctx, opcode, ctx->file_table.data.nr,
 				ctx->buf_table.nr, ret);
 	mutex_unlock(&ctx->uring_lock);
-	if (!use_registered_ring)
-		fput(file);
+
+	fput(file);
 	return ret;
 }
diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
index 964a47c8d85e..792c22b6f2d4 100644
--- a/io_uring/rsrc.c
+++ b/io_uring/rsrc.c
@@ -1073,7 +1073,7 @@ int io_register_clone_buffers(struct io_ring_ctx *ctx, void __user *arg)
 	if (IS_ERR(file))
 		return PTR_ERR(file);
 	ret = io_clone_buffers(ctx, file->private_data, &buf);
-	if (!registered_src)
-		fput(file);
+
+	fput(file);
 	return ret;
 }
-- 
2.47.1

RE: [PATCH 1/1] io_uring: clean up io_uring_register_get_file()

[lizetao]

Hi,

> -----Original Message-----
> From: Pavel Begunkov <asml.silence@gmail.com>
> Sent: Thursday, January 16, 2025 10:53 AM
> To: io-uring@vger.kernel.org
> Cc: asml.silence@gmail.com
> Subject: [PATCH 1/1] io_uring: clean up io_uring_register_get_file()
> 
> Make it always reference the returned file. It's safer, especially with
> unregistrations happening under it. And it makes the api cleaner with no
> conditional clean ups by the caller.
> 
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  io_uring/register.c | 6 ++++--
>  io_uring/rsrc.c     | 4 ++--
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/io_uring/register.c b/io_uring/register.c index
> 5e48413706ac..a93c979c2f38 100644
> --- a/io_uring/register.c
> +++ b/io_uring/register.c
> @@ -841,6 +841,8 @@ struct file *io_uring_register_get_file(unsigned int fd,
> bool registered)
>  			return ERR_PTR(-EINVAL);
>  		fd = array_index_nospec(fd, IO_RINGFD_REG_MAX);
>  		file = tctx->registered_rings[fd];
> +		if (file)
> +			get_file(file);

Should performance be a priority here?
>  	} else {
>  		file = fget(fd);
>  	}
> @@ -907,7 +909,7 @@ SYSCALL_DEFINE4(io_uring_register, unsigned int, fd,
> unsigned int, opcode,
>  	trace_io_uring_register(ctx, opcode, ctx->file_table.data.nr,
>  				ctx->buf_table.nr, ret);
>  	mutex_unlock(&ctx->uring_lock);
> -	if (!use_registered_ring)
> -		fput(file);
> +
> +	fput(file);
>  	return ret;
>  }
> diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c index
> 964a47c8d85e..792c22b6f2d4 100644
> --- a/io_uring/rsrc.c
> +++ b/io_uring/rsrc.c
> @@ -1073,7 +1073,7 @@ int io_register_clone_buffers(struct io_ring_ctx *ctx,
> void __user *arg)
>  	if (IS_ERR(file))
>  		return PTR_ERR(file);
>  	ret = io_clone_buffers(ctx, file->private_data, &buf);
> -	if (!registered_src)
> -		fput(file);
> +
> +	fput(file);
>  	return ret;
>  }
> --
> 2.47.1
> 


---
Li Zetao

Re: [PATCH 1/1] io_uring: clean up io_uring_register_get_file()

[Jens Axboe]

On 1/16/25 6:09 AM, lizetao wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Pavel Begunkov <asml.silence@gmail.com>
>> Sent: Thursday, January 16, 2025 10:53 AM
>> To: io-uring@vger.kernel.org
>> Cc: asml.silence@gmail.com
>> Subject: [PATCH 1/1] io_uring: clean up io_uring_register_get_file()
>>
>> Make it always reference the returned file. It's safer, especially with
>> unregistrations happening under it. And it makes the api cleaner with no
>> conditional clean ups by the caller.
>>
>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>> ---
>>  io_uring/register.c | 6 ++++--
>>  io_uring/rsrc.c     | 4 ++--
>>  2 files changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/io_uring/register.c b/io_uring/register.c index
>> 5e48413706ac..a93c979c2f38 100644
>> --- a/io_uring/register.c
>> +++ b/io_uring/register.c
>> @@ -841,6 +841,8 @@ struct file *io_uring_register_get_file(unsigned int fd,
>> bool registered)
>>  			return ERR_PTR(-EINVAL);
>>  		fd = array_index_nospec(fd, IO_RINGFD_REG_MAX);
>>  		file = tctx->registered_rings[fd];
>> +		if (file)
>> +			get_file(file);
> 
> Should performance be a priority here?

Performance only really matters for high frequency invocations, of which
the register part is not. So no, should not matter at all.

-- 
Jens Axboe

Re: [PATCH 1/1] io_uring: clean up io_uring_register_get_file()

[Jens Axboe]

On Thu, 16 Jan 2025 02:53:26 +0000, Pavel Begunkov wrote:
> Make it always reference the returned file. It's safer, especially with
> unregistrations happening under it. And it makes the api cleaner with no
> conditional clean ups by the caller.
> 
> 

Applied, thanks!

[1/1] io_uring: clean up io_uring_register_get_file()
      commit: 4415be009035b67b9101cf96b02cfee95621b3a6

Best regards,
-- 
Jens Axboe

RE: [PATCH 1/1] io_uring: clean up io_uring_register_get_file()

[lizetao]

Hi,

> -----Original Message-----
> From: Jens Axboe <axboe@kernel.dk>
> Sent: Thursday, January 16, 2025 10:10 PM
> To: lizetao <lizetao1@huawei.com>; Pavel Begunkov <asml.silence@gmail.com>
> Cc: io-uring@vger.kernel.org
> Subject: Re: [PATCH 1/1] io_uring: clean up io_uring_register_get_file()
> 
> On 1/16/25 6:09 AM, lizetao wrote:
> > Hi,
> >
> >> -----Original Message-----
> >> From: Pavel Begunkov <asml.silence@gmail.com>
> >> Sent: Thursday, January 16, 2025 10:53 AM
> >> To: io-uring@vger.kernel.org
> >> Cc: asml.silence@gmail.com
> >> Subject: [PATCH 1/1] io_uring: clean up io_uring_register_get_file()
> >>
> >> Make it always reference the returned file. It's safer, especially
> >> with unregistrations happening under it. And it makes the api cleaner
> >> with no conditional clean ups by the caller.
> >>
> >> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> >> ---
> >>  io_uring/register.c | 6 ++++--
> >>  io_uring/rsrc.c     | 4 ++--
> >>  2 files changed, 6 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/io_uring/register.c b/io_uring/register.c index
> >> 5e48413706ac..a93c979c2f38 100644
> >> --- a/io_uring/register.c
> >> +++ b/io_uring/register.c
> >> @@ -841,6 +841,8 @@ struct file *io_uring_register_get_file(unsigned
> >> int fd, bool registered)
> >>  			return ERR_PTR(-EINVAL);
> >>  		fd = array_index_nospec(fd, IO_RINGFD_REG_MAX);
> >>  		file = tctx->registered_rings[fd];
> >> +		if (file)
> >> +			get_file(file);
> >
> > Should performance be a priority here?
> 
> Performance only really matters for high frequency invocations, of which the
> register part is not. So no, should not matter at all.
Ok. I got it.

Tested-by: Li Zetao <lizetao1@huawei.com>
Reviewed-by: Li Zetao <lizetao1@huawei.com>
> 
> --
> Jens Axboe

---
Li Zetao