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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] riscv: stacktrace: off-by-one frame unwind in
  __walk_stackframe() for !FRAME_POINTER
--

commit 0ad8b77869cf04ae7e9c217cd0f43219461f68f7
Author: Karl Mehltretter <kmehltretter@gmail.com>

riscv: stacktrace: do not report the walker's own frame from arch_stack_walk()

This commit moves the stack walker into an always-inlined __walk_stackframe()
helper called directly from arch_stack_walk() and reduces the skip count in
return_address() from level + 3 to level + 2.

Link: https://lore.kernel.org/r/20261010132712.46931-1-kmehltretter@gmail.com

> diff --git a/arch/riscv/kernel/return_address.c b/arch/riscv/kernel/return_address.c
> index c8115ec8fb304..4da1cfd249d3d 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 c7555447149bf..28c5744889bac 100644
> --- a/arch/riscv/kernel/stacktrace.c
> +++ b/arch/riscv/kernel/stacktrace.c

[ ... ]

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

[Severity: Medium]
Does the !CONFIG_FRAME_POINTER version of __walk_stackframe() still report
walk_stackframe as its first entry before scanning the stack?

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

Unlike the CONFIG_FRAME_POINTER version, which initializes level = -1 so the
initial walk_stackframe placeholder is skipped, the !CONFIG_FRAME_POINTER
loop passes pc = (unsigned long)walk_stackframe directly to fn() on the first
iteration before reading arch_stack_walk()'s saved return address from *ksp++.
As a result, when CONFIG_FRAME_POINTER is disabled, stack_trace_save() still
starts one frame too early, and reducing data.level from level + 3 to
level + 2 in return_address() causes return_address(),
ftrace_return_address(), and CALLER_ADDRx to stop one caller too early.

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