Re: [PATCH v16 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 3 potential issue(s) to consider:

New issues:
- [Critical] Use-After-Free of maple tree nodes due to lockless traversal without RCU protection.
- [Medium] Mixing goto-based error handling with guard() in the same function.

Pre-existing issues:
- [High] Spinlock deadlock due to calling `mtree_erase` from NMI/interrupt context.
--

commit dd76a8e2b70424efa3c49c5cac126edfd3a77425
Author: Jens Remus <jremus@linux.ibm.com>

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 7f439600b0f0a..db88d993dff1f 100644
> --- a/kernel/unwind/sframe.c
> +++ b/kernel/unwind/sframe.c

[ ... ]

> @@ -875,6 +875,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) {

[Severity: Critical]
Does this lockless iteration risk a use-after-free on maple tree nodes?

The maple tree is initialized via INIT_MM_SFRAME as MTREE_INIT(sframe_mt,
0), which omits the MT_FLAGS_USE_RCU flag. Without this flag, tree
modifications free internal tree nodes synchronously via kfree() instead
of call_rcu().

If sframe_remove_section() modifies the tree concurrently, could this
lockless mt_for_each() access freed maple tree nodes?

[Severity: High]
This isn't a bug introduced by this patch, but looking at how the maple
tree is used here and in sframe_remove_section(), is there a deadlock
risk if sframe_remove_section() is called from NMI/interrupt context?

Since stack unwinding can occur asynchronously in NMI or hardirq context
(e.g., via perf_callchain_user), if it detects a corrupted section and
triggers sframe_remove_section() which ultimately calls mtree_erase(),
that will take a non-IRQ-safe spin_lock via mtree_lock(mt).

If an NMI interrupts a thread that is already holding the mtree_lock
(for instance, during sframe_add_section), could this instantly
hard-deadlock?

> +		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;
> +	}
> +
> +	return 0;
> +
> +err_free:

[Severity: Medium]
Are we mixing goto-based error handling and scope-based cleanups in the
same function?

This function declares guard(srcu)(&sframe_srcu) at the top, but then
uses a goto-based cleanup label here. According to the cleanup subsystem
guidelines in include/linux/cleanup.h, mixing goto and scope-based
cleanup helpers in the same function creates confusing ownership
semantics.

Should this function either convert all resources to scope-based cleanup,
or convert none of them?

> +	free_section(sec);
> +	return ret;
> +}

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