Re: [PATCH v7 08/10] riscv: stacktrace: switch to frame-pointer based unwinder

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] riscv: stacktrace: broken leaf-function unwind in
  kunwind_next_frame_record()
- [High] riscv: stacktrace: broken unwind across ftrace_caller and
  return_to_handler
- [Medium] riscv: stacktrace: bogus walk_stackframe entry in
  __walk_stackframe()
- [Low] riscv: stacktrace: inaccurate metadata type offset comment
--

commit 263985113a916e8cd08cc4485092812e49e28303
Author: Wang Han <wanghan@linux.alibaba.com>

riscv: stacktrace: switch to frame-pointer based unwinder

This commit replaces the open-coded frame-pointer stack walker with a
kunwind state machine, adds arch_stack_walk_reliable(), and routes
perf_callchain_kernel(), dump_backtrace(), and __get_wchan() through
arch_stack_walk().

> diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c
> index a5abb705d43ba..490092c28d612 100644
> --- a/arch/riscv/kernel/stacktrace.c
> +++ b/arch/riscv/kernel/stacktrace.c

[ ... ]

> +/*
> + * Unwind from one frame record to the next.
> + *
> + * On RISC-V, the frame record sits at fp - sizeof(struct frame_record),
> + * immediately below the address pointed to by fp/s0. This applies to both
> + * normal frame records and metadata frame records (embedded in pt_regs).
> + *
> + * A metadata record is identified by both fp and ra being zero in the
> + * frame_record portion, with a type value following at fp + 16.
> + */

[Severity: Low]
This isn't a bug, but since fp points immediately above struct
frame_record on RISC-V (meta_base = fp - sizeof(struct frame_record)),
meta->type is located at fp rather than fp + 16.

> +static __always_inline int
> +kunwind_next_frame_record(struct kunwind_state *state)
> +{
> +	unsigned long fp = state->common.fp;
> +	struct frame_record *record;
> +	struct stack_info *info;
> +	unsigned long new_fp, new_pc;
> +	unsigned long record_base;
> +
> +	if (fp & 0x7)
> +		return -EINVAL;
> +
> +	record_base = fp - sizeof(*record);
> +
> +	info = unwind_find_stack(&state->common, record_base, sizeof(*record));
> +	if (!info)
> +		return -EINVAL;
> +
> +	record = (struct frame_record *)record_base;
> +	new_fp = READ_ONCE(record->fp);
> +	new_pc = READ_ONCE(record->ra);
> +
> +	if (!new_fp && !new_pc)
> +		return kunwind_next_frame_record_meta(state);
> +
> +	unwind_consume_stack(&state->common, info, record_base,
> +			     sizeof(*record));
> +
> +	state->common.fp = new_fp;
> +	state->common.pc = new_pc;
> +	state->source = KUNWIND_SOURCE_FRAME;
> +
> +	return 0;
> +}

[Severity: High]
Does unwinding from KUNWIND_SOURCE_REGS_PC corrupt the unwind and leak
kernel stack addresses when an exception or interrupt hits a leaf
function?

On RISC-V with CONFIG_FRAME_POINTER=y, leaf functions do not push ra onto
the stack: they save only the caller's s0 at fp - sizeof(unsigned long)
(which aliases record->ra), leave the return address in regs->ra, and
leave fp - 2 * sizeof(unsigned long) (record->fp) as uninitialized stack
padding or local storage.

The previous unwinder checked for this case, whereas kunwind_init_from_regs()
and kunwind_next_regs_pc() store regs in state->regs without ever reading
it back, and kunwind_next_frame_record() unconditionally reads record->ra
as new_pc and record->fp as new_fp.  Because __kernel_text_address()
filtering is also removed and perf_callchain_kernel() now calls
arch_stack_walk(), a PMU sample in a leaf function passes the caller's
saved kernel stack pointer in record->ra to fill_callchain() ->
perf_callchain_store(), exposing it to userspace, while regs->ra is lost
and the uninitialized word in record->fp is used as the next frame
pointer.

[ ... ]

> +static __always_inline bool
> +arch_reliable_kunwind_consume_entry(const struct kunwind_state *state, void *cookie)
> +{
> +	/*
> +	 * At an exception boundary we can reliably consume the saved PC. We do
> +	 * not know whether ra was live when the exception was taken, and
> +	 * so we cannot perform the next unwind step reliably.
> +	 *
> +	 * All that matters is whether the *entire* unwind is reliable, so give
> +	 * up as soon as we hit an exception boundary.
> +	 */
> +	if (state->source == KUNWIND_SOURCE_REGS_PC)
> +		return false;
> +
> +	return arch_kunwind_consume_entry(state, cookie);
> +}

[Severity: High]
Can kunwind_next_frame_record() and arch_stack_walk_reliable() unwind
across ftrace_caller and return_to_handler?

Unlike arm64, RISC-V's ftrace trampolines do not maintain a valid struct
frame_record chain at s0 - sizeof(struct frame_record):

arch/riscv/kernel/mcount-dyn.S:SAVE_ABI_REGS {
	addi	sp, sp, -FREGS_SIZE_ON_STACK
	REG_S	t0,  FREGS_EPC(sp)
	REG_S	x1,  FREGS_RA(sp)
	REG_S	x8,  FREGS_S0(sp)
    ...
}

arch/riscv/kernel/mcount.S:SAVE_RET_ABI_STATE {
	addi	sp, sp, -FREGS_SIZE_ON_STACK
	REG_S	ra, FREGS_RA(sp)
	REG_S	s0, FREGS_S0(sp)
	REG_S	a0, FREGS_A0(sp)
	REG_S	a1, FREGS_A1(sp)
	addi	s0, sp, FREGS_SIZE_ON_STACK
}

In ftrace_caller, SAVE_ABI_REGS saves ftrace_regs without updating s0 or
creating a frame record, so a C tracer callback saves the traced
function's caller s0 alongside a return address in ftrace_caller.  Walking
past the callback jumps straight from ftrace_caller to the caller of the
traced function, silently skipping both the traced function and its
immediate caller while arch_stack_walk_reliable() still succeeds.  (Commit
"riscv: ftrace: always preserve s0 in dynamic ftrace register frame" noted
that the reliable unwinder would read ftrace_regs_get_frame_pointer(fregs)
directly, but kunwind has no ftrace_regs handling.)

In return_to_handler, SAVE_RET_ABI_STATE sets s0 to sp +
FREGS_SIZE_ON_STACK after saving only ra, s0, a0, and a1.  When unwinding
past ftrace_return_to_handler(), kunwind_next_frame_record() reads
s0 - sizeof(struct frame_record), which sits in the uninitialized upper
words of the __arch_ftrace_regs allocation rather than at FREGS_S0(sp).

[ ... ]

> +/*
>   * 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.
> + * 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 call the always-inlined 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)
> +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);
>  }

[Severity: Medium]
On !CONFIG_FRAME_POINTER builds, doesn't __walk_stackframe() still report
walk_stackframe as the first entry when unwinding the current task?

arch/riscv/kernel/stacktrace.c:__walk_stackframe() {
    ...
	} else if (task == NULL || task == current) {
		sp = current_stack_pointer;
		pc = (unsigned long)walk_stackframe;
	} else {
    ...
	ksp = (unsigned long *)sp;
	while (!kstack_end(ksp)) {
		if (__kernel_text_address(pc) && unlikely(!fn(arg, pc)))
			break;
		pc = READ_ONCE_NOCHECK(*ksp++);
	}
}

Unlike the removed CONFIG_FRAME_POINTER version (which started at
level = -1 to skip the placeholder pc), the !CONFIG_FRAME_POINTER loop
calls fn(arg, pc) before reading *ksp++.  As a result, arch_stack_walk()
reports walk_stackframe before scanning its own stack frame, shifting
stack_trace_save() and return_address() off by one and printing a bogus
walk_stackframe entry in dump_backtrace().

Also, since all callers of walk_stackframe() are converted to
arch_stack_walk() by this patch, walk_stackframe() has no remaining
callers and its extern declaration in arch/riscv/include/asm/stacktrace.h
is left without a definition when CONFIG_FRAME_POINTER=y.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110013750.36800-1-xueshuai@linux.alibaba.com?part=8