Re: [PATCH] Discard notification signals when a tracer exits

[Oleg Nesterov <oleg@tv-sign.ru>]

On 03/25, Oleg Nesterov wrote:
>
> This patch needs Roland's opinion. I can't really judge, but I
> have some (perhaps wrong) doubts.
> 
> On 03/25, Petr Tesarik wrote:
> >
> > When a tracer exits without detaching from the traced process, the
> > tracee may be at a tracer notification stop and will thus interpret
> > the value in task->exit_code (SIGTRAP | 0x80) as the signal to be
> > delivered.
> > 
> > This patch fixes the problem by clearing exit_code when detaching
> > the traced process from a dying tracer.
> > 
> > Signed-off-by: Petr Tesarik <ptesarik@suse.cz>
> > 
> > ---
> >  exit.c |    8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > --- a/kernel/exit.c
> > +++ b/kernel/exit.c
> > @@ -642,8 +642,10 @@ reparent_thread(struct task_struct *p, s
> >  			/*
> >  			 * If it was at a trace stop, turn it into
> >  			 * a normal stop since it's no longer being
> > -			 * traced.
> > +			 * traced.  Cancel the notification signal,
> > +			 * or the tracee may get a SIGTRAP.
> >  			 */
> > +			p->exit_code = 0;
> >  			ptrace_untrace(p);
> >  		}
> >  	}
> > @@ -713,6 +715,10 @@ static void forget_original_parent(struc
> >  			p->real_parent = reaper;
> >  			reparent_thread(p, father, 0);
> >  		} else {
> > +			/* cancel the notification signal at a trace stop */
> > +			if (p->state == TASK_TRACED)
> > +				p->exit_code = 0;
> 
> This reduce the likelihood that the tracee will be SIGTRAP'ed, but doesn't
> solve the problem, no?
> 
> Suppose that the tracee does send_sigtrap(current) in do_syscall_trace()
> and then ptracer exits. Or ptracer wakes up the TASK_TRACED tracee without
> clearing its ->exit_code and then you kill(ptracer, SIGKILL).
> 
> If we really need this, _perhaps_ it is better to change do_syscall_trace(),
> so that the tracee checks ->ptrace before sending the signal to itself.
> 
> 
> But actually, I don't understand what is the problem. Ptracer has full control,
> you should not kill it with SIGKILL, this may leave the child in some bad/
> inconsistent change.

Additional note. Suppose that the tracee dequeues the "good" signal, notices
PT_PTRACED and calls ptrace_stop(). We set TASK_TRACED under ->siglock, without
holding tasklist_lock. At this moment you kill strace, it clears ->exit_code.
The tracee notices it is not traced any longer and returns to get_signal_to_deliver().
Since ->exit_code is cleared, the "right" signal is lost.

So I think this patch adds a race. In some sense (yes I am biased) this is
even worse than the problem this patch tries to solve, because this race
is unlikely and is hard to trigger/debug, and it could be easily unnoticed.

Oleg.