[PATCH 0/1] ptrace: fix ptrace_signal() && STOP_DEQUEUED interaction

[PATCH 0/1] ptrace: fix ptrace_signal() && STOP_DEQUEUED interaction

[Oleg Nesterov]

Another very annoying and ancient problem which needs the fix,
and any fix obviously adds the subtle user-visible changes.

In short, in general it is simply impossible to know whether
PTRACE_CONT(SIGSTOP) will stop the tracee or not.

ee77f075 "signal: Turn SIGNAL_STOP_DEQUEUED into GROUP_STOP_DEQUEUED"
makes the things better, at least STOP_DEQUEUED can't be set/cleared
by another thread, but still without this patch the behavior is not
clearly defined.

Oleg.

[PATCH 1/1] ptrace: fix ptrace_signal() && STOP_DEQUEUED interaction

[Oleg Nesterov]

Simple test-case,

	int main(void)
	{
		int pid, status;

		pid = fork();
		if (!pid) {
			pause();
			assert(0);
			return 0x23;
		}

		assert(ptrace(PTRACE_ATTACH, pid, 0,0) == 0);
		assert(wait(&status) == pid);
		assert(WIFSTOPPED(status) && WSTOPSIG(status) == SIGSTOP);

		kill(pid, SIGCONT);	// <--- also clears STOP_DEQUEUD

		assert(ptrace(PTRACE_CONT, pid, 0,0) == 0);
		assert(wait(&status) == pid);
		assert(WIFSTOPPED(status) && WSTOPSIG(status) == SIGCONT);

		assert(ptrace(PTRACE_CONT, pid, 0, SIGSTOP) == 0);
		assert(wait(&status) == pid);
		assert(WIFSTOPPED(status) && WSTOPSIG(status) == SIGSTOP);

		kill(pid, SIGKILL);
		return 0;
	}

Without the patch it hangs. After the patch SIGSTOP "injected" by the
tracer is not ignored and stops the tracee.

Note also that if this test-case uses, say, SIGWINCH instead of SIGCONT,
everything works without the patch. This can't be right, and this is
confusing.

The problem is that SIGSTOP (or any other sig_kernel_stop() signal) has
no effect without JOBCTL_STOP_DEQUEUED. This means it is simply ignored
after PTRACE_CONT unless JOBCTL_STOP_DEQUEUED was set "by accident", say
it wasn't cleared after initial SIGSTOP sent by PTRACE_ATTACH.

At first glance we could change ptrace_signal() to add STOP_DEQUEUED
after return from ptrace_stop(), but this is not right in case when the
tracer does not change the reported SIGSTOP and SIGCONT comes in between.
This is even more wrong with PT_SEIZED, SIGCONT adds JOBCTL_TRAP_NOTIFY
which will be "lost" during the TRAP_STOP | TRAP_NOTIFY report.

So lets add STOP_DEQUEUED _before_ we report the signal. It has no effect
unless sig_kernel_stop() == T after the tracer resumes us, and in the
latter case the pending STOP_DEQUEUED means no SIGCONT in between, we
should stop.

Note also that if SIGCONT was sent, PT_SEIZED tracee will correctly
report PTRACE_EVENT_STOP/SIGTRAP and thus the tracer can notice the fact
SIGSTOP was cancelled.

Also, move the current->ptrace check from ptrace_signal() to its caller,
get_signal_to_deliver(), this looks more natural.

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

 kernel/signal.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

--- ptrace/kernel/signal.c~1_ptrace_signal_stop_dequeued	2011-07-07 18:34:44.000000000 +0200
+++ ptrace/kernel/signal.c	2011-07-07 20:34:41.000000000 +0200
@@ -2084,12 +2084,13 @@ static void do_jobctl_trap(void)
 static int ptrace_signal(int signr, siginfo_t *info,
 			 struct pt_regs *regs, void *cookie)
 {
-	if (!current->ptrace)
-		return signr;
-
 	ptrace_signal_deliver(regs, cookie);
-
-	/* Let the debugger run.  */
+	/*
+	 * Debugger can change sig_kernel_stop(signr) from F to T,
+	 * in this case we should stop iff no SIGCONT in between.
+	 * Otherwise this JOBCTL_STOP_DEQUEUED has no effect.
+	 */
+	current->jobctl |= JOBCTL_STOP_DEQUEUED;
 	ptrace_stop(signr, CLD_TRAPPED, 0, info);
 
 	/* We're back.  Did the debugger cancel the sig?  */
@@ -2193,7 +2194,7 @@ relock:
 		if (!signr)
 			break; /* will return 0 */
 
-		if (signr != SIGKILL) {
+		if (unlikely(current->ptrace) && signr != SIGKILL) {
 			signr = ptrace_signal(signr, info,
 					      regs, cookie);
 			if (!signr)

Re: [PATCH 1/1] ptrace: fix ptrace_signal() && STOP_DEQUEUED interaction

[Tejun Heo]

Hello, Oleg.  Sorry about the long delay.  Was lost somewhere else. :)

On Thu, Jul 07, 2011 at 09:03:24PM +0200, Oleg Nesterov wrote:
> Without the patch it hangs. After the patch SIGSTOP "injected" by the
> tracer is not ignored and stops the tracee.

I always felt the ability to 'inject' different signal there is rather
useless and prone to induce weird issues.  It would be better if
ptrace_signal() is part of signal delivery action after all the checks
so that the ptracer says whether to proceed with the action or not but
no more.  Well...

> So lets add STOP_DEQUEUED _before_ we report the signal. It has no effect
> unless sig_kernel_stop() == T after the tracer resumes us, and in the
> latter case the pending STOP_DEQUEUED means no SIGCONT in between, we
> should stop.

Anyways, yes, this seems to be a nice improvement but it looks very
weird (and difficult to comprehend) to be setting STOP_DEQUEUED
unconditionally in ptrace_signal().  Wouldn't it be better to flip the
flag so that we have CONT_RECEIVED before doing this?

Thanks.

-- 
tejun

Re: [PATCH 1/1] ptrace: fix ptrace_signal() && STOP_DEQUEUED interaction

[Oleg Nesterov]

Hi Tejun,

On 07/13, Tejun Heo wrote:
>
> Sorry about the long delay.

No, no, you shouldn't use this mantra.  I hold the copyright!

> On Thu, Jul 07, 2011 at 09:03:24PM +0200, Oleg Nesterov wrote:
> > Without the patch it hangs. After the patch SIGSTOP "injected" by the
> > tracer is not ignored and stops the tracee.
>
> I always felt the ability to 'inject' different signal there is rather
> useless and prone to induce weird issues.  It would be better if
> ptrace_signal() is part of signal delivery action after all the checks
> so that the ptracer says whether to proceed with the action or not but
> no more.  Well...

Oh, probably. If the tracer wants a different signr, it can simply do
tkill() + PTRACE_CONT(0). I agree. Although perhaps this is needed for
gdb, I dunno. But we can't change this.

And, I'd like to clarify... It is not that I think it is that important
to ensure PTRACE_CONT(SIGSTOP) will actually stop the tracee if the tracer
changes the original signal. I simply do not know if it is used this way.

The only important thing, imho, the behaviour shouldn't depend on
/dev/random, with this patch it is at least clearly defined.

> > So lets add STOP_DEQUEUED _before_ we report the signal. It has no effect
> > unless sig_kernel_stop() == T after the tracer resumes us, and in the
> > latter case the pending STOP_DEQUEUED means no SIGCONT in between, we
> > should stop.
>
> Anyways, yes, this seems to be a nice improvement but it looks very
> weird (and difficult to comprehend) to be setting STOP_DEQUEUED
> unconditionally in ptrace_signal().

Yeeees, agreed. I even added the comment to explain this weirdness.

> Wouldn't it be better to flip the
> flag so that we have CONT_RECEIVED before doing this?

May be. You know, I thought about this when I did ee77f075
"signal: Turn SIGNAL_STOP_DEQUEUED into GROUP_STOP_DEQUEUED".

Or may be we can simply rename it into STOP_ALLOWED. In this case
we can even set it unconditionally before dequeue_signal().


Anyway, whatever we do, this patch doesn't complicate the
CONT_RECEIVED/STOP_ALLOWED change. Can't we do this later?

Oleg.

Re: [PATCH 1/1] ptrace: fix ptrace_signal() && STOP_DEQUEUED interaction

[Tejun Heo]

Hello, Oleg.

On Wed, Jul 13, 2011 at 08:33:22PM +0200, Oleg Nesterov wrote:
> > On Thu, Jul 07, 2011 at 09:03:24PM +0200, Oleg Nesterov wrote:
> > > Without the patch it hangs. After the patch SIGSTOP "injected" by the
> > > tracer is not ignored and stops the tracee.
> >
> > I always felt the ability to 'inject' different signal there is rather
> > useless and prone to induce weird issues.  It would be better if
> > ptrace_signal() is part of signal delivery action after all the checks
> > so that the ptracer says whether to proceed with the action or not but
> > no more.  Well...
> 
> Oh, probably. If the tracer wants a different signr, it can simply do
> tkill() + PTRACE_CONT(0). I agree. Although perhaps this is needed for
> gdb, I dunno. But we can't change this.

Yeah, we can't change it.  I was just thinking out loud.  Sending
signals from ptrace code path seems generally wrong.  In this case, if
the signal becomes blocked inbetween, the signal gets sent again.
Seeing the same signal being delivered twice can be confusing.
Moreover, SIGCONT can be blocked and the action of 'sending' it has
side effects, so re-sending it is simply wrong.  This should have been
ack/nack for the action to take.

Anyways, not much point in ranting about it at this point, I guess.

> > Wouldn't it be better to flip the
> > flag so that we have CONT_RECEIVED before doing this?
> 
> May be. You know, I thought about this when I did ee77f075
> "signal: Turn SIGNAL_STOP_DEQUEUED into GROUP_STOP_DEQUEUED".
> 
> Or may be we can simply rename it into STOP_ALLOWED. In this case
> we can even set it unconditionally before dequeue_signal().
> 
> Anyway, whatever we do, this patch doesn't complicate the
> CONT_RECEIVED/STOP_ALLOWED change. Can't we do this later?

Never mind.  I for some reason thought flipping the flag would make
the extra step in ptrace_signal() unnecessary.  We need to clear it
all the same so it doesn't really improve anything.  I think the
current version should be fine (maybe the comment can be beefed up a
bit?).

Thanks.

-- 
tejun

Re: [PATCH 1/1] ptrace: fix ptrace_signal() && STOP_DEQUEUED interaction

[Oleg Nesterov]

On 07/14, Tejun Heo wrote:
>
> Never mind.  I for some reason thought flipping the flag would make
> the extra step in ptrace_signal() unnecessary.  We need to clear it
> all the same so it doesn't really improve anything.  I think the
> current version should be fine

Good.

> (maybe the comment can be beefed up a
> bit?).

I agree, this is not the best comment... OK, I'll try to make a
better one. Damn this is not easy ;) and I hate the really fat
comments.

Oleg.

Re: [PATCH 1/1] ptrace: fix ptrace_signal() && STOP_DEQUEUED interaction

[Oleg Nesterov]

On 07/14, Oleg Nesterov wrote:
>
> On 07/14, Tejun Heo wrote:
> >
> > Never mind.  I for some reason thought flipping the flag would make
> > the extra step in ptrace_signal() unnecessary.  We need to clear it
> > all the same so it doesn't really improve anything.  I think the
> > current version should be fine
>
> Good.
>
> > (maybe the comment can be beefed up a
> > bit?).
>
> I agree, this is not the best comment... OK, I'll try to make a
> better one. Damn this is not easy ;) and I hate the really fat
> comments.

OK, how about v2? The patch is the same, only the comment was updated.
Not sure it is really better though.

Oleg.

------------------------------------------------------------------------------
[PATCH v2] ptrace: fix ptrace_signal() && STOP_DEQUEUED interaction

Simple test-case,

	int main(void)
	{
		int pid, status;

		pid = fork();
		if (!pid) {
			pause();
			assert(0);
			return 0x23;
		}

		assert(ptrace(PTRACE_ATTACH, pid, 0,0) == 0);
		assert(wait(&status) == pid);
		assert(WIFSTOPPED(status) && WSTOPSIG(status) == SIGSTOP);

		kill(pid, SIGCONT);	// <--- also clears STOP_DEQUEUD

		assert(ptrace(PTRACE_CONT, pid, 0,0) == 0);
		assert(wait(&status) == pid);
		assert(WIFSTOPPED(status) && WSTOPSIG(status) == SIGCONT);

		assert(ptrace(PTRACE_CONT, pid, 0, SIGSTOP) == 0);
		assert(wait(&status) == pid);
		assert(WIFSTOPPED(status) && WSTOPSIG(status) == SIGSTOP);

		kill(pid, SIGKILL);
		return 0;
	}

Without the patch it hangs. After the patch SIGSTOP "injected" by the
tracer is not ignored and stops the tracee.

Note also that if this test-case uses, say, SIGWINCH instead of SIGCONT,
everything works without the patch. This can't be right, and this is
confusing.

The problem is that SIGSTOP (or any other sig_kernel_stop() signal) has
no effect without JOBCTL_STOP_DEQUEUED. This means it is simply ignored
after PTRACE_CONT unless JOBCTL_STOP_DEQUEUED was set "by accident", say
it wasn't cleared after initial SIGSTOP sent by PTRACE_ATTACH.

At first glance we could change ptrace_signal() to add STOP_DEQUEUED
after return from ptrace_stop(), but this is not right in case when the
tracer does not change the reported SIGSTOP and SIGCONT comes in between.
This is even more wrong with PT_SEIZED, SIGCONT adds JOBCTL_TRAP_NOTIFY
which will be "lost" during the TRAP_STOP | TRAP_NOTIFY report.

So lets add STOP_DEQUEUED _before_ we report the signal. It has no effect
unless sig_kernel_stop() == T after the tracer resumes us, and in the
latter case the pending STOP_DEQUEUED means no SIGCONT in between, we
should stop.

Note also that if SIGCONT was sent, PT_SEIZED tracee will correctly
report PTRACE_EVENT_STOP/SIGTRAP and thus the tracer can notice the fact
SIGSTOP was cancelled.

Also, move the current->ptrace check from ptrace_signal() to its caller,
get_signal_to_deliver(), this looks more natural.

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

 kernel/signal.c |   17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

--- ptrace/kernel/signal.c~1_ptrace_signal_stop_dequeued	2011-07-17 20:16:36.000000000 +0200
+++ ptrace/kernel/signal.c	2011-07-17 20:57:30.000000000 +0200
@@ -2084,12 +2084,17 @@ static void do_jobctl_trap(void)
 static int ptrace_signal(int signr, siginfo_t *info,
 			 struct pt_regs *regs, void *cookie)
 {
-	if (!current->ptrace)
-		return signr;
-
 	ptrace_signal_deliver(regs, cookie);
-
-	/* Let the debugger run.  */
+	/*
+	 * We do not check sig_kernel_stop(signr) but set this marker
+	 * unconditionally because we do not know whether debugger will
+	 * change signr. This flag has no meaning unless we are going
+	 * to stop after return from ptrace_stop(). In this case it will
+	 * be checked in do_signal_stop(), we should only stop if it was
+	 * not cleared by SIGCONT while we were sleeping. See also the
+	 * comment in dequeue_signal().
+	 */
+	current->jobctl |= JOBCTL_STOP_DEQUEUED;
 	ptrace_stop(signr, CLD_TRAPPED, 0, info);
 
 	/* We're back.  Did the debugger cancel the sig?  */
@@ -2193,7 +2198,7 @@ relock:
 		if (!signr)
 			break; /* will return 0 */
 
-		if (signr != SIGKILL) {
+		if (unlikely(current->ptrace) && signr != SIGKILL) {
 			signr = ptrace_signal(signr, info,
 					      regs, cookie);
 			if (!signr)

Re: [PATCH 1/1] ptrace: fix ptrace_signal() && STOP_DEQUEUED interaction

[Tejun Heo]

Hey, Oleg.

On Sun, Jul 17, 2011 at 09:03:37PM +0200, Oleg Nesterov wrote:
> > I agree, this is not the best comment... OK, I'll try to make a
> > better one. Damn this is not easy ;) and I hate the really fat
> > comments.
> 
> OK, how about v2? The patch is the same, only the comment was updated.
> Not sure it is really better though.

Heh, yeah, that's one fat ugly comment but I think the code there
deserves it. :)

Acked-by: Tejun Heo <tj@kernel.org>

Thank you.

-- 
tejun