Re: [PATCH v3] riscv: stacktrace: fixed walk_stackframe()

[Samuel Holland <samuel.holland@sifive.com>]

On 2024-05-21 2:13 PM, Matthew Bystrin wrote:
> If the load access fault occures in a leaf function (with
> CONFIG_FRAME_POINTER=y), when wrong stack trace will be displayed:
> 
> [<ffffffff804853c2>] regmap_mmio_read32le+0xe/0x1c
> ---[ end trace 0000000000000000 ]---
> 
> Registers dump:
>     ra     0xffffffff80485758 <regmap_mmio_read+36>
>     sp     0xffffffc80200b9a0
>     fp     0xffffffc80200b9b0
>     pc     0xffffffff804853ba <regmap_mmio_read32le+6>
> 
> Stack dump:
>     0xffffffc80200b9a0:  0xffffffc80200b9e0  0xffffffc80200b9e0
>     0xffffffc80200b9b0:  0xffffffff8116d7e8  0x0000000000000100
>     0xffffffc80200b9c0:  0xffffffd8055b9400  0xffffffd8055b9400
>     0xffffffc80200b9d0:  0xffffffc80200b9f0  0xffffffff8047c526
>     0xffffffc80200b9e0:  0xffffffc80200ba30  0xffffffff8047fe9a
> 
> The assembler dump of the function preambula:
>     add     sp,sp,-16
>     sd      s0,8(sp)
>     add     s0,sp,16
> 
> In the fist stack frame, where ra is not stored on the stack we can
> observe:
> 
>         0(sp)                  8(sp)
>         .---------------------------------------------.
>     sp->|       frame->fp      | frame->ra (saved fp) |
>         |---------------------------------------------|
>     fp->|         ....         |         ....         |
>         |---------------------------------------------|
>         |                      |                      |
> 
> and in the code check is performed:
> 	if (regs && (regs->epc == pc) && (frame->fp & 0x7))
> 
> I see no reason to check frame->fp value at all, because it is can be
> uninitialized value on the stack. A better way is to check frame->ra to
> be an address on the stack. After the stacktrace shows as expect:
> 
> [<ffffffff804853c2>] regmap_mmio_read32le+0xe/0x1c
> [<ffffffff80485758>] regmap_mmio_read+0x24/0x52
> [<ffffffff8047c526>] _regmap_bus_reg_read+0x1a/0x22
> [<ffffffff8047fe9a>] _regmap_read+0x5c/0xea
> [<ffffffff80480376>] _regmap_update_bits+0x76/0xc0
> ...
> ---[ end trace 0000000000000000 ]---
> As pointed by Samuel Holland it is incorrect to remove check of the stackframe
> entirely.
> 
> Changes since v2 [2]:
>  - Add accidentally forgotten curly brace
> 
> Changes since v1 [1]:
>  - Instead of just dropping frame->fp check, replace it with validation of
>    frame->ra, which should be a stack address.
>  - Move frame pointer validation into the separate function.
> 
> [1] https://lore.kernel.org/linux-riscv/20240426072701.6463-1-dev.mbstr@gmail.com/
> [2] https://lore.kernel.org/linux-riscv/20240521131314.48895-1-dev.mbstr@gmail.com/
> 
> Fixes: f766f77a74f5 ("riscv/stacktrace: Fix stack output without ra on the stack top")
> Signed-off-by: Matthew Bystrin <dev.mbstr@gmail.com>
> ---
>  arch/riscv/kernel/stacktrace.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c
> index 64a9c093aef9..528ec7cc9a62 100644
> --- a/arch/riscv/kernel/stacktrace.c
> +++ b/arch/riscv/kernel/stacktrace.c
> @@ -18,6 +18,16 @@
>  
>  extern asmlinkage void ret_from_exception(void);
>  
> +static inline int fp_is_valid(unsigned long fp, unsigned long sp)

It would make sense for this function to return bool, but the generated code
would be the same, so:

Reviewed-by: Samuel Holland <samuel.holland@sifive.com>

> +{
> +	unsigned long low, high;
> +
> +	low = sp + sizeof(struct stackframe);
> +	high = ALIGN(sp, THREAD_SIZE);
> +
> +	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)
>  {
> @@ -41,21 +51,19 @@ void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
>  	}
>  
>  	for (;;) {
> -		unsigned long low, high;
>  		struct stackframe *frame;
>  
>  		if (unlikely(!__kernel_text_address(pc) || (level++ >= 0 && !fn(arg, pc))))
>  			break;
>  
> -		/* Validate frame pointer */
> -		low = sp + sizeof(struct stackframe);
> -		high = ALIGN(sp, THREAD_SIZE);
> -		if (unlikely(fp < low || fp > high || fp & 0x7))
> +		if (unlikely(!fp_is_valid(fp, sp)))
>  			break;
> +
>  		/* Unwind stack frame */
>  		frame = (struct stackframe *)fp - 1;
>  		sp = fp;
> -		if (regs && (regs->epc == pc) && (frame->fp & 0x7)) {
> +		if (regs && (regs->epc == pc) && fp_is_valid(frame->ra, sp)) {
> +			/* We hit function where ra is not saved on the stack */
>  			fp = frame->ra;
>  			pc = regs->ra;
>  		} else {


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv