[PATCH] PPC32: cancel syscall restart on signal delivery

[PATCH] PPC32: cancel syscall restart on signal delivery

[Paul Mackerras]

Linus, please apply.

This patch ensures that the PPC kernel cancels any pending restarted
system call when it delivers a signal.  This is the PPC counterpart of
the change that has recently gone into i386 and other architectures.

BTW, do we have a test program that triggers the bug that this fixes?

Thanks,
Paul.

diff -urN linux-2.5/arch/ppc/kernel/signal.c pmac-2.5/arch/ppc/kernel/signal.c
--- linux-2.5/arch/ppc/kernel/signal.c	2003-09-27 19:46:43.000000000 +1000
+++ pmac-2.5/arch/ppc/kernel/signal.c	2003-11-14 23:08:22.000000000 +1100
@@ -569,10 +569,6 @@
 			regs->result = -EINTR;
 			regs->gpr[3] = EINTR;
 			/* note that the cr0.SO bit is already set */
-			/* clear any restart function that was set */
-			if (ret == ERESTART_RESTARTBLOCK)
-				current_thread_info()->restart_block.fn
-					= do_no_restart_syscall;
 		} else {
 			regs->nip -= 4;	/* Back up & retry system call */
 			regs->result = 0;
@@ -587,6 +583,9 @@
 	if (signr == 0)
 		return 0;		/* no signals delivered */
 
+	/* Always make any pending restarted system calls return -EINTR */
+	current_thread_info()->restart_block.fn = do_no_restart_syscall;
+
 	if ((ka->sa.sa_flags & SA_ONSTACK) && current->sas_ss_size
 	    && !on_sig_stack(regs->gpr[1]))
 		newsp = current->sas_ss_sp + current->sas_ss_size;

Re: [PATCH] PPC32: cancel syscall restart on signal delivery

[Linus Torvalds]

On Sat, 15 Nov 2003, Paul Mackerras wrote:
> 
> BTW, do we have a test program that triggers the bug that this fixes?

No. In fact, it's an _incredibly_ tiny race, because the old code that 
only handled it at the return path of the system call that got interrupted 
would catch the thing in all cases _except_ if the system

 (a) decided to restart the system call
 (b) got an interrupt _after_ the return to user mode but _before_ the 
     system call instruction itself actually did the restart
 (c) this interrupt caused a new signal with a handler (different from the
     one that caused the restart, since that one by definition had no
     handler) to the same program.

On x86, for the old calling conventions (ie "int 0x80"), that meant that 
the interrupt window was literally a single instruction. For the new one 
("sysenter") it was two instructions.  So you literally need a minimum of 
two different signals, and a incredibly tight window.

To make it even worse, even if the above incredibly unlikely thing
actually _happens_, most of the time it wouldn't really matter. The worst
case schenario is that the signal handler itself does a system call that
needs restarting, in which case on signal handler return we now restart
the _wrong_ system call once we return. But a more likely schenario is
that we restart the right system call after the signal handler has run,
rather than returning EINTR.

Quite frankly, the only reason I even thought about it was due to the 
clock_nanosleep() patch to fix the posix timer restart code. Which just 
made me trace the sequence again in my head.

			Linus

Re: [PATCH] PPC32: cancel syscall restart on signal delivery

[Linus Torvalds]

On Fri, 14 Nov 2003, Linus Torvalds wrote:
> 
> On x86, for the old calling conventions (ie "int 0x80"), that meant that 
> the interrupt window was literally a single instruction. For the new one 
> ("sysenter") it was two instructions.

Actually, three instructions for "sysenter": the "movl %esp,%ebp" and the
actual "sysenter" instruction itself are the main restart sequence, but to 
make it look the same as the legacy code from a kernel restart standpoint 
there is also a short backwards jump there.

Anyway, you can't even trigger it with the single-step flag, since the
kernel will disable single-stepping in sysenter (and it only gets
re-enabled once the system call is done - including retries).

Since it literally requires an external interrupt to happen, it would be 
pretty hard to create a test-case for. Maybe a dual-CPU setup with the 
other CPU sending a signal (and the interrupt is the resultant IPI) at 
just the right moment..

		Linus

Re: [PATCH] PPC32: cancel syscall restart on signal delivery

[Paul Mackerras]

Linus Torvalds writes:

> On Sat, 15 Nov 2003, Paul Mackerras wrote:
> > 
> > BTW, do we have a test program that triggers the bug that this fixes?
> 
> No. In fact, it's an _incredibly_ tiny race, because the old code that 
> only handled it at the return path of the system call that got interrupted 
> would catch the thing in all cases _except_ if the system
> 
>  (a) decided to restart the system call
>  (b) got an interrupt _after_ the return to user mode but _before_ the 
>      system call instruction itself actually did the restart
>  (c) this interrupt caused a new signal with a handler (different from the
>      one that caused the restart, since that one by definition had no
>      handler) to the same program.

I just had another look at the x86 syscall exit code.  It looks to me
as though you have interrupts disabled all the way from before you
check current_thread_info()->flags, through the call to do_signal,
until you return to userspace.  Also, you deliver one signal and then
return to userspace even if there are more signals pending.  I could
be wrong about either or both assertions, of course, my x86 assembler
isn't particularly strong.

(Having interrupts disabled during do_signal is interesting, given
that its subroutines call __put_user and friends. :)

On PPC, I disable interrupts before checking the thread_info flags,
but then reenable them before calling do_signal.  Then after do_signal
returns, I check the thread_info flags again (having first disabled
interrupts), so if there is another signal pending, it will get
delivered before we return to userspace.

Thus I think the race was possibly a little wider on PPC than on x86:
we didn't have to get back to userspace, the interrupt could happen
during do_signal.

However, I think the race window was actually wider on x86 than you
seem to imply, since the external interrupt could become pending any
time from when you disable interrupts before checking the thread_info
flags until you return to userspace, and still cause the problem.
Still, either way it's a pretty narrow race.

> To make it even worse, even if the above incredibly unlikely thing
> actually _happens_, most of the time it wouldn't really matter. The worst
> case schenario is that the signal handler itself does a system call that
> needs restarting, in which case on signal handler return we now restart
> the _wrong_ system call once we return.

Now you have me scared, because I can't see where the restart_syscall
system call resets current_thread_info()->restart_block.fn to
do_no_restart_syscall.  Now, when we get an ignored signal during a
restartable system call, the signal code sets up userspace to do a
restart_syscall system call.  If we get the race and then deliver a
signal to the process, we don't undo the changes to userspace, we just
arrange that when it does the restart_syscall call, it will give the
same effect as if the original system call had returned -EINTR.

Now if the signal handler does a restartable system call, which ends
needing to be restarted, that will undo the change that we made on
signal delivery where we set restart_block.fn = do_no_restart_syscall.
In other words, we will return from the signal handler with
restart_block.fn set to something else, and promptly proceed to
restart the wrong syscall.

Am I missing something?  Perhaps we should reset restart_block.fn in
sys_{,rt_}sigreturn, or possibly in sys_restart_syscall.

Paul.

Re: [PATCH] PPC32: cancel syscall restart on signal delivery

[Linus Torvalds]

On Sat, 15 Nov 2003, Paul Mackerras wrote:
> 
> (Having interrupts disabled during do_signal is interesting, given
> that its subroutines call __put_user and friends. :)

Actually, look closer. We don't do that.

Why? Check out get_signal_to_deliver(). And grok the absolute horridness.

> Thus I think the race was possibly a little wider on PPC than on x86:
> we didn't have to get back to userspace, the interrupt could happen
> during do_signal.

No. If it happens during do_signal() (on x86 or ppc), we'd just not
_handle_ the signal at all. We'd return to user space, restart the system 
call, and handle the signal as the restarted system call is returning. No 
bug.

In short, even on ppc, the window _literally_ is "after we've returned, 
but before we restart". 

> Now you have me scared, because I can't see where the restart_syscall
> system call resets current_thread_info()->restart_block.fn to
> do_no_restart_syscall.

It doesn't. 

The rule is: the restart_block is _only_ meaningful if you return 
-ERESTART_BLOCK. So at any other time it contains stale data.

> Am I missing something?  Perhaps we should reset restart_block.fn in
> sys_{,rt_}sigreturn, or possibly in sys_restart_syscall.

You're missing that the only thing that ever looks at restart_block is the 
code that is inside the signal handling of ERESTART_BLOCK.

The bug was that sometimes we had _already_ done that ERESTART_BLOCK 
handling (correctly), but then basically "aborted" (thanks to another 
signal) before the restart had actually taken effect.

And that race literally is only in user mode.

		Linus

Re: [PATCH] PPC32: cancel syscall restart on signal delivery

[Linus Torvalds]

On Fri, 14 Nov 2003, Linus Torvalds wrote:
> 
> Why? Check out get_signal_to_deliver(). And grok the absolute horridness.

Btw, don't get me wrong - I'm not proud about it. I only noticed how
horrid it was of x86 to depend on this behaviour in do_signal() about a
week ago, and since it's not a bug I'm not fixing it for 2.6.0.

It may not be pretty, but it works.

		Linus

Re: [PATCH] PPC32: cancel syscall restart on signal delivery

[Paul Mackerras]

Linus Torvalds writes:

> Why? Check out get_signal_to_deliver(). And grok the absolute horridness.

Yes, that is pretty special, isn't it. :)

> The rule is: the restart_block is _only_ meaningful if you return 
> -ERESTART_BLOCK. So at any other time it contains stale data.
> 
> > Am I missing something?  Perhaps we should reset restart_block.fn in
> > sys_{,rt_}sigreturn, or possibly in sys_restart_syscall.
> 
> You're missing that the only thing that ever looks at restart_block is the 
> code that is inside the signal handling of ERESTART_BLOCK.

... and sys_restart_syscall().  If your statement was true, why was it
so important to reset restart_block.fn when we deliver a signal?

Seems to me that we can get into a situation where we are in a signal
handler, and the interrupted state has (1) eip/nip pointing at a
system call instruction and (2) the syscall number register (eax/r0)
containing __NR_restart_syscall.  Now, if we get into this state we
will initially have restart_block.fn == do_no_restart_syscall.  But
that can get changed by the signal handler.

Now, when we resume that context we will call sys_restart_syscall
which will call restart_block.fn.  Which won't necessarily still point
to do_no_restart_syscall.  So I still think we have a problem.

Paul.

Re: [PATCH] PPC32: cancel syscall restart on signal delivery

[Linus Torvalds]

On Sat, 15 Nov 2003, Paul Mackerras wrote:
> 
> Now, when we resume that context we will call sys_restart_syscall
> which will call restart_block.fn.  Which won't necessarily still point
> to do_no_restart_syscall.  So I still think we have a problem.

Excellent point. We're actually much better off resetting it at signal
return.

Which should make all other resets unnecessary.

Yes, you can get out of a signal by using longjmp, but that doesn't
matter: you can't longjump to a restart call (well, you can, but only if
the user literally tries to do the restart by hand, ie he _intended_ to do
it).

So the _proper_ fix (for x86) should be as appended. Agreed?

		Linus

----
===== arch/i386/kernel/signal.c 1.33 vs edited =====
--- 1.33/arch/i386/kernel/signal.c	Tue Nov 11 21:18:46 2003
+++ edited/arch/i386/kernel/signal.c	Fri Nov 14 21:13:09 2003
@@ -132,6 +132,9 @@
 {
 	unsigned int err = 0;
 
+	/* Always make any pending restarted system calls return -EINTR */
+	current_thread_info()->restart_block.fn = do_no_restart_syscall;
+
 #define COPY(x)		err |= __get_user(regs->x, &sc->x)
 
 #define COPY_SEG(seg)							\
@@ -503,9 +506,6 @@
 	struct pt_regs * regs)
 {
 	struct k_sigaction *ka = &current->sighand->action[sig-1];
-
-	/* Always make any pending restarted system calls return -EINTR */
-	current_thread_info()->restart_block.fn = do_no_restart_syscall;
 
 	/* Are we from a system call? */
 	if (regs->orig_eax >= 0) {

Re: [PATCH] PPC32: cancel syscall restart on signal delivery

[Linus Torvalds]

On Fri, 14 Nov 2003, Linus Torvalds wrote:
> 
> So the _proper_ fix (for x86) should be as appended. Agreed?

Btw, while this won't oops the machine or anything like that, I consider
bugs like this where the kernel just silently does the wrong thing to be
potentially even more serious, since they cause endless head-scratching
and the "flaky" reports.

So if somebody can come up with a way to test this sanely, it would be
nice (thinking about it I suspect that you _should_ be able to trigger
this on x86 by using the old "int 0x80" syscalls together with setting TF
in eflags).

			Linus

Re: [PATCH] PPC32: cancel syscall restart on signal delivery

[Paul Mackerras]

Linus Torvalds writes:

> Excellent point. We're actually much better off resetting it at signal
> return.
> 
> Which should make all other resets unnecessary.

Yes.

> Yes, you can get out of a signal by using longjmp, but that doesn't
> matter: you can't longjump to a restart call (well, you can, but only if
> the user literally tries to do the restart by hand, ie he _intended_ to do
> it).
> 
> So the _proper_ fix (for x86) should be as appended. Agreed?

Agreed.  Here is the patch for PPC.

Thanks,
Paul.

diff -urN linux-2.5/arch/ppc/kernel/signal.c pmac-2.5/arch/ppc/kernel/signal.c
--- linux-2.5/arch/ppc/kernel/signal.c	2003-11-15 16:55:18.618797320 +1100
+++ pmac-2.5/arch/ppc/kernel/signal.c	2003-11-15 17:09:53.145849000 +1100
@@ -418,6 +418,9 @@
 {
 	struct rt_sigframe __user *rt_sf;
 
+	/* Always make any pending restarted system calls return -EINTR */
+	current_thread_info()->restart_block.fn = do_no_restart_syscall;
+
 	rt_sf = (struct rt_sigframe __user *)
 		(regs->gpr[1] + __SIGNAL_FRAMESIZE + 16);
 	if (verify_area(VERIFY_READ, rt_sf, sizeof(struct rt_sigframe)))
@@ -513,6 +516,9 @@
 	struct mcontext __user *sr;
 	sigset_t set;
 
+	/* Always make any pending restarted system calls return -EINTR */
+	current_thread_info()->restart_block.fn = do_no_restart_syscall;
+
 	sc = (struct sigcontext __user *)(regs->gpr[1] + __SIGNAL_FRAMESIZE);
 	if (copy_from_user(&sigctx, sc, sizeof(sigctx)))
 		goto badframe;
@@ -583,9 +589,6 @@
 	if (signr == 0)
 		return 0;		/* no signals delivered */
 
-	/* Always make any pending restarted system calls return -EINTR */
-	current_thread_info()->restart_block.fn = do_no_restart_syscall;
-
 	if ((ka->sa.sa_flags & SA_ONSTACK) && current->sas_ss_size
 	    && !on_sig_stack(regs->gpr[1]))
 		newsp = current->sas_ss_sp + current->sas_ss_size;