Re: [PATCH] powerpc/pseries: Fix exception handling in pSeries_reconfig_add_node()

Re: [PATCH] powerpc/pseries: Fix exception handling in pSeries_reconfig_add_node()

[Nathan Lynch]

Markus Elfring <Markus.Elfring@web.de> writes:
>
> The label “out_err” was used to jump to another pointer check despite of
> the detail in the implementation of the function “pSeries_reconfig_add_node”
> that it was determined already that the corresponding variable contained
> a null pointer (because of a failed function call in two cases).
>
> 1. Thus return directly after a call of the function “kzalloc” failed.
>
> 2. Use more appropriate labels instead.
>
> 3. Delete a redundant check.
>
> 4. Omit an explicit initialisation for the local variable “err”.
>
> This issue was detected by using the Coccinelle software.

Is there a correctness or safety issue here? The subject uses the word
"fix" but the commit message doesn't seem to identify one.

Can you share how Coccinelle is being invoked and its output?

Re: powerpc/pseries: Fix exception handling in pSeries_reconfig_add_node()

[Nathan Lynch]

Markus Elfring <Markus.Elfring@web.de> writes:
>>> The label “out_err” was used to jump to another pointer check despite of
>>> the detail in the implementation of the function “pSeries_reconfig_add_node”
>>> that it was determined already that the corresponding variable contained
>>> a null pointer (because of a failed function call in two cases).
>>>
>>> 1. Thus return directly after a call of the function “kzalloc” failed.
>>>
>>> 2. Use more appropriate labels instead.
>>>
>>> 3. Delete a redundant check.
>>>
>>> 4. Omit an explicit initialisation for the local variable “err”.
>>>
>>> This issue was detected by using the Coccinelle software.
>> Is there a correctness or safety issue here?
>
> I got the impression that the application of only a single label like “out_err”
> resulted in improvable implementation details.

I don't understand what you're trying to say here. It doesn't seem to
answer my question.

>> The subject uses the word "fix" but the commit message doesn't seem to identify one.
>
> Can you find the proposed adjustments reasonable?

In the absence of a bug fix or an improvement in readability, not
really, sorry. It adds to the function more goto labels and another
return, apparently to avoid checks that are sometimes redundant (but not
incorrect) at the C source code level. An optimizing compiler doesn't
necessarily arrange the generated code in the same way.

>> Can you share how Coccinelle is being invoked and its output?
>
> Please take another look at available information sources.
> https://lore.kernel.org/cocci/f9303bdc-b1a7-be5e-56c6-dfa8232b8b55@web.de/

I wasn't cc'd on this and I'm not subscribed to any lists in the
recipients for that message, so not sure how I would take "another"
look. :-)

Re: [PATCH] RDMA/siw: Fix exception handling in siw_accept_newconn()

[Leon Romanovsky]

On Sat, Mar 18, 2023 at 08:43:04PM +0100, Markus Elfring wrote:
> Date: Sat, 18 Mar 2023 20:30:12 +0100
> 
> The label “error” was used to jump to another pointer check despite of
> the detail in the implementation of the function “siw_accept_newconn”
> that it was determined already that corresponding variables contained
> still null pointers.
> 
> 1. Use more appropriate labels instead.
> 
> 2. Delete two questionable checks.
> 
> 3. Omit extra initialisations (for the variables “new_cep” and “new_s”)
>    which became unnecessary with this refactoring.
> 
> This issue was detected by using the Coccinelle software.
> 
> Fixes: 6c52fdc244b5ccc468006fd65a504d4ee33743c7 ("rdma/siw: connection management")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/infiniband/sw/siw/siw_cm.c | 32 ++++++++++++++----------------
>  1 file changed, 15 insertions(+), 17 deletions(-)

Please read Documentation/process/submitting-patches.rst and resubmit.
Your patch is not valid.

Thanks

Re: [PATCH] RDMA/erdma: Fix exception handling in erdma_accept_newconn()

[Leon Romanovsky]

On Sat, Mar 18, 2023 at 09:15:58PM +0100, Markus Elfring wrote:
> Date: Sat, 18 Mar 2023 21:08:58 +0100
> 
> The label “error” was used to jump to another pointer check despite of
> the detail in the implementation of the function “erdma_accept_newconn”
> that it was determined already that corresponding variables contained
> still null pointers.
> 
> 1. Thus return directly if
>    * the cep state is not the value “ERDMA_EPSTATE_LISTENING”
>      or
>    * a call of the function “erdma_cep_alloc” failed.
> 
> 2. Use more appropriate labels instead.
> 
> 3. Delete two questionable checks.
> 
> 4. Omit extra initialisations (for the variables “new_cep”, “new_s” and “ret”)
>    which became unnecessary with this refactoring.
> 
> 
> This issue was detected by using the Coccinelle software.
> 
> Fixes: 920d93eac8b97778fef48f34f10e58ddf870fc2a ("RDMA/erdma: Add connection management (CM) support")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/infiniband/hw/erdma/erdma_cm.c | 39 +++++++++++---------------
>  1 file changed, 17 insertions(+), 22 deletions(-)

Same comment as for your RDMA/siw patch.

Thanks

Re: [PATCH] RDMA/erdma: Fix exception handling in erdma_accept_newconn()

[Cheng Xu]

On 3/19/23 4:15 AM, Markus Elfring wrote:
> Date: Sat, 18 Mar 2023 21:08:58 +0100
> 

<...>

> +disassoc_socket:
> +    erdma_socket_disassoc(new_s);
> +    sock_release(new_s);
> +    new_cep->state = ERDMA_EPSTATE_CLOSED;
> +    erdma_cancel_mpatimer(new_cep);
> +put_cep:
> +    erdma_cep_put(new_cep);> +    new_cep->sock = NULL;

Thanks, but this causes an use-after-free issue because new_cep will be
released after last erdma_cep_put being called.

Cheng Xu

>  }
>  
>  static int erdma_newconn_connected(struct erdma_cep *cep)

Re: [PATCH] RDMA/siw: Fix exception handling in siw_accept_newconn()

[Leon Romanovsky]

On Sun, Mar 19, 2023 at 02:38:03PM +0100, Markus Elfring wrote:
> >> Date: Sat, 18 Mar 2023 20:30:12 +0100
> >>
> >> The label “error” was used to jump to another pointer check despite of
> >> the detail in the implementation of the function “siw_accept_newconn”
> >> that it was determined already that corresponding variables contained
> >> still null pointers.
> >>
> >> 1. Use more appropriate labels instead.
> >>
> >> 2. Delete two questionable checks.
> >>
> >> 3. Omit extra initialisations (for the variables “new_cep” and “new_s”)
> >>    which became unnecessary with this refactoring.
> >>
> >> This issue was detected by using the Coccinelle software.
> >>
> >> Fixes: 6c52fdc244b5ccc468006fd65a504d4ee33743c7 ("rdma/siw: connection management")
> >> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> >> ---
> >>  drivers/infiniband/sw/siw/siw_cm.c | 32 ++++++++++++++----------------
> >>  1 file changed, 15 insertions(+), 17 deletions(-)
> > Please read Documentation/process/submitting-patches.rst and resubmit.
> > Your patch is not valid.
> 
> 
> What do you find improvable here?

Did you read the guide above?

1. The patch is malformed and doesn't appear in lore and patchworks.
2. "Date ..." in the middle of patch
3. Wrong Fixes line.
4. Patch contains too much and too different things at the same time.

Thanks

> 
> Regards,
> Markus
> 

Re: RDMA/siw: Fix exception handling in siw_accept_newconn()

[Leon Romanovsky]

On Sun, Mar 19, 2023 at 04:40:17PM +0100, Markus Elfring wrote:
> >>>> Date: Sat, 18 Mar 2023 20:30:12 +0100
> >>>>
> >>>> The label “error” was used to jump to another pointer check despite of
> >>>> the detail in the implementation of the function “siw_accept_newconn”
> >>>> that it was determined already that corresponding variables contained
> >>>> still null pointers.
> >>>>
> >>>> 1. Use more appropriate labels instead.
> >>>>
> >>>> 2. Delete two questionable checks.
> >>>>
> >>>> 3. Omit extra initialisations (for the variables “new_cep” and “new_s”)
> >>>>    which became unnecessary with this refactoring.
> >>>>
> >>>> This issue was detected by using the Coccinelle software.
> >>>>
> >>>> Fixes: 6c52fdc244b5ccc468006fd65a504d4ee33743c7 ("rdma/siw: connection management")
> >>>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> >>>> ---
> >>>>  drivers/infiniband/sw/siw/siw_cm.c | 32 ++++++++++++++----------------
> >>>>  1 file changed, 15 insertions(+), 17 deletions(-)
> >>> Please read Documentation/process/submitting-patches.rst and resubmit.
> >>> Your patch is not valid.
> >>
> >> What do you find improvable here?
> > Did you read the guide above?
> 
> Yes, of course (several times before).

ok, I'm taking my request to resubmit back.
Please retain from posting to RDMA ML. I'm not going to apply any of
your patches.

Thanks

Re: [PATCH] Input: iforce - Fix exception handling in iforce_usb_probe()

[Dmitry Torokhov]

On Sun, Mar 19, 2023 at 07:03:00PM +0100, Markus Elfring wrote:
> Date: Sun, 19 Mar 2023 18:50:51 +0100
> 
> The label “fail” was used to jump to another pointer check despite of
> the detail in the implementation of the function “iforce_usb_probe”
> that it was determined already that a corresponding variable contained
> still a null pointer.
> 
> 1. Use more appropriate labels instead.
> 
> 2. Reorder jump targets at the end.
> 
> 3. Delete a redundant check.
> 
> 
> This issue was detected by using the Coccinelle software.

I am sorry, but I do not understand what the actual issue is. The fact
that come Coccinelle script complains is not enough to change the code.

Thanks.

-- 
Dmitry

Re: [PATCH] Input: iforce - Fix exception handling in iforce_usb_probe()

[Tetsuo Handa]

On 2023/03/20 13:21, Dmitry Torokhov wrote:
> On Sun, Mar 19, 2023 at 07:03:00PM +0100, Markus Elfring wrote:
>> Date: Sun, 19 Mar 2023 18:50:51 +0100
>>
>> The label “fail” was used to jump to another pointer check despite of
>> the detail in the implementation of the function “iforce_usb_probe”
>> that it was determined already that a corresponding variable contained
>> still a null pointer.
>>
>> 1. Use more appropriate labels instead.
>>
>> 2. Reorder jump targets at the end.
>>
>> 3. Delete a redundant check.
>>
>>
>> This issue was detected by using the Coccinelle software.
> 
> I am sorry, but I do not understand what the actual issue is. The fact
> that come Coccinelle script complains is not enough to change the code.
> 

Right. There is no issue with the code, for usb_free_urb(NULL) is a no-op.
Proposing as a cleanup, without Fixes: tags, could be possible though.

Re: [PATCH] Input: iforce - Fix exception handling in iforce_usb_probe()

[Dmitry Torokhov]

On Mon, Mar 20, 2023 at 01:34:52PM +0900, Tetsuo Handa wrote:
> On 2023/03/20 13:21, Dmitry Torokhov wrote:
> > On Sun, Mar 19, 2023 at 07:03:00PM +0100, Markus Elfring wrote:
> >> Date: Sun, 19 Mar 2023 18:50:51 +0100
> >>
> >> The label “fail” was used to jump to another pointer check despite of
> >> the detail in the implementation of the function “iforce_usb_probe”
> >> that it was determined already that a corresponding variable contained
> >> still a null pointer.
> >>
> >> 1. Use more appropriate labels instead.
> >>
> >> 2. Reorder jump targets at the end.
> >>
> >> 3. Delete a redundant check.
> >>
> >>
> >> This issue was detected by using the Coccinelle software.
> > 
> > I am sorry, but I do not understand what the actual issue is. The fact
> > that come Coccinelle script complains is not enough to change the code.
> > 
> 
> Right. There is no issue with the code, for usb_free_urb(NULL) is a no-op.
> Proposing as a cleanup, without Fixes: tags, could be possible though.

Yes, that would be acceptable.

Thanks.

-- 
Dmitry

Re: powerpc/pseries: Fix exception handling in pSeries_reconfig_add_node()

[Nathan Lynch]

Markus Elfring <Markus.Elfring@web.de> writes:
>>>>> The label “out_err” was used to jump to another pointer check despite of
>>>>> the detail in the implementation of the function “pSeries_reconfig_add_node”
>>>>> that it was determined already that the corresponding variable contained
>>>>> a null pointer (because of a failed function call in two cases).
>>>>>
>>>>> 1. Thus return directly after a call of the function “kzalloc” failed.
>>>>>
>>>>> 2. Use more appropriate labels instead.
>>>>>
>>>>> 3. Delete a redundant check.
>>>>>
>>>>> 4. Omit an explicit initialisation for the local variable “err”.
>>>>>
>>>>> This issue was detected by using the Coccinelle software.
>>>> Is there a correctness or safety issue here?
>>> I got the impression that the application of only a single label like “out_err”
>>> resulted in improvable implementation details.
>> I don't understand what you're trying to say here.
>
> What does hinder you to understand the presented change description better
> at the moment?
>
>
>> It doesn't seem to answer my question.
>
>
> I hope that my answer will trigger further helpful considerations.

I don't consider this response constructive, but I want to get this back
on track. It's been brought to my attention that there is in fact a
crash bug in this function's error path:

	np->parent = pseries_of_derive_parent(path);
	if (IS_ERR(np->parent)) {
		err = PTR_ERR(np->parent);
		goto out_err;
	}
...
out_err:
	if (np) {
		of_node_put(np->parent);

np->parent can be an encoded error value, we don't want to of_node_put()
that.

I believe the patch as written happens to fix the issue. Will you please
write it up as a bug fix and resubmit?

Re: [PATCH] media: hantro: HEVC: Fix exception handling in tile_buffer_reallocate()

[Benjamin Gaignard]

Hi Markus,

Thanks for your patch,

Le 20/03/2023 à 19:43, Markus Elfring a écrit :
> Date: Mon, 20 Mar 2023 19:13:20 +0100
>
> The label “err_free_tile_buffers” was used to jump to another pointer
> check despite of the detail in the implementation of the function
> “tile_buffer_reallocate” that it was determined already that
> a corresponding variable contained a null pointer because of a failed
> function call “dma_alloc_coherent”.
>
> * Thus use an additional label instead.
>
> * Delete a redundant check.
>
>
> This issue was detected by using the Coccinelle software.

If you want to optimize the error path I think the best
option is to return -ENOMEM when hevc_dec->tile_filter.cpu is NULL,
remove
	if (hevc_dec->tile_bsd.cpu)
		dma_free_coherent(vpu->dev, hevc_dec->tile_bsd.size,
				  hevc_dec->tile_bsd.cpu,
				  hevc_dec->tile_bsd.dma);
and reorder the two other dma_free to get something clean.

Regards,
Benjamin

>
> Fixes: cb5dd5a0fa518dff14ff2b90837c3c8f98f4dd5c ("media: hantro: Introduce G2/HEVC decoder")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>   drivers/media/platform/verisilicon/hantro_hevc.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/platform/verisilicon/hantro_hevc.c b/drivers/media/platform/verisilicon/hantro_hevc.c
> index 9383fb7081f6..ac60df18efb7 100644
> --- a/drivers/media/platform/verisilicon/hantro_hevc.c
> +++ b/drivers/media/platform/verisilicon/hantro_hevc.c
> @@ -109,7 +109,7 @@ static int tile_buffer_reallocate(struct hantro_ctx *ctx)
>                                  &hevc_dec->tile_filter.dma,
>                                  GFP_KERNEL);
>       if (!hevc_dec->tile_filter.cpu)
> -        goto err_free_tile_buffers;
> +        goto recheck_tile_sao_cpu;
>       hevc_dec->tile_filter.size = size;
>   
>       size = (VERT_SAO_RAM_SIZE * height64 * (num_tile_cols - 1) * ctx->bit_depth) / 8;
> @@ -133,12 +133,12 @@ static int tile_buffer_reallocate(struct hantro_ctx *ctx)
>       return 0;
>   
>   err_free_tile_buffers:
> -    if (hevc_dec->tile_filter.cpu)
> -        dma_free_coherent(vpu->dev, hevc_dec->tile_filter.size,
> -                  hevc_dec->tile_filter.cpu,
> -                  hevc_dec->tile_filter.dma);
> +    dma_free_coherent(vpu->dev, hevc_dec->tile_filter.size,
> +              hevc_dec->tile_filter.cpu,
> +              hevc_dec->tile_filter.dma);
>       hevc_dec->tile_filter.cpu = NULL;
>   
> +recheck_tile_sao_cpu:
>       if (hevc_dec->tile_sao.cpu)
>           dma_free_coherent(vpu->dev, hevc_dec->tile_sao.size,
>                     hevc_dec->tile_sao.cpu,
> --
> 2.40.0
>
>

Re: [PATCH] mm/mempolicy: Fix exception handling in shared_policy_replace()

[Vlastimil Babka]

Your patch doesn't apply, seems like it uses spaces instead of tabs. Also I
can't use 'b4' to download it as there are multiple different patches using
the same message-id:

https://lore.kernel.org/all/6e9ca062-939b-af96-c8ff-56ad485d6e79@web.de/

Re: subject, I don't see a bug that this would fix. You could say it's
"cleanup" and this function could use one, but for a cleanup it's not
improving the situation much.

On 3/23/23 18:30, Markus Elfring wrote:
> Date: Thu, 23 Mar 2023 18:18:59 +0100
> 
> The label “err_out” was used to jump to another pointer check despite of
> the detail in the implementation of the function “shared_policy_replace”
> that it was determined already that a corresponding variable contained a
> null pointer because of a failed call of the function “kmem_cache_alloc”.
> 
> 1. Use more appropriate labels instead.
> 
> 2. The implementation of the function “mpol_put” contains a pointer check
>    for its single input parameter.
>    Thus delete a redundant check in the caller.
> 
> 
> This issue was detected by using the Coccinelle software.
> 
> Fixes: 42288fe366c4f1ce7522bc9f27d0bc2a81c55264 ("mm: mempolicy: Convert shared_policy mutex to spinlock")

Again this is not a fix.

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  mm/mempolicy.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index a256a241fd1d..fb0485688dcb 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -2736,13 +2736,12 @@ static int shared_policy_replace(struct shared_policy *sp, unsigned long start,
>          sp_insert(sp, new);
>      write_unlock(&sp->lock);
>      ret = 0;
> +put_mpol:
> +    mpol_put(mpol_new);
>  
> -err_out:
> -    if (mpol_new)
> -        mpol_put(mpol_new);
>      if (n_new)
>          kmem_cache_free(sn_cache, n_new);
> -
> +exit:
>      return ret;
>  
>  alloc_new:
> @@ -2750,10 +2749,10 @@ static int shared_policy_replace(struct shared_policy *sp, unsigned long start,
>      ret = -ENOMEM;
>      n_new = kmem_cache_alloc(sn_cache, GFP_KERNEL);
>      if (!n_new)
> -        goto err_out;
> +        goto exit;

Just "return ret" and no need for exit label?

>      mpol_new = kmem_cache_alloc(policy_cache, GFP_KERNEL);
>      if (!mpol_new)
> -        goto err_out;
> +        goto put_mpol;

We are doing this because mpol_new == NULL, so we know there's no reason to
do mpol_put(), we could jump to the freeing of n_new.

>      atomic_set(&mpol_new->refcnt, 1);
>      goto restart;
>  }
> --
> 2.40.0
> 
> 
> 

Re: [PATCH resent] drm/amd/display: Fix exception handling in dm_validate_stream_and_context()

[Hamza Mahfooz]

Hi Markus,

On 3/24/23 11:42, Markus Elfring wrote:
> Date: Sat, 18 Mar 2023 16:21:32 +0100
> 
> The label “cleanup” was used to jump to another pointer check despite of
> the detail in the implementation of the function “dm_validate_stream_and_context”
> that it was determined already that corresponding variables contained
> still null pointers.
> 
> 1. Thus return directly if
>     * a null pointer was passed for the function parameter “stream”
>       or
>     * a call of the function “dc_create_plane_state” failed.
> 
> 2. Use a more appropriate label instead.
> 
> 3. Delete two questionable checks.
> 
> 4. Omit extra initialisations (for the variables “dc_state” and “dc_plane_state”)
>     which became unnecessary with this refactoring.
> 
> 
> This issue was detected by using the Coccinelle software.
> 
> Fixes: 5468c36d628524effbb89a9503eb1a2318804759 ("drm/amd/display: Filter Invalid 420 Modes for HDMI TMDS")

Please truncate the hash to 12 characters.

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

Also please ensure that your Signed-off-by matches the "From:" entry in 
the email.

> ---
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 20 ++++++++-----------
>   1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index eeaeca8b51f4..3086613f5f5d 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6426,19 +6426,19 @@ static enum dc_status dm_validate_stream_and_context(struct dc *dc,
>   						struct dc_stream_state *stream)
>   {
>   	enum dc_status dc_result = DC_ERROR_UNEXPECTED;
> -	struct dc_plane_state *dc_plane_state = NULL;
> -	struct dc_state *dc_state = NULL;
> +	struct dc_plane_state *dc_plane_state;
> +	struct dc_state *dc_state;
> 
>   	if (!stream)
> -		goto cleanup;
> +		return dc_result;
> 
>   	dc_plane_state = dc_create_plane_state(dc);
>   	if (!dc_plane_state)
> -		goto cleanup;
> +		return dc_result;
> 
>   	dc_state = dc_create_state(dc);
>   	if (!dc_state)
> -		goto cleanup;
> +		goto release_plane_state;
> 
>   	/* populate stream to plane */
>   	dc_plane_state->src_rect.height  = stream->src.height;
> @@ -6475,13 +6475,9 @@ static enum dc_status dm_validate_stream_and_context(struct dc *dc,
>   	if (dc_result == DC_OK)
>   		dc_result = dc_validate_global_state(dc, dc_state, true);
> 
> -cleanup:
> -	if (dc_state)
> -		dc_release_state(dc_state);
> -
> -	if (dc_plane_state)
> -		dc_plane_state_release(dc_plane_state);
> -
> +	dc_release_state(dc_state);
> +release_plane_state:
> +	dc_plane_state_release(dc_plane_state);
>   	return dc_result;
>   }
> 
> --
> 2.40.0
> 

-- 
Hamza

Re: [PATCH resent] drm/amd/display: Fix exception handling in dm_validate_stream_and_context()

[Hamza Mahfooz]

On 3/24/23 14:19, Markus Elfring wrote:
> May longer identifiers (or even the complete SHA-1 ID) occasionally also
> be tolerated for the tag “Fixes”?
> 
> How do you think about the proposed change possibilities?

Well, the hash length is restricted by convention (see
./scripts/checkpatch.pl in the kernel tree), so to propose that change
it would have done more broadly than just for amd-gfx.

> 
> Regards,
> Markus

-- 
Hamza

Re: [PATCH] selftests/bpf: Improve exception handling in rbtree_add_and_remove()

[Alexei Starovoitov]

On Fri, Mar 24, 2023 at 7:13 AM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> Date: Fri, 24 Mar 2023 14:54:18 +0100
>
> The label “err_out” was used to jump to another pointer check despite of
> the detail in the implementation of the function “rbtree_add_and_remove”
> that it was determined already that a corresponding variable contained
> a null pointer.
>
> 1. Thus return directly after the first call of the function
>    “bpf_obj_new” failed.
>
> 2. Delete two questionable checks.
>
> 3. Omit an extra initialisation (for the variable “m”)
>    which became unnecessary with this refactoring.
>
>
> This issue was detected by using the Coccinelle software.
>
> Fixes: 215249f6adc0359e3546829e7ee622b5e309b0ad ("selftests/bpf: Add rbtree selftests")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Nack.
Please stop sending such "cleanup" patches.

Re: [PATCH resent] bcache: Fix exception handling in mca_alloc()

[Coly Li]

On 3/25/23 5:31 PM, Markus Elfring wrote:
> Date: Mon, 20 Mar 2023 13:13:37 +0100
>
> The label “err” was used to jump to another pointer check despite of
> the detail in the implementation of the function “mca_alloc”
> that it was determined already that a corresponding variable contained
> a null pointer because of a failed function call “mca_bucket_alloc”.


Hmm, I don't get the exact point from the above long sentence. Could you 
please describe a bit more specific where the problem is at exact line 
number of the code?

> * Thus use a more appropriate label instead.

So far I am not convinced the modified label is more appropriate.

>
> * Delete a redundant check.

Where is the location of the redundant check?


>
>
> This issue was detected by using the Coccinelle software.

Just curious, what is the warning reported by the mentioned software ?


>
> Fixes: cafe563591446cf80bfbc2fe3bc72a2e36cf1060 ("bcache: A block layer cache")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>   drivers/md/bcache/btree.c | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
> index 147c493a989a..166afd7ec499 100644
> --- a/drivers/md/bcache/btree.c
> +++ b/drivers/md/bcache/btree.c
> @@ -921,18 +921,18 @@ static struct btree *mca_alloc(struct cache_set *c, struct btree_op *op,
>   		if (!mca_reap(b, 0, false)) {
>   			mca_data_alloc(b, k, __GFP_NOWARN|GFP_NOIO);
>   			if (!b->keys.set[0].data)
> -				goto err;
> +				goto unlock;
>   			else
>   				goto out;
>   		}
>
>   	b = mca_bucket_alloc(c, k, __GFP_NOWARN|GFP_NOIO);
>   	if (!b)
> -		goto err;
> +		goto unlock;
>
>   	BUG_ON(!down_write_trylock(&b->lock));
>   	if (!b->keys.set->data)
> -		goto err;
> +		goto unlock;
>   out:
>   	BUG_ON(b->io_mutex.count != 1);
>
> @@ -955,9 +955,8 @@ static struct btree *mca_alloc(struct cache_set *c, struct btree_op *op,
>   				    &b->c->expensive_debug_checks);
>
>   	return b;
> -err:
> -	if (b)
> -		rw_unlock(true, b);
> +unlock:
> +	rw_unlock(true, b);

If b is NULL, is it a bit fishing to send the NULL pointer into 
rw_unlock() ?


Thanks in advance for more information.


Coly Li

RE: [PATCH resent] mei: Fix exception handling in mei_cl_irq_read_msg()

[Winkler, Tomas]

> Date: Tue, 21 Mar 2023 18:11:13 +0100
> 
> The label “discard” was used to jump to another pointer check despite of the
> detail in the implementation of the function “mei_cl_irq_read_msg”
> that it was determined already that a corresponding variable contained a null
> pointer.
> 
> * Thus use an additional label instead.
> 
> * Delete a redundant check.
> 
> 
> This issue was detected by using the Coccinelle software.
> 
> Fixes: a808c80cdaa83939b220176fcdffca8385d88ba6 ("mei: add read callback
> on demand for fixed_address clients")
> Fixes: 17ba8a08b58a01bbac35790ffca4388ca92b7790 ("mei: consolidate
> repeating code in mei_cl_irq_read_msg")


This is a refactoring not a bug fix, or am I missing something 

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

This looks better than the original code, but I would drop the 'Fix' wording.  


> ---
>  drivers/misc/mei/interrupt.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/misc/mei/interrupt.c b/drivers/misc/mei/interrupt.c index
> 0a0e984e5673..9800d30b7693 100644
> --- a/drivers/misc/mei/interrupt.c
> +++ b/drivers/misc/mei/interrupt.c
> @@ -136,7 +136,7 @@ static int mei_cl_irq_read_msg(struct mei_cl *cl,
>  				cb->ext_hdr = kzalloc(sizeof(*gsc_f2h),
> GFP_KERNEL);
>  				if (!cb->ext_hdr) {
>  					cb->status = -ENOMEM;
> -					goto discard;
> +					goto move_tail;
>  				}
>  				break;
>  			case MEI_EXT_HDR_NONE:
> @@ -153,7 +153,7 @@ static int mei_cl_irq_read_msg(struct mei_cl *cl,
>  		if (!vtag_hdr && !gsc_f2h) {
>  			cl_dbg(dev, cl, "no vtag or gsc found in extended
> header.\n");
>  			cb->status = -EPROTO;
> -			goto discard;
> +			goto move_tail;
>  		}
>  	}
> 
> @@ -163,7 +163,7 @@ static int mei_cl_irq_read_msg(struct mei_cl *cl,
>  			cl_err(dev, cl, "mismatched tag: %d != %d\n",
>  			       cb->vtag, vtag_hdr->vtag);
>  			cb->status = -EPROTO;
> -			goto discard;
> +			goto move_tail;
>  		}
>  		cb->vtag = vtag_hdr->vtag;
>  	}
> @@ -174,18 +174,18 @@ static int mei_cl_irq_read_msg(struct mei_cl *cl,
>  		if (!dev->hbm_f_gsc_supported) {
>  			cl_err(dev, cl, "gsc extended header is not
> supported\n");
>  			cb->status = -EPROTO;
> -			goto discard;
> +			goto move_tail;
>  		}
> 
>  		if (length) {
>  			cl_err(dev, cl, "no data allowed in cb with gsc\n");
>  			cb->status = -EPROTO;
> -			goto discard;
> +			goto move_tail;
>  		}
>  		if (ext_hdr_len > sizeof(*gsc_f2h)) {
>  			cl_err(dev, cl, "gsc extended header is too big %u\n",
> ext_hdr_len);
>  			cb->status = -EPROTO;
> -			goto discard;
> +			goto move_tail;
>  		}
>  		memcpy(cb->ext_hdr, gsc_f2h, ext_hdr_len);
>  	}
> @@ -193,7 +193,7 @@ static int mei_cl_irq_read_msg(struct mei_cl *cl,
>  	if (!mei_cl_is_connected(cl)) {
>  		cl_dbg(dev, cl, "not connected\n");
>  		cb->status = -ENODEV;
> -		goto discard;
> +		goto move_tail;
>  	}
> 
>  	if (mei_hdr->dma_ring)
> @@ -205,14 +205,14 @@ static int mei_cl_irq_read_msg(struct mei_cl *cl,
>  		cl_err(dev, cl, "message is too big len %d idx %zu\n",
>  		       length, cb->buf_idx);
>  		cb->status = -EMSGSIZE;
> -		goto discard;
> +		goto move_tail;
>  	}
> 
>  	if (cb->buf.size < buf_sz) {
>  		cl_dbg(dev, cl, "message overflow. size %zu len %d idx
> %zu\n",
>  			cb->buf.size, length, cb->buf_idx);
>  		cb->status = -EMSGSIZE;
> -		goto discard;
> +		goto move_tail;
>  	}
> 
>  	if (mei_hdr->dma_ring) {
> @@ -235,9 +235,9 @@ static int mei_cl_irq_read_msg(struct mei_cl *cl,
> 
>  	return 0;
> 
> +move_tail:
> +	list_move_tail(&cb->list, cmpl_list);
>  discard:
> -	if (cb)
> -		list_move_tail(&cb->list, cmpl_list);
>  	mei_irq_discard_msg(dev, mei_hdr, length);
>  	return 0;
>  }
> --
> 2.40.0

Re: [PATCH v2] bcache: Fix exception handling in mca_alloc()

[Coly Li]

> 2023年3月25日 20:21，Markus Elfring <Markus.Elfring@web.de> 写道：
> 
> Date: Sat, 25 Mar 2023 13:08:01 +0100
> 
> The label “err” was used to jump to another pointer check despite of
> the detail in the implementation of the function “mca_alloc”
> that it was determined already that a corresponding variable contained
> a null pointer because of a failed function call “mca_bucket_alloc”.
> 
> 1. Thus use more appropriate labels instead.

It is not convinced to me that the new added labels are more appropriate. IMHO, the change just makes the code to be more complicated.


> 
> 2. Delete a repeated check (for the variable “b”)
>   which became unnecessary with this refactoring.
> 

To remove one line ‘if’ check, 13 lines are changed. IMHO this is not worthy. Yes the extra ‘if’ check can be avoided, but the code is more simple before adding labels unlock and cannibalize_mca.

The ‘if’ check is in error handling, which is not hot code path. Comparing to avoid an ‘if’ check, I prefer more for more simpler code. I am not supportive to this change.


Thanks.

Coly Li


> 
> This issue was detected by using the Coccinelle software.
> 
> Fixes: cafe563591446cf80bfbc2fe3bc72a2e36cf1060 ("bcache: A block layer cache")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> V2:
> Use another label.
> 
> drivers/md/bcache/btree.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
> index 147c493a989a..c6a20595302f 100644
> --- a/drivers/md/bcache/btree.c
> +++ b/drivers/md/bcache/btree.c
> @@ -921,18 +921,18 @@ static struct btree *mca_alloc(struct cache_set *c, struct btree_op *op,
> if (!mca_reap(b, 0, false)) {
> mca_data_alloc(b, k, __GFP_NOWARN|GFP_NOIO);
> if (!b->keys.set[0].data)
> - goto err;
> + goto unlock;
> else
> goto out;
> }
> 
> b = mca_bucket_alloc(c, k, __GFP_NOWARN|GFP_NOIO);
> if (!b)
> - goto err;
> + goto cannibalize_mca;
> 
> BUG_ON(!down_write_trylock(&b->lock));
> if (!b->keys.set->data)
> - goto err;
> + goto unlock;
> out:
> BUG_ON(b->io_mutex.count != 1);
> 
> @@ -955,10 +955,9 @@ static struct btree *mca_alloc(struct cache_set *c, struct btree_op *op,
>    &b->c->expensive_debug_checks);
> 
> return b;
> -err:
> - if (b)
> - rw_unlock(true, b);
> -
> +unlock:
> + rw_unlock(true, b);
> +cannibalize_mca:
> b = mca_cannibalize(c, op, k);
> if (!IS_ERR(b))
> goto out;
> --
> 2.40.0
> 

Re: [PATCH] selftests: cgroup: Fix exception handling in test_memcg_oom_group_score_events()

[Lorenzo Stoakes]

On Sat, Mar 25, 2023 at 07:30:21PM +0100, Markus Elfring wrote:
> Date: Sat, 25 Mar 2023 19:11:13 +0100
>
> The label “cleanup” was used to jump to another pointer check despite of
> the detail in the implementation of the function
> “test_memcg_oom_group_score_events” that it was determined already
> that a corresponding variable contained a null pointer.

This is poorly writte and confusing. Something like 'avoid unnecessary null
check/cg_destroy() invocation' would be far clearer.

>
> 1. Thus return directly after a call of the function “cg_name” failed.
>

This feels superfluious.

> 2. Use an additional label.

This also feels superfluious.

>
> 3. Delete a questionable check.

This seems superfluois and frankly, rude. It's not questionable, it's
readable, you should try to avoid language like 'questionable' when the
purpose of the check is obvious.

>
>
> This issue was detected by using the Coccinelle software.
>
> Fixes: a987785dcd6c8ae2915460582aebd6481c81eb67 ("Add tests for memory.oom.group")

Fixes what in the what now? This is not a bug fix, it's a 'questionable'
refactoring.

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  tools/testing/selftests/cgroup/test_memcontrol.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c
> index f4f7c0aef702..afcd1752413e 100644
> --- a/tools/testing/selftests/cgroup/test_memcontrol.c
> +++ b/tools/testing/selftests/cgroup/test_memcontrol.c
> @@ -1242,12 +1242,11 @@ static int test_memcg_oom_group_score_events(const char *root)
>  	int safe_pid;
>
>  	memcg = cg_name(root, "memcg_test_0");
> -
>  	if (!memcg)
> -		goto cleanup;
> +		return ret;
>
>  	if (cg_create(memcg))
> -		goto cleanup;
> +		goto free_cg;
>
>  	if (cg_write(memcg, "memory.max", "50M"))
>  		goto cleanup;
> @@ -1275,8 +1274,8 @@ static int test_memcg_oom_group_score_events(const char *root)
>  	ret = KSFT_PASS;
>
>  cleanup:
> -	if (memcg)
> -		cg_destroy(memcg);
> +	cg_destroy(memcg);
> +free_cg:
>  	free(memcg);
>
>  	return ret;
> --
> 2.40.0
>
>

I dislike this patch, it adds complexity for no discernible purpose and
actively makes the code _less_ readable and in a self-test of all places (!)

Not all pedantic Coccinelle results are actionable. Remember that it's
humans who are reading the code.

Your email client/scripting is still somehow broken, I couldn't get b4 to
pull it correctly and it seems to have a duplicate message ID. You really
need to take a look at that (git send-email should do this fine for
example).

Please try to filter the output of Coccinelle and instead of spamming
thousands of pointless patches that add no value, try to choose those that
do add value.

My advice overall would be to just stop.

Re: [PATCH] selftests: cgroup: Fix exception handling in test_memcg_oom_group_score_events()

[David Vernet]

On Sun, Mar 26, 2023 at 10:15:31AM +0200, Markus Elfring wrote:

[...]

> >>
> >> Fixes: a987785dcd6c8ae2915460582aebd6481c81eb67 ("Add tests for memory.oom.group")
> >
> > Fixes what in the what now?
> 
> 1. Check repetition (which can be undesirable)
> 
> 2. Can a cg_destroy() call ever work as expected if a cg_create() call failed?

Perhaps next time you can answer your own question by spending 30
seconds actually reading the code you're "fixing":

int cg_destroy(const char *cgroup)
{
        int ret;

retry:
        ret = rmdir(cgroup);
        if (ret && errno == EBUSY) {
                cg_killall(cgroup);
                usleep(100);
                goto retry;
        }

        if (ret && errno == ENOENT) <<< that case is explicitly handled here
                ret = 0;

        return ret;
}

Re: [PATCH resent] perf/x86/amd/uncore: Fix exception handling in amd_uncore_cpu_up_prepare()

[Adrian Hunter]

On 25/03/23 16:15, Markus Elfring wrote:
> Date: Fri, 17 Mar 2023 13:13:14 +0100
> 
> The label “fail” was used to jump to another pointer check despite of
> the detail in the implementation of the function “amd_uncore_cpu_up_prepare”
> that it was determined already that the corresponding variable contained
> a null pointer (because of a failed function call in two cases).
> 
> 1. Thus return directly after a call of the function “amd_uncore_alloc”
>    failed in the first if branch.
> 
> 2. Use more appropriate labels instead.
> 
> 3. Reorder jump targets at the end.
> 
> 4. Delete a redundant check and kfree() call.
> 
> 5. Omit an explicit initialisation for the local variable “uncore_llc”.
> 
> 
> This issue was detected by using the Coccinelle software.
> 
> Fixes: 39621c5808f5dda75d03dc4b2d4d2b13a5a1c34b ("perf/x86/amd/uncore: Use dynamic events array")
> Fixes: 503d3291a937b726757c1f7c45fa02389d2f4324 ("perf/x86/amd: Try to fix some mem allocation failure handling")

Commit should be only the first 12 characters of the hash.
Refer:	https://docs.kernel.org/process/submitting-patches.html

But this is not a fix. Redundant calls to kfree do not break
anything.

Also avoid using the term "exception" since, in x86, exceptions are
hardware events.  Better to just call it "error handling".

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  arch/x86/events/amd/uncore.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/events/amd/uncore.c b/arch/x86/events/amd/uncore.c
> index 83f15fe411b3..0a9b5cb97bb4 100644
> --- a/arch/x86/events/amd/uncore.c
> +++ b/arch/x86/events/amd/uncore.c
> @@ -440,13 +440,13 @@ amd_uncore_events_alloc(unsigned int num, unsigned int cpu)
> 
>  static int amd_uncore_cpu_up_prepare(unsigned int cpu)
>  {
> -	struct amd_uncore *uncore_nb = NULL, *uncore_llc = NULL;
> +	struct amd_uncore *uncore_nb = NULL, *uncore_llc;
> 
>  	if (amd_uncore_nb) {
>  		*per_cpu_ptr(amd_uncore_nb, cpu) = NULL;
>  		uncore_nb = amd_uncore_alloc(cpu);
>  		if (!uncore_nb)
> -			goto fail;
> +			return -ENOMEM;
>  		uncore_nb->cpu = cpu;
>  		uncore_nb->num_counters = num_counters_nb;
>  		uncore_nb->rdpmc_base = RDPMC_BASE_NB;
> @@ -455,7 +455,7 @@ static int amd_uncore_cpu_up_prepare(unsigned int cpu)
>  		uncore_nb->pmu = &amd_nb_pmu;
>  		uncore_nb->events = amd_uncore_events_alloc(num_counters_nb, cpu);
>  		if (!uncore_nb->events)
> -			goto fail;
> +			goto free_nb;
>  		uncore_nb->id = -1;
>  		*per_cpu_ptr(amd_uncore_nb, cpu) = uncore_nb;
>  	}
> @@ -464,7 +464,7 @@ static int amd_uncore_cpu_up_prepare(unsigned int cpu)
>  		*per_cpu_ptr(amd_uncore_llc, cpu) = NULL;
>  		uncore_llc = amd_uncore_alloc(cpu);
>  		if (!uncore_llc)
> -			goto fail;
> +			goto check_uncore_nb;
>  		uncore_llc->cpu = cpu;
>  		uncore_llc->num_counters = num_counters_llc;
>  		uncore_llc->rdpmc_base = RDPMC_BASE_LLC;
> @@ -473,24 +473,22 @@ static int amd_uncore_cpu_up_prepare(unsigned int cpu)
>  		uncore_llc->pmu = &amd_llc_pmu;
>  		uncore_llc->events = amd_uncore_events_alloc(num_counters_llc, cpu);
>  		if (!uncore_llc->events)
> -			goto fail;
> +			goto free_llc;
>  		uncore_llc->id = -1;
>  		*per_cpu_ptr(amd_uncore_llc, cpu) = uncore_llc;
>  	}
> 
>  	return 0;
> 
> -fail:
> +free_llc:
> +	kfree(uncore_llc);
> +check_uncore_nb:
>  	if (uncore_nb) {
>  		kfree(uncore_nb->events);
> +free_nb:
>  		kfree(uncore_nb);
>  	}
> 
> -	if (uncore_llc) {
> -		kfree(uncore_llc->events);
> -		kfree(uncore_llc);
> -	}
> -
>  	return -ENOMEM;
>  }
> 
> --
> 2.40.0
> 

Re: [PATCH] selftests: cgroup: Fix exception handling in test_memcg_oom_group_score_events()

[David Vernet]

On Mon, Mar 27, 2023 at 07:56:03AM +0200, Markus Elfring wrote:
> >> 2. Can a cg_destroy() call ever work as expected if a cg_create() call failed?
> >
> > Perhaps next time you can answer your own question by spending 30
> > seconds actually reading the code you're "fixing":
> >
> > int cg_destroy(const char *cgroup)
> > {
> …
> >         ret = rmdir(cgroup);
> …
> >         if (ret && errno == ENOENT) <<< that case is explicitly handled here
> >                 ret = 0;
> >
> >         return ret;
> > }
> 
> Is it interesting somehow that a non-existing directory (which would occasionally
> not be found) is tolerated so far?
> https://elixir.bootlin.com/linux/v6.3-rc3/source/tools/testing/selftests/cgroup/cgroup_util.c#L285
> 
> Should such a function call be avoided because of a failed cg_create() call?

The point is that (a) you were wrong that this is fixing anything, and
(b) this patch is functionally useless. Sure, we could move some goto's
around and subjectively improve "something". Why?  What's the point?
It's highly debatable that what you're doing is even an improvement, and
I'm not interested in wasting time pontificating about the merits of a
trivial "fix" for a test cleanup function that isn't even broken.

Several people have already either advised or directly asked you to stop
sending these patches. I'm not sure why you're choosing to ignore them,
but I'll throw my hat in the ring regardless and do the same. Please
stop sending these fake cleanup patches.

Re: [PATCH resent] drbd: Fix exception handling in nla_put_drbd_cfg_context()

[Christoph Böhmwalder]

Am 25.03.23 um 15:07 schrieb Markus Elfring:
> Date: Fri, 17 Mar 2023 18:32:05 +0100
> 
> The label “nla_put_failure” was used to jump to another pointer check
> despite of the detail in the implementation of the function
> “nla_put_drbd_cfg_context” that it was determined already that
> the corresponding variable contained a null pointer.
> 
> * Thus return directly after a call of the function
>   “nla_nest_start_noflag” failed.
> 
> * Delete an extra pointer check which became unnecessary
>   with this refactoring.
> 
> 
> This issue was detected by using the Coccinelle software.
> 
> Fixes: 543cc10b4cc5c60aa9fcc62705ccfb9998bf4697 ("drbd: drbd_adm_get_status needs to show some more detail")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/block/drbd/drbd_nl.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/block/drbd/drbd_nl.c b/drivers/block/drbd/drbd_nl.c
> index f49f2a5282e1..9cb947127472 100644
> --- a/drivers/block/drbd/drbd_nl.c
> +++ b/drivers/block/drbd/drbd_nl.c
> @@ -3187,7 +3187,7 @@ static int nla_put_drbd_cfg_context(struct sk_buff *skb,
>  	struct nlattr *nla;
>  	nla = nla_nest_start_noflag(skb, DRBD_NLA_CFG_CONTEXT);
>  	if (!nla)
> -		goto nla_put_failure;
> +		return -EMSGSIZE;
>  	if (device &&
>  	    nla_put_u32(skb, T_ctx_volume, device->vnr))
>  		goto nla_put_failure;
> @@ -3205,8 +3205,7 @@ static int nla_put_drbd_cfg_context(struct sk_buff *skb,
>  	return 0;
> 
>  nla_put_failure:
> -	if (nla)
> -		nla_nest_cancel(skb, nla);
> +	nla_nest_cancel(skb, nla);
>  	return -EMSGSIZE;
>  }
> 
> --
> 2.40.0
> 

Sorry, I fail to see how this is an improvement over the status quo,
much less a "fix".

Can you identify the issue with the current code and can you explain how
your patch makes it better?

-- 
Christoph Böhmwalder
LINBIT | Keeping the Digital World Running
DRBD HA —  Disaster Recovery — Software defined Storage

Re: [PATCH resent] perf/x86/amd/uncore: Fix exception handling in amd_uncore_cpu_up_prepare()

[Peter Zijlstra]

On Mon, Mar 27, 2023 at 12:11:54PM +0300, Adrian Hunter wrote:
> On 25/03/23 16:15, Markus Elfring wrote:
> > Date: Fri, 17 Mar 2023 13:13:14 +0100
> > 
> > The label “fail” was used to jump to another pointer check despite of
> > the detail in the implementation of the function “amd_uncore_cpu_up_prepare”
> > that it was determined already that the corresponding variable contained
> > a null pointer (because of a failed function call in two cases).
> > 
> > 1. Thus return directly after a call of the function “amd_uncore_alloc”
> >    failed in the first if branch.
> > 
> > 2. Use more appropriate labels instead.
> > 
> > 3. Reorder jump targets at the end.
> > 
> > 4. Delete a redundant check and kfree() call.
> > 
> > 5. Omit an explicit initialisation for the local variable “uncore_llc”.
> > 
> > 
> > This issue was detected by using the Coccinelle software.
> > 
> > Fixes: 39621c5808f5dda75d03dc4b2d4d2b13a5a1c34b ("perf/x86/amd/uncore: Use dynamic events array")
> > Fixes: 503d3291a937b726757c1f7c45fa02389d2f4324 ("perf/x86/amd: Try to fix some mem allocation failure handling")
> 
> Commit should be only the first 12 characters of the hash.
> Refer:	https://docs.kernel.org/process/submitting-patches.html
> 
> But this is not a fix. Redundant calls to kfree do not break
> anything.
> 
> Also avoid using the term "exception" since, in x86, exceptions are
> hardware events.  Better to just call it "error handling".

Don't feed the trolls; Markus is a bot or other weird construct that's
been banned from lkml.

Re: [PATCH v2] selinux: Adjust implementation of security_get_bools()

[Paul Moore]

On Mon, Mar 27, 2023 at 3:00 AM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> Date: Mon, 27 Mar 2023 08:50:56 +0200
>
> The label “err” was used to jump to another pointer check despite of
> the detail in the implementation of the function “security_get_bools”
> 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. Convert the statement “policydb = &policy->policydb;” into
>    a variable initialisation.
>
> 2. Replace the statement “goto out;” by “return -ENOMEM;”.
>
> 3. Return zero directly at two places.
>
> 4. Omit the variable “rc”.
>
> 5. Use more appropriate labels instead.
>
> 6. Reorder the assignment targets for two kcalloc() calls.
>
> 7. Reorder jump targets at the end.
>
> 8. Assign a value element only after a name assignment succeeded.
>
> 9. Delete an extra pointer check 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/selinux/ss/services.c | 52 ++++++++++++++--------------------
>  1 file changed, 22 insertions(+), 30 deletions(-)

Hmm, for some odd reason I don't see this patch in the SELinux mailing
list archive or the patchwork; replying here without comment (that
will come later) to make sure this hits the SELinux list.

> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index f14d1ffe54c5..702282954bf9 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -2964,53 +2964,45 @@ int security_fs_use(struct super_block *sb)
>  int security_get_bools(struct selinux_policy *policy,
>                        u32 *len, char ***names, int **values)
>  {
> -       struct policydb *policydb;
> +       struct policydb *policydb = &policy->policydb;
>         u32 i;
> -       int rc;
> -
> -       policydb = &policy->policydb;
>
>         *names = NULL;
>         *values = NULL;
> -
> -       rc = 0;
>         *len = policydb->p_bools.nprim;
>         if (!*len)
> -               goto out;
> -
> -       rc = -ENOMEM;
> -       *names = kcalloc(*len, sizeof(char *), GFP_ATOMIC);
> -       if (!*names)
> -               goto err;
> +               return 0;
>
> -       rc = -ENOMEM;
>         *values = kcalloc(*len, sizeof(int), GFP_ATOMIC);
>         if (!*values)
> -               goto err;
> +               goto reset_len;
>
> -       for (i = 0; i < *len; i++) {
> -               (*values)[i] = policydb->bool_val_to_struct[i]->state;
> +       *names = kcalloc(*len, sizeof(char *), GFP_ATOMIC);
> +       if (!*names)
> +               goto free_values;
>
> -               rc = -ENOMEM;
> +       for (i = 0; i < *len; i++) {
>                 (*names)[i] = kstrdup(sym_name(policydb, SYM_BOOLS, i),
>                                       GFP_ATOMIC);
>                 if (!(*names)[i])
> -                       goto err;
> -       }
> -       rc = 0;
> -out:
> -       return rc;
> -err:
> -       if (*names) {
> -               for (i = 0; i < *len; i++)
> -                       kfree((*names)[i]);
> -               kfree(*names);
> +                       goto free_names;
> +
> +               (*values)[i] = policydb->bool_val_to_struct[i]->state;
>         }
> -       kfree(*values);
> -       *len = 0;
> +       return 0;
> +
> +free_names:
> +       for (i = 0; i < *len; i++)
> +               kfree((*names)[i]);
> +
> +       kfree(*names);
>         *names = NULL;
> +free_values:
> +       kfree(*values);
>         *values = NULL;
> -       goto out;
> +reset_len:
> +       *len = 0;
> +       return -ENOMEM;
>  }
>
>
> --
> 2.40.0

-- 
paul-moore.com

Re: [PATCH v2] selinux: Adjust implementation of security_get_bools()

[Paul Moore]

On Mon, Mar 27, 2023 at 5:37 PM Paul Moore <paul@paul-moore.com> wrote:
> On Mon, Mar 27, 2023 at 3:00 AM Markus Elfring <Markus.Elfring@web.de> wrote:
> > Date: Mon, 27 Mar 2023 08:50:56 +0200
> >
> > The label “err” was used to jump to another pointer check despite of
> > the detail in the implementation of the function “security_get_bools”
> > 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. Convert the statement “policydb = &policy->policydb;” into
> >    a variable initialisation.
> >
> > 2. Replace the statement “goto out;” by “return -ENOMEM;”.
> >
> > 3. Return zero directly at two places.
> >
> > 4. Omit the variable “rc”.
> >
> > 5. Use more appropriate labels instead.
> >
> > 6. Reorder the assignment targets for two kcalloc() calls.
> >
> > 7. Reorder jump targets at the end.
> >
> > 8. Assign a value element only after a name assignment succeeded.
> >
> > 9. Delete an extra pointer check 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/selinux/ss/services.c | 52 ++++++++++++++--------------------
> >  1 file changed, 22 insertions(+), 30 deletions(-)
>
> Hmm, for some odd reason I don't see this patch in the SELinux mailing
> list archive or the patchwork; replying here without comment (that
> will come later) to make sure this hits the SELinux list.

For some reason the generated diff below is pretty messy, so I'm just
going to put some of my comments here:

Given the fairly extensive refactoring here, and the frequency with
which @len, @names, and @values are used in the function, I might
simply create local variables for each and only assign them to the
parameter pointers at the end when everything completes successfully
(you could still reset @len at the start if you wanted).  If nothing
else it will make the function easier to read, and I think it will
simplify the code a bit too.

I would probably also keep the combined @names/@values cleanup under
one jump label; this function isn't complicated enough to warrant that
many jump labels for error conditions.

> > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> > index f14d1ffe54c5..702282954bf9 100644
> > --- a/security/selinux/ss/services.c
> > +++ b/security/selinux/ss/services.c
> > @@ -2964,53 +2964,45 @@ int security_fs_use(struct super_block *sb)
> >  int security_get_bools(struct selinux_policy *policy,
> >                        u32 *len, char ***names, int **values)
> >  {
> > -       struct policydb *policydb;
> > +       struct policydb *policydb = &policy->policydb;
> >         u32 i;
> > -       int rc;
> > -
> > -       policydb = &policy->policydb;
> >
> >         *names = NULL;
> >         *values = NULL;
> > -
> > -       rc = 0;
> >         *len = policydb->p_bools.nprim;
> >         if (!*len)
> > -               goto out;
> > -
> > -       rc = -ENOMEM;
> > -       *names = kcalloc(*len, sizeof(char *), GFP_ATOMIC);
> > -       if (!*names)
> > -               goto err;
> > +               return 0;
> >
> > -       rc = -ENOMEM;
> >         *values = kcalloc(*len, sizeof(int), GFP_ATOMIC);
> >         if (!*values)
> > -               goto err;
> > +               goto reset_len;
> >
> > -       for (i = 0; i < *len; i++) {
> > -               (*values)[i] = policydb->bool_val_to_struct[i]->state;
> > +       *names = kcalloc(*len, sizeof(char *), GFP_ATOMIC);
> > +       if (!*names)
> > +               goto free_values;
> >
> > -               rc = -ENOMEM;
> > +       for (i = 0; i < *len; i++) {
> >                 (*names)[i] = kstrdup(sym_name(policydb, SYM_BOOLS, i),
> >                                       GFP_ATOMIC);
> >                 if (!(*names)[i])
> > -                       goto err;
> > -       }
> > -       rc = 0;
> > -out:
> > -       return rc;
> > -err:
> > -       if (*names) {
> > -               for (i = 0; i < *len; i++)
> > -                       kfree((*names)[i]);
> > -               kfree(*names);
> > +                       goto free_names;
> > +
> > +               (*values)[i] = policydb->bool_val_to_struct[i]->state;
> >         }
> > -       kfree(*values);
> > -       *len = 0;
> > +       return 0;
> > +
> > +free_names:
> > +       for (i = 0; i < *len; i++)
> > +               kfree((*names)[i]);
> > +
> > +       kfree(*names);
> >         *names = NULL;
> > +free_values:
> > +       kfree(*values);
> >         *values = NULL;
> > -       goto out;
> > +reset_len:
> > +       *len = 0;
> > +       return -ENOMEM;
> >  }
> >
> >
> > --
> > 2.40.0
>
> --
> paul-moore.com



-- 
paul-moore.com

Re: [PATCH resent] clk: at91: sama7g5: Add two jump labels in sama7g5_pmc_setup()

[Nicolas Ferre]

On 25/03/2023 at 15:05, Markus Elfring wrote:
> Date: Fri, 17 Mar 2023 20:02:34 +0100
> 
> The label “err_free” was used to jump to another pointer check despite of
> the detail in the implementation of the function “sama7g5_pmc_setup”
> that it was determined already that the corresponding variable contained
> a null pointer (because of a failed memory allocation).
> 
> * Thus use additional labels.
> 
> * Delete an extra pointer check at the end which became unnecessary
>    with this refactoring.
> 
> This issue was detected by using the Coccinelle software.

Fine, but I'm sorry that it complexity the function for no real value. 
Other clk drivers have the same pattern so I want them to all stay the same.

This is a NACK, sorry about that.

Regards,
   Nicolas

> Fixes: cb783bbbcf54c36256006895c215e86c5e7266d8 ("clk: at91: sama7g5: add clock support for sama7g5")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>   drivers/clk/at91/sama7g5.c | 23 +++++++++++------------
>   1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/clk/at91/sama7g5.c b/drivers/clk/at91/sama7g5.c
> index f135b662f1ff..224b1f2ebef2 100644
> --- a/drivers/clk/at91/sama7g5.c
> +++ b/drivers/clk/at91/sama7g5.c
> @@ -927,25 +927,25 @@ static void __init sama7g5_pmc_setup(struct device_node *np)
>                              (ARRAY_SIZE(sama7g5_mckx) + ARRAY_SIZE(sama7g5_gck)),
>                              GFP_KERNEL);
>          if (!alloc_mem)
> -               goto err_free;
> +               goto free_pmc;
> 
>          hw = at91_clk_register_main_rc_osc(regmap, "main_rc_osc", 12000000,
>                                             50000000);
>          if (IS_ERR(hw))
> -               goto err_free;
> +               goto free_alloc_mem;
> 
>          bypass = of_property_read_bool(np, "atmel,osc-bypass");
> 
>          hw = at91_clk_register_main_osc(regmap, "main_osc", mainxtal_name,
>                                          bypass);
>          if (IS_ERR(hw))
> -               goto err_free;
> +               goto free_alloc_mem;
> 
>          parent_names[0] = "main_rc_osc";
>          parent_names[1] = "main_osc";
>          hw = at91_clk_register_sam9x5_main(regmap, "mainck", parent_names, 2);
>          if (IS_ERR(hw))
> -               goto err_free;
> +               goto free_alloc_mem;
> 
>          sama7g5_pmc->chws[PMC_MAIN] = hw;
> 
> @@ -987,7 +987,7 @@ static void __init sama7g5_pmc_setup(struct device_node *np)
>                          }
> 
>                          if (IS_ERR(hw))
> -                               goto err_free;
> +                               goto free_alloc_mem;
> 
>                          if (sama7g5_plls[i][j].eid)
>                                  sama7g5_pmc->chws[sama7g5_plls[i][j].eid] = hw;
> @@ -999,7 +999,7 @@ static void __init sama7g5_pmc_setup(struct device_node *np)
>                                            &mck0_layout, &mck0_characteristics,
>                                            &pmc_mck0_lock, CLK_GET_RATE_NOCACHE, 5);
>          if (IS_ERR(hw))
> -               goto err_free;
> +               goto free_alloc_mem;
> 
>          sama7g5_pmc->chws[PMC_MCK] = hw;
> 
> @@ -1128,12 +1128,11 @@ static void __init sama7g5_pmc_setup(struct device_node *np)
>          return;
> 
>   err_free:
> -       if (alloc_mem) {
> -               for (i = 0; i < alloc_mem_size; i++)
> -                       kfree(alloc_mem[i]);
> -               kfree(alloc_mem);
> -       }
> -
> +       for (i = 0; i < alloc_mem_size; i++)
> +               kfree(alloc_mem[i]);
> +free_alloc_mem:
> +       kfree(alloc_mem);
> +free_pmc:
>          kfree(sama7g5_pmc);
>   }
> 
> --
> 2.40.0
> 

-- 
Nicolas Ferre

Re: selinux: Adjust implementation of security_get_bools()

[Paul Moore]

On Tue, Mar 28, 2023 at 3:30 AM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> …
> >>>  security/selinux/ss/services.c | 52 ++++++++++++++--------------------
> …
> > Given the fairly extensive refactoring here,
> …
> > If nothing else it will make the function easier to read,
> > and I think it will simplify the code a bit too.
>
> I am curious which change possibilities will finally be picked up.

It's hard to extract out the various changes due to the way the diff
was generated, however, looking at the changes in your commit
description, the only change I can saw with any certainty that I would
merge would be your item #2:

> 2. Replace the statement “goto out;” by “return -ENOMEM;”.

Agreed, gotos that jump straight to a return can be replaced.

> > I would probably also keep the combined @names/@values cleanup under
> > one jump label; this function isn't complicated enough to warrant that
> > many jump labels for error conditions.
>
> I got an other impression for the affected function implementation.
>
> Would you like to take advice from another information source
> better into account?

In this case, I prefer what I suggested.

-- 
paul-moore.com

Re: [PATCH resent] clk: at91: sama7g5: Add two jump labels in sama7g5_pmc_setup()

[Alexandre Belloni]

On 28/03/2023 21:24:00+0200, Markus Elfring wrote:
> >> The label “err_free” was used to jump to another pointer check despite of
> >> the detail in the implementation of the function “sama7g5_pmc_setup”
> >> that it was determined already that the corresponding variable contained
> >> a null pointer (because of a failed memory allocation).
> >>
> >> * Thus use additional labels.
> >>
> >> * Delete an extra pointer check at the end which became unnecessary
> >>    with this refactoring.
> >>
> >> This issue was detected by using the Coccinelle software.
> >
> > Fine, but I'm sorry that it complexity the function for no real value.
> 
> Under which circumstances can advice be taken better into account
> also from another 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
> 
> 
> 
> > Other clk drivers have the same pattern so I want them to all stay the same.
> > This is a NACK, sorry about that.
> 
> I am curious if other contributors (or code reviewers) would like to influence
> the software situation a bit more.
> 

I agree with Nicolas here, this is useless churn.


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH resent 0/2] md/raid: Adjustments for two function implementations

[Song Liu]

On Sat, Mar 25, 2023 at 2:20 AM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> Date: Mon, 20 Mar 2023 17:28:05 +0100
>
> A few update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (2):
>   raid1: Fix exception handling in setup_conf()
>   raid10: Fix exception handling in setup_conf()

The two functions look good to me as-is. I don't think anything is
broken except that the code analysis tool complains. I don't think
it is necessary to make these changes.

Thanks,
Song

Re: selinux: Adjust implementation of security_get_bools()

[Paul Moore]

On Wed, Mar 29, 2023 at 1:20 AM Markus Elfring <Markus.Elfring@web.de> wrote:
> >> Would you like to take advice from another information source
> >> better into account?
> >
> > In this case, I prefer what I suggested.
>
> What does hinder you to work with more jump labels for improved exception handling?

I'm the one who has the responsibility to fix bugs in the code when no
one else has the time or the desire, I'm also the one who shepherds
these changes up to Linus and argues for contentious changes which are
not popular outside the Linux Kernel security community.  Having to do
this with patches that I find bothersome hinders me in ways which are
sometimes difficult to explain, but easy to understand if you've ever
been responsible for maintaining a significant code base.

-- 
paul-moore.com

Re: [0/2] md/raid: Adjustments for two function implementations

[Song Liu]

On Tue, Mar 28, 2023 at 10:32 PM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> >>   raid1: Fix exception handling in setup_conf()
> >>   raid10: Fix exception handling in setup_conf()
> >
> > The two functions look good to me as-is. I don't think anything is
> > broken except that the code analysis tool complains. I don't think
> > it is necessary to make these changes.
>
> Will development interests ever grow also for advice from another 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

While I understand "goto chain" is a good pattern for error handling, I am
not convinced it is a better approach here. Specifically, the goto chain is so
long that it is hard to maintain. This also adds overhead for users of
git-blame. Therefore, I would rather keep these two functions as-is.

Thanks,
Song

Re: [PATCH resent] cpufreq: sparc: Fix exception handling in two functions

[Viresh Kumar]

On 25-03-23, 15:02, Markus Elfring wrote:
> Date: Sat, 18 Mar 2023 11:40:11 +0100
> 
> The label “err_out” was used to jump to another pointer check despite of
> the detail in the implementation of the functions “us2e_freq_init”
> and “us3_freq_init” that it was determined already that the corresponding
> variable contained a null pointer (because of a failed memory allocation).
> 
> 1. Use additional labels.
> 
> 2. Reorder jump targets at the end.
> 
> 3. Delete an extra pointer check which became unnecessary
>    with this refactoring.
> 
> 
> This issue was detected by using the Coccinelle software.
> 
> Fixes: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac ("Linux-2.6.12-rc2")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/cpufreq/sparc-us2e-cpufreq.c | 12 ++++++------
>  drivers/cpufreq/sparc-us3-cpufreq.c  | 12 ++++++------
>  2 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/cpufreq/sparc-us2e-cpufreq.c b/drivers/cpufreq/sparc-us2e-cpufreq.c
> index 92acbb25abb3..8534d8b1af56 100644
> --- a/drivers/cpufreq/sparc-us2e-cpufreq.c
> +++ b/drivers/cpufreq/sparc-us2e-cpufreq.c
> @@ -324,12 +324,12 @@ static int __init us2e_freq_init(void)
>  		ret = -ENOMEM;
>  		driver = kzalloc(sizeof(*driver), GFP_KERNEL);
>  		if (!driver)
> -			goto err_out;
> +			goto reset_freq_table;

I would just return error from here.

> 
>  		us2e_freq_table = kzalloc((NR_CPUS * sizeof(*us2e_freq_table)),
>  			GFP_KERNEL);
>  		if (!us2e_freq_table)
> -			goto err_out;
> +			goto free_driver;
> 
>  		driver->init = us2e_freq_cpu_init;
>  		driver->verify = cpufreq_generic_frequency_table_verify;
> @@ -346,11 +346,11 @@ static int __init us2e_freq_init(void)
>  		return 0;
> 
>  err_out:
> -		if (driver) {
> -			kfree(driver);
> -			cpufreq_us2e_driver = NULL;
> -		}
>  		kfree(us2e_freq_table);
> +free_driver:
> +		kfree(driver);
> +		cpufreq_us2e_driver = NULL;
> +reset_freq_table:
>  		us2e_freq_table = NULL;

This wasn't set at this point, no point clearing it here. Also this
clearing of global variables isn't required at all, as after this
point no other function shall get called.

similar comments for the other file.

>  		return ret;
>  	}
> diff --git a/drivers/cpufreq/sparc-us3-cpufreq.c b/drivers/cpufreq/sparc-us3-cpufreq.c
> index e41b35b16afd..325f61bb2e40 100644
> --- a/drivers/cpufreq/sparc-us3-cpufreq.c
> +++ b/drivers/cpufreq/sparc-us3-cpufreq.c
> @@ -172,12 +172,12 @@ static int __init us3_freq_init(void)
>  		ret = -ENOMEM;
>  		driver = kzalloc(sizeof(*driver), GFP_KERNEL);
>  		if (!driver)
> -			goto err_out;
> +			goto reset_freq_table;
> 
>  		us3_freq_table = kzalloc((NR_CPUS * sizeof(*us3_freq_table)),
>  			GFP_KERNEL);
>  		if (!us3_freq_table)
> -			goto err_out;
> +			goto free_driver;
> 
>  		driver->init = us3_freq_cpu_init;
>  		driver->verify = cpufreq_generic_frequency_table_verify;
> @@ -194,11 +194,11 @@ static int __init us3_freq_init(void)
>  		return 0;
> 
>  err_out:
> -		if (driver) {
> -			kfree(driver);
> -			cpufreq_us3_driver = NULL;
> -		}
>  		kfree(us3_freq_table);
> +free_driver:
> +		kfree(driver);
> +		cpufreq_us3_driver = NULL;
> +reset_freq_table:
>  		us3_freq_table = NULL;
>  		return ret;
>  	}
> --
> 2.40.0

-- 
viresh

Re: [PATCH] cpufreq: sparc: Fix exception handling in two functions

[Viresh Kumar]

On 03-04-23, 14:50, Markus Elfring wrote:
> >> +++ b/drivers/cpufreq/sparc-us2e-cpufreq.c
> >> @@ -324,12 +324,12 @@ static int __init us2e_freq_init(void)
> >>  		ret = -ENOMEM;
> >>  		driver = kzalloc(sizeof(*driver), GFP_KERNEL);
> >>  		if (!driver)
> >> -			goto err_out;
> >> +			goto reset_freq_table;
> >
> > I would just return error from here.
> 
> I got the impression from this function implementation that a bit of additional
> resource release would be relevant so far (at the end of a corresponding if branch).

That is exactly what you are looking to fix, so lets fix it once and
for all.

-- 
viresh

Re: [PATCH] spi: atmel: Improve exception handling in atmel_spi_configure_dma()

[Nicolas Ferre]

On 07/04/2023 at 08:22, Markus Elfring wrote:
> Date: Fri, 7 Apr 2023 08:08:59 +0200
> 
> The label “error” was used to jump to another pointer check despite of
> the detail in the implementation of the function “atmel_spi_configure_dma”
> that it was determined already that the corresponding variable
> contained an error pointer because of a failed call of
> the function “dma_request_chan”.
> 
> * Thus use more appropriate labels instead.
> 
> * Delete two redundant checks.
> 
> 
> This issue was detected by using the Coccinelle software.
> 
> Fixes: 398b6b310ec85eef9d98df5963d5ded18aa92ad8 ("spi: atmel: switch to use modern name")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

It's becoming a pattern, but still:
NACK.

Regards,
   Nicolas

> ---
>   drivers/spi/spi-atmel.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
> index 7f06305e16cb..ed8dc93c73e5 100644
> --- a/drivers/spi/spi-atmel.c
> +++ b/drivers/spi/spi-atmel.c
> @@ -511,12 +511,12 @@ static int atmel_spi_configure_dma(struct spi_controller *host,
>                   * requested tx channel.
>                   */
>                  dev_dbg(dev, "No RX DMA channel, DMA is disabled\n");
> -               goto error;
> +               goto release_channel_tx;
>          }
> 
>          err = atmel_spi_dma_slave_config(as, 8);
>          if (err)
> -               goto error;
> +               goto release_channel_rx;
> 
>          dev_info(&as->pdev->dev,
>                          "Using %s (tx) and %s (rx) for DMA transfers\n",
> @@ -524,11 +524,11 @@ static int atmel_spi_configure_dma(struct spi_controller *host,
>                          dma_chan_name(host->dma_rx));
> 
>          return 0;
> -error:
> -       if (!IS_ERR(host->dma_rx))
> -               dma_release_channel(host->dma_rx);
> -       if (!IS_ERR(host->dma_tx))
> -               dma_release_channel(host->dma_tx);
> +
> +release_channel_rx:
> +       dma_release_channel(host->dma_rx);
> +release_channel_tx:
> +       dma_release_channel(host->dma_tx);
>   error_clear:
>          host->dma_tx = host->dma_rx = NULL;
>          return err;
> --
> 2.40.0
> 

-- 
Nicolas Ferre

Re: [PATCH 0/2] IB/uverbs: Adjustments for create_qp()

[Leon Romanovsky]

On Thu, Apr 06, 2023 at 06:10:14PM +0200, Markus Elfring wrote:
> Date: Thu, 6 Apr 2023 17:50:05 +0200
> 
> A few update suggestions were taken into account
> from static source code analysis.
> 
> Markus Elfring (2):
>   Improve exception handling
>   Delete a duplicate check

Like I said it before,
NACK to any RDMA patches from you.

To make it very clear, if you send any patches to RDMA, please add the
following line:

Nacked-by Leon Romanovsky <leon@kernel.org>

Thanks

Re: [PATCH v2] cpufreq: sparc: Fix exception handling in two functions

[Viresh Kumar]

On 07-04-23, 19:48, Markus Elfring wrote:
> @@ -337,21 +337,17 @@ static int __init us2e_freq_init(void)
>  		driver->get = us2e_freq_get;
>  		driver->exit = us2e_freq_cpu_exit;
>  		strcpy(driver->name, "UltraSPARC-IIe");
> -
> -		cpufreq_us2e_driver = driver;

This changes the behavior of the code here as "cpufreq_us2e_driver"
is used in us2e_freq_cpu_exit(). If some failure occurs after a
policy is initialized, and driver doesn't register successfully, then
we won't set the frequency to the lowest index of the table anymore.

Same with other file.

-- 
viresh

Re: [PATCH] remoteproc: imx_dsp_rproc: Improve exception handling in imx_dsp_rproc_mbox_alloc()

[Mathieu Poirier]

On Thu, Apr 06, 2023 at 10:12:50PM +0200, Markus Elfring wrote:
> Date: Thu, 6 Apr 2023 22:00:24 +0200
> 
> The label “err_out” was used to jump to another pointer check
> despite of the detail in the implementation of the function
> “imx_dsp_rproc_mbox_alloc” that it was determined already
> that the corresponding variable contained an error pointer
> because of a failed call of the function “mbox_request_channel_byname”.
> 
> Thus perform the following adjustments:
> 
> 1. Return directly after a call of the function
>    “mbox_request_channel_byname” failed for the input parameter “tx”.
> 
> 2. Use more appropriate labels instead.
> 
> 3. Reorder jump targets at the end.
> 
> 4. Omit a function call and three extra checks.
> 
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/remoteproc/imx_dsp_rproc.c | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
>

Applied

Thanks,
Mathieu

> diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
> index 21759d9e5b7b..a8ad15ef1da0 100644
> --- a/drivers/remoteproc/imx_dsp_rproc.c
> +++ b/drivers/remoteproc/imx_dsp_rproc.c
> @@ -530,7 +530,7 @@ static int imx_dsp_rproc_mbox_alloc(struct imx_dsp_rproc *priv)
>  		ret = PTR_ERR(priv->tx_ch);
>  		dev_dbg(cl->dev, "failed to request tx mailbox channel: %d\n",
>  			ret);
> -		goto err_out;
> +		return ret;
>  	}
> 
>  	/* Channel for receiving message */
> @@ -539,7 +539,7 @@ static int imx_dsp_rproc_mbox_alloc(struct imx_dsp_rproc *priv)
>  		ret = PTR_ERR(priv->rx_ch);
>  		dev_dbg(cl->dev, "failed to request rx mailbox channel: %d\n",
>  			ret);
> -		goto err_out;
> +		goto free_channel_tx;
>  	}
> 
>  	cl = &priv->cl_rxdb;
> @@ -555,19 +555,15 @@ static int imx_dsp_rproc_mbox_alloc(struct imx_dsp_rproc *priv)
>  		ret = PTR_ERR(priv->rxdb_ch);
>  		dev_dbg(cl->dev, "failed to request mbox chan rxdb, ret %d\n",
>  			ret);
> -		goto err_out;
> +		goto free_channel_rx;
>  	}
> 
>  	return 0;
> 
> -err_out:
> -	if (!IS_ERR(priv->tx_ch))
> -		mbox_free_channel(priv->tx_ch);
> -	if (!IS_ERR(priv->rx_ch))
> -		mbox_free_channel(priv->rx_ch);
> -	if (!IS_ERR(priv->rxdb_ch))
> -		mbox_free_channel(priv->rxdb_ch);
> -
> +free_channel_rx:
> +	mbox_free_channel(priv->rx_ch);
> +free_channel_tx:
> +	mbox_free_channel(priv->tx_ch);
>  	return ret;
>  }
> 
> --
> 2.40.0
> 

Re: [v2] cpufreq: sparc: Fix exception handling in two functions

[Viresh Kumar]

On 10-04-23, 15:08, Markus Elfring wrote:
> >> @@ -337,21 +337,17 @@ static int __init us2e_freq_init(void)
> >>  		driver->get = us2e_freq_get;
> >>  		driver->exit = us2e_freq_cpu_exit;
> >>  		strcpy(driver->name, "UltraSPARC-IIe");
> >> -
> >> -		cpufreq_us2e_driver = driver;
> >
> > This changes the behavior of the code here as "cpufreq_us2e_driver"
> > is used in us2e_freq_cpu_exit(). If some failure occurs after a
> > policy is initialized, and driver doesn't register successfully, then
> > we won't set the frequency to the lowest index of the table anymore.
> 
> The setting of the variables “cpufreq_us…_driver” influences the need
> to reset them to null pointers for the desired exception handling,
> doesn't it?

This is what all should be done for these drivers I guess. There is no
points doing the dance of {de}allocating resources unnecessarily.

diff --git a/drivers/cpufreq/sparc-us2e-cpufreq.c b/drivers/cpufreq/sparc-us2e-cpufreq.c
index 92acbb25abb3..b31fb07f3f39 100644
--- a/drivers/cpufreq/sparc-us2e-cpufreq.c
+++ b/drivers/cpufreq/sparc-us2e-cpufreq.c
@@ -20,7 +20,14 @@
 #include <asm/asi.h>
 #include <asm/timer.h>

-static struct cpufreq_driver *cpufreq_us2e_driver;
+static struct cpufreq_driver cpufreq_us2e_driver = {
+       .name = "UltraSPARC-IIe",
+       .init = us2e_freq_cpu_init,
+       .verify = cpufreq_generic_frequency_table_verify,
+       .target_index = us2e_freq_target,
+       .get = us2e_freq_get,
+       .exit = us2e_freq_cpu_exit,
+};

 struct us2e_freq_percpu_info {
        struct cpufreq_frequency_table table[6];
@@ -300,9 +307,7 @@ static int __init us2e_freq_cpu_init(struct cpufreq_policy *policy)

 static int us2e_freq_cpu_exit(struct cpufreq_policy *policy)
 {
-       if (cpufreq_us2e_driver)
-               us2e_freq_target(policy, 0);
-
+       us2e_freq_target(policy, 0);
        return 0;
 }

@@ -319,39 +324,15 @@ static int __init us2e_freq_init(void)
        impl  = ((ver >> 32) & 0xffff);

        if (manuf == 0x17 && impl == 0x13) {
-               struct cpufreq_driver *driver;
-
-               ret = -ENOMEM;
-               driver = kzalloc(sizeof(*driver), GFP_KERNEL);
-               if (!driver)
-                       goto err_out;
-
                us2e_freq_table = kzalloc((NR_CPUS * sizeof(*us2e_freq_table)),
                        GFP_KERNEL);
                if (!us2e_freq_table)
-                       goto err_out;
+                       return -ENOMEM;

-               driver->init = us2e_freq_cpu_init;
-               driver->verify = cpufreq_generic_frequency_table_verify;
-               driver->target_index = us2e_freq_target;
-               driver->get = us2e_freq_get;
-               driver->exit = us2e_freq_cpu_exit;
-               strcpy(driver->name, "UltraSPARC-IIe");
-
-               cpufreq_us2e_driver = driver;
                ret = cpufreq_register_driver(driver);
                if (ret)
-                       goto err_out;
-
-               return 0;
+                       kfree(us2e_freq_table);

-err_out:
-               if (driver) {
-                       kfree(driver);
-                       cpufreq_us2e_driver = NULL;
-               }
-               kfree(us2e_freq_table);
-               us2e_freq_table = NULL;
                return ret;
        }

@@ -360,13 +341,8 @@ static int __init us2e_freq_init(void)

 static void __exit us2e_freq_exit(void)
 {
-       if (cpufreq_us2e_driver) {
-               cpufreq_unregister_driver(cpufreq_us2e_driver);
-               kfree(cpufreq_us2e_driver);
-               cpufreq_us2e_driver = NULL;
-               kfree(us2e_freq_table);
-               us2e_freq_table = NULL;
-       }
+       cpufreq_unregister_driver(cpufreq_us2e_driver);
+       kfree(us2e_freq_table);
 }

 MODULE_AUTHOR("David S. Miller <davem@redhat.com>");

-- 
viresh

Re: [v2] cpufreq: sparc: Fix exception handling in two functions

[Viresh Kumar]

On 11-04-23, 08:15, Markus Elfring wrote:
> >> The setting of the variables “cpufreq_us…_driver” influences the need
> >> to reset them to null pointers for the desired exception handling,
> >> doesn't it?
> >
> > This is what all should be done for these drivers I guess. There is no
> > points doing the dance of {de}allocating resources unnecessarily.
> 
> Are you going to integrate your source code adjustment according to
> reduced dynamic memory allocation?

You can prepare and send a patch for this if you want, else I will do
it.

-- 
viresh

Re: [cocci] [PATCH] firmware: ti_sci: Fix exception handling in ti_sci_probe()

[Nishanth Menon]

On 22:10-20230405, Markus Elfring wrote:
> Date: Wed, 5 Apr 2023 22:00:18 +0200

B4 does'nt pick this patch up cleanly. And for some reason, I get
mangled patch from public-inbox as well :( a clean git-send-email might
help.

> 
> The label “out” was used to jump to another pointer check despite of

Please use " for quotes.

> the detail in the implementation of the function “ti_sci_probe”
> that it was determined already that the corresponding variable
> contained an error pointer because of a failed call of
> the function “mbox_request_channel_byname”.

> 
> * Thus use more appropriate labels instead.
> 
> * Delete two redundant checks.
> 

How about this:

Optimize out the redundant pointer check in exit path of "out" using
appropriate labels to jump in the error path
> 
Drop the extra EoL

> This issue was detected by using the Coccinelle software.

Curious: what rule of coccicheck caught this?

> 
> Fixes: aa276781a64a5f15ecc21e920960c5b1f84e5fee ("firmware: Add basic support for TI System Control Interface (TI-SCI) protocol")

12 char sha please. Please read Documentation/process/submitting-patches.rst

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/firmware/ti_sci.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c
> index 039d92a595ec..77012d2f4160 100644
> --- a/drivers/firmware/ti_sci.c
turns out as =2D-- instead of -- (might check the git format-patch
output closer).

> +++ b/drivers/firmware/ti_sci.c
> @@ -3433,18 +3433,18 @@ static int ti_sci_probe(struct platform_device *pdev)
>  	info->chan_rx = mbox_request_channel_byname(cl, "rx");
>  	if (IS_ERR(info->chan_rx)) {
>  		ret = PTR_ERR(info->chan_rx);
> -		goto out;
> +		goto remove_debugfs;
>  	}
> 
>  	info->chan_tx = mbox_request_channel_byname(cl, "tx");
>  	if (IS_ERR(info->chan_tx)) {
>  		ret = PTR_ERR(info->chan_tx);
> -		goto out;
> +		goto free_mbox_channel_rx;
>  	}
>  	ret = ti_sci_cmd_get_revision(info);
>  	if (ret) {
>  		dev_err(dev, "Unable to communicate with TISCI(%d)\n", ret);
> -		goto out;
> +		goto free_mbox_channel_tx;
>  	}
> 
>  	ti_sci_setup_ops(info);
> @@ -3456,7 +3456,7 @@ static int ti_sci_probe(struct platform_device *pdev)
>  		ret = register_restart_handler(&info->nb);
>  		if (ret) {
>  			dev_err(dev, "reboot registration fail(%d)\n", ret);
> -			goto out;
> +			goto free_mbox_channel_tx;
>  		}
>  	}
> 
> @@ -3470,11 +3470,12 @@ static int ti_sci_probe(struct platform_device *pdev)
>  	mutex_unlock(&ti_sci_list_mutex);
> 
>  	return of_platform_populate(dev->of_node, NULL, NULL, dev);
> -out:
> -	if (!IS_ERR(info->chan_tx))
> -		mbox_free_channel(info->chan_tx);
> -	if (!IS_ERR(info->chan_rx))
> -		mbox_free_channel(info->chan_rx);
> +
> +free_mbox_channel_tx:
> +	mbox_free_channel(info->chan_tx);
> +free_mbox_channel_rx:
> +	mbox_free_channel(info->chan_rx);
> +remove_debugfs:
>  	debugfs_remove(info->d);
>  	return ret;
>  }
> --
> 2.40.0
> 

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [cocci] [PATCH] firmware: ti_sci: Fix exception handling in ti_sci_probe()

[Dan Carpenter]

On Tue, May 16, 2023 at 10:20:43AM -0500, Nishanth Menon wrote:
> On 22:10-20230405, Markus Elfring wrote:
> > Date: Wed, 5 Apr 2023 22:00:18 +0200
> 
> B4 does'nt pick this patch up cleanly. And for some reason, I get
> mangled patch from public-inbox as well :( a clean git-send-email might
> help.
> 

It's an awkward thing.  B4 doesn't work because Markus was banned from
LKML because he doesn't listen to feedback.

> > 
> > The label “out” was used to jump to another pointer check despite of
> 
> Please use " for quotes.
> 
> > the detail in the implementation of the function “ti_sci_probe”
> > that it was determined already that the corresponding variable
> > contained an error pointer because of a failed call of
> > the function “mbox_request_channel_byname”.
> 
> > 
> > * Thus use more appropriate labels instead.
> > 
> > * Delete two redundant checks.
> > 
> 
> How about this:
> 
> Optimize out the redundant pointer check in exit path of "out" using
> appropriate labels to jump in the error path
> > 
> Drop the extra EoL
> 
> > This issue was detected by using the Coccinelle software.
> 
> Curious: what rule of coccicheck caught this?
> 
> > 
> > Fixes: aa276781a64a5f15ecc21e920960c5b1f84e5fee ("firmware: Add basic support for TI System Control Interface (TI-SCI) protocol")
> 
> 12 char sha please. Please read Documentation/process/submitting-patches.rst
> 

For example, Markus has been told to use 12 char shas several times
before.

> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > ---
> >  drivers/firmware/ti_sci.c | 19 ++++++++++---------
> >  1 file changed, 10 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c
> > index 039d92a595ec..77012d2f4160 100644
> > --- a/drivers/firmware/ti_sci.c
> turns out as =2D-- instead of -- (might check the git format-patch
> output closer).
> 
> > +++ b/drivers/firmware/ti_sci.c
> > @@ -3433,18 +3433,18 @@ static int ti_sci_probe(struct platform_device *pdev)
> >  	info->chan_rx = mbox_request_channel_byname(cl, "rx");
> >  	if (IS_ERR(info->chan_rx)) {
> >  		ret = PTR_ERR(info->chan_rx);
> > -		goto out;
> > +		goto remove_debugfs;
> >  	}
> > 
> >  	info->chan_tx = mbox_request_channel_byname(cl, "tx");
> >  	if (IS_ERR(info->chan_tx)) {
> >  		ret = PTR_ERR(info->chan_tx);
> > -		goto out;
> > +		goto free_mbox_channel_rx;
> >  	}
> >  	ret = ti_sci_cmd_get_revision(info);
> >  	if (ret) {
> >  		dev_err(dev, "Unable to communicate with TISCI(%d)\n", ret);
> > -		goto out;
> > +		goto free_mbox_channel_tx;
> >  	}
> > 
> >  	ti_sci_setup_ops(info);
> > @@ -3456,7 +3456,7 @@ static int ti_sci_probe(struct platform_device *pdev)
> >  		ret = register_restart_handler(&info->nb);
> >  		if (ret) {
> >  			dev_err(dev, "reboot registration fail(%d)\n", ret);
> > -			goto out;
> > +			goto free_mbox_channel_tx;
> >  		}
> >  	}
> > 
> > @@ -3470,11 +3470,12 @@ static int ti_sci_probe(struct platform_device *pdev)
> >  	mutex_unlock(&ti_sci_list_mutex);
> > 
> >  	return of_platform_populate(dev->of_node, NULL, NULL, dev);

There is a bug here because the resources are not freed if
of_platform_populate() fails.  The "info" struct is devm_ allocated but
it's still on the &ti_sci_list list, so that will lead to a use after
free.

regards,
dan carpenter

> > -out:
> > -	if (!IS_ERR(info->chan_tx))
> > -		mbox_free_channel(info->chan_tx);
> > -	if (!IS_ERR(info->chan_rx))
> > -		mbox_free_channel(info->chan_rx);
> > +
> > +free_mbox_channel_tx:
> > +	mbox_free_channel(info->chan_tx);
> > +free_mbox_channel_rx:
> > +	mbox_free_channel(info->chan_rx);
> > +remove_debugfs:
> >  	debugfs_remove(info->d);
> >  	return ret;
> >  }

Re: [PATCH resent v2 0/2] powerpc/pseries: Fixes for exception handling in pSeries_reconfig_add_node()

[Markus Elfring]

> Date: Tue, 21 Mar 2023 11:26:32 +0100
>
> A few update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (2):
>   Do not pass an error pointer to of_node_put()
>   Fix exception handling
>
>  arch/powerpc/platforms/pseries/reconfig.c | 26 ++++++++++++-----------
>  1 file changed, 14 insertions(+), 12 deletions(-)

Is this patch series still in review queues?

Regards,
Markus

Re: [PATCH v2 0/4] powerpc/4xx: Adjustments for four function implementations

[Markus Elfring]

> Date: Sat, 25 Mar 2023 16:10:23 +0100
>
> Some update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (4):
>   Fix exception handling in ppc4xx_pciex_port_setup_hose()
>   Fix exception handling in ppc4xx_probe_pcix_bridge()
>   Fix exception handling in ppc4xx_probe_pci_bridge()
>   Delete unnecessary variable initialisations in four functions
>
>  arch/powerpc/platforms/4xx/pci.c | 69 ++++++++++++++------------------
>  1 file changed, 31 insertions(+), 38 deletions(-)

Is this patch series still in review queues?

Regards,
Markus