Re: [PATCH v15 13/20] unwind_user: Enable archs that pass RA in a register

[sashiko-bot@kernel.org]

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

New issues:
- [Low] The header file `<linux/unwind_user.h>` uses `pr_debug_once` and `current` without including the necessary headers.

Pre-existing issues:
- [High] The SFrame FRE size calculation computes an incorrect length because `dataword_count` is consumed before it is used in the size formula.
--

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

unwind_user: Enable archs that pass RA in a register

This commit updates the user-space unwinder to support architectures that do
not mandate saving the return address (RA) on the stack, such as s390 and
arm64. It treats an RA offset of zero as an indication that the RA is in the
link register for the topmost frame, and allows the stack pointer to remain
unchanged in the topmost frame.

> diff --git a/include/linux/unwind_user.h b/include/linux/unwind_user.h
> index 64618618febde..7bf58f23aa64a 100644
> --- a/include/linux/unwind_user.h
> +++ b/include/linux/unwind_user.h
> @@ -23,6 +23,16 @@ static inline bool unwind_user_at_function_start(struct pt_regs *regs)
>  #define unwind_user_at_function_start unwind_user_at_function_start
>  #endif
>  
> +#ifndef unwind_user_get_ra_reg
> +static inline int unwind_user_get_ra_reg(unsigned long *val)
> +{
> +	pr_debug_once("%s (%d): unwind_user_get_ra_reg() not implemented\n",
> +		      current->comm, current->pid);

[Severity: Low]
Does this require including <linux/printk.h> and <linux/sched.h> or
<asm/current.h>?

Using pr_debug_once and current without these headers might cause compilation
failures for files that include <linux/unwind_user.h> without prior inclusion
of these dependencies.

> +	return -EINVAL;
> +}
> +#define unwind_user_get_ra_reg unwind_user_get_ra_reg
> +#endif

[ ... ]

> diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c
> index b5f984fb2df21..4af533e9f9804 100644
> --- a/kernel/unwind/sframe.c
> +++ b/kernel/unwind/sframe.c
> @@ -255,10 +255,8 @@ static __always_inline int __read_fre(struct sframe_section *sec,
>  	dataword_count--;
>  
>  	ra_off = sec->ra_off;
> -	if (!ra_off) {
> -		if (!dataword_count--)
> -			return -EINVAL;
> -
> +	if (!ra_off && dataword_count) {
> +		dataword_count--;

[Severity: High]
This isn't a bug introduced by this patch, but does the SFrame FRE size
calculation compute an incorrect length here?

Because dataword_count is decremented as each offset is read from user space,
by the time it reaches the size calculation at the end of __read_fre():

    if (dataword_count)
        return -EINVAL;

done:
    fre->size = addr_size + 1 + (dataword_count * dataword_size);

dataword_count is guaranteed to be 0.

This completely omits the size of the parsed datawords, causing fre_addr to
advance by an incorrectly small amount during iteration and breaking SFrame
unwinding for functions with multiple FREs.

This issue was introduced earlier in the series by commit "unwind_user/sframe:
Add support for reading .sframe contents", and appears to be fixed later in
the series in commit "unwind_user/sframe: Add support for SFrame V3 flexible
FDEs", where the size assignment is moved before the decrements.

>  		UNSAFE_GET_USER_INC(ra_off, cur, dataword_size, Efault);
>  	}

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