Push nwfilter update locking up to top level

The NWFilter code has as a deadlock race condition between
the virNWFilter{Define,Undefine} APIs and starting of guest
VMs due to mis-matched lock ordering.

In the virNWFilter{Define,Undefine} codepaths the lock ordering
is

  1. nwfilter driver lock
  2. virt driver lock
  3. nwfilter update lock
  4. domain object lock

In the VM guest startup paths the lock ordering is

  1. virt driver lock
  2. domain object lock
  3. nwfilter update lock

As can be seen the domain object and nwfilter update locks are
not acquired in a consistent order.

The fix used is to push the nwfilter update lock upto the top
level resulting in a lock ordering for virNWFilter{Define,Undefine}
of

  1. nwfilter driver lock
  2. nwfilter update lock
  3. virt driver lock
  4. domain object lock

and VM start using

  1. nwfilter update lock
  2. virt driver lock
  3. domain object lock

This has the effect of serializing VM startup once again, even if
no nwfilters are applied to the guest. There is also the possibility
of deadlock due to a call graph loop via virNWFilterInstantiate
and virNWFilterInstantiateFilterLate.

These two problems mean the lock must be turned into a read/write
lock instead of a plain mutex at the same time. The lock is used to
serialize changes to the "driver->nwfilters" hash, so the write lock
only needs to be held by the define/undefine methods. All other
methods can rely on a read lock which allows good concurrency.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
(cherry picked from commit 6e5c79a1b5a8b3a23e7df7ffe58fb272aa17fbfb)

diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c
index b4971b9c720a4f50a5e07848e6f4aad087a6d6d9..1553bfdfb1a3a44ef1fe02fb47b2fbb168801b03 100644 (file)
--- a/src/conf/nwfilter_conf.c
+++ b/src/conf/nwfilter_conf.c
@@ -143,17 +143,22 @@ static const struct int_map chain_priorities[] = {
 /*
  * only one filter update allowed
  */
-static virMutex updateMutex;
+static virRWLock updateLock;
 static bool initialized = false;
 
 void
-virNWFilterLockFilterUpdates(void) {
-    virMutexLock(&updateMutex);
+virNWFilterReadLockFilterUpdates(void) {
+    virRWLockRead(&updateLock);
+}
+
+void
+virNWFilterWriteLockFilterUpdates(void) {
+    virRWLockWrite(&updateLock);
 }
 
 void
 virNWFilterUnlockFilterUpdates(void) {
-    virMutexUnlock(&updateMutex);
+    virRWLockUnlock(&updateLock);
 }
 
 
@@ -2981,14 +2986,12 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters,
         return NULL;
     }
 
-    virNWFilterLockFilterUpdates();
 
     if ((nwfilter = virNWFilterObjFindByName(nwfilters, def->name))) {
 
         if (virNWFilterDefEqual(def, nwfilter->def, false)) {
             virNWFilterDefFree(nwfilter->def);
             nwfilter->def = def;
-            virNWFilterUnlockFilterUpdates();
             return nwfilter;
         }
 
@@ -2996,7 +2999,6 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters,
         /* trigger the update on VMs referencing the filter */
         if (virNWFilterTriggerVMFilterRebuild()) {
             nwfilter->newDef = NULL;
-            virNWFilterUnlockFilterUpdates();
             virNWFilterObjUnlock(nwfilter);
             return NULL;
         }
@@ -3004,12 +3006,9 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters,
         virNWFilterDefFree(nwfilter->def);
         nwfilter->def = def;
         nwfilter->newDef = NULL;
-        virNWFilterUnlockFilterUpdates();
         return nwfilter;
     }
 
-    virNWFilterUnlockFilterUpdates();
-
     if (VIR_ALLOC(nwfilter) < 0)
         return NULL;
 
@@ -3474,7 +3473,7 @@ int virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB,
 
     initialized = true;
 
-    if (virMutexInitRecursive(&updateMutex) < 0)
+    if (virRWLockInit(&updateLock) < 0)
         return -1;
 
     return 0;
@@ -3486,7 +3485,7 @@ void virNWFilterConfLayerShutdown(void)
     if (!initialized)
         return;
 
-    virMutexDestroy(&updateMutex);
+    virRWLockDestroy(&updateLock);
 
     initialized = false;
     virNWFilterDomainFWUpdateOpaque = NULL;

diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h
index 29906f1389c6920515a2a2ef7732e093943f5e02..d460a0856531c628a49560eae700298ab2d6c34a 100644 (file)
--- a/src/conf/nwfilter_conf.h
+++ b/src/conf/nwfilter_conf.h
@@ -716,7 +716,8 @@ virNWFilterDefPtr virNWFilterDefParseFile(const char *filename);
 void virNWFilterObjLock(virNWFilterObjPtr obj);
 void virNWFilterObjUnlock(virNWFilterObjPtr obj);
 
-void virNWFilterLockFilterUpdates(void);
+void virNWFilterWriteLockFilterUpdates(void);
+void virNWFilterReadLockFilterUpdates(void);
 void virNWFilterUnlockFilterUpdates(void);
 
 int virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB, void *opaque);

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 6915b920e66049cec6a09eb440100d808b40a27e..d25397ce826dc1e754febf9303883df22594fd65 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -567,7 +567,6 @@ virNWFilterDefParseString;
 virNWFilterInstFiltersOnAllVMs;
 virNWFilterJumpTargetTypeToString;
 virNWFilterLoadAllConfigs;
-virNWFilterLockFilterUpdates;
 virNWFilterObjAssignDef;
 virNWFilterObjDeleteDef;
 virNWFilterObjFindByName;
@@ -579,6 +578,7 @@ virNWFilterObjSaveDef;
 virNWFilterObjUnlock;
 virNWFilterPrintStateMatchFlags;
 virNWFilterPrintTCPFlags;
+virNWFilterReadLockFilterUpdates;
 virNWFilterRegisterCallbackDriver;
 virNWFilterRuleActionTypeToString;
 virNWFilterRuleDirectionTypeToString;
@@ -586,6 +586,7 @@ virNWFilterRuleProtocolTypeToString;
 virNWFilterTestUnassignDef;
 virNWFilterUnlockFilterUpdates;
 virNWFilterUnRegisterCallbackDriver;
+virNWFilterWriteLockFilterUpdates;
 
 
 # conf/nwfilter_ipaddrmap.h

diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index fb65782dfa51cd1a68d5e4786999bbca19acd74b..d37962d58e42c439ae7f0c282b602278ee621377 100644 (file)
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -1015,6 +1015,8 @@ static int lxcDomainCreateWithFiles(virDomainPtr dom,
 
     virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, -1);
 
+    virNWFilterReadLockFilterUpdates();
+
     if (!(vm = lxcDomObjFromDomain(dom)))
         goto cleanup;
 
@@ -1053,6 +1055,7 @@ cleanup:
     if (event)
         virDomainEventStateQueue(driver->domainEventState, event);
     virObjectUnref(cfg);
+    virNWFilterUnlockFilterUpdates();
     return ret;
 }
 
@@ -1109,6 +1112,8 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn,
 
     virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, NULL);
 
+    virNWFilterReadLockFilterUpdates();
+
     if (!(caps = virLXCDriverGetCapabilities(driver, false)))
         goto cleanup;
 
@@ -1164,6 +1169,7 @@ cleanup:
         virDomainEventStateQueue(driver->domainEventState, event);
     virObjectUnref(caps);
     virObjectUnref(cfg);
+    virNWFilterUnlockFilterUpdates();
     return dom;
 }
 

diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
index 0a4d60546895f8992aee2b8162a20b005669ab82..df3a143885060250da0b95d260989f38b95c7c1f 100644 (file)
--- a/src/nwfilter/nwfilter_driver.c
+++ b/src/nwfilter/nwfilter_driver.c
@@ -284,12 +284,14 @@ nwfilterStateReload(void)
     virNWFilterLearnThreadsTerminate(true);
 
     nwfilterDriverLock(driverState);
+    virNWFilterWriteLockFilterUpdates();
     virNWFilterCallbackDriversLock();
 
     virNWFilterLoadAllConfigs(&driverState->nwfilters,
                               driverState->configDir);
 
     virNWFilterCallbackDriversUnlock();
+    virNWFilterUnlockFilterUpdates();
     nwfilterDriverUnlock(driverState);
 
     virNWFilterInstFiltersOnAllVMs();
@@ -555,6 +557,7 @@ nwfilterDefineXML(virConnectPtr conn,
     virNWFilterPtr ret = NULL;
 
     nwfilterDriverLock(driver);
+    virNWFilterWriteLockFilterUpdates();
     virNWFilterCallbackDriversLock();
 
     if (!(def = virNWFilterDefParseString(xml)))
@@ -581,6 +584,7 @@ cleanup:
         virNWFilterObjUnlock(nwfilter);
 
     virNWFilterCallbackDriversUnlock();
+    virNWFilterUnlockFilterUpdates();
     nwfilterDriverUnlock(driver);
     return ret;
 }
@@ -593,10 +597,9 @@ nwfilterUndefine(virNWFilterPtr obj) {
     int ret = -1;
 
     nwfilterDriverLock(driver);
+    virNWFilterWriteLockFilterUpdates();
     virNWFilterCallbackDriversLock();
 
-    virNWFilterLockFilterUpdates();
-
     nwfilter = virNWFilterObjFindByUUID(&driver->nwfilters, obj->uuid);
     if (!nwfilter) {
         virReportError(VIR_ERR_NO_NWFILTER,
@@ -627,9 +630,8 @@ cleanup:
     if (nwfilter)
         virNWFilterObjUnlock(nwfilter);
 
-    virNWFilterUnlockFilterUpdates();
-
     virNWFilterCallbackDriversUnlock();
+    virNWFilterUnlockFilterUpdates();
     nwfilterDriverUnlock(driver);
     return ret;
 }

diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c
index f517569b09f08ff6435c437caeec63af699214cb..203ba0e0e00cb910dd150d418e38361c829d80fd 100644 (file)
--- a/src/nwfilter/nwfilter_gentech_driver.c
+++ b/src/nwfilter/nwfilter_gentech_driver.c
@@ -934,8 +934,6 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver,
     int ifindex;
     int rc;
 
-    virNWFilterLockFilterUpdates();
-
     /* after grabbing the filter update lock check for the interface; if
        it's not there anymore its filters will be or are being removed
        (while holding the lock) and we don't want to build new ones */
@@ -963,8 +961,6 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver,
                                         foundNewFilter);
 
 cleanup:
-    virNWFilterUnlockFilterUpdates();
-
     return rc;
 }
 
@@ -983,7 +979,7 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver,
     int rc;
     bool foundNewFilter = false;
 
-    virNWFilterLockFilterUpdates();
+    virNWFilterReadLockFilterUpdates();
 
     rc = __virNWFilterInstantiateFilter(driver,
                                         vmuuid,

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c2c6eb4e5d2f7c4a054b611ebc8a70b50efb916d..f52647be7b4269d0903ba7c4a192388d50a2bfd6 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1570,6 +1570,8 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn,
     if (flags & VIR_DOMAIN_START_AUTODESTROY)
         start_flags |= VIR_QEMU_PROCESS_START_AUTODESTROY;
 
+    virNWFilterReadLockFilterUpdates();
+
     if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
         goto cleanup;
 
@@ -1647,6 +1649,7 @@ cleanup:
     }
     virObjectUnref(caps);
     virObjectUnref(qemuCaps);
+    virNWFilterUnlockFilterUpdates();
     return dom;
 }
 
@@ -6040,6 +6043,8 @@ qemuDomainCreateWithFlags(virDomainPtr dom, unsigned int flags)
                   VIR_DOMAIN_START_BYPASS_CACHE |
                   VIR_DOMAIN_START_FORCE_BOOT, -1);
 
+    virNWFilterReadLockFilterUpdates();
+
     if (!(vm = qemuDomObjFromDomain(dom)))
         return -1;
 
@@ -6067,6 +6072,7 @@ endjob:
 cleanup:
     if (vm)
         virObjectUnlock(vm);
+    virNWFilterUnlockFilterUpdates();
     return ret;
 }
 

diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
index eb0254209f06895fc3ba0ca22e69c967e7252e23..b580b324a517c1670571dd7f597d1e945c9d5bd3 100644 (file)
--- a/src/uml/uml_driver.c
+++ b/src/uml/uml_driver.c
@@ -1574,6 +1574,7 @@ static virDomainPtr umlDomainCreateXML(virConnectPtr conn, const char *xml,
 
     virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, NULL);
 
+    virNWFilterReadLockFilterUpdates();
     umlDriverLock(driver);
     if (!(def = virDomainDefParseString(xml, driver->caps, driver->xmlopt,
                                         1 << VIR_DOMAIN_VIRT_UML,
@@ -1613,6 +1614,7 @@ cleanup:
     if (event)
         umlDomainEventQueue(driver, event);
     umlDriverUnlock(driver);
+    virNWFilterUnlockFilterUpdates();
     return dom;
 }
 
@@ -1997,6 +1999,7 @@ static int umlDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) {
 
     virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, -1);
 
+    virNWFilterReadLockFilterUpdates();
     umlDriverLock(driver);
     vm = virDomainObjListFindByUUID(driver->domains, dom->uuid);
 
@@ -2023,6 +2026,7 @@ cleanup:
     if (event)
         umlDomainEventQueue(driver, event);
     umlDriverUnlock(driver);
+    virNWFilterUnlockFilterUpdates();
     return ret;
 }