jerpelea opened a new pull request, #19621:
URL: https://github.com/apache/nuttx/pull/19621
## Summary
The RISC-V implementation of up_backtrace()
(arch/risc-v/src/common/riscv_backtrace.c) has several correctness issues under
CONFIG_BUILD_KERNEL. This PR fixes three independent, compounding problems
found while debugging dumpstack on rv-virt:knsh_romfs.
Problem 1: xcp.ustkptr is stale and no longer maintained
ustkptr was introduced in 77e90d9c875 ("RISC-V: Include support for kernel
stack"). The intent was: on syscall entry, update ustkptr to the user-mode SP;
on syscall exit, reset it back to NULL.
76e5204a806 ("risc-v/backtrace: correct stack pointer if enable
ARCH_KERNEL_STACK backtrace") then started consuming ustkptr in up_backtrace(),
using *(ustkptr + 1) as the starting frame pointer for a task that is currently
inside a syscall (running on the kernel stack instead of the user stack).
However, e6973c764cd ("riscv/syscall: Optimize user service call
performance") appears to have dropped the maintenance of ustkptr on the syscall
entry/exit path. As a result, ustkptr is only ever set once, at task creation
time, and never updated afterwards — the != NULL check in up_backtrace() is
effectively dead, and the value it reads is always the stale "initial" one.
This alone causes dumpstack to return zero frames for any user-mode task,
because ustkptr + 1 no longer points anywhere near a valid frame — it's out of
range of the user stack entirely.
Separately, I have to admit I don't fully understand what *(ustkptr + 1) was
supposed to compute in the first place — it doesn't correspond to any
ABI-defined frame pointer slot I can find. If I'm missing something about how
76e5204a806 was verified at the time, I'd appreciate clarification; otherwise
this looks like it was never exercised against a real out-of-range case.
Fix: instead of relying on ustkptr, use the existing TCB_FLAG_SYSCALL flag
together with xcp.sregs[REG_FP] / xcp.sregs[REG_EPC] (the syscall entry's saved
register context, already maintained elsewhere) to detect "this task is
currently inside a syscall" and get its correct frame pointer/PC.
Problem 2: cross-tcb backtrace reads across the wrong address environment
Independently of problem 1, when dumpstack targets a different task's TCB
(CONFIG_ARCH_ADDRENV, e.g. knsh_romfs), backtrace()'s fp/ra chain lives in the
target task's address space, while buffer (where results are written) belongs
to the caller. The RISC-V backtrace() never switched address environments at
all, so the fp-chain walk dereferenced the target task's stack addresses using
the caller's page tables — reading whatever physical page happens to be mapped
at that virtual address in the caller's own environment, not the target's
actual stack contents.
Fix: backtrace() now takes an optional addrenv parameter and wraps only the
two frame-chain reads (ra = *(fp - 1), next_fp = *(fp - 2)) in
addrenv_select()/addrenv_restore(). The write into buffer[i++] = ra stays
outside that window, since buffer is the caller's own memory and must be
written using the caller's own address environment — switching addrenv around
that write as well would corrupt/lose the result (confirmed by hardware
testing; an earlier, wider-scoped attempt that included the buffer write inside
the switch produced all-zero backtraces).
Problem 3: leaf syscall wrapper functions don't save ra on the stack
Even with problems 1 and 2 fixed, backtraces crossing a syscall boundary can
still be broken. sys_call0()..sys_call6() in arch/risc-v/include/syscall.h are
leaf functions as far as the compiler can tell (they never call another C
function themselves), so even with CONFIG_FRAME_POINTER
(-fno-omit-frame-pointer), the compiler only bothers to save s0 (the frame
pointer) around the ecall — it never allocates or writes a stack slot for ra,
because as far as the compiler's leaf-function analysis is concerned, ra is
never clobbered inside the function body and ret (which is jalr x0, 0(x1))
works correctly straight out of the register.
The problem is that backtrace()'s fp-chain walk assumes the standard
prologue layout (ra at fp - 1, saved s0 at fp - 2). In these leaf wrappers, s0
ends up at fp - 1 instead (since there's no ra slot to begin with), and fp - 2
is simply never written — uninitialized stack garbage. Disassembly of waitpid()
before the fix:
c000ba08 <waitpid>:
c000ba08: 1141 addi sp,sp,-16
c000ba0a: c622 sw s0,12(sp)
c000ba0c: 872a mv a4,a0
c000ba0e: 0800 addi s0,sp,16
c000ba10: 87ae mv a5,a1
c000ba12: 86b2 mv a3,a2
c000ba14: 02300513 li a0,35
c000ba18: 85ba mv a1,a4
c000ba1a: 863e mv a2,a5
c000ba1c: 00000073 ecall
c000ba20: 0001 nop
c000ba22: 4432 lw s0,12(sp)
c000ba24: 0141 addi sp,sp,16
c000ba26: 8082 ret
Note s0 is saved at sp+12 (i.e. fp - 1), and ra is never spilled at all.
backtrace() reading *(fp - 1) here gets the caller's old s0, not a return
address; *(fp - 2) gets whatever garbage was left on the stack from a previous
call.
Fix: force ra to be spilled/reloaded around the ecall by adding it to the
clobber list, gated behind a new RISCV_ECALL_CLOBBERS macro:
#if defined(CONFIG_FRAME_POINTER) && defined(CONFIG_SCHED_BACKTRACE)
# define RISCV_ECALL_CLOBBERS "memory", "ra"
#else
# define RISCV_ECALL_CLOBBERS "memory"
#endif
This only takes effect when both CONFIG_FRAME_POINTER (the fp-chain is
structurally meaningful at all) and CONFIG_SCHED_BACKTRACE (the only consumer
of up_backtrace()'s fp-chain walk) are enabled — otherwise there is no fp-chain
consumer to fix up for, and the extra spill/reload would be pure overhead.
Disassembly of waitpid() after the fix:
c000ba9c <waitpid>:
c000ba9c: 1141 addi sp,sp,-16
c000ba9e: c422 sw s0,8(sp)
c000baa0: c606 sw ra,12(sp)
c000baa2: 0800 addi s0,sp,16
c000baa4: 872a mv a4,a0
c000baa6: 87ae mv a5,a1
c000baa8: 86b2 mv a3,a2
c000baaa: 02300513 li a0,35
c000baae: 85ba mv a1,a4
c000bab0: 863e mv a2,a5
c000bab2: 00000073 ecall
c000bab6: 0001 nop
c000bab8: 40b2 lw ra,12(sp)
c000baba: 4422 lw s0,8(sp)
c000babc: 0141 addi sp,sp,16
c000babe: 8082 ret
ra and s0 are now both correctly saved at fp - 1/fp - 2, matching what
backtrace() expects. This adds two instructions and one stack word of overhead
to each sys_callN() wrapper, but only in configurations where both gating
options above are enabled.
Known follow-up (not part of this PR)
While debugging this, I found that AArch64's up_backtrace() under
CONFIG_BUILD_KERNEL appears to have a similar class of problem. I'd like to
hold off on that until this PR is reviewed/confirmed, and will submit it as a
separate follow-up PR.
## Impact
RELEASE
## Testing
CI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
