Date: Mon, 7 Aug 2023 00:59:49 GMT From: Konstantin Belousov <kib@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org Subject: git: 2b0cd3b55294 - stable/13 - killpg(2): close a race with fork(2), part1 Message-ID: <202308070059.3770xnGF077187@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch stable/13 has been updated by kib: URL: https://cgit.FreeBSD.org/src/commit/?id=2b0cd3b552942c642a84f8e224b989c02d97125d commit 2b0cd3b552942c642a84f8e224b989c02d97125d Author: Konstantin Belousov <kib@FreeBSD.org> AuthorDate: 2023-06-12 07:33:43 +0000 Commit: Konstantin Belousov <kib@FreeBSD.org> CommitDate: 2023-08-07 00:48:45 +0000 killpg(2): close a race with fork(2), part1 (cherry picked from commit 3360b48525fc966894e77b8cd9c124669664472d) --- sys/kern/init_main.c | 1 + sys/kern/kern_fork.c | 28 ++++++++++++++++++++++++++++ sys/kern/kern_proc.c | 20 ++++++++++++++++++++ sys/kern/kern_prot.c | 17 +++++++++++++---- sys/kern/kern_sig.c | 8 ++++++++ sys/sys/proc.h | 2 ++ 6 files changed, 72 insertions(+), 4 deletions(-) diff --git a/sys/kern/init_main.c b/sys/kern/init_main.c index c8cb0f5613ee..2f98d8a577d9 100644 --- a/sys/kern/init_main.c +++ b/sys/kern/init_main.c @@ -489,6 +489,7 @@ proc0_init(void *dummy __unused) LIST_INSERT_HEAD(&allproc, p, p_list); LIST_INSERT_HEAD(PIDHASH(0), p, p_hash); mtx_init(&pgrp0.pg_mtx, "process group", NULL, MTX_DEF | MTX_DUPOK); + sx_init(&pgrp0.pg_killsx, "killpg racer"); p->p_pgrp = &pgrp0; LIST_INSERT_HEAD(PGRPHASH(0), &pgrp0, pg_hash); LIST_INIT(&pgrp0.pg_members); diff --git a/sys/kern/kern_fork.c b/sys/kern/kern_fork.c index 2d7e7bc0de4a..6fb9d2a83c8f 100644 --- a/sys/kern/kern_fork.c +++ b/sys/kern/kern_fork.c @@ -851,11 +851,13 @@ fork1(struct thread *td, struct fork_req *fr) struct vmspace *vm2; struct ucred *cred; struct file *fp_procdesc; + struct pgrp *pg; vm_ooffset_t mem_charged; int error, nprocs_new; static int curfail; static struct timeval lastfail; int flags, pages; + bool killsx_locked; flags = fr->fr_flags; pages = fr->fr_pages; @@ -912,6 +914,7 @@ fork1(struct thread *td, struct fork_req *fr) fp_procdesc = NULL; newproc = NULL; vm2 = NULL; + killsx_locked = false; /* * Increment the nprocs resource before allocations occur. @@ -941,6 +944,28 @@ fork1(struct thread *td, struct fork_req *fr) } } + /* + * Atomically check for signals and block threads from sending + * a signal to our process group until the child is visible. + */ + pg = p1->p_pgrp; + if (sx_slock_sig(&pg->pg_killsx) != 0) { + error = ERESTART; + goto fail2; + } else if (__predict_false(p1->p_pgrp != pg || sig_intr() != 0)) { + /* + * Either the process was moved to other process + * group, or there is pending signal. sx_slock_sig() + * does not check for signals if not sleeping for the + * lock. + */ + sx_sunlock(&pg->pg_killsx); + error = ERESTART; + goto fail2; + } else { + killsx_locked = true; + } + /* * If required, create a process descriptor in the parent first; we * will abandon it if something goes wrong. We don't finit() until @@ -1031,6 +1056,7 @@ fork1(struct thread *td, struct fork_req *fr) } do_fork(td, fr, newproc, td2, vm2, fp_procdesc); + sx_sunlock(&pg->pg_killsx); return (0); fail0: error = EAGAIN; @@ -1049,6 +1075,8 @@ fail2: fdrop(fp_procdesc, td); } atomic_add_int(&nprocs, -1); + if (killsx_locked) + sx_sunlock(&pg->pg_killsx); pause("fork", hz / 2); return (error); } diff --git a/sys/kern/kern_proc.c b/sys/kern/kern_proc.c index 2879af8f9335..09796d542b4d 100644 --- a/sys/kern/kern_proc.c +++ b/sys/kern/kern_proc.c @@ -311,6 +311,7 @@ pgrp_init(void *mem, int size, int flags) pg = mem; mtx_init(&pg->pg_mtx, "process group", NULL, MTX_DEF | MTX_DUPOK); + sx_init(&pg->pg_killsx, "killpg racer"); return (0); } @@ -574,6 +575,7 @@ errout: int enterpgrp(struct proc *p, pid_t pgid, struct pgrp *pgrp, struct session *sess) { + struct pgrp *old_pgrp; sx_assert(&proctree_lock, SX_XLOCKED); @@ -585,6 +587,11 @@ enterpgrp(struct proc *p, pid_t pgid, struct pgrp *pgrp, struct session *sess) KASSERT(!SESS_LEADER(p), ("enterpgrp: session leader attempted setpgrp")); + old_pgrp = p->p_pgrp; + if (!sx_try_xlock(&old_pgrp->pg_killsx)) + return (ERESTART); + MPASS(old_pgrp == p->p_pgrp); + if (sess != NULL) { /* * new session @@ -626,6 +633,7 @@ enterpgrp(struct proc *p, pid_t pgid, struct pgrp *pgrp, struct session *sess) doenterpgrp(p, pgrp); + sx_xunlock(&old_pgrp->pg_killsx); return (0); } @@ -635,6 +643,7 @@ enterpgrp(struct proc *p, pid_t pgid, struct pgrp *pgrp, struct session *sess) int enterthispgrp(struct proc *p, struct pgrp *pgrp) { + struct pgrp *old_pgrp; sx_assert(&proctree_lock, SX_XLOCKED); PROC_LOCK_ASSERT(p, MA_NOTOWNED); @@ -647,8 +656,19 @@ enterthispgrp(struct proc *p, struct pgrp *pgrp) KASSERT(pgrp != p->p_pgrp, ("%s: p %p belongs to pgrp %p", __func__, p, pgrp)); + old_pgrp = p->p_pgrp; + if (!sx_try_xlock(&old_pgrp->pg_killsx)) + return (ERESTART); + MPASS(old_pgrp == p->p_pgrp); + if (!sx_try_xlock(&pgrp->pg_killsx)) { + sx_xunlock(&old_pgrp->pg_killsx); + return (ERESTART); + } + doenterpgrp(p, pgrp); + sx_xunlock(&pgrp->pg_killsx); + sx_xunlock(&old_pgrp->pg_killsx); return (0); } diff --git a/sys/kern/kern_prot.c b/sys/kern/kern_prot.c index cf88cae3a72d..3677c6db368e 100644 --- a/sys/kern/kern_prot.c +++ b/sys/kern/kern_prot.c @@ -332,12 +332,13 @@ sys_setsid(struct thread *td, struct setsid_args *uap) struct pgrp *newpgrp; struct session *newsess; - error = 0; pgrp = NULL; newpgrp = uma_zalloc(pgrp_zone, M_WAITOK); newsess = malloc(sizeof(struct session), M_SESSION, M_WAITOK | M_ZERO); +again: + error = 0; sx_xlock(&proctree_lock); if (p->p_pgid == p->p_pid || (pgrp = pgfind(p->p_pid)) != NULL) { @@ -345,7 +346,12 @@ sys_setsid(struct thread *td, struct setsid_args *uap) PGRP_UNLOCK(pgrp); error = EPERM; } else { - (void)enterpgrp(p, p->p_pid, newpgrp, newsess); + error = enterpgrp(p, p->p_pid, newpgrp, newsess); + if (error == ERESTART) { + sx_xunlock(&proctree_lock); + goto again; + } + MPASS(error == 0); td->td_retval[0] = p->p_pid; newpgrp = NULL; newsess = NULL; @@ -391,10 +397,11 @@ sys_setpgid(struct thread *td, struct setpgid_args *uap) if (uap->pgid < 0) return (EINVAL); - error = 0; - newpgrp = uma_zalloc(pgrp_zone, M_WAITOK); +again: + error = 0; + sx_xlock(&proctree_lock); if (uap->pid != 0 && uap->pid != curp->p_pid) { if ((targp = pfind(uap->pid)) == NULL) { @@ -456,6 +463,8 @@ done: sx_xunlock(&proctree_lock); KASSERT(error == 0 || newpgrp != NULL, ("setpgid failed and newpgrp is NULL")); + if (error == ERESTART) + goto again; uma_zfree(pgrp_zone, newpgrp); return (error); } diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c index 79440c2866df..77f63252454a 100644 --- a/sys/kern/kern_sig.c +++ b/sys/kern/kern_sig.c @@ -1769,6 +1769,7 @@ killpg1(struct thread *td, int sig, int pgid, int all, ksiginfo_t *ksi) } sx_sunlock(&allproc_lock); } else { +again: sx_slock(&proctree_lock); if (pgid == 0) { /* @@ -1784,10 +1785,17 @@ killpg1(struct thread *td, int sig, int pgid, int all, ksiginfo_t *ksi) } } sx_sunlock(&proctree_lock); + if (!sx_try_xlock(&pgrp->pg_killsx)) { + PGRP_UNLOCK(pgrp); + sx_xlock(&pgrp->pg_killsx); + sx_xunlock(&pgrp->pg_killsx); + goto again; + } LIST_FOREACH(p, &pgrp->pg_members, p_pglist) { killpg1_sendsig(p, false, &arg); } PGRP_UNLOCK(pgrp); + sx_xunlock(&pgrp->pg_killsx); } MPASS(arg.ret != 0 || arg.found || !arg.sent); if (arg.ret == 0 && !arg.sent) diff --git a/sys/sys/proc.h b/sys/sys/proc.h index 997ff110bcdc..d3daa0c6879a 100644 --- a/sys/sys/proc.h +++ b/sys/sys/proc.h @@ -113,6 +113,8 @@ struct pgrp { pid_t pg_id; /* (c) Process group id. */ struct mtx pg_mtx; /* Mutex to protect members */ int pg_flags; /* (m) PGRP_ flags */ + struct sx pg_killsx; /* Mutual exclusion between group member + * fork() and killpg() */ }; #define PGRP_ORPHANED 0x00000001 /* Group is orphaned */
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202308070059.3770xnGF077187>