[cocci] Reconsidering kfree() calls for null pointers (with SmPL)

[cocci] Reconsidering kfree() calls for null pointers (with SmPL)

[Markus Elfring]

Hello,

I tried the following SmPL script out also on the source files from
the software “Linux next-20230324”.

@display@
expression call, storage;
identifier target;
@@
*storage = call(...);
*if (!storage)
    goto target;
 ...
*target:
*kfree(storage)


119 source files were found where a check was performed for a failed function call
and a kfree() call is immediately performed for the same variable.
Thus a null pointer is probably passed at these source code places.
I imagine that such code should be reconsidered once more and improved accordingly.

How do you think about to achieve any adjustments in this design direction?

Regards,
Markus

Re: [cocci] Reconsidering kfree() calls for null pointers (with SmPL)

[Vlastimil Babka]

On 3/28/23 17:55, Markus Elfring wrote:
> Hello,
> 
> I tried the following SmPL script out also on the source files from
> the software “Linux next-20230324”.
> 
> @display@
> expression call, storage;
> identifier target;
> @@
> *storage = call(...);
> *if (!storage)
>     goto target;
>  ...
> *target:
> *kfree(storage)
> 
> 
> 119 source files were found where a check was performed for a failed function call
> and a kfree() call is immediately performed for the same variable.
> Thus a null pointer is probably passed at these source code places.

It is valid to pass a NULL pointer to kfree(), it checks for that.

> I imagine that such code should be reconsidered once more and improved accordingly.

The only potential downside in the scenario is checking storage == NULL
twice. But this is an error handling path, not a fast path. On the other
hand, the code may look like this:

storage = call(...);
if (!storage)
    goto target;

if (some_other_condition)
    goto target;

target:
kfree(storage)


For some_other_condition, we want the kfree(). If you changed the code, to
remove the extra NULL pointer check, you would have:

storage = call(...);
if (!storage)
    goto target1;

if (some_other_condition)
    goto target2;

target2:
kfree(storage)
target1:

The extra goto label and larger code is not worth saving the NULL pointer
check, because this is error handling path.

> How do you think about to achieve any adjustments in this design direction?

Only in cases where it would not make the code more complex, i.e. if there
are no "some_other_condition" that jumps to the same target after allocation
to storage is successful.

Vlastimil

> Regards,
> Markus

Re: [cocci] Reconsidering kfree() calls for null pointers (with SmPL)

[Markus Elfring]

> It is valid to pass a NULL pointer to kfree(), it checks for that.
>
>> I imagine that such code should be reconsidered once more and improved accordingly.
>
> The only potential downside in the scenario is checking storage == NULL twice.

Will programming concerns evolve any further?


> But this is an error handling path, not a fast path.

I hope that also exception handling code can become efficient in more use cases.


> On the other hand, the code may look like this:
>
> storage = call(...);
> if (!storage)
>     goto target;
>
> if (some_other_condition)
>     goto target;
>
> target:
> kfree(storage)
>
>
> For some_other_condition, we want the kfree().

Allocated resources should be properly released.


> If you changed the code, to remove the extra NULL pointer check, you would have:
>
> storage = call(...);
> if (!storage)
>     goto target1;
>
> if (some_other_condition)
>     goto target2;
>
> target2:
> kfree(storage)

A label like “free_this_resource” can be nicer, can't it?


> target1:
>
> The extra goto label and larger code is not worth saving the NULL pointer check,
> because this is error handling path.

Some code reviewers and programmers represent other development views.


>> How do you think about to achieve any adjustments in this design direction?
>
> Only in cases where it would not make the code more complex, i.e. if there
> are no "some_other_condition" that jumps to the same target after allocation
> to storage is successful.

Do you find any implementation details worth for another look also according to
a special information source?
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#MEM12C.Considerusingagotochainwhenleavingafunctiononerrorwhenusingandreleasingresources-CompliantSolution%28POSIX,GotoChain%29


Will circumstances evolve in ways so that you would adjust a desire to stick
to only a single jump label?

Regards,
Markus

Re: [cocci] Reconsidering kfree() calls for null pointers (with SmPL)

[Vlastimil Babka]

On 3/29/23 08:18, Markus Elfring wrote:
>> It is valid to pass a NULL pointer to kfree(), it checks for that.
>>
>>> I imagine that such code should be reconsidered once more and improved accordingly.
>>
>> The only potential downside in the scenario is checking storage == NULL twice.
> 
> Will programming concerns evolve any further?

It's very unlikely that the behavior of kfree(NULL) will change.

>> But this is an error handling path, not a fast path.
> 
> I hope that also exception handling code can become efficient in more use cases.

Nobody will notice the efficiency, and it's not the only goal, code
readability is also important.

>> On the other hand, the code may look like this:
>>
>> storage = call(...);
>> if (!storage)
>>     goto target;
>>
>> if (some_other_condition)
>>     goto target;
>>
>> target:
>> kfree(storage)
>>
>>
>> For some_other_condition, we want the kfree().
> 
> Allocated resources should be properly released.

Sure. They are properly released in the above example.

>> If you changed the code, to remove the extra NULL pointer check, you would have:
>>
>> storage = call(...);
>> if (!storage)
>>     goto target1;
>>
>> if (some_other_condition)
>>     goto target2;
>>
>> target2:
>> kfree(storage)
> 
> A label like “free_this_resource” can be nicer, can't it?

Sure, name was not the point. Just that there are now 2 labels.

>> target1:
>>
>> The extra goto label and larger code is not worth saving the NULL pointer check,
>> because this is error handling path.
> 
> Some code reviewers and programmers represent other development views.

That's the universal truth :)

> 
>>> How do you think about to achieve any adjustments in this design direction?
>>
>> Only in cases where it would not make the code more complex, i.e. if there
>> are no "some_other_condition" that jumps to the same target after allocation
>> to storage is successful.
> 
> Do you find any implementation details worth for another look also according to
> a special information source?
> 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#MEM12C.Considerusingagotochainwhenleavingafunctiononerrorwhenusingandreleasingresources-CompliantSolution%28POSIX,GotoChain%29
> 
> 
> Will circumstances evolve in ways so that you would adjust a desire to stick
> to only a single jump label?

Depends on specific code cleanup. But generally, not if the change is only
to add an extra label to skip over kfree() - I will unlikely consider such
patches as useful.

> Regards,
> Markus

[cocci] [PATCH 0/2] ARM: Adjustments for init_atags_procfs()

[Markus Elfring]

Date: Tue, 28 Mar 2023 19:06:32 +0200

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

Markus Elfring (2):
  Delete an error message for a failed memory allocation
  Return directly after a failed kmalloc()

 arch/arm/kernel/atags_proc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

--
2.40.0

[cocci] [PATCH 1/2] ARM: Delete an error message for a failed memory allocation in init_atags_procfs()

[Markus Elfring]

Date: Tue, 28 Mar 2023 18:40:26 +0200

Omit an extra message for a memory allocation failure in this function.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/arm/kernel/atags_proc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm/kernel/atags_proc.c b/arch/arm/kernel/atags_proc.c
index 3ec2afe78423..2f08ab08e68f 100644
--- a/arch/arm/kernel/atags_proc.c
+++ b/arch/arm/kernel/atags_proc.c
@@ -69,8 +69,6 @@ static int __init init_atags_procfs(void)

 nomem:
 	kfree(b);
-	pr_err("Exporting ATAGs: not enough memory\n");
-
 	return -ENOMEM;
 }
 arch_initcall(init_atags_procfs);
--
2.40.0

[cocci] [PATCH 2/2] ARM: Return directly after a failed kmalloc() in init_atags_procfs()

[Markus Elfring]

Date: Tue, 28 Mar 2023 18:56:31 +0200

Return directly after a call of the function “kmalloc” failed
at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/arm/kernel/atags_proc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/kernel/atags_proc.c b/arch/arm/kernel/atags_proc.c
index 2f08ab08e68f..1c7441f3af85 100644
--- a/arch/arm/kernel/atags_proc.c
+++ b/arch/arm/kernel/atags_proc.c
@@ -56,7 +56,7 @@ static int __init init_atags_procfs(void)

 	b = kmalloc(sizeof(*b) + size, GFP_KERNEL);
 	if (!b)
-		goto nomem;
+		return -ENOMEM;

 	b->size = size;
 	memcpy(b->data, atags_copy, size);
--
2.40.0

Re: [cocci] [PATCH 0/2] ARM: Adjustments for init_atags_procfs()

[Markus Elfring]

> Date: Tue, 28 Mar 2023 19:06:32 +0200
>
> A few update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (2):
>   Delete an error message for a failed memory allocation
>   Return directly after a failed kmalloc()
>
>  arch/arm/kernel/atags_proc.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

Is this patch series still in review queues?

See also:
https://lore.kernel.org/cocci/2258ce64-2a14-6778-8319-b342b06a1f33@web.de/
https://sympa.inria.fr/sympa/arc/cocci/2023-04/msg00034.html

Regards,
Markus

Re: [cocci] [0/2] ARM: Adjustments for init_atags_procfs()

[Markus Elfring]

> Is this patch series still in review queues?

See also:
https://lore.kernel.org/cocci/562a6f99-3f8e-9a77-e519-b668e24dced2@web.de/
https://sympa.inria.fr/sympa/arc/cocci/2023-03/msg00098.html

Regards,
Markus

Re: [cocci] [0/2] ARM: Adjustments for init_atags_procfs()

[Markus Elfring]

>>> Is this patch series still in review queues?
>>
>> See also:
>> https://lore.kernel.org/cocci/562a6f99-3f8e-9a77-e519-b668e24dced2@web.de/
>> https://sympa.inria.fr/sympa/arc/cocci/2023-03/msg00098.html
>
> I suspect no one looked at it, sorry.

Special mailing list settings probably influenced this situation.


>                                       I don't catch everything that is
> on the mailing list. Looks fine to me but it needs to end up in the
> patch system to be applied.

Can you collaborate also with mentioned mailing list archive interfaces?

Regards,
Markus

Re: [cocci] [0/2] ARM: Adjustments for init_atags_procfs()

[Markus Elfring]

>>>>> Is this patch series still in review queues?
>>>>
>>>> See also:
>>>> https://lore.kernel.org/cocci/562a6f99-3f8e-9a77-e519-b668e24dced2@web.de/
>>>> https://sympa.inria.fr/sympa/arc/cocci/2023-03/msg00098.html
>>>
>>> I suspect no one looked at it, sorry.
>>
>> Special mailing list settings probably influenced this situation.

Did any communication filters hinder the clarification of further development ideas?


>>>                                       I don't catch everything that is
>>> on the mailing list. Looks fine to me but it needs to end up in the
>>> patch system to be applied.
>>
>> Can you collaborate also with mentioned mailing list archive interfaces?
>
> Err what? Sorry, I don't understand your comment.

Are you going to pick any patches up from linked information sources?

Regards,
Markus

Re: [cocci] [0/2] ARM: Adjustments for init_atags_procfs()

[Markus Elfring]

>> Are you going to pick any patches up from linked information sources?
>
> No. Did you read my first reply which told you how patches get applied
> to arch/arm? If you didn't, your patch won't get applied.

Do you request a resend of affected patches here?

Regards,
Markus

Re: [cocci] [0/2] ARM: Adjustments for init_atags_procfs()

[Markus Elfring]

> I am just generally following along here, but to give some context it
> seems like Markus is banned from posting to various kernel mailing
> lists[0][1][2].

This was the case for a while.
Were such communication filters reconsidered anyhow recently?

Regards,
Markus

[cocci] [PATCH 0/2] perf/x86/intel/pt: Adjustments for pt_pmu_hw_init()

[Markus Elfring]

Date: Tue, 28 Mar 2023 20:34:32 +0200

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

Markus Elfring (2):
  Use -ENOMEM directly for a return statement
  Return directly after a failed kzalloc()

 arch/x86/events/intel/pt.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

--
2.40.0

[cocci] [PATCH 1/2] perf/x86/intel/pt: Use -ENOMEM directly for a return statement in pt_pmu_hw_init()

[Markus Elfring]

Date: Tue, 28 Mar 2023 20:22:05 +0200

* Return the value “-ENOMEM” directly without an extra
  assignment statement.

* Omit the variable “ret”.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/x86/events/intel/pt.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
index 42a55794004a..93af27edded3 100644
--- a/arch/x86/events/intel/pt.c
+++ b/arch/x86/events/intel/pt.c
@@ -190,7 +190,6 @@ static int __init pt_pmu_hw_init(void)
 	struct attribute **attrs;
 	size_t size;
 	u64 reg;
-	int ret;
 	long i;

 	rdmsrl(MSR_PLATFORM_INFO, reg);
@@ -242,7 +241,6 @@ static int __init pt_pmu_hw_init(void)
 			    &pt_pmu.caps[CPUID_EDX + i*PT_CPUID_REGS_NUM]);
 	}

-	ret = -ENOMEM;
 	size = sizeof(struct attribute *) * (ARRAY_SIZE(pt_caps)+1);
 	attrs = kzalloc(size, GFP_KERNEL);
 	if (!attrs)
@@ -273,8 +271,7 @@ static int __init pt_pmu_hw_init(void)

 fail:
 	kfree(attrs);
-
-	return ret;
+	return -ENOMEM;
 }

 #define RTIT_CTL_CYC_PSB (RTIT_CTL_CYCLEACC	| \
--
2.40.0

[cocci] [PATCH 2/2] perf/x86/intel/pt: Return directly after a failed kzalloc() in pt_pmu_hw_init()

[Markus Elfring]

Date: Tue, 28 Mar 2023 20:24:16 +0200

Return directly after a call of the function “kzalloc” failed
at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/x86/events/intel/pt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
index 93af27edded3..18c009bdb827 100644
--- a/arch/x86/events/intel/pt.c
+++ b/arch/x86/events/intel/pt.c
@@ -244,7 +244,7 @@ static int __init pt_pmu_hw_init(void)
 	size = sizeof(struct attribute *) * (ARRAY_SIZE(pt_caps)+1);
 	attrs = kzalloc(size, GFP_KERNEL);
 	if (!attrs)
-		goto fail;
+		return -ENOMEM;

 	size = sizeof(struct dev_ext_attribute) * (ARRAY_SIZE(pt_caps)+1);
 	de_attrs = kzalloc(size, GFP_KERNEL);
--
2.40.0

Re: [cocci] [PATCH 0/2] perf/x86/intel/pt: Adjustments for pt_pmu_hw_init()

[Markus Elfring]

> Date: Tue, 28 Mar 2023 20:34:32 +0200
>
> A few update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (2):
>   Use -ENOMEM directly for a return statement
>   Return directly after a failed kzalloc()
>
>  arch/x86/events/intel/pt.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)

Is this patch series still in review queues?

See also:
https://lore.kernel.org/cocci/d9c673f9-2c32-282f-f261-b4d5762409bb@web.de/
https://sympa.inria.fr/sympa/arc/cocci/2023-03/msg00101.html

Regards,
Markus

[cocci] [PATCH] apparmor: Return directly after a failed kzalloc() in two functions

[Markus Elfring]

Date: Wed, 29 Mar 2023 11:50:44 +0200

1. Return directly after a call of the function “kzalloc” failed
   at the beginning in these function implementations.

2. Omit extra initialisations (for a few local variables)
   which became unnecessary with this refactoring.


This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 security/apparmor/crypto.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/security/apparmor/crypto.c b/security/apparmor/crypto.c
index b498ed302461..6724e2ff6da8 100644
--- a/security/apparmor/crypto.c
+++ b/security/apparmor/crypto.c
@@ -28,15 +28,15 @@ unsigned int aa_hash_size(void)
 char *aa_calc_hash(void *data, size_t len)
 {
 	SHASH_DESC_ON_STACK(desc, apparmor_tfm);
-	char *hash = NULL;
-	int error = -ENOMEM;
+	char *hash;
+	int error;

 	if (!apparmor_tfm)
 		return NULL;

 	hash = kzalloc(apparmor_hash_size, GFP_KERNEL);
 	if (!hash)
-		goto fail;
+		return ERR_PTR(-ENOMEM);

 	desc->tfm = apparmor_tfm;

@@ -62,7 +62,7 @@ int aa_calc_profile_hash(struct aa_profile *profile, u32 version, void *start,
 			 size_t len)
 {
 	SHASH_DESC_ON_STACK(desc, apparmor_tfm);
-	int error = -ENOMEM;
+	int error;
 	__le32 le32_version = cpu_to_le32(version);

 	if (!aa_g_hash_policy)
@@ -73,7 +73,7 @@ int aa_calc_profile_hash(struct aa_profile *profile, u32 version, void *start,

 	profile->hash = kzalloc(apparmor_hash_size, GFP_KERNEL);
 	if (!profile->hash)
-		goto fail;
+		return -ENOMEM;

 	desc->tfm = apparmor_tfm;

--
2.40.0

[cocci] [PATCH 0/3] lru_cache: Adjustments for lc_create()

[Markus Elfring]

Date: Wed, 29 Mar 2023 15:30:23 +0200

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

Markus Elfring (3):
  Return directly after a failed kzalloc()
  Improve two size determinations
  Improve exception handling

 lib/lru_cache.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

--
2.40.0

[cocci] [PATCH 1/3] lru_cache: Return directly after a failed kzalloc() in lc_create()

[Markus Elfring]

Date: Wed, 29 Mar 2023 14:45:34 +0200

1. Return directly after a call of the function “kzalloc” failed
   at the beginning in these function implementations.

2. Omit extra initialisations (for the variables “slot” and “element”)
   which became unnecessary with this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 lib/lru_cache.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lib/lru_cache.c b/lib/lru_cache.c
index b3d9187611de..e0db27b3a2d7 100644
--- a/lib/lru_cache.c
+++ b/lib/lru_cache.c
@@ -78,8 +78,8 @@ struct lru_cache *lc_create(const char *name, struct kmem_cache *cache,
 		unsigned max_pending_changes,
 		unsigned e_count, size_t e_size, size_t e_off)
 {
-	struct hlist_head *slot = NULL;
-	struct lc_element **element = NULL;
+	struct hlist_head *slot;
+	struct lc_element **element;
 	struct lru_cache *lc;
 	struct lc_element *e;
 	unsigned cache_obj_size = kmem_cache_size(cache);
@@ -96,7 +96,8 @@ struct lru_cache *lc_create(const char *name, struct kmem_cache *cache,

 	slot = kcalloc(e_count, sizeof(struct hlist_head), GFP_KERNEL);
 	if (!slot)
-		goto out_fail;
+		return NULL;
+
 	element = kcalloc(e_count, sizeof(struct lc_element *), GFP_KERNEL);
 	if (!element)
 		goto out_fail;
--
2.40.0

[cocci] [PATCH 2/3] lru_cache: Improve two size determinations in lc_create()

[Markus Elfring]

Date: Wed, 29 Mar 2023 15:00:13 +0200

Replace the specification of data structures by pointer dereferences
as the parameter for the operator “sizeof” to make the corresponding size
determination a bit safer according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 lib/lru_cache.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/lru_cache.c b/lib/lru_cache.c
index e0db27b3a2d7..31820f03b146 100644
--- a/lib/lru_cache.c
+++ b/lib/lru_cache.c
@@ -94,11 +94,11 @@ struct lru_cache *lc_create(const char *name, struct kmem_cache *cache,
 	if (e_count > LC_MAX_ACTIVE)
 		return NULL;

-	slot = kcalloc(e_count, sizeof(struct hlist_head), GFP_KERNEL);
+	slot = kcalloc(e_count, sizeof(*slot), GFP_KERNEL);
 	if (!slot)
 		return NULL;

-	element = kcalloc(e_count, sizeof(struct lc_element *), GFP_KERNEL);
+	element = kcalloc(e_count, sizeof(*element), GFP_KERNEL);
 	if (!element)
 		goto out_fail;

--
2.40.0

[cocci] [PATCH 3/3] lru_cache: Improve exception handling in lc_create()

[Markus Elfring]

Date: Wed, 29 Mar 2023 15:20:39 +0200

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

Thus use more appropriate labels instead.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 lib/lru_cache.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lib/lru_cache.c b/lib/lru_cache.c
index 31820f03b146..fdc8bd6fc888 100644
--- a/lib/lru_cache.c
+++ b/lib/lru_cache.c
@@ -100,11 +100,11 @@ struct lru_cache *lc_create(const char *name, struct kmem_cache *cache,

 	element = kcalloc(e_count, sizeof(*element), GFP_KERNEL);
 	if (!element)
-		goto out_fail;
+		goto free_slot;

 	lc = kzalloc(sizeof(*lc), GFP_KERNEL);
 	if (!lc)
-		goto out_fail;
+		goto free_element;

 	INIT_LIST_HEAD(&lc->in_use);
 	INIT_LIST_HEAD(&lc->lru);
@@ -142,8 +142,9 @@ struct lru_cache *lc_create(const char *name, struct kmem_cache *cache,
 		kmem_cache_free(cache, p - e_off);
 	}
 	kfree(lc);
-out_fail:
+free_element:
 	kfree(element);
+free_slot:
 	kfree(slot);
 	return NULL;
 }
--
2.40.0

Re: [cocci] [PATCH 0/3] lru_cache: Adjustments for lc_create()

[Markus Elfring]

> Date: Wed, 29 Mar 2023 15:30:23 +0200
>
> Some update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (3):
>   Return directly after a failed kzalloc()
>   Improve two size determinations
>   Improve exception handling
>
>  lib/lru_cache.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)


Is this patch series still in review queues?

See also:
https://lore.kernel.org/cocci/33226beb-4fe2-3da5-5d69-a33e683dec57@web.de/
https://sympa.inria.fr/sympa/arc/cocci/2023-03/msg00110.html

Regards,
Markus

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

[Markus Elfring]

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.

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


This issue was detected by using the Coccinelle software.

Fixes: 206aefde4f886fdeb3b6339aacab3a85fb74cb7e ("io_uring: reduce/pack size of io_ring_ctx")
Fixes: 78076bb64aa8ba5b7207c38b2660a9e10ffa8cc7 ("io_uring: use hash table for poll command lookups")
Fixes: 9cfc7e94e42be9c73072606b84d4574a0a2ec270 ("io_uring: get rid of hashed provided buffer groups")
Fixes: 9ca9fb24d5febccea354089c41f96a8ad0d853f8 ("io_uring: mutex locked poll hashing")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 io_uring/io_uring.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 536940675c67..fceb74d7d851 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -291,19 +291,19 @@ 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;

 	ctx->dummy_ubuf = kzalloc(sizeof(*ctx->dummy_ubuf), GFP_KERNEL);
 	if (!ctx->dummy_ubuf)
-		goto err;
+		goto free_cancel_table_locked_hbs;
 	/* set invalid range, so io_import_fixed() fails meeting it */
 	ctx->dummy_ubuf->ubuf = -1UL;

 	if (percpu_ref_init(&ctx->refs, io_ring_ctx_ref_free,
 			    0, GFP_KERNEL))
-		goto err;
+		goto free_ubuf;

 	ctx->flags = p->flags;
 	init_waitqueue_head(&ctx->sqo_sq_wait);
@@ -337,11 +337,14 @@ static __cold struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
 	INIT_DELAYED_WORK(&ctx->fallback_work, io_fallback_req_func);
 	INIT_WQ_LIST(&ctx->submit_state.compl_reqs);
 	return ctx;
-err:
+
+free_ubuf:
 	kfree(ctx->dummy_ubuf);
-	kfree(ctx->cancel_table.hbs);
+free_cancel_table_locked_hbs:
 	kfree(ctx->cancel_table_locked.hbs);
-	kfree(ctx->io_bl);
+free_cancel_table_hbs:
+	kfree(ctx->cancel_table.hbs);
+destroy_io_bl_xa:
 	xa_destroy(&ctx->io_bl_xa);
 	kfree(ctx);
 	return NULL;
--
2.40.0

Re: [cocci] 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

[cocci] [PATCH 0/4] overlayfs: Adjustments for ovl_fill_super()

[Markus Elfring]

Date: Thu, 30 Mar 2023 10:38:23 +0200

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

Markus Elfring (4):
  Return directly for two checks
  Improve two size determinations
  Improve exception handling
  Move some assignments for the variable “err”

 fs/overlayfs/super.c | 72 ++++++++++++++++++++++++--------------------
 1 file changed, 39 insertions(+), 33 deletions(-)

--
2.40.0

[cocci] [PATCH 1/4] overlayfs: Return directly for two checks in ovl_fill_super()

[Markus Elfring]

Date: Thu, 30 Mar 2023 08:48:34 +0200

1. Return directly after a call of the function “kzalloc” failed
   or the “user_ns” was different in a data structure
   at the beginning.

2. Delete the label “out” which became unnecessary with this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 fs/overlayfs/super.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index f97ad8b40dbb..c0d33ab331ec 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1894,16 +1894,14 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	unsigned int numlower;
 	int err;

-	err = -EIO;
 	if (WARN_ON(sb->s_user_ns != current_user_ns()))
-		goto out;
+		return -EIO;

 	sb->s_d_op = &ovl_dentry_operations;

-	err = -ENOMEM;
 	ofs = kzalloc(sizeof(struct ovl_fs), GFP_KERNEL);
 	if (!ofs)
-		goto out;
+		return -ENOMEM;

 	err = -ENOMEM;
 	ofs->creator_cred = cred = prepare_creds();
@@ -2073,7 +2071,6 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	kfree(splitlower);
 	path_put(&upperpath);
 	ovl_free_fs(ofs);
-out:
 	return err;
 }

--
2.40.0

[cocci] [PATCH 2/4] overlayfs: Improve two size determinations in ovl_fill_super()

[Markus Elfring]

Date: Thu, 30 Mar 2023 09:02:10 +0200

Replace the specification of data structures by pointer dereferences
as the parameter for the operator “sizeof” to make the corresponding size
determination a bit safer according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 fs/overlayfs/super.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index c0d33ab331ec..c721d6fe9cb4 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1899,7 +1899,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)

 	sb->s_d_op = &ovl_dentry_operations;

-	ofs = kzalloc(sizeof(struct ovl_fs), GFP_KERNEL);
+	ofs = kzalloc(sizeof(*ofs), GFP_KERNEL);
 	if (!ofs)
 		return -ENOMEM;

@@ -1941,7 +1941,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	}

 	err = -ENOMEM;
-	layers = kcalloc(numlower + 1, sizeof(struct ovl_layer), GFP_KERNEL);
+	layers = kcalloc(numlower + 1, sizeof(*layers), GFP_KERNEL);
 	if (!layers)
 		goto out_err;

--
2.40.0

[cocci] [PATCH 3/4] overlayfs: Improve exception handling in ovl_fill_super()

[Markus Elfring]

Date: Thu, 30 Mar 2023 09:51:02 +0200

The label “out_err” was used to jump to a kfree() call despite of
the detail in the implementation of the function “ovl_fill_super”
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. Omit an extra initialisation (for the variable “splitlower”)
   which became unnecessary with this refactoring.


This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 fs/overlayfs/super.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index c721d6fe9cb4..df5628056d93 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1890,7 +1890,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	struct ovl_fs *ofs;
 	struct ovl_layer *layers;
 	struct cred *cred;
-	char *splitlower = NULL;
+	char *splitlower;
 	unsigned int numlower;
 	int err;

@@ -1906,7 +1906,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	err = -ENOMEM;
 	ofs->creator_cred = cred = prepare_creds();
 	if (!cred)
-		goto out_err;
+		goto out_free_ofs;

 	/* Is there a reason anyone would want not to share whiteouts? */
 	ofs->share_whiteout = true;
@@ -1918,32 +1918,32 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	ofs->config.metacopy = ovl_metacopy_def;
 	err = ovl_parse_opt((char *) data, &ofs->config);
 	if (err)
-		goto out_err;
+		goto out_free_ofs;

 	err = -EINVAL;
 	if (!ofs->config.lowerdir) {
 		if (!silent)
 			pr_err("missing 'lowerdir'\n");
-		goto out_err;
+		goto out_free_ofs;
 	}

 	err = -ENOMEM;
 	splitlower = kstrdup(ofs->config.lowerdir, GFP_KERNEL);
 	if (!splitlower)
-		goto out_err;
+		goto out_free_ofs;

 	err = -EINVAL;
 	numlower = ovl_split_lowerdirs(splitlower);
 	if (numlower > OVL_MAX_STACK) {
 		pr_err("too many lower directories, limit is %d\n",
 		       OVL_MAX_STACK);
-		goto out_err;
+		goto out_free_splitlower;
 	}

 	err = -ENOMEM;
 	layers = kcalloc(numlower + 1, sizeof(*layers), GFP_KERNEL);
 	if (!layers)
-		goto out_err;
+		goto out_free_splitlower;

 	ofs->layers = layers;
 	/* Layer 0 is reserved for upper even if there's no upper */
@@ -1970,12 +1970,12 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 		err = -EINVAL;
 		if (!ofs->config.workdir) {
 			pr_err("missing 'workdir'\n");
-			goto out_err;
+			goto out_free_splitlower;
 		}

 		err = ovl_get_upper(sb, ofs, &layers[0], &upperpath);
 		if (err)
-			goto out_err;
+			goto out_free_splitlower;

 		upper_sb = ovl_upper_mnt(ofs)->mnt_sb;
 		if (!ovl_should_sync(ofs)) {
@@ -1983,13 +1983,13 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 			if (errseq_check(&upper_sb->s_wb_err, ofs->errseq)) {
 				err = -EIO;
 				pr_err("Cannot mount volatile when upperdir has an unseen error. Sync upperdir fs to clear state.\n");
-				goto out_err;
+				goto out_free_splitlower;
 			}
 		}

 		err = ovl_get_workdir(sb, ofs, &upperpath);
 		if (err)
-			goto out_err;
+			goto out_free_splitlower;

 		if (!ofs->workdir)
 			sb->s_flags |= SB_RDONLY;
@@ -2000,7 +2000,7 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	oe = ovl_get_lowerstack(sb, splitlower, numlower, ofs, layers);
 	err = PTR_ERR(oe);
 	if (IS_ERR(oe))
-		goto out_err;
+		goto out_free_splitlower;

 	/* If the upper fs is nonexistent, we mark overlayfs r/o too */
 	if (!ovl_upper_mnt(ofs))
@@ -2067,9 +2067,11 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 out_free_oe:
 	ovl_entry_stack_free(oe);
 	kfree(oe);
-out_err:
+out_free_splitlower:
 	kfree(splitlower);
+out_put_path:
 	path_put(&upperpath);
+out_free_ofs:
 	ovl_free_fs(ofs);
 	return err;
 }
--
2.40.0

[cocci] [PATCH 4/4] overlayfs: Move some assignments for the variable “err” in ovl_fill_super()

[Markus Elfring]

Date: Thu, 30 Mar 2023 10:25:22 +0200

One local variable was set to an error code in some cases before
a concrete error situation was detected. Thus move the corresponding
assignments into if branches to indicate a software failure there.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 fs/overlayfs/super.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index df5628056d93..cdef9934faf6 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1903,10 +1903,11 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	if (!ofs)
 		return -ENOMEM;

-	err = -ENOMEM;
 	ofs->creator_cred = cred = prepare_creds();
-	if (!cred)
+	if (!cred) {
+		err = -ENOMEM;
 		goto out_free_ofs;
+	}

 	/* Is there a reason anyone would want not to share whiteouts? */
 	ofs->share_whiteout = true;
@@ -1920,30 +1921,34 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	if (err)
 		goto out_free_ofs;

-	err = -EINVAL;
 	if (!ofs->config.lowerdir) {
 		if (!silent)
 			pr_err("missing 'lowerdir'\n");
+
+		err = -EINVAL;
 		goto out_free_ofs;
 	}

-	err = -ENOMEM;
 	splitlower = kstrdup(ofs->config.lowerdir, GFP_KERNEL);
-	if (!splitlower)
+	if (!splitlower) {
+		err = -ENOMEM;
 		goto out_free_ofs;
+	}

-	err = -EINVAL;
 	numlower = ovl_split_lowerdirs(splitlower);
 	if (numlower > OVL_MAX_STACK) {
 		pr_err("too many lower directories, limit is %d\n",
 		       OVL_MAX_STACK);
+
+		err = -EINVAL;
 		goto out_free_splitlower;
 	}

-	err = -ENOMEM;
 	layers = kcalloc(numlower + 1, sizeof(*layers), GFP_KERNEL);
-	if (!layers)
+	if (!layers) {
+		err = -ENOMEM;
 		goto out_free_splitlower;
+	}

 	ofs->layers = layers;
 	/* Layer 0 is reserved for upper even if there's no upper */
@@ -1967,9 +1972,9 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	if (ofs->config.upperdir) {
 		struct super_block *upper_sb;

-		err = -EINVAL;
 		if (!ofs->config.workdir) {
 			pr_err("missing 'workdir'\n");
+			err = -EINVAL;
 			goto out_free_splitlower;
 		}

@@ -1998,9 +2003,10 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 		sb->s_time_gran = upper_sb->s_time_gran;
 	}
 	oe = ovl_get_lowerstack(sb, splitlower, numlower, ofs, layers);
-	err = PTR_ERR(oe);
-	if (IS_ERR(oe))
+	if (IS_ERR(oe)) {
+		err = PTR_ERR(oe);
 		goto out_free_splitlower;
+	}

 	/* If the upper fs is nonexistent, we mark overlayfs r/o too */
 	if (!ovl_upper_mnt(ofs))
@@ -2052,10 +2058,11 @@ static int ovl_fill_super(struct super_block *sb, void *data, int silent)
 	sb->s_flags |= SB_POSIXACL;
 	sb->s_iflags |= SB_I_SKIP_SYNC;

-	err = -ENOMEM;
 	root_dentry = ovl_get_root(sb, upperpath.dentry, oe);
-	if (!root_dentry)
+	if (!root_dentry) {
+		err = -ENOMEM;
 		goto out_free_oe;
+	}

 	mntput(upperpath.mnt);
 	kfree(splitlower);
--
2.40.0

Re: [cocci] [PATCH 0/4] overlayfs: Adjustments for ovl_fill_super()

[Markus Elfring]

> Date: Thu, 30 Mar 2023 10:38:23 +0200
>
> Some update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (4):
>   Return directly for two checks
>   Improve two size determinations
>   Improve exception handling
>   Move some assignments for the variable “err”
>
>  fs/overlayfs/super.c | 72 ++++++++++++++++++++++++--------------------
>  1 file changed, 39 insertions(+), 33 deletions(-)

Is this patch series still in review queues?

See also:
https://lore.kernel.org/cocci/87b65f8e-abde-2aff-4da8-df6e0b464677@web.de/
https://sympa.inria.fr/sympa/arc/cocci/2023-03/msg00115.html

Regards,
Markus

Re: [cocci] [0/4] overlayfs: Adjustments for ovl_fill_super()

[Markus Elfring]

>> See also:
>> https://lore.kernel.org/cocci/87b65f8e-abde-2aff-4da8-df6e0b464677@web.de/
>> https://sympa.inria.fr/sympa/arc/cocci/2023-03/msg00115.html
>
> I will queue cleanup patches 1-2,

Thanks for this positive feedback.


>                                   but I do not like patches 3/4 and 4/4.
> I do not think that they make the code better to read or maintain.

I would appreciate if the details for such change reluctance can be clarified better.

Regards,
Markus

Re: [cocci] [0/4] overlayfs: Adjustments for ovl_fill_super()

[Markus Elfring]

>>>> See also:
>>>> https://lore.kernel.org/cocci/87b65f8e-abde-2aff-4da8-df6e0b464677@web.de/
>>>> https://sympa.inria.fr/sympa/arc/cocci/2023-03/msg00115.html
>>>
>>> I will queue cleanup patches 1-2,
>>
>> Thanks for this positive feedback.
>
> Sorry, these patches do not apply to master branch and patch 1
> is no longer correct in master branch and the new mount api changes.

Do you want that I adapt the linked development ideas to the current situation
a bit more?


>>>                                   but I do not like patches 3/4 and 4/4.
>>> I do not think that they make the code better to read or maintain.
>>
>> I would appreciate if the details for such change reluctance can be clarified better.
>
> patch 3:
> I much rather a single error handling label that takes care of
> all the cleanups - it is harder to make mistakes and jump to
> the wrong label when adding new error conditions.

There are different coding style preferences involved.

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


> patch 4:
> Overlayfs uses this coding style all over the place
>
>   err = -ENOMEM;
>   ofs->creator_cred = cred = prepare_creds();
>   if (!cred)
>       goto out_free_ofs;
>
> I don't see the benefit in making err = -ENOMEM conditional.
> I don't see the style after your patch as clearly better than before.

Can it be nicer to set error codes only in exceptional data processing situations?

Regards,
Markus

[cocci] [PATCH] squashfs: Improve exception handling in squashfs_decompressor_create()

[Markus Elfring]

Date: Thu, 30 Mar 2023 18:03:47 +0200

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

Thus perform the following adjustments:

1. Return directly after a call of the function “kzalloc” failed
   at the beginning.

2. Use more appropriate labels instead.

3. Omit extra initialisations (for the variables “decomp_strm” and “err”)
   which became unnecessary with this refactoring.


This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 fs/squashfs/decompressor_multi.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/fs/squashfs/decompressor_multi.c b/fs/squashfs/decompressor_multi.c
index 416c53eedbd1..0a054ba4c541 100644
--- a/fs/squashfs/decompressor_multi.c
+++ b/fs/squashfs/decompressor_multi.c
@@ -62,12 +62,12 @@ static void *squashfs_decompressor_create(struct squashfs_sb_info *msblk,
 				void *comp_opts)
 {
 	struct squashfs_stream *stream;
-	struct decomp_stream *decomp_strm = NULL;
-	int err = -ENOMEM;
+	struct decomp_stream *decomp_strm;
+	int err;

 	stream = kzalloc(sizeof(*stream), GFP_KERNEL);
 	if (!stream)
-		goto out;
+		return ERR_PTR(-ENOMEM);

 	stream->comp_opts = comp_opts;
 	mutex_init(&stream->mutex);
@@ -81,22 +81,25 @@ static void *squashfs_decompressor_create(struct squashfs_sb_info *msblk,
 	 * file system works.
 	 */
 	decomp_strm = kmalloc(sizeof(*decomp_strm), GFP_KERNEL);
-	if (!decomp_strm)
-		goto out;
+	if (!decomp_strm) {
+		err = -ENOMEM;
+		goto free_stream;
+	}

 	decomp_strm->stream = msblk->decompressor->init(msblk,
 						stream->comp_opts);
 	if (IS_ERR(decomp_strm->stream)) {
 		err = PTR_ERR(decomp_strm->stream);
-		goto out;
+		goto free_decomp_stream;
 	}

 	list_add(&decomp_strm->list, &stream->strm_list);
 	stream->avail_decomp = 1;
 	return stream;

-out:
+free_decomp_stream:
 	kfree(decomp_strm);
+free_stream:
 	kfree(stream);
 	return ERR_PTR(err);
 }
--
2.40.0

Re: [cocci] [PATCH] squashfs: Improve exception handling in squashfs_decompressor_create()

[Markus Elfring]

> Date: Thu, 30 Mar 2023 18:03:47 +0200
>
> The label “out” was used to jump to a kfree() call despite of
> the detail in the implementation of the function
> “squashfs_decompressor_create” that it was determined already
> that a corresponding variable contained a null pointer because of
> a failed memory allocation.
>
> Thus perform the following adjustments:
>
> 1. Return directly after a call of the function “kzalloc” failed
>    at the beginning.
>
> 2. Use more appropriate labels instead.
>
> 3. Omit extra initialisations (for the variables “decomp_strm” and “err”)
>    which became unnecessary with this refactoring.
>
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  fs/squashfs/decompressor_multi.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/fs/squashfs/decompressor_multi.c b/fs/squashfs/decompressor_multi.c
> index 416c53eedbd1..0a054ba4c541 100644
> --- a/fs/squashfs/decompressor_multi.c
> +++ b/fs/squashfs/decompressor_multi.c
> @@ -62,12 +62,12 @@ static void *squashfs_decompressor_create(struct squashfs_sb_info *msblk,
>  				void *comp_opts)
>  {
>  	struct squashfs_stream *stream;
> -	struct decomp_stream *decomp_strm = NULL;
> -	int err = -ENOMEM;
> +	struct decomp_stream *decomp_strm;
> +	int err;
>
>  	stream = kzalloc(sizeof(*stream), GFP_KERNEL);
>  	if (!stream)
> -		goto out;
> +		return ERR_PTR(-ENOMEM);
>
>  	stream->comp_opts = comp_opts;
>  	mutex_init(&stream->mutex);
> @@ -81,22 +81,25 @@ static void *squashfs_decompressor_create(struct squashfs_sb_info *msblk,
>  	 * file system works.
>  	 */
>  	decomp_strm = kmalloc(sizeof(*decomp_strm), GFP_KERNEL);
> -	if (!decomp_strm)
> -		goto out;
> +	if (!decomp_strm) {
> +		err = -ENOMEM;
> +		goto free_stream;
> +	}
>
>  	decomp_strm->stream = msblk->decompressor->init(msblk,
>  						stream->comp_opts);
>  	if (IS_ERR(decomp_strm->stream)) {
>  		err = PTR_ERR(decomp_strm->stream);
> -		goto out;
> +		goto free_decomp_stream;
>  	}
>
>  	list_add(&decomp_strm->list, &stream->strm_list);
>  	stream->avail_decomp = 1;
>  	return stream;
>
> -out:
> +free_decomp_stream:
>  	kfree(decomp_strm);
> +free_stream:
>  	kfree(stream);
>  	return ERR_PTR(err);
>  }

Is this patch still in review queues?

See also:
https://lore.kernel.org/cocci/f1712777-97ff-d89c-0bdd-d72faed9a7f1@web.de/
https://sympa.inria.fr/sympa/arc/cocci/2023-03/msg00120.html

Regards,
Markus