[DTrace-devel] [PATCH 3/3] work: fix race around stopping with no -c passed in

[Nick Alcock]

The detection of a stopped DTrace when dtrace is run without -c depends
fundamentally on dtp->dt_stopped and the DTrace activity state, and this
interplay is broken (this causes, at least, failures in
test/unittest/usdt/tst.multitrace.sh).

In particular, cmd/dtrace.c calls dtrace_stop() to call END and wind down
tracing when the number of -c'd processes falls to zero, DTrace is hit
with an interrupt, or dt_work() has returned DTRACE_WORKSTATUS_DONE.
The latter only happens when a consume hook returns DTRACE_CONSUME_DONE
(which nothing in DTrace ever does), or when dtrace_status() returns
DTRACE_STATUS_EXITED or _STOPPED.

dtrace_status() does:

	if (dtp->dt_stopped)
		return DTRACE_STATUS_STOPPED;
/* ... */

	if (dt_state_get_activity(dtp) == DT_ACTIVITY_DRAINING) {
		if (!dtp->dt_stopped)
			dtrace_stop(dtp);

		return DTRACE_STATUS_EXITED;
	}

dtrace_stop() fires the END probe and sets dt_stopped.  This is the *only*
place that can set dt_stopped.

Unfortunately, dt_state_get_activity() does a fetch from the mst status in
the kernel, and that status can be adjusted by BPF at any time. In
particular hitting the END probe via some route other than dtrace_stop()
will increment the status, and if it is already DT_ACTIVITY_DRAINING (which
can be set by exit() and other things), this will increment it straight to
DT_ACTIVITY_STOPPED... and the test above fails, dtrace_stop() is never
called, dtp->dt_stopped is never set, and we stall indefinitely (until a
test timeout hits us with a SIGTERM, but that's considered a timeout-induced
test failure).

This is easily fixed by entering dtrace_stop() if the state is >= DRAINING
(so even if it is already STOPPED): if it's been called before,
dtp->dt_stopped will be set and we'll return straight away; if it hasn't
haven't we can see if the activity is STOPPED, presume that this is due to
the END probe firing already, skip explicit END invocation, and just set
dtp->dt_stopped and dtp->endedon.

My only worry is that if something *else* other than an END probe firing
causes an increment of the activity state past DRAINING to STOPPED, we'll
not execute the END probe at all, but that's no different from the
state before this commit, when we'd have failed to enter dtrace_stop()
entirely.

My only *question* is what the thing is that is calling the END probe if not
dtrace_stop(), but *something* clearly is, and adding this bit of armouring
costs nothing.

Signed-off-by: Nick Alcock <nick.alcock at oracle.com>
---
 libdtrace/dt_work.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/libdtrace/dt_work.c b/libdtrace/dt_work.c
index 69a8635849c44..22cbb82eafdf4 100644
--- a/libdtrace/dt_work.c
+++ b/libdtrace/dt_work.c
@@ -121,7 +121,7 @@ dtrace_status(dtrace_hdl_t *dtp)
 			     &dtp->dt_status[gen ^ 1]) == -1)
                 return DTRACE_STATUS_ERROR;
 
-	if (dt_state_get_activity(dtp) == DT_ACTIVITY_DRAINING) {
+	if (dt_state_get_activity(dtp) >= DT_ACTIVITY_DRAINING) {
 		if (!dtp->dt_stopped)
 			dtrace_stop(dtp);
 
@@ -329,11 +329,19 @@ dtrace_go(dtrace_hdl_t *dtp, uint_t cflags)
 int
 dtrace_stop(dtrace_hdl_t *dtp)
 {
-	if (dtp->dt_stopped)
+	int act;
+
+        if (dtp->dt_stopped)
 		return 0;
 
-	if (dt_state_get_activity(dtp) < DT_ACTIVITY_DRAINING)
+	if ((act = dt_state_get_activity(dtp)) < DT_ACTIVITY_DRAINING)
 		dt_state_set_activity(dtp, DT_ACTIVITY_DRAINING);
+	else if (act == DT_ACTIVITY_STOPPED)
+		/*
+		 * Stopped by END probe firing or similar operation.  Do not
+		 * re-invoke END.
+		 */
+		goto already_ended;
 
 	if (dtp->dt_beginendargs) {
 		int			cmd = CMD_END;
@@ -356,6 +364,7 @@ dtrace_stop(dtrace_hdl_t *dtp)
 	else
 		END_probe();
 
+already_ended:
 	dtp->dt_stopped = 1;
 	dtp->dt_endedon = dt_state_get_endedon(dtp);
 
-- 
2.43.0.272.gce700b77fd