Re: [PATCH v16 05/20] unwind_user/sframe: Add support for reading .sframe contents

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Mixing unsafe_copy_from_user() and unsafe_get_user() inside a user_access_begin() block breaks user access state (PAN) on architectures like ARM64.
- [Low] Mixing goto-based cleanup with scoped_guard() in sframe_add_section() violates the kernel's cleanup subsystem rules.
--

Note: The standard inline-quoting format has been altered to a free-form
summary due to text recitation restrictions. Code snippets from the
original patch are omitted.

commit b0f7b5132f15107b2df84ba67a33ced97fd3372a
Author: Josh Poimboeuf <jpoimboe@kernel.org>
Subject: unwind_user/sframe: Add support for reading .sframe contents

This commit adds an sframe_find() interface to support unwinding user
space stacks using sframe information. It implements performant user space
access using user_read_access_begin() and unsafe accessors.

Link: https://lore.kernel.org/all/77c0d1ec143bf2a53d66c4ecb190e7e0a576fbfd.1737511963.git.jpoimboe@kernel.org/
Link: https://lore.kernel.org/all/b35ca3a3-8de5-4d32-8d30-d4e562f6b0de@linux.ibm.com/

[Severity: High]
In sframe_find(), scoped_user_read_access_size() opens a user access block.
Inside this block, __find_fde() uses unsafe_copy_from_user(), followed by
__find_fre() using unsafe_get_user() via UNSAFE_GET_USER_INC().

Does mixing these accessors risk breaking the user access state (PAN) on
architectures like ARM64?

On platforms that do not explicitly override unsafe_copy_from_user(), it
falls back to calling raw_copy_from_user(). With CONFIG_ARM64_SW_TTBR0_PAN
enabled, raw_copy_from_user() explicitly enables and then disables TTBR0,
re-enabling PAN during the copy. When it returns, user access is prematurely
disabled for the rest of the block.

Could the subsequent unsafe_get_user() in __find_fre() then trigger a fatal
page fault and kernel panic because TTBR0 is unmapped?

[Severity: Low]
In sframe_add_section(), scoped_guard(mmap_read_lock, mm) is used for
scope-based locking, but later the function uses goto err_free; to clean up
the allocated sec object on error paths.

This isn't a bug, but does this violate the cleanup subsystem guidelines?

The documentation for the cleanup subsystem states that functions should
convert all resources that need a goto cleanup to scope-based cleanup, or
convert none of them, specifically recommending against mixing goto-based
error handling and guard declarations in the same routine.

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