"utrace_stop: fix UTRACE_DETACH race"
commit 00932dbe24bd4273e6d723baa9342e7b3e248911,
> @@ -808,7 +808,25 @@ static void utrace_stop(struct task_struct *task, struct
> utrace *utrace,
> * waking us up, we must synchronize with the signal bookkeeping
> * for stop signals and SIGCONT.
> */
> +relock:
> spin_lock(&utrace->lock);
> +
> + if (unlikely(!(task->utrace_flags & ENGINE_STOP))) {
> + /*
> + * UTRACE_DETACH was used in some utrace_control()
> + * call since the last time we ran utrace_reset().
I believe the comment and "unlikely" above are not right.
Neither utrace_control(STOP) nor finish_callback(action => STOP) set
ENGINE_STOP in ->utrace_flags. They change engine->flags only, and
ENGINE_STOP can only "migrate" to ->utrace_flags after utrace_reset().
Yes, the only reason (but see below) we check ENGINE_STOP here is that
perhaps UTRACE_DETACH was used, but if we don't see ENGINE_STOP this
doesn't mean UTRACE_DETACH was used.
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.
> + enum utrace_resume_action stop = UTRACE_STOP;
> + utrace_reset(task, utrace, &stop);
> + if (stop != UTRACE_STOP)
> + return;
This doesn't look right. Note that utrace_reset() changes *action only
when flags == 0, this is only possible if there are no attached engines.
If we have another engine but engine_wants_stop() == F, stop is not
changed and we are going to spin forever.
> @@ -1083,6 +1101,16 @@ int utrace_control(struct task_struct *target,
> resume = resume || utrace_do_stop(target, utrace);
> if (!resume) {
> /*
> + * This is a marker in case utrace_stop() gets
> + * utrace->lock before utrace_reset() does,
> + * meaning UTRACE_STOP was just returned from a
> + * callback before our UTRACE_DETACH is seen.
> + * It must double-check that our now-detached
> + * engine was not the only reason to stop.
> + */
> + target->utrace_flags &= ~ENGINE_STOP;
Well, yes. But this only helps for ENGINE_STOP. Consider
utrace_control(UTRACE_STOP);
utrace_control(UTRACE_RESUME);
the tracee can stop. OK, this is not as critical as UTRACE_DETACH problem,
but still. Perhaps RESUME and INTERRUPT interrupt should clear ENGINE_STOP
too?
Oleg.
