signal-race-fix.patch

signal-race-fix.patch

[Andrew Morton]

Kind sirs,

We have an SMP race in the signal code.  A fix for x86 is below.  All archs
need updating.

Could you please send me the arch patches for this when convenient?  Once I
have a decent collection I'll merge it all up.  Or you may choose to wait
until this hits Linus's tree post-2.6.5 sometime.  Up to you.

Thanks.


From: Corey Minyard <minyard@acm.org>

The problem:

  In arch/i386/signal.c, in the do_signal() function, it calls
  get_signal_to_deliver() which returns the signal number to deliver (along
  with siginfo).  get_signal_to_deliver() grabs and releases the lock, so
  the signal handler lock is not held in do_signal().  Then the do_signal()
  calls handle_signal(), which uses the signal number to extract the
  sa_handler, etc.

  Since no lock is held, it seems like another thread with the same
  signal handler set can come in and call sigaction(), it can change
  sa_handler between the call to get_signal_to_deliver() and fetching the
  value of sa_handler.  If the sigaction() call set it to SIG_IGN, SIG_DFL,
  or some other fundamental change, that bad things can happen.

The patch:

  You have to get the sigaction information that will be delivered while
  holding sighand->siglock in get_signal_to_deliver().

  In 2.4, it can be fixed per-arch and requires no change to the
  arch-independent code because the arch fetches the signal with
  dequeue_signal() and does all the checking.

The test app:

  The program below has three threads that share signal handlers.  Thread
  1 changes the signal handler for a signal from a handler to SIG_IGN and
  back.  Thread 0 sends signals to thread 3, which just receives them. 
  What I believe is happening is that thread 1 changes the signal handler
  in the process of thread 3 receiving the signal, between the time that
  thread 3 fetches the signal info using get_signal_to_deliver() and
  actually delivers the signal with handle_signal().

  Although the program is obvously an extreme case, it seems like any
  time you set the handler value of a signal to SIG_IGN or SIG_DFL, you can
  have this happen.  Changing signal attributes might also cause problems,
  although I am not so sure about that.

  (akpm: this test app segv'd on SMP within milliseconds for me)


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

char stack1[16384];
char stack2[16384];

void sighnd(int sig)
{
}

int child1(void *data)
{
	struct sigaction act;

	sigemptyset(&act.sa_mask);
	act.sa_flags = 0;
	for (;;) {
		act.sa_handler = sighnd;
		sigaction(45, &act, NULL);
		act.sa_handler = SIG_IGN;
		sigaction(45, &act, NULL);
	}
}

int child2(void *data)
{
	for (;;) {
		sleep(100);
	}
}

int main(int argc, char *argv[])
{
	int pid1, pid2;

	signal(45, SIG_IGN);
	pid2 = clone(child2, stack2 + sizeof(stack2) - 8,
			CLONE_SIGHAND | CLONE_VM, NULL);
	pid1 = clone(child1, stack1 + sizeof(stack2) - 8,
			CLONE_SIGHAND | CLONE_VM, NULL);

	for (;;) {
		kill(pid2, 45);
	}
}



---

 25-akpm/arch/i386/kernel/signal.c |   11 +++++------
 25-akpm/include/linux/signal.h    |    2 +-
 25-akpm/kernel/signal.c           |    8 ++++++--
 3 files changed, 12 insertions(+), 9 deletions(-)

diff -puN arch/i386/kernel/signal.c~signal-race-fix arch/i386/kernel/signal.c
--- 25/arch/i386/kernel/signal.c~signal-race-fix	2004-03-19 11:46:01.512324288 -0800
+++ 25-akpm/arch/i386/kernel/signal.c	2004-03-19 11:46:01.530321552 -0800
@@ -502,11 +502,9 @@ give_sigsegv:
  */	
 
 static void
-handle_signal(unsigned long sig, siginfo_t *info, sigset_t *oldset,
-	struct pt_regs * regs)
+handle_signal(unsigned long sig, siginfo_t *info, struct k_sigaction *ka,
+	      sigset_t *oldset,	struct pt_regs * regs)
 {
-	struct k_sigaction *ka = &current->sighand->action[sig-1];
-
 	/* Are we from a system call? */
 	if (regs->orig_eax >= 0) {
 		/* If so, check system call restarting.. */
@@ -555,6 +553,7 @@ int fastcall do_signal(struct pt_regs *r
 {
 	siginfo_t info;
 	int signr;
+	struct k_sigaction ka;
 
 	/*
 	 * We want the common case to go fast, which
@@ -573,7 +572,7 @@ int fastcall do_signal(struct pt_regs *r
 	if (!oldset)
 		oldset = &current->blocked;
 
-	signr = get_signal_to_deliver(&info, regs, NULL);
+	signr = get_signal_to_deliver(&info, &ka, regs, NULL);
 	if (signr > 0) {
 		/* Reenable any watchpoints before delivering the
 		 * signal to user space. The processor register will
@@ -583,7 +582,7 @@ int fastcall do_signal(struct pt_regs *r
 		__asm__("movl %0,%%db7"	: : "r" (current->thread.debugreg[7]));
 
 		/* Whee!  Actually deliver the signal.  */
-		handle_signal(signr, &info, oldset, regs);
+		handle_signal(signr, &info, &ka, oldset, regs);
 		return 1;
 	}
 
diff -puN include/linux/signal.h~signal-race-fix include/linux/signal.h
--- 25/include/linux/signal.h~signal-race-fix	2004-03-19 11:46:01.513324136 -0800
+++ 25-akpm/include/linux/signal.h	2004-03-19 11:46:01.518323376 -0800
@@ -213,7 +213,7 @@ extern int sigprocmask(int, sigset_t *, 
 
 #ifndef HAVE_ARCH_GET_SIGNAL_TO_DELIVER
 struct pt_regs;
-extern int get_signal_to_deliver(siginfo_t *info, struct pt_regs *regs, void *cookie);
+extern int get_signal_to_deliver(siginfo_t *info, struct k_sigaction *return_ka, struct pt_regs *regs, void *cookie);
 #endif
 
 #endif /* __KERNEL__ */
diff -puN kernel/signal.c~signal-race-fix kernel/signal.c
--- 25/kernel/signal.c~signal-race-fix	2004-03-19 11:46:01.515323832 -0800
+++ 25-akpm/kernel/signal.c	2004-03-19 11:46:01.529321704 -0800
@@ -1699,7 +1699,8 @@ static inline int handle_group_stop(void
 	return 1;
 }
 
-int get_signal_to_deliver(siginfo_t *info, struct pt_regs *regs, void *cookie)
+int get_signal_to_deliver(siginfo_t *info, struct k_sigaction *return_ka,
+			  struct pt_regs *regs, void *cookie)
 {
 	sigset_t *mask = &current->blocked;
 	int signr = 0;
@@ -1768,8 +1769,11 @@ relock:
 		ka = &current->sighand->action[signr-1];
 		if (ka->sa.sa_handler == SIG_IGN) /* Do nothing.  */
 			continue;
-		if (ka->sa.sa_handler != SIG_DFL) /* Run the handler.  */
+		if (ka->sa.sa_handler != SIG_DFL) {
+			/* Run the handler.  */
+			*return_ka = *ka;
 			break; /* will return non-zero "signr" value */
+		}
 
 		/*
 		 * Now we are doing the default action for this signal.

_

Re: signal-race-fix.patch

[David S. Miller]

On Fri, 19 Mar 2004 12:01:51 -0800
Andrew Morton <akpm@osdl.org> wrote:

> We have an SMP race in the signal code.  A fix for x86 is below.  All archs
> need updating.

I think the fix may need fixing :-)

Now that we're passing a stack local k_sigaction into handle_signal()
the real sigaction is not being updated f.e. when SA_ONESHOT causes
ka->sa.sa_handler to be set to SIG_DFL.  Only the stack local copy
is going to be set like this, not the one in the signals struct which
is where it is needed.

I noticed this while coding up the sparc versions which I'll defer until
this is cleared up.

Re: signal-race-fix.patch

[Corey Minyard]

David S. Miller wrote:

>On Fri, 19 Mar 2004 12:01:51 -0800
>Andrew Morton <akpm@osdl.org> wrote:
>
>  
>
>>We have an SMP race in the signal code.  A fix for x86 is below.  All archs
>>need updating.
>>    
>>
>
>I think the fix may need fixing :-)
>
>Now that we're passing a stack local k_sigaction into handle_signal()
>the real sigaction is not being updated f.e. when SA_ONESHOT causes
>ka->sa.sa_handler to be set to SIG_DFL.  Only the stack local copy
>is going to be set like this, not the one in the signals struct which
>is where it is needed.
>
>I noticed this while coding up the sparc versions which I'll defer until
>this is cleared up.
>  
>
Yes, I see that now.  Is there some reason that get_signal_to_deliver() 
does not do modify ka->sa.sa_handler?

-Corey

Re: signal-race-fix.patch

[Corey Minyard]

[-- Attachment #1: Type: text/plain, Size: 717 bytes --]

David S. Miller wrote:

>On Fri, 19 Mar 2004 12:01:51 -0800
>Andrew Morton <akpm@osdl.org> wrote:
>
>  
>
>>We have an SMP race in the signal code.  A fix for x86 is below.  All archs
>>need updating.
>>    
>>
>
>I think the fix may need fixing :-)
>
>Now that we're passing a stack local k_sigaction into handle_signal()
>the real sigaction is not being updated f.e. when SA_ONESHOT causes
>ka->sa.sa_handler to be set to SIG_DFL.  Only the stack local copy
>is going to be set like this, not the one in the signals struct which
>is where it is needed.
>
>I noticed this while coding up the sparc versions which I'll defer until
>this is cleared up.
>  
>
BTW, here is a new patch that covers that problem.

-Corey

[-- Attachment #2: sigrace-fix2-2.6.diff --]
[-- Type: text/plain, Size: 2942 bytes --]

--- linux.orig/include/linux/signal.h	2004-02-19 19:28:23.000000000 -0600
+++ linux/include/linux/signal.h	2004-03-19 08:07:45.000000000 -0600
@@ -213,7 +213,7 @@
 
 #ifndef HAVE_ARCH_GET_SIGNAL_TO_DELIVER
 struct pt_regs;
-extern int get_signal_to_deliver(siginfo_t *info, struct pt_regs *regs, void *cookie);
+extern int get_signal_to_deliver(siginfo_t *info, struct k_sigaction *return_ka, struct pt_regs *regs, void *cookie);
 #endif
 
 #endif /* __KERNEL__ */
--- linux.orig/kernel/signal.c	2004-03-16 17:20:06.000000000 -0600
+++ linux/kernel/signal.c	2004-03-19 16:39:57.000000000 -0600
@@ -1692,7 +1692,8 @@
 	return 1;
 }
 
-int get_signal_to_deliver(siginfo_t *info, struct pt_regs *regs, void *cookie)
+int get_signal_to_deliver(siginfo_t *info, struct k_sigaction *return_ka,
+			  struct pt_regs *regs, void *cookie)
 {
 	sigset_t *mask = &current->blocked;
 	int signr = 0;
@@ -1761,8 +1762,15 @@
 		ka = &current->sighand->action[signr-1];
 		if (ka->sa.sa_handler == SIG_IGN) /* Do nothing.  */
 			continue;
-		if (ka->sa.sa_handler != SIG_DFL) /* Run the handler.  */
+		if (ka->sa.sa_handler != SIG_DFL) {
+			/* Run the handler.  */
+			*return_ka = *ka;
+
+			if (ka->sa.sa_flags & SA_ONESHOT)
+				ka->sa.sa_handler = SIG_DFL;
+
 			break; /* will return non-zero "signr" value */
+		}
 
 		/*
 		 * Now we are doing the default action for this signal.
--- linux.orig/arch/i386/kernel/signal.c	2004-03-16 17:19:41.000000000 -0600
+++ linux/arch/i386/kernel/signal.c	2004-03-19 16:38:50.000000000 -0600
@@ -502,11 +502,9 @@
  */	
 
 static void
-handle_signal(unsigned long sig, siginfo_t *info, sigset_t *oldset,
-	struct pt_regs * regs)
+handle_signal(unsigned long sig, siginfo_t *info, struct k_sigaction *ka,
+	      sigset_t *oldset,	struct pt_regs * regs)
 {
-	struct k_sigaction *ka = &current->sighand->action[sig-1];
-
 	/* Are we from a system call? */
 	if (regs->orig_eax >= 0) {
 		/* If so, check system call restarting.. */
@@ -534,9 +532,6 @@
 	else
 		setup_frame(sig, ka, oldset, regs);
 
-	if (ka->sa.sa_flags & SA_ONESHOT)
-		ka->sa.sa_handler = SIG_DFL;
-
 	if (!(ka->sa.sa_flags & SA_NODEFER)) {
 		spin_lock_irq(&current->sighand->siglock);
 		sigorsets(&current->blocked,&current->blocked,&ka->sa.sa_mask);
@@ -555,6 +550,7 @@
 {
 	siginfo_t info;
 	int signr;
+	struct k_sigaction ka;
 
 	/*
 	 * We want the common case to go fast, which
@@ -573,7 +569,7 @@
 	if (!oldset)
 		oldset = &current->blocked;
 
-	signr = get_signal_to_deliver(&info, regs, NULL);
+	signr = get_signal_to_deliver(&info, &ka, regs, NULL);
 	if (signr > 0) {
 		/* Reenable any watchpoints before delivering the
 		 * signal to user space. The processor register will
@@ -583,7 +579,7 @@
 		__asm__("movl %0,%%db7"	: : "r" (current->thread.debugreg[7]));
 
 		/* Whee!  Actually deliver the signal.  */
-		handle_signal(signr, &info, oldset, regs);
+		handle_signal(signr, &info, &ka, oldset, regs);
 		return 1;
 	}
 

Re: signal-race-fix.patch

[David S. Miller]

On Fri, 19 Mar 2004 16:42:03 -0600
Corey Minyard <minyard@acm.org> wrote:

> BTW, here is a new patch that covers that problem.

Ok, I'll cook up the sparc bits.

Although I can just see someone updating something in the 'ka' and
expecting it to take effect, then spending a serious amount of time
debugging only to find it's a pointer to a stack local copy of the
info.

:-)

Re: signal-race-fix.patch

[Corey Minyard]

David S. Miller wrote:

>On Fri, 19 Mar 2004 16:42:03 -0600
>Corey Minyard <minyard@acm.org> wrote:
>
>  
>
>>BTW, here is a new patch that covers that problem.
>>    
>>
>
>Ok, I'll cook up the sparc bits.
>
>Although I can just see someone updating something in the 'ka' and
>expecting it to take effect, then spending a serious amount of time
>debugging only to find it's a pointer to a stack local copy of the
>info.
>
>:-)
>  
>
True.  I should probably rename the variable to ka_copy, or something 
like that to avoid somebody using it.  It actually happens in another 
place in the code, too, on x86.  There is a situation in setup_frame() 
and setup_rt_frame() where if it cannot write the signal frame to the 
stack, it sets the handler to SIG_DFL if it is a SEGV.

I'm not sure what to do about that one yet.

-Corey

Re: signal-race-fix.patch

[David S. Miller]

On Fri, 19 Mar 2004 16:42:03 -0600
Corey Minyard <minyard@acm.org> wrote:

> BTW, here is a new patch that covers that problem.

This still doesn't cut it, next the code in handle_signal() performs operations
on ka->sa.sa_mask if SA_{NOMASK,NODEFER} is not set,
then a recalc_sigpending() is made.

I think we really need to pull the locking up a level or something like that.

Re: signal-race-fix.patch

[Corey Minyard]

[-- Attachment #1: Type: text/plain, Size: 691 bytes --]

David S. Miller wrote:

>On Fri, 19 Mar 2004 16:42:03 -0600
>Corey Minyard <minyard@acm.org> wrote:
>
>  
>
>>BTW, here is a new patch that covers that problem.
>>    
>>
>
>This still doesn't cut it, next the code in handle_signal() performs operations
>on ka->sa.sa_mask if SA_{NOMASK,NODEFER} is not set,
>then a recalc_sigpending() is made.
>
>I think we really need to pull the locking up a level or something like that.
>  
>
I saw it using the sa_sigmask, but not modifying it.

But you still may be right, pulling up the lock a level may be more 
optimal.  It just may mess with latency.

Here's another patch with the SEGV problems handled and the ka's renamed 
to ka_copy.

-Corey

[-- Attachment #2: sigrace-fix3-2.6.diff --]
[-- Type: text/plain, Size: 7108 bytes --]

--- linux.orig/include/linux/signal.h	2004-02-19 19:28:23.000000000 -0600
+++ linux/include/linux/signal.h	2004-03-19 08:07:45.000000000 -0600
@@ -213,7 +213,7 @@
 
 #ifndef HAVE_ARCH_GET_SIGNAL_TO_DELIVER
 struct pt_regs;
-extern int get_signal_to_deliver(siginfo_t *info, struct pt_regs *regs, void *cookie);
+extern int get_signal_to_deliver(siginfo_t *info, struct k_sigaction *return_ka, struct pt_regs *regs, void *cookie);
 #endif
 
 #endif /* __KERNEL__ */
--- linux.orig/kernel/signal.c	2004-03-16 17:20:06.000000000 -0600
+++ linux/kernel/signal.c	2004-03-19 16:39:57.000000000 -0600
@@ -1692,7 +1692,8 @@
 	return 1;
 }
 
-int get_signal_to_deliver(siginfo_t *info, struct pt_regs *regs, void *cookie)
+int get_signal_to_deliver(siginfo_t *info, struct k_sigaction *return_ka,
+			  struct pt_regs *regs, void *cookie)
 {
 	sigset_t *mask = &current->blocked;
 	int signr = 0;
@@ -1761,8 +1762,15 @@
 		ka = &current->sighand->action[signr-1];
 		if (ka->sa.sa_handler == SIG_IGN) /* Do nothing.  */
 			continue;
-		if (ka->sa.sa_handler != SIG_DFL) /* Run the handler.  */
+		if (ka->sa.sa_handler != SIG_DFL) {
+			/* Run the handler.  */
+			*return_ka = *ka;
+
+			if (ka->sa.sa_flags & SA_ONESHOT)
+				ka->sa.sa_handler = SIG_DFL;
+
 			break; /* will return non-zero "signr" value */
+		}
 
 		/*
 		 * Now we are doing the default action for this signal.
--- linux.orig/arch/i386/kernel/signal.c	2004-03-16 17:19:41.000000000 -0600
+++ linux/arch/i386/kernel/signal.c	2004-03-19 18:04:43.000000000 -0600
@@ -312,7 +312,7 @@
  * Determine which stack to use..
  */
 static inline void __user *
-get_sigframe(struct k_sigaction *ka, struct pt_regs * regs, size_t frame_size)
+get_sigframe(struct k_sigaction *ka_copy, struct pt_regs * regs, size_t frame_size)
 {
 	unsigned long esp;
 
@@ -320,16 +320,16 @@
 	esp = regs->esp;
 
 	/* This is the X/Open sanctioned signal stack switching.  */
-	if (ka->sa.sa_flags & SA_ONSTACK) {
+	if (ka_copy->sa.sa_flags & SA_ONSTACK) {
 		if (sas_ss_flags(esp) == 0)
 			esp = current->sas_ss_sp + current->sas_ss_size;
 	}
 
 	/* This is the legacy signal stack switching. */
 	else if ((regs->xss & 0xffff) != __USER_DS &&
-		 !(ka->sa.sa_flags & SA_RESTORER) &&
-		 ka->sa.sa_restorer) {
-		esp = (unsigned long) ka->sa.sa_restorer;
+		 !(ka_copy->sa.sa_flags & SA_RESTORER) &&
+		 ka_copy->sa.sa_restorer) {
+		esp = (unsigned long) ka_copy->sa.sa_restorer;
 	}
 
 	return (void __user *)((esp - frame_size) & -8ul);
@@ -339,14 +339,14 @@
    See vsyscall-sigreturn.S.  */
 extern void __kernel_sigreturn, __kernel_rt_sigreturn;
 
-static void setup_frame(int sig, struct k_sigaction *ka,
+static void setup_frame(int sig, struct k_sigaction *ka_copy,
 			sigset_t *set, struct pt_regs * regs)
 {
 	void *restorer;
 	struct sigframe __user *frame;
 	int err = 0;
 
-	frame = get_sigframe(ka, regs, sizeof(*frame));
+	frame = get_sigframe(ka_copy, regs, sizeof(*frame));
 
 	if (!access_ok(VERIFY_WRITE, frame, sizeof(*frame)))
 		goto give_sigsegv;
@@ -372,8 +372,8 @@
 		goto give_sigsegv;
 
 	restorer = &__kernel_sigreturn;
-	if (ka->sa.sa_flags & SA_RESTORER)
-		restorer = ka->sa.sa_restorer;
+	if (ka_copy->sa.sa_flags & SA_RESTORER)
+		restorer = ka_copy->sa.sa_restorer;
 
 	/* Set up to return from userspace.  */
 	err |= __put_user(restorer, &frame->pretcode);
@@ -394,7 +394,7 @@
 
 	/* Set up registers for signal handler */
 	regs->esp = (unsigned long) frame;
-	regs->eip = (unsigned long) ka->sa.sa_handler;
+	regs->eip = (unsigned long) ka_copy->sa.sa_handler;
 
 	set_fs(USER_DS);
 	regs->xds = __USER_DS;
@@ -412,18 +412,18 @@
 
 give_sigsegv:
 	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
+		current->sighand->action[sig-1].sa.sa_handler = SIG_DFL;
 	force_sig(SIGSEGV, current);
 }
 
-static void setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
+static void setup_rt_frame(int sig, struct k_sigaction *ka_copy, siginfo_t *info,
 			   sigset_t *set, struct pt_regs * regs)
 {
 	void *restorer;
 	struct rt_sigframe __user *frame;
 	int err = 0;
 
-	frame = get_sigframe(ka, regs, sizeof(*frame));
+	frame = get_sigframe(ka_copy, regs, sizeof(*frame));
 
 	if (!access_ok(VERIFY_WRITE, frame, sizeof(*frame)))
 		goto give_sigsegv;
@@ -455,8 +455,8 @@
 
 	/* Set up to return from userspace.  */
 	restorer = &__kernel_rt_sigreturn;
-	if (ka->sa.sa_flags & SA_RESTORER)
-		restorer = ka->sa.sa_restorer;
+	if (ka_copy->sa.sa_flags & SA_RESTORER)
+		restorer = ka_copy->sa.sa_restorer;
 	err |= __put_user(restorer, &frame->pretcode);
 	 
 	/*
@@ -475,7 +475,7 @@
 
 	/* Set up registers for signal handler */
 	regs->esp = (unsigned long) frame;
-	regs->eip = (unsigned long) ka->sa.sa_handler;
+	regs->eip = (unsigned long) ka_copy->sa.sa_handler;
 
 	set_fs(USER_DS);
 	regs->xds = __USER_DS;
@@ -493,7 +493,7 @@
 
 give_sigsegv:
 	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
+		current->sighand->action[sig-1].sa.sa_handler = SIG_DFL;
 	force_sig(SIGSEGV, current);
 }
 
@@ -502,11 +502,9 @@
  */	
 
 static void
-handle_signal(unsigned long sig, siginfo_t *info, sigset_t *oldset,
-	struct pt_regs * regs)
+handle_signal(unsigned long sig, siginfo_t *info, struct k_sigaction *ka_copy,
+	      sigset_t *oldset,	struct pt_regs * regs)
 {
-	struct k_sigaction *ka = &current->sighand->action[sig-1];
-
 	/* Are we from a system call? */
 	if (regs->orig_eax >= 0) {
 		/* If so, check system call restarting.. */
@@ -517,7 +515,7 @@
 				break;
 
 			case -ERESTARTSYS:
-				if (!(ka->sa.sa_flags & SA_RESTART)) {
+				if (!(ka_copy->sa.sa_flags & SA_RESTART)) {
 					regs->eax = -EINTR;
 					break;
 				}
@@ -529,17 +527,14 @@
 	}
 
 	/* Set up the stack frame */
-	if (ka->sa.sa_flags & SA_SIGINFO)
-		setup_rt_frame(sig, ka, info, oldset, regs);
+	if (ka_copy->sa.sa_flags & SA_SIGINFO)
+		setup_rt_frame(sig, ka_copy, info, oldset, regs);
 	else
-		setup_frame(sig, ka, oldset, regs);
-
-	if (ka->sa.sa_flags & SA_ONESHOT)
-		ka->sa.sa_handler = SIG_DFL;
+		setup_frame(sig, ka_copy, oldset, regs);
 
-	if (!(ka->sa.sa_flags & SA_NODEFER)) {
+	if (!(ka_copy->sa.sa_flags & SA_NODEFER)) {
 		spin_lock_irq(&current->sighand->siglock);
-		sigorsets(&current->blocked,&current->blocked,&ka->sa.sa_mask);
+		sigorsets(&current->blocked,&current->blocked,&ka_copy->sa.sa_mask);
 		sigaddset(&current->blocked,sig);
 		recalc_sigpending();
 		spin_unlock_irq(&current->sighand->siglock);
@@ -555,6 +550,7 @@
 {
 	siginfo_t info;
 	int signr;
+	struct k_sigaction ka_copy;
 
 	/*
 	 * We want the common case to go fast, which
@@ -573,7 +569,7 @@
 	if (!oldset)
 		oldset = &current->blocked;
 
-	signr = get_signal_to_deliver(&info, regs, NULL);
+	signr = get_signal_to_deliver(&info, &ka_copy, regs, NULL);
 	if (signr > 0) {
 		/* Reenable any watchpoints before delivering the
 		 * signal to user space. The processor register will
@@ -583,7 +579,7 @@
 		__asm__("movl %0,%%db7"	: : "r" (current->thread.debugreg[7]));
 
 		/* Whee!  Actually deliver the signal.  */
-		handle_signal(signr, &info, oldset, regs);
+		handle_signal(signr, &info, &ka_copy, oldset, regs);
 		return 1;
 	}
 

Re: signal-race-fix.patch

[Andrew Morton]

Corey Minyard <minyard@acm.org> wrote:
>
> Here's another patch with the SEGV problems handled and the ka's renamed 
>  to ka_copy.

Are there any remaining concerns with Corey's final patch?


 25-akpm/arch/i386/kernel/signal.c |   60 +++++++++++++++++---------------------
 25-akpm/include/linux/signal.h    |    2 -
 25-akpm/kernel/signal.c           |   12 ++++++-
 3 files changed, 39 insertions(+), 35 deletions(-)

diff -puN arch/i386/kernel/signal.c~signal-race-fix arch/i386/kernel/signal.c
--- 25/arch/i386/kernel/signal.c~signal-race-fix	2004-03-23 02:18:54.810193056 -0800
+++ 25-akpm/arch/i386/kernel/signal.c	2004-03-23 02:18:54.817191992 -0800
@@ -312,7 +312,7 @@ setup_sigcontext(struct sigcontext __use
  * Determine which stack to use..
  */
 static inline void __user *
-get_sigframe(struct k_sigaction *ka, struct pt_regs * regs, size_t frame_size)
+get_sigframe(struct k_sigaction *ka_copy, struct pt_regs * regs, size_t frame_size)
 {
 	unsigned long esp;
 
@@ -320,16 +320,16 @@ get_sigframe(struct k_sigaction *ka, str
 	esp = regs->esp;
 
 	/* This is the X/Open sanctioned signal stack switching.  */
-	if (ka->sa.sa_flags & SA_ONSTACK) {
+	if (ka_copy->sa.sa_flags & SA_ONSTACK) {
 		if (sas_ss_flags(esp) == 0)
 			esp = current->sas_ss_sp + current->sas_ss_size;
 	}
 
 	/* This is the legacy signal stack switching. */
 	else if ((regs->xss & 0xffff) != __USER_DS &&
-		 !(ka->sa.sa_flags & SA_RESTORER) &&
-		 ka->sa.sa_restorer) {
-		esp = (unsigned long) ka->sa.sa_restorer;
+		 !(ka_copy->sa.sa_flags & SA_RESTORER) &&
+		 ka_copy->sa.sa_restorer) {
+		esp = (unsigned long) ka_copy->sa.sa_restorer;
 	}
 
 	return (void __user *)((esp - frame_size) & -8ul);
@@ -339,14 +339,14 @@ get_sigframe(struct k_sigaction *ka, str
    See vsyscall-sigreturn.S.  */
 extern void __kernel_sigreturn, __kernel_rt_sigreturn;
 
-static void setup_frame(int sig, struct k_sigaction *ka,
+static void setup_frame(int sig, struct k_sigaction *ka_copy,
 			sigset_t *set, struct pt_regs * regs)
 {
 	void *restorer;
 	struct sigframe __user *frame;
 	int err = 0;
 
-	frame = get_sigframe(ka, regs, sizeof(*frame));
+	frame = get_sigframe(ka_copy, regs, sizeof(*frame));
 
 	if (!access_ok(VERIFY_WRITE, frame, sizeof(*frame)))
 		goto give_sigsegv;
@@ -372,8 +372,8 @@ static void setup_frame(int sig, struct 
 		goto give_sigsegv;
 
 	restorer = &__kernel_sigreturn;
-	if (ka->sa.sa_flags & SA_RESTORER)
-		restorer = ka->sa.sa_restorer;
+	if (ka_copy->sa.sa_flags & SA_RESTORER)
+		restorer = ka_copy->sa.sa_restorer;
 
 	/* Set up to return from userspace.  */
 	err |= __put_user(restorer, &frame->pretcode);
@@ -394,7 +394,7 @@ static void setup_frame(int sig, struct 
 
 	/* Set up registers for signal handler */
 	regs->esp = (unsigned long) frame;
-	regs->eip = (unsigned long) ka->sa.sa_handler;
+	regs->eip = (unsigned long) ka_copy->sa.sa_handler;
 
 	set_fs(USER_DS);
 	regs->xds = __USER_DS;
@@ -412,18 +412,18 @@ static void setup_frame(int sig, struct 
 
 give_sigsegv:
 	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
+		current->sighand->action[sig-1].sa.sa_handler = SIG_DFL;
 	force_sig(SIGSEGV, current);
 }
 
-static void setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
+static void setup_rt_frame(int sig, struct k_sigaction *ka_copy, siginfo_t *info,
 			   sigset_t *set, struct pt_regs * regs)
 {
 	void *restorer;
 	struct rt_sigframe __user *frame;
 	int err = 0;
 
-	frame = get_sigframe(ka, regs, sizeof(*frame));
+	frame = get_sigframe(ka_copy, regs, sizeof(*frame));
 
 	if (!access_ok(VERIFY_WRITE, frame, sizeof(*frame)))
 		goto give_sigsegv;
@@ -455,8 +455,8 @@ static void setup_rt_frame(int sig, stru
 
 	/* Set up to return from userspace.  */
 	restorer = &__kernel_rt_sigreturn;
-	if (ka->sa.sa_flags & SA_RESTORER)
-		restorer = ka->sa.sa_restorer;
+	if (ka_copy->sa.sa_flags & SA_RESTORER)
+		restorer = ka_copy->sa.sa_restorer;
 	err |= __put_user(restorer, &frame->pretcode);
 	 
 	/*
@@ -475,7 +475,7 @@ static void setup_rt_frame(int sig, stru
 
 	/* Set up registers for signal handler */
 	regs->esp = (unsigned long) frame;
-	regs->eip = (unsigned long) ka->sa.sa_handler;
+	regs->eip = (unsigned long) ka_copy->sa.sa_handler;
 
 	set_fs(USER_DS);
 	regs->xds = __USER_DS;
@@ -493,7 +493,7 @@ static void setup_rt_frame(int sig, stru
 
 give_sigsegv:
 	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
+		current->sighand->action[sig-1].sa.sa_handler = SIG_DFL;
 	force_sig(SIGSEGV, current);
 }
 
@@ -502,11 +502,9 @@ give_sigsegv:
  */	
 
 static void
-handle_signal(unsigned long sig, siginfo_t *info, sigset_t *oldset,
-	struct pt_regs * regs)
+handle_signal(unsigned long sig, siginfo_t *info, struct k_sigaction *ka_copy,
+	      sigset_t *oldset,	struct pt_regs * regs)
 {
-	struct k_sigaction *ka = &current->sighand->action[sig-1];
-
 	/* Are we from a system call? */
 	if (regs->orig_eax >= 0) {
 		/* If so, check system call restarting.. */
@@ -517,7 +515,7 @@ handle_signal(unsigned long sig, siginfo
 				break;
 
 			case -ERESTARTSYS:
-				if (!(ka->sa.sa_flags & SA_RESTART)) {
+				if (!(ka_copy->sa.sa_flags & SA_RESTART)) {
 					regs->eax = -EINTR;
 					break;
 				}
@@ -529,17 +527,14 @@ handle_signal(unsigned long sig, siginfo
 	}
 
 	/* Set up the stack frame */
-	if (ka->sa.sa_flags & SA_SIGINFO)
-		setup_rt_frame(sig, ka, info, oldset, regs);
+	if (ka_copy->sa.sa_flags & SA_SIGINFO)
+		setup_rt_frame(sig, ka_copy, info, oldset, regs);
 	else
-		setup_frame(sig, ka, oldset, regs);
-
-	if (ka->sa.sa_flags & SA_ONESHOT)
-		ka->sa.sa_handler = SIG_DFL;
+		setup_frame(sig, ka_copy, oldset, regs);
 
-	if (!(ka->sa.sa_flags & SA_NODEFER)) {
+	if (!(ka_copy->sa.sa_flags & SA_NODEFER)) {
 		spin_lock_irq(&current->sighand->siglock);
-		sigorsets(&current->blocked,&current->blocked,&ka->sa.sa_mask);
+		sigorsets(&current->blocked,&current->blocked,&ka_copy->sa.sa_mask);
 		sigaddset(&current->blocked,sig);
 		recalc_sigpending();
 		spin_unlock_irq(&current->sighand->siglock);
@@ -555,6 +550,7 @@ int fastcall do_signal(struct pt_regs *r
 {
 	siginfo_t info;
 	int signr;
+	struct k_sigaction ka_copy;
 
 	/*
 	 * We want the common case to go fast, which
@@ -573,7 +569,7 @@ int fastcall do_signal(struct pt_regs *r
 	if (!oldset)
 		oldset = &current->blocked;
 
-	signr = get_signal_to_deliver(&info, regs, NULL);
+	signr = get_signal_to_deliver(&info, &ka_copy, regs, NULL);
 	if (signr > 0) {
 		/* Reenable any watchpoints before delivering the
 		 * signal to user space. The processor register will
@@ -583,7 +579,7 @@ int fastcall do_signal(struct pt_regs *r
 		__asm__("movl %0,%%db7"	: : "r" (current->thread.debugreg[7]));
 
 		/* Whee!  Actually deliver the signal.  */
-		handle_signal(signr, &info, oldset, regs);
+		handle_signal(signr, &info, &ka_copy, oldset, regs);
 		return 1;
 	}
 
diff -puN include/linux/signal.h~signal-race-fix include/linux/signal.h
--- 25/include/linux/signal.h~signal-race-fix	2004-03-23 02:18:54.811192904 -0800
+++ 25-akpm/include/linux/signal.h	2004-03-23 02:18:54.814192448 -0800
@@ -213,7 +213,7 @@ extern int sigprocmask(int, sigset_t *, 
 
 #ifndef HAVE_ARCH_GET_SIGNAL_TO_DELIVER
 struct pt_regs;
-extern int get_signal_to_deliver(siginfo_t *info, struct pt_regs *regs, void *cookie);
+extern int get_signal_to_deliver(siginfo_t *info, struct k_sigaction *return_ka, struct pt_regs *regs, void *cookie);
 #endif
 
 #endif /* __KERNEL__ */
diff -puN kernel/signal.c~signal-race-fix kernel/signal.c
--- 25/kernel/signal.c~signal-race-fix	2004-03-23 02:18:54.812192752 -0800
+++ 25-akpm/kernel/signal.c	2004-03-23 02:18:54.816192144 -0800
@@ -1699,7 +1699,8 @@ static inline int handle_group_stop(void
 	return 1;
 }
 
-int get_signal_to_deliver(siginfo_t *info, struct pt_regs *regs, void *cookie)
+int get_signal_to_deliver(siginfo_t *info, struct k_sigaction *return_ka,
+			  struct pt_regs *regs, void *cookie)
 {
 	sigset_t *mask = &current->blocked;
 	int signr = 0;
@@ -1768,8 +1769,15 @@ relock:
 		ka = &current->sighand->action[signr-1];
 		if (ka->sa.sa_handler == SIG_IGN) /* Do nothing.  */
 			continue;
-		if (ka->sa.sa_handler != SIG_DFL) /* Run the handler.  */
+		if (ka->sa.sa_handler != SIG_DFL) {
+			/* Run the handler.  */
+			*return_ka = *ka;
+
+			if (ka->sa.sa_flags & SA_ONESHOT)
+				ka->sa.sa_handler = SIG_DFL;
+
 			break; /* will return non-zero "signr" value */
+		}
 
 		/*
 		 * Now we are doing the default action for this signal.

_

Re: signal-race-fix.patch

[David S. Miller]

On Tue, 23 Mar 2004 02:20:20 -0800
Andrew Morton <akpm@osdl.org> wrote:

> Corey Minyard <minyard@acm.org> wrote:
> >
> > Here's another patch with the SEGV problems handled and the ka's renamed 
> >  to ka_copy.
> 
> Are there any remaining concerns with Corey's final patch?

I'm fine with it.

Re: signal-race-fix.patch

[Roland McGrath]

There is nothing wrong with this patch.  But while we are at it (and every
arch will be required to update anyway), I would favor moving the sa_mask
processing into common code in get_signal_to_deliver right next to where
Corey has moved the SA_ONESHOT handling.


Thanks,
Roland

Re: signal-race-fix.patch

[David S. Miller]

On Tue, 23 Mar 2004 11:35:09 -0800
Roland McGrath <roland@redhat.com> wrote:

> There is nothing wrong with this patch.  But while we are at it (and every
> arch will be required to update anyway), I would favor moving the sa_mask
> processing into common code in get_signal_to_deliver right next to where
> Corey has moved the SA_ONESHOT handling.

I agree, but we can do that with a follow on patch.  I think it's
more important to get this fix into Linus's tree soon than to combine
the two changes into one.

We've already spent enough time on what should have been a quick and
easy fix :-)

Re: signal-race-fix.patch

[David Mosberger]

>>>>> On Tue, 23 Mar 2004 02:20:20 -0800, Andrew Morton <akpm@osdl.org> said:

  Andrew> Are there any remaining concerns with Corey's final patch?

This looks rather dubious to me:

 	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
+		current->sighand->action[sig-1].sa.sa_handler = SIG_DFL;
 	force_sig(SIGSEGV, current);

Yeah, it only preseves the status quo (updating sa_handler without
holding the lock) and it's presumably only a single-word store which
is atomic, but it's not all that hard to imagine code that would break
if sa_handler were to change underneath someone holding the
sighand->siglock.

I suppose I can just acquire/release the lock in the ia64-specific
code but since other arches have the same issue, perhaps that's not
the right level for the fix.

	--david

Re: signal-race-fix.patch

[Roland McGrath]

> Yeah, it only preseves the status quo (updating sa_handler without
> holding the lock) and it's presumably only a single-word store which
> is atomic, but it's not all that hard to imagine code that would break
> if sa_handler were to change underneath someone holding the
> sighand->siglock.

I can't really imagine the code that would make anything untoward happen in
the situation where the process is being forced to exit with SIGSEGV anyway.

Re: signal-race-fix.patch

[David Mosberger]

>>>>> On Tue, 23 Mar 2004 19:58:38 -0800, Roland McGrath <roland@redhat.com> said:

  >> Yeah, it only preseves the status quo (updating sa_handler
  >> without holding the lock) and it's presumably only a single-word
  >> store which is atomic, but it's not all that hard to imagine code
  >> that would break if sa_handler were to change underneath someone
  >> holding the sighand-> siglock.

  Roland> I can't really imagine the code that would make anything
  Roland> untoward happen in the situation where the process is being
  Roland> forced to exit with SIGSEGV anyway.

I'm not sure I can either.  The current code in kernel/signal.c
certainly looks pretty safe in that regard.  Still, if this were to
bite, it would be a nightmare to debug.  Plus the resetting of
sa_handler to SIG_DFL only happens in the exceptional case of a bad
stack frame, so it's not exactly performance-critical.

	--david

Re: signal-race-fix.patch

[David Mosberger]

>>>>> On Tue, 23 Mar 2004 02:20:20 -0800, Andrew Morton <akpm@osdl.org> said:

  Andrew> Corey Minyard <minyard@acm.org> wrote:

  >> Here's another patch with the SEGV problems handled and the ka's renamed
  >> to ka_copy.

  Andrew> Are there any remaining concerns with Corey's final patch?

It looks fine to me, except that I decided to play chicken as far
as the give_sigsegv update of sa_handler is concerned.

Arun, I hope I got the ia32 emulation parts right, but you may want to
double-check.

The patch seems to work fine as far as I have tested.  I'm seeing some
oddity in context-switch overhead and pipe latency as reported by
LMbench, but I suspect that's due to another change that happened
somewhere between 2.6.5-rc1 and Linus' bk tree as of this morning.

Thanks,

	--david

diff -Nru a/arch/ia64/ia32/ia32_signal.c b/arch/ia64/ia32/ia32_signal.c
--- a/arch/ia64/ia32/ia32_signal.c	Wed Mar 24 13:48:15 2004
+++ b/arch/ia64/ia32/ia32_signal.c	Wed Mar 24 13:48:15 2004
@@ -1,7 +1,7 @@
 /*
  * IA32 Architecture-specific signal handling support.
  *
- * Copyright (C) 1999, 2001-2002 Hewlett-Packard Co
+ * Copyright (C) 1999, 2001-2002, 2004 Hewlett-Packard Co
  *	David Mosberger-Tang <davidm@hpl.hp.com>
  * Copyright (C) 1999 Arun Sharma <arun.sharma@intel.com>
  * Copyright (C) 2000 VA Linux Co
@@ -815,7 +815,7 @@
  * Determine which stack to use..
  */
 static inline void *
-get_sigframe (struct k_sigaction *ka, struct pt_regs * regs, size_t frame_size)
+get_sigframe (struct k_sigaction *ka_copy, struct pt_regs * regs, size_t frame_size)
 {
 	unsigned long esp;
 
@@ -823,7 +823,7 @@
 	esp = (unsigned int) regs->r12;
 
 	/* This is the X/Open sanctioned signal stack switching.  */
-	if (ka->sa.sa_flags & SA_ONSTACK) {
+	if (ka_copy->sa.sa_flags & SA_ONSTACK) {
 		if (!on_sig_stack(esp))
 			esp = current->sas_ss_sp + current->sas_ss_size;
 	}
@@ -832,17 +832,40 @@
 	return (void *)((esp - frame_size) & -8ul);
 }
 
+static long
+force_sigsegv (int sig)
+{
+	unsigned long flags;
+
+	if (sig == SIGSEGV) {
+		/*
+		 * Acquiring siglock around the sa_handler-update is almost
+		 * certainly overkill, but this isn't a
+		 * performance-critical path and I'd rather play it safe
+		 * here than having to debug a nasty race if and when
+		 * something changes in kernel/signal.c that would make it
+		 * no longer safe to modify sa_handler without holding the
+		 * lock.
+		 */
+		spin_lock_irqsave(&current->sighand->siglock, flags);
+		current->sighand->action[sig - 1].sa.sa_handler = SIG_DFL;
+		spin_unlock_irqrestore(&current->sighand->siglock, flags);
+	}
+	force_sig(SIGSEGV, current);
+	return 0;
+}
+
 static int
-setup_frame_ia32 (int sig, struct k_sigaction *ka, sigset_t *set, struct pt_regs * regs)
+setup_frame_ia32 (int sig, struct k_sigaction *ka_copy, sigset_t *set, struct pt_regs * regs)
 {
 	struct exec_domain *ed = current_thread_info()->exec_domain;
 	struct sigframe_ia32 *frame;
 	int err = 0;
 
-	frame = get_sigframe(ka, regs, sizeof(*frame));
+	frame = get_sigframe(ka_copy, regs, sizeof(*frame));
 
 	if (!access_ok(VERIFY_WRITE, frame, sizeof(*frame)))
-		goto give_sigsegv;
+		return force_sigsegv(sig);
 
 	err |= __put_user((ed && ed->signal_invmap && sig < 32
 			   ? (int)(ed->signal_invmap[sig]) : sig), &frame->sig);
@@ -855,8 +878,8 @@
 
 	/* Set up to return from userspace.  If provided, use a stub
 	   already in userspace.  */
-	if (ka->sa.sa_flags & SA_RESTORER) {
-		unsigned int restorer = IA32_SA_RESTORER(ka);
+	if (ka_copy->sa.sa_flags & SA_RESTORER) {
+		unsigned int restorer = IA32_SA_RESTORER(ka_copy);
 		err |= __put_user(restorer, &frame->pretcode);
 	} else {
 		err |= __put_user((long)frame->retcode, &frame->pretcode);
@@ -868,11 +891,11 @@
 	}
 
 	if (err)
-		goto give_sigsegv;
+		return force_sigsegv(sig);
 
 	/* Set up registers for signal handler */
 	regs->r12 = (unsigned long) frame;
-	regs->cr_iip = IA32_SA_HANDLER(ka);
+	regs->cr_iip = IA32_SA_HANDLER(ka_copy);
 
 	set_fs(USER_DS);
 
@@ -880,32 +903,26 @@
 	regs->eflags &= ~TF_MASK;
 #endif
 
-#if 0
+#if DEBUG_SIG
 	printk("SIG deliver (%s:%d): sig=%d sp=%p pc=%lx ra=%x\n",
                current->comm, current->pid, sig, (void *) frame, regs->cr_iip, frame->pretcode);
 #endif
 
 	return 1;
-
-  give_sigsegv:
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
-	return 0;
 }
 
 static int
-setup_rt_frame_ia32 (int sig, struct k_sigaction *ka, siginfo_t *info,
+setup_rt_frame_ia32 (int sig, struct k_sigaction *ka_copy, siginfo_t *info,
 		     sigset_t *set, struct pt_regs * regs)
 {
 	struct exec_domain *ed = current_thread_info()->exec_domain;
 	struct rt_sigframe_ia32 *frame;
 	int err = 0;
 
-	frame = get_sigframe(ka, regs, sizeof(*frame));
+	frame = get_sigframe(ka_copy, regs, sizeof(*frame));
 
 	if (!access_ok(VERIFY_WRITE, frame, sizeof(*frame)))
-		goto give_sigsegv;
+		return force_sigsegv(sig);
 
 	err |= __put_user((ed && ed->signal_invmap
 			   && sig < 32 ? ed->signal_invmap[sig] : sig), &frame->sig);
@@ -922,12 +939,12 @@
 	err |= setup_sigcontext_ia32(&frame->uc.uc_mcontext, &frame->fpstate, regs, set->sig[0]);
 	err |= __copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set));
 	if (err)
-		goto give_sigsegv;
+		return force_sigsegv(sig);
 
 	/* Set up to return from userspace.  If provided, use a stub
 	   already in userspace.  */
-	if (ka->sa.sa_flags & SA_RESTORER) {
-		unsigned int restorer = IA32_SA_RESTORER(ka);
+	if (ka_copy->sa.sa_flags & SA_RESTORER) {
+		unsigned int restorer = IA32_SA_RESTORER(ka_copy);
 		err |= __put_user(restorer, &frame->pretcode);
 	} else {
 		err |= __put_user((long)frame->retcode, &frame->pretcode);
@@ -938,11 +955,11 @@
 	}
 
 	if (err)
-		goto give_sigsegv;
+		return force_sigsegv(sig);
 
 	/* Set up registers for signal handler */
 	regs->r12 = (unsigned long) frame;
-	regs->cr_iip = IA32_SA_HANDLER(ka);
+	regs->cr_iip = IA32_SA_HANDLER(ka_copy);
 
 	set_fs(USER_DS);
 
@@ -950,29 +967,23 @@
 	regs->eflags &= ~TF_MASK;
 #endif
 
-#if 0
+#if DEBUG_SIG
 	printk("SIG deliver (%s:%d): sp=%p pc=%lx ra=%x\n",
                current->comm, current->pid, (void *) frame, regs->cr_iip, frame->pretcode);
 #endif
 
 	return 1;
-
-give_sigsegv:
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
-	return 0;
 }
 
 int
-ia32_setup_frame1 (int sig, struct k_sigaction *ka, siginfo_t *info,
+ia32_setup_frame1 (int sig, struct k_sigaction *ka_copy, siginfo_t *info,
 		   sigset_t *set, struct pt_regs *regs)
 {
        /* Set up the stack frame */
-       if (ka->sa.sa_flags & SA_SIGINFO)
-               return setup_rt_frame_ia32(sig, ka, info, set, regs);
+       if (ka_copy->sa.sa_flags & SA_SIGINFO)
+               return setup_rt_frame_ia32(sig, ka_copy, info, set, regs);
        else
-               return setup_frame_ia32(sig, ka, set, regs);
+               return setup_frame_ia32(sig, ka_copy, set, regs);
 }
 
 asmlinkage long
diff -Nru a/arch/ia64/kernel/signal.c b/arch/ia64/kernel/signal.c
--- a/arch/ia64/kernel/signal.c	Wed Mar 24 13:48:15 2004
+++ b/arch/ia64/kernel/signal.c	Wed Mar 24 13:48:15 2004
@@ -1,7 +1,7 @@
 /*
  * Architecture-specific signal handling support.
  *
- * Copyright (C) 1999-2003 Hewlett-Packard Co
+ * Copyright (C) 1999-2004 Hewlett-Packard Co
  *	David Mosberger-Tang <davidm@hpl.hp.com>
  *
  * Derived from i386 and Alpha versions.
@@ -397,18 +397,47 @@
 }
 
 static long
-setup_frame (int sig, struct k_sigaction *ka, siginfo_t *info, sigset_t *set,
+force_sigsegv (int sig, void *addr)
+{
+	unsigned long flags;
+	struct siginfo si;
+
+	if (sig == SIGSEGV) {
+		/*
+		 * Acquiring siglock around the sa_handler-update is almost
+		 * certainly overkill, but this isn't a
+		 * performance-critical path and I'd rather play it safe
+		 * here than having to debug a nasty race if and when
+		 * something changes in kernel/signal.c that would make it
+		 * no longer safe to modify sa_handler without holding the
+		 * lock.
+		 */
+		spin_lock_irqsave(&current->sighand->siglock, flags);
+		current->sighand->action[sig - 1].sa.sa_handler = SIG_DFL;
+		spin_unlock_irqrestore(&current->sighand->siglock, flags);
+	}
+	si.si_signo = SIGSEGV;
+	si.si_errno = 0;
+	si.si_code = SI_KERNEL;
+	si.si_pid = current->pid;
+	si.si_uid = current->uid;
+	si.si_addr = addr;
+	force_sig_info(SIGSEGV, &si, current);
+	return 0;
+}
+
+static long
+setup_frame (int sig, struct k_sigaction *ka_copy, siginfo_t *info, sigset_t *set,
 	     struct sigscratch *scr)
 {
 	extern char __kernel_sigtramp[];
 	unsigned long tramp_addr, new_rbs = 0;
 	struct sigframe *frame;
-	struct siginfo si;
 	long err;
 
 	frame = (void *) scr->pt.r12;
 	tramp_addr = (unsigned long) __kernel_sigtramp;
-	if ((ka->sa.sa_flags & SA_ONSTACK) && sas_ss_flags((unsigned long) frame) == 0) {
+	if ((ka_copy->sa.sa_flags & SA_ONSTACK) && sas_ss_flags((unsigned long) frame) == 0) {
 		frame = (void *) ((current->sas_ss_sp + current->sas_ss_size)
 				  & ~(STACK_ALIGN - 1));
 		/*
@@ -422,14 +451,14 @@
 	frame = (void *) frame - ((sizeof(*frame) + STACK_ALIGN - 1) & ~(STACK_ALIGN - 1));
 
 	if (!access_ok(VERIFY_WRITE, frame, sizeof(*frame)))
-		goto give_sigsegv;
+		return force_sigsegv(sig, frame);
 
 	err  = __put_user(sig, &frame->arg0);
 	err |= __put_user(&frame->info, &frame->arg1);
 	err |= __put_user(&frame->sc, &frame->arg2);
 	err |= __put_user(new_rbs, &frame->sc.sc_rbs_base);
 	err |= __put_user(0, &frame->sc.sc_loadrs);	/* initialize to zero */
-	err |= __put_user(ka->sa.sa_handler, &frame->handler);
+	err |= __put_user(ka_copy->sa.sa_handler, &frame->handler);
 
 	err |= copy_siginfo_to_user(&frame->info, info);
 
@@ -438,8 +467,8 @@
 	err |= __put_user(sas_ss_flags(scr->pt.r12), &frame->sc.sc_stack.ss_flags);
 	err |= setup_sigcontext(&frame->sc, set, scr);
 
-	if (err)
-		goto give_sigsegv;
+	if (unlikely(err))
+		return force_sigsegv(sig, frame);
 
 	scr->pt.r12 = (unsigned long) frame - 16;	/* new stack pointer */
 	scr->pt.ar_fpsr = FPSR_DEFAULT;			/* reset fpsr for signal handler */
@@ -466,40 +495,25 @@
 	       current->comm, current->pid, sig, scr->pt.r12, frame->sc.sc_ip, frame->handler);
 #endif
 	return 1;
-
-  give_sigsegv:
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	si.si_signo = SIGSEGV;
-	si.si_errno = 0;
-	si.si_code = SI_KERNEL;
-	si.si_pid = current->pid;
-	si.si_uid = current->uid;
-	si.si_addr = frame;
-	force_sig_info(SIGSEGV, &si, current);
-	return 0;
 }
 
 static long
-handle_signal (unsigned long sig, struct k_sigaction *ka, siginfo_t *info, sigset_t *oldset,
+handle_signal (unsigned long sig, struct k_sigaction *ka_copy, siginfo_t *info, sigset_t *oldset,
 	       struct sigscratch *scr)
 {
 	if (IS_IA32_PROCESS(&scr->pt)) {
 		/* send signal to IA-32 process */
-		if (!ia32_setup_frame1(sig, ka, info, oldset, &scr->pt))
+		if (!ia32_setup_frame1(sig, ka_copy, info, oldset, &scr->pt))
 			return 0;
 	} else
 		/* send signal to IA-64 process */
-		if (!setup_frame(sig, ka, info, oldset, scr))
+		if (!setup_frame(sig, ka_copy, info, oldset, scr))
 			return 0;
 
-	if (ka->sa.sa_flags & SA_ONESHOT)
-		ka->sa.sa_handler = SIG_DFL;
-
-	if (!(ka->sa.sa_flags & SA_NODEFER)) {
+	if (!(ka_copy->sa.sa_flags & SA_NODEFER)) {
 		spin_lock_irq(&current->sighand->siglock);
 		{
-			sigorsets(&current->blocked, &current->blocked, &ka->sa.sa_mask);
+			sigorsets(&current->blocked, &current->blocked, &ka_copy->sa.sa_mask);
 			sigaddset(&current->blocked, sig);
 			recalc_sigpending();
 		}
@@ -515,7 +529,7 @@
 long
 ia64_do_signal (sigset_t *oldset, struct sigscratch *scr, long in_syscall)
 {
-	struct k_sigaction *ka;
+	struct k_sigaction ka_copy;
 	siginfo_t info;
 	long restart = in_syscall;
 	long errno = scr->pt.r8;
@@ -537,7 +551,7 @@
 	 * need to push through a forced SIGSEGV.
 	 */
 	while (1) {
-		int signr = get_signal_to_deliver(&info, &scr->pt, NULL);
+		int signr = get_signal_to_deliver(&info, &ka_copy, &scr->pt, NULL);
 
 		/*
 		 * get_signal_to_deliver() may have run a debugger (via notify_parent())
@@ -564,8 +578,6 @@
 		if (signr <= 0)
 			break;
 
-		ka = &current->sighand->action[signr - 1];
-
 		if (unlikely(restart)) {
 			switch (errno) {
 			      case ERESTART_RESTARTBLOCK:
@@ -575,7 +587,7 @@
 				break;
 
 			      case ERESTARTSYS:
-				if ((ka->sa.sa_flags & SA_RESTART) == 0) {
+				if ((ka_copy.sa.sa_flags & SA_RESTART) == 0) {
 					scr->pt.r8 = ERR_CODE(EINTR);
 					/* note: scr->pt.r10 is already -1 */
 					break;
@@ -594,7 +606,7 @@
 		 * Whee!  Actually deliver the signal.  If the delivery failed, we need to
 		 * continue to iterate in this loop so we can deliver the SIGSEGV...
 		 */
-		if (handle_signal(signr, ka, &info, oldset, scr))
+		if (handle_signal(signr, &ka_copy, &info, oldset, scr))
 			return 1;
 	}
 

Re: signal-race-fix.patch

[Arun Sharma]

On 3/24/2004 1:53 PM, David Mosberger wrote:

> It looks fine to me, except that I decided to play chicken as far
> as the give_sigsegv update of sa_handler is concerned.
> 
> Arun, I hope I got the ia32 emulation parts right, but you may want to
> double-check.

Yes, the patch looks good to me and it survived a 32 bit version of Andrew's test program.

	-Arun

Re: signal-race-fix.patch

[Corey Minyard]

I haven't seen anything for this, this is not in the current Linus or mm 
releases.  Did I miss something?

Thanks,

-Corey

Andrew Morton wrote:

>Corey Minyard <minyard@acm.org> wrote:
>  
>
>>Here's another patch with the SEGV problems handled and the ka's renamed 
>> to ka_copy.
>>    
>>
>
>Are there any remaining concerns with Corey's final patch?
>
>
> 25-akpm/arch/i386/kernel/signal.c |   60 +++++++++++++++++---------------------
> 25-akpm/include/linux/signal.h    |    2 -
> 25-akpm/kernel/signal.c           |   12 ++++++-
> 3 files changed, 39 insertions(+), 35 deletions(-)
>  
>

Re: signal-race-fix.patch

[Andrew Morton]

Corey Minyard <minyard@acm.org> wrote:
>
> I haven't seen anything for this, this is not in the current Linus or mm 
> releases.  Did I miss something?

Andi had some legitimate gripes over it - the patch was a bit messy and
could have some performance impact.  So I kinda forgot about it in the hope
that a nicer solution would come along.


> Thanks,
> 
> -Corey
> 
> Andrew Morton wrote:
> 
> >Corey Minyard <minyard@acm.org> wrote:
> >  
> >
> >>Here's another patch with the SEGV problems handled and the ka's renamed 
> >> to ka_copy.
> >>    
> >>
> >
> >Are there any remaining concerns with Corey's final patch?
> >
> >
> > 25-akpm/arch/i386/kernel/signal.c |   60 +++++++++++++++++---------------------
> > 25-akpm/include/linux/signal.h    |    2 -
> > 25-akpm/kernel/signal.c           |   12 ++++++-
> > 3 files changed, 39 insertions(+), 35 deletions(-)
> >  
> >

Re: signal-race-fix.patch

[Corey Minyard]

Andrew Morton wrote:

>Corey Minyard <minyard@acm.org> wrote:
>  
>
>>I haven't seen anything for this, this is not in the current Linus or mm 
>>releases.  Did I miss something?
>>    
>>
>
>Andi had some legitimate gripes over it - the patch was a bit messy and
>could have some performance impact.  So I kinda forgot about it in the hope
>that a nicer solution would come along.
>  
>
Ok.  I searched and could not find Andi's email; do you have a pointer 
to it?  I'll fix the problems.

I looked at it, though, and the only possible performance hit I could 
find was that it copies the sa_handler data structure.  That's going to 
be hard to avoid.  And there was that big nasty line with too many "->" 
and "." setting sa_handler to SIG_DFL...

Thanks,

-Corey

Re: signal-race-fix.patch

[Andrew Morton]

Corey Minyard <minyard@acm.org> wrote:
>
> Andrew Morton wrote:
> 
>  >Corey Minyard <minyard@acm.org> wrote:
>  >  
>  >
>  >>I haven't seen anything for this, this is not in the current Linus or mm 
>  >>releases.  Did I miss something?
>  >>    
>  >>
>  >
>  >Andi had some legitimate gripes over it - the patch was a bit messy and
>  >could have some performance impact.  So I kinda forgot about it in the hope
>  >that a nicer solution would come along.
>  >  
>  >
>  Ok.  I searched and could not find Andi's email; do you have a pointer 
>  to it?

err, no.

I resurrected the patches - I'll send them out to remind people what they
do.  It affects all arches, and I only have x86, ia64, s390 and x86_64 in
hand.

It'll be a fair bit of work all round, so I'd ask people to take some time
out to think about this please.

Re: signal-race-fix.patch

[Roland McGrath]

> This still doesn't cut it, next the code in handle_signal() performs
> operations on ka->sa.sa_mask if SA_{NOMASK,NODEFER} is not set, then a
> recalc_sigpending() is made.

That just reads the sa_mask copy and that's all it needs to do.
That said, it would be cleaner if the sa_mask code were in a shared
function rather than being duplicated in every arch's handle_signal.

> I think we really need to pull the locking up a level or something like that.

As get_signal_to_deliver always takes the siglock on the way in, it
obviously wouldn't hurt any to instead have the lock taken by the caller.
But we do not want to hold that lock through the whole existing
handle_signal path, with page faults on the user stack and all.  So I'd say
the caller of get_signal_to_deliver ought to be doing the sigaction
diddling and releasing the lock before calling setup_{rt_,}frame.  If
that's what they should do, then might as well have common code do that,
e.g. by having get_signal_to_deliver handle sa_mask and SA_ONESHOT before
returning.