Patch "riscv: stacktrace: fixed walk_stackframe()" has been added to the 6.9-stable tree

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



This is a note to let you know that I've just added the patch titled

    riscv: stacktrace: fixed walk_stackframe()

to the 6.9-stable tree which can be found at:
    http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
     riscv-stacktrace-fixed-walk_stackframe.patch
and it can be found in the queue-6.9 subdirectory.

If you, or anyone else, feels it should not be added to the stable tree,
please let <stable@xxxxxxxxxxxxxxx> know about it.



commit 40102264201527df644fe49cfaf0ea7cef66d881
Author: Matthew Bystrin <dev.mbstr@xxxxxxxxx>
Date:   Tue May 21 22:13:13 2024 +0300

    riscv: stacktrace: fixed walk_stackframe()
    
    [ Upstream commit a2a4d4a6a0bf5eba66f8b0b32502cc20d82715a0 ]
    
    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@xxxxxxxxx/
    [2] https://lore.kernel.org/linux-riscv/20240521131314.48895-1-dev.mbstr@xxxxxxxxx/
    
    Fixes: f766f77a74f5 ("riscv/stacktrace: Fix stack output without ra on the stack top")
    Signed-off-by: Matthew Bystrin <dev.mbstr@xxxxxxxxx>
    Reviewed-by: Samuel Holland <samuel.holland@xxxxxxxxxx>
    Link: https://lore.kernel.org/r/20240521191727.62012-1-dev.mbstr@xxxxxxxxx
    Signed-off-by: Palmer Dabbelt <palmer@xxxxxxxxxxxx>
    Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>

diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c
index 64a9c093aef93..528ec7cc9a622 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)
+{
+	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 {




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux