[PATCH -mm 2/2] ptrace_untrace: fix the SIGNAL_STOP_STOPPED check

[PATCH -mm 2/2] 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.

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

--- 6.29-rc3/kernel/ptrace.c~2_FIX_STOPPED	2009-02-13 11:06:56.000000000 +0100
+++ 6.29-rc3/kernel/ptrace.c	2009-02-13 11:38:50.000000000 +0100
@@ -60,11 +60,15 @@ 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 the group stop is completed or in progress,
+		 * this thread was already counted as stopped.
+		 */
+		if (child->signal->flags & SIGNAL_STOP_STOPPED ||
+		    child->signal->group_stop_count)
 			__set_task_state(child, TASK_STOPPED);
-		} else {
+		else
 			signal_wake_up(child, 1);
-		}
 	}
 	spin_unlock(&child->sighand->siglock);
 }

Re: [PATCH -mm 2/2] ptrace_untrace: fix the SIGNAL_STOP_STOPPED check

[Roland McGrath]

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

Right, the change looks correct.

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

Neither of these should be true today as I understand the code.  So if you
have reason to think these, I must be missing something that is likely to
be relevant to reviewing this change.


Thanks,
Roland

Re: [PATCH -mm 2/2] ptrace_untrace: fix the SIGNAL_STOP_STOPPED check

[Oleg Nesterov]

On 02/19, Roland McGrath wrote:
>
> > This bug is ancient too. ptrace_untrace() must not resume the task
> > if the group stop in progress, we should set TASK_STOPPED instead.
>
> Right, the change looks correct.
>
> > 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.
>
> Neither of these should be true today as I understand the code.

Suppose we have

	int main(void)
	{
		kill(getpid(), SIGSTOP);

		for (;;)
			;
	}

If you strace this task nobody clears SIGNAL_STOP_STOPPED.

As for bookkeeping of ->group_stop_count, I think ptrace_stop() is wrong
in many ways. For example, it decrements ->group_stop_count but doesn't
set SIGNAL_STOP_STOPPED if it becomes zero.

Oleg.