Signal restarting happing multiple time in do_signal

Signal restarting happing multiple time in do_signal

[Ralf Baechle]

In the MIPS entry.S the symbol resume_userspace to the call to
do_notify_resume form a loop through which the kernel will iterate as
long as work is pending.  If we iterate through this loop more than once
with no signal pending for at least one but the last iteration we will
perform the syscall restarting multiple times resulting in a syscall
return prior to the the syscall instruction in userspace because the
return program counter will be adjusted several times by:

        regs->regs[7] = regs->regs[26];
        regs->cp0_epc -= 8;

resulting in return to userspace at a too low address.  On MIPS this often
happens to be a stackpointer adjustment but in many cases this may also
just be a harmless instruction such as reloading the syscall number so
may go unnoticed.

We were able to trigger this when debugging a multithreaded programs.
At least i386 uses virtually identical algorithms so I think is likely
to have the same issue.

The fix in case of MIPS is trivial:

diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
index 73f8aad..8676055 100644
--- a/arch/mips/kernel/signal.c
+++ b/arch/mips/kernel/signal.c
@@ -465,6 +464,7 @@ void do_signal(struct pt_regs *regs)
 			regs->regs[7] = regs->regs[26];
 			regs->cp0_epc -= 4;
 		}
+		regs->regs[0] = 0;	/* Don't deal with this again.  */
 	}
 
 	/*

  Ralf

Re: Signal restarting happing multiple time in do_signal

[David Miller]

From: Ralf Baechle <ralf@linux-mips.org>
Date: Tue, 8 Aug 2006 12:04:37 +0100

> We were able to trigger this when debugging a multithreaded programs.
> At least i386 uses virtually identical algorithms so I think is likely
> to have the same issue.

Other platforms, including x86, clear the state out in one way or
another before the next iteration.

Actually on x86 it occurs as a side effect of setting
regs->eax, it is reset to the system call number requested
at syscall trap time, so it will not match any of the
error return values in this switch statement:

	/* Are we from a system call? */
	if (regs->orig_eax >= 0) {
		/* If so, check system call restarting.. */
		switch (regs->eax) {
		        case -ERESTART_RESTARTBLOCK:
			case -ERESTARTNOHAND:
				regs->eax = -EINTR;
				break;

			case -ERESTARTSYS:
				if (!(ka->sa.sa_flags & SA_RESTART)) {
					regs->eax = -EINTR;
					break;
				}
			/* fallthrough */
			case -ERESTARTNOINTR:
				regs->eax = regs->orig_eax;
				regs->eip -= 2;
		}
	}

So after the first iteration, the next time through here we
won't be "from a system call".

Re: Signal restarting happing multiple time in do_signal

[Andi Kleen]

> 
> So after the first iteration, the next time through here we
> won't be "from a system call".

Thanks for the analysis David. I agree it shouldn't be a problem for x86.

-Andi

Re: Signal restarting happing multiple time in do_signal

[Heiko Carstens]

> In the MIPS entry.S the symbol resume_userspace to the call to
> do_notify_resume form a loop through which the kernel will iterate as
> long as work is pending.  If we iterate through this loop more than once
> with no signal pending for at least one but the last iteration we will
> perform the syscall restarting multiple times resulting in a syscall
> return prior to the the syscall instruction in userspace because the
> return program counter will be adjusted several times by:
> 
>         regs->regs[7] = regs->regs[26];
>         regs->cp0_epc -= 8;
> 
> resulting in return to userspace at a too low address.  On MIPS this often
> happens to be a stackpointer adjustment but in many cases this may also
> just be a harmless instruction such as reloading the syscall number so
> may go unnoticed.
> 
> We were able to trigger this when debugging a multithreaded programs.
> At least i386 uses virtually identical algorithms so I think is likely
> to have the same issue.

s390 has the same problem. Thanks for pointing this out.
btw. is there a reason why mips doesn't have a 'return' within do_signal()
(see patch below)? That would save you some cycles.
Also I think that powerpc's do_signal() could be converted to be a void
function and that their EXPORT_SYMBOL(do_signal) could be removed. Would
also save some cycles/memory.

diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
index 6b4d9be..43c37d9 100644
--- a/arch/mips/kernel/signal.c
+++ b/arch/mips/kernel/signal.c
@@ -446,6 +446,7 @@ void do_signal(struct pt_regs *regs)
 			if (test_thread_flag(TIF_RESTORE_SIGMASK))
 				clear_thread_flag(TIF_RESTORE_SIGMASK);
 		}
+		return;
 	}
 
 no_signal: