Page MenuHomeFreeBSD

Make stack_save*() more robust on MIPS.
ClosedPublic

Authored by jhb on Nov 24 2020, 9:48 PM.

Details

Summary
  • Validate any stack addresses read from against td_kstack before reading. If an unwind operation would attempt to read outside the bounds of td_kstack, abort the unwind instead.
  • For stack_save_td(), don't use the PC and SP from the current thread, instead read the PC and SP from pcb_context[].
  • For stack_save(), use the current PC and SP of the current thread, not the values from pcb_regs (the horribly named td_frame of the outermost trapframe). The result was that stack_trace() never logged _any_ kernel frames but only the frame from the saved userspace registers on entry from the kernel.
  • Inline the one use of stack_register_fetch().
  • Add a VALID_PC() helper macro and simplify types to remove excessive casts in stack_capture().
  • Fix stack_capture() to work on compilers written in this century. Don't treat function epilogues as function prologues by skipping additions to SP when searching for a function start.
  • Add some comments to stack_capture() and fix some style bugs.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jhb requested review of this revision.Nov 24 2020, 9:48 PM
jhb created this revision.

LGTM.

sys/mips/mips/stack_machdep.c
102 ↗(On Diff #79954)

This won't work with stack frames > 32K since it won't use an immediate add, but those shouldn't exist anywhere in the kernel.
Maybe add a comment to the switch that this only checks for addi since we don't handle stack frames over 32k?

This revision is now accepted and ready to land.Dec 1 2020, 11:24 AM
sys/mips/mips/stack_machdep.c
102 ↗(On Diff #79954)

This won't work with stack frames > 32K since it won't use an immediate add, but those shouldn't exist anywhere in the kernel.
Maybe add a comment to the switch that this only checks for addi since we don't handle stack frames over 32k?

The kstack itself is fixed at 8k on MIPS (or 16k in CheriBSD), so it can't be that large anyway. I think we still can see daddi in an n64 kernel since it uses the 64-bit register instead of 32-bit? (I've seen daddi instructions in disassembly when debugging this code even.)

sys/mips/mips/stack_machdep.c
102 ↗(On Diff #79954)

Seems like GCC always has the ADDI even for large frames, but clang doesn't: https://godbolt.org/z/6W5r9Y

Anyway this doesn't really matter since such functions should not exist in the kernel anyway.

This revision was automatically updated to reflect the committed changes.