From owner-svn-src-stable-12@freebsd.org Wed Nov 18 14:27:25 2020 Return-Path: Delivered-To: svn-src-stable-12@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id A7DDF468B2B; Wed, 18 Nov 2020 14:27:25 +0000 (UTC) (envelope-from markj@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4CblWd48SLz3hN9; Wed, 18 Nov 2020 14:27:25 +0000 (UTC) (envelope-from markj@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 7C8B412FF2; Wed, 18 Nov 2020 14:27:25 +0000 (UTC) (envelope-from markj@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id 0AIERPkG003684; Wed, 18 Nov 2020 14:27:25 GMT (envelope-from markj@FreeBSD.org) Received: (from markj@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id 0AIERO1o003680; Wed, 18 Nov 2020 14:27:24 GMT (envelope-from markj@FreeBSD.org) Message-Id: <202011181427.0AIERO1o003680@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: markj set sender to markj@FreeBSD.org using -f From: Mark Johnston Date: Wed, 18 Nov 2020 14:27:24 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-12@freebsd.org Subject: svn commit: r367791 - in stable/12/sys: kern sys X-SVN-Group: stable-12 X-SVN-Commit-Author: markj X-SVN-Commit-Paths: in stable/12/sys: kern sys X-SVN-Commit-Revision: 367791 X-SVN-Commit-Repository: base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-stable-12@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: SVN commit messages for only the 12-stable src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 18 Nov 2020 14:27:25 -0000 Author: markj Date: Wed Nov 18 14:27:24 2020 New Revision: 367791 URL: https://svnweb.freebsd.org/changeset/base/367791 Log: MFC r367588: Fix a pair of races in SIGIO registration Modified: stable/12/sys/kern/kern_descrip.c stable/12/sys/kern/kern_exit.c stable/12/sys/kern/kern_proc.c stable/12/sys/sys/signalvar.h Directory Properties: stable/12/ (props changed) Modified: stable/12/sys/kern/kern_descrip.c ============================================================================== --- stable/12/sys/kern/kern_descrip.c Wed Nov 18 13:52:13 2020 (r367790) +++ stable/12/sys/kern/kern_descrip.c Wed Nov 18 14:27:24 2020 (r367791) @@ -954,6 +954,40 @@ unlock: return (error); } +static void +sigiofree(struct sigio *sigio) +{ + crfree(sigio->sio_ucred); + free(sigio, M_SIGIO); +} + +static struct sigio * +funsetown_locked(struct sigio *sigio) +{ + struct proc *p; + struct pgrp *pg; + + SIGIO_ASSERT_LOCKED(); + + if (sigio == NULL) + return (NULL); + *(sigio->sio_myref) = NULL; + if (sigio->sio_pgid < 0) { + pg = sigio->sio_pgrp; + PGRP_LOCK(pg); + SLIST_REMOVE(&sigio->sio_pgrp->pg_sigiolst, sigio, + sigio, sio_pgsigio); + PGRP_UNLOCK(pg); + } else { + p = sigio->sio_proc; + PROC_LOCK(p); + SLIST_REMOVE(&sigio->sio_proc->p_sigiolst, sigio, + sigio, sio_pgsigio); + PROC_UNLOCK(p); + } + return (sigio); +} + /* * If sigio is on the list associated with a process or process group, * disable signalling from the device, remove sigio from the list and @@ -964,92 +998,82 @@ funsetown(struct sigio **sigiop) { struct sigio *sigio; + /* Racy check, consumers must provide synchronization. */ if (*sigiop == NULL) return; + SIGIO_LOCK(); - sigio = *sigiop; - if (sigio == NULL) { - SIGIO_UNLOCK(); - return; - } - *(sigio->sio_myref) = NULL; - if ((sigio)->sio_pgid < 0) { - struct pgrp *pg = (sigio)->sio_pgrp; - PGRP_LOCK(pg); - SLIST_REMOVE(&sigio->sio_pgrp->pg_sigiolst, sigio, - sigio, sio_pgsigio); - PGRP_UNLOCK(pg); - } else { - struct proc *p = (sigio)->sio_proc; - PROC_LOCK(p); - SLIST_REMOVE(&sigio->sio_proc->p_sigiolst, sigio, - sigio, sio_pgsigio); - PROC_UNLOCK(p); - } + sigio = funsetown_locked(*sigiop); SIGIO_UNLOCK(); - crfree(sigio->sio_ucred); - free(sigio, M_SIGIO); + if (sigio != NULL) + sigiofree(sigio); } /* - * Free a list of sigio structures. - * We only need to lock the SIGIO_LOCK because we have made ourselves - * inaccessible to callers of fsetown and therefore do not need to lock - * the proc or pgrp struct for the list manipulation. + * Free a list of sigio structures. The caller must ensure that new sigio + * structures cannot be added after this point. For process groups this is + * guaranteed using the proctree lock; for processes, the P_WEXIT flag serves + * as an interlock. */ void funsetownlst(struct sigiolst *sigiolst) { struct proc *p; struct pgrp *pg; - struct sigio *sigio; + struct sigio *sigio, *tmp; + /* Racy check. */ sigio = SLIST_FIRST(sigiolst); if (sigio == NULL) return; + p = NULL; pg = NULL; + SIGIO_LOCK(); + sigio = SLIST_FIRST(sigiolst); + if (sigio == NULL) { + SIGIO_UNLOCK(); + return; + } + /* - * Every entry of the list should belong - * to a single proc or pgrp. + * Every entry of the list should belong to a single proc or pgrp. */ if (sigio->sio_pgid < 0) { pg = sigio->sio_pgrp; - PGRP_LOCK_ASSERT(pg, MA_NOTOWNED); + sx_assert(&proctree_lock, SX_XLOCKED); + PGRP_LOCK(pg); } else /* if (sigio->sio_pgid > 0) */ { p = sigio->sio_proc; - PROC_LOCK_ASSERT(p, MA_NOTOWNED); + PROC_LOCK(p); + KASSERT((p->p_flag & P_WEXIT) != 0, + ("%s: process %p is not exiting", __func__, p)); } - SIGIO_LOCK(); - while ((sigio = SLIST_FIRST(sigiolst)) != NULL) { - *(sigio->sio_myref) = NULL; + SLIST_FOREACH(sigio, sigiolst, sio_pgsigio) { + *sigio->sio_myref = NULL; if (pg != NULL) { KASSERT(sigio->sio_pgid < 0, ("Proc sigio in pgrp sigio list")); KASSERT(sigio->sio_pgrp == pg, ("Bogus pgrp in sigio list")); - PGRP_LOCK(pg); - SLIST_REMOVE(&pg->pg_sigiolst, sigio, sigio, - sio_pgsigio); - PGRP_UNLOCK(pg); } else /* if (p != NULL) */ { KASSERT(sigio->sio_pgid > 0, ("Pgrp sigio in proc sigio list")); KASSERT(sigio->sio_proc == p, ("Bogus proc in sigio list")); - PROC_LOCK(p); - SLIST_REMOVE(&p->p_sigiolst, sigio, sigio, - sio_pgsigio); - PROC_UNLOCK(p); } - SIGIO_UNLOCK(); - crfree(sigio->sio_ucred); - free(sigio, M_SIGIO); - SIGIO_LOCK(); } + + if (pg != NULL) + PGRP_UNLOCK(pg); + else + PROC_UNLOCK(p); SIGIO_UNLOCK(); + + SLIST_FOREACH_SAFE(sigio, sigiolst, sio_pgsigio, tmp) + sigiofree(sigio); } /* @@ -1063,7 +1087,7 @@ fsetown(pid_t pgid, struct sigio **sigiop) { struct proc *proc; struct pgrp *pgrp; - struct sigio *sigio; + struct sigio *osigio, *sigio; int ret; if (pgid == 0) { @@ -1073,13 +1097,14 @@ fsetown(pid_t pgid, struct sigio **sigiop) ret = 0; - /* Allocate and fill in the new sigio out of locks. */ sigio = malloc(sizeof(struct sigio), M_SIGIO, M_WAITOK); sigio->sio_pgid = pgid; sigio->sio_ucred = crhold(curthread->td_ucred); sigio->sio_myref = sigiop; sx_slock(&proctree_lock); + SIGIO_LOCK(); + osigio = funsetown_locked(*sigiop); if (pgid > 0) { proc = pfind(pgid); if (proc == NULL) { @@ -1095,20 +1120,21 @@ fsetown(pid_t pgid, struct sigio **sigiop) * restrict FSETOWN to the current process or process * group for maximum safety. */ - PROC_UNLOCK(proc); if (proc->p_session != curthread->td_proc->p_session) { + PROC_UNLOCK(proc); ret = EPERM; goto fail; } - pgrp = NULL; + sigio->sio_proc = proc; + SLIST_INSERT_HEAD(&proc->p_sigiolst, sigio, sio_pgsigio); + PROC_UNLOCK(proc); } else /* if (pgid < 0) */ { pgrp = pgfind(-pgid); if (pgrp == NULL) { ret = ESRCH; goto fail; } - PGRP_UNLOCK(pgrp); /* * Policy - Don't allow a process to FSETOWN a process @@ -1119,44 +1145,28 @@ fsetown(pid_t pgid, struct sigio **sigiop) * group for maximum safety. */ if (pgrp->pg_session != curthread->td_proc->p_session) { + PGRP_UNLOCK(pgrp); ret = EPERM; goto fail; } - proc = NULL; - } - funsetown(sigiop); - if (pgid > 0) { - PROC_LOCK(proc); - /* - * Since funsetownlst() is called without the proctree - * locked, we need to check for P_WEXIT. - * XXX: is ESRCH correct? - */ - if ((proc->p_flag & P_WEXIT) != 0) { - PROC_UNLOCK(proc); - ret = ESRCH; - goto fail; - } - SLIST_INSERT_HEAD(&proc->p_sigiolst, sigio, sio_pgsigio); - sigio->sio_proc = proc; - PROC_UNLOCK(proc); - } else { - PGRP_LOCK(pgrp); SLIST_INSERT_HEAD(&pgrp->pg_sigiolst, sigio, sio_pgsigio); sigio->sio_pgrp = pgrp; PGRP_UNLOCK(pgrp); } sx_sunlock(&proctree_lock); - SIGIO_LOCK(); *sigiop = sigio; SIGIO_UNLOCK(); + if (osigio != NULL) + sigiofree(osigio); return (0); fail: + SIGIO_UNLOCK(); sx_sunlock(&proctree_lock); - crfree(sigio->sio_ucred); - free(sigio, M_SIGIO); + sigiofree(sigio); + if (osigio != NULL) + sigiofree(osigio); return (ret); } Modified: stable/12/sys/kern/kern_exit.c ============================================================================== --- stable/12/sys/kern/kern_exit.c Wed Nov 18 13:52:13 2020 (r367790) +++ stable/12/sys/kern/kern_exit.c Wed Nov 18 14:27:24 2020 (r367791) @@ -355,7 +355,7 @@ exit1(struct thread *td, int rval, int signo) /* * Reset any sigio structures pointing to us as a result of - * F_SETOWN with our pid. + * F_SETOWN with our pid. The P_WEXIT flag interlocks with fsetown(). */ funsetownlst(&p->p_sigiolst); Modified: stable/12/sys/kern/kern_proc.c ============================================================================== --- stable/12/sys/kern/kern_proc.c Wed Nov 18 13:52:13 2020 (r367790) +++ stable/12/sys/kern/kern_proc.c Wed Nov 18 14:27:24 2020 (r367791) @@ -686,7 +686,8 @@ pgdelete(struct pgrp *pgrp) /* * Reset any sigio structures pointing to us as a result of - * F_SETOWN with our pgid. + * F_SETOWN with our pgid. The proctree lock ensures that + * new sigio structures will not be added after this point. */ funsetownlst(&pgrp->pg_sigiolst); Modified: stable/12/sys/sys/signalvar.h ============================================================================== --- stable/12/sys/sys/signalvar.h Wed Nov 18 13:52:13 2020 (r367790) +++ stable/12/sys/sys/signalvar.h Wed Nov 18 14:27:24 2020 (r367791) @@ -320,7 +320,7 @@ struct thread; #define SIGIO_TRYLOCK() mtx_trylock(&sigio_lock) #define SIGIO_UNLOCK() mtx_unlock(&sigio_lock) #define SIGIO_LOCKED() mtx_owned(&sigio_lock) -#define SIGIO_ASSERT(type) mtx_assert(&sigio_lock, type) +#define SIGIO_ASSERT_LOCKED(type) mtx_assert(&sigio_lock, MA_OWNED) extern struct mtx sigio_lock;