riscv: stacktrace: fixed walk_stackframe()
PATCH 1/1 riscv: stacktrace: fixed walk_stackframe()
If the load access fault occures in a function without callee (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 0xffffffff8047fe9aThe assembler dump of the function preambula:
add sp,sp,-16
sd s0,8(sp)
add s0,sp,16In 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, so removed it. 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 ]---
---
I've catched this bug on v6.1 with gcc 12.2.0 (Debian 12.2.0-13). Different compiler versions generate the same assembler code. So I think this is not a compiler issue.
arch/riscv/kernel/stacktrace.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/riscv/kernel/stacktrace.c +1 -1
@@ -55,7 +55,7 @@ void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,/* Unwind stack frame */ frame = (struct stackframe *)fp -1 ; sp = fp; -if (regs && (regs->epc == pc) && (frame->fp &0x7 )) { +if (regs && regs->epc == pc) { fp = frame->ra; pc = regs->ra; }else {
Signature · 7 lines
2.43.0 _______________________________________________ linux-riscv mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-riscv
Hi Matthew,
Thanks for the patch!
Quoted 35 lines · Matthew Bystrin
If the load access fault occures in a function without callee (
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 0xffffffff8047fe9aThe assembler dump of the function preambula:
add sp,sp,-16 sd s0,8(sp) add s0,sp,16In 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->| .... | .... | |---------------------------------------------| | | |
Quoted 5 lines
and in the code check is performed:
if (regs && (regs->epc == pc) && (frame->fp & 0x7))I see no reason to check
frame->fpvalue at all, because it is can be uninitialized value on the stack, so removed it. After the stacktrace
The purpose of this check is to distinguish leaf functions from non-leaf functions, which have a different frame record layout. regs->epc does not necessarily point to somewhere in a leaf function, for example if a non-leaf function was preempted by an interrupt. So it incorrect to remove the check entirely and always consider the first frame in the stack trace to be a leaf function.
You are correct that for a leaf function, frame->fp could be uninitialized or an arbitrary saved register, so the existing check is wrong. A better check would be based on frame->ra, which is always pushed. For non-leaf functions, frame->ra is a kernel text address. For leaf functions, frame->ra is a stack address. So checking if frame->ra is within the current stack (between low and high) should be sufficient to detect this case.
Quoted 10 lines
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 ]---
Quoted 6 lines
Fixes f766f77a74f5 ("riscv/stacktrace: Fix stack output without ra on the stack top")Signed-off-by Matthew Bystrin <[email protected]>---
I've catched this bug on v6.1 with gcc 12.2.0 (Debian 12.2.0-13). Different compiler versions generate the same assembler code. So I think this is not a compiler issue.
Unfortunately, this is indeed a mismatch between the compiler and the documented ABI[1]. The ABI was created based on the existing GCC behavior[2]. However, the GCC behavior is different for leaf functions. Specifically, the logic for determining if ra needs to be saved on the stack does not check if frame pointers are enabled. Since GCC was not trying to maintain a specific stack frame layout, and just pushing registers sequentially as needed, this makes the frame record layout different for leaf functions. So existing versions of GCC are incompatible with the ABI as written.
This was fixed for GCC 14 as a side effect of adding the -m(no)-omit-leaf-frame-pointer option[3]. After this change, ra is always saved if a frame record is being saved, so the frame record layout is always the same, matching the ABI.
For the kernel, since we want frame records to be available for stack walking, we should pass -mno-omit-leaf-frame-pointer if that option is supported. If the option is supported, we know the ABI is fixed, so in that case we can omit the workaround described above.
We may still want a compiler version check, because there is a much simpler GCC change that would fix the ABI incompatibility in older branches without adding the new option:
gcc/config/riscv/riscv.cc +2 -1
@@ -3894,7 +3894,8 @@ riscv_save_reg_p (unsigned int regno) if (regno == HARD_FRAME_POINTER_REGNUM && frame_pointer_needed) return true; - if (regno == RETURN_ADDR_REGNUM && crtl->calls_eh_return) + if (regno == RETURN_ADDR_REGNUM && (crtl->calls_eh_return + || frame_pointer_needed)) return true; /* If this is an interrupt handler, then must save extra registers. */
[1]:
https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-cc.adoc#frame-pointer-convention
[2]: https://github.com/riscv-non-isa/riscv-elf-psabi-doc/issues/18
[3]:
https://github.com/gcc-mirror/gcc/commit/39663298b5934831a0125e12f113ebd83248c3be
Regards,
Samuel
Quoted 9 lines
arch/riscv/kernel/stacktrace.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/riscv/kernel/stacktrace.c +0
-0
arch/riscv/kernel/stacktrace.c void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs, · line 61
Quoted 8 lines
arch/riscv/kernel/stacktrace.c +1
-1@@ -55,7 +55,7 @@ void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,/* Unwind stack frame */ frame = (struct stackframe *)fp -1 ; sp = fp; -if (regs && (regs->epc == pc) && (frame->fp &0x7 )) { +if (regs && regs->epc == pc) { fp = frame->ra; pc = regs->ra; }else {
_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv
Palmer Dabbelt
maintainerQuoted 53 lines · On Wed, 08 May 2024 08:51:04 PDT (-0700), [email protected] wrote:
Hi Matthew,
Thanks for the patch!
Quoted 35 lines · Matthew Bystrin
If the load access fault occures in a function without callee (
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 0xffffffff8047fe9aThe assembler dump of the function preambula:
add sp,sp,-16 sd s0,8(sp) add s0,sp,16In 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->| .... | .... | |---------------------------------------------| | | |Quoted 5 lines
and in the code check is performed:
if (regs && (regs->epc == pc) && (frame->fp & 0x7))I see no reason to check
frame->fpvalue at all, because it is can be uninitialized value on the stack, so removed it. After the stacktraceThe purpose of this check is to distinguish leaf functions from non-leaf functions, which have a different frame record layout.
regs->epcdoes not necessarily point to somewhere in a leaf function, for example if a non-leaf function was preempted by an interrupt. So it incorrect to remove the check entirely and always consider the first frame in the stack trace to be a leaf function.
Quoted 6 lines
You are correct that for a leaf function,
frame->fpcould be uninitialized or an arbitrary saved register, so the existing check is wrong. A better check would be based onframe->ra, which is always pushed. For non-leaf functions,frame->rais a kernel text address. For leaf functions,frame->rais a stack address. So checking ifframe->rais within the current stack (betweenlowandhigh) should be sufficient to detect this case.
I was just playing with this, looks like GCC is treating tail-call-only functions as leaf functions. So I think whatever workaround we come up with loses the tail-call-only function in the backtrace, not sure if there's any way we can work around that as there's essentially no frame remaining for it. From https://godbolt.org/z/7bMsEbqjn and GCC trunk
int glob;
void leaf(int x) {
glob = x;
}
void tail(int x) {
leaf(x);
}
void func(int x) {
leaf(x);
glob = x+1;
}
-O2 -fno-inline -fno-omit-frame-pointer -momit-leaf-frame-pointer
leaf(int):
lui a5,%hi(glob)
sw a0,%lo(glob)(a5)
ret
tail(int):
tail leaf(int)
func(int):
addi sp,sp,-32
sd s0,16(sp)
sd s1,8(sp)
… addi sp,sp,32
jr ra
glob:
.zero 4Quoted 27 lines
Quoted 11 lines
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 ]---Quoted 6 lines
Fixes f766f77a74f5 ("riscv/stacktrace: Fix stack output without ra on the stack top")Signed-off-by Matthew Bystrin <[email protected]>---
I've catched this bug on v6.1 with gcc 12.2.0 (Debian 12.2.0-13). Different compiler versions generate the same assembler code. So I think this is not a compiler issue.Unfortunately, this is indeed a mismatch between the compiler and the documented ABI[1]. The ABI was created based on the existing GCC behavior[2]. However, the GCC behavior is different for leaf functions. Specifically, the logic for determining if ra needs to be saved on the stack does not check if frame pointers are enabled. Since GCC was not trying to maintain a specific stack frame layout, and just pushing registers sequentially as needed, this makes the frame record layout different for leaf functions. So existing versions of GCC are incompatible with the ABI as written.
Quoted 4 lines
This was fixed for GCC 14 as a side effect of adding the -m(no)-omit-leaf-frame-pointer option[3]. After this change, ra is always saved if a frame record is being saved, so the frame record layout is always the same, matching the ABI.
Ya, I think so -- IIUC we don't strictly need the option because the default seems to follow -fno-omit-frame-pointer, so we only strictly need to check for the presence of -mno-omit-leaf-frame-pointer. Probably cleaner to just stick the option in there when it's supported, though, just to be on the safe side.
Quoted 14 lines
For the kernel, since we want frame records to be available for stack walking, we should pass -mno-omit-leaf-frame-pointer if that option is supported. If the option is supported, we know the ABI is fixed, so in that case we can omit the workaround described above.
We may still want a compiler version check, because there is a much simpler GCC change that would fix the ABI incompatibility in older branches without adding the new option:
gcc/config/riscv/riscv.cc +0
-0
gcc/config/riscv/riscv.cc riscv_save_reg_p (unsigned int regno) · line 3901
Quoted 9 lines
gcc/config/riscv/riscv.cc +2
-1@@ -3894,7 +3894,8 @@ riscv_save_reg_p (unsigned int regno) if (regno == HARD_FRAME_POINTER_REGNUM && frame_pointer_needed) return true; - if (regno == RETURN_ADDR_REGNUM && crtl->calls_eh_return) + if (regno == RETURN_ADDR_REGNUM && (crtl->calls_eh_return + || frame_pointer_needed)) return true; /* If this is an interrupt handler, then must save extra registers. */
IMO we should try backporting the option to GCC-13, that's a cleaner user interface.
Quoted 24 lines
[1]:
https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-cc.adoc#frame-pointer-convention
[2]: https://github.com/riscv-non-isa/riscv-elf-psabi-doc/issues/18
[3]:
https://github.com/gcc-mirror/gcc/commit/39663298b5934831a0125e12f113ebd83248c3beRegards,
SamuelQuoted 15 lines
arch/riscv/kernel/stacktrace.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/riscv/kernel/stacktrace.c +1
-1@@ -55,7 +55,7 @@ void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,/* Unwind stack frame */ frame = (struct stackframe *)fp -1 ; sp = fp; -if (regs && (regs->epc == pc) && (frame->fp &0x7 )) { +if (regs && regs->epc == pc) { fp = frame->ra;
Quoted 8 lines
arch/riscv/kernel/stacktrace.c void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs, · line 61
Quoted 2 lines
arch/riscv/kernel/stacktrace.c +0
-0pc = regs->ra; }else {_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv
_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv
Thanks for taking your time to review the patch!
Quoted 4 lines · Samuel Holland
A better check would be based on
frame->ra, which is always pushed. For non-leaf functions,frame->rais a kernel text address. For leaf functions,frame->rais a stack address. So checking ifframe->rais within the current stack (betweenlowandhigh) should be sufficient to detect this case.
I've came up with slightly different approach. Checking that frame->ra is not a kernel text address seems like a more compact solution. Palmer, Samuel, what do you think?
arch/riscv/kernel/stacktrace.c +2 -1
@@ -55,7 +55,8 @@ void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,/* Unwind stack frame */ frame = (struct stackframe *)fp -1 ; sp = fp; -if (regs && (regs->epc == pc) && (frame->fp &0x7 )) { +if (regs && (regs->epc == pc) && !__kernel_text_address (frame->ra)) { +/* We hit function where ra is not saved on the stack */ fp = frame->ra; pc = regs->ra; }else {
Quoted 5 lines · Palmer Dabbelt
I was just playing with this, looks like GCC is treating tail-call-only functions as leaf functions. So I think whatever workaround we come up with loses the tail-call-only function in the backtrace, not sure if there's any way we can work around that as there's essentially no frame remaining for it.
Yes, we definitely will miss some functions. But at least we can observe underlying call stack in older compiler versions.
Quoted 22 lines · Palmer Dabbelt
Quoted 22 lines
For the kernel, since we want frame records to be available for stack walking, we should pass -mno-omit-leaf-frame-pointer if that option is supported. If the option is supported, we know the ABI is fixed, so in that case we can omit the workaround described above.
We may still want a compiler version check, because there is a much simpler GCC change that would fix the ABI incompatibility in older branches without adding the new option:
gcc/config/riscv/riscv.cc +2
-1@@ -3894,7 +3894,8 @@ riscv_save_reg_p (unsigned int regno) if (regno == HARD_FRAME_POINTER_REGNUM && frame_pointer_needed) return true; - if (regno == RETURN_ADDR_REGNUM && crtl->calls_eh_return) + if (regno == RETURN_ADDR_REGNUM && (crtl->calls_eh_return + || frame_pointer_needed)) return true;
Quoted 4 lines
gcc/config/riscv/riscv.cc riscv_save_reg_p (unsigned int regno) · line 3901
Quoted 1 line
gcc/config/riscv/riscv.cc +0
-0/* If this is an interrupt handler, then must save extra registers. */IMO we should try backporting the option to GCC-13, that's a cleaner user interface.
Agreed. And what about older compiler versions? Is that a good idea to move unwinding logic into the separate function which can have different implementations (via conditional compilation) depending on the compiler version?
Now speaking about the implementation. I'm not sure where -mno-omit-leaf-frame-pointer flag should be added. Do you have any suggestions for a proper way to do it or maybe related examples? Also I can discuss it with Masahiro.
Signature · 7 lines
Best regards, Matthew Bystrin <[email protected]> _______________________________________________ linux-riscv mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-riscv