Re: [RFC PATCH 2/2] sparc64: Support PERF_SAMPLE_REGS_USER and PERF_SAMPLE_STACK_USER

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] perf: sparc64 user stack dumps read from unmapped biased pointer
- [Medium] perf: sparc64 truncates 64-bit kernel registers for 32-bit compat tasks
--

commit 549c908d74e0e448486cd60566d4f3e6ff5d6f8e
Author: Stian Halseth <stian@itx.no>
sparc64: Support PERF_SAMPLE_REGS_USER and PERF_SAMPLE_STACK_USER

This commit adds support for PERF_SAMPLE_REGS_USER and PERF_SAMPLE_STACK_USER
on sparc64. It selects HAVE_PERF_REGS and HAVE_PERF_USER_STACK_DUMP,
implements the perf_regs interface to expose registers mirroring pt_regs, and
provides perf_arch_prepare_ustack() to flush user register windows.

> diff --git a/arch/sparc/include/asm/perf_event.h b/arch/sparc/include/asm/perf_event.h
> index c2aec0c7f4f5e..03f1ab1618b95 100644
> --- a/arch/sparc/include/asm/perf_event.h
> +++ b/arch/sparc/include/asm/perf_event.h
[ ... ]
> @@ -25,6 +25,9 @@ do {							\
>  	(regs)->u_regs[UREG_I6] = _fp;			\
>  	(regs)->u_regs[UREG_I7] = _i7;			\
>  } while (0)
> +
> +void perf_arch_prepare_ustack(void);
> +#define perf_arch_prepare_ustack	perf_arch_prepare_ustack

[Severity: Medium]
Since HAVE_PERF_USER_STACK_DUMP is selected in Kconfig, shouldn't there also
be an override for perf_user_stack_pointer() here to account for STACK_BIAS
on 64-bit tasks?

When a user requests a perf record with user stack sampling
(--call-graph dwarf) on a 64-bit SPARC process, the generic perf core
attempts to copy memory from the address returned by
perf_user_stack_pointer(regs). On 64-bit SPARC, the stack pointer
register %o6 (UREG_FP) is biased by 2047 bytes.

Without overriding perf_user_stack_pointer() to add STACK_BIAS, the core
perf code will attempt to read memory starting 2047 bytes below the
valid stack. Does this cause the copy to fail immediately upon hitting
an unmapped guard page, resulting in an empty stack dump?

> diff --git a/arch/sparc/kernel/perf_regs.c b/arch/sparc/kernel/perf_regs.c
> new file mode 100644
> index 0000000000000..644a0a11cb907
> --- /dev/null
> +++ b/arch/sparc/kernel/perf_regs.c
> @@ -0,0 +1,65 @@
[ ... ]
> +u64 perf_reg_value(struct pt_regs *regs, int idx)
> +{
> +	u64 val;
> +
> +	switch (idx) {
[ ... ]
> +	case PERF_REG_SPARC_Y:
> +		return regs->y;
> +	default:
> +		WARN_ON_ONCE(1);
> +		return 0;
> +	}
> +
> +	if (test_thread_flag(TIF_32BIT))
> +		val = (u32)val;

[Severity: Medium]
Will this unconditionally truncate 64-bit kernel registers for 32-bit compat
tasks?

When profiling kernel state via PERF_SAMPLE_REGS_INTR, the interrupted
registers belong to the 64-bit kernel mode, even if the current task is a
32-bit compat task (for instance, when servicing a system call).

Should this truncation be conditional on user_mode(regs) to avoid destroying
kernel register samples and breaking unwinding and symbol resolution for
kernel execution?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260922135653.1622301-1-stian@itx.no?part=2