[PATCH] Discard notification signals when a tracer exits

[PATCH] Discard notification signals when a tracer exits

[Petr Tesarik]

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;
+
 			/* reparent ptraced task to its real parent */
 			__ptrace_unlink (p);
 			if (p->exit_state == EXIT_ZOMBIE && p->exit_signal != -1 &&

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

[Petr Tesarik]

[-- Attachment #1: Type: text/plain, Size: 1930 bytes --]

On Tue, 2008-03-25 at 15:31 +0100, 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>
> 

Oh, and here is a testing script for the first hunk. It fails on all
kernels I have tried. The second hunk can also be tested if you run
strace on the traced process instead of attaching to a running one, but
I didn't figure out how to get the PID of the traced process within a
script, so you'd have to trigger the bug manually.

> ---
>  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;
> +
>  			/* reparent ptraced task to its real parent */
>  			__ptrace_unlink (p);
>  			if (p->exit_state == EXIT_ZOMBIE && p->exit_signal != -1 &&
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[-- Attachment #2: test-trap.sh --]
[-- Type: application/x-shellscript, Size: 747 bytes --]

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

[Oleg Nesterov]

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. If strace/whatever itself exits without taking care about
its tracees, then we should fix the tracer, not the kernel.

Yes, I think this can prevent the death of the tracee if you do kill(strace,9),
but why this is important? Why should you kill it with SIGKILL?


But again, let's see what Roland thinks.

Oleg.

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

[Oleg Nesterov]

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.

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

[Roland McGrath]

I think that's wrong.  When the tracer dies unceremoniously, the tracee
should continue as if it had not been traced.  Your change makes it swallow
a normal signal waiting to be delivered, and then resume as if noone had
ever sent a signal.  

There is indeed a longstanding problem for tracing-induced signals
(e.g. SIGTRAP for single-step or exec).  One can be queued but not yet
delivered while the tracer detaches or dies, and then it gets delivered
to the unsuspecting process without any debugger to intercept it.
I plan to address that eventually, but there is no quick fix for it.

In the non-signal (ptrace_notify) cases, we don't have the queuing issue.
But it's still not as simple as checking current->ptrace to decide whether
to follow current->exit_code or clear it.  When PTRACE_DETACH is called
with a parting signal (nonzero data argument), the tracee will wake up to
see current->ptrace = 0 and current->exit_code = signo and it is explicitly
intended to act on signo rather than ignore it.

The particular case you mentioned could be addressed by something like this:

--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1654,6 +1654,9 @@ static void ptrace_stop(int exit_code, i
 	spin_lock_irq(&current->sighand->siglock);
 	current->last_siginfo = NULL;
 
+	if (current->exit_code & ~0x7f)
+		current->exit_code = 0;
+
 	/*
 	 * Queued signals ignored us while we were stopped for tracing.
 	 * So check for any that we should take before resuming user mode.

But that only catches the PTRACE_O_TRACESYSGOOD cases, and not bare SIGTRAP.
To be clean, there is no way to distinguish a left-over SIGTRAP that ought
to be ignored from an intentional SIGTRAP placed there by the debugger for
the tracee to deliver.  (Granted passing SIGTRAP through after a syscall is
rarely or never done.  But that doesn't make a special-case kludge right.)

I do very much appreciate the concerns that motivated your change, and share
them.  But you can see that this is a small part of a large can of worms.
It has been this way for a long time.  I am working steadily towards new
ways to clean up the entire picture.  In the meantime, I don't really favor
any marginal changes like this that complicate the behavior with more
special cases that address only a tiny corner of the whole set of problems.


Thanks,
Roland

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

[Petr Tesarik]

On Tue, 2008-03-25 at 19:16 +0300, 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 the ptracer wakes up the tracee, then it is no longer in the state
TASK_TRACED.

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

You're missing the point. The child _is_ traced before sending the
signal. It leaves the notification code in ->exit_code, so that the
tracer can fetch it with a call to wait4(). Later, the same field is
used to tell the tracee which signal the tracer delivered to it.
However, if the tracer dies before it reads (and resets) the value in
->exit_code, the tracee interprets the notification code as the signal
to be delivered.

> 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. If strace/whatever itself exits without taking care about
> its tracees, then we should fix the tracer, not the kernel.

Hm, what if the tracer gets actually killed by the kernel, e.g. by the
OOM killer? How would you fix that in userspace?

Anyway, if you really want to have broken behaviour on unexpected tracer
exits, then we'd better not change the tracee's state from TASK_TRACED
at all. That way it stays hanging in the system and the admin can decide
whether they want to shoot it down with a SIGKILL or attach a debugger
to it and somehow resume the process. Arranging for a delivery of a
non-existent SIGTRAP seems utterly illogical to me.

Cheers,
Petr Tesarik

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

[Petr Tesarik]

On Tue, 2008-03-25 at 19:33 +0300, Oleg Nesterov wrote:
> 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.

Yes, you're right. My patch only works OK in the ptrace_notify() case,
not when it is called from get_signal_to_deliver().

So, do you think it's a better idea to add a new flag to notify the
tracee that its tracer disappeared? That way it can decide how to handle
the situation in ptrace_stop(), something along these lines:

--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1628,6 +1628,8 @@ ptrace_stop(int exit_code, int c
 		do_notify_parent_cldstop(current, CLD_TRAPPED);
 		read_unlock(&tasklist_lock);
 		schedule();
+		if (current->flags & PF_PTRACEORPHAN & clear_code)
+			current->exit_code = 0;
 	} else {
 		/*
 		 * By the time we got the lock, our tracer went away.

And then replace p->exit_code = 0 in my original patch with something
like p->flags |= PF_PTRACEORPHAN. Better?

Cheers,
Petr Tesarik

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

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

[Oleg Nesterov]

On 03/26, Petr Tesarik wrote:
>
> On Tue, 2008-03-25 at 19:16 +0300, 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:
> > >
> > > --- 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 the ptracer wakes up the tracee, then it is no longer in the state
> TASK_TRACED.

Exactly. I meant this patch can't help in that case, the problem is "wider".

> > 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.
> 
> You're missing the point. The child _is_ traced before sending the
> signal. It leaves the notification code in ->exit_code, so that the
> tracer can fetch it with a call to wait4(). Later, the same field is
> used to tell the tracee which signal the tracer delivered to it.
> However, if the tracer dies before it reads (and resets) the value in
> ->exit_code, the tracee interprets the notification code as the signal
> to be delivered.

I see! That is why I suggested to re-check ->ptrace, and if we are not
ptraced any longer - discard the notification. Even better, we can change
ptrace_stop() as Roland pointed out.

> > 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. If strace/whatever itself exits without taking care about
> > its tracees, then we should fix the tracer, not the kernel.
> 
> Hm, what if the tracer gets actually killed by the kernel, e.g. by the
> OOM killer? How would you fix that in userspace?

I think in that case a user has much worse problems ;)

> Anyway, if you really want to have broken behaviour on unexpected tracer
> exits, then we'd better not change the tracee's state from TASK_TRACED
> at all. That way it stays hanging in the system and the admin can decide
> whether they want to shoot it down with a SIGKILL or attach a debugger
> to it and somehow resume the process. Arranging for a delivery of a
> non-existent SIGTRAP seems utterly illogical to me.

No, I don't want to have broken behaviour on unexpected tracer exits,
but I don't see a "good" way to fix this relatively minor problem.

But I _personally_ don't like this particular patch, sorry. And please
note that I said "I can't really judge".

> > 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.
> 
> Yes, you're right. My patch only works OK in the ptrace_notify() case,
> not when it is called from get_signal_to_deliver().

And this means the patch is buggy, that was my point. Actually I think
it has other problems.

> So, do you think it's a better idea to add a new flag to notify the
> tracee that its tracer disappeared? That way it can decide how to handle
> the situation in ptrace_stop(), something along these lines:
>
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -1628,6 +1628,8 @@ ptrace_stop(int exit_code, int c
>                 do_notify_parent_cldstop(current, CLD_TRAPPED);
>                 read_unlock(&tasklist_lock);
>                 schedule();
> +               if (current->flags & PF_PTRACEORPHAN & clear_code)
> +                       current->exit_code = 0;
>         } else {
>                 /*
>                  * By the time we got the lock, our tracer went away.
> 
> And then replace p->exit_code = 0 in my original patch with something
> like p->flags |= PF_PTRACEORPHAN. Better?

This is racy, and we can't modify p->flags, and I don't really understand
how this can help.

I am sorry Petr, I have no idea how to fix this, but I don't agree with
your approach.

(Yes I know, it is very easy to blame somebody else's code ;)

Oleg.

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

[Petr Tesarik]

On Wed, 2008-03-26 at 21:17 +0300, Oleg Nesterov wrote:
> [...]
> > > 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.
> > 
> > You're missing the point. The child _is_ traced before sending the
> > signal. It leaves the notification code in ->exit_code, so that the
> > tracer can fetch it with a call to wait4(). Later, the same field is
> > used to tell the tracee which signal the tracer delivered to it.
> > However, if the tracer dies before it reads (and resets) the value in
> > ->exit_code, the tracee interprets the notification code as the signal
> > to be delivered.
> 
> I see! That is why I suggested to re-check ->ptrace, and if we are not
> ptraced any longer - discard the notification. Even better, we can change
> ptrace_stop() as Roland pointed out.

That's not what you want. It's totally OK if the tracer resumes the
tracee with a signal and immediately exits before the tracee returns
from schedule(), so this approach won't work. Sorry.

> > > 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. If strace/whatever itself exits without taking care about
> > > its tracees, then we should fix the tracer, not the kernel.
> > 
> > Hm, what if the tracer gets actually killed by the kernel, e.g. by the
> > OOM killer? How would you fix that in userspace?
> 
> I think in that case a user has much worse problems ;)

Well, there are many more cases. What about an admin who forgot a
running strace on a system daemon and then used SAK (SysRq+K) to make
sure the system was sane before logging in?

Anyway, I have a very real bug report from a paying customer, so
whatever their use case, I'm bound to solve it for them. And I always
thought that pushing a fix upstreams is considered "the right
thing" (TM).

> > Anyway, if you really want to have broken behaviour on unexpected tracer
> > exits, then we'd better not change the tracee's state from TASK_TRACED
> > at all. That way it stays hanging in the system and the admin can decide
> > whether they want to shoot it down with a SIGKILL or attach a debugger
> > to it and somehow resume the process. Arranging for a delivery of a
> > non-existent SIGTRAP seems utterly illogical to me.
> 
> No, I don't want to have broken behaviour on unexpected tracer exits,
> but I don't see a "good" way to fix this relatively minor problem.
> 
> But I _personally_ don't like this particular patch, sorry. And please
> note that I said "I can't really judge".

That's perfectly fine. More eyes can see more bugs. My patch was buggy,
indeed, and I don't want to introduce new bugs.

>[...]
> > So, do you think it's a better idea to add a new flag to notify the
> > tracee that its tracer disappeared? That way it can decide how to handle
> > the situation in ptrace_stop(), something along these lines:
> >
> > --- a/kernel/signal.c
> > +++ b/kernel/signal.c
> > @@ -1628,6 +1628,8 @@ ptrace_stop(int exit_code, int c
> >                 do_notify_parent_cldstop(current, CLD_TRAPPED);
> >                 read_unlock(&tasklist_lock);
> >                 schedule();
> > +               if (current->flags & PF_PTRACEORPHAN & clear_code)
> > +                       current->exit_code = 0;
> >         } else {
> >                 /*
> >                  * By the time we got the lock, our tracer went away.
> > 
> > And then replace p->exit_code = 0 in my original patch with something
> > like p->flags |= PF_PTRACEORPHAN. Better?
> 
> This is racy, and we can't modify p->flags, and I don't really understand
> how this can help.

Why is it racy? It's ugly, but where's the race condition? The tracee
cannot get out of schedule() until the tracer lets it go. And it doesn't
let it go, because there can only be one tracer for any given task and
that's the task which is exiting. So AFAICS doing (almost) anything to
the tracee is safe.

To explain how my second patch is different from the previous one - the
tracee now decides whether the signal should be discarded or not, and it
can distinguish the self-induced tracer notification signal from a real
one by looking at the clear_code argument to ptrace_stop().

Regards,
Petr Tesarik

> I am sorry Petr, I have no idea how to fix this, but I don't agree with
> your approach.
> 
> (Yes I know, it is very easy to blame somebody else's code ;)
> 
> Oleg.
> 

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

[Oleg Nesterov]

On 03/27, Petr Tesarik wrote:
>
> On Wed, 2008-03-26 at 21:17 +0300, Oleg Nesterov wrote:
> > [...]
> > > > 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.
> > > 
> > > You're missing the point. The child _is_ traced before sending the
> > > signal. It leaves the notification code in ->exit_code, so that the
> > > tracer can fetch it with a call to wait4(). Later, the same field is
> > > used to tell the tracee which signal the tracer delivered to it.
> > > However, if the tracer dies before it reads (and resets) the value in
> > > ->exit_code, the tracee interprets the notification code as the signal
> > > to be delivered.
> > 
> > I see! That is why I suggested to re-check ->ptrace, and if we are not
> > ptraced any longer - discard the notification. Even better, we can change
> > ptrace_stop() as Roland pointed out.
> 
> That's not what you want. It's totally OK if the tracer resumes the
> tracee with a signal and immediately exits before the tracee returns
> from schedule(), so this approach won't work. Sorry.

You misunderstood. I didn't claim this approach is right, but I think
it is better if we desperately need to fix this problem. Yes the tracee
can lost the right SIGTRAP, this is obvious. But this can happen with
your patch as well, the kernel just can't know what should we do with
SIGTRAP in ->exit_code.

In short, I (roughly) meant

	ptrace_stop:
		...
		// return path
		spin_lock_irq(->siglock);
		current->last_siginfo = NULL;

		// can be false positive
		if (!->ptrace && (->exit_code & 7f) == SIGTRAP)
			->exit_code = 0;

		...

Yes, this is not right too. (and yes, ptrace_stop() is much better than
do_syscall_trace() I suggested originally).

> Anyway, I have a very real bug report from a paying customer, so
> whatever their use case, I'm bound to solve it for them.

Just curious: what is the bug report? Why do they want to SIGKILL strace?

> And I always
> thought that pushing a fix upstreams is considered "the right
> thing" (TM).

Once again: of course I agree it would be nice to fix the problem if we
had a clean fix.

Yes I think this problem is _relatively_ minor, and I don't really think
it is BUG. But I am not maintainer or expert, just my personal opinion.
I jumped into discussion only because I don't agree with the patch, not
because I think we should not fix this.

(btw, I think that maintainer has already give a good summary ;)

> > > --- a/kernel/signal.c
> > > +++ b/kernel/signal.c
> > > @@ -1628,6 +1628,8 @@ ptrace_stop(int exit_code, int c
> > >                 do_notify_parent_cldstop(current, CLD_TRAPPED);
> > >                 read_unlock(&tasklist_lock);
> > >                 schedule();
> > > +               if (current->flags & PF_PTRACEORPHAN & clear_code)
> > > +                       current->exit_code = 0;
> > >         } else {
> > >                 /*
> > >                  * By the time we got the lock, our tracer went away.
> > > 
> > > And then replace p->exit_code = 0 in my original patch with something
> > > like p->flags |= PF_PTRACEORPHAN. Better?
> > 
> > This is racy, and we can't modify p->flags, and I don't really understand
> > how this can help.
> 
> Why is it racy? It's ugly, but where's the race condition? The tracee
> cannot get out of schedule() until the tracer lets it go. And it doesn't
> let it go, because there can only be one tracer for any given task and
> that's the task which is exiting. So AFAICS doing (almost) anything to
> the tracee is safe.

SIGKILL can wake up the tracee, it could be TASK_RUNNING when the tracer
plays with its flag, this is wrong.

But there are other problems. It is racy because TASK_TRACED doesn't
necessary mean the tracee sleeps in TASK_TRACED state, it is possible
that the tracee is running and waits for tasklist_lock() in ptrace_stop.
As a very minimum, we should clear PF_PTRACEORPHAN.

More. Suppose that we set PF_PTRACEORPHAN, and then ptrace_untrace()
changes TASK_TRACED to TASK_STOPPED. Another tracer attaches to the
poor tracee, ptrace_check_attach() changes TASK_STOPPED to TASK_TRACED.
When the new tracer wakes up the tracee, it will see PF_PTRACEORPHAN
and clear ->exit_code.

Oleg.