nwfilter: merge updateMutex and updateLock

The updateLock is a R/W lock held by anything which needs to read or
modify the rules associated with an NWFilter.

APIs for defining/undefining NW filters rules hold a write lock on
updateLock.

APIs for creating/deleting NW filter bindings hold a read lock on
updateLock, which prevents define/undefine taking place concurrently.

The problems arise when we attempt to creating two NW filter bindings in
parallel.

Thread 1 can acquire the mutex for filter A

Thread 2 can acquire the mutex for filter B

Consider if filters A and B both reference filters C and D, but in
different orders:

  Filter A
     -> filter C
     -> filter D

  Filter B
     -> filter D
     -> filter C

Thread 1 will try to acquire locks in order A, C, D while thread 1 will
try to acquire in order A, D, C. Deadlock can still occur.

Think we can sort the list of filters before acquiring locks on all of
them ? Nope, we allow arbitrary recursion:

  Filter A
     -> filter C
          -> filter E
             -> filter F
                -> filter H
                -> filter K
     -> filter D
         -> filter G
         -> filter I

So we can't tell from looking at 'A' which filters we're going to
need to lock. We can only see the first level of filters references
and we need to lock those before we can see the second level of
filters, etc.

We could probably come up with some cleverness to address this but
it isn't worth the time investment. It is simpler to just keep the
process of creating NW filter bindings totally serialized.

Using two separate locks for this serialization though is pointless.

Every code path which gets a read(updateLock) will go on to hold
updateMutex. It is simpler to just hold write(updateLock) and
get rid of updateMutex. At that point we don't need updateLock
to be a R/W lock, it can be a plain mutex.

Thus this patch gets rid of the current updateLock and updateMutex
and introduces a new top level updateMutex.

This has a secondary benefit of introducing fairness into the
locking.  With a POSIX R/W lock, you get writer starvation if
you have lots of readers. IOW, if we call virNWFilterBIndingCreate
and virNWFilterBindingDelete in a tight loop from a couple of
threads, we can prevent virNWFilterDefine from ever acquiring
a write lock.

Getting rid of the R/W lock gives us FIFO lock acquisition
preventing starvation of any API call servicing.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c
index 3e1d4f5927db9f00c4903684ed4428080ace6950..5171145c750a2d46632a2e7371447af3775c071f 100644 (file)
--- 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 bbe12284a55debfc90c641233fd180c133402e70..46867afc8c5f4867ca190690995c2a7244fe8725 100644 (file)
--- 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 c365d0f28af9b15398cc440ffa039138befd2478..cb103280e8edd58a34fce33fa1eb08423464e4c5 100644 (file)
--- 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 3a8e16525bd3c624f074a1f5375a6ea2953b0692..03697d81a8eb17eae86e9d6342271e06f524f582 100644 (file)
--- 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 cac73c50e575ad9908dc6c80154e3e19e979e512..1f7d40e1b0587eea9f0bd54b718e9bd7b9b93622 100644 (file)
--- 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();
-
-        virNWFilterObjListLoadAllConfigs(driver->nwfilters, driver->configDir);
+        VIR_WITH_MUTEX_LOCK_GUARD(&driver->updateLock) {
+            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;
 
-    if (virNWFilterDeleteDef(driver->configDir, def) < 0)
-        goto cleanup;
-
-    virNWFilterObjListRemove(driver->nwfilters, obj);
-    obj = NULL;
+        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 6c6e61524aeb34abcc576427015d8185c2daafe8..7e323afb1ab53d381a44d3e3e20921b918f109a2 100644 (file)
--- 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);
 }