Re: [PATCH v14 02/19] unwind_user/sframe: Add support for reading .sframe headers

[Jens Remus <jremus@linux.ibm.com>]

On 5/6/2026 3:42 PM, Jens Remus wrote:
> On 5/5/2026 2:49 PM, sashiko-bot@kernel.org wrote:

>> [ ... ]
>>> +	sec = kzalloc(sizeof(*sec), GFP_KERNEL);
>>> +	if (!sec)
>>> +		return -ENOMEM;
>>> +
>>> +	sec->sframe_start	= sframe_start;
>>> +	sec->sframe_end		= sframe_end;
>>> +	sec->text_start		= text_start;
>>> +	sec->text_end		= text_end;
>>> +
>>> +	ret = sframe_read_header(sec);
>>> +	if (ret)
>>> +		goto err_free;
>>
>> Is it acceptable to mix goto error handling with scoped_guard() cleanup
>> helpers in the same function?
>>
>> The kernel's cleanup subsystem guidelines generally suggest that usage of
>> goto and cleanup helpers shouldn't be mixed in the same routine.
> 
> This references to the following comment in include/linux/cleanup.h:
> 
>   Lastly, given that the benefit of cleanup helpers is removal of
>   "goto", and that the "goto" statement can jump between scopes, the
>   expectation is that usage of "goto" and cleanup helpers is never
>   mixed in the same function. I.e. for a given routine, convert all
>   resources that need a "goto" cleanup to scope-based cleanup, or
>   convert none of them.
> 
>>
>> Could sec be allocated using __free(kfree) to avoid the goto entirely?
> 
> The goto error handling is used outside the scoped_guard().  So I think
> it is fine.

Let's do that.

> Do you see any value in converting it as follows (my naive attempt):
> 
> diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c
> @@ -722,13 +722,14 @@ static int sframe_read_header(struct sframe_section *sec)
>         return 0;
>  }
> 
> +DEFINE_FREE(free_section, struct sframe_section *, if (_T) free_section(_T))
>  int sframe_add_section(unsigned long sframe_start, unsigned long sframe_end,
>                        unsigned long text_start, unsigned long text_end)
>  {
>         struct maple_tree *sframe_mt = &current->mm->sframe_mt;
>         struct vm_area_struct *sframe_vma, *text_vma;
>         struct mm_struct *mm = current->mm;
> -       struct sframe_section *sec;
> +       struct sframe_section *sec __free(free_section) = NULL;

Obviously this whole approach does not work, as it causes the section
to get freed once set goes out of scope.  Just learned the hard way,
that I had not thought this through well enough...

Regards,
Jens
-- 
Jens Remus
Linux on Z Development (D3303)
jremus@de.ibm.com / jremus@linux.ibm.com

IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft: Ehningen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/