Re: [PATCH 9/11] - UML - fix signal mask on delivery error

Re: [PATCH 9/11] - UML - fix signal mask on delivery error

[Andrew Morton]

(removed linux-kernel, added linux-arch)

Jeff Dike <jdike@addtoit.com> wrote:
>
> >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).

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


> Signed-off-by: Jeff Dike <jdike@addtoit.com>
> 
> Index: 2.6.9/arch/um/kernel/signal_kern.c
> ===================================================================
> --- 2.6.9.orig/arch/um/kernel/signal_kern.c	2004-11-12 16:24:18.000000000 -0500
> +++ 2.6.9/arch/um/kernel/signal_kern.c	2004-11-12 18:05:26.000000000 -0500
> @@ -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)

Re: [PATCH 9/11] - UML - fix signal mask on delivery error

[Jeff Dike]

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

akpm@osdl.org 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


[-- Attachment #2: testtools-bodo-2004-10-29.tar --]
[-- Type: application/x-tar , Size: 40960 bytes --]

Re: [PATCH 9/11] - UML - fix signal mask on delivery error

[David Woodhouse]

On Sun, 2004-11-14 at 17:13 -0500, Jeff Dike wrote:
> 	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). 

The 2.4 kernel also has these problems on most arches -- the fixes for
this are around, but weren't pushed to Marcelo because they weren't
considered a really necessary fix. Was that wrong? Should I dig them out
and send them on?

--- linux-2.6.9/arch/ppc64/kernel/signal.c.sstep	2004-10-18 22:53:05.000000000 +0100
+++ linux-2.6.9/arch/ppc64/kernel/signal.c	2004-11-09 18:48:09.000000000 +0000
@@ -26,6 +26,7 @@
 #include <linux/unistd.h>
 #include <linux/stddef.h>
 #include <linux/elf.h>
+#include <linux/ptrace.h>
 #include <asm/sigcontext.h>
 #include <asm/ucontext.h>
 #include <asm/uaccess.h>
@@ -439,6 +440,9 @@ static void setup_rt_frame(int signr, st
 	if (err)
 		goto badframe;
 
+	if (test_thread_flag(TIF_SINGLESTEP))
+		ptrace_notify(SIGTRAP);
+
 	return;
 
 badframe:
--- linux-2.6.9/arch/ppc64/kernel/entry.S.sstep	2004-10-18 22:53:06.000000000 +0100
+++ linux-2.6.9/arch/ppc64/kernel/entry.S	2004-11-09 18:32:30.000000000 +0000
@@ -162,7 +162,7 @@ syscall_error_cont:
 
 	/* check for syscall tracing or audit */
 	ld	r9,TI_FLAGS(r12)
-	andi.	r0,r9,_TIF_SYSCALL_T_OR_A
+	andi.	r0,r9,(_TIF_SYSCALL_T_OR_A|_TIF_SINGLESTEP)
 	bne-	syscall_exit_trace
 syscall_exit_trace_cont:
 
@@ -317,7 +317,7 @@ _GLOBAL(ppc64_rt_sigreturn)
 	blt	syscall_exit
 	clrrdi	r4,r1,THREAD_SHIFT
 	ld	r4,TI_FLAGS(r4)
-	andi.	r4,r4,_TIF_SYSCALL_T_OR_A
+	andi.	r4,r4,(_TIF_SYSCALL_T_OR_A|_TIF_SINGLESTEP)
 	beq+	81f
 	bl	.do_syscall_trace_leave
 81:	b	.ret_from_except
--- linux-2.6.9/arch/ppc64/kernel/ptrace.c.sstep	2004-10-18 22:54:55.000000000 +0100
+++ linux-2.6.9/arch/ppc64/kernel/ptrace.c	2004-11-09 18:17:29.000000000 +0000
@@ -318,7 +318,8 @@ void do_syscall_trace_leave(void)
 	if (unlikely(current->audit_context))
 		audit_syscall_exit(current, 0);	/* FIXME: pass pt_regs */
 
-	if (test_thread_flag(TIF_SYSCALL_TRACE)
+	if ((test_thread_flag(TIF_SYSCALL_TRACE)
+	     || test_thread_flag(TIF_SINGLESTEP))
 	    && (current->ptrace & PT_PTRACED))
 		do_syscall_trace();
 }
--- linux-2.6.9/arch/ppc64/kernel/signal32.c.sstep	2004-10-18 22:55:07.000000000 +0100
+++ linux-2.6.9/arch/ppc64/kernel/signal32.c	2004-11-09 18:48:39.000000000 +0000
@@ -25,6 +25,7 @@
 #include <linux/errno.h>
 #include <linux/elf.h>
 #include <linux/compat.h>
+#include <linux/ptrace.h>
 #include <asm/ppc32.h>
 #include <asm/uaccess.h>
 #include <asm/ppcdebug.h>
@@ -696,6 +697,9 @@ static void handle_rt_signal32(unsigned 
 	regs->trap = 0;
 	regs->result = 0;
 
+	if (test_thread_flag(TIF_SINGLESTEP))
+		ptrace_notify(SIGTRAP);
+
 	return;
 
 badframe:
@@ -859,6 +863,9 @@ static void handle_signal32(unsigned lon
 	regs->trap = 0;
 	regs->result = 0;
 
+	if (test_thread_flag(TIF_SINGLESTEP))
+		ptrace_notify(SIGTRAP);
+
 	return;
 
 badframe:
--- linux-2.6.9/include/asm-ppc64/ptrace-common.h.sstep	2004-10-18 22:54:37.000000000 +0100
+++ linux-2.6.9/include/asm-ppc64/ptrace-common.h	2004-11-09 18:45:29.000000000 +0000
@@ -58,6 +58,7 @@ static inline void set_single_step(struc
 	struct pt_regs *regs = task->thread.regs;
 	if (regs != NULL)
 		regs->msr |= MSR_SE;
+	set_ti_thread_flag(task->thread_info, TIF_SINGLESTEP);
 }
 
 static inline void clear_single_step(struct task_struct *task)
@@ -65,6 +66,7 @@ static inline void clear_single_step(str
 	struct pt_regs *regs = task->thread.regs;
 	if (regs != NULL)
 		regs->msr &= ~MSR_SE;
+	clear_ti_thread_flag(task->thread_info, TIF_SINGLESTEP);
 }
 
 #endif /* _PPC64_PTRACE_COMMON_H */
--- linux-2.6.9/include/asm-ppc64/thread_info.h.sstep	2004-10-18 22:53:21.000000000 +0100
+++ linux-2.6.9/include/asm-ppc64/thread_info.h	2004-11-09 18:25:59.000000000 +0000
@@ -97,6 +97,7 @@ static inline struct thread_info *curren
 #define TIF_RUN_LIGHT		6	/* iSeries run light */
 #define TIF_ABI_PENDING		7	/* 32/64 bit switch needed */
 #define TIF_SYSCALL_AUDIT	8	/* syscall auditing active */
+#define TIF_SINGLESTEP		9	/* singlestepping active */
 
 /* as above, but as bit values */
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
@@ -108,6 +109,7 @@ static inline struct thread_info *curren
 #define _TIF_RUN_LIGHT		(1<<TIF_RUN_LIGHT)
 #define _TIF_ABI_PENDING	(1<<TIF_ABI_PENDING)
 #define _TIF_SYSCALL_AUDIT	(1<<TIF_SYSCALL_AUDIT)
+#define _TIF_SINGLESTEP		(1<<TIF_SINGLESTEP)
 #define _TIF_SYSCALL_T_OR_A	(_TIF_SYSCALL_TRACE|_TIF_SYSCALL_AUDIT)
 
 #define _TIF_USER_WORK_MASK	(_TIF_NOTIFY_RESUME | _TIF_SIGPENDING | \

 
-- 
dwmw2

Re: [PATCH 9/11] - UML - fix signal mask on delivery error

[Bodo Stroesser]

Jeff Dike wrote:
> akpm@osdl.org 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
I guess, you are notb using i386 2.6.9. On a 2.6.9-vanilla i386 the test
doesn't fail! In 2.6.9 some enhancement of singlestepping syscalls and
singlestepping signal handlers is included. The test checks, whether
UML 2.6.9 is compatible to 2.6.9-i386. With my patchset, it is.
> 
> 	interrupted_syscall.c - counts signals and system calls during 
> interrupted system calls, under ptrace and not.  Fails on x86 under ptrace.
On my 2.6.7 and 2.6.9 kernels, the test doesn't fail. What kerenel are you
using? Could you please send me a listing of the messages from the test?
> 
> 	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.
On my SuSE 9 system, k_sigaction comes from /usr/include/asm/signal.h. This
again comes from glibc-devel-2.3.3-98.28, SuSE supplies. I had to write a
"#define __KERNEL__" into the test to have k_sigaction included. If your
system lacks a k_sigaction in the includes, please copy a definition from
the kernel sources to my test. Yes, that's dirty, but it's a test only!
k_sigaction is needed to call sys_sigaction directly, bypassing the lib.
This was the only way to test the kernels restorer-stub, since the lib
always inserted its own stub, if I used "NULL".
> 
> 	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.
Yes. This should be the only test, that shows differences between i386
2.6.9-vanilla and UML 2.6.9 with my patches.
> 
> 				Jeff

Re: [PATCH 9/11] - UML - fix signal mask on delivery error

[Jeff Dike]

bstroesser@fujitsu-siemens.com 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

Re: [PATCH 9/11] - UML - fix signal mask on delivery error

[Bodo Stroesser]

Jeff Dike wrote:
> bstroesser@fujitsu-siemens.com 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
> 
Yes. That makes sense. There are substantial changes in syscall restarting
between 2.4 and 2.6 (e.g ERESTART_RESTARTBLOCK added). Please run the tests on
2.6.9-i386 to get the correct result: all tests are OK, but sigmasking.

Bodo

Re: [PATCH 9/11] - UML - fix signal mask on delivery error

[David Woodhouse]

On Mon, 2004-11-15 at 08:35 +0000, David Woodhouse wrote:
> On Sun, 2004-11-14 at 17:13 -0500, Jeff Dike wrote:
> > 	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). 

I fixed it too hard -- we don't need to stop _twice_ at the same
instruction on entry to a signal handler; once will suffice...

--- linux-2.6.9/arch/ppc64/kernel/entry.S.sstep	2004-10-18 22:53:06.000000000 +0100
+++ linux-2.6.9/arch/ppc64/kernel/entry.S	2004-11-09 18:32:30.000000000 +0000
@@ -162,7 +162,7 @@ syscall_error_cont:
 
 	/* check for syscall tracing or audit */
 	ld	r9,TI_FLAGS(r12)
-	andi.	r0,r9,_TIF_SYSCALL_T_OR_A
+	andi.	r0,r9,(_TIF_SYSCALL_T_OR_A|_TIF_SINGLESTEP)
 	bne-	syscall_exit_trace
 syscall_exit_trace_cont:
 
@@ -317,7 +317,7 @@ _GLOBAL(ppc64_rt_sigreturn)
 	blt	syscall_exit
 	clrrdi	r4,r1,THREAD_SHIFT
 	ld	r4,TI_FLAGS(r4)
-	andi.	r4,r4,_TIF_SYSCALL_T_OR_A
+	andi.	r4,r4,(_TIF_SYSCALL_T_OR_A|_TIF_SINGLESTEP)
 	beq+	81f
 	bl	.do_syscall_trace_leave
 81:	b	.ret_from_except
--- linux-2.6.9/arch/ppc64/kernel/ptrace.c.sstep	2004-10-18 22:54:55.000000000 +0100
+++ linux-2.6.9/arch/ppc64/kernel/ptrace.c	2004-11-09 18:17:29.000000000 +0000
@@ -318,7 +318,8 @@ void do_syscall_trace_leave(void)
 	if (unlikely(current->audit_context))
 		audit_syscall_exit(current, 0);	/* FIXME: pass pt_regs */
 
-	if (test_thread_flag(TIF_SYSCALL_TRACE)
+	if ((test_thread_flag(TIF_SYSCALL_TRACE)
+	     || test_thread_flag(TIF_SINGLESTEP))
 	    && (current->ptrace & PT_PTRACED))
 		do_syscall_trace();
 }
--- linux-2.6.9/include/asm-ppc64/ptrace-common.h.sstep	2004-10-18 22:54:37.000000000 +0100
+++ linux-2.6.9/include/asm-ppc64/ptrace-common.h	2004-11-09 18:45:29.000000000 +0000
@@ -58,6 +58,7 @@ static inline void set_single_step(struc
 	struct pt_regs *regs = task->thread.regs;
 	if (regs != NULL)
 		regs->msr |= MSR_SE;
+	set_ti_thread_flag(task->thread_info, TIF_SINGLESTEP);
 }
 
 static inline void clear_single_step(struct task_struct *task)
@@ -65,6 +66,7 @@ static inline void clear_single_step(str
 	struct pt_regs *regs = task->thread.regs;
 	if (regs != NULL)
 		regs->msr &= ~MSR_SE;
+	clear_ti_thread_flag(task->thread_info, TIF_SINGLESTEP);
 }
 
 #endif /* _PPC64_PTRACE_COMMON_H */
--- linux-2.6.9/include/asm-ppc64/thread_info.h.sstep	2004-10-18 22:53:21.000000000 +0100
+++ linux-2.6.9/include/asm-ppc64/thread_info.h	2004-11-09 18:25:59.000000000 +0000
@@ -97,6 +97,7 @@ static inline struct thread_info *curren
 #define TIF_RUN_LIGHT		6	/* iSeries run light */
 #define TIF_ABI_PENDING		7	/* 32/64 bit switch needed */
 #define TIF_SYSCALL_AUDIT	8	/* syscall auditing active */
+#define TIF_SINGLESTEP		9	/* singlestepping active */
 
 /* as above, but as bit values */
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
@@ -108,6 +109,7 @@ static inline struct thread_info *curren
 #define _TIF_RUN_LIGHT		(1<<TIF_RUN_LIGHT)
 #define _TIF_ABI_PENDING	(1<<TIF_ABI_PENDING)
 #define _TIF_SYSCALL_AUDIT	(1<<TIF_SYSCALL_AUDIT)
+#define _TIF_SINGLESTEP		(1<<TIF_SINGLESTEP)
 #define _TIF_SYSCALL_T_OR_A	(_TIF_SYSCALL_TRACE|_TIF_SYSCALL_AUDIT)
 
 #define _TIF_USER_WORK_MASK	(_TIF_NOTIFY_RESUME | _TIF_SIGPENDING | \




-- 
dwmw2

Re: [PATCH 9/11] - UML - fix signal mask on delivery error

[David Woodhouse]

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

On Sun, 2004-11-14 at 17:13 -0500, Jeff Dike wrote:
> 	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?

-- 
dwmw2

[-- Attachment #2: linux-2.6.10-ppc64-sigmasking.patch --]
[-- Type: text/x-patch, Size: 4353 bytes --]

===== arch/ppc64/kernel/signal.c 1.44 vs edited =====
--- 1.44/arch/ppc64/kernel/signal.c	2004-10-28 08:39:49 +01:00
+++ edited/arch/ppc64/kernel/signal.c	2004-11-26 14:13:18 +00:00
@@ -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 =====
--- 1.61/arch/ppc64/kernel/signal32.c	2004-10-28 08:39:49 +01:00
+++ edited/arch/ppc64/kernel/signal32.c	2004-11-26 14:13:18 +00:00
@@ -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);

Re: [PATCH 9/11] - UML - fix signal mask on delivery error

[Bodo Stroesser]

David Woodhouse wrote:
> On Sun, 2004-11-14 at 17:13 -0500, Jeff Dike wrote:
> 
>>	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.
I don't know much about ppc64 and also don't have a machine to test.
So, I'm commenting the code only. If I've missed something, sorry for that.

The changes we did in UML, fiy two major issues:
1) they correct the sigmask after a segfault that happened in signal-delivery
2) they immediately deliver that SIGSEGV, before returning to user.

AFAICS, the patch seems to cover the first, but not the second.
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
next interrupt or syscall happens. IMHO this behavior is bad.
So, in UML we changed (kern_)do_signal, to loop over get_signal_to_deliver
and handle_signal, until a signal is delivered correctly or the task exits.

Bodo
> 
> 
> Corresponding patch for ppc64. Are we actually going to fix this for all
> architectures?
> 
> 
> 
> ------------------------------------------------------------------------
> 
> ===== arch/ppc64/kernel/signal.c 1.44 vs edited =====
> --- 1.44/arch/ppc64/kernel/signal.c	2004-10-28 08:39:49 +01:00
> +++ edited/arch/ppc64/kernel/signal.c	2004-11-26 14:13:18 +00:00
> @@ -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 =====
> --- 1.61/arch/ppc64/kernel/signal32.c	2004-10-28 08:39:49 +01:00
> +++ edited/arch/ppc64/kernel/signal32.c	2004-11-26 14:13:18 +00:00
> @@ -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);

Re: [PATCH 9/11] - UML - fix signal mask on delivery error

[Paul Mackerras]

Bodo Stroesser writes:

> 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.

Re: [PATCH 9/11] - UML - fix signal mask on delivery error

[Bodo Stroesser]

Paul Mackerras wrote:
> Bodo Stroesser writes:
> 
> 
>>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.
Aha! Sorry, didn't recognize that loop.

Bodo