- UML - fix signal mask on delivery error

From Bodo Stroesser - If the user stack limit is reached or the

Quoted 2 lines

From Bodo Stroesser - If the user stack limit is reached or the

signal stack assigned with sigaltstack() is invalid when a user signal handler with SA_ONSTACK has to be started, the signal mask of the interrupted user program is modified. This happens because the mask, that should be used with the handler only, is written to "current->blocked" even if the handler could not be started. But without a handler, no rewrite of the original mask at sys_sigreturn will be done. A slightly different case is sys_sigsuspend(), where the mask is already modified when kern_do_signal() is started. "*oldset" and "current->blocked" are not equal here and thus current->blocked has to be set to *oldset, if an error occurs in handle_signal(). For both cases I've written small tests, and with the patch the result is OK.
This issue is relevant for other architectures too (e.g. i386, I've seen).

Signed-off-by Jeff Dike <[email protected]>

Index: 2.6.9/arch/um/kernel/signal_kern.c
===================================================================

arch/um/kernel/signal_kern.c +8 -4

@@ -79,7 +79,14 @@
 	else
 		err = setup_signal_stack_sc(sp, signr, ka, regs, oldset);
 
-	if (!err && !(ka->sa.sa_flags & SA_NODEFER)) {
+	if(err){
+		spin_lock_irq(&current->sighand->siglock);
+		current->blocked = *oldset;
+		recalc_sigpending();
+		spin_unlock_irq(&current->sighand->siglock);
+		force_sigsegv(signr, current);
+	}
+	else if(!(ka->sa.sa_flags & SA_NODEFER)){
 		spin_lock_irq(&current->sighand->siglock);
 		sigorsets(&current->blocked, &current->blocked, 
 			  &ka->sa.sa_mask);

@@ -87,9 +94,6 @@
 		recalc_sigpending();
 		spin_unlock_irq(&current->sighand->siglock);
 	}
-
-	if(err)
-		force_sigsegv(signr, current);
 }
 
 static int kern_do_signal(struct pt_regs *regs, sigset_t *oldset)

Could you send one of the test apps? This is something which the arch maintainers might want to look into fixing, thanks.

step_sighdlr.c - PTRACE_SINGLESTEPs into a signal handler, if I'm
reading the code right, fails on i386

Quoted 2 lines · Jeff Dike

step_sighdlr.c - PTRACE_SINGLESTEPs into a signal handler, if I'm
reading the code right, fails on i386

I haven't looked at this particular test but given the description I think it'll fail on ppc64 too. This ought to fix it for 2.6, along with single-stepping over syscalls and single-stepping over sigreturn (which is a special case).

Quoted 2 lines · [email protected] said:

Could you send one of the test apps? This is something which the arch maintainers might want to look into fixing, thanks.

Attached...

Included are
breakout.c - UML-specific breakouts, fixed in 2.6 and 2.4 now, but
please don't popularize these :-)

step_sighdlr.c - PTRACE_SINGLESTEPs into a signal handler, if I'm
reading the code right, fails on i386

interrupted_syscall.c - counts signals and system calls during
interrupted system calls, under ptrace and not. Fails on x86 under ptrace.

kernel_restorer.c - I can't get this to compile because it's too
intimate with the libc headers, expecting to get a k_sigaction from them.

sigmasking.c - Makes sure that when a signal is (not) delivered to
a bogus stack, that a segfault is delivered then, and not after returning to userspace. This is the test relevant to the patch that Andrew replied to.

Jeff

I guess, you are notb using i386 2.6.9. On a 2.6.9-vanilla i386 the test doesn't fail!

Quoted 2 lines · [email protected] said:

I guess, you are notb using i386 2.6.9. On a 2.6.9-vanilla i386 the test doesn't fail!

Sorry, I should have mentioned that this host was running 2.4 (stock 2.4.26).

Jeff

sigmasking.c - Makes sure that when a signal is (not) delivered to
a bogus stack, that a segfault is delivered then, and not after returning to userspace. This is the test relevant to the patch that Andrew replied to.

Quoted 4 lines · Jeff Dike

sigmasking.c - Makes sure that when a signal is (not) delivered to
a bogus stack, that a segfault is delivered then, and not after returning to userspace. This is the test relevant to the patch that Andrew replied to.

Corresponding patch for ppc64. Are we actually going to fix this for all architectures?

------------------------------------------------------------------------

===== arch/ppc64/kernel/signal.c 1.44 vs edited =====

arch/ppc64/kernel/signal.c +10 -7

@@ -387,7 +387,7 @@
 	return 0;
 }
 
-static void setup_rt_frame(int signr, struct k_sigaction *ka, siginfo_t *info,
+static int setup_rt_frame(int signr, struct k_sigaction *ka, siginfo_t *info,
 		sigset_t *set, struct pt_regs *regs)
 {
 	/* Handler is *really* a pointer to the function descriptor for

@@ -452,7 +452,7 @@
 	if (err)
 		goto badframe;
 
-	return;
+	return 0;
 
 badframe:
 #if DEBUG_SIG

@@ -460,17 +460,19 @@
 	       regs, frame, newsp);
 #endif
 	force_sigsegv(signr, current);
+	return -1;
 }
 
 
 /*
  * OK, we're invoking a handler
  */
-static void handle_signal(unsigned long sig, struct k_sigaction *ka,
-			  siginfo_t *info, sigset_t *oldset, struct pt_regs *regs)
+static int handle_signal(unsigned long sig, struct k_sigaction *ka,
+			 siginfo_t *info, sigset_t *oldset, struct pt_regs *regs)
 {
 	/* Set up Signal Frame */
-	setup_rt_frame(sig, ka, info, oldset, regs);
+	if (setup_rt_frame(sig, ka, info, oldset, regs))
+	    return 0;
 
 	if (!(ka->sa.sa_flags & SA_NODEFER)) {
 		spin_lock_irq(&current->sighand->siglock);

@@ -479,6 +481,8 @@
 		recalc_sigpending();
 		spin_unlock_irq(&current->sighand->siglock);
 	}
+
+	return 1;
 }
 
 static inline void syscall_restart(struct pt_regs *regs, struct k_sigaction *ka)

@@ -538,8 +542,7 @@
 		/* Whee!  Actually deliver the signal.  */
 		if (TRAP(regs) == 0x0C00)
 			syscall_restart(regs, &ka);
-		handle_signal(signr, &ka, &info, oldset, regs);
-		return 1;
+		return handle_signal(signr, &ka, &info, oldset, regs);
 	}
 
 	if (TRAP(regs) == 0x0C00) {	/* System Call! */

===== arch/ppc64/kernel/signal32.c 1.61 vs edited =====

arch/ppc64/kernel/signal32.c +20 -11

@@ -653,9 +653,9 @@
  * Set up a signal frame for a "real-time" signal handler
  * (one which gets siginfo).
  */
-static void handle_rt_signal32(unsigned long sig, struct k_sigaction *ka,
-			       siginfo_t *info, sigset_t *oldset,
-			       struct pt_regs * regs, unsigned long newsp)
+static int handle_rt_signal32(unsigned long sig, struct k_sigaction *ka,
+			      siginfo_t *info, sigset_t *oldset,
+			      struct pt_regs * regs, unsigned long newsp)
 {
 	struct rt_sigframe32 __user *rt_sf;
 	struct mcontext32 __user *frame;

@@ -704,14 +704,14 @@
 	regs->trap = 0;
 	regs->result = 0;
 
-	return;
+	return 1;
 
 badframe:
 #if DEBUG_SIG
 	printk("badframe in handle_rt_signal, regs=%p frame=%p newsp=%lx\n",
 	       regs, frame, newsp);
 #endif
-	force_sigsegv(sig, current);
+	return 0;
 }
 
 static long do_setcontext32(struct ucontext32 __user *ucp, struct pt_regs *regs, int sig)

@@ -822,7 +822,7 @@
 /*
  * OK, we're invoking a handler
  */
-static void handle_signal32(unsigned long sig, struct k_sigaction *ka,
+static int handle_signal32(unsigned long sig, struct k_sigaction *ka,
 			    siginfo_t *info, sigset_t *oldset,
 			    struct pt_regs * regs, unsigned long newsp)
 {

@@ -867,14 +867,14 @@
 	regs->trap = 0;
 	regs->result = 0;
 
-	return;
+	return 1;
 
 badframe:
 #if DEBUG_SIG
 	printk("badframe in handle_signal, regs=%p frame=%x newsp=%x\n",
 	       regs, frame, *newspp);
 #endif
-	force_sigsegv(sig, current);
+	return 0;
 }
 
 /*

@@ -984,11 +984,20 @@
 
 	/* Whee!  Actually deliver the signal.  */
 	if (ka.sa.sa_flags & SA_SIGINFO)
-		handle_rt_signal32(signr, &ka, &info, oldset, regs, newsp);
+		ret = handle_rt_signal32(signr, &ka, &info, oldset, regs, newsp);
 	else
-		handle_signal32(signr, &ka, &info, oldset, regs, newsp);
+		ret = handle_signal32(signr, &ka, &info, oldset, regs, newsp);
 
-	if (!(ka.sa.sa_flags & SA_NODEFER)) {
+	if (!ret) {
+		/* Setting up the stack frame failed, but if we came here
+		   from sigsuspend we may already have masked signals. 
+		   Put back the old sigmask before forcing SEGV. */
+		spin_lock_irq(&current->sighand->siglock);
+		current->blocked = *oldset;
+		recalc_sigpending();
+		spin_unlock_irq(&current->sighand->siglock);
+		force_sigsegv(signr, current);
+	} else if (!(ka.sa.sa_flags & SA_NODEFER)) {
 		spin_lock_irq(&current->sighand->siglock);
 		sigorsets(&current->blocked, &current->blocked,
 			  &ka.sa.sa_mask);

If a SIGSEGV is forced since the stackframe/sigcontext for a signal-handler could not be created, this SIGSEGV is queued only. To deliver it, do_signal() must be called again, which normally won't happen (with the exception of sys_sigretrun/sys_rt_sigreturn). Thus, SIGSEGV stays in the queue until the

Quoted 4 lines · Bodo Stroesser

If a SIGSEGV is forced since the stackframe/sigcontext for a signal-handler could not be created, this SIGSEGV is queued only. To deliver it, do_signal() must be called again, which normally won't happen (with the exception of sys_sigretrun/sys_rt_sigreturn). Thus, SIGSEGV stays in the queue until the

We got this right on ppc/ppc64, I believe. After calling do_signal, we loop around again (in the exception exit path in entry.S) and check the signal pending and reschedule bits again (in the thread_info flags). We only exit to usermode when they are both zero.

Paul.