Re: [PATCH v10 RESEND 6/9] perf: RISC-V: support callchains with SSE delivery

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] perf: RISC-V: user stack unwinder retains context-corrupting nested faults
- [Medium] perf: RISC-V: kernel callchain truncation without CONFIG_FRAME_POINTER
--

commit a95d15a6320c76f0780598b2a4d0ec479bdebc3c
Author: Zhanpeng Zhang <zhangzhanpeng.jasper@bytedance.com>

perf: RISC-V: support callchains with SSE delivery

PMU overflow delivery through SSE enters Linux with a synthetic supervisor
context on a dedicated event stack. This patch implements an SSE-specific
RISC-V user and kernel callchain unwinder that attempts to avoid
exception-table handled faults.

> diff --git a/arch/riscv/kernel/perf_callchain.c b/arch/riscv/kernel/perf_callchain.c
> index b465bc9eb870e..ec75689c7aecb 100644
> --- a/arch/riscv/kernel/perf_callchain.c
> +++ b/arch/riscv/kernel/perf_callchain.c

[ ... ]

> +static bool sse_kernel_regs_safe(struct pt_regs *regs)
> +{
> +	unsigned long fp = frame_pointer(regs);
> +	unsigned long pc = instruction_pointer(regs);
> +	unsigned long sp = user_stack_pointer(regs);
> +
> +	if (!__kernel_text_address(pc))
> +		return false;
> +	if (!sse_addr_on_task_stack(sp, sizeof(unsigned long)))
> +		return false;
> +	if (fp < sizeof(struct stackframe))
> +		return false;
> +
> +	return sse_addr_on_task_stack(fp - sizeof(struct stackframe),
> +				      sizeof(struct stackframe));

[Severity: Medium]
Does this logic unconditionally enforce frame pointer validity, which might
break stack unwinding when the kernel doesn't use frame pointers?

When CONFIG_FRAME_POINTER is disabled, regs->s0 (fp) is a general-purpose
callee-saved register containing arbitrary data, not a valid frame pointer.
Unconditionally validating fp as a valid stack address could fail, causing
sse_callchain_kernel() to skip the walk_stackframe() fallback and truncate
PMU kernel callchains to a single PC entry.

The walk_stackframe() fallback correctly handles CONFIG_FRAME_POINTER=n by
iterating the stack pointer. Can we avoid unconditionally requiring a valid
frame pointer here?

[ ... ]

>  void perf_callchain_user(struct perf_callchain_entry_ctx *entry,
>  			 struct pt_regs *regs)
>  {
> +#ifdef CONFIG_RISCV_SBI_SSE
> +	const struct riscv_sse_interrupted_context *context;
> +
> +	context = riscv_sse_get_interrupted_context();
> +	/* A guest stack cannot be walked using the host address space. */
> +	if (sse_callchain_is_guest(context))
> +		return;
> +#endif
> +
>  	if (perf_guest_state()) {
>  		/* TODO: We don't support guest os callchain now */
>  		return;
>  	}
>  
>  	arch_stack_walk_user(fill_callchain, entry, regs);

[Severity: High]
Does this still invoke the user unwinder which relies on
__copy_from_user_inatomic(), conflicting with the patch's goal of avoiding
hardware faults in SSE handlers?

The commit message states that taking an exception-table handled fault
from the SSE handler can corrupt the interrupted kernel context. While this
patch introduces arch_perf_out_copy_user() to bypass
__copy_from_user_inatomic() for raw stack sampling, perf_callchain_user()
continues to use arch_stack_walk_user().

Since arch_stack_walk_user() reads user memory via
__copy_from_user_inatomic() and hardware traps could corrupt the SSE
context, won't the user stack unwinder trigger the same context
corruption when encountering a non-resident page?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1789974241.git.zhangzhanpeng.jasper@bytedance.com?part=6