Re: [PATCH 3/3] ptrace_untrace: fix the SIGNAL_STOP_STOPPED check

[Oleg Nesterov <oleg@redhat.com>]

On 02/08, Roland McGrath wrote:
>
> Yes, I believe this is correct.  It matches the flip side of the
> bookkeeping where we adjust group_stop_count when going into TASK_TRACED
> (ptrace_stop).  I think it warrants a comment with your change, saying that
> treating group_stop_count as "we should be already stopped" is consistent
> with decrementing an active group_stop_count when we enter TASK_TRACED.

Yes, I tried to make the comment, but failed.

Because we have another case. The group stop is in progress, and some
thread T does do_signal_stop()->finish_stop(). It is TASK_STOPPED.
Now we do PTRACE_ATTACH + PTRACE_DETACH. And the second sys_ptrace()
changes T->state to TASK_TRACED.

And. It it also possible that we ptrace the single sub-thread, then
the group stop starts. The first thread which enters do_signal_stop()
will not count the TASK_TRACED child, so it should stay stopped.

> > 	- if the process/thread was traced, SIGNAL_STOP_STOPPED
> > 	  does not necessary means this thread group is stopped.
> >
> > 	- ptrace breaks the bookkeeping of ->group_stop_count.
>
> SIGNAL_STOP_STOPPED is only set when all live threads in the group are in
> either TASK_TRACED or TASK_STOPPED.  PTRACE_DETACH respects this and this
> it stopped.  However, PTRACE_CONT et al (ptrace_resume) do not respect it
> and can resume an individual thread regardless of SIGNAL_STOP_STOPPED.
> That's what you mean here, right?

Yes. (and of course, we don't even need threads to hit this problem).

> > (the comment above ptrace_untrace() doesn't look exactly right too).
>
> How so?

Perhaps this is just my misunderstanding, but

	/*
	 * Turn a tracing stop into a normal stop now, since with no tracer there
	 * would be no way to wake it up with SIGCONT or SIGKILL.

This looks as if we always do /TRACED/STOPPED/ unconditionally.

	                                                           If there was a
	 * signal sent that would resume the child, but didn't because it was in
	 * TASK_TRACED, resume it now.

No, we resume it not because it may have signals, and we don't even check
it has pending signals.

	 * Requires that irqs be disabled.
	 */

this is correct ;)

Oleg.