From owner-dev-commits-src-all@freebsd.org Tue Jan 19 01:27:44 2021 Return-Path: Delivered-To: dev-commits-src-all@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 D7ED04DBB6B; Tue, 19 Jan 2021 01:27:44 +0000 (UTC) (envelope-from git@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 "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4DKWHN5Jntz3nBW; Tue, 19 Jan 2021 01:27:44 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (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 did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id A90791E040; Tue, 19 Jan 2021 01:27:44 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.16.1/8.16.1) with ESMTP id 10J1RiSD098808; Tue, 19 Jan 2021 01:27:44 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 10J1RiA1098807; Tue, 19 Jan 2021 01:27:44 GMT (envelope-from git) Date: Tue, 19 Jan 2021 01:27:44 GMT Message-Id: <202101190127.10J1RiA1098807@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Jamie Gritton Subject: git: effad35ed1e5 - main - jail: Clean up some function placement and improve comments. MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: jamie X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: effad35ed1e52196469f8f2709b0f1f74cd2ce8c Auto-Submitted: auto-generated X-BeenThere: dev-commits-src-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for all branches of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 19 Jan 2021 01:27:44 -0000 The branch main has been updated by jamie: URL: https://cgit.FreeBSD.org/src/commit/?id=effad35ed1e52196469f8f2709b0f1f74cd2ce8c commit effad35ed1e52196469f8f2709b0f1f74cd2ce8c Author: Jamie Gritton AuthorDate: 2021-01-19 01:23:51 +0000 Commit: Jamie Gritton CommitDate: 2021-01-19 01:23:51 +0000 jail: Clean up some function placement and improve comments. Move prison_hold, prison_hold_locked ,prison_proc_hold, and prison_proc_free to a more intuitive part of the file (together with with prison_free and prison_free_locked), and add or improve comments to these and others, to better describe what's going in the prison reference cycle. No functional changes. --- sys/kern/kern_jail.c | 157 +++++++++++++++++++++++++++++++-------------------- 1 file changed, 96 insertions(+), 61 deletions(-) diff --git a/sys/kern/kern_jail.c b/sys/kern/kern_jail.c index 4893e4df2781..757b8bd06b89 100644 --- a/sys/kern/kern_jail.c +++ b/sys/kern/kern_jail.c @@ -2604,8 +2604,35 @@ prison_allow(struct ucred *cred, unsigned flag) } /* - * Remove a prison reference. If that was the last reference, remove the - * prison itself - but not in this context in case there are locks held. + * Hold a prison reference, by incrementing pr_ref. It is generally + * an error to hold a prison that does not already have a reference. + * A prison record will remain valid as long as it has at least one + * reference, and will not be removed as long as either the prison + * mutex or the allprison lock is held (allprison_lock may be shared). + */ +void +prison_hold_locked(struct prison *pr) +{ + + mtx_assert(&pr->pr_mtx, MA_OWNED); + KASSERT(pr->pr_ref > 0, + ("Trying to hold dead prison %p (jid=%d).", pr, pr->pr_id)); + pr->pr_ref++; +} + +void +prison_hold(struct prison *pr) +{ + + mtx_lock(&pr->pr_mtx); + prison_hold_locked(pr); + mtx_unlock(&pr->pr_mtx); +} + +/* + * Remove a prison reference. If that was the last reference, the + * prison will be removed (at a later time). Return with the prison + * unlocked. */ void prison_free_locked(struct prison *pr) @@ -2615,8 +2642,13 @@ prison_free_locked(struct prison *pr) mtx_assert(&pr->pr_mtx, MA_OWNED); ref = --pr->pr_ref; mtx_unlock(&pr->pr_mtx); - if (ref == 0) + if (ref == 0) { + /* + * Don't remove the last reference in this context, + * in case there are locks held. + */ taskqueue_enqueue(taskqueue_thread, &pr->pr_task); + } } void @@ -2627,6 +2659,54 @@ prison_free(struct prison *pr) prison_free_locked(pr); } +/* + * Hold a a prison for user visibility, by incrementing pr_uref. + * It is generally an error to hold a prison that isn't already + * user-visible, except through the the jail system calls. It is also + * an error to hold an invalid prison. A prison record will remain + * alive as long as it has at least one user reference, and will not + * be set to the dying state was long as the prison mutex is held. + */ +void +prison_proc_hold(struct prison *pr) +{ + + mtx_lock(&pr->pr_mtx); + KASSERT(pr->pr_uref > 0, + ("Cannot add a process to a non-alive prison (jid=%d)", pr->pr_id)); + pr->pr_uref++; + mtx_unlock(&pr->pr_mtx); +} + +/* + * Remove a prison user reference. If it was the last reference, the + * prison will be considered "dying", and may be removed once all of + * its references are dropped. + */ +void +prison_proc_free(struct prison *pr) +{ + + mtx_lock(&pr->pr_mtx); + KASSERT(pr->pr_uref > 0, + ("Trying to kill a process in a dead prison (jid=%d)", pr->pr_id)); + if (pr->pr_uref > 1) + pr->pr_uref--; + else { + /* + * Don't remove the last user reference in this context, + * which is expected to be a process that is not only locked, + * but also half dead. Add a reference so any calls to + * prison_free() won't re-submit the task. + */ + pr->pr_ref++; + mtx_unlock(&pr->pr_mtx); + taskqueue_enqueue(taskqueue_thread, &pr->pr_task); + return; + } + mtx_unlock(&pr->pr_mtx); +} + /* * Complete a call to either prison_free or prison_proc_free. */ @@ -2637,16 +2717,24 @@ prison_complete(void *context, int pending) sx_xlock(&allprison_lock); mtx_lock(&pr->pr_mtx); - prison_deref(pr, pr->pr_uref + /* + * If this is completing a call to prison_proc_free, there will still + * be a user reference held; clear that as well as the reference that + * was added. No references are expected if this is completing a call + * to prison_free, but prison_deref is still called for the cleanup. + */ + prison_deref(pr, pr->pr_uref > 0 ? PD_DEREF | PD_DEUREF | PD_LOCKED | PD_LIST_XLOCKED : PD_LOCKED | PD_LIST_XLOCKED); } /* - * Remove a prison reference (usually). This internal version assumes no - * mutexes are held, except perhaps the prison itself. If there are no more - * references, release and delist the prison. On completion, the prison lock - * and the allprison lock are both unlocked. + * Remove a prison reference and/or user reference (usually). + * This assumes context that allows sleeping (for allprison_lock), + * with no non-sleeping locks held, except perhaps the prison itself. + * If there are no more references, release and delist the prison. + * On completion, the prison lock and the allprison lock are both + * unlocked. */ static void prison_deref(struct prison *pr, int flags) @@ -2750,59 +2838,6 @@ prison_deref(struct prison *pr, int flags) } } -void -prison_hold_locked(struct prison *pr) -{ - - mtx_assert(&pr->pr_mtx, MA_OWNED); - KASSERT(pr->pr_ref > 0, - ("Trying to hold dead prison %p (jid=%d).", pr, pr->pr_id)); - pr->pr_ref++; -} - -void -prison_hold(struct prison *pr) -{ - - mtx_lock(&pr->pr_mtx); - prison_hold_locked(pr); - mtx_unlock(&pr->pr_mtx); -} - -void -prison_proc_hold(struct prison *pr) -{ - - mtx_lock(&pr->pr_mtx); - KASSERT(pr->pr_uref > 0, - ("Cannot add a process to a non-alive prison (jid=%d)", pr->pr_id)); - pr->pr_uref++; - mtx_unlock(&pr->pr_mtx); -} - -void -prison_proc_free(struct prison *pr) -{ - - mtx_lock(&pr->pr_mtx); - KASSERT(pr->pr_uref > 0, - ("Trying to kill a process in a dead prison (jid=%d)", pr->pr_id)); - if (pr->pr_uref > 1) - pr->pr_uref--; - else { - /* - * Don't remove the last user reference in this context, which - * is expected to be a process that is not only locked, but - * also half dead. - */ - pr->pr_ref++; - mtx_unlock(&pr->pr_mtx); - taskqueue_enqueue(taskqueue_thread, &pr->pr_task); - return; - } - mtx_unlock(&pr->pr_mtx); -} - /* * Set or clear a permission bit in the pr_allow field, passing restrictions * (cleared permission) down to child jails.