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)

Conflicts:
	src/conf/nwfilter_conf.c
          - virReportOOMError() in context of one hunk.
	src/lxc/lxc_driver.c
          - functions renamed, and lxc object locking changed, creating
            a conflict in the context.

diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c
index 0eaf00054fd53373011123c6a0fff90d97b3d55b..ae10258e7abd51551ac507ec0ff8b9f12323ef49 100644 (file)
--- a/src/conf/nwfilter_conf.c
+++ b/src/conf/nwfilter_conf.c
@@ -2,7 +2,7 @@
  * nwfilter_conf.c: network filter XML processing
  *                  (derived from storage_conf.c)
  *
- * Copyright (C) 2006-2012 Red Hat, Inc.
+ * Copyright (C) 2006-2012, 2014 Red Hat, Inc.
  * Copyright (C) 2006-2008 Daniel P. Berrange
  *
  * Copyright (C) 2010-2011 IBM Corporation
@@ -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);
 }
 
 
@@ -2999,14 +3004,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;
         }
 
@@ -3014,7 +3017,6 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters,
         /* trigger the update on VMs referencing the filter */
         if (virNWFilterTriggerVMFilterRebuild()) {
             nwfilter->newDef = NULL;
-            virNWFilterUnlockFilterUpdates();
             virNWFilterObjUnlock(nwfilter);
             return NULL;
         }
@@ -3022,12 +3024,9 @@ virNWFilterObjAssignDef(virNWFilterObjListPtr nwfilters,
         virNWFilterDefFree(nwfilter->def);
         nwfilter->def = def;
         nwfilter->newDef = NULL;
-        virNWFilterUnlockFilterUpdates();
         return nwfilter;
     }
 
-    virNWFilterUnlockFilterUpdates();
-
     if (VIR_ALLOC(nwfilter) < 0) {
         virReportOOMError();
         return NULL;
@@ -3501,7 +3500,7 @@ int virNWFilterConfLayerInit(virDomainObjListIterator domUpdateCB,
 
     initialized = true;
 
-    if (virMutexInitRecursive(&updateMutex) < 0)
+    if (virRWLockInit(&updateLock) < 0)
         return -1;
 
     return 0;
@@ -3513,7 +3512,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 1b72ba8cba4074d071c5a7c26045da695cd340c2..359244ffce429055a19d5bb35ebbcf374147d4fd 100644 (file)
--- a/src/conf/nwfilter_conf.h
+++ b/src/conf/nwfilter_conf.h
@@ -717,7 +717,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 4cd5f6bd80b7f780a5973589cd64b22fea5329fb..4c95334a1787d3f27e6051eb17b0b98317d68b2f 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -510,7 +510,6 @@ virNWFilterDefParseString;
 virNWFilterInstFiltersOnAllVMs;
 virNWFilterJumpTargetTypeToString;
 virNWFilterLoadAllConfigs;
-virNWFilterLockFilterUpdates;
 virNWFilterObjAssignDef;
 virNWFilterObjDeleteDef;
 virNWFilterObjFindByName;
@@ -522,6 +521,7 @@ virNWFilterObjSaveDef;
 virNWFilterObjUnlock;
 virNWFilterPrintStateMatchFlags;
 virNWFilterPrintTCPFlags;
+virNWFilterReadLockFilterUpdates;
 virNWFilterRegisterCallbackDriver;
 virNWFilterRuleActionTypeToString;
 virNWFilterRuleDirectionTypeToString;
@@ -529,6 +529,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 bd71e808bc172d826c0c5bc576a92ee010fe1000..1b8b22d5ffef6268d602a49913d4d7cc2ec09bca 100644 (file)
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2010-2013 Red Hat, Inc.
+ * Copyright (C) 2010-2014 Red Hat, Inc.
  * Copyright IBM Corp. 2008
  *
  * lxc_driver.c: linux container driver functions
@@ -1034,6 +1034,8 @@ static int lxcDomainCreateWithFlags(virDomainPtr dom, unsigned int flags)
 
     virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, -1);
 
+    virNWFilterReadLockFilterUpdates();
+
     lxcDriverLock(driver);
     vm = virDomainObjListFindByUUID(driver->domains, dom->uuid);
     if (!vm) {
@@ -1075,6 +1077,7 @@ cleanup:
     if (event)
         virDomainEventStateQueue(driver->domainEventState, event);
     lxcDriverUnlock(driver);
+    virNWFilterUnlockFilterUpdates();
     return ret;
 }
 
@@ -1113,6 +1116,8 @@ lxcDomainCreateXML(virConnectPtr conn,
 
     virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, NULL);
 
+    virNWFilterReadLockFilterUpdates();
+
     lxcDriverLock(driver);
     if (!(def = virDomainDefParseString(xml, driver->caps, driver->xmlopt,
                                         1 << VIR_DOMAIN_VIRT_LXC,
@@ -1161,6 +1166,7 @@ cleanup:
     if (event)
         virDomainEventStateQueue(driver->domainEventState, event);
     lxcDriverUnlock(driver);
+    virNWFilterUnlockFilterUpdates();
     return dom;
 }
 

diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
index 0659b214807f9c5e009714b442f03f11eb04fc46..ad3824e61d58ef3dc025e501862da942f82e20ef 100644 (file)
--- a/src/nwfilter/nwfilter_driver.c
+++ b/src/nwfilter/nwfilter_driver.c
@@ -282,12 +282,14 @@ nwfilterStateReload(void)
     virNWFilterLearnThreadsTerminate(true);
 
     nwfilterDriverLock(driverState);
+    virNWFilterWriteLockFilterUpdates();
     virNWFilterCallbackDriversLock();
 
     virNWFilterLoadAllConfigs(&driverState->nwfilters,
                               driverState->configDir);
 
     virNWFilterCallbackDriversUnlock();
+    virNWFilterUnlockFilterUpdates();
     nwfilterDriverUnlock(driverState);
 
     virNWFilterInstFiltersOnAllVMs();
@@ -523,6 +525,7 @@ nwfilterDefineXML(virConnectPtr conn,
     virNWFilterPtr ret = NULL;
 
     nwfilterDriverLock(driver);
+    virNWFilterWriteLockFilterUpdates();
     virNWFilterCallbackDriversLock();
 
     if (!(def = virNWFilterDefParseString(xml)))
@@ -546,6 +549,7 @@ cleanup:
         virNWFilterObjUnlock(nwfilter);
 
     virNWFilterCallbackDriversUnlock();
+    virNWFilterUnlockFilterUpdates();
     nwfilterDriverUnlock(driver);
     return ret;
 }
@@ -558,10 +562,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,
@@ -589,9 +592,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 5323e455dbb7735a4806f16d2dd7ed0c7ecc4e1b..0aa0f0caaca39c6773bc1be179d5a91cbca63093 100644 (file)
--- a/src/nwfilter/nwfilter_gentech_driver.c
+++ b/src/nwfilter/nwfilter_gentech_driver.c
@@ -948,8 +948,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 */
@@ -977,8 +975,6 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver,
                                         foundNewFilter);
 
 cleanup:
-    virNWFilterUnlockFilterUpdates();
-
     return rc;
 }
 
@@ -997,7 +993,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 4de4e953d0045d68ab0bdea9316ecc391da1a8e5..e9408f2b690f5e6fd622517df128b312089cea2a 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1479,6 +1479,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;
 
@@ -1553,6 +1555,7 @@ cleanup:
     }
     virObjectUnref(caps);
     virObjectUnref(qemuCaps);
+    virNWFilterUnlockFilterUpdates();
     return dom;
 }
 
@@ -5431,6 +5434,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;
 
@@ -5455,6 +5460,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 61586b0f835ca71d9ab7aaaea45aeaa265ef7f1c..8c487003103d15c0c26e84d93c567505e423b9b9 100644 (file)
--- a/src/uml/uml_driver.c
+++ b/src/uml/uml_driver.c
@@ -1523,6 +1523,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,
@@ -1559,6 +1560,7 @@ cleanup:
     if (event)
         umlDomainEventQueue(driver, event);
     umlDriverUnlock(driver);
+    virNWFilterUnlockFilterUpdates();
     return dom;
 }
 
@@ -1907,6 +1909,7 @@ static int umlDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) {
 
     virCheckFlags(VIR_DOMAIN_START_AUTODESTROY, -1);
 
+    virNWFilterReadLockFilterUpdates();
     umlDriverLock(driver);
     vm = virDomainObjListFindByUUID(driver->domains, dom->uuid);
 
@@ -1930,6 +1933,7 @@ cleanup:
     if (event)
         umlDomainEventQueue(driver, event);
     umlDriverUnlock(driver);
+    virNWFilterUnlockFilterUpdates();
     return ret;
 }