[PATCH 1/2] riscv: stacktrace: do not report the walker's own frame from arch_stack_walk()

[PATCH 1/2] riscv: stacktrace: do not report the walker's own frame from arch_stack_walk()

[Karl Mehltretter]

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 <kmehltretter@gmail.com>
---
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/20260914092648.51254-1-xueshuai@linux.alibaba.com

 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);
 }
 
 /*

[PATCH 2/2] riscv: Add KUnit test for stack trace start frames

[Karl Mehltretter]

stack_trace_save() and return_address() depend on the first frame reported
by arch_stack_walk(), but no test checks their frame numbering.

Add a test that calls both through a chain of three noinline functions
and compares the symbol of the result with the function expected for
each skip count or level.

Require FRAME_POINTER because the stack-scanning walker cannot provide an
exact start frame.

Assisted-by: LLM
Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
---
 arch/riscv/kernel/tests/Kconfig.debug     |  12 ++
 arch/riscv/kernel/tests/Makefile          |   1 +
 arch/riscv/kernel/tests/stacktrace_test.c | 142 ++++++++++++++++++++++
 3 files changed, 155 insertions(+)

diff --git a/arch/riscv/kernel/tests/Kconfig.debug b/arch/riscv/kernel/tests/Kconfig.debug
index 40f8dafffa0a..eb79df5b2c5a 100644
--- a/arch/riscv/kernel/tests/Kconfig.debug
+++ b/arch/riscv/kernel/tests/Kconfig.debug
@@ -42,6 +42,18 @@ config RISCV_KPROBES_KUNIT
 
          If unsure, say N.
 
+config RISCV_STACKTRACE_KUNIT_TEST
+	bool "KUnit tests for RISC-V stack trace start frames"
+	depends on KUNIT && STACKTRACE && FRAME_POINTER && KALLSYMS
+	help
+	  Check that stack_trace_save() starts at its caller and skips exactly
+	  the requested number of frames through a known call chain, and that
+	  ftrace_return_address(), which CALLER_ADDR1 and up are built on,
+	  returns the caller it was asked for. This catches a stack walker
+	  that exposes its own implementation frames to its users.
+
+	  If unsure, say N.
+
 endif # RUNTIME_TESTING_MENU
 
 endmenu # "arch/riscv/kernel runtime Testing"
diff --git a/arch/riscv/kernel/tests/Makefile b/arch/riscv/kernel/tests/Makefile
index 407e7e6c28dc..9ca4135bffb5 100644
--- a/arch/riscv/kernel/tests/Makefile
+++ b/arch/riscv/kernel/tests/Makefile
@@ -1,2 +1,3 @@
 obj-$(CONFIG_RISCV_MODULE_LINKING_KUNIT)	+= module_test/
 obj-$(CONFIG_RISCV_KPROBES_KUNIT)		+= kprobes/
+obj-$(CONFIG_RISCV_STACKTRACE_KUNIT_TEST)	+= stacktrace_test.o
diff --git a/arch/riscv/kernel/tests/stacktrace_test.c b/arch/riscv/kernel/tests/stacktrace_test.c
new file mode 100644
index 000000000000..bb299a106773
--- /dev/null
+++ b/arch/riscv/kernel/tests/stacktrace_test.c
@@ -0,0 +1,142 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Check where stack_trace_save() and ftrace_return_address() start. */
+#include <kunit/test.h>
+#include <linux/ftrace.h>
+#include <linux/kallsyms.h>
+#include <linux/module.h>
+#include <linux/stacktrace.h>
+
+#define RISCV_STACKTRACE_DEPTH 16
+
+static noinline unsigned int
+riscv_stacktrace_leaf(unsigned long *entries, unsigned int skip)
+{
+	unsigned int count = stack_trace_save(entries, RISCV_STACKTRACE_DEPTH, skip);
+
+	barrier();
+	return count;
+}
+
+static noinline unsigned int
+riscv_stacktrace_middle(unsigned long *entries, unsigned int skip)
+{
+	unsigned int count = riscv_stacktrace_leaf(entries, skip);
+
+	barrier();
+	return count;
+}
+
+static noinline unsigned int
+riscv_stacktrace_outer(unsigned long *entries, unsigned int skip)
+{
+	unsigned int count = riscv_stacktrace_middle(entries, skip);
+
+	barrier();
+	return count;
+}
+
+static void riscv_stacktrace_check_skip(struct kunit *test, unsigned int skip)
+{
+	static const char * const expected[] = {
+		"riscv_stacktrace_leaf",
+		"riscv_stacktrace_middle",
+		"riscv_stacktrace_outer",
+	};
+	unsigned long entries[RISCV_STACKTRACE_DEPTH];
+	char symbol[KSYM_SYMBOL_LEN];
+	unsigned int count;
+
+	count = riscv_stacktrace_outer(entries, skip);
+	KUNIT_ASSERT_GT(test, count, 0U);
+	sprint_symbol_no_offset(symbol, entries[0]);
+	kunit_info(test, "skip=%u first=%s count=%u\n", skip, symbol, count);
+	KUNIT_EXPECT_STREQ(test, symbol, expected[skip]);
+}
+
+static void riscv_stacktrace_skip_zero(struct kunit *test)
+{
+	riscv_stacktrace_check_skip(test, 0);
+}
+
+static void riscv_stacktrace_skip_one(struct kunit *test)
+{
+	riscv_stacktrace_check_skip(test, 1);
+}
+
+static void riscv_stacktrace_skip_two(struct kunit *test)
+{
+	riscv_stacktrace_check_skip(test, 2);
+}
+
+static noinline void *riscv_return_address_leaf(unsigned int level)
+{
+	void *addr = (void *)ftrace_return_address(level);
+
+	barrier();
+	return addr;
+}
+
+static noinline void *riscv_return_address_middle(unsigned int level)
+{
+	void *addr = riscv_return_address_leaf(level);
+
+	barrier();
+	return addr;
+}
+
+static noinline void *riscv_return_address_outer(unsigned int level)
+{
+	void *addr = riscv_return_address_middle(level);
+
+	barrier();
+	return addr;
+}
+
+/*
+ * ftrace_return_address(n) is what CALLER_ADDRn expands to. Like
+ * __builtin_return_address(n), level 0 is the caller of the function that
+ * uses it.
+ */
+static void riscv_return_address_check(struct kunit *test, unsigned int level)
+{
+	static const char * const expected[] = {
+		"riscv_return_address_middle",
+		"riscv_return_address_outer",
+	};
+	char symbol[KSYM_SYMBOL_LEN];
+	void *addr;
+
+	addr = riscv_return_address_outer(level);
+	KUNIT_ASSERT_NOT_NULL(test, addr);
+	sprint_symbol_no_offset(symbol, (unsigned long)addr);
+	kunit_info(test, "level=%u caller=%s\n", level, symbol);
+	KUNIT_EXPECT_STREQ(test, symbol, expected[level]);
+}
+
+static void riscv_return_address_level_zero(struct kunit *test)
+{
+	riscv_return_address_check(test, 0);
+}
+
+static void riscv_return_address_level_one(struct kunit *test)
+{
+	riscv_return_address_check(test, 1);
+}
+
+static struct kunit_case riscv_stacktrace_cases[] = {
+	KUNIT_CASE(riscv_stacktrace_skip_zero),
+	KUNIT_CASE(riscv_stacktrace_skip_one),
+	KUNIT_CASE(riscv_stacktrace_skip_two),
+	KUNIT_CASE(riscv_return_address_level_zero),
+	KUNIT_CASE(riscv_return_address_level_one),
+	{}
+};
+
+static struct kunit_suite riscv_stacktrace_suite = {
+	.name = "riscv-stacktrace-skip",
+	.test_cases = riscv_stacktrace_cases,
+};
+
+kunit_test_suite(riscv_stacktrace_suite);
+
+MODULE_LICENSE("GPL");

Re: [PATCH 1/2] riscv: stacktrace: do not report the walker's own frame from arch_stack_walk()

[Shuai Xue]

On 10/10/26 9:27 PM, Karl Mehltretter wrote:
> 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 <kmehltretter@gmail.com>
> ---

Hi, Karl,

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


Hi Karl,

As promised in the other thread, here are test results for this series.

Short version: the fix is correct, the KUnit suite reproduces the bug
precisely, and the whole series passes here.

Testing
-------

Your numbers were collected with clang 22, plus the mainline
reproduction with GCC 15.2 you noted in the commit message, so I
re-ran the full matrix with gcc: riscv64 cross-gcc 16.2.0 for the
first three configurations below and a native gcc 14.3 for the last
one, all under QEMU (qemu-system-riscv64 11.1.1, -machine virt,
-cpu rv64) on top of your base af32da41b032.

A. base + 2/2 only (tests, no fix): pass:2 fail:3, i.e. exactly the
    "stack_trace_save 0/3, return_address 2/2" you reported for
    mainline.  The failures show the off-by-one shift nicely; with
    skip = 0 the first recorded symbol is the walker itself:

        # riscv_stacktrace_skip_zero: skip=0 first=stack_trace_save count=11
        #     symbol == "stack_trace_save"
        #     expected[skip] == "riscv_stacktrace_leaf"

    The two return_address() cases still pass on unpatched mainline,
    which is consistent with the analysis: the current "level + 3" is
    calibrated for the buggy frame layout, so ftrace_return_address()
    keeps working while stack_trace_save() - and everything built on
    top of it - records one bogus leading entry.

B. base + 1/2 + 2/2: pass:5 fail:0.

C. My v6 reliable stack unwinding series [1] + your 2/2, with the
    level constant in return_address.c adjusted to the same
    unconditional "level + 2" as in your 1/2: pass:5 fail:0, with
    per-case results identical to B (skip=0/1/2 first =
    leaf/middle/outer, count = 10/9/8).

D. Cross-toolchain check: I rebuilt the C tree with a native riscv64
    gcc 14.3 inside an Anolis OS 23.4 riscv64 guest - no cross
    toolchain involved at any step - using the same Kconfig options,
    and booted the resulting Image under the same QEMU: pass:5 fail:0
    again, identical output.  The result does not depend on one
    particular compiler's frame layout.

Please feel free to add

     Tested-by: Shuai Xue <xueshuai@linux.alibaba.com>
     Reviewed-by: Shuai Xue <xueshuai@linux.alibaba.com>

Thanks.
Shuai

Re: [PATCH 2/2] riscv: Add KUnit test for stack trace start frames

[Shuai Xue]

On 10/10/26 9:27 PM, Karl Mehltretter wrote:
> stack_trace_save() and return_address() depend on the first frame reported
> by arch_stack_walk(), but no test checks their frame numbering.
> 
> Add a test that calls both through a chain of three noinline functions
> and compares the symbol of the result with the function expected for
> each skip count or level.
> 
> Require FRAME_POINTER because the stack-scanning walker cannot provide an
> exact start frame.
> 
> Assisted-by: LLM
> Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
> ---
>   arch/riscv/kernel/tests/Kconfig.debug     |  12 ++
>   arch/riscv/kernel/tests/Makefile          |   1 +
>   arch/riscv/kernel/tests/stacktrace_test.c | 142 ++++++++++++++++++++++
>   3 files changed, 155 insertions(+)
> 
> diff --git a/arch/riscv/kernel/tests/Kconfig.debug b/arch/riscv/kernel/tests/Kconfig.debug
> index 40f8dafffa0a..eb79df5b2c5a 100644
> --- a/arch/riscv/kernel/tests/Kconfig.debug
> +++ b/arch/riscv/kernel/tests/Kconfig.debug
> @@ -42,6 +42,18 @@ config RISCV_KPROBES_KUNIT
>   
>            If unsure, say N.
>   
> +config RISCV_STACKTRACE_KUNIT_TEST
> +	bool "KUnit tests for RISC-V stack trace start frames"
> +	depends on KUNIT && STACKTRACE && FRAME_POINTER && KALLSYMS

Hi, Karl

One small nit: the new RISCV_STACKTRACE_KUNIT_TEST entry does
not follow the KUNIT_ALL_TESTS pattern of the two existing entries
right next to it (RISCV_KPROBES_KUNIT, RISCV_MODULE_LINKING_KUNIT):

     config RISCV_STACKTRACE_KUNIT_TEST
             bool "KUnit tests for RISC-V stack trace start frames" if !KUNIT_ALL_TESTS
             default KUNIT_ALL_TESTS

Without "default KUNIT_ALL_TESTS" the suite is never enabled by
kunit.all_tests, and without the "if !KUNIT_ALL_TESTS" prompt guard
it shows up as a redundant prompt when KUNIT_ALL_TESTS is already
set.  This is not a blocker in any way, just a nice tweak for a
potential v2.

Please feel free to add

     Tested-by: Shuai Xue <xueshuai@linux.alibaba.com>

to both patches, and additionally on 1/2

     Reviewed-by: Shuai Xue <xueshuai@linux.alibaba.com>

Thanks!
Shuai