[PATCH 1/3] ptrace_detach: the wrong wakeup breaks the ERESTARTxxx logic

[PATCH 1/3] ptrace_detach: the wrong wakeup breaks the ERESTARTxxx logic

[Oleg Nesterov]

Another ancient bug. Consider this trivial test-case,

	int main(void)
	{
		int pid = fork();

		if (pid) {
			ptrace(PTRACE_ATTACH, pid, NULL, NULL);
			wait(NULL);
			ptrace(PTRACE_DETACH, pid, NULL, NULL);
		} else {
			pause();
			printf("WE HAVE A KERNEL BUG!!!\n");
		}

		return 0;
	}

the child must not "escape" for sys_pause(), but it can and this was seen
in practice.

This is because ptrace_detach does:

	if (!child->exit_state)
		wake_up_process(child);

this wakeup can happen after this child has already restarted sys_pause(),
because it gets another wakeup from ptrace_untrace().

It is not clear to me what was the rationale, but this is obviously wrong.
Remove this wakeup. The caller saw this task in TASK_TRACED state, and
unless it was SIGKILL'ed in between __ptrace_unlink()->ptrace_untrace()
should handle this case correctly. If it was SIGKILL'ed, we don't need
to wakup the dying tracee too.

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

--- 6.29-rc3/kernel/ptrace.c~1_KILL_WAKE	2009-02-08 04:26:57.000000000 +0100
+++ 6.29-rc3/kernel/ptrace.c	2009-02-08 05:11:52.000000000 +0100
@@ -250,11 +250,7 @@ int ptrace_detach(struct task_struct *ch
 	/* protect against de_thread()->release_task() */
 	if (child->ptrace) {
 		child->exit_code = data;
-
 		dead = __ptrace_detach(current, child);
-
-		if (!child->exit_state)
-			wake_up_process(child);
 	}
 	write_unlock_irq(&tasklist_lock);
 

Re: [PATCH 1/3] ptrace_detach: the wrong wakeup breaks the ERESTARTxxx logic

[Roland McGrath]

> This is because ptrace_detach does:
> 
> 	if (!child->exit_state)
> 		wake_up_process(child);

I'm pretty sure that all these uses of wake_up_process were just blindly
copied from an original use in ptrace code (what's now ptrace_resume).
That original use just dates from the beforetime, the long long ago.
(I don't think it indicates any coherent original intent.)

It's many kinds of wrong.  It's also always been wrong in case of a
simultaneous SIGKILL that already woke the child, which has then blocked on
some mutex or semaphore or whatnot.  I don't know what the stated general
policy about spurious wakeups from schedule() is supposed to be.  Perhaps
to be pedantic, the sys_pause() code has been wrong to return without
checking signal_pending().

It's also about as wrong to use blind wake_up_process in ptrace_resume.
It ought to be wake_up_state(__TASK_TRACED|__TASK_STOPPED).

Frankly, I've always been afraid of strange cruft that might unexpectedly
turn out to rely on this "wrong" (unconditional) wake-up.  Probably the
things like that historically were all just to do with the stopped/traced
bookkeeping and would be covered by explicitly dealing with PTRACE_CONT vs
group stop et al.  But FWIW my reaction to fiddling the wake_up_process
bogons in the past has been, "Be afraid."


Thanks,
Roland

Re: [PATCH 1/3] ptrace_detach: the wrong wakeup breaks the ERESTARTxxx logic

[Oleg Nesterov]

On 02/08, Roland McGrath wrote:
>
> > This is because ptrace_detach does:
> >
> > 	if (!child->exit_state)
> > 		wake_up_process(child);
>
> I'm pretty sure that all these uses of wake_up_process were just blindly
> copied from an original use in ptrace code (what's now ptrace_resume).
> That original use just dates from the beforetime, the long long ago.
> (I don't think it indicates any coherent original intent.)
>
> It's many kinds of wrong.  It's also always been wrong in case of a
> simultaneous SIGKILL that already woke the child, which has then blocked on
> some mutex or semaphore or whatnot.  I don't know what the stated general
> policy about spurious wakeups from schedule() is supposed to be.  Perhaps
> to be pedantic, the sys_pause() code has been wrong to return without
> checking signal_pending().

Yes, I thought about fixing sys_pause() too, but I'm afraid we can have
the similar code.

> Frankly, I've always been afraid of strange cruft that might unexpectedly
> turn out to rely on this "wrong" (unconditional) wake-up.  Probably the
> things like that historically were all just to do with the stopped/traced
> bookkeeping and would be covered by explicitly dealing with PTRACE_CONT vs
> group stop et al.  But FWIW my reaction to fiddling the wake_up_process
> bogons in the past has been, "Be afraid."

Yes, I am afraid, seriously ;)

This can reveal other subtle problems, of course. But there is another reason
why this wakeup is wrong. It clearly breaks the SIGNAL_GROUP_STOPPED logic
in ptrace_untrace().

Oleg.