Re: [PATCH v15 18/20] unwind_user/sframe: Duplicate registered .sframe section data on clone/fork

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Mixing goto-based error handling with guard() scope-based cleanup in sframe_dup_mm().
- [Low] Redundant #ifdef blocks around sframe functions in dup_mmap().
--

commit 06e12a491c0dc4cc93fac24f7de9fe0b02b9a9e0
Author: Jens Remus <jremus@linux.ibm.com>
Subject: unwind_user/sframe: Duplicate registered .sframe section data on clone/fork

When duplicating a process' virtual memory mappings also duplicate all
of its registered .sframe sections stored in the per-mm maple tree to
enable stacktracing using sframe of the child process.

> diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c
> index ec8318977a2ef..0661090dd5bcd 100644
> --- a/kernel/unwind/sframe.c
> +++ b/kernel/unwind/sframe.c
[ ... ]
> @@ -886,6 +886,54 @@ int sframe_remove_section(unsigned long sframe_start)
[ ... ]
> +int sframe_dup_mm(struct mm_struct *mm, struct mm_struct *oldmm)
> +{
> +	struct sframe_section *sec, *oldsec;
> +	unsigned long index = 0;
> +	int ret;
> +
> +	guard(srcu)(&sframe_srcu);
> +
> +	mt_for_each(&oldmm->sframe_mt, oldsec, index, ULONG_MAX) {
> +		sec = kzalloc(sizeof(*sec), GFP_KERNEL_ACCOUNT);
> +		if (!sec)
> +			return -ENOMEM;
> +
> +		__sframe_dup_section(sec, oldsec);
> +
> +		ret = mtree_insert_range(&mm->sframe_mt,
> +					 sec->text_start,
> +					 sec->text_end - 1,
> +					 sec, GFP_KERNEL_ACCOUNT);
> +		if (ret)
> +			goto err_free;

[Severity: Medium]
Is it intentional to mix goto-based error handling with scope-based cleanup
in the same function?

The cleanup subsystem guidelines recommend against mixing goto labels and
guard declarations to prevent confusing ownership semantics and lifecycle
bugs. Would it be better to drop the err_free label entirely and explicitly
call free_section(sec) followed by returning ret directly inside this block?

> +	}
> +
> +	return 0;
> +
> +err_free:
> +	free_section(sec);
> +	return ret;
> +}
[ ... ]
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 2311ae7c2ff45..d5aa8c54f8c1c 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
[ ... ]
> @@ -1846,6 +1847,11 @@ __latent_entropy int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm)
>  	}
>  	/* a new mm has just been created */
>  	retval = arch_dup_mmap(oldmm, mm);
> +#ifdef CONFIG_HAVE_UNWIND_USER_SFRAME
> +	if (!retval) {
> +		retval = sframe_dup_mm(mm, oldmm);
> +	}
> +#endif

[Severity: Low]
This isn't a bug, but are these preprocessor guards necessary?

Since include/linux/sframe.h already provides static inline dummy versions
of sframe_dup_mm() and sframe_free_mm() when CONFIG_HAVE_UNWIND_USER_SFRAME
is disabled, these conditional blocks could potentially be removed to keep
the C source code cleaner.

>  loop_out:
>  	vma_iter_free(&vmi);
>  	if (!retval) {
[ ... ]
> @@ -1893,6 +1899,9 @@ __latent_entropy int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm)
>  			vm_unacct_memory(charge);
>  		}
>  		__mt_destroy(&mm->mm_mt);
> +#ifdef CONFIG_HAVE_UNWIND_USER_SFRAME
> +		sframe_free_mm(mm);
> +#endif

[Severity: Low]
This isn't a bug, but similarly, could this block be dropped since the dummy
stub handles the disabled case without needing explicit conditionals?

>  		/*
>  		 * The mm_struct is going to exit, but the locks will be dropped
>  		 * first.  Set the mm_struct as unstable is advisable as it is

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520154004.3845823-1-jremus@linux.ibm.com?part=18