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

Commit Message

Markus Elfring Jan. 10, 2024, 8:50 p.m. UTC

From: Markus Elfring <[e-mail]>
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 <[e-mail]>
---

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(-)

--
2.43.0

Comments

Pavel Begunkov Jan. 11, 2024, 1:23 p.m. UTC | #1

On 1/10/24 20:50, Markus Elfring wrote:
> From: Markus Elfring <[e-mail]>
> 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 <[e-mail]>
> ---
> 
> 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
>

Gabriel Krisman Bertazi Jan. 12, 2024, 2:30 p.m. UTC | #2

Markus Elfring <[e-mail]> writes:

> From: Markus Elfring <[e-mail]>
> 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.

Markus Elfring Jan. 12, 2024, 3 p.m. UTC | #3

>> 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

Patch

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;