Re: io_uring: Fix exception handling in io_ring_ctx_alloc()

Re: io_uring: Fix exception handling in io_ring_ctx_alloc()

[Markus Elfring]

> The label “err” was used to jump to a kfree() call despite of
> the detail in the implementation of the function “io_ring_ctx_alloc”
> that it was determined already that a corresponding variable contained
> a null pointer because of a failed memory allocation.
>
> 1. Thus use more appropriate labels instead.
>
> 2. Reorder jump targets at the end.
>
> 3. Omit the statement “kfree(ctx->io_bl);”.

Is this patch still in review queues?

See also:
https://lore.kernel.org/cocci/aa867594-e79d-6d08-a08e-8c9e952b4724@web.de/
https://sympa.inria.fr/sympa/arc/cocci/2023-03/msg00114.html

Regards,
Markus

Re: [cocci] [PATCH] io_uring: Fix exception handling in io_ring_ctx_alloc()

[Gabriel Krisman Bertazi]

Markus Elfring <Markus.Elfring@web.de> writes:

> Date: Wed, 29 Mar 2023 17:35:16 +0200
>
> The label “err” was used to jump to a kfree() call despite of
> the detail in the implementation of the function “io_ring_ctx_alloc”
> that it was determined already that a corresponding variable contained
> a null pointer because of a failed memory allocation.
>
> 1. Thus use more appropriate labels instead.
>
> 2. Reorder jump targets at the end.

FWIW, I don't think it makes sense to have the extra labels or re-sort
without a good reason. kfree works fine with the NULL pointers, so there
is no bug to be fixed and moving code around for no reason just makes
life painful for backporters.

Also, the patch no longer applies.

> 3. Omit the statement “kfree(ctx->io_bl);”.

From a quick look, this might still make sense.  can you confirm and make
that change into a separate patch?

-- 
Gabriel Krisman Bertazi

[PATCH v2 0/2] io_uring: Adjustments for io_ring_ctx_alloc()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 10 Jan 2024 21:38:12 +0100

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (2):
  Delete a redundant kfree() call
  Improve exception handling

 io_uring/io_uring.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

--
2.43.0

[PATCH v2 1/2] io_uring: Delete a redundant kfree() call in io_ring_ctx_alloc()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 10 Jan 2024 20:54:43 +0100

Another useful pointer was not reassigned to the data structure member
“io_bl” by this function implementation.
Thus omit a redundant call of the function “kfree” at the end.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---

v2:
A change request by Gabriel Krisman Bertazi was applied here.


 io_uring/io_uring.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 86761ec623f9..c9a63c39cdd0 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -344,7 +344,6 @@ static __cold struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
 err:
 	kfree(ctx->cancel_table.hbs);
 	kfree(ctx->cancel_table_locked.hbs);
-	kfree(ctx->io_bl);
 	xa_destroy(&ctx->io_bl_xa);
 	kfree(ctx);
 	return NULL;
--
2.43.0

[PATCH v2 2/2] io_uring: Improve exception handling in io_ring_ctx_alloc()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 10 Jan 2024 21:15:48 +0100

The label “err” was used to jump to a kfree() call despite of
the detail in the implementation of the function “io_ring_ctx_alloc”
that it was determined already that a corresponding variable contained
a null pointer because of a failed memory allocation.

1. Thus use more appropriate labels instead.

2. Reorder jump targets at the end.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---

See also:
https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources


 io_uring/io_uring.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index c9a63c39cdd0..7727cdd505ae 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -295,12 +295,14 @@ static __cold struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
 	hash_bits = ilog2(p->cq_entries) - 5;
 	hash_bits = clamp(hash_bits, 1, 8);
 	if (io_alloc_hash_table(&ctx->cancel_table, hash_bits))
-		goto err;
+		goto destroy_io_bl_xa;
+
 	if (io_alloc_hash_table(&ctx->cancel_table_locked, hash_bits))
-		goto err;
+		goto free_cancel_table_hbs;
+
 	if (percpu_ref_init(&ctx->refs, io_ring_ctx_ref_free,
 			    0, GFP_KERNEL))
-		goto err;
+		goto free_cancel_table_locked_hbs;

 	ctx->flags = p->flags;
 	init_waitqueue_head(&ctx->sqo_sq_wait);
@@ -341,9 +343,12 @@ static __cold struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
 	INIT_WQ_LIST(&ctx->submit_state.compl_reqs);
 	INIT_HLIST_HEAD(&ctx->cancelable_uring_cmd);
 	return ctx;
-err:
-	kfree(ctx->cancel_table.hbs);
+
+free_cancel_table_locked_hbs:
 	kfree(ctx->cancel_table_locked.hbs);
+free_cancel_table_hbs:
+	kfree(ctx->cancel_table.hbs);
+destroy_io_bl_xa:
 	xa_destroy(&ctx->io_bl_xa);
 	kfree(ctx);
 	return NULL;
--
2.43.0

Re: [PATCH v2 2/2] io_uring: Improve exception handling in io_ring_ctx_alloc()

[Pavel Begunkov]

On 1/10/24 20:50, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 10 Jan 2024 21:15:48 +0100
> 
> The label “err” was used to jump to a kfree() call despite of
> the detail in the implementation of the function “io_ring_ctx_alloc”
> that it was determined already that a corresponding variable contained
> a null pointer because of a failed memory allocation.

It's _much_ simpler the way it currently is, compare it with maintaining
a bunch of labels. That is the advantage of being able to distinguish
un-allocated state like NULL, just kfree them and don't care about
jumping to a wrong one or keeping them in order.

  
> 1. Thus use more appropriate labels instead.
> 
> 2. Reorder jump targets at the end.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> 
> See also:
> https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources
> 
> 
>   io_uring/io_uring.c | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
> index c9a63c39cdd0..7727cdd505ae 100644
> --- a/io_uring/io_uring.c
> +++ b/io_uring/io_uring.c
> @@ -295,12 +295,14 @@ static __cold struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
>   	hash_bits = ilog2(p->cq_entries) - 5;
>   	hash_bits = clamp(hash_bits, 1, 8);
>   	if (io_alloc_hash_table(&ctx->cancel_table, hash_bits))
> -		goto err;
> +		goto destroy_io_bl_xa;
> +
>   	if (io_alloc_hash_table(&ctx->cancel_table_locked, hash_bits))
> -		goto err;
> +		goto free_cancel_table_hbs;
> +
>   	if (percpu_ref_init(&ctx->refs, io_ring_ctx_ref_free,
>   			    0, GFP_KERNEL))
> -		goto err;
> +		goto free_cancel_table_locked_hbs;
> 
>   	ctx->flags = p->flags;
>   	init_waitqueue_head(&ctx->sqo_sq_wait);
> @@ -341,9 +343,12 @@ static __cold struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
>   	INIT_WQ_LIST(&ctx->submit_state.compl_reqs);
>   	INIT_HLIST_HEAD(&ctx->cancelable_uring_cmd);
>   	return ctx;
> -err:
> -	kfree(ctx->cancel_table.hbs);
> +
> +free_cancel_table_locked_hbs:
>   	kfree(ctx->cancel_table_locked.hbs);
> +free_cancel_table_hbs:
> +	kfree(ctx->cancel_table.hbs);
> +destroy_io_bl_xa:
>   	xa_destroy(&ctx->io_bl_xa);
>   	kfree(ctx);
>   	return NULL;
> --
> 2.43.0
> 

-- 
Pavel Begunkov

Re: [PATCH v2 1/2] io_uring: Delete a redundant kfree() call in io_ring_ctx_alloc()

[Gabriel Krisman Bertazi]

Markus Elfring <Markus.Elfring@web.de> writes:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 10 Jan 2024 20:54:43 +0100
>
> Another useful pointer was not reassigned to the data structure member
> “io_bl” by this function implementation.
> Thus omit a redundant call of the function “kfree” at the end.

Perhaps rewrite this to:

ctx->io_bl is initialized later through IORING_OP_PROVIDE_BUFFERS or
IORING_REGISTER_PBUF_RING later on, so there is nothing to free in the
ctx allocation error path.

Other than that, and for this patch only:

Reviewed-by: Gabriel Krisman Bertazi <krisman@suse.de>

thanks,

>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>
> v2:
> A change request by Gabriel Krisman Bertazi was applied here.
>
>
>  io_uring/io_uring.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
> index 86761ec623f9..c9a63c39cdd0 100644
> --- a/io_uring/io_uring.c
> +++ b/io_uring/io_uring.c
> @@ -344,7 +344,6 @@ static __cold struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
>  err:
>  	kfree(ctx->cancel_table.hbs);
>  	kfree(ctx->cancel_table_locked.hbs);
> -	kfree(ctx->io_bl);
>  	xa_destroy(&ctx->io_bl_xa);
>  	kfree(ctx);
>  	return NULL;
> --
> 2.43.0
>

-- 
Gabriel Krisman Bertazi

Re: [PATCH v2 2/2] io_uring: Improve exception handling in io_ring_ctx_alloc()

[Gabriel Krisman Bertazi]

Markus Elfring <Markus.Elfring@web.de> writes:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 10 Jan 2024 21:15:48 +0100
>
> The label “err” was used to jump to a kfree() call despite of
> the detail in the implementation of the function “io_ring_ctx_alloc”
> that it was determined already that a corresponding variable contained
> a null pointer because of a failed memory allocation.
>
> 1. Thus use more appropriate labels instead.
>
> 2. Reorder jump targets at the end.
>

As I mentioned on v1, this doesn't do us any good, as kfree can handle
NULL pointers just fine, and changes like this becomes churn later when
backporting or modifying the code.

-- 
Gabriel Krisman Bertazi

Re: [v2 2/2] io_uring: Improve exception handling in io_ring_ctx_alloc()

[Markus Elfring]

>> The label “err” was used to jump to a kfree() call despite of
>> the detail in the implementation of the function “io_ring_ctx_alloc”
>> that it was determined already that a corresponding variable contained
>> a null pointer because of a failed memory allocation.
>>
>> 1. Thus use more appropriate labels instead.
>>
>> 2. Reorder jump targets at the end.
>>
>
> As I mentioned on v1, this doesn't do us any good,

I dare to present other development views.


> as kfree can handle NULL pointers just fine,

Yes, this is the case.

Would you dare to categorise such a special function calls as redundant?

May it be skipped in more cases?


> and changes like this becomes churn later when backporting or modifying the code.

There are usual opportunities to consider for further collateral evolution.

Regards,
Markus

Re: [PATCH v2 1/2] io_uring: Delete a redundant kfree() call in io_ring_ctx_alloc()

[Jens Axboe]

On 1/12/24 7:25 AM, Gabriel Krisman Bertazi wrote:
> Markus Elfring <Markus.Elfring@web.de> writes:
> 
>> From: Markus Elfring <elfring@users.sourceforge.net>
>> Date: Wed, 10 Jan 2024 20:54:43 +0100
>>
>> Another useful pointer was not reassigned to the data structure member
>> ?io_bl? by this function implementation.
>> Thus omit a redundant call of the function ?kfree? at the end.

This is just nonsense...

On top of that, this patch is pointless, and the 2nd patch is even worse
in that it just makes a mess of cleanup. And for what reasoning?
Absolutely none.

There's a reason why I filter emails from this particular author
straight to the trash, there's a long history of this kind of thing and
not understanding feedback.

-- 
Jens Axboe

Re: [PATCH v2 1/2] io_uring: Delete a redundant kfree() call in io_ring_ctx_alloc()

[Gabriel Krisman Bertazi]

Jens Axboe <axboe@kernel.dk> writes:

> On 1/12/24 7:25 AM, Gabriel Krisman Bertazi wrote:
>> Markus Elfring <Markus.Elfring@web.de> writes:
>> 
>>> From: Markus Elfring <elfring@users.sourceforge.net>
>>> Date: Wed, 10 Jan 2024 20:54:43 +0100
>>>
>>> Another useful pointer was not reassigned to the data structure member
>>> ?io_bl? by this function implementation.
>>> Thus omit a redundant call of the function ?kfree? at the end.
>
> This is just nonsense...
>
> On top of that, this patch is pointless, and the 2nd patch is even worse
> in that it just makes a mess of cleanup. And for what reasoning?
> Absolutely none.

Ah, The description is non-sense, but the change in this patch seemed
correct to me, even if pointless, which is why I reviewed it.  patch 2
is just garbage.

> There's a reason why I filter emails from this particular author
> straight to the trash, there's a long history of this kind of thing and
> not understanding feedback.

Clearly there is background with this author that I wasn't aware, and
just based on his responses, I can see your point. So I apologize for
giving him space to continue the spamming.  My bad.

-- 
Gabriel Krisman Bertazi

Re: [v2 1/2] io_uring: Delete a redundant kfree() call in io_ring_ctx_alloc()

[Markus Elfring]

> patch 2 is just garbage.

Such a view can eventually be reconsidered if the support would ever grow
also for the application of additional labels.
https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources


> Clearly there is background with this author that I wasn't aware, and
> just based on his responses, I can see your point. So I apologize for
> giving him space to continue the spamming.

The change reluctance can be adapted according to the spectrum of
presented source code adjustment possibilities, can't it?

Regards,
Markus