Re: [PATCH v6 5/7] riscv: stacktrace: switch to frame-pointer based unwinder

[Shuai Xue <xueshuai@linux.alibaba.com>]

On 10/14/26 4:18 AM, Martin Kaiser wrote:
> Thus wrote Shuai Xue (xueshuai@linux.alibaba.com):
> 
>> From: Wang Han <wanghan@linux.alibaba.com>
> 
>> Replace the open-coded frame-pointer walker in arch_stack_walk() with a
>> robust kunwind state machine, modelled on arch/arm64/kernel/stacktrace.c
>> and retargeted to the RISC-V {fp, ra} frame record convention. The new
>> walker tracks stack bounds, consumes frame records monotonically,
>> understands the metadata pt_regs records added in the previous frame
>> record metadata patch, and recovers return addresses replaced by
>> function graph tracing and kretprobes.
> 
>> This commit introduces arch_stack_walk_reliable() but does not yet
>> select HAVE_RELIABLE_STACKTRACE; that is done in a follow-up Kconfig
>> patch so this commit can be reviewed and bisected as a pure unwinder
>> replacement. Until that Kconfig change lands, livepatch is not yet
>> enabled and arch_stack_walk_reliable() has no in-tree caller.
> 
>> Three related callers are updated to keep the same frame-record
>> assumptions everywhere:
> 
>>    * Function graph tracing: the old RISC-V unwinder matched function
>>      graph return-stack entries by the saved return-address slot. That
>>      was consistent with the static mcount path, but not with the dynamic
>>      ftrace path where the parent slot is ftrace_regs::ra. Use the
>>      architectural frame pointer as the function graph return-address
>>      cookie, matching the kunwind walker.
> 
>>    * Perf callchains: route kernel callchain collection through
>>      arch_stack_walk() so perf sees the same frame-pointer unwind
>>      behaviour as dump_stack() and the upcoming livepatch path.
> 
>>    * dump_backtrace() / __get_wchan() / show_stack(): these now go
>>      through arch_stack_walk(); the explicit "Call Trace:" header is
>>      moved into dump_backtrace() to preserve the original output.
> 
>> The non-frame-pointer fallback walker is kept untouched for
>> !CONFIG_FRAME_POINTER builds.
> 
>> Assisted-by: Qoder:Qwen-3.8-MAX-Preview
>> Signed-off-by: Wang Han <wanghan@linux.alibaba.com>
>> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
>> ---
>>   arch/riscv/kernel/ftrace.c         |   6 +-
>>   arch/riscv/kernel/perf_callchain.c |   2 +-
>>   arch/riscv/kernel/stacktrace.c     | 561 ++++++++++++++++++++++++-----
>>   3 files changed, 473 insertions(+), 96 deletions(-)
> 
>> diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
>> index be8b685144174..0facd824c911d 100644
>> --- a/arch/riscv/kernel/ftrace.c
>> +++ b/arch/riscv/kernel/ftrace.c
>> @@ -245,7 +245,8 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
>>   	 */
>>   	old = *parent;
> 
>> -	if (!function_graph_enter(old, self_addr, frame_pointer, parent))
>> +	if (!function_graph_enter(old, self_addr, frame_pointer,
>> +				  (void *)frame_pointer))
>>   		*parent = return_hooker;
>>   }
> 
>> @@ -267,7 +268,8 @@ void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
>>   	 */
>>   	old = *parent;
> 
>> -	if (!function_graph_enter_regs(old, ip, frame_pointer, parent, fregs))
>> +	if (!function_graph_enter_regs(old, ip, frame_pointer,
>> +				       (void *)frame_pointer, fregs))
>>   		*parent = return_hooker;
>>   }
> 
> 
> Are these two changes fixing a bug in the current code (that is independent
> from the rest of this patch)?
> 
> Without your changes, function_graph_enter_regs writes a struct ftrace_ret_stack
> with retp == the saved ra.
> 
> And the only place where retp is fetched and checked is
> 
> function_get_true_parent_ip
>     ftrace_graph_ret_addr(..., (uintptr_t *)ftrace_regs_get_stack_pointer(fregs));
> 
> The stored retp is checked against the stack pointer.
> 
> Your changes store fp instead of ra, this seems the right thing to do. (And
> it's in line with other archs.)
> 
> Should those two changes be in a separate patch (that can be backported to
> stable kernels)?

Hi, Martin,

Thanks for the sharp question.

You are right that the stored cookie is broken. What
ftrace_graph_func() passes as retp is &arch_ftrace_regs(fregs)->ra,
i.e. the slot inside the transient ftrace trampoline frame, which is
popped as soon as the callback returns. That has been the case since
35e61e8827ee8 ("riscv: ftrace: Make function graph use ftrace
directly") introduced ftrace_graph_func() in v6.8. (The code was
copied from arm64's ftrace_graph_func(), but arm64 passes
(void *)frame_pointer there - see 41705c4262aac for the current
shape.)

Since distro kernels only ever use the dynamic path, the net effect
is that the shadow stack is never consulted during unwinding:
whenever the function graph tracer is running, dump_stack(), perf
callchains and __get_wchan() report return_to_handler instead of the
real caller.

I will split these two changes into a separate patch and cc stable.

Thanks.
Shuai

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv