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

On 10/14/26 4:18 AM, Martin Kaiser wrote:

Thus wrote Shuai Xue (xueshuai@linux.alibaba.com):

quoted

From: Wang Han <redacted>

quoted

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.

quoted

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.

quoted

Three related callers are updated to keep the same frame-record
assumptions everywhere:

quoted

   * 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.

quoted

   * 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.

quoted

   * 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.

quoted

The non-frame-pointer fallback walker is kept untouched for
!CONFIG_FRAME_POINTER builds.

quoted

Assisted-by: Qoder:Qwen-3.8-MAX-Preview
Signed-off-by: Wang Han <redacted>
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(-)

quoted

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;

quoted

-	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;
  }

quoted

@@ -267,7 +268,8 @@ void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
  	 */
  	old = *parent;

quoted

-	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