Subject: unkillable process due to setup_frame() failure

From: Atsushi Nemoto <[email-masked]>
Date: Wed, 07 Sep 2005 01:42:34 +0900 (JST)

For a long time, this test program is unkillable (by SIGKILL) on
Linux/MIPS.

#include <stdio.h>
#include <stdlib.h>
#include <signal.h>

void sighandler(int sig)
{
	printf("SIGNAL %d!\n", sig);
	exit(2);
}
void setup_signal(int sig)
{
	struct sigaction act;
	memset(&act, 0, sizeof(act));
	act.sa_handler = sighandler;
	act.sa_flags = SA_NOMASK | SA_RESTART;
	sigaction(sig, &act, 0);
}

int main(int argc, char **argv)
{
	setup_signal(SIGTRAP);

	__asm__ __volatile__("move $29,$0");
	__asm__ __volatile__("break");
	printf("done!\n");
	return 0;
}

If we run this program,

1.  The "break" instruction raises a exception.
2.  The exception handler queues SIGTRAP(5).
3.  dequeue_signal() dequeue a signal with LOWEST number (i.e. SIGTRAP).
4.  setup_frame() fails due to bad stack pointer and queues SIGSEGV(11).
5.  returns to user process (pc unchanged).
6.  goto 1. (forever)

So, the process can not be kill by SIGKILL.  In 2.6.12, 'sigkill
priority fix' was applied to __dequeue_signal(), but it does not help
while the SIGTRAP is queued to tsk->pending but SIGKILL (by kill
command) is queued to tsk->signal->shared_pending.

I have two proposal fix:

1. Now do_signal() returns 0 if setup_frame() failed.  Therefore if
   do_signal returned 0 (and there is still pending signal), calling
   do_signal again will handle the SIGSIGV.  Like this:

--- linux-mips/arch/mips/kernel/signal.c	2005-08-30 11:02:00.000000000 +0900
+++ linux/arch/mips/kernel/signal.c	2005-09-06 15:07:45.000000000 +0900
@@ -481,6 +481,10 @@
 {
 	/* deal with pending signal delivery */
 	if (thread_info_flags & _TIF_SIGPENDING) {
-		current->thread.abi->do_signal(oldset, regs);
+		if (unlikely(!current->thread.abi->do_signal(oldset, regs)) &&
+		    unlikely(signal_pending(current))) {
+			/* SIGSEGV mighe be sent in setup_frame */
+			current->thread.abi->do_signal(oldset, regs);
+		}
 	}
 }

2. Make 'sigkill priority fix' can handle this case.  First search
   SIGKILL in tsk->pending and tsk->signal->shared_pending, then
   search another signals.  Like this:

--- linux-mips/kernel/signal.c	2005-08-29 08:41:01.000000000 +0900
+++ linux/kernel/signal.c	2005-09-07 01:33:52.338420760 +0900
@@ -520,19 +520,14 @@
 }
 
 static int __dequeue_signal(struct sigpending *pending, sigset_t *mask,
-			siginfo_t *info)
+			siginfo_t *info, int sig)
 {
-	int sig = 0;
-
-	/* SIGKILL must have priority, otherwise it is quite easy
-	 * to create an unkillable process, sending sig < SIGKILL
-	 * to self */
-	if (unlikely(sigismember(&pending->signal, SIGKILL))) {
-		if (!sigismember(mask, SIGKILL))
-			sig = SIGKILL;
-	}
-
-	if (likely(!sig))
+	if (sig) {
+		/* check signal with priority first */
+		if (likely(!sigismember(&pending->signal, sig)) ||
+		    sigismember(mask, sig))
+			sig = 0;
+	} else
 		sig = next_signal(pending, mask);
 	if (sig) {
 		if (current->notifier) {
@@ -561,10 +556,18 @@
  */
 int dequeue_signal(struct task_struct *tsk, sigset_t *mask, siginfo_t *info)
 {
-	int signr = __dequeue_signal(&tsk->pending, mask, info);
+	/* SIGKILL must have priority, otherwise it is quite easy
+	 * to create an unkillable process, sending sig < SIGKILL
+	 * to self */
+	int signr = __dequeue_signal(&tsk->pending, mask, info, SIGKILL);
+	if (likely(!signr))
+		signr = __dequeue_signal(&tsk->signal->shared_pending,
+					 mask, info, SIGKILL);
+	if (likely(!signr))
+		signr = __dequeue_signal(&tsk->pending, mask, info, 0);
 	if (!signr)
 		signr = __dequeue_signal(&tsk->signal->shared_pending,
-					 mask, info);
+					 mask, info, 0);
  	if (signr && unlikely(sig_kernel_stop(signr))) {
  		/*
  		 * Set a marker that we have dequeued a stop signal.  Our

Which are preferred?  Or is there other solutions?

---
Atsushi Nemoto

---

From: Ralf Baechle <[email-masked]>
Date: Tue, 6 Sep 2005 19:41:19 +0100

> command) is queued to tsk->signal->shared_pending.

The behaviour of not advancing the EPC beyond the faulting instruction is
part of the problem - but I believe that was the usual behaviour for
MIPS UNIXoid operating systems.

  Ralf

---

From: "Maciej W. Rozycki" <[email-masked]>
Date: Wed, 7 Sep 2005 10:14:16 +0100 (BST)

> MIPS UNIXoid operating systems.

 Well, SIGKILL should always work and frankly I can't see a reason to 
return back to user space in the affected context in the first place.  
What's left in EPC shouldn't matter.

  Maciej

---

From: Ralf Baechle <[email-masked]>
Date: Wed, 7 Sep 2005 14:47:17 +0100

> What's left in EPC shouldn't matter.

I said it's part of the problem - not that it should be changed.  The
behaviour as far as I can say is also standard for MIPS unixoid operating
systems.  That said, I definately prefer the approach of Atushi's suggested
fix #2.  The other question is why we try to continue if delivering a
signal failed and we already know that repeated attempts would fail again.

  Ralf

---

From: Atsushi Nemoto <[email-masked]>
Date: Wed, 07 Sep 2005 23:44:13 +0900 (JST)

>>>>> On Wed, 7 Sep 2005 14:47:17 +0100, Ralf Baechle <[email-masked]> said:

ralf> That said, I definately prefer the approach of Atushi's
ralf> suggested fix #2.  The other question is why we try to continue
ralf> if delivering a signal failed and we already know that repeated
ralf> attempts would fail again.

The question is for my fix #1 ?

Well, let me explain more.  With that fix, things will go like this:

1.  The "break" instruction raises a exception.
2.  The exception handler queues SIGTRAP(5).
3.  dequeue_signal() dequeue a signal with LOWEST number (i.e. SIGTRAP).
4.  setup_frame() fails due to bad stack pointer and queues SIGSEGV(11).
5.  do_signal() called again (by fix #1)
6.  dequeue_signal() dequeue a signal (i.e. SIGSEGV).
7.  If SIGSEGV did not have a signal handler, default action (coredump
    and exit) is taken.  End.
8.  If SIGSEGV had a signal handler, setup_frame() fails and queues
    SIGSEGV again.  It resets SIGSEGV's handler to SIG_DFL.
9.  returns to user process (pc unchanged).
10.  goto 1.  Then ended at 7.

My fix #2 is more generic approach, but even with the fix, the test
program eats CPU until killed by SIGKILL.  With my fix #1, the test
program will exit immediately.  

So my "which is preferred" question was inappropriate.  I had to ask
"#1 or #2 or both or other ?"

---
Atsushi Nemoto

---

From: Atsushi Nemoto <[email-masked]>
Date: Wed, 07 Sep 2005 23:56:49 +0900 (JST)

>>>>> On Wed, 07 Sep 2005 23:44:13 +0900 (JST), Atsushi Nemoto <[email-masked]> said:

anemo> My fix #2 is more generic approach, but even with the fix, the
anemo> test program eats CPU until killed by SIGKILL.  With my fix #1,
anemo> the test program will exit immediately.

Oh, I forgot to say: my fix #1 will not work on 64bit kernel due to
another problem.  I posted a patch for it a few days ago.

http://www.linux-mips.org/cgi-bin/mesg.cgi?a=linux-mips&i=20050901.011551.96687558.anemo%40mba.ocn.ne.jp

Please apply this first.  Thank you.

---
Atsushi Nemoto

---

From: "Maciej W. Rozycki" <[email-masked]>
Date: Wed, 7 Sep 2005 16:24:05 +0100 (BST)

> "#1 or #2 or both or other ?"

 We should be consistent with other platforms -- having a look at e.g. the 
i386 (as it used to be the reference) and the alpha (as close-enough to 
MIPS) should reveal the answer.  IIRC, a SIGSEGV that has a handler 
installed, but which cannot be callled due to a bad stack pointer is 
forced to SIG_DFL, but you may want to double-check it.

  Maciej

---

From: Ralf Baechle <[email-masked]>
Date: Wed, 7 Sep 2005 17:11:57 +0100

> forced to SIG_DFL, but you may want to double-check it.

That's what's already happening.  We call force_sigsegv which is like
force_sig unless it's trying to deliver a SIGSEGV in which case it'll
reset the handler to SIG_DFL, return to userspace where it hits the
break instruction and starts all over to process the SIGTRAP.

  Ralf

---

From: "Maciej W. Rozycki" <[email-masked]>
Date: Wed, 7 Sep 2005 17:34:57 +0100 (BST)

> break instruction and starts all over to process the SIGTRAP.

 Except that SIG_DFL for SIGSEGV is killing the process (with a core 
dump).  Therefore user space shouldn't ever be reached again in this 
context.

  Maciej

---

From: Atsushi Nemoto <[email-masked]>
Date: Fri, 16 Sep 2005 01:17:38 +0900 (JST)

>>>>> On Wed, 07 Sep 2005 23:44:13 +0900 (JST), Atsushi Nemoto <[email-masked]> said:

anemo> So my "which is preferred" question was inappropriate.  I had
anemo> to ask "#1 or #2 or both or other ?"

In 2.6.14-rc1, another fix is done in arch/i386/kernel/entry.S.
It also fixes a race condition in signal delivery.

>    [PATCH] i386: Don't miss pending signals returning to user mode after signal processing
>    
>    Signed-off-by: Roland McGrath <[email-masked]>

Let's follow.

--- linux-mips/arch/mips/kernel/entry.S	2005-03-04 22:17:29.000000000 +0900
+++ linux/arch/mips/kernel/entry.S	2005-09-16 01:04:52.365022536 +0900
@@ -105,7 +105,7 @@
 	move	a0, sp
 	li	a1, 0
 	jal	do_notify_resume	# a2 already loaded
-	j	restore_all
+	j	resume_userspace
 
 FEXPORT(syscall_exit_work_partial)
 	SAVE_STATIC

---
Atsushi Nemoto

---

From: Ralf Baechle <[email-masked]>
Date: Thu, 15 Sep 2005 17:44:33 +0100

> Let's follow.

Indeed.  Applied,

  Ralf