Date: Sat, 22 Aug 2020 21:32:11 +0000 (UTC) From: Konstantin Belousov <kib@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r364495 - in head/sys: kern sys Message-ID: <202008222132.07MLWBQV055438@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: kib Date: Sat Aug 22 21:32:11 2020 New Revision: 364495 URL: https://svnweb.freebsd.org/changeset/base/364495 Log: 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 Modified: head/sys/kern/kern_exit.c head/sys/kern/kern_proc.c head/sys/sys/proc.h Modified: head/sys/kern/kern_exit.c ============================================================================== --- head/sys/kern/kern_exit.c Sat Aug 22 20:52:02 2020 (r364494) +++ head/sys/kern/kern_exit.c Sat Aug 22 21:32:11 2020 (r364495) @@ -391,7 +391,6 @@ exit1(struct thread *td, int rval, int signo) } vmspace_exit(td); - killjobc(); (void)acct_process(td); #ifdef KTRACE @@ -434,6 +433,12 @@ exit1(struct thread *td, int rval, int signo) PROC_LOCK(p); 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: Modified: head/sys/kern/kern_proc.c ============================================================================== --- head/sys/kern/kern_proc.c Sat Aug 22 20:52:02 2020 (r364494) +++ head/sys/kern/kern_proc.c Sat Aug 22 21:32:11 2020 (r364495) @@ -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); @@ -633,6 +634,43 @@ enterthispgrp(struct proc *p, struct pgrp *pgrp) } /* + * 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 */ static void @@ -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); } /* Modified: head/sys/sys/proc.h ============================================================================== --- head/sys/sys/proc.h Sat Aug 22 20:52:02 2020 (r364494) +++ head/sys/sys/proc.h Sat Aug 22 21:32:11 2020 (r364495) @@ -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 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 *,
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202008222132.07MLWBQV055438>