From ffe362b00ee53b3d4c78939a3fa9eafc9fce949e Mon Sep 17 00:00:00 2001 From: Corey Minyard Date: Wed, 25 Aug 2004 04:00:58 -0700 Subject: [PATCH] signal handling race fix 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 #include #include 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 Signed-off-by: Linus Torvalds --- kernel/signal.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) (limited to 'kernel') diff --git a/kernel/signal.c b/kernel/signal.c index cd6f2fe4ffe6..e5d6cbc50c1e 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -1743,7 +1743,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 = ¤t->blocked; int signr = 0; @@ -1812,8 +1813,15 @@ relock: ka = ¤t->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. -- cgit v1.2.3