Page MenuHomeFreeBSD
Differential D27766

Various changes to DTrace FBT to avoid crashed on FreeBSD/arm64.
ClosedPublic
Actions

Authored by rwatson on Dec 24 2020, 5:30 PM.
Tags
Referenced Files
Unknown Object (File)
Fri, Jan 30, 10:02 AM
Unknown Object (File)
Wed, Jan 14, 12:53 PM
Unknown Object (File)
Wed, Jan 14, 11:26 AM
Unknown Object (File)
Tue, Jan 13, 10:05 AM
Unknown Object (File)
Mon, Jan 12, 2:29 PM
Unknown Object (File)
Mon, Jan 12, 1:46 PM
Unknown Object (File)
Mon, Jan 12, 1:45 PM
Unknown Object (File)
Sat, Jan 10, 4:40 PM
View All Files
Subscribers
andrew
bz
emaste
imp
jrtc27

Details

Reviewers
gnn
andrew
kp
markj
jrtc27
Commits
rG30b68ecda84e: Changes that improve DTrace FBT reliability on freebsd/arm64:
Summary

A number of changes to DTrace to improve robustness (i.e., not panic quickly) on FreeBSD/arm64:

  • Don't allow FBT to instrument the exception handlers used to implement FBT.
  • Don't allow FBT to instrument DDB, which makes debugging FBT even harder.
  • Don't allow FBT to instrument memcpy(), which may be used by the compiler to optimise parts of the DTrace implementation, as well as in exception handlers.
  • Implement a dtrace_getnanouptime(), modeled on the existing dtrace_getnanotime(), so that we don't have to block FBT instrumentation of getnanouptime().

(Note: I'm pretty sure that dtrace_gethrtime() should not use getnanouptime(), but I'll change its semantics in some later commit -- this change preserve current semantics.)

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 35679
Build 32568: arc lint + arc unit

Event Timeline

rwatson created this revision.Dec 24 2020, 5:30 PM
Herald added a reviewer: gnn. · View Herald TranscriptDec 24 2020, 5:30 PM
Herald added subscribers: emaste, andrew, imp. · View Herald Transcript
Harbormaster completed remote builds in B35679: Diff 81140.Dec 24 2020, 5:30 PM
rwatson requested review of this revision.Dec 24 2020, 5:30 PM
rwatson added projects: DTrace, arm64.Dec 24 2020, 5:31 PM
lwhsu changed the repository for this revision from rS FreeBSD src repository - subversion to rG FreeBSD src repository.Dec 24 2020, 5:32 PM
rwatson added a reviewer: andrew.Dec 24 2020, 7:32 PM
rwatson edited the summary of this revision. (Show Details)Dec 24 2020, 7:36 PM
bz added a subscriber: bz.Dec 26 2020, 2:57 PM
rwatson added a reviewer: kp.Dec 26 2020, 5:15 PM
lwhsu added a reviewer: markj.Dec 26 2020, 5:31 PM
kp accepted this revision.Dec 27 2020, 1:34 PM
andrew added inline comments.Dec 27 2020, 1:53 PM
sys/kern/kern_tc.c
1007

Should this and dtrace_getnanotime be under KDTRACE_HOOKS?

rwatson added inline comments.Jan 1 2021, 1:25 PM
sys/kern/kern_tc.c
1007

It's an interesting question. I guess I simply maintained the status quo. I'm tempted to stick with that in this change, and then fix both at once, assuming no surprises arise?

markj accepted this revision.Jan 1 2021, 8:59 PM
jrtc27 added a subscriber: jrtc27.Jan 4 2021, 1:53 AM
jrtc27 added inline comments.
sys/cddl/dev/fbt/fbt.c
169
169–170

I feel this belongs in the aarch64 fbt_provide_module_function? At least, that's where x86 puts its architecture-specific exclusions for functions (well, just one in its case) on the synchronous trap path that FBT probes themselves trigger. In theory unwind_frame also belongs there too though one could argue that all architectures should follow the same clear model and then it'd be able to be MI.

rwatson updated this revision to Diff 82018.Jan 10 2021, 9:30 AM
rwatson edited the summary of this revision. (Show Details)

Various fixes to the DTrace FBT provider on FreeBSD/arm64:

Exclude kdb_ symbols from FBT.

Add bcopy(9) to FBT exclusions for arm64.

Re-sort exclusions with more clear comments as to why they are excluded.
Harbormaster completed remote builds in B36085: Diff 82018.Jan 10 2021, 9:30 AM
rwatson updated this revision to Diff 82019.Jan 10 2021, 9:48 AM
  • Since non-aarch64 haven't required a memcpy() exclusion in the past, don't exclude it now.
  • Move arm64-specific exclusions from the generic FBT code to the arm64-specific code.

    Suggested by: @jrtc27
Harbormaster completed remote builds in B36086: Diff 82019.Jan 10 2021, 9:48 AM
rwatson marked an inline comment as done.Jan 10 2021, 9:49 AM
rwatson added inline comments.
sys/cddl/dev/fbt/fbt.c
169–170

Yes, sounds good. I've left unwind_frame() and moved the bcopy()/memcpy() exclusions, and also exception-handling exclusions. The memcpy() exclusion might be needed on other architectures too, but since it seems not to have been triggering unhappiness there, I'll move it and assume it was the lack of conventional aarch64 function prologue/epilogue that was causing issues for me previously.

rwatson updated this revision to Diff 82032.Jan 10 2021, 12:32 PM
rwatson marked an inline comment as done.

Updates to candidate commit message.

Harbormaster completed remote builds in B36087: Diff 82032.Jan 10 2021, 12:32 PM
rwatson added a comment.EditedJan 10 2021, 12:33 PM

Candidate commit message (will update Reviewed by notes as needed):

A number of changes that improve DTrace FBT reliability on freebsd/arm64:
    
  - Implement a dtrace_getnanouptime(), matching the existing
    dtrace_getnanotime(), to avoid DTrace calling out to a potentially
    instrumentable function.
  
    (These should probably both be under KDTRACE_HOOKS.  Also, it's not clear
    to me that they are correct implementations for the DTrace thread time
    functions they are used in .. fixes for another commit.)
    
  - Don't allow FBT to instrument DDB and KDB functions, as that makes it
    rather harder to debug FBT problems.
    
  - Don't allow FBT to instrument functions involved in EL1 exception handling
    that are involved in FBT trap processing: handle_el1h_sync() and
    do_el1h_sync().
  
  Prior to these changes, use of FBT on FreeBSD/arm64 rapidly led to kernel
  panics due to a blend of memory corruption and recursion within DTrace.
    
  MFC after:      2 weeks
  Reviewed by:    jrtc27, kp, markj (earlier version)
  Differential revision:  https://reviews.freebsd.org/D27766
kp accepted this revision.Jan 10 2021, 1:45 PM

Still happy with this. I'll rebase the riscv fixes on top of this once it gets committed and push them as wel.

rwatson added a reviewer: jrtc27.Jan 11 2021, 8:50 AM
andrew accepted this revision.Jan 11 2021, 1:32 PM
rwatson updated this revision to Diff 82067.Jan 11 2021, 3:38 PM

Trim exclusion of memcpy() and bcopy() as @andrew's in-progress patch to
avoid improper FBT instrumentation of functions without conventional stack
frames now solves that problem in a better way.

Harbormaster completed remote builds in B36100: Diff 82067.Jan 11 2021, 3:38 PM
This revision was not accepted when it landed; it landed in state Needs Review.Jan 11 2021, 3:45 PM
Closed by commit rG30b68ecda84e: Changes that improve DTrace FBT reliability on freebsd/arm64: (authored by rwatson). · Explain Why
This revision was automatically updated to reflect the committed changes.
rwatson added a commit: rG30b68ecda84e: Changes that improve DTrace FBT reliability on freebsd/arm64:.

Revision Contents
Changeset List

PathSize
sys/
cddl/
dev/
dtrace/
aarch64/
3 lines
arm/
3 lines
riscv/
3 lines
fbt/
26 lines
kern/
15 lines

Diff 81140

View Options

sys/cddl/dev/dtrace/aarch64/dtrace_subr.c

Loading...
View Options

sys/cddl/dev/dtrace/arm/dtrace_subr.c

Loading...
View Options

sys/cddl/dev/dtrace/riscv/dtrace_subr.c

Loading...
View Options

sys/cddl/dev/fbt/fbt.c

Loading...
View Options

sys/kern/kern_tc.c

Loading...