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

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
> 

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

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: [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/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] 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: [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: [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: [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: [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: [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/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: [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: 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: [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.

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

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

[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: [cocci] [PATCH] io_uring: Fix exception handling in io_ring_ctx_alloc()

[Gabriel Krisman Bertazi]

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

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

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

Also, the patch no longer applies.

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

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

-- 
Gabriel Krisman Bertazi

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

[Markus Elfring]

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

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

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

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

--
2.43.0

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

[Markus Elfring]

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

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

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

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


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

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

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

[Markus Elfring]

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

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

1. Thus use more appropriate labels instead.

2. Reorder jump targets at the end.

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

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


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

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

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

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

[Pavel Begunkov]

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

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

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

-- 
Pavel Begunkov

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

[Gabriel Krisman Bertazi]

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

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

Perhaps rewrite this to:

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

Other than that, and for this patch only:

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

thanks,

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

-- 
Gabriel Krisman Bertazi

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

[Gabriel Krisman Bertazi]

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

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

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

-- 
Gabriel Krisman Bertazi

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

[Markus Elfring]

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

I dare to present other development views.


> as kfree can handle NULL pointers just fine,

Yes, this is the case.

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

May it be skipped in more cases?


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

There are usual opportunities to consider for further collateral evolution.

Regards,
Markus

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

[Jens Axboe]

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

This is just nonsense...

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

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

-- 
Jens Axboe

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

[Gabriel Krisman Bertazi]

Jens Axboe <axboe@kernel.dk> writes:

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

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

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

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

-- 
Gabriel Krisman Bertazi

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

[Markus Elfring]

> patch 2 is just garbage.

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


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

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

Regards,
Markus