Re: [PATCH v16 03/20] unwind_user/sframe: Store .sframe section data in per-mm maple tree

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Concurrent `sframe_remove_section` calls lead to a TOCTOU race causing SRCU double-queueing and memory leaks.
- [High] The `sframe_mt` maple tree is initialized without `MT_FLAGS_USE_RCU`, causing use-after-free during lockless traversal.
- [High] `sframe_free_mm()` bypasses SRCU and synchronously frees objects, racing with concurrent SRCU readers.
- [Low] Mixing `scoped_guard()` with goto-based error handling violates subsystem cleanup rules.
- [Low] `current_has_sframe()` dereferences `current->mm` without including `<linux/sched.h>`, causing incomplete type errors.
--

commit 4b0aee91d63748e3221127b74ad789be7e3d2c6b
Author: Josh Poimboeuf <jpoimboe@kernel.org>
Subject: unwind_user/sframe: Store .sframe section data in per-mm maple tree

Associate an .sframe section with its mm by adding it to a per-mm maple
tree which is indexed by the corresponding text address range.  A single
.sframe section can be associated with multiple text ranges.

> diff --git a/include/linux/sframe.h b/include/linux/sframe.h
> index 0642595534f95..38047760e252e 100644
> --- a/include/linux/sframe.h
> +++ b/include/linux/sframe.h
> @@ -19,18 +24,31 @@ struct sframe_section {
>  	signed char	fp_off;
>  };
>  
> +#define INIT_MM_SFRAME .sframe_mt = MTREE_INIT(sframe_mt, 0),

[Severity: High]
Does this maple tree initialization need the MT_FLAGS_USE_RCU flag?

Since sframe_remove_section() traverses the tree locklessly using
mt_for_each(), omitting the RCU flag means internal tree nodes split or
merged by concurrent updates could be freed immediately via
kmem_cache_free(), potentially causing a use-after-free for the
lockless readers.

> +extern void sframe_free_mm(struct mm_struct *mm);
> +
>  extern int sframe_add_section(unsigned long sframe_start, unsigned long sframe_end,
>  			      unsigned long text_start, unsigned long text_end);
>  extern int sframe_remove_section(unsigned long sframe_addr);
>  
> +static inline bool current_has_sframe(void)
> +{
> +	struct mm_struct *mm = current->mm;
> +
> +	return mm && !mtree_empty(&mm->sframe_mt);
> +}

[Severity: Low]
Will this compile cleanly in all contexts?

The header dereferences current->mm, but <linux/sched.h> doesn't appear
to be included here. Files including <linux/sframe.h> without also
including sched.h might fail to compile with incomplete type errors.

> diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c
> index d24e9d4f8bef7..46dba3cb016df 100644
> --- a/kernel/unwind/sframe.c
> +++ b/kernel/unwind/sframe.c
[ ... ]
> @@ -122,15 +125,74 @@ int sframe_add_section(unsigned long sframe_start, unsigned long sframe_end,
>  	if (ret)
>  		goto err_free;
>  
> -	/* TODO nowhere to store it yet - just free it and return an error */
> -	ret = -ENOSYS;
> +	ret = mtree_insert_range(sframe_mt, sec->text_start, sec->text_end - 1,
> +				 sec, GFP_KERNEL_ACCOUNT);
> +	if (ret) {
> +		dbg("mtree_insert_range failed: text=%lx-%lx\n",
> +		    sec->text_start, sec->text_end);
> +		goto err_free;
> +	}
> +
> +	return 0;
>  
>  err_free:
>  	free_section(sec);
>  	return ret;
>  }

[Severity: Low]
This isn't a bug, but does mixing goto-based error handling (err_free)
with the scoped_guard() used earlier in this function violate the
cleanup subsystem guidelines?

The standard suggests managing the sec pointer via __free(kfree) instead
of using goto labels to avoid confusing ownership semantics.

> +static int __sframe_remove_section(struct mm_struct *mm,
> +				   struct sframe_section *sec)
> +{
> +	if (!mtree_erase(&mm->sframe_mt, sec->text_start)) {
> +		dbg("mtree_erase failed: text=%lx\n", sec->text_start);
> +		return -EINVAL;
> +	}
> +
> +	call_srcu(&sframe_srcu, &sec->rcu, sframe_free_srcu);
> +
> +	return 0;
> +}

[Severity: High]
Could this lockless tree access result in corrupting the SRCU callback
list and leaking memory?

If a concurrent thread inserts a new section at the same text_start
address before mtree_erase() is called, mtree_erase() will successfully
remove and return the new section.

Because the return value of mtree_erase() is not checked against sec,
the code proceeds to call call_srcu() on the original sec pointer. This
could double-queue the old section's RCU head and permanently leak the
newly inserted section.

[ ... ]
> +void sframe_free_mm(struct mm_struct *mm)
> +{
> +	struct sframe_section *sec;
> +	unsigned long index = 0;
> +
> +	if (!mm)
> +		return;
> +
> +	mt_for_each(&mm->sframe_mt, sec, index, ULONG_MAX)
> +		free_section(sec);
> +
> +	mtree_destroy(&mm->sframe_mt);
>  }

[Severity: High]
Does bypassing the SRCU grace period here cause a use-after-free for
concurrent readers?

If an unwinder accesses a section under sframe_srcu and then drops its
mm reference, triggering teardown, the synchronous free_section()
(which calls kfree directly) will free the section while the unwinder
is still relying on SRCU to keep it alive. Should this teardown path
use call_srcu() as well?

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