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