Date: Wed, 17 Apr 2002 08:59:49 -0700 (PDT) From: Robert Watson <rwatson@FreeBSD.org> To: Perforce Change Reviews <perforce@freebsd.org> Subject: PERFORCE change 9913 for review Message-ID: <200204171559.g3HFxnJ05061@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://people.freebsd.org/~peter/p4db/chv.cgi?CH=9913 Change 9913 by rwatson@rwatson_tislabs on 2002/04/17 08:59:13 Remove use of an SX lock to protect the MAC policy chain. Instead, use a mutex to protect the list in addition to a busy count. Read-only consumers of the list (such as policy evaluation entry points) will lock the mutex, increment the busy count, and release the lock, prevent long-term holding of a lock and interference with lock orders (as well as permitting recursion). When done, they'll grab the lock, decrement the busy count, and release it. Exclusive consumers, such as those registering and unregistering policies, grab the mutex, check that the busy count is zero, and return EBUSY if it is. They then hold the mutex for the duration of the list modification, as well as the entry points for module initialization and shutdown. It may be eventually we need two classes of busy to avoid that. Note: no waiting occurs if the list is busy and exclusive access is required. This means that kldloading a module can fail with EBUSY if a policy check is occurring simultaneously. As Giant gets pushed down, this will become an issue. Eventually, a CV should be used to wake up consumers waiting on exclusive access so that this does not occur. This commit gets rid of the various lock order warnings concerning the MAC policy lock. Affected files ... ... //depot/projects/trustedbsd/mac/sys/kern/kern_mac.c#125 edit Differences ... ==== //depot/projects/trustedbsd/mac/sys/kern/kern_mac.c#125 (text+ko) ==== @@ -134,9 +134,28 @@ * operations, locks should be held for the entire compound operation, * and that this is not yet done for relabel requests. */ -static struct sx mac_policy_list_lock; +static struct mtx mac_policy_list_lock; static LIST_HEAD(, mac_policy_conf) mac_policy_list; +static int mac_policy_list_busy; +#define MAC_POLICY_LIST_LOCKINIT() mtx_init(&mac_policy_list_lock, \ + "mac_policy_list_lock", NULL, MTX_DEF); +#define MAC_POLICY_LIST_LOCK() mtx_lock(&mac_policy_list_lock); +#define MAC_POLICY_LIST_UNLOCK() mtx_unlock(&mac_policy_list_lock); +#define MAC_POLICY_LIST_BUSY() do { \ + MAC_POLICY_LIST_LOCK(); \ + mac_policy_list_busy++; \ + MAC_POLICY_LIST_UNLOCK(); \ +} while (0) + +#define MAC_POLICY_LIST_UNBUSY() do { \ + MAC_POLICY_LIST_LOCK(); \ + mac_policy_list_busy--; \ + if (mac_policy_list_busy < 0) \ + panic("Extra mac_policy_list_busy--"); \ + MAC_POLICY_LIST_UNLOCK(); \ +} while (0) + /* * MAC_CHECK performs the designated check by walking the policy * module list and checking with each as to how it feels about the @@ -147,14 +166,14 @@ struct mac_policy_conf *mpc; \ \ error = 0; \ - sx_slock(&mac_policy_list_lock); \ + MAC_POLICY_LIST_BUSY(); \ LIST_FOREACH(mpc, &mac_policy_list, mpc_list) { \ - if (mpc->mpc_ops.mpo_ ## check != NULL) \ + if (mpc->mpc_ops.mpo_ ## check != NULL) \ error = error_select( \ mpc->mpc_ops.mpo_ ## check (## args), \ error); \ } \ - sx_sunlock(&mac_policy_list_lock); \ + MAC_POLICY_LIST_UNBUSY(); \ } while (0) /* @@ -168,13 +187,13 @@ #define MAC_BOOLEAN(operation, composition, args...) do { \ struct mac_policy_conf *mpc; \ \ - sx_slock(&mac_policy_list_lock); \ + MAC_POLICY_LIST_BUSY(); \ LIST_FOREACH(mpc, &mac_policy_list, mpc_list) { \ if (mpc->mpc_ops.mpo_ ## operation != NULL) \ result = result composition \ mpc->mpc_ops.mpo_ ## operation ( ## args); \ } \ - sx_sunlock(&mac_policy_list_lock); \ + MAC_POLICY_LIST_UNBUSY(); \ } while (0) /* @@ -184,12 +203,12 @@ #define MAC_PERFORM(operation, args...) do { \ struct mac_policy_conf *mpc; \ \ - sx_slock(&mac_policy_list_lock); \ + MAC_POLICY_LIST_BUSY(); \ LIST_FOREACH(mpc, &mac_policy_list, mpc_list) { \ if (mpc->mpc_ops.mpo_ ## operation != NULL) \ mpc->mpc_ops.mpo_ ## operation (## args); \ } \ - sx_sunlock(&mac_policy_list_lock); \ + MAC_POLICY_LIST_UNBUSY(); \ } while (0) /* @@ -200,7 +219,7 @@ { LIST_INIT(&mac_policy_list); - sx_init(&mac_policy_list_lock, "MAC policy chain lock"); + MAC_POLICY_LIST_LOCKINIT(); } /* @@ -480,17 +499,21 @@ return (EINVAL); } } - sx_xlock(&mac_policy_list_lock); + MAC_POLICY_LIST_LOCK(); + if (mac_policy_list_busy > 0) { + MAC_POLICY_LIST_UNLOCK(); + return (EBUSY); + } LIST_FOREACH(tmpc, &mac_policy_list, mpc_list) { if (strcmp(tmpc->mpc_name, mpc->mpc_name) == 0) { - sx_xunlock(&mac_policy_list_lock); + MAC_POLICY_LIST_UNLOCK(); return (EEXIST); } } if (mpc->mpc_field_off) { slot = ffs(mac_policies_free); if (slot == 0) { - sx_xunlock(&mac_policy_list_lock); + MAC_POLICY_LIST_UNLOCK(); return (ENOMEM); } slot--; @@ -505,7 +528,7 @@ /* Per-policy initialization. */ if (mpc->mpc_ops.mpo_init != NULL) (*(mpc->mpc_ops.mpo_init))(mpc); - sx_xunlock(&mac_policy_list_lock); + MAC_POLICY_LIST_UNLOCK(); return (0); } @@ -519,12 +542,16 @@ */ if (mpc->mpc_field_off != -1) return (EBUSY); - sx_xlock(&mac_policy_list_lock); + MAC_POLICY_LIST_LOCK(); + if (mac_policy_list_busy > 0) { + MAC_POLICY_LIST_UNLOCK(); + return (EBUSY); + } if (mpc->mpc_ops.mpo_destroy != NULL) (*(mpc->mpc_ops.mpo_destroy))(mpc); LIST_REMOVE(mpc, mpc_list); - sx_xunlock(&mac_policy_list_lock); + MAC_POLICY_LIST_UNLOCK(); return (0); } To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe p4-projects" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200204171559.g3HFxnJ05061>