[patch 1/8] signal handling race fix

[patch 1/8] signal handling race fix

[akpm]

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);
	}
}


Signed-off-by: Andrew Morton <akpm@osdl.org>
---

 25-akpm/arch/i386/kernel/signal.c |   64 +++++++++++++++++---------------------
 25-akpm/include/linux/signal.h    |    2 -
 25-akpm/kernel/signal.c           |   12 +++++--
 3 files changed, 41 insertions(+), 37 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-08-25 14:26:46.505625576 -0700
+++ 25-akpm/arch/i386/kernel/signal.c	2004-08-25 14:26:46.513624360 -0700
@@ -311,7 +311,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;
 
@@ -319,16 +319,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
 extern void __user __kernel_sigreturn;
 extern void __user __kernel_rt_sigreturn;
 
-static void setup_frame(int sig, struct k_sigaction *ka,
-			sigset_t *set, struct pt_regs * regs)
+static void setup_frame(int sig, struct k_sigaction *ka_copy,
+			sigset_t *set, struct pt_regs *regs)
 {
 	void __user *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,
-			   sigset_t *set, struct pt_regs * regs)
+static void setup_rt_frame(int sig, struct k_sigaction *ka_copy,
+			siginfo_t *info, sigset_t *set, struct pt_regs *regs)
 {
 	void __user *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-08-25 14:26:46.507625272 -0700
+++ 25-akpm/include/linux/signal.h	2004-08-25 14:26:46.513624360 -0700
@@ -217,7 +217,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-08-25 14:26:46.508625120 -0700
+++ 25-akpm/kernel/signal.c	2004-08-25 14:26:46.515624056 -0700
@@ -1724,7 +1724,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;
@@ -1793,8 +1794,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: [patch 1/8] signal handling race fix

[Linus Torvalds]

On Wed, 25 Aug 2004 akpm@osdl.org wrote:
> 
> The patch:
> 
>   You have to get the sigaction information that will be delivered while
>   holding sighand->siglock in get_signal_to_deliver().

I'm ok with the patch, but I _hate_ the renaming of "ka" to "ka_copy". 
There's no real point except for the initial change (to make sure the 
compiler warns about any missed entries), and once that has been done it 
just makes the patches horribly noisy and not at all more readable.

As far as I can tell, the patch would be just a few lines long without 
that renaming. Can we _please_ reconsider?

Oh, and ia64 suddenly does the "force_sigsegv()" right, while at least the 
regular x86 port is not fixed. Shouldn't we just do the "force_sigsegv()" 
right for everybody, and put that one into kernel/signal.c?

What do you guys say?

		Linus

Re: [patch 1/8] signal handling race fix

[David S. Miller]

On Wed, 25 Aug 2004 15:40:52 -0700 (PDT)
Linus Torvalds <torvalds@osdl.org> wrote:

> I'm ok with the patch, but I _hate_ the renaming of "ka" to "ka_copy". 
> There's no real point except for the initial change (to make sure the 
> compiler warns about any missed entries), and once that has been done it 
> just makes the patches horribly noisy and not at all more readable.
> 
> As far as I can tell, the patch would be just a few lines long without 
> that renaming. Can we _please_ reconsider?

I agree.

> Oh, and ia64 suddenly does the "force_sigsegv()" right, while at least the 
> regular x86 port is not fixed. Shouldn't we just do the "force_sigsegv()" 
> right for everybody, and put that one into kernel/signal.c?

I think this should be a seperate change and not block this one.

Re: [patch 1/8] signal handling race fix

[Linus Torvalds]

On Wed, 25 Aug 2004, David S. Miller wrote:
> 
> I think this should be a seperate change and not block this one.

Agreed. This looks trivially obvious. No?

		Linus

-----
===== include/linux/sched.h 1.244 vs edited =====
--- 1.244/include/linux/sched.h	2004-08-24 02:08:42 -07:00
+++ edited/include/linux/sched.h	2004-08-25 17:00:19 -07:00
@@ -718,6 +718,7 @@
 extern void release_task(struct task_struct * p);
 extern int send_sig_info(int, struct siginfo *, struct task_struct *);
 extern int send_group_sig_info(int, struct siginfo *, struct task_struct *);
+extern int force_sigsegv(int, struct task_struct *);
 extern int force_sig_info(int, struct siginfo *, struct task_struct *);
 extern int __kill_pg_info(int sig, struct siginfo *info, pid_t pgrp);
 extern int kill_pg_info(int, struct siginfo *, pid_t);
===== kernel/signal.c 1.127 vs edited =====
--- 1.127/kernel/signal.c	2004-07-06 23:25:30 -07:00
+++ edited/kernel/signal.c	2004-08-25 17:04:37 -07:00
@@ -1243,6 +1243,25 @@
 	force_sig_info(sig, (void*)1L, p);
 }
 
+/*
+ * When things go south during signal handling, we
+ * will force a SIGSEGV. And if the signal that caused
+ * the problem was already a SIGSEGV, we'll want to
+ * make sure we don't even try to deliver the signal..
+ */
+int
+force_sigsegv(int sig, struct task_struct *p)
+{
+	if (sig == SIGSEGV) {
+		unsigned long flags;
+		spin_lock_irqsave(&p->sighand->siglock, flags);
+		p->sighand->action[sig - 1].sa.sa_handler = SIG_DFL;
+		spin_unlock_irqrestore(&p->sighand->siglock, flags);
+	}
+	force_sig(SIGSEGV, p);
+	return 0;
+}
+
 int
 kill_pg(pid_t pgrp, int sig, int priv)
 {
===== arch/alpha/kernel/signal.c 1.24 vs edited =====
--- 1.24/arch/alpha/kernel/signal.c	2004-07-15 10:56:07 -07:00
+++ edited/arch/alpha/kernel/signal.c	2004-08-25 16:58:09 -07:00
@@ -469,9 +469,7 @@
 	return;
 
 give_sigsegv:
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(sig, current);
 }
 
 static void
@@ -533,9 +531,7 @@
 	return;
 
 give_sigsegv:
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(sig, current);
 }
 
 
===== arch/arm/kernel/signal.c 1.33 vs edited =====
--- 1.33/arch/arm/kernel/signal.c	2004-06-18 10:58:27 -07:00
+++ edited/arch/arm/kernel/signal.c	2004-08-25 16:54:07 -07:00
@@ -639,9 +639,7 @@
 		return;
 	}
 
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, tsk);
+	force_sigsegv(sig, tsk);
 }
 
 /*
===== arch/arm26/kernel/signal.c 1.1 vs edited =====
--- 1.1/arch/arm26/kernel/signal.c	2003-06-04 04:15:45 -07:00
+++ edited/arch/arm26/kernel/signal.c	2004-08-25 16:54:07 -07:00
@@ -465,9 +465,7 @@
 		return;
 	}
 
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, tsk);
+	force_sigsegv(sig, tsk);
 }
 
 /*
===== arch/cris/arch-v10/kernel/signal.c 1.13 vs edited =====
--- 1.13/arch/cris/arch-v10/kernel/signal.c	2004-06-01 02:27:58 -07:00
+++ edited/arch/cris/arch-v10/kernel/signal.c	2004-08-25 16:54:07 -07:00
@@ -409,9 +409,7 @@
 	return;
 
 give_sigsegv:
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(sig, current);
 }
 
 static void setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
@@ -475,9 +473,7 @@
 	return;
 
 give_sigsegv:
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(sig, current);
 }
 
 /*
===== arch/h8300/kernel/signal.c 1.6 vs edited =====
--- 1.6/arch/h8300/kernel/signal.c	2004-05-14 19:00:16 -07:00
+++ edited/arch/h8300/kernel/signal.c	2004-08-25 16:55:15 -07:00
@@ -391,9 +391,7 @@
 	return;
 
 give_sigsegv:
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(sig, current);
 }
 
 static void setup_rt_frame (int sig, struct k_sigaction *ka, siginfo_t *info,
@@ -443,9 +441,7 @@
 	return;
 
 give_sigsegv:
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(sig, current);
 }
 
 static inline void
===== arch/i386/kernel/signal.c 1.41 vs edited =====
--- 1.41/arch/i386/kernel/signal.c	2004-07-13 05:53:49 -07:00
+++ edited/arch/i386/kernel/signal.c	2004-08-25 16:54:07 -07:00
@@ -411,9 +411,7 @@
 	return;
 
 give_sigsegv:
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(sig, current);
 }
 
 static void setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
@@ -492,9 +490,7 @@
 	return;
 
 give_sigsegv:
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(sig, current);
 }
 
 /*
===== arch/ia64/ia32/ia32_signal.c 1.28 vs edited =====
--- 1.28/arch/ia64/ia32/ia32_signal.c	2004-05-10 23:44:41 -07:00
+++ edited/arch/ia64/ia32/ia32_signal.c	2004-08-25 16:54:07 -07:00
@@ -882,9 +882,7 @@
 	return 1;
 
   give_sigsegv:
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(sig, current);
 	return 0;
 }
 
@@ -952,9 +950,7 @@
 	return 1;
 
 give_sigsegv:
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(sig, current);
 	return 0;
 }
 
===== arch/m68k/kernel/signal.c 1.19 vs edited =====
--- 1.19/arch/m68k/kernel/signal.c	2004-07-28 21:58:40 -07:00
+++ edited/arch/m68k/kernel/signal.c	2004-08-25 16:54:07 -07:00
@@ -842,9 +842,7 @@
 	return;
 
 give_sigsegv:
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(sig, current);
 	goto adjust_stack;
 }
 
@@ -925,9 +923,7 @@
 	return;
 
 give_sigsegv:
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(sig, current);
 	goto adjust_stack;
 }
 
===== arch/m68knommu/kernel/signal.c 1.10 vs edited =====
--- 1.10/arch/m68knommu/kernel/signal.c	2004-02-18 16:43:09 -08:00
+++ edited/arch/m68knommu/kernel/signal.c	2004-08-25 16:54:07 -07:00
@@ -616,9 +616,7 @@
 	return;
 
 give_sigsegv:
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(sig, current);
 	goto adjust_stack;
 }
 
@@ -685,9 +683,7 @@
 	return;
 
 give_sigsegv:
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(sig, current);
 	goto adjust_stack;
 }
 
===== arch/mips/kernel/irixsig.c 1.13 vs edited =====
--- 1.13/arch/mips/kernel/irixsig.c	2004-05-29 02:13:12 -07:00
+++ edited/arch/mips/kernel/irixsig.c	2004-08-25 17:12:52 -07:00
@@ -121,9 +121,7 @@
 	return;
 
 segv_and_exit:
-	if (signr == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(signr, current);
 }
 
 static void inline
===== arch/mips/kernel/signal.c 1.16 vs edited =====
--- 1.16/arch/mips/kernel/signal.c	2004-05-10 04:25:30 -07:00
+++ edited/arch/mips/kernel/signal.c	2004-08-25 16:54:07 -07:00
@@ -406,9 +406,7 @@
         return;
 
 give_sigsegv:
-	if (signr == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(signr, current);
 }
 #endif
 
@@ -475,9 +473,7 @@
 	return;
 
 give_sigsegv:
-	if (signr == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(signr, current);
 }
 
 extern void setup_rt_frame_n32(struct k_sigaction * ka,
===== arch/mips/kernel/signal32.c 1.17 vs edited =====
--- 1.17/arch/mips/kernel/signal32.c	2004-05-10 04:25:30 -07:00
+++ edited/arch/mips/kernel/signal32.c	2004-08-25 17:14:16 -07:00
@@ -574,9 +574,7 @@
         return;
 
 give_sigsegv:
-	if (signr == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(signr, current);
 }
 
 static inline void setup_rt_frame(struct k_sigaction * ka,
@@ -647,9 +645,7 @@
 	return;
 
 give_sigsegv:
-	if (signr == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(signr, current);
 }
 
 static inline void handle_signal(unsigned long sig, siginfo_t *info,
===== arch/mips/kernel/signal_n32.c 1.4 vs edited =====
--- 1.4/arch/mips/kernel/signal_n32.c	2004-05-10 04:25:30 -07:00
+++ edited/arch/mips/kernel/signal_n32.c	2004-08-25 17:12:52 -07:00
@@ -208,7 +208,5 @@
 	return;
 
 give_sigsegv:
-	if (signr == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(signr, current);
 }
===== arch/ppc/kernel/signal.c 1.35 vs edited =====
--- 1.35/arch/ppc/kernel/signal.c	2004-07-26 14:42:38 -07:00
+++ edited/arch/ppc/kernel/signal.c	2004-08-25 16:54:07 -07:00
@@ -404,9 +404,7 @@
 	printk("badframe in handle_rt_signal, regs=%p frame=%p newsp=%lx\n",
 	       regs, frame, newsp);
 #endif
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(sig, current);
 }
 
 static int do_setcontext(struct ucontext __user *ucp, struct pt_regs *regs, int sig)
@@ -556,9 +554,7 @@
 	printk("badframe in handle_signal, regs=%p frame=%p newsp=%lx\n",
 	       regs, frame, newsp);
 #endif
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(sig, current);
 }
 
 /*
===== arch/ppc64/kernel/signal32.c 1.54 vs edited =====
--- 1.54/arch/ppc64/kernel/signal32.c	2004-07-05 03:27:10 -07:00
+++ edited/arch/ppc64/kernel/signal32.c	2004-08-25 17:12:52 -07:00
@@ -699,9 +699,7 @@
 	printk("badframe in handle_rt_signal, regs=%p frame=%p newsp=%lx\n",
 	       regs, frame, newsp);
 #endif
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(sig, current);
 }
 
 static long do_setcontext32(struct ucontext32 __user *ucp, struct pt_regs *regs, int sig)
@@ -864,9 +862,7 @@
 	printk("badframe in handle_signal, regs=%p frame=%x newsp=%x\n",
 	       regs, frame, *newspp);
 #endif
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(sig, current);
 }
 
 /*
===== arch/s390/kernel/compat_signal.c 1.12 vs edited =====
--- 1.12/arch/s390/kernel/compat_signal.c	2004-08-07 11:05:35 -07:00
+++ edited/arch/s390/kernel/compat_signal.c	2004-08-25 16:54:07 -07:00
@@ -552,9 +552,7 @@
 	return;
 
 give_sigsegv:
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(sig, current);
 }
 
 static void setup_rt_frame32(int sig, struct k_sigaction *ka, siginfo_t *info,
@@ -604,9 +602,7 @@
 	return;
 
 give_sigsegv:
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(sig, current);
 }
 
 /*
===== arch/s390/kernel/signal.c 1.27 vs edited =====
--- 1.27/arch/s390/kernel/signal.c	2004-07-11 01:59:28 -07:00
+++ edited/arch/s390/kernel/signal.c	2004-08-25 16:54:07 -07:00
@@ -358,9 +358,7 @@
 	return;
 
 give_sigsegv:
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(sig, current);
 }
 
 static void setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
@@ -414,9 +412,7 @@
 	return;
 
 give_sigsegv:
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(sig, current);
 }
 
 /*
===== arch/sh/kernel/signal.c 1.13 vs edited =====
--- 1.13/arch/sh/kernel/signal.c	2004-02-13 07:19:27 -08:00
+++ edited/arch/sh/kernel/signal.c	2004-08-25 16:54:07 -07:00
@@ -413,9 +413,7 @@
 	return;
 
 give_sigsegv:
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(sig, current);
 }
 
 static void setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
@@ -490,9 +488,7 @@
 	return;
 
 give_sigsegv:
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(sig, current);
 }
 
 /*
===== arch/um/kernel/signal_kern.c 1.16 vs edited =====
--- 1.16/arch/um/kernel/signal_kern.c	2004-08-24 02:08:29 -07:00
+++ edited/arch/um/kernel/signal_kern.c	2004-08-25 17:12:52 -07:00
@@ -31,17 +31,6 @@
 EXPORT_SYMBOL(block_signals);
 EXPORT_SYMBOL(unblock_signals);
 
-static void force_segv(int sig)
-{
-	if(sig == SIGSEGV){
-		struct k_sigaction *ka;
-
-		ka = &current->sighand->action[SIGSEGV - 1];
-		ka->sa.sa_handler = SIG_DFL;
-	}
-	force_sig(SIGSEGV, current);
-}
-
 #define _S(nr) (1<<((nr)-1))
 
 #define _BLOCKABLE (~(_S(SIGKILL) | _S(SIGSTOP)))
@@ -124,7 +113,7 @@
 
 	return(0);
  segv:
-	force_segv(signr);
+	force_sigsegv(signr, current);
 	return(1);
 }
 
===== arch/v850/kernel/signal.c 1.8 vs edited =====
--- 1.8/arch/v850/kernel/signal.c	2003-02-17 18:41:09 -08:00
+++ edited/arch/v850/kernel/signal.c	2004-08-25 16:54:07 -07:00
@@ -344,9 +344,7 @@
 	return;
 
 give_sigsegv:
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(sig, current);
 }
 
 static void setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
@@ -421,9 +419,7 @@
 	return;
 
 give_sigsegv:
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(sig, current);
 }
 
 /*
===== arch/x86_64/ia32/ia32_signal.c 1.27 vs edited =====
--- 1.27/arch/x86_64/ia32/ia32_signal.c	2004-08-24 02:08:31 -07:00
+++ edited/arch/x86_64/ia32/ia32_signal.c	2004-08-25 17:12:52 -07:00
@@ -499,9 +499,7 @@
 	return;
 
 give_sigsegv:
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	signal_fault(regs,frame,"32bit signal deliver");
+	force_sigsegv(sig, current);
 }
 
 void ia32_setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
@@ -595,8 +593,6 @@
 	return;
 
 give_sigsegv:
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	signal_fault(regs, frame, "32bit rt signal setup"); 
+	force_sigsegv(sig, current);
 }
 
===== arch/x86_64/kernel/signal.c 1.24 vs edited =====
--- 1.24/arch/x86_64/kernel/signal.c	2004-08-24 02:08:31 -07:00
+++ edited/arch/x86_64/kernel/signal.c	2004-08-25 17:12:52 -07:00
@@ -329,9 +329,7 @@
 	return;
 
 give_sigsegv:
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	signal_fault(regs,frame,"signal deliver");
+	force_sigsegv(sig, current);
 }
 
 /*
===== arch/sh64/kernel/signal.c 1.1 vs edited =====
--- 1.1/arch/sh64/kernel/signal.c	2004-06-29 07:44:46 -07:00
+++ edited/arch/sh64/kernel/signal.c	2004-08-25 16:56:02 -07:00
@@ -520,9 +520,7 @@
 	return;
 
 give_sigsegv:
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(sig, current);
 }
 
 static void setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t *info,
@@ -628,9 +626,7 @@
 	return;
 
 give_sigsegv:
-	if (sig == SIGSEGV)
-		ka->sa.sa_handler = SIG_DFL;
-	force_sig(SIGSEGV, current);
+	force_sigsegv(sig, current);
 }
 
 /*

Re: [patch 1/8] signal handling race fix

[David S. Miller]

On Wed, 25 Aug 2004 17:19:02 -0700 (PDT)
Linus Torvalds <torvalds@osdl.org> wrote:

> On Wed, 25 Aug 2004, David S. Miller wrote:
> > 
> > I think this should be a seperate change and not block this one.
> 
> Agreed. This looks trivially obvious. No?

Looks ok to me.

Let's get Andrew's edited version of the original patch
in, then slap this one on top.

I'll then convert Sparc's signal code over to using this
force_sigsegv() stuff, I just call exit() directly currently
which isn't so nice.

Re: [patch 1/8] signal handling race fix

[Linus Torvalds]

On Wed, 25 Aug 2004, David S. Miller wrote:
> 
> Let's get Andrew's edited version of the original patch
> in, then slap this one on top.

I'd prefer doing it the other way - since Andrew's patch won't work 
without this _or_ some ugly changes.

Ie the old signal code usually did

	if (sig == SIGSEGV)
		ka->handler = SIG_DFL;

which just doesn't work if "ka" is just a copy of the real handler
information.

So the signal handling race fix needs to either have my quick cleanup 
patch in place already, or it needs to change all the "ka->..." to 
"current->sighand->" only to have it all be removed later immediately by 
my cleanup patch.

So the cleanup patch logically goes first. After that, Andrew's patch is 
going to be really small, methinks.

		Linus

Re: [patch 1/8] signal handling race fix

[Linus Torvalds]

On Wed, 25 Aug 2004, David S. Miller wrote:
> 
> I'll then convert Sparc's signal code over to using this
> force_sigsegv() stuff, I just call exit() directly currently
> which isn't so nice.

Ok, I've merged (hopefully correctly) Andrew's series. It was _much_ 
smaller after removing the renaming, some whitespace stuff, and using the 
generic cleanup patch.

I also, just to check, tried to do the ARM conversion just to see what it 
looks like when the patch hasn't gone through several different 
incarnations and some hand-editing.. Russell, can you check my test patch 
(appended).

Can architecture people check the result of their respective 
architectures? I'll check x86 and ppc64 myself, but..

		Linus

----
===== arch/arm/kernel/signal.c 1.34 vs edited =====
--- 1.34/arch/arm/kernel/signal.c	2004-08-25 17:37:25 -07:00
+++ edited/arch/arm/kernel/signal.c	2004-08-25 18:38:23 -07:00
@@ -573,12 +573,12 @@
  * OK, we're invoking a handler
  */	
 static void
-handle_signal(unsigned long sig, siginfo_t *info, sigset_t *oldset,
+handle_signal(unsigned long sig, struct k_sigaction *ka,
+	      siginfo_t *info, sigset_t *oldset,
 	      struct pt_regs * regs, int syscall)
 {
 	struct thread_info *thread = current_thread_info();
 	struct task_struct *tsk = current;
-	struct k_sigaction *ka = &tsk->sighand->action[sig-1];
 	int usig = sig;
 	int ret;
 
@@ -633,11 +633,8 @@
 		spin_unlock_irq(&tsk->sighand->siglock);
 	}
 
-	if (ret == 0) {
-		if (ka->sa.sa_flags & SA_ONESHOT)
-			ka->sa.sa_handler = SIG_DFL;
+	if (ret == 0)
 		return;
-	}
 
 	force_sigsegv(sig, tsk);
 }
@@ -653,6 +650,7 @@
  */
 static int do_signal(sigset_t *oldset, struct pt_regs *regs, int syscall)
 {
+	struct k_sigaction ka;
 	siginfo_t info;
 	int signr;
 
@@ -673,9 +671,9 @@
 	if (current->ptrace & PT_SINGLESTEP)
 		ptrace_cancel_bpt(current);
 
-	signr = get_signal_to_deliver(&info, regs, NULL);
+	signr = get_signal_to_deliver(&info, &ka, regs, NULL);
 	if (signr > 0) {
-		handle_signal(signr, &info, oldset, regs, syscall);
+		handle_signal(signr, &ka, &info, oldset, regs, syscall);
 		if (current->ptrace & PT_SINGLESTEP)
 			ptrace_set_bpt(current);
 		return 1;

Re: [patch 1/8] signal handling race fix

[Russell King]

On Wed, Aug 25, 2004 at 06:46:40PM -0700, Linus Torvalds wrote:
> I also, just to check, tried to do the ARM conversion just to see what it 
> looks like when the patch hasn't gone through several different 
> incarnations and some hand-editing.. Russell, can you check my test patch 
> (appended).

It looks fine - test booted here, ran through a few signal handling tests
and everything seems to be happy.  Who's going to apply this patch?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

Re: [patch 1/8] signal handling race fix

[Linus Torvalds]

On Thu, 26 Aug 2004, Russell King wrote:
> 
> It looks fine - test booted here, ran through a few signal handling tests
> and everything seems to be happy.  Who's going to apply this patch?

Please you do it, I'll get it from the next arm merge. 

		Linus

Re: [patch 1/8] signal handling race fix

[Andrew Morton]

Linus Torvalds <torvalds@osdl.org> wrote:
>
> 
> 
> On Wed, 25 Aug 2004 akpm@osdl.org wrote:
> > 
> > The patch:
> > 
> >   You have to get the sigaction information that will be delivered while
> >   holding sighand->siglock in get_signal_to_deliver().
> 
> I'm ok with the patch, but I _hate_ the renaming of "ka" to "ka_copy". 
> There's no real point except for the initial change (to make sure the 
> compiler warns about any missed entries), and once that has been done it 
> just makes the patches horribly noisy and not at all more readable.

OK, I edited all the diffs.  I'll let this cook for a bit longer.

> Oh, and ia64 suddenly does the "force_sigsegv()" right, while at least the 
> regular x86 port is not fixed. Shouldn't we just do the "force_sigsegv()" 
> right for everybody, and put that one into kernel/signal.c?

hm.  What does "right" mean?  It newly takes that lock, and cleans things
up a bit.  And it adds two different implementations of force_sigsegv().

This an area in which I do not normally dabble...

Re: [patch 1/8] signal handling race fix

[David S. Miller]

On Wed, 25 Aug 2004 16:49:00 -0700
Andrew Morton <akpm@osdl.org> wrote:

> > I'm ok with the patch, but I _hate_ the renaming of "ka" to "ka_copy". 
> > There's no real point except for the initial change (to make sure the 
> > compiler warns about any missed entries), and once that has been done it 
> > just makes the patches horribly noisy and not at all more readable.
> 
> OK, I edited all the diffs.  I'll let this cook for a bit longer.

Andrew please no, this bug fix has been gathering dust long
enough. :-)  I remember seeing the first version of it half
a year ago.

Re: [patch 1/8] signal handling race fix

[Andrew Morton]

"David S. Miller" <davem@redhat.com> wrote:
>
> > OK, I edited all the diffs.  I'll let this cook for a bit longer.
> 
> Andrew please no, this bug fix has been gathering dust long
> enough. :-)  I remember seeing the first version of it half
> a year ago.

OK.  It compiles on three architectures..

Re: [patch 1/8] signal handling race fix

[Martin Schwidefsky]

> Can architecture people check the result of their respective 
> architectures? I'll check x86 and ppc64 myself, but..

Small name clash on s390. The attached patch renames the
offending force_sigsegv in arch/s390/mm/fault to do_sigsegv.
With the change the test app, the signal tests from the glibc
testsuite and the signal tests from the ltp testsuite run fine
with the current BitKeeper tree.

blue skies,
  Martin.

---

[PATCH] s390: force_sigsegv name clash.

From: Martin Schwidefsky <schwidefsky@de.ibm.com>

The recent signal fix broke s390 because of a name clash.
Rename the s390 arch function to do_sigsegv.

Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>

diffstat:
 arch/s390/mm/fault.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff -urN linux-2.6/arch/s390/mm/fault.c linux-2.6-s390/arch/s390/mm/fault.c
--- linux-2.6/arch/s390/mm/fault.c	Sat Aug 14 12:56:26 2004
+++ linux-2.6-s390/arch/s390/mm/fault.c	Fri Aug 27 13:24:18 2004
@@ -126,8 +126,8 @@
  * Send SIGSEGV to task.  This is an external routine
  * to keep the stack usage of do_page_fault small.
  */
-static void force_sigsegv(struct pt_regs *regs, unsigned long error_code,
-			  int si_code, unsigned long address)
+static void do_sigsegv(struct pt_regs *regs, unsigned long error_code,
+		       int si_code, unsigned long address)
 {
 	struct siginfo si;
 
@@ -282,7 +282,7 @@
         if (regs->psw.mask & PSW_MASK_PSTATE) {
                 tsk->thread.prot_addr = address;
                 tsk->thread.trap_no = error_code;
-		force_sigsegv(regs, error_code, si_code, address);
+		do_sigsegv(regs, error_code, si_code, address);
                 return;
 	}
 

[patch 1/8] signal handling race fix

[akpm]

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);
	}
}


Signed-off-by: Andrew Morton <akpm@osdl.org>
---

 25-akpm/arch/i386/kernel/signal.c |   24 ++++++++++--------------
 25-akpm/include/linux/signal.h    |    2 +-
 25-akpm/kernel/signal.c           |   12 ++++++++++--
 3 files changed, 21 insertions(+), 17 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	Wed Aug 25 17:13:40 2004
+++ 25-akpm/arch/i386/kernel/signal.c	Wed Aug 25 17:13:40 2004
@@ -340,7 +340,7 @@ extern void __user __kernel_sigreturn;
 extern void __user __kernel_rt_sigreturn;
 
 static void setup_frame(int sig, struct k_sigaction *ka,
-			sigset_t *set, struct pt_regs * regs)
+			sigset_t *set, struct pt_regs *regs)
 {
 	void __user *restorer;
 	struct sigframe __user *frame;
@@ -412,12 +412,12 @@ 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,
-			   sigset_t *set, struct pt_regs * regs)
+static void setup_rt_frame(int sig, struct k_sigaction *ka,
+			siginfo_t *info, sigset_t *set, struct pt_regs *regs)
 {
 	void __user *restorer;
 	struct rt_sigframe __user *frame;
@@ -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,
+	      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 @@ handle_signal(unsigned long sig, siginfo
 	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 @@ 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 +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, 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, 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	Wed Aug 25 17:13:40 2004
+++ 25-akpm/include/linux/signal.h	Wed Aug 25 17:13:40 2004
@@ -217,7 +217,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	Wed Aug 25 17:13:40 2004
+++ 25-akpm/kernel/signal.c	Wed Aug 25 17:35:08 2004
@@ -1724,7 +1724,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;
@@ -1793,8 +1794,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.
_

[patch 1/8] signal handling race fix

[akpm]

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);
	}
}


Signed-off-by: Andrew Morton <akpm@osdl.org>
---

 25-akpm/arch/i386/kernel/signal.c |   64 +++++++++++++++++---------------------
 25-akpm/include/linux/signal.h    |    2 -
 25-akpm/kernel/signal.c           |   12 +++++--
 3 files changed, 41 insertions(+), 37 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-08-25 14:26:46.505625576 -0700
+++ 25-akpm/arch/i386/kernel/signal.c	2004-08-25 14:26:46.513624360 -0700
@@ -311,7 +311,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;
 
@@ -319,16 +319,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
 extern void __user __kernel_sigreturn;
 extern void __user __kernel_rt_sigreturn;
 
-static void setup_frame(int sig, struct k_sigaction *ka,
-			sigset_t *set, struct pt_regs * regs)
+static void setup_frame(int sig, struct k_sigaction *ka_copy,
+			sigset_t *set, struct pt_regs *regs)
 {
 	void __user *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,
-			   sigset_t *set, struct pt_regs * regs)
+static void setup_rt_frame(int sig, struct k_sigaction *ka_copy,
+			siginfo_t *info, sigset_t *set, struct pt_regs *regs)
 {
 	void __user *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-08-25 14:26:46.507625272 -0700
+++ 25-akpm/include/linux/signal.h	2004-08-25 14:26:46.513624360 -0700
@@ -217,7 +217,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-08-25 14:26:46.508625120 -0700
+++ 25-akpm/kernel/signal.c	2004-08-25 14:26:46.515624056 -0700
@@ -1724,7 +1724,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;
@@ -1793,8 +1794,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.
_