dtrace: Fix RISC-V user stack unwinder

[jrtc27]

Summary

Firstly, oldfp was unitialised, as discovered in f222a6b88614, so start
off pointing at the current frame pointer like other architctures.

Secondly, the actual unwind logic was copied from AArch64 which follows
the peculiar AACPS (where, unlike typical RISC architectures, its frame
pointer follows an x86/stack machine-like convention where the frame
pointer points at the bottom of the frame record, not the top). Delete
the pointless riscv_frame struct and fix this.

[mjg]

Drop me from 'reported by'.

I have no basis to comment on the patch.

[jrtc27]

In D28054#626850, @mjg wrote:

Drop me from 'reported by'.

I have no basis to comment on the patch.

Well, you reported the compile error, but sure, will do.

[kp]

That at the very least fixes the build error, so on those grounds alone we should commit this.

However, I do see a panic with dtrace -n 'syscall:freebsd:write:entry { ustack(); }'. I've not yet dug into it to see if it's a direct result of this. I also see that stack() doesn't appear to work and I'm pretty sure that it used to.

root@pkgbuilder:~ # dtrace -n 'syscall:freebsd:write:entry { ustack(); }' &
[1] 718
root@pkgbuilder:~ # dtrace: description 'syscall:freebsd:write:entry ' matched 1 probe
t[0] == 0x0000000000000007
t[1] == 0xffffffc0d282e4bc
t[2] == 0x000000000000000a
t[3] == 0xffffffc0002ccb52
t[4] == 0x0000000040941600
t[5] == 0xfffffffffffffe93
t[6] == 0x0a3d70a3d70a3d71
s[0] == 0x0000000000000013
s[1] == 0x0000000000000011
s[2] == 0xffffffc0d2a00018
s[3] == 0x0000000000000014
s[4] == 0x0000000000000011
s[5] == 0x0000000000000001
s[6] == 0xffffffc0d2807000
s[7] == 0x0000000000000001
s[8] == 0x0000004000000001
s[9] == 0x0000000040242566
s[10] == 0xffffffc0d2a00020
s[11] == 0xffffffd13ccff600
a[0] == 0x0000000000000009
a[1] == 0xffffffc0d2807000
a[2] == 0xffffffc0764a0c50
a[3] == 0x0000000000000000
a[4] == 0xffffffc0d2807000
a[5] == 0x0000000000004ed6
a[6] == 0xffffffd003fac000
a[7] == 0xffffffc0d2a00000
ra == 0xffffffc0d2816fba
sp == 0xffffffc0764a0830
gp == 0x0000000000000001
tp == 0x0000000000000000
sepc == 0xffffffc0d282d870
sstatus == 0x0000000000004100
panic: Fatal page fault at 0xffffffc0d282d870: 0x00000000000009
cpuid = 0
time = 1610206153
KDB: stack backtrace:
db_trace_self() at db_trace_self
db_fetch_ksymtab() at db_fetch_ksymtab+0x170
kdb_backtrace() at kdb_backtrace+0x2c
vpanic() at vpanic+0x144
panic() at panic+0x26
do_trap_supervisor() at do_trap_supervisor+0x504
do_trap_supervisor() at do_trap_supervisor+0x64
cpu_exception_handler_supervisor() at cpu_exception_handler_supervisor+0x68
--- exception 13, tval = 0x9
KDB: enter: panic
[ thread pid 710 tid 100047 ]
Stopped at      kdb_enter+0x4c: sd      zero,0(a0)
db>

[jrtc27]

In D28054#627033, @kp wrote:

That at the very least fixes the build error, so on those grounds alone we should commit this.

However, I do see a panic with dtrace -n 'syscall:freebsd:write:entry { ustack(); }'. I've not yet dug into it to see if it's a direct result of this. I also see that stack() doesn't appear to work and I'm pretty sure that it used to.

root@pkgbuilder:~ # dtrace -n 'syscall:freebsd:write:entry { ustack(); }' &
[1] 718
root@pkgbuilder:~ # dtrace: description 'syscall:freebsd:write:entry ' matched 1 probe
t[0] == 0x0000000000000007
t[1] == 0xffffffc0d282e4bc
t[2] == 0x000000000000000a
t[3] == 0xffffffc0002ccb52
t[4] == 0x0000000040941600
t[5] == 0xfffffffffffffe93
t[6] == 0x0a3d70a3d70a3d71
s[0] == 0x0000000000000013
s[1] == 0x0000000000000011
s[2] == 0xffffffc0d2a00018
s[3] == 0x0000000000000014
s[4] == 0x0000000000000011
s[5] == 0x0000000000000001
s[6] == 0xffffffc0d2807000
s[7] == 0x0000000000000001
s[8] == 0x0000004000000001
s[9] == 0x0000000040242566
s[10] == 0xffffffc0d2a00020
s[11] == 0xffffffd13ccff600
a[0] == 0x0000000000000009
a[1] == 0xffffffc0d2807000
a[2] == 0xffffffc0764a0c50
a[3] == 0x0000000000000000
a[4] == 0xffffffc0d2807000
a[5] == 0x0000000000004ed6
a[6] == 0xffffffd003fac000
a[7] == 0xffffffc0d2a00000
ra == 0xffffffc0d2816fba
sp == 0xffffffc0764a0830
gp == 0x0000000000000001
tp == 0x0000000000000000
sepc == 0xffffffc0d282d870
sstatus == 0x0000000000004100
panic: Fatal page fault at 0xffffffc0d282d870: 0x00000000000009
cpuid = 0
time = 1610206153
KDB: stack backtrace:
db_trace_self() at db_trace_self
db_fetch_ksymtab() at db_fetch_ksymtab+0x170
kdb_backtrace() at kdb_backtrace+0x2c
vpanic() at vpanic+0x144
panic() at panic+0x26
do_trap_supervisor() at do_trap_supervisor+0x504
do_trap_supervisor() at do_trap_supervisor+0x64
cpu_exception_handler_supervisor() at cpu_exception_handler_supervisor+0x68
--- exception 13, tval = 0x9
KDB: enter: panic
[ thread pid 710 tid 100047 ]
Stopped at      kdb_enter+0x4c: sd      zero,0(a0)
db>

Hm, seems strange that KDB's unwinder didn't unwind further. What's 0xffffffc0d282d870? My guess is it's dtrace_fuword64_nocheck; that function isn't enabling user access so will fault on trying to dereference a user address (and even then has no provision for handling a page fault), so I contest that it could ever have worked. I don't understand how the same problem doesn't occur on arm64 though, its functions look equally under-baked.

[kp]

In D28054#627186, @jrtc27 wrote:

What's 0xffffffc0d282d870? My guess is it's dtrace_fuword64_nocheck;

That appears to be the case, yes. dtrace.ko is loaded at 0xffffffc0d2808000, dtrace_fuword64_nocheck is 0x25870 into the module, so that's at 0xFFFFFFC0D282D870.

[jrtc27]

In D28054#627282, @kp wrote:

In D28054#627186, @jrtc27 wrote:

What's 0xffffffc0d282d870? My guess is it's dtrace_fuword64_nocheck;

That appears to be the case, yes. dtrace.ko is loaded at 0xffffffc0d2808000, dtrace_fuword64_nocheck is 0x25870 into the module, so that's at 0xFFFFFFC0D282D870.

I just tried it without this patch (just using #pragma to suppress the warning) and I see the page fault, so this isn't a regression. It looks like arm64 works because the equivalent of RISC-V's SUM/x86's SMAP, PAN (Privileged Access Never), was part of v8.1, and things like the RPi4 use a Cortex A72 that's only v8.0 (later versions would just get empty stack traces due to the details below).

I'll take a look at why it's getting a bogus address, and fix the fu* functions to enter/exit user access, but they're supposed to not be fatal even then it seems; dtrace_trap tries to catch faults, but it's only handling access faults, leaving page faults to be fatal. I guess technically we would want to try pmap_fault_fixup/vm_fault_trap first rather than immediately suppressing it, but even amd64 doesn't do that, it just marks it as faulting, and that's probably a good idea in case you're tracing the VM subsystem (or I/O if swap is involved), so it's not really feasible to do and is just a best effort (though you probably _could_ condition it on whether you're doing user or kernel accesses).

[jrtc27]

Ok, so *without* this patch I see a fatal page fault for a small integer address, but *with* this patch I see it attempting (but failing due to the buggy dtrace_fuword64 implementation) to read from a sensible address:

root@freebsd-riscv64:~ # dtrace -n 'syscall:freebsd:write:entry { ustack(); }'
dtrace: description 'syscall:freebsd:write:entry ' matched 1 probe

^Ct[0] == 0x0000000000000007
t[1] == 0xffffffc09882e65c
t[2] == 0x00000000402d8aa0
t[3] == 0xffffffc0002cd4fa
t[4] == 0x0000000000400000
t[5] == 0x0000000000001000
t[6] == 0x0000000000000040
s[0] == 0x0000000000000013
s[1] == 0x00000000402d6458
s[2] == 0xffffffc098a00018
s[3] == 0x0000000000000014
s[4] == 0x00000000402d6458
s[5] == 0x0000000000000001
s[6] == 0xffffffc098807000
s[7] == 0x0000000000000001
s[8] == 0x0000004000000001
s[9] == 0x0000000040234ef6
s[10] == 0xffffffc098a00020
s[11] == 0xffffffd05e986600
a[0] == 0x00000000402d6450
a[1] == 0xffffffc098807000
a[2] == 0xffffffc000dc6c50
a[3] == 0x0000000000000000
a[4] == 0xffffffc098807000
a[5] == 0x00000000000014f3
a[6] == 0xffffffd01bc10800
a[7] == 0xffffffc098a00000
ra == 0xffffffc09881704a
sp == 0xffffffc000dc6830
gp == 0xffffffc0007924c0
tp == 0xffffffc0835c5b80
sepc == 0xffffffc09882da10
sstatus == 0x8000000000006100
panic: Fatal page fault at 0xffffffc09882da10: 0x000000402d6450
cpuid = 0
time = 1610258833
KDB: stack backtrace:
db_trace_self() at db_trace_self
db_fetch_ksymtab() at db_fetch_ksymtab+0x170
kdb_backtrace() at kdb_backtrace+0x2c
vpanic() at vpanic+0x144
panic() at panic+0x26
do_trap_supervisor() at do_trap_supervisor+0x504
do_trap_supervisor() at do_trap_supervisor+0x64
cpu_exception_handler_supervisor() at cpu_exception_handler_supervisor+0x68
--- exception 13, tval = 0x402d6450
KDB: enter: panic
[ thread pid 749 tid 100051 ]
Stopped at      kdb_enter+0x4c: sd      zero,0(a0)
db>

So I think fixing dtrace_fu* and dtrace_trap should be sufficient for things to work.

[markj]

In D28054#627430, @jrtc27 wrote:

So I think fixing dtrace_fu* and dtrace_trap should be sufficient for things to work.

Do you intend to make further changes to this particular diff?

[mhorne]

Looking at other implementations, I believe oldfp should be updated on each loop iteration.

Otherwise, the change seems correct to me, assuming the discussed issues will be addressed separately. I'd like to see the build fix committed soon.

[jrtc27]

In D28054#629246, @mhorne wrote:

Looking at other implementations, I believe oldfp should be updated on each loop iteration.

Otherwise, the change seems correct to me, assuming the discussed issues will be addressed separately. I'd like to see the build fix committed soon.

Oh probably. I have a bunch of other fixes locally, I just need to clean them up and put them for review.

[mhorne]

FYI I handled the required follow-up changes discussed above in D38298, D38299. This fix can go in as-is, and I'll handle the commit if preferred.