DTrace: Add an invariant to check for recursion in dtrace_probe()
ClosedPublic
Actions

Authored by domagoj.stolfa_gmail.com on Mar 26 2018, 11:23 PM.

Details

Reviewers

gnn
markj

Group Reviewers

DTrace

Commits

rS332364: Assert that dtrace_probe() doesn't re-enter itself.

Summary

dtrace_probe() is a function that can be called in many sensitive contexts and relies on never recursing. This is because if there is a recursive call to dtrace_probe(), there will either be an ordering violation or there is a programmer error and a function was instrumented that should not have been.

Currently, when the latter happens the machine locks up due to continuously recursing in the kernel and it can be difficult to debug. This patch adds a new flag in DTrace-specific thread options that denotes recursion and asserts that we are not recursing unless dtrace_probe() is called with dtrace_probeid_error.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

domagoj.stolfa_gmail.com created this revision.Mar 26 2018, 11:23 PM

Herald added 1 blocking reviewer(s): gnn. · View Herald TranscriptMar 26 2018, 11:23 PM

Herald added a subscriber: imp. · View Herald Transcript

domagoj.stolfa_gmail.com edited the summary of this revision. (Show Details)Mar 26 2018, 11:25 PM

Is there anything else I can do for this to land?

markj added inline comments.Apr 6 2018, 3:51 PM

sys/cddl/contrib/opensolaris/uts/common/dtrace/dtrace.c
7318 ↗	(On Diff #40775)	Maybe make a dtrace_probe_{enter,exit}() that handles this flag and interrupts.
sys/cddl/dev/dtrace/dtrace_cddl.h
66 ↗	(On Diff #40775)	This isn't really the right place. Note the union with _td_dtrace_ft, which is supposed to contain fasttrap flags, and note also that u_long is 32-bits on ILP32 platforms. You can pack the new field after td_dtrace_sig. This union is kind of weird. I prefer the FreeBSD model where we just have an explicit flags variable. Changing that is out of scope for this change though. dtrace_wouldrecurse isn't a great name IMO, as it's more specific than it needs to be. I'd call it td_dtrace_in_probe or something like that.

Address comments by @markj.

domagoj.stolfa_gmail.com marked 2 inline comments as done.Apr 6 2018, 4:51 PM

domagoj.stolfa_gmail.com removed 1 blocking reviewer(s): gnn.

Herald added 1 blocking reviewer(s): gnn. · View Herald TranscriptApr 6 2018, 4:51 PM

Looks ok to me modulo some pedantic nitpicking.

sys/cddl/contrib/opensolaris/uts/common/dtrace/dtrace.c
7265 ↗	(On Diff #41189)	I prefer dtrace_probe_enter(), as it's only for use by dtrace_probe().
7267 ↗	(On Diff #41189)	There should be a newline after the declaration.
7271 ↗	(On Diff #41189)	I don't think this comment is very useful: it's pretty standard for assertions to be compiled out of non-debug builds in any code base.
7282 ↗	(On Diff #41189)	You can set this before disabling interrupts.

markj added inline comments.Apr 6 2018, 5:04 PM

sys/cddl/contrib/opensolaris/uts/common/dtrace/dtrace.c
7282 ↗	(On Diff #41189)	No, I'm wrong. The thread might be interrupted after setting inprobe, and a function in the interrupt handling path may be instrumented.