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

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

[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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[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

[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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[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

[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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [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: [0/2] ARM: Adjustments for init_atags_procfs()

[Russell King (Oracle)]

On Wed, Jan 10, 2024 at 12:52:10PM +0100, Markus Elfring wrote:
> > 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. 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. See signature below.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Russell King (Oracle)]

On Wed, Jan 10, 2024 at 12:52:10PM +0100, Markus Elfring wrote:
> > 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. 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. See signature below.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [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: [0/2] ARM: Adjustments for init_atags_procfs()

[Russell King (Oracle)]

On Wed, Jan 10, 2024 at 01:44:01PM +0100, Markus Elfring wrote:
> >>> 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?

Err what? Sorry, I don't understand your comment.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Russell King (Oracle)]

On Wed, Jan 10, 2024 at 01:44:01PM +0100, Markus Elfring wrote:
> >>> 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?

Err what? Sorry, I don't understand your comment.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [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: [0/2] ARM: Adjustments for init_atags_procfs()

[Russell King (Oracle)]

On Wed, Jan 10, 2024 at 01:52:34PM +0100, Markus Elfring wrote:
> >>>>> 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?

How about doing the research yourself? Using lore, it's possible to
work it out. Find the message id. Then visit

https://lore.kernel.org/r/<message-id>

and study the result. It will give you the answer you want.

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

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Russell King (Oracle)]

On Wed, Jan 10, 2024 at 01:52:34PM +0100, Markus Elfring wrote:
> >>>>> 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?

How about doing the research yourself? Using lore, it's possible to
work it out. Find the message id. Then visit

https://lore.kernel.org/r/<message-id>

and study the result. It will give you the answer you want.

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

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [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: [0/2] ARM: Adjustments for init_atags_procfs()

[Russell King (Oracle)]

On Wed, Jan 10, 2024 at 03:00:11PM +0100, Markus Elfring wrote:
> >> 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?

Welcome to my /dev/null mailbox. It has plenty of space. Feel free to
continue sending your claptrap there. Rest assured I won't read it
anymore.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Russell King (Oracle)]

On Wed, Jan 10, 2024 at 03:00:11PM +0100, Markus Elfring wrote:
> >> 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?

Welcome to my /dev/null mailbox. It has plenty of space. Feel free to
continue sending your claptrap there. Rest assured I won't read it
anymore.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Christian Heusel]

[-- Attachment #1: Type: text/plain, Size: 1250 bytes --]

On 24/01/10 12:47PM, Russell King (Oracle) wrote:
> On Wed, Jan 10, 2024 at 01:44:01PM +0100, Markus Elfring wrote:
> > >>> 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?
> 
> Err what? Sorry, I don't understand your comment.

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

Cheers,
Chris

[0]: https://lkml.org/lkml/2023/6/19/38
[1]: https://lore.kernel.org/lkml/CAHC9VhREfdgiCji=uEeCrc4w1kPGfnWGKnJuUYKXwTApdneSjQ@mail.gmail.com/T/#m1a55ecc3205045fe63ab0f12451705df911d31a0
[2]: https://lore.kernel.org/all/20200629081039.GA1221843@kroah.com/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Christian Heusel]

[-- Attachment #1.1: Type: text/plain, Size: 1250 bytes --]

On 24/01/10 12:47PM, Russell King (Oracle) wrote:
> On Wed, Jan 10, 2024 at 01:44:01PM +0100, Markus Elfring wrote:
> > >>> 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?
> 
> Err what? Sorry, I don't understand your comment.

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

Cheers,
Chris

[0]: https://lkml.org/lkml/2023/6/19/38
[1]: https://lore.kernel.org/lkml/CAHC9VhREfdgiCji=uEeCrc4w1kPGfnWGKnJuUYKXwTApdneSjQ@mail.gmail.com/T/#m1a55ecc3205045fe63ab0f12451705df911d31a0
[2]: https://lore.kernel.org/all/20200629081039.GA1221843@kroah.com/

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

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

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

[John Johansen]

On 3/29/23 03:12, Markus Elfring wrote:
> 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>

Acked-by: John Johansen <john.johansen@canonical.com>

thanks, I have pulled this into the apparmor tree

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

[Drbd-dev] [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

[Drbd-dev] [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

[Drbd-dev] [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

[Drbd-dev] [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

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

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

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

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

[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: [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: [PATCH 0/4] overlayfs: Adjustments for ovl_fill_super()

[Amir Goldstein]

On Wed, Jan 10, 2024 at 2:25 PM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> > 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?
>

Sorry, this series was not on my radar.

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

Thanks,
Amir.

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: [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: [0/4] overlayfs: Adjustments for ovl_fill_super()

[Amir Goldstein]

On Wed, Jan 10, 2024 at 3:01 PM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> >> 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.

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

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.

Thanks,
Amir.

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

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

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

[Amir Goldstein]

On Wed, Jan 10, 2024 at 3:33 PM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> >>>> 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?
>

No thanks.
Patch 1 just doesn't work for the new mount api.

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

As long as coding styles are not mandatory
I prefer what we have right now.

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

It's a matter of taste.
I'll stay with what we have.

Thanks,
Amir.

[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

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