[PATCH] Fix to signal handler processing bug

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,
   The attached patch fixes the signal handler processing bug
described in the last mail. You can use the procedure of that mail to
verify that the bug is really fixed.

Greetings,

Juan

PREVIOUS OPERATION AND BUGS

1. Checking of signals was done at two places: after a task switch and
at the end of a system call, before returning to user space. There is
the possibility of duplicating the work of manipulating the user stack
in a call to schedule() during a system call and again at the end of
the system call.

2. At the end of interrupts, a call to schedule is done before
returning to user space, but checking of signals will be done only
after a task switch, thus missing perfectly valid opportunities to
deliver signals.

3. The user stack layout is different during a system call than during
an interrupt. The function that modifies the user stack to call a
signal handler ("arch_setup_sighandler_stack") assumes the user stack
layout to be as during a system call.

4. However, installing a signal handler after an interrupt did not
(immediately) crash the task because the manipulation of the user
stack did not modify the return address, and the modification to the
user stack pointer was ignored. As result, the signal handler is not
called.

NEW OPERATION

1. Checking of signals is done at the end of a system call, before
returning to user space. Also at the end of any interrupt, if the
interrupt routine will return to user space. It is removed from the
function schedule. Now, checking of signals is done only at the
moments when it makes sense.

2. Function "arch_setup_sighandler_stack" accounts for the difference
in the user stack layout at different times.

3. User stack pointer is reloaded from the task structure before
returning to user space from an interrupt. Now, delivery of signals
always work.

OTHER CHANGES

1. A small optimization to reduce code size was done to function
"run_init_process".

2. Despite the above optimization, there is an increase in code size
of 32 bytes.

 The Image builded without errors. The kernel was tested with QEMU and
dioscuri emulators. Also in a PPro pc booting from floppy.
diff -Nurb elks.orig/arch/i86/kernel/irqtab.c elks/arch/i86/kernel/irqtab.c
--- elks.orig/arch/i86/kernel/irqtab.c	2012-06-20 13:45:28.000000000 -0500
+++ elks/arch/i86/kernel/irqtab.c	2012-06-26 14:14:08.000000000 -0500
@@ -19,19 +19,17 @@
 
    #define SEG_IRQ_DATA es
    #define stashed_ds       [0]
-   /*      _our_ds          [18]   bios16.c  */
 
 #else
  #define SEG_IRQ_DATA cs
-   #define stashed_ds       cseg_stashed_ds
 
 #ifndef S_SPLINT_S
 #asm
-	.globl cseg_stashed_ds
+        .globl stashed_ds
 
         .even
 
-cseg_stashed_ds:
+stashed_ds:
 	.word	0
 
 #endasm
@@ -39,6 +37,8 @@
 
 #endif
 
+extern void sig_check(void);
+
 void irqtab_init(void)
 {
 #ifndef S_SPLINT_S
@@ -413,20 +413,22 @@
         mov     ss,bx           ! /* Set SS: right */
 	mov	bx,_current
 	cmp	dx,TASK_USER_SS[bx] ! entry ss = current->t_regs.ss?
-	je	utask		! Switch to kernel
-!
-!	Bios etc - switch to interrupt stack
-!
-	mov	sp,#_intstack
-        j       ktask
+	jne	btask		! Switch to interrupt stack
 !
 !	User task. Extract kernel SP. (BX already holds current)
 !	At this point, the kernel stack is empty. Thus, we can load
 !       the kernel stack pointer without accesing memory
 !
-utask:
+        mov     TASK_USER_SP[bx],sp
         lea     sp,TASK_KSTKTOP[bx] ! switch to kernel stack ptr
+        lea     bp,TASK_USER_SP[bx]
 	inc	ch		! Switch allowable
+        j       updct
+!
+!	Bios etc - switch to interrupt stack
+!
+btask:
+	mov	sp,#_intstack
 !
 !	In ktask state we have a suitable stack. It might be 
 !	better to use the intstack..
@@ -437,8 +439,8 @@
 !	leave them in stashed_ss/sp as we could re-enter the
 !	routine on a reschedule.
 ! */
-	push	bp		! push entry SP
 	push	dx		! push entry SS
+	push	bp		! push entry SP
 !
 !	The registers are now stored. Remember where
 !
@@ -446,6 +448,7 @@
 !
 !   Update intr_count
 !
+updct:
         inc     _intr_count
 !
 !       We are on a suitable stack and ch says whether
@@ -513,40 +516,40 @@
 !	mov	bx,_need_resched	! Schedule needed
 !	cmp	bx,#0			! 
 !	je	nosched			! No
-	call	_schedule		! Task switch
 !
 ! This path will return directly to user space
 !
-	pop	ax	! stacked SS
-	pop	cx	! stacked SP
-#ifdef CONFIG_ADVANCED_MM
+	call	_schedule		! Task switch
         mov     bx,_current
-	mov ax, TASK_USER_SS[bx] ! user ds
-	mov bp, sp		
-;	mov 12[bp], ax	! change the es in the stack
-;	mov 14[bp], ax	! change the ds in the stack
-#endif
+        mov     8[bx],#1
+        call    _sig_check              ! Check signals
 !
 !	At this point, the kernel stack is empty. Thus, there is no
 !       need to save the kernel stack pointer.
 !
+        mov     bx,_current
+        mov     sp,TASK_USER_SP[bx]
+#ifndef CONFIG_ADVANCED_MM
+        mov     ss,TASK_USER_SS[bx]
+#else
+	mov ax, TASK_USER_SS[bx] ! user ds
+	mov bp, sp
+        mov ss, ax
+	mov 12[bp], ax	! change the es in the stack
+	mov 14[bp], ax	! change the ds in the stack
+#endif
 	j	noschedpop
-	
-nosched:
 !
 !	Now we have to rescue our stack pointer/segment.
 !
-	pop	ax	! SS
+nosched:
 	pop	cx	! SP
-!
-!	Switch stacks to the interrupting stack
-!
-noschedpop:
-	mov	ss,ax
+	pop	ss	! SS
 	mov	sp,cx
 !
 !	Restore registers and return
 !
+noschedpop:
 	pop	bp
 	pop 	di
 	pop	si
diff -Nurb elks.orig/arch/i86/kernel/process.c elks/arch/i86/kernel/process.c
--- elks.orig/arch/i86/kernel/process.c	2012-06-20 13:45:28.000000000 -0500
+++ elks/arch/i86/kernel/process.c	2012-06-26 14:16:09.000000000 -0500
@@ -50,8 +50,6 @@
 
 #else
 
-#define stashed_ds	cseg_stashed_ds
-
 #ifndef S_SPLINT_S
 #asm
 
@@ -63,7 +61,7 @@
  *  linker doesnt store them in block.
  */
 
-	.extern	cseg_stashed_ds
+    .extern stashed_ds
 
 /* and now code */
 
@@ -79,11 +77,8 @@
 
 void sig_check(void)
 {
-    register __ptask currentp = current;
-
-    if (currentp->signal)
-	(void) do_signal();
-    currentp->signal = 0;
+    if (current->signal)
+        do_signal();
 }
 
 #ifndef S_SPLINT_S
@@ -189,6 +184,8 @@
 	pop	ax
 	call	_syscall
 	push	ax
+        mov     bx,_current
+        mov     8[bx],#0
 	call	_sig_check
 	pop	ax
 	pop	bx
@@ -310,14 +307,7 @@
      */
     {
 #asm
-	cli
-	mov bx, _current
-	mov sp, TASK_USER_SP[bx] ! user stack offset
-	mov ax, TASK_USER_SS[bx] ! user stack segment
-	mov ss, ax
-	mov ds, ax
-	mov es, ax
-	iret			! reloads flags = >reenables interrupts
+        br  _ret_from_syscall
 #endasm
     }
 #endif
@@ -411,16 +401,23 @@
 void arch_setup_sighandler_stack(register struct task_struct *t,
 				 __sighandler_t addr,unsigned signr)
 {
-    debug4("Stack %x was %x %x %x\n", addr, get_ustack(t,0), get_ustack(t,2),
-	   get_ustack(t,4));
-    put_ustack(t, 2, (int) get_ustack(t,0));
-    put_ustack(t, 0, (int) get_ustack(t,4));
-    put_ustack(t, 4, (int) signr);
-    put_ustack(t, -2, (int)t->t_regs.cs);
-    put_ustack(t, -4, (int) addr);
+    register char *i;
+
+    i = 0;
+    if(t->t_regs.bx != 0) {
+        for(; (int)i < 18; i += 2)
+            put_ustack(t, (int)i-4, (int)get_ustack(t,(int)i));
+    }
+    debug4("Stack %x was %x %x %x\n", addr, get_ustack(t,(int)i), get_ustack(t,(int)i+2),
+	   get_ustack(t,(int)i+4));
+    put_ustack(t, (int)i-4, (int)addr);
+    put_ustack(t, (int)i-2, (int)get_ustack(t,(int)i+2));
+    put_ustack(t, (int)i+2, (int)get_ustack(t,(int)i));
+    put_ustack(t, (int)i, (int)get_ustack(t,(int)i+4));
+    put_ustack(t, (int)i+4, (int)signr);
     t->t_regs.sp -= 4;
-    debug5("Stack is %x %x %x %x %x\n", get_ustack(t,0), get_ustack(t,2),
-	   get_ustack(t,4),get_ustack(t,6), get_ustack(t,8));
+    debug5("Stack is %x %x %x %x %x\n", get_ustack(t,(int)i), get_ustack(t,(int)i+2),
+	   get_ustack(t,(int)i+4),get_ustack(t,(int)i+6), get_ustack(t,(int)i+8));
 }
 
 /*
diff -Nurb elks.orig/arch/i86/kernel/signal.c elks/arch/i86/kernel/signal.c
--- elks.orig/arch/i86/kernel/signal.c	2012-05-11 13:26:27.000000000 -0500
+++ elks/arch/i86/kernel/signal.c	2012-06-25 13:06:36.000000000 -0500
@@ -24,10 +24,9 @@
     register struct sigaction *sa;
     unsigned signr;
 
-    while (currentp->signal) {
+    while ((currentp->signal &= ((1 << NSIG) - 1))) {
 	signr = find_first_non_zero_bit(&currentp->signal, NSIG);
-	if (signr == NSIG)
-	    panic("No signal set!\n");
+        clear_bit(signr, &currentp->signal);
 
 	debug2("Process %d has signal %d.\n", currentp->pid, signr);
 	sa = &currentp->sig.action[signr];
@@ -81,6 +80,7 @@
 	arch_setup_sighandler_stack(current, sa->sa_handler, signr);
 	debug1("Stack at %x\n", current->t_regs.sp);
 	sa->sa_handler = SIG_DFL;
+        currentp->signal = 0;
 
 	return 1;
     }
diff -Nurb elks.orig/arch/i86/sibo/irqtab.c elks/arch/i86/sibo/irqtab.c
--- elks.orig/arch/i86/sibo/irqtab.c	2012-06-20 13:45:28.000000000 -0500
+++ elks/arch/i86/sibo/irqtab.c	2012-06-26 14:19:40.000000000 -0500
@@ -19,19 +19,17 @@
 
    #define SEG_IRQ_DATA es
    #define stashed_ds       [0]
-   /*      _our_ds          [18]   bios16.c  */
 
 #else
  #define SEG_IRQ_DATA cs
-   #define stashed_ds       cseg_stashed_ds
 
 #ifndef S_SPLINT_S
 #asm
-	.globl	cseg_stashed_ds
+        .globl	stashed_ds
 
         .even
 
-cseg_stashed_ds:
+stashed_ds:
 	.word	0
 
 #endasm
@@ -39,6 +37,8 @@
 
 #endif
 
+extern void sig_check(void);
+
 void irqtab_init(void)
 {
 #ifndef S_SPLINT_S
@@ -412,20 +412,22 @@
         mov     ss,bx           ! /* Set SS: right */
 	mov	bx,_current
 	cmp	dx,TASK_USER_SS[bx] ! entry ss = current->t_regs.ss?
-	je	utask		! Switch to kernel
-!
-!	Bios etc - switch to interrupt stack
-!
-	mov	sp,#_intstack
-        j       ktask
+	jne	btask		! Switch to interrupt stack
 !
 !	User task. Extract kernel SP. (BX already holds current)
 !	At this point, the kernel stack is empty. Thus, we can load
 !       the kernel stack pointer without accesing memory
 !
-utask:
+        mov     TASK_USER_SP[bx],sp
         lea     sp,TASK_KSTKTOP[bx] ! switch to kernel stack ptr
+        lea     bp,TASK_USER_SP[bx]
 	inc	ch		! Switch allowable
+        j       updct
+!
+!	Bios etc - switch to interrupt stack
+!
+btask:
+	mov	sp,#_intstack
 !
 !	In ktask state we have a suitable stack. It might be 
 !	better to use the intstack..
@@ -436,8 +438,8 @@
 !	leave them in stashed_ss/sp as we could re-enter the
 !	routine on a reschedule.
 ! */
-	push	bp		! push entry SP
 	push	dx		! push entry SS
+	push	bp		! push entry SP
 !
 !	The registers are now stored. Remember where
 !
@@ -445,6 +447,7 @@
 !
 !   Update intr_count
 !
+updct:
         inc     _intr_count
 !
 !       We are on a suitable stack and ch says whether
@@ -483,40 +486,40 @@
 !	mov	bx,_need_resched	! Schedule needed
 !	cmp	bx,#0			! 
 !	je	nosched			! No
-	call	_schedule		! Task switch
 !
 ! This path will return directly to user space
 !
-	pop	ax	! stacked SS
-	pop	cx	! stacked SP
-#ifdef CONFIG_ADVANCED_MM
+	call	_schedule		! Task switch
         mov     bx,_current
-	mov ax, TASK_USER_SS[bx] ! user ds
-	mov bp, sp
-;	mov 12[bp], ax	! change the es in the stack
-;	mov 14[bp], ax	! change the ds in the stack
-#endif
+        mov     8[bx],#1
+        call    _sig_check              ! Check signals
 !
 !	At this point, the kernel stack is empty. Thus, there in no
 !       need to save the kernel stack pointer.
 !
+        mov     bx,_current
+        mov     sp,TASK_USER_SP[bx]
+#ifndef CONFIG_ADVANCED_MM
+        mov     ss,TASK_USER_SS[bx]
+#else
+	mov ax, TASK_USER_SS[bx] ! user ds
+	mov bp, sp
+        mov ss, ax
+	mov 12[bp], ax	! change the es in the stack
+	mov 14[bp], ax	! change the ds in the stack
+#endif
 	j	noschedpop
-	
-nosched:
 !
 !	Now we have to rescue our stack pointer/segment.
 !
-	pop	ax	! SS
+nosched:
 	pop	cx	! SP
-!
-!	Switch stacks to the interrupting stack
-!
-noschedpop:
-	mov	ss,ax
+	pop	ss	! SS
 	mov	sp,cx
 !
 !	Restore registers and return
 !
+noschedpop:
 	pop	bp
 	pop 	di
 	pop	si
diff -Nurb elks.orig/kernel/sched.c elks/kernel/sched.c
--- elks.orig/kernel/sched.c	2012-06-20 13:45:28.000000000 -0500
+++ elks/kernel/sched.c	2012-06-25 13:06:36.000000000 -0500
@@ -24,8 +24,6 @@
 
 extern int intr_count;
 
-extern int do_signal(void);
-
 static void run_timer_list();
 
 void add_to_runqueue(register struct task_struct *p)
@@ -150,11 +148,6 @@
 
         tswitch();  /* Won't return for a new task */
 
-        if(current->signal){
-            do_signal();
-            current->signal = 0;
-        }
-                
         if (timeout) {
             del_timer(&timer);
         }

[Index of Archives]     [Kernel]     [Linux ia64]     [DCCP]     [Linux for ARM]     [Yosemite News]     [Linux SCSI]     [Linux Hams]

  Powered by Linux