Page MenuHomeFreeBSD

Check for invalid pc in unwind_frame
AbandonedPublic

Authored by mhorne on Aug 9 2020, 4:42 PM.

Details

Reviewers
jhb
jrtc27
Summary

Rather than expecting callers to validate the PC of the returned frame,
we are better off just returning an error. This helps
db_trace_stack_cmd, which would unwind one frame too far for proc0
because it did not do this.

Test Plan

Before:

db> bt 0
Tracing pid 0 tid 100000 td 0xffffffc000726b00
sched_switch() at sched_switch+0x572
mi_switch() at mi_switch+0x186
sleepq_wait() at sleepq_wait+0x170
sleepq_timedwait() at sleepq_timedwait+0x3c
_sleep() at _sleep+0x23c
swapper() at swapper+0xac
mi_startup() at mi_startup+0x296
_start() at _start+0x18c
(null)() at -0x4

After:

db> bt 0
Tracing pid 0 tid 100000 td 0xffffffc000726b00
sched_switch() at sched_switch+0x572
mi_switch() at mi_switch+0x186
sleepq_wait() at sleepq_wait+0x170
sleepq_timedwait() at sleepq_timedwait+0x3c
_sleep() at _sleep+0x23c
swapper() at swapper+0xac
mi_startup() at mi_startup+0x296
_start() at _start+0x18c

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 32892
Build 30294: arc lint + arc unit

Event Timeline

mhorne requested review of this revision.Aug 9 2020, 4:42 PM
mhorne created this revision.
jrtc27 requested changes to this revision.Aug 9 2020, 4:50 PM
jrtc27 added inline comments.
sys/riscv/riscv/stack_machdep.c
59–61

The error code isn't checked, so now this will keep unwinding. An existing bug as frame->fp could have been invalid (though presumably the above ad-hoc if is meant to check that up-front?), but made more apparent by this change.

This revision now requires changes to proceed.Aug 9 2020, 4:50 PM

Check the error from unwind_frame.

I guess this is ok. Nominally if you could have a user pc for things like faults and exceptions, but those all require a custom unwinder in DDB anyway. unwind_frame() doesn't handle them. If we fixed unwind_frame() to learn about trap frames and unwind across them, then you would perhaps not want this check in unwind_frame(). Is this still needed after the changes in D26016?

In D26015#577980, @jhb wrote:

I guess this is ok. Nominally if you could have a user pc for things like faults and exceptions, but those all require a custom unwinder in DDB anyway. unwind_frame() doesn't handle them. If we fixed unwind_frame() to learn about trap frames and unwind across them, then you would perhaps not want this check in unwind_frame(). Is this still needed after the changes in D26016?

I see what you mean, just because it's not a kernel PC doesn't mean it's invalid, just unsupported in our case. The other change does properly address the issue I was seeing, so let's let this be.