Fix several issues with process group orphanage.

Attempt of adding assertions that pgrp->pg_jobc counters do not
underflow in r361967, reverted in r362910, points out bugs in the
handling of job control.  Peter Holm was able to narrow down the
problem to very easy reproduction with timeout(1) which uses reaping.

The following list of problems with calculation of pg_jobs which
directs SIGHUP/SIGCONT delivery for orphaned process group was
identified:
- Re-calculation of the orphaned status for children of exiting parent
  was wrong, but mostly unnoticed when all children were reparented to
  init(8).  When child can be reparented to a different process which
  could affect the child' job control state, it was not properly
  accounted for in pg_jobc.
- Lockless check for exiting process' parent process group is racy
  because nothing prevents the parent from changing its group
  membership.
- Exited process is left in the process group, until waited. This
  affects other calculations of pg_jobc.

Split handling of job control status on process changing its process
group, and process exiting.  Calculate increments and decrements for
pg_jobs by exact checking the orphanage instead of assuming process
group membership for children and parent.  Move the call to killjobc()
later under the proctree_lock.  Mark exiting process in killjobc()
with a new flag P_TREE_GRPEXITED and skip it for all pg_jobc
calculations after the flag is set.

Add checker that independently recalculates pg_jobc value and compares
it with the memoized process group state. This is enabled under INVARIANTS.

Reviewed by:	jilles
Discussed with:	kevans
Tested by:	pho
Sponsored by:	The FreeBSD Foundation
MFC after:	2 weeks
Differential revision:	https://reviews.freebsd.org/D26116

sys/kern/kern_exit.c

@@ -391,7 +391,6 @@ exit1(struct thread *td, int rval, int signo)
 	}
 
 	vmspace_exit(td);
-	killjobc();
 	(void)acct_process(td);
 
 #ifdef KTRACE
@@ -435,6 +434,12 @@ exit1(struct thread *td, int rval, int signo)
 	p->p_flag &= ~(P_TRACED | P_PPWAIT | P_PPTRACE);
 	PROC_UNLOCK(p);
 
+	/*
+	 * killjobc() might drop and re-acquire proctree_lock to
+	 * revoke control tty if exiting process was a session leader.
+	 */
+	killjobc();
+
 	/*
 	 * Reparent all children processes:
 	 * - traced ones to the original parent (or init if we are that parent)

sys/kern/kern_proc.c

@@ -102,13 +102,14 @@ MALLOC_DEFINE(M_SESSION, "session", "session header");
 static MALLOC_DEFINE(M_PROC, "proc", "Proc structures");
 MALLOC_DEFINE(M_SUBPROC, "subproc", "Proc sub-structures");
 
+static void fixjobc_enterpgrp(struct proc *p, struct pgrp *pgrp);
 static void doenterpgrp(struct proc *, struct pgrp *);
 static void orphanpg(struct pgrp *pg);
 static void fill_kinfo_aggregate(struct proc *p, struct kinfo_proc *kp);
 static void fill_kinfo_proc_only(struct proc *p, struct kinfo_proc *kp);
 static void fill_kinfo_thread(struct thread *td, struct kinfo_proc *kp,
     int preferthread);
-static void pgadjustjobc(struct pgrp *pgrp, int entering);
+static void pgadjustjobc(struct pgrp *pgrp, bool entering);
 static void pgdelete(struct pgrp *);
 static int proc_ctor(void *mem, int size, void *arg, int flags);
 static void proc_dtor(void *mem, int size, void *arg);
@@ -632,6 +633,43 @@ enterthispgrp(struct proc *p, struct pgrp *pgrp)
 	return (0);
 }
 
+/*
+ * If true, any child of q which belongs to group pgrp, qualifies the
+ * process group pgrp as not orphaned.
+ */
+static bool
+isjobproc(struct proc *q, struct pgrp *pgrp)
+{
+	sx_assert(&proctree_lock, SX_LOCKED);
+	return (q->p_pgrp != pgrp &&
+	    q->p_pgrp->pg_session == pgrp->pg_session);
+}
+
+#ifdef INVARIANTS
+static void
+check_pgrp_jobc(struct pgrp *pgrp)
+{
+	struct proc *q;
+	int cnt;
+
+	sx_assert(&proctree_lock, SX_LOCKED);
+	PGRP_LOCK_ASSERT(pgrp, MA_NOTOWNED);
+
+	cnt = 0;
+	PGRP_LOCK(pgrp);
+	LIST_FOREACH(q, &pgrp->pg_members, p_pglist) {
+		if ((q->p_treeflag & P_TREE_GRPEXITED) != 0 ||
+		    q->p_pptr == NULL)
+			continue;
+		if (isjobproc(q->p_pptr, pgrp))
+			cnt++;
+	}
+	KASSERT(pgrp->pg_jobc == cnt, ("pgrp %d %p pg_jobc %d cnt %d",
+	    pgrp->pg_id, pgrp, pgrp->pg_jobc, cnt));
+	PGRP_UNLOCK(pgrp);
+}
+#endif
+
 /*
  * Move p to a process group
  */
@@ -648,13 +686,15 @@ doenterpgrp(struct proc *p, struct pgrp *pgrp)
 
 	savepgrp = p->p_pgrp;
 
+#ifdef INVARIANTS
+	check_pgrp_jobc(pgrp);
+	check_pgrp_jobc(savepgrp);
+#endif
+
 	/*
 	 * Adjust eligibility of affected pgrps to participate in job control.
-	 * Increment eligibility counts before decrementing, otherwise we
-	 * could reach 0 spuriously during the first call.
 	 */
-	fixjobc(p, pgrp, 1);
-	fixjobc(p, p->p_pgrp, 0);
+	fixjobc_enterpgrp(p, pgrp);
 
 	PGRP_LOCK(pgrp);
 	PGRP_LOCK(savepgrp);
@@ -728,19 +768,15 @@ pgdelete(struct pgrp *pgrp)
 }
 
 static void
-pgadjustjobc(struct pgrp *pgrp, int entering)
+pgadjustjobc(struct pgrp *pgrp, bool entering)
 {
 
 	PGRP_LOCK(pgrp);
 	if (entering) {
-#ifdef notyet
 		MPASS(pgrp->pg_jobc >= 0);
-#endif
 		pgrp->pg_jobc++;
 	} else {
-#ifdef notyet
 		MPASS(pgrp->pg_jobc > 0);
-#endif
 		--pgrp->pg_jobc;
 		if (pgrp->pg_jobc == 0)
 			orphanpg(pgrp);
@@ -755,43 +791,95 @@ pgadjustjobc(struct pgrp *pgrp, int entering)
  * process group of the same session).  If that count reaches zero, the
  * process group becomes orphaned.  Check both the specified process'
  * process group and that of its children.
- * entering == 0 => p is leaving specified group.
- * entering == 1 => p is entering specified group.
+ * We increment eligibility counts before decrementing, otherwise we
+ * could reach 0 spuriously during the decrement.
  */
-void
-fixjobc(struct proc *p, struct pgrp *pgrp, int entering)
+static void
+fixjobc_enterpgrp(struct proc *p, struct pgrp *pgrp)
 {
-	struct pgrp *hispgrp;
-	struct session *mysession;
 	struct proc *q;
+	struct pgrp *childpgrp;
+	bool future_jobc;
 
 	sx_assert(&proctree_lock, SX_LOCKED);
 	PROC_LOCK_ASSERT(p, MA_NOTOWNED);
 	PGRP_LOCK_ASSERT(pgrp, MA_NOTOWNED);
 	SESS_LOCK_ASSERT(pgrp->pg_session, MA_NOTOWNED);
 
+	if (p->p_pgrp == pgrp)
+		return;
+
+	if (isjobproc(p->p_pptr, pgrp))
+		pgadjustjobc(pgrp, true);
+	LIST_FOREACH(q, &p->p_children, p_sibling) {
+		if ((q->p_treeflag & P_TREE_GRPEXITED) != 0)
+			continue;
+		childpgrp = q->p_pgrp;
+		future_jobc = childpgrp != pgrp &&
+		    childpgrp->pg_session == pgrp->pg_session;
+		if (!isjobproc(p, childpgrp) && future_jobc)
+			pgadjustjobc(childpgrp, true);
+	}
+
+	if (isjobproc(p->p_pptr, p->p_pgrp))
+		pgadjustjobc(p->p_pgrp, false);
+	LIST_FOREACH(q, &p->p_children, p_sibling) {
+		if ((q->p_treeflag & P_TREE_GRPEXITED) != 0)
+			continue;
+		childpgrp = q->p_pgrp;
+		future_jobc = childpgrp != pgrp &&
+		    childpgrp->pg_session == pgrp->pg_session;
+		if (isjobproc(p, childpgrp) && !future_jobc)
+			pgadjustjobc(childpgrp, false);
+	}
+}
+
+static void
+fixjobc_kill(struct proc *p)
+{
+	struct proc *q;
+	struct pgrp *childpgrp, *pgrp;
+
+	sx_assert(&proctree_lock, SX_LOCKED);
+	PROC_LOCK_ASSERT(p, MA_NOTOWNED);
+	pgrp = p->p_pgrp;
+	PGRP_LOCK_ASSERT(pgrp, MA_NOTOWNED);
+	SESS_LOCK_ASSERT(pgrp->pg_session, MA_NOTOWNED);
+
+	/*
+	 * p no longer affects process group orphanage for children.
+	 * It is marked by the flag because p is only physically
+	 * removed from its process group on wait(2).
+	 */
+	p->p_treeflag |= P_TREE_GRPEXITED;
+
 	/*
 	 * Check p's parent to see whether p qualifies its own process
 	 * group; if so, adjust count for p's process group.
 	 */
-	mysession = pgrp->pg_session;
-	if ((hispgrp = p->p_pptr->p_pgrp) != pgrp &&
-	    hispgrp->pg_session == mysession)
-		pgadjustjobc(pgrp, entering);
+	if (isjobproc(p->p_pptr, pgrp))
+		pgadjustjobc(pgrp, false);
 
 	/*
 	 * Check this process' children to see whether they qualify
-	 * their process groups; if so, adjust counts for children's
-	 * process groups.
+	 * their process groups after reparenting to reaper.  If so,
+	 * adjust counts for children's process groups.
 	 */
 	LIST_FOREACH(q, &p->p_children, p_sibling) {
-		hispgrp = q->p_pgrp;
-		if (hispgrp == pgrp ||
-		    hispgrp->pg_session != mysession)
+		if ((q->p_treeflag & P_TREE_GRPEXITED) != 0)
 			continue;
-		if (q->p_state == PRS_ZOMBIE)
+		childpgrp = q->p_pgrp;
+		if (isjobproc(q->p_reaper, childpgrp) &&
+		    !isjobproc(p, childpgrp))
+			pgadjustjobc(childpgrp, true);
+	}
+	LIST_FOREACH(q, &p->p_children, p_sibling) {
+		if ((q->p_treeflag & P_TREE_GRPEXITED) != 0)
 			continue;
-		pgadjustjobc(hispgrp, entering);
+		childpgrp = q->p_pgrp;
+		if (!isjobproc(q->p_reaper, childpgrp) &&
+		    isjobproc(p, childpgrp))
+			pgadjustjobc(childpgrp, false);
 	}
 }
 
@@ -805,20 +893,8 @@ killjobc(void)
 
 	p = curproc;
 	MPASS(p->p_flag & P_WEXIT);
-	/*
-	 * Do a quick check to see if there is anything to do with the
-	 * proctree_lock held. pgrp and LIST_EMPTY checks are for fixjobc().
-	 */
-	PROC_LOCK(p);
-	if (!SESS_LEADER(p) &&
-	    (p->p_pgrp == p->p_pptr->p_pgrp) &&
-	    LIST_EMPTY(&p->p_children)) {
-		PROC_UNLOCK(p);
-		return;
-	}
-	PROC_UNLOCK(p);
+	sx_assert(&proctree_lock, SX_LOCKED);
 
-	sx_xlock(&proctree_lock);
 	if (SESS_LEADER(p)) {
 		sp = p->p_session;
 
@@ -864,8 +940,7 @@ killjobc(void)
 			sx_xlock(&proctree_lock);
 		}
 	}
-	fixjobc(p, p->p_pgrp, 0);
-	sx_xunlock(&proctree_lock);
+	fixjobc_kill(p);
 }
 
 /*

sys/sys/proc.h

@@ -790,6 +790,7 @@ struct proc {
 #define	P_TREE_FIRST_ORPHAN	0x00000002	/* First element of orphan
 						   list */
 #define	P_TREE_REAPER		0x00000004	/* Reaper of subtree */
+#define	P_TREE_GRPEXITED	0x00000008	/* exit1() done with job ctl */
 
 /*
  * These were process status values (p_stat), now they are only used in
@@ -1045,7 +1046,6 @@ int	enterpgrp(struct proc *p, pid_t pgid, struct pgrp *pgrp,
 	    struct session *sess);
 int	enterthispgrp(struct proc *p, struct pgrp *pgrp);
 void	faultin(struct proc *p);
-void	fixjobc(struct proc *p, struct pgrp *pgrp, int entering);
 int	fork1(struct thread *, struct fork_req *);
 void	fork_rfppwait(struct thread *);
 void	fork_exit(void (*)(void *, struct trapframe *), void *,