Fix a long-standing bug inherited from FreeBSD. It is possible for a
authorMatthew Dillon <dillon@dragonflybsd.org>
Tue, 10 Oct 2006 15:40:47 +0000 (15:40 +0000)
committerMatthew Dillon <dillon@dragonflybsd.org>
Tue, 10 Oct 2006 15:40:47 +0000 (15:40 +0000)
signal sent to a process group to race a fork().  If the signal is received
by the process doing the fork() before it finishes hooking up the new child
to the process group the new child will not receive the signal.  The result
is that ^C does not always kill all the processes in the process group.

The problem occurs on UP systems if fork1() in the kernel blocks prior to
finishing the process setup.  Such blocking can occur in MALLOC.  The problem
can also occur if an interrupt occurs just as the process issues the fork()
system call, or on SMP systems where the signal is sent from a different cpu.

The solution is to use a lockmgr() lock to interlock the pgrp structure when
a signal is being sent to a process group and to have fork() check for
signal pre-conditions and return ERESTART if such conditions exist to force
processing of the pending signals.

NOTE!  BMAKE, MAKE, GNUMAKE ALSO HAVE UNRELATED SIGNALING BUGS.  These
programs improperly install a SIG_IGN for a few microseconds in order to
test the current signal function for SIGINT and various other signals.
If a ^C is sent while *MAKE is in this window, the *MAKE will ignore the
signal.

Reviewed-by: "Simon 'corecode' Schubert" <corecode@fs.ei.tum.de>
sys/kern/kern_fork.c
sys/kern/kern_proc.c
sys/kern/kern_sig.c
sys/sys/proc.h
sys/sys/signalvar.h
sys/sys/unistd.h

index f8e4c03..537978a 100644 (file)
@@ -37,7 +37,7 @@
  *
  *     @(#)kern_fork.c 8.6 (Berkeley) 4/8/94
  * $FreeBSD: src/sys/kern/kern_fork.c,v 1.72.2.14 2003/06/26 04:15:10 silby Exp $
- * $DragonFly: src/sys/kern/kern_fork.c,v 1.57 2006/09/19 11:47:35 corecode Exp $
+ * $DragonFly: src/sys/kern/kern_fork.c,v 1.58 2006/10/10 15:40:46 dillon Exp $
  */
 
 #include "opt_ktrace.h"
@@ -93,7 +93,7 @@ sys_fork(struct fork_args *uap)
        struct proc *p2;
        int error;
 
-       error = fork1(lp, RFFDG | RFPROC, &p2);
+       error = fork1(lp, RFFDG | RFPROC | RFPGLOCK, &p2);
        if (error == 0) {
                start_forked_proc(lp, p2);
                uap->sysmsg_fds[0] = p2->p_pid;
@@ -110,7 +110,7 @@ sys_vfork(struct vfork_args *uap)
        struct proc *p2;
        int error;
 
-       error = fork1(lp, RFFDG | RFPROC | RFPPWAIT | RFMEM, &p2);
+       error = fork1(lp, RFFDG | RFPROC | RFPPWAIT | RFMEM | RFPGLOCK, &p2);
        if (error == 0) {
                start_forked_proc(lp, p2);
                uap->sysmsg_fds[0] = p2->p_pid;
@@ -140,7 +140,7 @@ sys_rfork(struct rfork_args *uap)
        if ((uap->flags & RFKERNELONLY) != 0)
                return (EINVAL);
 
-       error = fork1(lp, uap->flags, &p2);
+       error = fork1(lp, uap->flags | RFPGLOCK, &p2);
        if (error == 0) {
                if (p2)
                        start_forked_proc(lp, p2);
@@ -158,9 +158,10 @@ fork1(struct lwp *lp1, int flags, struct proc **procp)
 {
        struct proc *p1 = lp1->lwp_proc;
        struct proc *p2, *pptr;
+       struct pgrp *pgrp;
        struct lwp *lp2;
        uid_t uid;
-       int ok;
+       int ok, error;
        static int curfail = 0;
        static struct timeval lastfail;
        struct forklist *ep;
@@ -202,6 +203,26 @@ fork1(struct lwp *lp1, int flags, struct proc **procp)
                return (0);
        }
 
+       /*
+        * Interlock against process group signal delivery.  If signals
+        * are pending after the interlock is obtained we have to restart
+        * the system call to process the signals.  If we don't the child
+        * can miss a pgsignal (such as ^C) sent during the fork.
+        *
+        * We can't use CURSIG() here because it will process any STOPs
+        * and cause the process group lock to be held indefinitely.  If
+        * a STOP occurs, the fork will be restarted after the CONT.
+        */
+       error = 0;
+       pgrp = NULL;
+       if ((flags & RFPGLOCK) && (pgrp = p1->p_pgrp) != NULL) {
+               lockmgr(&pgrp->pg_lock, LK_SHARED);
+               if (CURSIGNB(p1)) {
+                       error = ERESTART;
+                       goto done;
+               }
+       }
+
        /*
         * Although process entries are dynamically created, we still keep
         * a global limit on the maximum number we will create.  Don't allow
@@ -215,7 +236,8 @@ fork1(struct lwp *lp1, int flags, struct proc **procp)
                        printf("maxproc limit exceeded by uid %d, please "
                               "see tuning(7) and login.conf(5).\n", uid);
                tsleep(&forksleep, 0, "fork", hz / 2);
-               return (EAGAIN);
+               error = EAGAIN;
+               goto done;
        }
        /*
         * Increment the nprocs resource before blocking can occur.  There
@@ -238,7 +260,8 @@ fork1(struct lwp *lp1, int flags, struct proc **procp)
                        printf("maxproc limit exceeded by uid %d, please "
                               "see tuning(7) and login.conf(5).\n", uid);
                tsleep(&forksleep, 0, "fork", hz / 2);
-               return (EAGAIN);
+               error = EAGAIN;
+               goto done;
        }
 
        /* Allocate new proc. */
@@ -506,7 +529,10 @@ fork1(struct lwp *lp1, int flags, struct proc **procp)
         * Return child proc pointer to parent.
         */
        *procp = p2;
-       return (0);
+done:
+       if (pgrp)
+               lockmgr(&pgrp->pg_lock, LK_RELEASE);
+       return (error);
 }
 
 /*
index 56b6a05..912f939 100644 (file)
@@ -32,7 +32,7 @@
  *
  *     @(#)kern_proc.c 8.7 (Berkeley) 2/14/95
  * $FreeBSD: src/sys/kern/kern_proc.c,v 1.63.2.9 2003/05/08 07:47:16 kbyanc Exp $
- * $DragonFly: src/sys/kern/kern_proc.c,v 1.28 2006/09/05 00:55:45 dillon Exp $
+ * $DragonFly: src/sys/kern/kern_proc.c,v 1.29 2006/10/10 15:40:46 dillon Exp $
  */
 
 #include <sys/param.h>
@@ -228,6 +228,7 @@ enterpgrp(struct proc *p, pid_t pgid, int mksess)
                LIST_INSERT_HEAD(PGRPHASH(pgid), pgrp, pg_hash);
                pgrp->pg_jobc = 0;
                SLIST_INIT(&pgrp->pg_sigiolst);
+               lockinit(&pgrp->pg_lock, "pgwt", 0, 0);
        } else if (pgrp == p->p_pgrp)
                return (0);
 
index 5936bc8..c90dec8 100644 (file)
@@ -37,7 +37,7 @@
  *
  *     @(#)kern_sig.c  8.7 (Berkeley) 4/18/94
  * $FreeBSD: src/sys/kern/kern_sig.c,v 1.72.2.17 2003/05/16 16:34:34 obrien Exp $
- * $DragonFly: src/sys/kern/kern_sig.c,v 1.54 2006/09/19 11:47:35 corecode Exp $
+ * $DragonFly: src/sys/kern/kern_sig.c,v 1.55 2006/10/10 15:40:46 dillon Exp $
  */
 
 #include "opt_ktrace.h"
@@ -56,6 +56,7 @@
 #include <sys/systm.h>
 #include <sys/acct.h>
 #include <sys/fcntl.h>
+#include <sys/lock.h>
 #include <sys/wait.h>
 #include <sys/ktrace.h>
 #include <sys/syslog.h>
@@ -630,6 +631,7 @@ killpg(int sig, int pgid, int all)
                        if (pgrp == NULL)
                                return (ESRCH);
                }
+               lockmgr(&pgrp->pg_lock, LK_EXCLUSIVE);
                LIST_FOREACH(p, &pgrp->pg_members, p_pglist) {
                        if (p->p_pid <= 1 || 
                            (p->p_flag & (P_SYSTEM | P_ZOMBIE)) ||
@@ -640,6 +642,7 @@ killpg(int sig, int pgid, int all)
                        if (sig)
                                ksignal(p, sig);
                }
+               lockmgr(&pgrp->pg_lock, LK_RELEASE);
        }
        return (info.nfound ? 0 : ESRCH);
 }
@@ -713,16 +716,23 @@ gsignal(int pgid, int sig)
 /*
  * Send a signal to a process group.  If checktty is 1,
  * limit to members which have a controlling terminal.
+ *
+ * pg_lock interlocks against a fork that might be in progress, to
+ * ensure that the new child process picks up the signal.
  */
 void
 pgsignal(struct pgrp *pgrp, int sig, int checkctty)
 {
        struct proc *p;
 
-       if (pgrp)
-               LIST_FOREACH(p, &pgrp->pg_members, p_pglist)
+       if (pgrp) {
+               lockmgr(&pgrp->pg_lock, LK_EXCLUSIVE);
+               LIST_FOREACH(p, &pgrp->pg_members, p_pglist) {
                        if (checkctty == 0 || p->p_flag & P_CONTROLT)
                                ksignal(p, sig);
+               }
+               lockmgr(&pgrp->pg_lock, LK_RELEASE);
+       }
 }
 
 /*
@@ -1751,10 +1761,13 @@ pgsigio(struct sigio *sigio, int sig, int checkctty)
        } else if (sigio->sio_pgid < 0) {
                struct proc *p;
 
-               LIST_FOREACH(p, &sigio->sio_pgrp->pg_members, p_pglist)
+               lockmgr(&sigio->sio_pgrp->pg_lock, LK_EXCLUSIVE);
+               LIST_FOREACH(p, &sigio->sio_pgrp->pg_members, p_pglist) {
                        if (CANSIGIO(sigio->sio_ruid, sigio->sio_ucred, p) &&
                            (checkctty == 0 || (p->p_flag & P_CONTROLT)))
                                ksignal(p, sig);
+               }
+               lockmgr(&sigio->sio_pgrp->pg_lock, LK_RELEASE);
        }
 }
 
index ca78c61..268ccf6 100644 (file)
@@ -37,7 +37,7 @@
  *
  *     @(#)proc.h      8.15 (Berkeley) 5/19/95
  * $FreeBSD: src/sys/sys/proc.h,v 1.99.2.9 2003/06/06 20:21:32 tegge Exp $
- * $DragonFly: src/sys/sys/proc.h,v 1.86 2006/09/30 22:14:31 swildner Exp $
+ * $DragonFly: src/sys/sys/proc.h,v 1.87 2006/10/10 15:40:47 dillon Exp $
  */
 
 #ifndef _SYS_PROC_H_
@@ -54,6 +54,7 @@
 #include <sys/queue.h>
 #include <sys/rtprio.h>                        /* For struct rtprio. */
 #include <sys/signal.h>
+#include <sys/lock.h>
 #ifndef _KERNEL
 #include <sys/time.h>                  /* For structs itimerval, timeval. */
 #endif
@@ -92,6 +93,7 @@ struct        pgrp {
        struct  sigiolst pg_sigiolst;   /* List of sigio sources. */
        pid_t   pg_id;                  /* Pgrp id. */
        int     pg_jobc;        /* # procs qualifying pgrp for job control */
+       struct lock pg_lock;            /* Lock during fork */
 };
 
 struct procsig {
index c29be3b..785491f 100644 (file)
@@ -32,7 +32,7 @@
  *
  *     @(#)signalvar.h 8.6 (Berkeley) 2/19/95
  * $FreeBSD: src/sys/sys/signalvar.h,v 1.34.2.1 2000/05/16 06:58:05 dillon Exp $
- * $DragonFly: src/sys/sys/signalvar.h,v 1.14 2006/09/03 18:29:17 dillon Exp $
+ * $DragonFly: src/sys/sys/signalvar.h,v 1.15 2006/10/10 15:40:47 dillon Exp $
  */
 
 #ifndef        _SYS_SIGNALVAR_H_               /* tmp for user.h */
@@ -212,6 +212,7 @@ int checkpoint_signal_handler(struct proc *p);
  * Inline functions:
  */
 #define        CURSIG(p)       __cursig(p)
+#define CURSIGNB(p)    __cursignb(p)
 
 /*
  * Determine signal that should be delivered to process p, the current
@@ -220,7 +221,9 @@ int checkpoint_signal_handler(struct proc *p);
  *
  * MP SAFE
  */
-static __inline int __cursig(struct proc *p)
+static __inline
+int
+__cursig(struct proc *p)
 {
        sigset_t tmpset;
        int r;
@@ -235,6 +238,21 @@ static __inline int __cursig(struct proc *p)
        return(r);
 }
 
+static __inline
+int
+__cursignb(struct proc *p)
+{
+       sigset_t tmpset;
+
+       tmpset = p->p_siglist;
+       SIGSETNAND(tmpset, p->p_sigmask);
+       if (SIGISEMPTY(p->p_siglist) ||
+            (!(p->p_flag & P_TRACED) && SIGISEMPTY(tmpset))) {
+               return(FALSE);
+       }
+       return (TRUE);
+}
+
 #endif /* _KERNEL */
 
 #endif /* !_SYS_SIGNALVAR_H_ */
index 8ee1421..e00a9c7 100644 (file)
@@ -32,7 +32,7 @@
  *
  *     @(#)unistd.h    8.2 (Berkeley) 1/7/94
  * $FreeBSD: src/sys/sys/unistd.h,v 1.22.2.2 2000/08/22 01:46:30 jhb Exp $
- * $DragonFly: src/sys/sys/unistd.h,v 1.5 2005/12/03 02:19:52 joerg Exp $
+ * $DragonFly: src/sys/sys/unistd.h,v 1.6 2006/10/10 15:40:47 dillon Exp $
  */
 
 #ifndef _SYS_UNISTD_H_
 #define RFTHREAD       (1<<13) /* enable kernel thread support */
 #define RFSIGSHARE     (1<<14) /* share signal handlers */
 #define RFLINUXTHPN     (1<<16) /* do linux clone exit parent notification */
+#define RFPGLOCK       (1<<30) /* process group interlock for signal race */
 #define RFPPWAIT       (1<<31) /* parent sleeps until child exits (vfork) */
-#define RFKERNELONLY   RFPPWAIT
+#define RFKERNELONLY   (RFPPWAIT|RFPGLOCK)
 
 
 #endif /* !_POSIX_SOURCE */