diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 3e1d4f5927..5171145c75 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -151,33 +151,6 @@ static const struct int_map chain_priorities[] = { }; -/* - * only one filter update allowed - */ -static virRWLock updateLock; -static bool initialized; - -void -virNWFilterReadLockFilterUpdates(void) -{ - virRWLockRead(&updateLock); -} - - -void -virNWFilterWriteLockFilterUpdates(void) -{ - virRWLockWrite(&updateLock); -} - - -void -virNWFilterUnlockFilterUpdates(void) -{ - virRWLockUnlock(&updateLock); -} - - /* * attribute names for the rules XML */ @@ -3059,6 +3032,7 @@ virNWFilterDefFormat(const virNWFilterDef *def) return virBufferContentAndReset(&buf); } +static bool initialized; static virNWFilterTriggerRebuildCallback rebuildCallback; static void *rebuildOpaque; @@ -3074,9 +3048,6 @@ virNWFilterConfLayerInit(virNWFilterTriggerRebuildCallback cb, initialized = true; - if (virRWLockInit(&updateLock) < 0) - return -1; - return 0; } @@ -3087,8 +3058,6 @@ virNWFilterConfLayerShutdown(void) if (!initialized) return; - virRWLockDestroy(&updateLock); - initialized = false; rebuildCallback = NULL; rebuildOpaque = NULL; diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index bbe12284a5..46867afc8c 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -552,15 +552,6 @@ virNWFilterDefParseString(const char *xml, virNWFilterDef * virNWFilterDefParseFile(const char *filename); -void -virNWFilterWriteLockFilterUpdates(void); - -void -virNWFilterReadLockFilterUpdates(void); - -void -virNWFilterUnlockFilterUpdates(void); - typedef int (*virNWFilterTriggerRebuildCallback)(void *opaque); int diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h index c365d0f28a..cb103280e8 100644 --- a/src/conf/virnwfilterobj.h +++ b/src/conf/virnwfilterobj.h @@ -42,6 +42,9 @@ struct _virNWFilterDriverState { char *stateDir; char *configDir; char *bindingDir; + + /* Recursive. Hold for filter changes, instantiation or deletion */ + virMutex updateLock; }; virNWFilterDef * diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3a8e16525b..03697d81a8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -949,7 +949,6 @@ virNWFilterDeleteDef; virNWFilterJumpTargetTypeToString; virNWFilterPrintStateMatchFlags; virNWFilterPrintTCPFlags; -virNWFilterReadLockFilterUpdates; virNWFilterRuleActionTypeToString; virNWFilterRuleDirectionTypeToString; virNWFilterRuleIsProtocolEthernet; @@ -958,8 +957,6 @@ virNWFilterRuleIsProtocolIPv6; virNWFilterRuleProtocolTypeToString; virNWFilterSaveConfig; virNWFilterTriggerRebuild; -virNWFilterUnlockFilterUpdates; -virNWFilterWriteLockFilterUpdates; # conf/nwfilter_ipaddrmap.h diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index cac73c50e5..1f7d40e1b0 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -166,6 +166,7 @@ nwfilterStateCleanupLocked(void) /* free inactive nwfilters */ virNWFilterObjListFree(driver->nwfilters); + virMutexDestroy(&driver->updateLock); g_clear_pointer(&driver, g_free); return 0; @@ -211,6 +212,9 @@ nwfilterStateInitialize(bool privileged, driver = g_new0(virNWFilterDriverState, 1); driver->lockFD = -1; + if (virMutexInitRecursive(&driver->updateLock) < 0) + goto err_free_driverstate; + driver->privileged = privileged; if (!(driver->nwfilters = virNWFilterObjListNew())) @@ -323,11 +327,10 @@ nwfilterStateReload(void) virNWFilterLearnThreadsTerminate(true); VIR_WITH_MUTEX_LOCK_GUARD(&driverMutex) { - virNWFilterWriteLockFilterUpdates(); + VIR_WITH_MUTEX_LOCK_GUARD(&driver->updateLock) { + virNWFilterObjListLoadAllConfigs(driver->nwfilters, driver->configDir); + } - virNWFilterObjListLoadAllConfigs(driver->nwfilters, driver->configDir); - - virNWFilterUnlockFilterUpdates(); virNWFilterBuildAll(driver, false); } @@ -538,16 +541,16 @@ nwfilterDefineXMLFlags(virConnectPtr conn, return NULL; } - virNWFilterWriteLockFilterUpdates(); - if (!(def = virNWFilterDefParseString(xml, flags))) goto cleanup; if (virNWFilterDefineXMLFlagsEnsureACL(conn, def) < 0) goto cleanup; - if (!(obj = virNWFilterObjListAssignDef(driver->nwfilters, def))) - goto cleanup; + VIR_WITH_MUTEX_LOCK_GUARD(&driver->updateLock) { + if (!(obj = virNWFilterObjListAssignDef(driver->nwfilters, def))) + goto cleanup; + } def = NULL; objdef = virNWFilterObjGetDef(obj); @@ -562,8 +565,6 @@ nwfilterDefineXMLFlags(virConnectPtr conn, virNWFilterDefFree(def); if (obj) virNWFilterObjUnlock(obj); - - virNWFilterUnlockFilterUpdates(); return nwfilter; } @@ -584,34 +585,33 @@ nwfilterUndefine(virNWFilterPtr nwfilter) virNWFilterDef *def; int ret = -1; - virNWFilterWriteLockFilterUpdates(); + VIR_WITH_MUTEX_LOCK_GUARD(&driver->updateLock) { + if (!(obj = nwfilterObjFromNWFilter(nwfilter->uuid))) + goto cleanup; + def = virNWFilterObjGetDef(obj); - if (!(obj = nwfilterObjFromNWFilter(nwfilter->uuid))) - goto cleanup; - def = virNWFilterObjGetDef(obj); + if (virNWFilterUndefineEnsureACL(nwfilter->conn, def) < 0) + goto cleanup; - if (virNWFilterUndefineEnsureACL(nwfilter->conn, def) < 0) - goto cleanup; + if (virNWFilterObjTestUnassignDef(obj) < 0) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", + _("nwfilter is in use")); + goto cleanup; + } - if (virNWFilterObjTestUnassignDef(obj) < 0) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", - _("nwfilter is in use")); - goto cleanup; + if (virNWFilterDeleteDef(driver->configDir, def) < 0) + goto cleanup; + + virNWFilterObjListRemove(driver->nwfilters, obj); + obj = NULL; } - - if (virNWFilterDeleteDef(driver->configDir, def) < 0) - goto cleanup; - - virNWFilterObjListRemove(driver->nwfilters, obj); - obj = NULL; ret = 0; cleanup: if (obj) virNWFilterObjUnlock(obj); - virNWFilterUnlockFilterUpdates(); return ret; } @@ -750,14 +750,14 @@ nwfilterBindingCreateXML(virConnectPtr conn, if (!(ret = virGetNWFilterBinding(conn, def->portdevname, def->filter))) goto cleanup; - virNWFilterReadLockFilterUpdates(); - if (virNWFilterInstantiateFilter(driver, def) < 0) { - virNWFilterUnlockFilterUpdates(); - virNWFilterBindingObjListRemove(driver->bindings, obj); - g_clear_pointer(&ret, virObjectUnref); - goto cleanup; + VIR_WITH_MUTEX_LOCK_GUARD(&driver->updateLock) { + if (virNWFilterInstantiateFilter(driver, def) < 0) { + virNWFilterBindingObjListRemove(driver->bindings, obj); + g_clear_pointer(&ret, virObjectUnref); + goto cleanup; + } } - virNWFilterUnlockFilterUpdates(); + virNWFilterBindingObjSave(obj, driver->bindingDir); cleanup: @@ -794,9 +794,9 @@ nwfilterBindingDelete(virNWFilterBindingPtr binding) if (virNWFilterBindingDeleteEnsureACL(binding->conn, def) < 0) goto cleanup; - virNWFilterReadLockFilterUpdates(); - virNWFilterTeardownFilter(def); - virNWFilterUnlockFilterUpdates(); + VIR_WITH_MUTEX_LOCK_GUARD(&driver->updateLock) { + virNWFilterTeardownFilter(def); + } virNWFilterBindingObjDelete(obj, driver->bindingDir); virNWFilterBindingObjListRemove(driver->bindings, obj); diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 6c6e61524a..7e323afb1a 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -55,56 +55,10 @@ static virNWFilterTechDriver *filter_tech_drivers[] = { NULL }; -/* - * Serializes instantiation of filters. - * - * When instantiating a filter, we need to resolve references - * to other filters and acquire locks on them. - * - * We retain a lock on the referenced filter once found. - * The order in which the locks are acquired depends on - * the order in which filters reference each other. - * - * Filter A: - * Reference Filter C - * Reference Filter D - * - * Filter B: - * Reference Filter D - * Reference Filter C - * - * In one example, we lock A, C, D, in the other example - * we lock A, D, C. - * - * Because C & D are locked in differing orders we are - * once again at risk of deadlocks. - * - * There can be multiple levels of recursion, so it is - * not viable to determine the lock order upfront, it - * has to be done as we traverse the tree. - * - * Thus we serialize any code that needs to traverse - * the filter references. - * - * This covers the following APIs: - * - * virNWFilterDefineXML - * virNWFilterUndefine - * virNWFilterBindingCreate - * virNWFilterBindingDelete - * - * In addition to the asynchronous filter instantiation - * triggered by the IP address learning backends. - */ -static virMutex updateMutex; - int virNWFilterTechDriversInit(bool privileged) { size_t i = 0; VIR_DEBUG("Initializing NWFilter technology drivers"); - if (virMutexInitRecursive(&updateMutex) < 0) - return -1; - while (filter_tech_drivers[i]) { if (!(filter_tech_drivers[i]->flags & TECHDRV_FLAG_INITIALIZED)) filter_tech_drivers[i]->init(privileged); @@ -122,7 +76,6 @@ void virNWFilterTechDriversShutdown(void) filter_tech_drivers[i]->shutdown(); i++; } - virMutexDestroy(&updateMutex); } @@ -744,7 +697,6 @@ virNWFilterInstantiateFilterInternal(virNWFilterDriverState *driver, bool *foundNewFilter) { int ifindex; - VIR_LOCK_GUARD lock = virLockGuardLock(&updateMutex); /* after grabbing the filter update lock check for the interface; if it's not there anymore its filters will be or are being removed @@ -770,11 +722,9 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverState *driver, virNWFilterBindingDef *binding, int ifindex) { - int rc; + int rc = 0; bool foundNewFilter = false; - VIR_LOCK_GUARD lock = virLockGuardLock(&updateMutex); - - virNWFilterReadLockFilterUpdates(); + VIR_LOCK_GUARD lock = virLockGuardLock(&driver->updateLock); rc = virNWFilterInstantiateFilterUpdate(driver, true, binding, ifindex, @@ -790,8 +740,6 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverState *driver, } } - virNWFilterUnlockFilterUpdates(); - return rc; } @@ -912,8 +860,6 @@ _virNWFilterTeardownFilter(const char *ifname) int virNWFilterTeardownFilter(virNWFilterBindingDef *binding) { - VIR_LOCK_GUARD lock = virLockGuardLock(&updateMutex); - return _virNWFilterTeardownFilter(binding->portdevname); }