Date: Fri, 29 May 2026 16:01:56 +0000 From: Olivier Certner <olce@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: 0db7f110cb81 - main - MAC/do: Support for atomically modifying configurations Message-ID: <6a19b874.367a6.3a018215@gitrepo.freebsd.org>
index | next in thread | raw e-mail
The branch main has been updated by olce: URL: https://cgit.FreeBSD.org/src/commit/?id=0db7f110cb810b7aa6d29df221edf9091d66b58a commit 0db7f110cb810b7aa6d29df221edf9091d66b58a Author: Olivier Certner <olce@FreeBSD.org> AuthorDate: 2026-04-29 16:28:44 +0000 Commit: Olivier Certner <olce@FreeBSD.org> CommitDate: 2026-05-29 15:35:19 +0000 MAC/do: Support for atomically modifying configurations As mentioned in previous commits "MAC/do: parse_and_set_conf(): Require the model configuration" and "MAC/do: Sequential consistency for configuration retrieval", the introduction of the "executable path" feature, more fundamentally, the fact that there is now more than one per-jail parameter and that parameters can be independently modified or copied, causes an atomicity problem in case of concurrent accesses to of a jail's applicable configuration. Partially modifying a configuration is indeed akin to a read-modify-write operation, where the read is either to the current or an inherited configuration. More precisely, once pointed to by a jail, a configuration object is immutable, and changing the jail's configuration means making the jail point to another configuration object. To change a jail's configuration, a new configuration object is thus built, and if only some parameters have been explicitly specified, those that have not been are set by copying the corresponding values from an existing configuration object (in case of partial modification of the existing configuration, from the original configuration object that is going to be replaced; in case of breakage of inheritance or at jail creation, from the applicable configuration object, which is on an ancestor jail). This process is not immune to concurrent modifications because nothing prevents changes of configurations between reading existing values and setting the new configuration. Thus, some other thread could change the value of a parameter after a copy of it has been made into the new object but before that copy is actually installed, which effectively will erase the other thread's modification. To avoid this, we introduce support for serializing configuration changes on a given jail. To this end, we move the jail climbing process from find_conf() into find_conf_locked(), and make the former call the latter in a read-locked section. Similarly, we isolate setting a configuration in the new set_conf_locked() function, and make set_conf() call it inside a write-locked section. The new *_unlocked() variants make it possible to prevent any configuration access between determining and reading an applicable configuration, computing from it a new configuration object and finally setting it, by holding a write lock over the whole process (there is a trade-off here, as read-mostly locks cannot be upgraded), effectively making it atomic and realizing full sequential consistency of configuration changes. Also, the 'mac_do_rm' global read-mostly lock is made sleepable so that it can be write-locked over sysctl_handle*() functions or memory allocations (eases implementation, at the expense of a potential loss of concurrency which is most probably irrelevant). No functional change (intended) at this point. Reviewed by: bapt Fixes: 9818224174c4 ("MAC/do: Executable paths feature (GSoC 2025's final state)") MFC after: 1 month Sponsored by: The FreeBSD Foundation Pull Request: https://ron-dev.freebsd.org/FreeBSD/src/pulls/38 --- sys/security/mac_do/mac_do.c | 64 +++++++++++++++++++++++++++++++++----------- 1 file changed, 49 insertions(+), 15 deletions(-) diff --git a/sys/security/mac_do/mac_do.c b/sys/security/mac_do/mac_do.c index 3775466326f4..8065ff4a0e47 100644 --- a/sys/security/mac_do/mac_do.c +++ b/sys/security/mac_do/mac_do.c @@ -1244,13 +1244,12 @@ drop_conf(struct conf *const conf) * one, establishing inheritance from higher up). */ static struct conf * -find_conf(struct prison *const pr, struct prison **const hpr) +find_conf_locked(struct prison *const pr, struct prison **const hpr) { struct prison *cpr, *ppr; /* Current and parent. */ struct conf *conf; - struct rm_priotracker rmpt; - rm_rlock(&mac_do_rml, &rmpt); + rm_assert(&mac_do_rml, RA_LOCKED); /* * We do not need to take any locks here to climb the prison tree as * either the start prison ('pr') is that of the current thread (and our @@ -1278,11 +1277,20 @@ find_conf(struct prison *const pr, struct prison **const hpr) } hold_conf(conf); - rm_runlock(&mac_do_rml, &rmpt); - if (hpr != NULL) *hpr = cpr; + return (conf); +} + +static struct conf * +find_conf(struct prison *const pr, struct prison **const hpr) +{ + struct conf *conf; + struct rm_priotracker rmpt; + rm_rlock(&mac_do_rml, &rmpt); + conf = find_conf_locked(pr, hpr); + rm_runlock(&mac_do_rml, &rmpt); return (conf); } @@ -1330,22 +1338,29 @@ dealloc_jail_osd(void *const value) } /* - * Assign an already-built configuration to a jail. + * Sets a mac_do(4) configuration on a jail. * - * Takes care of write-locking 'mac_do_rm', which should be unlocked on entry - * and will be unlocked on exit. + * 'conf' is the new conf to set (can be NULL), and an additional reference will + * be taken on it to represent the jail holding it (if not NULL). 'rsv' must + * have been allocated through osd_reserve() (if 'conf' is not NULL; else can + * be NULL). + * + * The previous configuration on the jail (or NULL) is returned (with an + * associated reference if not NULL). */ -static void -set_conf(struct prison *const pr, struct conf *const conf) +static struct conf * +set_conf_locked(struct prison *const pr, struct conf *const conf, + void **const rsv) { - void **const rsv = conf != NULL ? osd_reserve(osd_jail_slot) : NULL; struct conf *old_conf; int error __diagused; + KASSERT(conf == NULL || rsv != NULL, + ("MAC/do: OSD reserve needed to avoid allocating memory")); + rm_assert(&mac_do_rml, RA_WLOCKED); + if (conf != NULL) hold_conf(conf); - - rm_wlock(&mac_do_rml); old_conf = osd_jail_get_unlocked(pr, osd_jail_slot); error = osd_jail_set_reserved(pr, osd_jail_slot, rsv, conf); KASSERT(error == 0, ("MAC/do: osd_jail_set_reserved() failed " @@ -1356,8 +1371,27 @@ set_conf(struct prison *const pr, struct conf *const conf) * destructor since we've just put NULL into the slot. */ osd_jail_del(pr, osd_jail_slot); - rm_wunlock(&mac_do_rml); + return (old_conf); +} +/* + * Immediately replace the jail's configuration. + * + * To be used only if the configuration to set does not depend in any way on the + * currently applicable configuration. + * + * Takes care of write-locking 'mac_do_rml', which should be unlocked on entry + * and will be unlocked on exit. + */ +static void +set_conf(struct prison *const pr, struct conf *const conf) +{ + void **const rsv = conf != NULL ? osd_reserve(osd_jail_slot) : NULL; + struct conf *old_conf; + + rm_wlock(&mac_do_rml); + old_conf = set_conf_locked(pr, conf, rsv); + rm_wunlock(&mac_do_rml); if (old_conf != NULL) drop_conf(old_conf); } @@ -2540,7 +2574,7 @@ mac_do_init(struct mac_policy_conf *mpc) struct conf *const default_conf = new_default_conf(); struct prison *pr; - rm_init(&mac_do_rml, "mac_do(4)"); + rm_init_flags(&mac_do_rml, "mac_do(4)", RM_SLEEPABLE); osd_jail_slot = osd_jail_register(dealloc_jail_osd, osd_methods); set_conf(&prison0, default_conf);home | help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?6a19b874.367a6.3a018215>