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

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

[Oleg Nesterov]

This bug is ancient too. ptrace_untrace() must not resume the task
if the group stop in progress, we should set TASK_STOPPED instead.

Unfortunately, we still have problems here:

	- 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.

(the comment above ptrace_untrace() doesn't look exactly right too).

Signed-off-by: Oleg Nesterov <oleg@redhat.com>

--- 6.29-rc3/kernel/ptrace.c~3_FIX_STOPPED	2009-02-08 06:22:26.000000000 +0100
+++ 6.29-rc3/kernel/ptrace.c	2009-02-08 08:52:14.000000000 +0100
@@ -60,7 +60,8 @@ static void ptrace_untrace(struct task_s
 {
 	spin_lock(&child->sighand->siglock);
 	if (task_is_traced(child)) {
-		if (child->signal->flags & SIGNAL_STOP_STOPPED)
+		if (child->signal->flags & SIGNAL_STOP_STOPPED ||
+		    child->signal->group_stop_count)
 			__set_task_state(child, TASK_STOPPED);
 		else
 			wake_up_process(child);

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

[Roland McGrath]

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.

> 	- 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?

Doing that without a SIGCONT having been generated is an anomalous way for
a debugger to behave, since it's going to eat the effects of the job
control stop.  But who knows that things they do, so this could be sticky
to fix so that SIGNAL_STOP_STOPPED really means all stopped.

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

How so?


Thanks,
Roland

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

[Oleg Nesterov]

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.

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

[Roland McGrath]

> 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.

There is no problem here.  The original TASK_STOPPED and the new
TASK_TRACED count the same as far as group-stop accounting.

> 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.

As it will, by checking for group_stop_count || SIGNAL_STOP_STOPPED.

> 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.

... until you read the next sentence that describes the other case.

> 	                                                           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.

The comment is accurate: it doesn't have anything to do with pending signals.
An ignored SIGCONT "was a signal sent that would resume the child", but is
not pending.  (Likewise a caught SIGCONT already dequeued by another thread.)
SIGNAL_STOP_STOPPED being clear is what indicates that "there was a signal
sent that would resume the child".  (It is prepare_signal() that would have
resumed the task, not any present or past state of pendingness of any signal.)

> 	 * Requires that irqs be disabled.
> 	 */
> 
> this is correct ;)

... too. ;-)

Seriously, feel free to rewrite comments so they are more unambiguous.  
But this one is not incorrect when interpreted as I do (as it was
intended), just apparently ambiguous to some eyes such as yours.
The purpose of comments is to be clear to everyone, so change may be
warranted.  But clarity of unambiguous expression is not found in
interpreting ambiguity as being unambiguously clear and wrong.


Thanks,
Roland