- Add a new global mutex 'ppeers_lock' to protect the p_peers list of

  processes forked with RFTHREAD.
- Use a goto to a label for common code when exiting from fork1() in case
  of an error.
- Move the RFTHREAD linkage setup code later in fork since the ppeers_lock
  cannot be locked while holding a proc lock.  Handle the race of a task
  leader exiting and killing its peers while a peer is forking a new child.
  In that case, go ahead and let the peer process proceed normally as the
  parent is about to kill it.  However, the task leader may have already
  gone to sleep to wait for the peers to die, so the new child process may
  not receive a SIGKILL from the task leader.  Rather than try to destruct
  the new child process, just go ahead and send it a SIGKILL directly and
  add it to the p_peers list.  This ensures that the task leader will wait
  until both the peer process doing the fork() and the new child process
  have received their KILL signals and exited.

Discussed with:	truckman (earlier versions)

4 files changed, 61 insertions, 45 deletions

diff --git a/sys/kern/kern_exit.c b/sys/kern/kern_exit.c
index 6c83432..68bebfc 100644
--- a/sys/kern/kern_exit.c
+++ b/sys/kern/kern_exit.c
@@ -207,8 +207,8 @@ exit1(td, rv)
 	PROC_UNLOCK(p);
 
 	/* Are we a task leader? */
-	PROC_LOCK(p);
 	if (p == p->p_leader) {
+		mtx_lock(&ppeers_lock);
 		q = p->p_peers;
 		while (q != NULL) {
 			PROC_LOCK(q);
@@ -216,10 +216,10 @@ exit1(td, rv)
 			PROC_UNLOCK(q);
 			q = q->p_peers;
 		}
-		while (p->p_peers) 
-			msleep(p, &p->p_mtx, PWAIT, "exit1", 0);
+		while (p->p_peers != NULL) 
+			msleep(p, &ppeers_lock, PWAIT, "exit1", 0);
+		mtx_unlock(&ppeers_lock);
 	}
-	PROC_UNLOCK(p);
 
 #ifdef PGINPROF
 	vmsizmon();
@@ -265,7 +265,7 @@ exit1(td, rv)
 	/*
 	 * Remove ourself from our leader's peer list and wake our leader.
 	 */
-	PROC_LOCK(p->p_leader);
+	mtx_lock(&ppeers_lock);
 	if (p->p_leader->p_peers) {
 		q = p->p_leader;
 		while (q->p_peers != p)
@@ -273,7 +273,7 @@ exit1(td, rv)
 		q->p_peers = p->p_peers;
 		wakeup(p->p_leader);
 	}
-	PROC_UNLOCK(p->p_leader);
+	mtx_unlock(&ppeers_lock);
 
 	/* The next two chunks should probably be moved to vmspace_exit. */
 	vm = p->p_vmspace;
diff --git a/sys/kern/kern_fork.c b/sys/kern/kern_fork.c
index 9c2169a..1efb908 100644
--- a/sys/kern/kern_fork.c
+++ b/sys/kern/kern_fork.c
@@ -236,6 +236,7 @@ fork1(td, flags, pages, procp)
 	struct ksegrp *kg2;
 	struct sigacts *newsigacts;
 	struct procsig *newprocsig;
+	int error;
 
 	GIANT_REQUIRED;
 
@@ -319,16 +320,10 @@ fork1(td, flags, pages, procp)
 	sx_xlock(&allproc_lock);
 	uid = td->td_ucred->cr_ruid;
 	if ((nprocs >= maxproc - 10 && uid != 0) || nprocs >= maxproc) {
-		sx_xunlock(&allproc_lock);
-		uma_zfree(proc_zone, newproc);
-		if (p1->p_flag & P_KSES) {
-			PROC_LOCK(p1);
-			thread_single_end();
-			PROC_UNLOCK(p1);
-		}
-		tsleep(&forksleep, PUSER, "fork", hz / 2);
-		return (EAGAIN);
+		error = EAGAIN;
+		goto fail;
 	}
+
 	/*
 	 * Increment the count of procs running with this uid. Don't allow
 	 * a nonprivileged user to exceed their current limit.
@@ -338,15 +333,8 @@ fork1(td, flags, pages, procp)
 		(uid != 0) ? p1->p_rlimit[RLIMIT_NPROC].rlim_cur : 0);
 	PROC_UNLOCK(p1);
 	if (!ok) {
-		sx_xunlock(&allproc_lock);
-		uma_zfree(proc_zone, newproc);
-		if (p1->p_flag & P_KSES) {
-			PROC_LOCK(p1);
-			thread_single_end();
-			PROC_UNLOCK(p1);
-		}
-		tsleep(&forksleep, PUSER, "fork", hz / 2);
-		return (EAGAIN);
+		error = EAGAIN;
+		goto fail;
 	}
 
 	/*
@@ -526,26 +514,6 @@ again:
 	p2->p_ucred = crhold(td->td_ucred);
 	td2->td_ucred = crhold(p2->p_ucred);	/* XXXKSE */
 
-	/*
-	 * Setup linkage for kernel based threading
-	 */
-	if((flags & RFTHREAD) != 0) {
-		/*
-		 * XXX: This assumes a leader is a parent or grandparent of
-		 * all processes in a task.
-		 */
-		if (p1->p_leader != p1)
-			PROC_LOCK(p1->p_leader);
-		p2->p_peers = p1->p_peers;
-		p1->p_peers = p2;
-		p2->p_leader = p1->p_leader;
-		if (p1->p_leader != p1)
-			PROC_UNLOCK(p1->p_leader);
-	} else {
-		p2->p_peers = NULL;
-		p2->p_leader = p2;
-	}
-
 	pargs_hold(p2->p_args);
 
 	if (flags & RFSIGSHARE) {
@@ -595,6 +563,40 @@ again:
 		p2->p_limit->p_refcnt++;
 	}
 
+	/*
+	 * Setup linkage for kernel based threading
+	 */
+	if((flags & RFTHREAD) != 0) {
+		mtx_lock(&ppeers_lock);
+		p2->p_peers = p1->p_peers;
+		p1->p_peers = p2;
+		p2->p_leader = p1->p_leader;
+		mtx_unlock(&ppeers_lock);
+		PROC_LOCK(p1->p_leader);
+		if ((p1->p_leader->p_flag & P_WEXIT) != 0) {
+			PROC_UNLOCK(p1->p_leader);
+			/*
+			 * The task leader is exiting, so process p1 is
+			 * going to be killed shortly.  Since p1 obviously
+			 * isn't dead yet, we know that the leader is either
+			 * sending SIGKILL's to all the processes in this
+			 * task or is sleeping waiting for all the peers to
+			 * exit.  We let p1 complete the fork, but we need
+			 * to go ahead and kill the new process p2 since
+			 * the task leader may not get a chance to send
+			 * SIGKILL to it.  We leave it on the list so that
+			 * the task leader will wait for this new process
+			 * to commit suicide.
+			 */
+			PROC_LOCK(p2);
+			psignal(p2, SIGKILL);
+			PROC_UNLOCK(p2);
+		}
+	} else {
+		p2->p_peers = NULL;
+		p2->p_leader = p2;
+	}
+
 	sx_xlock(&proctree_lock);
 	PGRP_LOCK(p1->p_pgrp);
 	PROC_LOCK(p2);
@@ -752,6 +754,16 @@ again:
 	 */
 	*procp = p2;
 	return (0);
+fail:
+	sx_xunlock(&allproc_lock);
+	uma_zfree(proc_zone, newproc);
+	if (p1->p_flag & P_KSES) {
+		PROC_LOCK(p1);
+		thread_single_end();
+		PROC_UNLOCK(p1);
+	}
+	tsleep(&forksleep, PUSER, "fork", hz / 2);
+	return (error);
 }
 
 /*
diff --git a/sys/kern/kern_proc.c b/sys/kern/kern_proc.c
index 82138a5..345ee22 100644
--- a/sys/kern/kern_proc.c
+++ b/sys/kern/kern_proc.c
@@ -96,6 +96,7 @@ struct proclist zombproc;
 struct sx allproc_lock;
 struct sx proctree_lock;
 struct mtx pargs_ref_lock;
+struct mtx ppeers_lock;
 uma_zone_t proc_zone;
 uma_zone_t ithread_zone;
 
@@ -118,6 +119,7 @@ procinit()
 	sx_init(&allproc_lock, "allproc");
 	sx_init(&proctree_lock, "proctree");
 	mtx_init(&pargs_ref_lock, "struct pargs.ref", NULL, MTX_DEF);
+	mtx_init(&ppeers_lock, "p_peers", NULL, MTX_DEF);
 	LIST_INIT(&allproc);
 	LIST_INIT(&zombproc);
 	pidhashtbl = hashinit(maxproc / 4, M_PROC, &pidhash);
diff --git a/sys/sys/proc.h b/sys/sys/proc.h
index 14e9f4e..9b7cb59 100644
--- a/sys/sys/proc.h
+++ b/sys/sys/proc.h
@@ -150,6 +150,7 @@ struct pargs {
  *      n - not locked, lazy
  *      o - ktrace lock
  *      p - select lock (sellock)
+ *      r - p_peers lock
  *
  * If the locking key specifies two identifiers (for example, p_pptr) then
  * either lock is sufficient for read access, but both locks must be held
@@ -585,7 +586,7 @@ struct proc {
 	struct user	*p_uarea;	/* (k) Kernel VA of u-area (CPU) */
 	u_short		p_acflag;	/* (c) Accounting flags. */
 	struct rusage	*p_ru;		/* (a) Exit information. XXX */
-	struct proc	*p_peers;	/* (c) */
+	struct proc	*p_peers;	/* (r) */
 	struct proc	*p_leader;	/* (b) */
 	void		*p_emuldata;	/* (c) Emulator state data. */
 };
@@ -804,6 +805,7 @@ extern u_long pgrphash;
 extern struct sx allproc_lock;
 extern struct sx proctree_lock;
 extern struct mtx pargs_ref_lock;
+extern struct mtx ppeers_lock;
 extern struct proc proc0;		/* Process slot for swapper. */
 extern struct thread thread0;		/* Primary thread in proc0 */
 extern struct ksegrp ksegrp0;		/* Primary ksegrp in proc0 */