> Yep. And utrace_reset() can be called because ->stopped == 1.
Right.
> Let me explain. Again, let's suppose D attaches engine E to the target T.
>
> T enters utrace_report_jctl() with ->stopped == 1.
>
> D calls utrace_set_events(events => 0), this removes JCTL from E->flags.
>
> D calls, say, utrace_control(UTRACE_RESUME). Since ->stopped == 1, this
> calls utrace_reset() and removes JCTL from T->utrace_flags.
Right. In the utrace-indirect code this would have reset the utrace
pointer too.
> T takes utrace->lock, clears ->stopped, and drops the lock.
In the utrace-indirect code, this part would have been harmless even in the
race case where it happened (the more likely case being that task->utrace
was cleared already before utrace_report_jctl looked at it). (That code
just had the dangling utrace pointer issue I noticed yesterday, at the end
of the function.)
But, yes, this is a problem. I think this ought to cover it:
@@ -1659,6 +1659,16 @@ void utrace_report_jctl(int notify, int what)
* longer considered stopped while we run callbacks.
*/
spin_lock(&utrace->lock);
+ /*
+ * Now that we have the lock, check in case utrace_reset() has
+ * just now cleared UTRACE_EVENT(JCTL) while it considered us
+ * safely stopped. In that case, we should not touch ->stopped
+ * and have nothing else to do.
+ */
+ if (unlikely(!(task->utrace_flags & UTRACE_EVENT(JCTL)))) {
+ spin_unlock(&utrace->lock);
+ return;
+ }
utrace->stopped = 0;
utrace->report = 0;
spin_unlock(&utrace->lock);
> > 2. utrace_set_events(target, UTRACE_EVENT(JCTL)) was called when target was
> > already in TASK_STOPPED (and really stopped, or at least got past
> > tracehook_notify_jctl before JCTL was set). It sets ->stopped before
> > adding JCTL to ->utrace_flags,
>
> Yes, thanks. I missed this.
I feel I should also point out the case where exit_signals() calls
tracehook_notify_jctl, because I just noticed it. I don't think that path
existed the last time I thought seriously about the utrace_report_jctl
logic. (This is not a #3 in that list, but in general is another path we
need to keep in mind here.)
> Yes sure. I wasn't clear. I meant, what if SIGNAL_STOP_STOPPED is not set?
> This doesn't mean we don't need __set_current_state(TASK_STOPPED), it is
> possible that the group-stop is in progress and ->group_stop_count != 0.
Right.
Thanks,
Roland
