stack_trace_save() uses skipnr + 1 because arch_stack_walk() is expected
to first report the return into stack_trace_save(). With FRAME_POINTER,
the RISC-V arch_stack_walk() calls walk_stackframe(), which starts from
its own frame and first reports the return into arch_stack_walk().
stack_trace_save() therefore starts one frame too early. For example,
page_owner records begin at __set_page_owner() instead of the allocation
site.
Move the walker into an always-inlined helper and call it directly from
arch_stack_walk(). Keep walk_stackframe() as a wrapper for its existing
callers.
return_address() skips level + 3 entries, while the arm64 implementation
skips level + 2. After removing the extra frame, use level + 2 so
ftrace_return_address() and CALLER_ADDR results do not shift by one caller.
Fixes: 6a00ef449370 ("riscv: eliminate unreliable __builtin_frame_address(1)")
Assisted-by: LLM
Signed-off-by: Karl Mehltretter <[email protected]>
---
Tested with the KUnit suite from patch 2 on QEMU's virt machine, clang 22
(LLVM=1), on af32da41b032:
mainline stack_trace_save 0/3, return_address 2/2
stacktrace.c change alone stack_trace_save 3/3, return_address 0/2
this patch stack_trace_save 3/3, return_address 2/2
A BeagleV Ahead (TH1520) booted with this patch passes all five cases.
The stack_trace_save() result on mainline (0/3) was also reproduced with
GCC 15.2 and QEMU 10.2.1.
Without CONFIG_FRAME_POINTER the walker scans the stack for anything that
looks like a text address and its start frame is not exact before or
after this patch. What return_address() returns there is unchanged: the
walk loses one leading entry and the skip count goes down by one.
The reliable unwinder series [1] rewrites this walker and has the correct
start frame for stack_trace_save() as a side effect, but leaves
return_address() at level + 3. This patch conflicts with 5/7 of that
series; if that goes in first, only the return_address.c hunk is needed.
[1] https://lore.kernel.org/r/[email protected]
arch/riscv/kernel/return_address.c | 2 +-
arch/riscv/kernel/stacktrace.c | 40 +++++++++++++++++++++++++++++-----
2 files changed, 36 insertions(+), 6 deletions(-)
diff --git a/arch/riscv/kernel/return_address.c
b/arch/riscv/kernel/return_address.c
index c8115ec8fb30..4da1cfd249d3 100644
--- a/arch/riscv/kernel/return_address.c
+++ b/arch/riscv/kernel/return_address.c
@@ -33,7 +33,7 @@ noinline void *return_address(unsigned int level)
{
struct return_address_data data;
- data.level = level + 3;
+ data.level = level + 2;
data.addr = NULL;
arch_stack_walk(save_return_addr, &data, current, NULL);
diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c
index c7555447149b..28c5744889ba 100644
--- a/arch/riscv/kernel/stacktrace.c
+++ b/arch/riscv/kernel/stacktrace.c
@@ -45,8 +45,16 @@ static inline int fp_is_valid(unsigned long fp, unsigned
long sp)
return !(fp < low || fp > high || fp & 0x07);
}
-void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
- bool (*fn)(void *, unsigned long), void *arg)
+/*
+ * Always inlined so that the frame the walk starts from is the caller's own,
+ * and the first entry reported for the current task is the caller's return
+ * address. arch_stack_walk() relies on that to report the same frames as
+ * the other architectures, see the comment there.
+ */
+static __always_inline void __walk_stackframe(struct task_struct *task,
+ struct pt_regs *regs,
+ bool (*fn)(void *, unsigned long),
+ void *arg)
{
unsigned long fp, sp, pc;
int graph_idx = 0;
@@ -59,6 +67,7 @@ void notrace walk_stackframe(struct task_struct *task, struct
pt_regs *regs,
} else if (task == NULL || task == current) {
fp = (unsigned long)__builtin_frame_address(0);
sp = current_stack_pointer;
+ /* Placeholder for the frame the walk starts from, never
reported. */
pc = (unsigned long)walk_stackframe;
level = -1;
} else {
@@ -102,10 +111,18 @@ void notrace walk_stackframe(struct task_struct *task,
struct pt_regs *regs,
}
}
+void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
+ bool (*fn)(void *, unsigned long), void *arg)
+{
+ __walk_stackframe(task, regs, fn, arg);
+}
+
#else /* !CONFIG_FRAME_POINTER */
-void notrace walk_stackframe(struct task_struct *task,
- struct pt_regs *regs, bool (*fn)(void *, unsigned long), void *arg)
+static __always_inline void __walk_stackframe(struct task_struct *task,
+ struct pt_regs *regs,
+ bool (*fn)(void *, unsigned long),
+ void *arg)
{
unsigned long sp, pc;
unsigned long *ksp;
@@ -133,6 +150,12 @@ void notrace walk_stackframe(struct task_struct *task,
}
}
+void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
+ bool (*fn)(void *, unsigned long), void *arg)
+{
+ __walk_stackframe(task, regs, fn, arg);
+}
+
#endif /* CONFIG_FRAME_POINTER */
static bool print_trace_address(void *arg, unsigned long pc)
@@ -176,10 +199,17 @@ unsigned long __get_wchan(struct task_struct *task)
return pc;
}
+/*
+ * The generic code expects the first entry for the current task to be the
+ * return address of arch_stack_walk() itself, the way x86 and arm64 report
+ * it, and stack_trace_save() sizes its skip count accordingly. Walking from
+ * inside a called walk_stackframe() adds that function's own frame on top,
+ * so inline the walker here and start from this frame instead.
+ */
noinline noinstr void arch_stack_walk(stack_trace_consume_fn consume_entry,
void *cookie,
struct task_struct *task, struct pt_regs *regs)
{
- walk_stackframe(task, regs, consume_entry, cookie);
+ __walk_stackframe(task, regs, consume_entry, cookie);
}
/*
