[uml-devel] Signal handling: deliver segfault immediately

[uml-devel] Signal handling: deliver segfault immediately

[Stroesser, Bodo]

This one covers the fact, that the SIGSEGV signal, which is created by
force_sigsegv() in case of an error in handle_signal(), is not delivered
to the user immediately. In the worst case it even could be masked if a
sigprocmask() systemcall follows immediately after return from kernel.
The patch is relevant for other architectures, too.

As a further consequence of the patch the first instruction of a signal
handler will be checked for a system call, if the process should be
singlestepped (not a big risk, but better close the hole, I think). If
you don't want this, replace the "break" by "return 1".

Bodo


--- arch/um/kernel/signal_kern.c.orig	2004-09-30 16:34:07.610109219
+0200
+++ arch/um/kernel/signal_kern.c	2004-09-30 16:51:51.743255023
+0200
@@ -38,7 +38,7 @@
 /*
  * OK, we're invoking a handler
  */	
-static void handle_signal(struct pt_regs *regs, unsigned long signr, 
+static int  handle_signal(struct pt_regs *regs, unsigned long signr, 
 			  struct k_sigaction *ka, siginfo_t *info, 
 			  sigset_t *oldset)
 {
@@ -103,23 +103,25 @@
 		recalc_sigpending();
 		spin_unlock_irq(&current->sighand->siglock);
 	}
+
+	return err;
 }
 
 static int kern_do_signal(struct pt_regs *regs, sigset_t *oldset)
 {
 	struct k_sigaction ka_copy;
 	siginfo_t info;
-	int sig;
+	int sig, handled_sig = 0;
 
-	sig = get_signal_to_deliver(&info, &ka_copy, regs, NULL);
-	if(sig > 0){
+	while ( (sig = get_signal_to_deliver(&info, &ka_copy, regs,
NULL)) ) {
+		handled_sig = 1;
 		/* Whee!  Actually deliver the signal.  */
-		handle_signal(regs, sig, &ka_copy, &info, oldset);
-		return(1);
+		if ( ! handle_signal(regs, sig, &ka_copy, &info, oldset)
)
+			break;
 	}
 
 	/* Did we come from a system call? */
-	if(PT_REGS_SYSCALL_NR(regs) >= 0){
+	if(!handled_sig && PT_REGS_SYSCALL_NR(regs) >= 0){
 		/* Restart the system call - no handlers present */
 		if(PT_REGS_SYSCALL_RET(regs) == -ERESTARTNOHAND ||
 		   PT_REGS_SYSCALL_RET(regs) == -ERESTARTSYS ||
@@ -143,7 +145,8 @@
 	if((current->ptrace & PT_DTRACE) && 
 	   is_syscall(PT_REGS_IP(&current->thread.regs)))
  		(void)
CHOOSE_MODE(current->thread.mode.tt.singlestep_syscall = 1, 0);
-	return(0);
+
+	return( handled_sig);
 }
 
 int do_signal(void)


-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] Signal handling: deliver segfault immediately

[Jeff Dike]

> -	if(sig > 0){
> +	while ( (sig = get_signal_to_deliver(&info, &ka_copy, regs,
> NULL)) ) {

This one makes me nervous.  I never checked that nesting signal frames for
multiple pending signals works, although the frame format is identical, so
it should.

Have you checked that?

				Jeff



-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

RE: [uml-devel] Signal handling: deliver segfault immediately

[Stroesser, Bodo]

> > -	if(sig > 0){
> > +	while ( (sig = get_signal_to_deliver(&info, &ka_copy, regs,
> > NULL)) ) {
> 
> This one makes me nervous.  I never checked that nesting signal frames
for multiple pending signals works, although the frame format is
identical, so it should.
> 
> Have you checked that?
> 
> 				Jeff
No, this will not produce nested frames.
If setup_signal_stack_XX() returns an error, no signal frame is
generated. But a new SIGSEGV signal is queued. Normally do_signal() is
called from interrupt_end(), so without the patch the user code will be
started next without any signal handler being delivered and the SIGSEGV
staying in queue! Call me pedantic, but I believe, that's not correct.
I've seen this occur with one of my small tests. Calling sigprocmask()
before the kernel is entered again, one could mask the SIGSEGV
(intentional or by accident).
The patch changes this, so that the next signal is taken from queue, if
the last stack frame creation failed.

And, yes. I've tested this.

Bodo


-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] Signal handling: deliver segfault immediately

[Jeff Dike]

Bodo.Stroesser@fujitsu-siemens.com said:
> I've seen this occur with one of my small tests.

As an aside, can you send your test programs?

Bodo.Stroesser@fujitsu-siemens.com said:
> No, this will not produce nested frames. 

Here's the piece in question:

-	if(sig > 0){
+	while ( (sig = get_signal_to_deliver(&info, &ka_copy, regs,
NULL)) ) {
+		handled_sig = 1;
 		/* Whee!  Actually deliver the signal.  */
-		handle_signal(regs, sig, &ka_copy, &info, oldset);
-		return(1);
+		if ( ! handle_signal(regs, sig, &ka_copy, &info, oldset)
)
+			break;

That's somewhat misleading.  In the normal case, you break, and looping is
only done in the error case.  I'd prefer that the structure of the code reflect
how it normally runs.

> But a new SIGSEGV signal is queued. Normally do_signal() is called
> from interrupt_end(), so without the patch the user code will be
> started next without any signal handler being delivered and the
> SIGSEGV staying in queue! Call me pedantic, but I believe, that's not
> correct.

Yeah, I would agree with that.

> If it is writing the restorers code to stack, please note, there have
> to be different restorers in setup_signal_stack_sc() and
> setup_signal_stack_si(). They must use different systemcalls
> (sys_sigreturn / sys_rt_sigreturn) to remove the signal stack frame
> (refer to arch/i386/kernel/signal.c). With the patch applied, UML
> exactly does, what i386 does.

Yup.  I wasn't paying close enough attention, and compared the i386 rt handler
with the UML non-rt handler.

> No. sys_sigsuspend() modifies current->blocked before calling
> kern_do_signal(). Because of that, it takes a copy of the original
> mask and passes it to kern_do_signal(). This mask has to be restored
> by the signal handler at sys_sigreturn. But if an error occurs, the
> handler will not be started and the mask will not be restored. Then
> kern_do_signal returns to sys_sigsuspend() with the wrong mask set,
> sys_sigsuspend() returns (the original mask is lost at that moment)
> and the SIGSEGV-handler is started later when passing interrupt_end(),
> which calls do_signal() again. The SIGSEGV handler doesn't even see
> the real "original mask", because do_signal() now calls
> kern_do_signal() with current->blocked as oldset, which is the mask
> modified by sys_sigsuspend(). So I believe, handle_signal() should
> restore the original mask if setup_signal_stack_XX() returns an error.
> (For the first case I described, this is a NOOP.) 

OK, you win.  These are now all applied, and will reach Andrew in due course.

				Jeff



-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel