setup_frame() failure

setup_frame() failure

[Atsushi Nemoto]

[-- Attachment #1: Type: Text/Plain, Size: 1439 bytes --]

On 07 Dec 2000, Carsten Langgaard posted a question about failure of
setup_frame() function. (and there was no response on ML)

I found that if setup_frame() fails in certain conditions the process
which caused the signal grabs CPU and never be killed.

Here is a sample test program.

#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(SIGILL);
	setup_signal(SIGQUIT);
	setup_signal(SIGINT);

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


This program setups signal handlers and causes Coprocessor Unusable
Exception with $sp == 0.

If we run this program,

1.  "mfc1" instruction raises a exception.
2.  The exception handler queues SIGILL(4).
3.  do_signal() dequeue a signal with LOWEST number.
4.  setup_frame() fails and queues SIGSEGV(11).
5.  returns to user process.
6.  again from 1. (forever)

So, even SIGKILL can not kill the process.  (SIGHUP can do it).

I make a change for do_signal() to check failure of setup_frame() and
continue processing pending signals.  It seems work for me.  Here is
the patch.  Any comments are welcome.

---
Atsushi Nemoto

[-- Attachment #2: signal.c.patch --]
[-- Type: Text/Plain, Size: 2167 bytes --]

diff -ur linux.sgi/arch/mips/kernel/signal.c linux/arch/mips/kernel/signal.c
--- linux.sgi/arch/mips/kernel/signal.c	Mon Jun 25 22:56:56 2001
+++ linux/arch/mips/kernel/signal.c	Fri Sep  7 18:52:23 2001
@@ -382,7 +382,7 @@
 	return (void *)((sp - frame_size) & ALMASK);
 }
 
-static void inline
+static int inline
 setup_frame(struct k_sigaction * ka, struct pt_regs *regs,
             int signr, sigset_t *set)
 {
@@ -437,15 +437,16 @@
 	printk("SIG deliver (%s:%d): sp=0x%p pc=0x%p ra=0x%p\n",
 	       current->comm, current->pid, frame, regs->cp0_epc, frame->code);
 #endif
-        return;
+	return 0;
 
 give_sigsegv:
 	if (signr == SIGSEGV)
 		ka->sa.sa_handler = SIG_DFL;
 	force_sig(SIGSEGV, current);
+	return SIGSEGV;
 }
 
-static void inline
+static int inline
 setup_rt_frame(struct k_sigaction * ka, struct pt_regs *regs,
                int signr, sigset_t *set, siginfo_t *info)
 {
@@ -513,22 +514,26 @@
 	printk("SIG deliver (%s:%d): sp=0x%p pc=0x%p ra=0x%p\n",
 	       current->comm, current->pid, frame, regs->cp0_epc, frame->code);
 #endif
-	return;
+	return 0;
 
 give_sigsegv:
 	if (signr == SIGSEGV)
 		ka->sa.sa_handler = SIG_DFL;
 	force_sig(SIGSEGV, current);
+	return SIGSEGV;
 }
 
-static inline void
+static inline int
 handle_signal(unsigned long sig, struct k_sigaction *ka,
 	siginfo_t *info, sigset_t *oldset, struct pt_regs * regs)
 {
+	int newsig;
 	if (ka->sa.sa_flags & SA_SIGINFO)
-		setup_rt_frame(ka, regs, sig, oldset, info);
+		newsig = setup_rt_frame(ka, regs, sig, oldset, info);
 	else
-		setup_frame(ka, regs, sig, oldset);
+		newsig = setup_frame(ka, regs, sig, oldset);
+	if (newsig)
+		return newsig;
 
 	if (ka->sa.sa_flags & SA_ONESHOT)
 		ka->sa.sa_handler = SIG_DFL;
@@ -539,6 +544,7 @@
 		recalc_sigpending(current);
 		spin_unlock_irq(&current->sigmask_lock);
 	}
+	return 0;
 }
 
 static inline void
@@ -672,8 +678,9 @@
 		if (regs->regs[0])
 			syscall_restart(regs, ka);
 		/* Whee!  Actually deliver the signal.  */
-		handle_signal(signr, ka, &info, oldset, regs);
-		return 1;
+		if (handle_signal(signr, ka, &info, oldset, regs) == 0)
+			return 1;
+		/* another signal queued.  continue. */
 	}
 
 	/*

Re: setup_frame() failure

[Ralf Baechle]

On Fri, Sep 07, 2001 at 08:26:52PM +0900, Atsushi Nemoto wrote:

> I found that if setup_frame() fails in certain conditions the process
> which caused the signal grabs CPU and never be killed.
> 
> Here is a sample test program.
> 
> #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(SIGILL);
> 	setup_signal(SIGQUIT);
> 	setup_signal(SIGINT);
> 
> 	__asm__ __volatile__("move $29,$0");
> 	__asm__ __volatile__("mfc0 $0,$0");
> 	printf("done!\n");
> 	return 0;
> }
> 
> 
> This program setups signal handlers and causes Coprocessor Unusable
> Exception with $sp == 0.
> 
> If we run this program,
> 
> 1.  "mfc1" instruction raises a exception.
> 2.  The exception handler queues SIGILL(4).
> 3.  do_signal() dequeue a signal with LOWEST number.
> 4.  setup_frame() fails and queues SIGSEGV(11).
> 5.  returns to user process.
> 6.  again from 1. (forever)
> 
> So, even SIGKILL can not kill the process.  (SIGHUP can do it).
> 
> I make a change for do_signal() to check failure of setup_frame() and
> continue processing pending signals.  It seems work for me.  Here is
> the patch.  Any comments are welcome.

Nice test case.  Thanks. I decied for a differnet fix attached below.

  Ralf

Index: arch/mips64/kernel/traps.c
===================================================================
RCS file: /home/pub/cvs/linux/arch/mips64/kernel/traps.c,v
retrieving revision 1.20
diff -u -r1.20 traps.c
--- arch/mips64/kernel/traps.c 2001/07/11 23:32:54 1.20  
+++ arch/mips64/kernel/traps.c 2001/09/07 23:29:16   
@@ -347,11 +347,9 @@
 
 void do_ri(struct pt_regs *regs)
 {
-	printk("Cpu%d[%s:%d] Illegal instruction at %08lx ra=%08lx\n",
-	        smp_processor_id(), current->comm, current->pid, regs->cp0_epc, 
-		regs->regs[31]);
 	if (compute_return_epc(regs))
 		return;
+
 	force_sig(SIGILL, current);
 }
 
@@ -388,6 +386,7 @@
 	return;
 
 bad_cid:
+	compute_return_epc(regs);
 	force_sig(SIGILL, current);
 }
 
Index: arch/mips/kernel/traps.c
===================================================================
RCS file: /home/pub/cvs/linux/arch/mips/kernel/traps.c,v
retrieving revision 1.78
diff -u -r1.78 traps.c
--- arch/mips/kernel/traps.c 2001/09/06 13:22:24 1.78  
+++ arch/mips/kernel/traps.c 2001/09/07 23:29:16   
@@ -606,9 +606,6 @@
 
 asmlinkage void do_ri(struct pt_regs *regs)
 {
-	unsigned int opcode;
-
-	get_insn_opcode(regs, &opcode);
 	if (compute_return_epc(regs))
 		return;
 
@@ -659,6 +656,7 @@
 	return;
 
 bad_cid:
+	compute_return_epc(regs);
 	force_sig(SIGILL, current);
 }
 

Re: setup_frame() failure

[Atsushi Nemoto]

>>>>> On Sat, 8 Sep 2001 01:36:38 +0200, Ralf Baechle <ralf@oss.sgi.com> said:
>> I make a change for do_signal() to check failure of setup_frame()
>> and continue processing pending signals.  It seems work for me.
>> Here is the patch.  Any comments are welcome.

ralf> Nice test case.  Thanks. I decied for a differnet fix attached
ralf> below.

I think your fix is good for Coprocessor Unusable exception.  And same
fix required for Trap or Breakpoint exception.  (Am I right?)

But, does not a debugger confused by skipping the instruction which
cause Trap or Breakpoint exception?  (I do not know much about
communication between kernel and debugger...)

---
Atsushi Nemoto

Re: setup_frame() failure

[Atsushi Nemoto]

>>>>> On Mon, 10 Sep 2001 11:44:02 +0900 (JST), Atsushi Nemoto <nemoto@toshiba-tops.co.jp> said:
ralf> Nice test case.  Thanks. I decied for a differnet fix attached
ralf> below.

nemoto> I think your fix is good for Coprocessor Unusable exception.
nemoto> And same fix required for Trap or Breakpoint exception.  (Am I
nemoto> right?)

nemoto> But, does not a debugger confused by skipping the instruction
nemoto> which cause Trap or Breakpoint exception?  (I do not know much
nemoto> about communication between kernel and debugger...)

I tried same fix for Trap exception (I inserted compute_return_epc()
before force_sig(SIGTRAP, current) line in do_tr()).  With this fix,
gdb did not work correctly.

So we should take another fix (at least for Trap exception) ?

---
Atsushi Nemoto

Re: setup_frame() failure

[Ralf Baechle]

On Wed, Sep 12, 2001 at 01:09:14PM +0900, Atsushi Nemoto wrote:

> nemoto> But, does not a debugger confused by skipping the instruction
> nemoto> which cause Trap or Breakpoint exception?  (I do not know much
> nemoto> about communication between kernel and debugger...)
> 
> I tried same fix for Trap exception (I inserted compute_return_epc()
> before force_sig(SIGTRAP, current) line in do_tr()).  With this fix,
> gdb did not work correctly.
> 
> So we should take another fix (at least for Trap exception) ?

Below a fix.  It's not the real thing but at least solved the problem
pretty reliable as normal compiler generated code will never place trap
and break instructions in delay slots.  The actual fix should be skipping
over the faulting instruction when returning from the signal handler.

  Ralf

Index: arch/mips64/kernel/traps.c
===================================================================
RCS file: /home/pub/cvs/linux/arch/mips64/kernel/traps.c,v
retrieving revision 1.21
diff -u -r1.21 traps.c
--- arch/mips64/kernel/traps.c 2001/09/07 23:35:57 1.21  
+++ arch/mips64/kernel/traps.c 2001/09/13 01:01:25   
@@ -291,7 +291,7 @@
 			info.si_code = FPE_INTOVF;
 		info.si_signo = SIGFPE;
 		info.si_errno = 0;
-		info.si_addr = (void *)compute_return_epc(regs);
+		info.si_addr = (void *)regs->cp0_epc;
 		force_sig_info(SIGFPE, &info, current);
 		break;
 	default:
@@ -333,7 +333,7 @@
 			info.si_code = FPE_INTOVF;
 		info.si_signo = SIGFPE;
 		info.si_errno = 0;
-		info.si_addr = (void *)compute_return_epc(regs);
+		info.si_addr = (void *)regs->cp0_epc;
 		force_sig_info(SIGFPE, &info, current);
 		break;
 	default:
Index: arch/mips/kernel/traps.c
===================================================================
RCS file: /home/pub/cvs/linux/arch/mips/kernel/traps.c,v
retrieving revision 1.79
diff -u -r1.79 traps.c
--- arch/mips/kernel/traps.c 2001/09/07 23:35:57 1.79  
+++ arch/mips/kernel/traps.c 2001/09/13 01:01:25   
@@ -424,7 +424,7 @@
 			info.si_code = FPE_INTOVF;
 		info.si_signo = SIGFPE;
 		info.si_errno = 0;
-		info.si_addr = (void *)compute_return_epc(regs);
+		info.si_addr = (void *)regs->cp0_epc;
 		force_sig_info(SIGFPE, &info, current);
 		break;
 	default:
@@ -464,7 +464,7 @@
 			info.si_code = FPE_INTOVF;
 		info.si_signo = SIGFPE;
 		info.si_errno = 0;
-		info.si_addr = (void *)compute_return_epc(regs);
+		info.si_addr = (void *)regs->cp0_epc;
 		force_sig_info(SIGFPE, &info, current);
 		break;
 	default:

Re: setup_frame() failure

[Atsushi Nemoto]

>>>>> On Thu, 13 Sep 2001 03:11:19 +0200, Ralf Baechle <ralf@oss.sgi.com> said:
ralf> The actual fix should be skipping over the faulting instruction
ralf> when returning from the signal handler.

Since the signal handler may want to know the faulting instruction,
the "skipping" should be done AFTER the returning from the handler.
On the other hand, the handler may do the "skipping" by itself...

The symptom I reported first ("the process can not be killd by
SIGKILL") does not occur if the signal handler executed successfully
because do_signal() will be called when returning from sys_sygreturn.
The symptom occur if setup_frame() failed.  So I still think there is
a point to check a failure of setup_frame().

---
Atsushi Nemoto

Re: setup_frame() failure

[Ralf Baechle]

On Fri, Sep 14, 2001 at 11:16:32AM +0900, Atsushi Nemoto wrote:

> ralf> The actual fix should be skipping over the faulting instruction
> ralf> when returning from the signal handler.
> 
> Since the signal handler may want to know the faulting instruction,
> the "skipping" should be done AFTER the returning from the handler.
> On the other hand, the handler may do the "skipping" by itself...
> 
> The symptom I reported first ("the process can not be killd by
> SIGKILL") does not occur if the signal handler executed successfully
> because do_signal() will be called when returning from sys_sygreturn.
> The symptom occur if setup_frame() failed.  So I still think there is
> a point to check a failure of setup_frame().

Certain I/O models use a large number of signals so we're trying hard to
keep signal latency down.  The current code already can guarantee proper
termination in case of a stack fault, just not the shortest way.

  Ralf

Re: setup_frame() failure

[Atsushi Nemoto]

Thanks for your answer.

>>>>> On Fri, 14 Sep 2001 22:14:55 +0200, Ralf Baechle <ralf@oss.sgi.com> said:
ralf> Certain I/O models use a large number of signals so we're trying
ralf> hard to keep signal latency down.

I agree.

ralf> The current code already can guarantee proper termination in
ralf> case of a stack fault, just not the shortest way.

I can not believe it.  There is a case we can not terminate the
process:

a)  If we installed a signal handler for SIGTRAP,
b)  and a signal queued by a "trap" instruction,
c)  and a stack fault occured.

I will show the reason (again):

1.  "trap" instruction raises a exception.
2.  The exception handler queues SIGTRAP(5).
3.  do_signal() dequeue a signal with LOWEST number.
4.  setup_frame() fails and queues SIGSEGV(11).
5.  returns to user process (without compute_return_epc()).
6.  again from 1. (forever)

In this case, dequeue_signal() always return SIGTRAP and any signal
which has a larger number than SIGTRAP never be processed, isn't it?
Or am I missing something?

Thank you.
---
Atsushi Nemoto

Re: setup_frame() failure

[Carsten Langgaard]

Ralf Baechle wrote:

> On Fri, Sep 07, 2001 at 08:26:52PM +0900, Atsushi Nemoto wrote:
>
> > I found that if setup_frame() fails in certain conditions the process
> > which caused the signal grabs CPU and never be killed.
> >
> > Here is a sample test program.
> >
> > #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(SIGILL);
> >       setup_signal(SIGQUIT);
> >       setup_signal(SIGINT);
> >
> >       __asm__ __volatile__("move $29,$0");
> >       __asm__ __volatile__("mfc0 $0,$0");
> >       printf("done!\n");
> >       return 0;
> > }
> >
> >
> > This program setups signal handlers and causes Coprocessor Unusable
> > Exception with $sp == 0.
> >
> > If we run this program,
> >
> > 1.  "mfc1" instruction raises a exception.
> > 2.  The exception handler queues SIGILL(4).
> > 3.  do_signal() dequeue a signal with LOWEST number.
> > 4.  setup_frame() fails and queues SIGSEGV(11).
> > 5.  returns to user process.
> > 6.  again from 1. (forever)
> >
> > So, even SIGKILL can not kill the process.  (SIGHUP can do it).
> >
> > I make a change for do_signal() to check failure of setup_frame() and
> > continue processing pending signals.  It seems work for me.  Here is
> > the patch.  Any comments are welcome.
>
> Nice test case.  Thanks. I decied for a differnet fix attached below.

I like the printout then getting a Reserved Instruction Exception, it indicates a
problem and things are much easier to debug when getting such messages.
So it would be a pity, if we need to get rid of that.


>
>   Ralf
>
> Index: arch/mips64/kernel/traps.c
> ===================================================================
> RCS file: /home/pub/cvs/linux/arch/mips64/kernel/traps.c,v
> retrieving revision 1.20
> diff -u -r1.20 traps.c
> --- arch/mips64/kernel/traps.c 2001/07/11 23:32:54 1.20
> +++ arch/mips64/kernel/traps.c 2001/09/07 23:29:16
> @@ -347,11 +347,9 @@
>
>  void do_ri(struct pt_regs *regs)
>  {
> -       printk("Cpu%d[%s:%d] Illegal instruction at %08lx ra=%08lx\n",
> -               smp_processor_id(), current->comm, current->pid, regs->cp0_epc,
> -               regs->regs[31]);
>         if (compute_return_epc(regs))
>                 return;
> +
>         force_sig(SIGILL, current);
>  }
>
> @@ -388,6 +386,7 @@
>         return;
>
>  bad_cid:
> +       compute_return_epc(regs);
>         force_sig(SIGILL, current);
>  }
>
> Index: arch/mips/kernel/traps.c
> ===================================================================
> RCS file: /home/pub/cvs/linux/arch/mips/kernel/traps.c,v
> retrieving revision 1.78
> diff -u -r1.78 traps.c
> --- arch/mips/kernel/traps.c 2001/09/06 13:22:24 1.78
> +++ arch/mips/kernel/traps.c 2001/09/07 23:29:16
> @@ -606,9 +606,6 @@
>
>  asmlinkage void do_ri(struct pt_regs *regs)
>  {
> -       unsigned int opcode;
> -
> -       get_insn_opcode(regs, &opcode);
>         if (compute_return_epc(regs))
>                 return;
>
> @@ -659,6 +656,7 @@
>         return;
>
>  bad_cid:
> +       compute_return_epc(regs);
>         force_sig(SIGILL, current);
>  }
>

--
_    _ ____  ___   Carsten Langgaard   Mailto:carstenl@mips.com
|\  /|||___)(___   MIPS Denmark        Direct: +45 4486 5527
| \/ |||    ____)  Lautrupvang 4B      Switch: +45 4486 5555
  TECHNOLOGIES     2750 Ballerup       Fax...: +45 4486 5556
                   Denmark             http://www.mips.com

Re: setup_frame() failure

[Ralf Baechle]

On Mon, Sep 10, 2001 at 09:28:02AM +0200, Carsten Langgaard wrote:

> I like the printout then getting a Reserved Instruction Exception, it
> indicates a problem and things are much easier to debug when getting
> such messages.  So it would be a pity, if we need to get rid of that.

A denial of service can be constructed from such printout, so this can't
stay in an release version.  Think of what

#include <signal.h>

static void si(void) { }

int main(int argc, char *argv[])
{
        signal(SIGILL, sh);

        while(1)
                asm volatile("mfc0 $0, $0");
}

would do if each exception would result in a line in syslog.

  Ralf