[PATCH] utrace_stop: fix UTRACE_DETACH race

[Oleg Nesterov]

On 08/30, Roland McGrath wrote:
>
> > This case is not unlikely, for example ENGINE_STOP is cleared after the
> > previous wakeup. We are running, if ENGINE_STOP is set, UTRACE_STOP was
> > used and utrace_reset() was called after that. Otherwise, _perhaps_ it
> > was cleared by UTRACE_DETACH.
>
> What we want is to change things so that this case *is* unlikely.  That is,
> we really want to avoid any extra O(n) work such as utrace_reset calls.  It
> should never be common to do one of those we don't really want to do.  And,
> all else being equal, when some extra O(n) work is unavoidably required,
> IMHO it's better that it be in the tracer than in the tracee.

Yes, agreed.

> I took a different tack and committed these:
>
> 	96fe3cc Revert "utrace_stop: fix UTRACE_DETACH race"
> 	d1a14ce utrace: change UTRACE_STOP bookkeeping
> 	64a8ca3 utrace_reset cleanup

I think this should work, but I'd like to re-read these changes carefully,
this all is subtle. And, at first glance, we need something like the patch
below.

Because now utrace_control(UTRACE_DETACH) does

	case UTRACE_DETACH:
		...	
		if (engine_wants_stop(engine))
			resume = true;

OK, we need to call utrace_reset() to recalc ->utrace_flags. But utrace_reset()
assumes it is safe to play with utrace->attached list, now this is not true.
IOW, with this patch utrace_control(UTRACE_DETACH)->utrace_reset() can race
with REPORT_CALLBACKS/etc which does list_for_each_entry(->attached).
(minor, but perhaps resume should be renamed to "bool reset")

"task != current" before utrace_wakeup() doesn't look right too, I think we
should check ->stopped instead. Harmless, but otherwise we can call wakeup()
while it is not needed.

Oleg.

--- a/kernel/utrace.c
+++ b/kernel/utrace.c
@@ -722,11 +722,12 @@ static bool utrace_reset(struct task_str
 	__releases(utrace->lock)
 {
 	struct utrace_engine *engine, *next;
+	bool xxx = (utrace->stopped || task == current);
 	unsigned long flags = 0;
 	LIST_HEAD(detached);
 
-	splice_attaching(utrace);
-
+	if (xxx)
+		splice_attaching(utrace);
 	/*
 	 * Update the set of events of interest from the union
 	 * of the interests of the remaining tracing engines.
@@ -734,7 +735,7 @@ static bool utrace_reset(struct task_str
 	 * We'll collect them on the detached list.
 	 */
 	list_for_each_entry_safe(engine, next, &utrace->attached, entry) {
-		if (engine->ops == &utrace_detached_ops) {
+		if (xxx && engine->ops == &utrace_detached_ops) {
 			engine->ops = NULL;
 			list_move(&engine->entry, &detached);
 		} else {
@@ -765,7 +766,7 @@ static bool utrace_reset(struct task_str
 		 */
 		utrace->interrupt = utrace->report = utrace->signal_handler = 0;
 
-	if (!(flags & ENGINE_STOP) && task != current)
+	if (!(flags & ENGINE_STOP) && utrace->stopped)
 		/*
 		 * No more engines want it stopped.  Wake it up.
 		 */