[PATCH 2.5.73] Fix broken signal optimization for i386

[PATCH 2.5.73] Fix broken signal optimization for i386

[Jörn Engel]

Hi!

I guess it is an optimizasion that there is no flag to indicate
whether a process is operating on the signal stack or not.  That
information is instead generated by checking whether the stack pointer
is inside the signal stack or not.

But the stack pointer is under the control of the userspace.  Thus, a
broken program can cause the kernel to have utterly unexpected
behaviour.  You can recreate this by running the program below, which
I would expect to cause a segmentation fault and dump a core in the
process.

This patch fixes the problem for i386.  It could still some work, but
at least, it is better than the current behaviour.  One possible
improvement may be to cause the final SIGSEGV a bit sooner, another to
fold the flag into an unused bit somewhere in struct task_struct,
possibly in the low bits of the two sas_ss_* fields.

Two more patches for ppc are in the queue, ppc64 should be quite
similar.  I guess those should go to Paul and Anton, right?

[ My fscking email is still broken for some, including kernel.org.
  The admin that thought DNS entries should be cached for a full week
  to save some network bandwidth ought to be shot after some very
  cruel torture.  Sorry, folks. ]

---<snip>---
#define __USE_BSD
#include <signal.h>


#if defined(i386)
# define killstack() asm("mov %ebx,0")
#elif defined(PPC)
# define killstack() asm("lis 1,0")
#else
# define killstack() fprintf(stderr, "cannot kill stack on this arch\n")
#endif


char stack[8192];

struct sigaltstack ss = {
	.ss_sp = stack,
	.ss_flags = 0,
	.ss_size = sizeof(stack),
};

void null_handler(int signal)
{
	printf("SIGNAL .... %d\n", signal);
	killstack();
}

int main(int argc, char** argv)
{
	struct sigaction sa = {
		.sa_handler     = null_handler,
		.sa_flags       = SA_ONSTACK,
	};

	sigaltstack(&ss, 0);
	sigaction(SIGSEGV, &sa, 0);
	killstack();
	return 0; /* yeah, right! */
}
---<snap>---

Jörn

-- 
"Error protection by error detection and correction."
-- from a university class

--- linux-2.5.73/arch/i386/kernel/signal.c~signal_i386	2003-07-03 19:17:21.000000000 +0200
+++ linux-2.5.73/arch/i386/kernel/signal.c	2003-07-03 22:04:30.000000000 +0200
@@ -181,6 +181,9 @@
 		}
 	}
 
+	if (sas_ss_flags(regs->esp) == 0)
+		current->sas_ss_inuse = 0;
+
 	err |= __get_user(*peax, &sc->eax);
 	return err;
 
@@ -317,9 +320,22 @@
 	esp = regs->esp;
 
 	/* This is the X/Open sanctioned signal stack switching.  */
-	if (ka->sa.sa_flags & SA_ONSTACK) {
-		if (sas_ss_flags(esp) == 0)
-			esp = current->sas_ss_sp + current->sas_ss_size;
+	if ((ka->sa.sa_flags & SA_ONSTACK) && (sas_ss_flags(esp) == 0)) {
+		/* If we have switches to the signal stack before,
+		 * something bad has happened to it, asking for a
+		 * segmentation fault.
+		 * If not, remember it for the next time
+		 */
+		if (current->sas_ss_inuse) {
+			ka->sa.sa_handler = SIG_DFL;
+			force_sig(SIGSEGV, current);
+			/* XXX would it be simpler to return some broken
+			 * value like NULL and have the calling function
+			 * signal the segv?
+			 */
+		}
+		current->sas_ss_inuse = 1;
+		esp = current->sas_ss_sp + current->sas_ss_size;
 	}
 
 	/* This is the legacy signal stack switching. */
--- linux-2.5.73/include/linux/sched.h~signal_i386	2003-07-03 19:17:21.000000000 +0200
+++ linux-2.5.73/include/linux/sched.h	2003-07-03 19:17:58.000000000 +0200
@@ -425,6 +425,7 @@
 
 	unsigned long sas_ss_sp;
 	size_t sas_ss_size;
+	int sas_ss_inuse;
 	int (*notifier)(void *priv);
 	void *notifier_data;
 	sigset_t *notifier_mask;

Re: [PATCH 2.5.73] Fix broken signal optimization for i386

[Jörn Engel]

On Thu, 3 July 2003 22:24:10 +0200, Jörn Engel wrote:
> 
> I guess it is an optimizasion that there is no flag to indicate
> whether a process is operating on the signal stack or not.  That
> information is instead generated by checking whether the stack pointer
> is inside the signal stack or not.

Ben Herrenschmidt agreed in principle with the patch and applied a
Mighty Cluebat (tm) to my brain to use a flag instead of an int.
After that treatment, the patch looks a bit better already.

Jörn

-- 
Fantasy is more important than knowlegde. Knowlegde is limited,
while fantasy embraces the whole world.
-- Albert Einstein

[PATCH 2.5.73] Signal stack fixes #1 introduce PF_SS_ACTIVE

[Jörn Engel]

Hi Linus!

This is the generic part of the signal stack fixes, it simply
introduces a new PF-flag that indicates whether we are using the
signal stack right now or not.

Please apply.

Jörn

-- 
Data expands to fill the space available for storage.
-- Parkinson's Law

--- linux-2.5.73/include/linux/sched.h~ss_generic	2003-07-04 18:57:01.000000000 +0200
+++ linux-2.5.73/include/linux/sched.h	2003-07-04 18:59:03.000000000 +0200
@@ -480,6 +480,7 @@
 #define PF_FSTRANS	0x00020000	/* inside a filesystem transaction */
 #define PF_KSWAPD	0x00040000	/* I am kswapd */
 #define PF_SWAPOFF	0x00080000	/* I am in swapoff */
+#define PF_SS_ACTIVE	0x00100000	/* Executing on signal stack now */
 #define PF_LESS_THROTTLE 0x01000000	/* Throttle me less: I clena memory */
 
 #ifdef CONFIG_SMP

[PATCH 2.5.73] Signal stack fixes #2 i386-specific

[Jörn Engel]

Hi Linus!

This is the i386 specific part of the signal stack fixes.  It sets the
flag, when switching to the signal stack and clears it, when switching
back.  When the kernel tries to switch to the signal stack again,
without switching back, the process screwed up the signal stack, so we
kill it with a SIGSEGV.

Actually, the process doesn't get killed right away yet, so there is
room for improvement, but the general behaviour is the right one.

Please apply.

Jörn

-- 
There's nothing better for promoting creativity in a medium than
making an audience feel "Hmm ­ I could do better than that!"
-- Douglas Adams in a slashdot interview

--- linux-2.5.73/arch/i386/kernel/signal.c~ss_i386	2003-07-04 18:57:01.000000000 +0200
+++ linux-2.5.73/arch/i386/kernel/signal.c	2003-07-04 18:59:04.000000000 +0200
@@ -181,6 +181,9 @@
 		}
 	}
 
+	if (sas_ss_flags(regs->esp) == 0)
+		current->flags &= ~PF_SS_ACTIVE;
+
 	err |= __get_user(*peax, &sc->eax);
 	return err;
 
@@ -317,9 +320,22 @@
 	esp = regs->esp;
 
 	/* This is the X/Open sanctioned signal stack switching.  */
-	if (ka->sa.sa_flags & SA_ONSTACK) {
-		if (sas_ss_flags(esp) == 0)
-			esp = current->sas_ss_sp + current->sas_ss_size;
+	if ((ka->sa.sa_flags & SA_ONSTACK) && (sas_ss_flags(esp) == 0)) {
+		/* If we have switches to the signal stack before,
+		 * something bad has happened to it, asking for a
+		 * segmentation fault.
+		 * If not, remember it for the next time
+		 */
+		if (current->flags & PF_SS_ACTIVE) {
+			ka->sa.sa_handler = SIG_DFL;
+			force_sig(SIGSEGV, current);
+			/* XXX would it be simpler to return some broken
+			 * value like NULL and have the calling function
+			 * signal the segv?
+			 */
+		}
+		current->flags |= PF_SS_ACTIVE;
+		esp = current->sas_ss_sp + current->sas_ss_size;
 	}
 
 	/* This is the legacy signal stack switching. */

Re: [PATCH 2.5.73] Signal stack fixes #1 introduce PF_SS_ACTIVE

[Jörn Engel]

Hi!

This should be the ppc specific part of the signal stack fixes.  It sets the
flag, when switching to the signal stack and clears it, when switching
back.  When the kernel tries to switch to the signal stack again,   
without switching back, the process screwed up the signal stack, so we
kill it with a SIGSEGV.

Well, it should be, but it ain't.  I didn't find the correct spot to
clear the flag again, so this patch is incomplete.  Maybe someone else
knows the 2.5 ppc signal handling better than I do?

Jörn

-- 
Good warriors cause others to come to them and do not go to others.
-- Sun Tzu

--- linux-2.5.73/arch/ppc/kernel/signal.c~ss_ppc	2003-07-04 19:01:55.000000000 +0200
+++ linux-2.5.73/arch/ppc/kernel/signal.c	2003-07-04 19:21:44.000000000 +0200
@@ -496,9 +496,18 @@
 	if (signr > 0) {
 		ka = &current->sighand->action[signr-1];
 		if ( (ka->sa.sa_flags & SA_ONSTACK)
-		     && (! on_sig_stack(regs->gpr[1])))
+		     && (! on_sig_stack(regs->gpr[1]))) {
+			/* FIXME: Need to find the correct spot to clear
+			 * this flag again
+			 */
+			if (current->flags & PF_SS_ACTIVE) {
+				ka->sa.sa_handler = SIG_DFL;
+				force_sig(SIGSEGV, current);
+				return 0;
+			}
+			current->flags |= PF_SS_ACTIVE;
 			newsp = (current->sas_ss_sp + current->sas_ss_size);
-		else
+		} else
 			newsp = regs->gpr[1];
 		newsp = frame = newsp - sizeof(struct sigregs);
 

[PATCH 2.5.73] Signal handling fix for ppc

[Jörn Engel]

Hi!

This patch causes an application with a broken stack to die with a
SIGSEGV (and a core dump and all that) instead of a silent do_exit.

Most other architectures already have it, so ppc should get it as
well.

Anton, did I understand it correctly that you already have the
equivalent for ppc64 in the queue?

Jörn

-- 
With a PC, I always felt limited by the software available. On Unix, 
I am limited only by my knowledge.
-- Peter J. Schoenster

--- linux-2.5.73/arch/ppc/kernel/signal.c~sigsegv_ppc	2003-07-03 19:17:21.000000000 +0200
+++ linux-2.5.73/arch/ppc/kernel/signal.c	2003-07-04 19:01:55.000000000 +0200
@@ -234,7 +234,9 @@
 	return 0;
 
 badframe:
-	do_exit(SIGSEGV);
+	if (sig == SIGSEGV)
+		ka->sa.sa_handler = SIG_DFL;
+	force_sig(SIGSEGV, current);
 }
 
 static void
@@ -285,7 +287,9 @@
 	printk("badframe in setup_rt_frame, regs=%p frame=%p newsp=%lx\n",
 	       regs, frame, newsp);
 #endif
-	do_exit(SIGSEGV);
+	if (sig == SIGSEGV)
+		ka->sa.sa_handler = SIG_DFL;
+	force_sig(SIGSEGV, current);
 }
 
 /*
@@ -332,7 +336,9 @@
 	return 0;
 
 badframe:
-	do_exit(SIGSEGV);
+	if (sig == SIGSEGV)
+		ka->sa.sa_handler = SIG_DFL;
+	force_sig(SIGSEGV, current);
 }	
 
 /*
@@ -375,7 +381,9 @@
 	printk("badframe in setup_frame, regs=%p frame=%p newsp=%lx\n",
 	       regs, frame, newsp);
 #endif
-	do_exit(SIGSEGV);
+	if (sig == SIGSEGV)
+		ka->sa.sa_handler = SIG_DFL;
+	force_sig(SIGSEGV, current);
 }
 
 /*
@@ -462,7 +470,9 @@
 	       regs, frame, *newspp);
 	printk("sc=%p sig=%d ka=%p info=%p oldset=%p\n", sc, sig, ka, info, oldset);
 #endif
-	do_exit(SIGSEGV);
+	if (sig == SIGSEGV)
+		ka->sa.sa_handler = SIG_DFL;
+	force_sig(SIGSEGV, current);
 }
 
 /*

Re: [PATCH 2.5.73] Signal stack fixes #1 introduce PF_SS_ACTIVE

[Linus Torvalds]

On Fri, 4 Jul 2003, Jörn Engel wrote:
> 
> This is the generic part of the signal stack fixes, it simply
> introduces a new PF-flag that indicates whether we are using the
> signal stack right now or not.

My reason for disliking this patch is that it adds user-space information 
to the kernel - in a place where user space cannot get at it.

In particular, any traditional cooperative user-space threading package
wants to switch its own stack around, and they all do it by just changing
%esp directly. The whole point of such threading is that it's _fast_,
since it doesn't need any kernel support (and since it's cooperative, you
can avoid locking).

The old "optimization" that you didn't like was not an optimization at 
all: it got the case of user space changing stacks _right_, while still 
allowing yet another stack for signal handling and exiting the signal by 
hand.

Does anybody do that? I don't know. But it was done the way it was done on 
purpose.

		Linus

Re: [PATCH 2.5.73] Signal stack fixes #1 introduce PF_SS_ACTIVE

[Jörn Engel]

On Fri, 4 July 2003 12:21:23 -0700, Linus Torvalds wrote:
> On Fri, 4 Jul 2003, Jörn Engel wrote:
> > 
> > This is the generic part of the signal stack fixes, it simply
> > introduces a new PF-flag that indicates whether we are using the
> > signal stack right now or not.
> 
> My reason for disliking this patch is that it adds user-space information 
> to the kernel - in a place where user space cannot get at it.

Which is the whole point of it.

> In particular, any traditional cooperative user-space threading package
> wants to switch its own stack around, and they all do it by just changing
> %esp directly. The whole point of such threading is that it's _fast_,
> since it doesn't need any kernel support (and since it's cooperative, you
> can avoid locking).
> 
> The old "optimization" that you didn't like was not an optimization at 
> all: it got the case of user space changing stacks _right_, while still 
> allowing yet another stack for signal handling and exiting the signal by 
> hand.

So some application has it's signal handler on the signal stack and
instead of returning to the kernel, it detect where it left off before
the signal, mangles the last two stack frames, and goes back directly?

If this is correct, it definitely saves a lot of time, but it also
means that the kernel has no way to ever detect a broken signal
handler, if it operates from the signal stack.

What is the point of the seperate signal stack anyway.  I like it,
because it allows me to handle signals, even when the normal stack is
broken for some reason.  So the point is robustness, not performance.
If that is the only point, I don't mind making the signal stack a bit
slower, but even more robust.

Jörn

-- 
I can say that I spend most of my time fixing bugs even if I have lots
of new features to implement in mind, but I give bugs more priority.
-- Andrea Arcangeli, 2000

Re: [PATCH 2.5.73] Signal stack fixes #1 introduce PF_SS_ACTIVE

[Davide Libenzi]

On Fri, 4 Jul 2003, Linus Torvalds wrote:

> My reason for disliking this patch is that it adds user-space information
> to the kernel - in a place where user space cannot get at it.
>
> In particular, any traditional cooperative user-space threading package
> wants to switch its own stack around, and they all do it by just changing
> %esp directly. The whole point of such threading is that it's _fast_,
> since it doesn't need any kernel support (and since it's cooperative, you
> can avoid locking).
>
> The old "optimization" that you didn't like was not an optimization at
> all: it got the case of user space changing stacks _right_, while still
> allowing yet another stack for signal handling and exiting the signal by
> hand.
>
> Does anybody do that? I don't know. But it was done the way it was done on
> purpose.

Yes, GNU Pth :

http://www.gnu.org/software/pth/

and, for example :

http://www.xmailserver.org/libpcl.html

They use the generic POSIX stack setup described here :

http://www.gnu.org/software/pth/rse-pmt.ps

My Pine's 'd' key deleted his patch before I could take an exhaustive
look, but it should be fine though. They both do use to enter the signal
handler simply to get a snapshot of the context, and they exit the handler
by letting the kernel to clean the on-sig-stack flag. try to search
archives to take a better look at the patch ...



- Davide

Re: [PATCH 2.5.73] Signal stack fixes #1 introduce PF_SS_ACTIVE

[Linus Torvalds]

On Fri, 4 Jul 2003, Jörn Engel wrote:
> 
> So some application has it's signal handler on the signal stack and
> instead of returning to the kernel, it detect where it left off before
> the signal, mangles the last two stack frames, and goes back directly?

Yeah, basically a lot of old threading stuff did the equivalent of 
longjump by hand.

It is entirely possible that they do not do this out of signal handlers, 
since that has its own set of problems anyway, and one of the reasons for 
doing co-operative user level threading is to not need locking, and thus 
you never want to do any thread switching asynchronously (eg from a signal 
context).

So I'm not saying that your patch will necessarily break stuff, I'm just 
pointing out that it was actually done the way it is done on purpose.

> If this is correct, it definitely saves a lot of time, but it also
> means that the kernel has no way to ever detect a broken signal
> handler, if it operates from the signal stack.

Sure it does. It can detect just about _any_ brokenness, except for the 
very rare case of total stack pointer corruption.

> What is the point of the seperate signal stack anyway.  I like it,
> because it allows me to handle signals, even when the normal stack is
> broken for some reason.

The people who use it tend to do user-space memory management, and for 
example put hard limits on their stack usage - possibly because they have 
a lot of stacks because they use threads.

Most of the time if the original stack is blown, the fault is
non-recoverable. But you can use the alternate stack to either just give a 
nice debug message (even in the presense of otherwise non-recoverable 
errors), _or_ you can actually do things like fix up the stack 
dynamically.

Quite frankly, for the recursive SIGSEGV problem, I'd much rather look at
the signal mask. If SIGSEGV is blocked, we should probably just kill the
program instead of clearing the blocking and trying to handle the SIGSEGV 
anyway. That should fix your test case, _without_ any subtle side effects.

		Linus

Re: [PATCH 2.5.73] Signal stack fixes #1 introduce PF_SS_ACTIVE

[Jörn Engel]

On Fri, 4 July 2003 13:06:50 -0700, Linus Torvalds wrote:
> 
> Yeah, basically a lot of old threading stuff did the equivalent of 
> longjump by hand.
> 
> It is entirely possible that they do not do this out of signal handlers, 
> since that has its own set of problems anyway, and one of the reasons for 
> doing co-operative user level threading is to not need locking, and thus 
> you never want to do any thread switching asynchronously (eg from a signal 
> context).
> 
> So I'm not saying that your patch will necessarily break stuff, I'm just 
> pointing out that it was actually done the way it is done on purpose.

And there actually is a possibility for my patch to break things.
Davide's example should still work, but there may be others.  Point
taken.

> Sure it does. It can detect just about _any_ brokenness, except for the 
> very rare case of total stack pointer corruption.

Well, that rare case is one that some people are quite concerned
about.  The sigaltstack is nice because it reduces the likelyhood, but
it doesn't cure the paranoia completely.

> The people who use it tend to do user-space memory management, and for 
> example put hard limits on their stack usage - possibly because they have 
> a lot of stacks because they use threads.
> 
> Most of the time if the original stack is blown, the fault is
> non-recoverable. But you can use the alternate stack to either just give a 
> nice debug message (even in the presense of otherwise non-recoverable 
> errors), _or_ you can actually do things like fix up the stack 
> dynamically.

Both of which I want to have, right.

> Quite frankly, for the recursive SIGSEGV problem, I'd much rather look at
> the signal mask. If SIGSEGV is blocked, we should probably just kill the
> program instead of clearing the blocking and trying to handle the SIGSEGV 
> anyway. That should fix your test case, _without_ any subtle side effects.

What do we do, if a program also uses SA_NOMASK for the SIGSEGV
handler?  This is totally stupid, I agree, but it is legal.  Should we
declare it illegal from this day on, or is that path blocked as well?

Jörn

-- 
When in doubt, use brute force.
-- Ken Thompson

Re: [PATCH 2.5.73] Signal stack fixes #1 introduce PF_SS_ACTIVE

[Jörn Engel]

On Fri, 4 July 2003 12:39:16 -0700, Davide Libenzi wrote:
> 
> Yes, GNU Pth :
> 
> http://www.gnu.org/software/pth/
> 
> and, for example :
> 
> http://www.xmailserver.org/libpcl.html
> 
> They use the generic POSIX stack setup described here :
> 
> http://www.gnu.org/software/pth/rse-pmt.ps

If I read that paper correctly, my patch should not break this
specific design.  In fact, it explicitly states the possibility of an
equivalent of my patch: "the signal handler scope is often just a flag
in the process control block (PCB) ..."

As Linus pointed out, there might be other cases though.

> My Pine's 'd' key deleted his patch before I could take an exhaustive
> look, but it should be fine though. They both do use to enter the signal
> handler simply to get a snapshot of the context, and they exit the handler
> by letting the kernel to clean the on-sig-stack flag. try to search
> archives to take a better look at the patch ...

The patches are small, so read below for convenience.

Jörn

-- 
It's just what we asked for, but not what we want!
-- anonymous

--- linux-2.5.73/include/linux/sched.h~ss_generic	2003-07-04 18:57:01.000000000 +0200
+++ linux-2.5.73/include/linux/sched.h	2003-07-04 18:59:03.000000000 +0200
@@ -480,6 +480,7 @@
 #define PF_FSTRANS	0x00020000	/* inside a filesystem transaction */
 #define PF_KSWAPD	0x00040000	/* I am kswapd */
 #define PF_SWAPOFF	0x00080000	/* I am in swapoff */
+#define PF_SS_ACTIVE	0x00100000	/* Executing on signal stack now */
 #define PF_LESS_THROTTLE 0x01000000	/* Throttle me less: I clena memory */
 
 #ifdef CONFIG_SMP

--- linux-2.5.73/arch/i386/kernel/signal.c~ss_i386	2003-07-04 18:57:01.000000000 +0200
+++ linux-2.5.73/arch/i386/kernel/signal.c	2003-07-04 18:59:04.000000000 +0200
@@ -181,6 +181,9 @@
 		}
 	}
 
+	if (sas_ss_flags(regs->esp) == 0)
+		current->flags &= ~PF_SS_ACTIVE;
+
 	err |= __get_user(*peax, &sc->eax);
 	return err;
 
@@ -317,9 +320,22 @@
 	esp = regs->esp;
 
 	/* This is the X/Open sanctioned signal stack switching.  */
-	if (ka->sa.sa_flags & SA_ONSTACK) {
-		if (sas_ss_flags(esp) == 0)
-			esp = current->sas_ss_sp + current->sas_ss_size;
+	if ((ka->sa.sa_flags & SA_ONSTACK) && (sas_ss_flags(esp) == 0)) {
+		/* If we have switches to the signal stack before,
+		 * something bad has happened to it, asking for a
+		 * segmentation fault.
+		 * If not, remember it for the next time
+		 */
+		if (current->flags & PF_SS_ACTIVE) {
+			ka->sa.sa_handler = SIG_DFL;
+			force_sig(SIGSEGV, current);
+			/* XXX would it be simpler to return some broken
+			 * value like NULL and have the calling function
+			 * signal the segv?
+			 */
+		}
+		current->flags |= PF_SS_ACTIVE;
+		esp = current->sas_ss_sp + current->sas_ss_size;
 	}
 
 	/* This is the legacy signal stack switching. */

Re: [PATCH 2.5.73] Signal stack fixes #1 introduce PF_SS_ACTIVE

[Paul Mackerras]

Jörn Engel writes:

> This should be the ppc specific part of the signal stack fixes.  It sets the
> flag, when switching to the signal stack and clears it, when switching
> back.  When the kernel tries to switch to the signal stack again,   
> without switching back, the process screwed up the signal stack, so we
> kill it with a SIGSEGV.

This is madness.

There is nothing in POSIX that says that you have to exit a signal
handler by returning from it (which, under Linux, ends up doing a
sigreturn or rt_sigreturn system call).  It is explicitly permitted to
return from a RT signal handler with setcontext(), for instance.  And
it is at least long-standing practice to return using longjmp().
Neither setcontext nor longjmp will do a system call (yes, setcontext
is a system call on sparc, but it isn't on x86 AFAIK).

So - the kernel doesn't (and can't and shouldn't need to) know about
all transitions to or from a signal stack.  Therefore the PF_SS_ACTIVE
bit is useless since it will be wrong some of the time.

Anyway, what is the problem with taking a signal on the signal stack
when you in a signal handler using the signal stack?  You just keep
going down the stack from where you are, which is what the code
already does.

BTW, I am the PPC maintainer; Ben is the powermac maintainer.

Paul.

Re: [PATCH 2.5.73] Signal handling fix for ppc

[Paul Mackerras]

Jörn Engel writes:

> This patch causes an application with a broken stack to die with a
> SIGSEGV (and a core dump and all that) instead of a silent do_exit.
> 
> Most other architectures already have it, so ppc should get it as
> well.

OK.  The changes are OK in principle but your patch is a bit borken
since you add a check "if (sig == SIGSEGV)" in sys_sigreturn and
sys_rt_sigreturn, but there is no variable called "sig" in those
functions.

I have some other signal changes pending.  I'll roll in your changes
and push it on to Linus shortly.

Paul.

Re: [PATCH 2.5.73] Signal stack fixes #1 introduce PF_SS_ACTIVE

[Linus Torvalds]

On Fri, 4 Jul 2003, Jörn Engel wrote:
> 
> > Quite frankly, for the recursive SIGSEGV problem, I'd much rather look at
> > the signal mask. If SIGSEGV is blocked, we should probably just kill the
> > program instead of clearing the blocking and trying to handle the SIGSEGV 
> > anyway. That should fix your test case, _without_ any subtle side effects.
> 
> What do we do, if a program also uses SA_NOMASK for the SIGSEGV
> handler?  This is totally stupid, I agree, but it is legal.  Should we
> declare it illegal from this day on, or is that path blocked as well?

I think we should just continue to do what we do now - sure, we'll loop on 
SIGSEGV, but hey, it's a user space bug, it's not the kernels problem. 
It's better to let people continue to do stupid things than try to force 
changes.

So how about something like the appended? Very simple patch,i and in fact 
it's more logical than the old behaviour (the old behaviour punched 
through blocked signals, the new ones says "if you block or ignore the 
signal we will just kill you through the default action").

		Linus

---
--- 1.86/kernel/signal.c	Mon Jun  2 13:37:11 2003
+++ edited/kernel/signal.c	Fri Jul  4 17:29:43 2003
@@ -797,10 +797,11 @@
 	int ret;
 
 	spin_lock_irqsave(&t->sighand->siglock, flags);
-	if (t->sighand->action[sig-1].sa.sa_handler == SIG_IGN)
+	if (sigismember(&t->blocked, sig) || t->sighand->action[sig-1].sa.sa_handler == SIG_IGN) {
 		t->sighand->action[sig-1].sa.sa_handler = SIG_DFL;
-	sigdelset(&t->blocked, sig);
-	recalc_sigpending_tsk(t);
+		sigdelset(&t->blocked, sig);
+		recalc_sigpending_tsk(t);
+	}
 	ret = specific_send_sig_info(sig, info, t);
 	spin_unlock_irqrestore(&t->sighand->siglock, flags);
 

Re: [PATCH 2.5.73] Signal stack fixes #1 introduce PF_SS_ACTIVE

[Jörn Engel]

On Fri, 4 July 2003 17:39:01 -0700, Linus Torvalds wrote:
> 
> I think we should just continue to do what we do now - sure, we'll loop on 
> SIGSEGV, but hey, it's a user space bug, it's not the kernels problem. 
> It's better to let people continue to do stupid things than try to force 
> changes.
> 
> So how about something like the appended? Very simple patch,i and in fact 
> it's more logical than the old behaviour (the old behaviour punched 
> through blocked signals, the new ones says "if you block or ignore the 
> signal we will just kill you through the default action").

That seems to be the best solution.  Thanks!

> ---
> --- 1.86/kernel/signal.c	Mon Jun  2 13:37:11 2003
> +++ edited/kernel/signal.c	Fri Jul  4 17:29:43 2003
> @@ -797,10 +797,11 @@
>  	int ret;
>  
>  	spin_lock_irqsave(&t->sighand->siglock, flags);
> -	if (t->sighand->action[sig-1].sa.sa_handler == SIG_IGN)
> +	if (sigismember(&t->blocked, sig) || t->sighand->action[sig-1].sa.sa_handler == SIG_IGN) {
>  		t->sighand->action[sig-1].sa.sa_handler = SIG_DFL;
> -	sigdelset(&t->blocked, sig);
> -	recalc_sigpending_tsk(t);
> +		sigdelset(&t->blocked, sig);
> +		recalc_sigpending_tsk(t);
> +	}
>  	ret = specific_send_sig_info(sig, info, t);
>  	spin_unlock_irqrestore(&t->sighand->siglock, flags);
>  
> 

Jörn

-- 
Simplicity is prerequisite for reliability.
-- Edsger W. Dijkstra

Re: [PATCH 2.5.73] Signal handling fix for ppc

[Jörn Engel]

On Sat, 5 July 2003 09:26:40 +1000, Paul Mackerras wrote:
> 
> OK.  The changes are OK in principle but your patch is a bit borken
> since you add a check "if (sig == SIGSEGV)" in sys_sigreturn and
> sys_rt_sigreturn, but there is no variable called "sig" in those
> functions.

Yes, I just did stupid copy'n'paste.

> I have some other signal changes pending.  I'll roll in your changes
> and push it on to Linus shortly.

Cool, thanks!

Jörn

-- 
Do not stop an army on its way home.
-- Sun Tzu

Re: [PATCH 2.5.73] Signal stack fixes #1 introduce PF_SS_ACTIVE

[Jörn Engel]

On Sat, 5 July 2003 09:18:21 +1000, Paul Mackerras wrote:
> 
> This is madness.
> 
> There is nothing in POSIX that says that you have to exit a signal
> handler by returning from it (which, under Linux, ends up doing a
> sigreturn or rt_sigreturn system call).  It is explicitly permitted to
> return from a RT signal handler with setcontext(), for instance.  And
> it is at least long-standing practice to return using longjmp().
> Neither setcontext nor longjmp will do a system call (yes, setcontext
> is a system call on sparc, but it isn't on x86 AFAIK).
> 
> So - the kernel doesn't (and can't and shouldn't need to) know about
> all transitions to or from a signal stack.  Therefore the PF_SS_ACTIVE
> bit is useless since it will be wrong some of the time.

Ack.

> Anyway, what is the problem with taking a signal on the signal stack
> when you in a signal handler using the signal stack?  You just keep
> going down the stack from where you are, which is what the code
> already does.

The problem is with a broken signal handler, that moves the stack
pointer to nirvana.  You get a signal, set up the signal stack, move
the pointer to nirvana, get a signal, set up the signal stack, move
the pointer to nirvana, get a signal, ...

If I was just going down the signal stack, I would be perfectly happy,
but instead the kernel believes each signal is the very first on the
signal stack and sets it up again (and again...) each time.

> BTW, I am the PPC maintainer; Ben is the powermac maintainer.

Sorry about that.

Jörn

-- 
Measure. Don't tune for speed until you've measured, and even then
don't unless one part of the code overwhelms the rest.
-- Rob Pike

Re: [PATCH 2.5.73] Signal stack fixes #1 introduce PF_SS_ACTIVE

[Jörn Engel]

On Sat, 5 July 2003 09:30:31 +0200, Jörn Engel wrote:
> On Fri, 4 July 2003 17:39:01 -0700, Linus Torvalds wrote:
> > 
> > So how about something like the appended? Very simple patch,i and in fact 
> > it's more logical than the old behaviour (the old behaviour punched 
> > through blocked signals, the new ones says "if you block or ignore the 
> > signal we will just kill you through the default action").
> 
> That seems to be the best solution.  Thanks!

Except that the patch didn't match the description.  My test loops
just as happily as before and the conditional part of give_sigsegv is
pointless now.  That might really break some threading stuff.

Anyway, the idea still makes sense, so I will try to come up with a
working patch.

Jörn

-- 
"Translations are and will always be problematic. They inflict violence 
upon two languages." (translation from German)

Re: [PATCH 2.5.73] Signal stack fixes #1 introduce PF_SS_ACTIVE

[Jamie Lokier]

Jörn Engel wrote:
> > It is entirely possible that they do not do this out of signal handlers, 
> > since that has its own set of problems anyway, and one of the reasons for 
> > doing co-operative user level threading is to not need locking, and thus 
> > you never want to do any thread switching asynchronously (eg from a signal 
> > context).

longjmp() out of signal handlers has a fine tradition, not just for
threading but also code written for systems where SIGCLD doesn't
interrupt select(), to pick a real example.  Admittedly such code
doesn't have to be written that way on Linux, but it does exist and
has been run on Linux.  I doubt such code ever uses sigaltstack().

About co-operative threading: one of the points is that the locks are
cheaper, and it's possible for a thread to disable/enable pre-emption
very fast, with no system calls or locked memory cycles.  In that
environment, longjmp() or setcontext() out of timer signals makes sense.

Many years ago I looked at a paper about fixups from signal handlers,
an ML run-time environment on SunOS I think, and they decided that
longjmp() from the handler was not a reliable strategy.  What they did
instead was to change the instruction pointer in the sigcontext, and
return from the handler.  This works on Linux too, but there are two
disadvantages: 1. two system calls instead of one (which is an issue
when these are a high rate of SIGSEGVs for memory management); 2. the
code is necessarily architecture specific, whereas longjmp() from
timer signals is relatively portable.

-- Jamie

Re: [PATCH 2.5.73] Signal stack fixes #1 introduce PF_SS_ACTIVE

[Linus Torvalds]

On Sat, 5 Jul 2003, Jörn Engel wrote:
> 
> Except that the patch didn't match the description.  My test loops
> just as happily as before and the conditional part of give_sigsegv is
> pointless now.  That might really break some threading stuff.

Hmm? I tried it, and for me it does:

	torvalds@home:~> ./a.out 
	SIGNAL .... 11
	Segmentation fault

but I have to admit that I didn't even try it before my kernel change, so 
maybe it worked for me before too ;)

There could easily be glibc version issues here, ie maybe your library 
sets SA_NOMASK and mine doesn't.

		Linus

Re: [PATCH 2.5.73] Signal stack fixes #1 introduce PF_SS_ACTIVE

[Eric W. Biederman]

Linus Torvalds <torvalds@osdl.org> writes:

> On Fri, 4 Jul 2003, Jörn Engel wrote:
> > 
> > So some application has it's signal handler on the signal stack and
> > instead of returning to the kernel, it detect where it left off before
> > the signal, mangles the last two stack frames, and goes back directly?
> 
> Yeah, basically a lot of old threading stuff did the equivalent of 
> longjump by hand.
> 
> It is entirely possible that they do not do this out of signal handlers, 
> since that has its own set of problems anyway, and one of the reasons for 
> doing co-operative user level threading is to not need locking, and thus 
> you never want to do any thread switching asynchronously (eg from a signal 
> context).
> 
> So I'm not saying that your patch will necessarily break stuff, I'm just 
> pointing out that it was actually done the way it is done on purpose.

I would have to double check but I am pretty certain dosemu does
this when running dpmi applications.  An alternative stack is setup
for signals so we get a stack we can control, and if we want to
return to dosemu instead of the dpmi application we must change the
stack we return to. 

Eric

Re: [PATCH 2.5.73] Signal stack fixes #1 introduce PF_SS_ACTIVE

[Paul Mackerras]

Jörn Engel writes:

> The problem is with a broken signal handler, that moves the stack
> pointer to nirvana.  You get a signal, set up the signal stack, move
> the pointer to nirvana, get a signal, set up the signal stack, move
> the pointer to nirvana, get a signal, ...

You can get the same effect by doing kill(0, SIGINT) inside a handler
for SIGINT.  All you seem to be saying is "if you behave stupidly then
bad things happen to you".  I don't see that this example exposes any
bug or vulnerability in the kernel.

> If I was just going down the signal stack, I would be perfectly happy,
> but instead the kernel believes each signal is the very first on the
> signal stack and sets it up again (and again...) each time.

You had to go to some trouble to get this effect - you had to use an
asm statement to change the stack pointer, which is well and truly
into "undefined behaviour" territory, and so you deserve all you
get. :)  It's a very contrived example IMHO.

Paul.

Re: [PATCH 2.5.73] Signal stack fixes #1 introduce PF_SS_ACTIVE

[Jörn Engel]

On Sun, 6 July 2003 18:47:50 +1000, Paul Mackerras wrote:
> 
> You can get the same effect by doing kill(0, SIGINT) inside a handler
> for SIGINT.  All you seem to be saying is "if you behave stupidly then
> bad things happen to you".  I don't see that this example exposes any
> bug or vulnerability in the kernel.

Maybe we are just working under different assumptions, so let me
explain my background a little.

Two of the reasons, why open source works well, are frequent releases
and lots of feedback.  In the embedded world, a typical number of
releases is one and a typical amount of feedback is none.  So you
either create a perfect product or you arrange for feedback yourself.

Without any user interaction tools around, the best feedback you can
get is a core dump plus maybe some information from /proc.  Remember
the borken patch for ppc I sent to you?  We didn't get a core dump and
people were quite unhappy, so the investigation began.

In the course of the investigation, I found another spot, where we
didn't get a core dump, which started this whole thread.  Guess what,
people aren't happy either.  One workaround would be to never use the
signal stack, but if this can be fixed properly, I would see more
happy faces at work.  And I like happy faces.

> You had to go to some trouble to get this effect - you had to use an
> asm statement to change the stack pointer, which is well and truly
> into "undefined behaviour" territory, and so you deserve all you
> get. :)  It's a very contrived example IMHO.

There is an open source web server that, combined with a closed source
library, fscks up your stack pointer.  I don't know how they did it
and I don't even care.  What I do care about is that it happened, that
it can happen again any time, and that we handle this problem as
gracefully as possible.  A core dump is graceful, a do_exit(SIGSEGV),
as it was in the ppc code is not, and an inifite loop is anything but
graceful.

I agree that my initial patch can cause other problems, but the
problem itself should still get fixed.

Jörn

-- 
More computing sins are committed in the name of efficiency (without
necessarily achieving it) than for any other single reason - including
blind stupidity.
-- W. A. Wulf 

Re: [PATCH 2.5.73] Signal stack fixes #1 introduce PF_SS_ACTIVE

[Jörn Engel]

On Sat, 5 July 2003 10:16:26 -0700, Linus Torvalds wrote:
> 
> Hmm? I tried it, and for me it does:
> 
> 	torvalds@home:~> ./a.out 
> 	SIGNAL .... 11
> 	Segmentation fault

Weird.  I've fixed the bracket stuff, but that's about it.

> but I have to admit that I didn't even try it before my kernel change, so 
> maybe it worked for me before too ;)
> 
> There could easily be glibc version issues here, ie maybe your library 
> sets SA_NOMASK and mine doesn't.

Hopefully not.

But all that doesn't matter, as your change appears to be worse for
threading libraries, than mine.  Imagine a thread that almost busted
it's stack limit.  The signal code tries to add the extra stack
frames, fails, and calls force_sig(SIGSEGV).  Shooting through the
signal mask, the SIGSEGV can be handled by killing off one thread, or
maby even recovering.  With your patch, it will always get killed.

My current best idea is to check, whether the stack pointer is valid
before going to the signal stack.  As long as it points to memory that
is writable for the current process, things are not completely
hopeless.  When the stack is totally broken, kill that cancer cell.

Working on a translation to diff -u right now.

Jörn

-- 
Invincibility is in oneself, vulnerability is in the opponent.
-- Sun Tzu

[PATCH 2.5.74] Signal stack safety #2 i386 specific

[Jörn Engel]

On Sun, 6 July 2003 14:51:03 +0200, Jörn Engel wrote:
> 
> My current best idea is to check, whether the stack pointer is valid
> before going to the signal stack.  As long as it points to memory that
> is writable for the current process, things are not completely
> hopeless.  When the stack is totally broken, kill that cancer cell.

Also stupid.  People use a segfault handler on a signal stack,
*because* the previous stack may be broken.

Since any trick we try appears to (possibly) break some existing
software, the next idea is to add yet another flag to signal.h.  With
this, the user can decide whether he want to get extra safety or not.

Comments?

Jörn

-- 
The only real mistake is the one from which we learn nothing.
-- John Powell

--- linux-2.5.74/arch/i386/kernel/signal.c~ss_i386	2003-07-07 10:30:30.000000000 +0200
+++ linux-2.5.74/arch/i386/kernel/signal.c	2003-07-07 10:31:09.000000000 +0200
@@ -181,6 +181,9 @@
 		}
 	}
 
+	if (sas_ss_flags(regs->esp) == 0)
+		current->flags &= ~PF_SS_ACTIVE;
+
 	err |= __get_user(*peax, &sc->eax);
 	return err;
 
@@ -317,9 +320,23 @@
 	esp = regs->esp;
 
 	/* This is the X/Open sanctioned signal stack switching.  */
-	if (ka->sa.sa_flags & SA_ONSTACK) {
-		if (sas_ss_flags(esp) == 0)
-			esp = current->sas_ss_sp + current->sas_ss_size;
+	if ((ka->sa.sa_flags & SA_ONSTACK) && (sas_ss_flags(esp) == 0)) {
+		/* If we have switches to the signal stack before,
+		 * something bad has happened to it, asking for a
+		 * segmentation fault.
+		 * If not, remember it for the next time
+		 */
+		if ((ka->sa.sa_flags & SA_KERNEL_RET) &&
+				(current->flags & PF_SS_ACTIVE)) {
+			ka->sa.sa_handler = SIG_DFL;
+			force_sig(SIGSEGV, current);
+			/* XXX would it be simpler to return some broken
+			 * value like NULL and have the calling function
+			 * signal the segv?
+			 */
+		}
+		current->flags |= PF_SS_ACTIVE;
+		esp = current->sas_ss_sp + current->sas_ss_size;
 	}
 
 	/* This is the legacy signal stack switching. */
--- linux-2.5.74/include/asm-i386/signal.h~ss_i386	2003-07-07 10:30:30.000000000 +0200
+++ linux-2.5.74/include/asm-i386/signal.h	2003-07-07 11:29:42.000000000 +0200
@@ -76,6 +76,8 @@
  * SA_FLAGS values:
  *
  * SA_ONSTACK indicates that a registered stack_t will be used.
+ * SA_KERNEL_RET indices that the handler returns through the kernel, not
+ *               with longjmp or similar.
  * SA_INTERRUPT is a no-op, but left due to historical reasons. Use the
  * SA_RESTART flag to get restarting signals (which were the default long ago)
  * SA_NOCLDSTOP flag to turn off SIGCHLD when children stop.
@@ -89,6 +91,7 @@
 #define SA_NOCLDSTOP	0x00000001u
 #define SA_NOCLDWAIT	0x00000002u
 #define SA_SIGINFO	0x00000004u
+#define SA_KERNEL_RET	0x01000000u
 #define SA_ONSTACK	0x08000000u
 #define SA_RESTART	0x10000000u
 #define SA_NODEFER	0x40000000u

Re: [PATCH 2.5.73] Signal stack fixes #1 introduce PF_SS_ACTIVE

[Paul Mackerras]

=?iso-8859-1?Q?J=81=F6rn?= Engel writes:

> In the course of the investigation, I found another spot, where we
> didn't get a core dump, which started this whole thread.  Guess what,
> people aren't happy either.  One workaround would be to never use the
> signal stack, but if this can be fixed properly, I would see more
> happy faces at work.  And I like happy faces.

Well, the most reliable way to get a core dump when you trash the
stack or something is to have no SIGSEGV handler at all. :)

In any case, there are all sorts of things that could go wrong and
leave the process stuck in an infinite loop.  If you really want a
core dump when things go wrong then you probably need some sort of
watchdog process plus a way for one process to force another to dump
core and exit (like a SIGKILL but with a core dump as well).

> There is an open source web server that, combined with a closed source
> library, fscks up your stack pointer.  I don't know how they did it

On PPC?  Sounds like you are overrunning an array on the stack and
clearing the stack backchain word.

> and I don't even care.  What I do care about is that it happened, that
> it can happen again any time, and that we handle this problem as
> gracefully as possible.  A core dump is graceful, a do_exit(SIGSEGV),
> as it was in the ppc code is not, and an inifite loop is anything but
> graceful.
> 
> I agree that my initial patch can cause other problems, but the
> problem itself should still get fixed.

You haven't convinced me that the kernel is doing anything wrong or
even suboptimal - it seems to me that you have run into some
unintended consequences of your code, that's all.  You can't expect
the kernel to work around all the bugs in your user processes. :)

Paul.

Re: [PATCH 2.5.73] Signal stack fixes #1 introduce PF_SS_ACTIVE

[Paul Mackerras]

=?iso-8859-1?Q?J=81=F6rn?= Engel writes:

> A core dump is graceful, a do_exit(SIGSEGV),
> as it was in the ppc code is not, and an inifite loop is anything but
> graceful.

It just occurred to me that the simplest and best fix for the specific
problem you mention is for you to set the SA_ONESHOT flag when you
install the SIGSEGV handler.  That way, if you get another
segmentation violation while you are already in the SIGSEGV handler,
it will just dump core straight away.

Paul.

Re: [PATCH 2.5.73] Signal stack fixes #1 introduce PF_SS_ACTIVE

[Jörn Engel]

On Mon, 7 July 2003 21:33:45 +1000, Paul Mackerras wrote:
> 
> It just occurred to me that the simplest and best fix for the specific
> problem you mention is for you to set the SA_ONESHOT flag when you
> install the SIGSEGV handler.  That way, if you get another
> segmentation violation while you are already in the SIGSEGV handler,
> it will just dump core straight away.

That makes sense.  One would still have to re-enable the signal
handler inside the signal handler, but if htat is the last code before
function return, we should be safe.

Great idea, thanks!

Jörn

-- 
A defeated army first battles and then seeks victory.
-- Sun Tzu

Re: [PATCH 2.5.73] Signal stack fixes #1 introduce PF_SS_ACTIVE

[Jörn Engel]

On Mon, 7 July 2003 21:29:35 +1000, Paul Mackerras wrote:
> 
> Well, the most reliable way to get a core dump when you trash the
> stack or something is to have no SIGSEGV handler at all. :)

True.  For the old ppc code, the problem occurs with any other signal
as well, but the signal stack problem is specific to SIGSEGV and
possibly SIGBUS, depending on architecture.

> In any case, there are all sorts of things that could go wrong and
> leave the process stuck in an infinite loop.  If you really want a
> core dump when things go wrong then you probably need some sort of
> watchdog process plus a way for one process to force another to dump
> core and exit (like a SIGKILL but with a core dump as well).

That would be optimal, yes.  This external core dump signal doesn't
exist yet, does it?

> You haven't convinced me that the kernel is doing anything wrong or
> even suboptimal - it seems to me that you have run into some
> unintended consequences of your code, that's all.  You can't expect
> the kernel to work around all the bugs in your user processes. :)

Correct.  The userspace will have bugs and it will have plenty.  The
interesting question is only, if we can gather enough debugging data
to identify and fix those bugs, when they occur.  This is embedded, no
user is going to narrow down the problem.

Jörn

-- 
A defeated army first battles and then seeks victory.
-- Sun Tzu