From 167e6ecba2a139576866996945521464379d3cc3 Mon Sep 17 00:00:00 2001 From: Matthew Dillon Date: Tue, 10 Oct 2006 15:40:47 +0000 Subject: [PATCH] Fix a long-standing bug inherited from FreeBSD. It is possible for a 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" --- sys/kern/kern_fork.c | 42 ++++++++++++++++++++++++++++++++++-------- sys/kern/kern_proc.c | 3 ++- sys/kern/kern_sig.c | 21 +++++++++++++++++---- sys/sys/proc.h | 4 +++- sys/sys/signalvar.h | 22 ++++++++++++++++++++-- sys/sys/unistd.h | 5 +++-- 6 files changed, 79 insertions(+), 18 deletions(-) diff --git a/sys/kern/kern_fork.c b/sys/kern/kern_fork.c index f8e4c03cd6..537978abec 100644 --- a/sys/kern/kern_fork.c +++ b/sys/kern/kern_fork.c @@ -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); } /* diff --git a/sys/kern/kern_proc.c b/sys/kern/kern_proc.c index 56b6a05000..912f939b8d 100644 --- a/sys/kern/kern_proc.c +++ b/sys/kern/kern_proc.c @@ -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 @@ -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); diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c index 5936bc8b44..c90dec899c 100644 --- a/sys/kern/kern_sig.c +++ b/sys/kern/kern_sig.c @@ -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 #include #include +#include #include #include #include @@ -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); } } diff --git a/sys/sys/proc.h b/sys/sys/proc.h index ca78c617f8..268ccf6afb 100644 --- a/sys/sys/proc.h +++ b/sys/sys/proc.h @@ -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 #include /* For struct rtprio. */ #include +#include #ifndef _KERNEL #include /* 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 { diff --git a/sys/sys/signalvar.h b/sys/sys/signalvar.h index c29be3b2b4..785491f884 100644 --- a/sys/sys/signalvar.h +++ b/sys/sys/signalvar.h @@ -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_ */ diff --git a/sys/sys/unistd.h b/sys/sys/unistd.h index 8ee142145e..e00a9c7fc9 100644 --- a/sys/sys/unistd.h +++ b/sys/sys/unistd.h @@ -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_ @@ -219,8 +219,9 @@ #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 */ -- 2.41.0