Re: [PATCH v15 11/20] unwind_user/sframe: Show file name in debug output

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] Incorrect use of `vma->vm_file` instead of `!vma_is_anonymous(vma)` to classify a VMA as file-backed.

Pre-existing issues:
- [Critical] Calling `sframe_remove_section` from `sframe_find` acquires a spinlock in NMI context, leading to deadlocks, and triggers a user-controllable WARN.
--

commit 60b05617f9a9c00f287a96dda25f8e4086c6178b
Author: Josh Poimboeuf <jpoimboe@kernel.org>

unwind_user/sframe: Show file name in debug output

This commit prefixes debug output strings with the file name to make sframe
debugging easier. It looks up the VMA corresponding to the sframe section
and extracts the filename based on the VMA properties.

> diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c
> --- a/kernel/unwind/sframe.c
> +++ b/kernel/unwind/sframe.c
[ ... ]
> @@ -374,14 +374,17 @@ int sframe_find(unsigned long ip, struct unwind_user_frame *frame)
>  	user_read_access_end();
>  
>  end:
> -	if (ret && ret != -ENOENT)
> +	if (ret && ret != -ENOENT) {
> +		dbg_sec("removing bad .sframe section\n");
>  		WARN_ON_ONCE(sframe_remove_section(sec->sframe_start));
> +	}

[Severity: Critical]
This is a pre-existing issue, but can this lead to deadlocks in NMI context?

sframe_find() is invoked for user stack unwinding from contexts like perf,
which executes from NMI or hardirq contexts. sframe_remove_section() calls
mtree_erase(), which acquires the maple tree's non-IRQ-safe mtree_lock.

Acquiring a regular spinlock in NMI context can cause a hard CPU deadlock
if the lock was already held by the interrupted thread.

Also, could an unprivileged user trivially trigger this WARN_ON_ONCE by
unmapping the section, potentially crashing the system if panic_on_warn
is enabled?

[ ... ]
> diff --git a/kernel/unwind/sframe_debug.h b/kernel/unwind/sframe_debug.h
> --- a/kernel/unwind/sframe_debug.h
> +++ b/kernel/unwind/sframe_debug.h
[ ... ]
> +static inline void dbg_init(struct sframe_section *sec)
> +{
> +	struct mm_struct *mm = current->mm;
> +	struct vm_area_struct *vma;
> +
> +	guard(mmap_read_lock)(mm);
> +	vma = vma_lookup(mm, sec->sframe_start);
> +	if (!vma)
> +		sec->filename = kstrdup("(vma gone???)", GFP_KERNEL_ACCOUNT);
> +	else if (vma->vm_file)
> +		sec->filename = kstrdup_quotable_file(vma->vm_file, GFP_KERNEL_ACCOUNT);

[Severity: Low]
Does this code accurately classify file-backed VMAs?

Anonymous mappings, such as private mappings of /dev/zero, can also have
vm_file populated. Relying only on vm_file might misclassify these mappings
for the debug output.

Would it be more reliable to use !vma_is_anonymous(vma) to check if the
VMA is file-backed before accessing vm_file?

> +	else if (vma->vm_ops && vma->vm_ops->name)
> +		sec->filename = kstrdup(vma->vm_ops->name(vma), GFP_KERNEL_ACCOUNT);
[ ... ]

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