Add a mutex to serialize updates to firewall

The nwfilter conf update mutex previously serialized
updates to the internal data structures for firewall
rules, and updates to the firewall itself. The latter
was recently turned into a read/write lock, and filter
instantiation allowed to proceed in parallel. It was
believed that this was ok, since each filter is created
on a separate iptables/ebtables chain.

It turns out that there is a subtle lock ordering problem
on virNWFilterObjPtr instances. __virNWFilterInstantiateFilter
will hold a lock on the virNWFilterObjPtr it is instantiating.
This in turn invokes virNWFilterInstantiate which then invokes
virNWFilterDetermineMissingVarsRec which then invokes
virNWFilterObjFindByName. This iterates over every single
virNWFilterObjPtr in the list, locking them and checking their
name. So if 2 or more threads try to instantiate a filter in
parallel, they'll all hold 1 lock at the top level in the
__virNWFilterInstantiateFilter method which will cause the
other thread to deadlock in virNWFilterObjFindByName.

The fix is to add an exclusive mutex to serialize the
execution of __virNWFilterInstantiateFilter.

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

diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
index 5908df723c4c5e44d70e899623ac5c4d9068f0db..2e89d07b64127d936e32267d2e6bb3a732209873 100644 (file)
--- a/src/nwfilter/nwfilter_driver.c
+++ b/src/nwfilter/nwfilter_driver.c
@@ -200,7 +200,8 @@ nwfilterStateInitialize(bool privileged,
     if (virNWFilterDHCPSnoopInit() < 0)
         goto err_exit_learnshutdown;
 
-    virNWFilterTechDriversInit(privileged);
+    if (virNWFilterTechDriversInit(privileged) < 0)
+        goto err_dhcpsnoop_shutdown;
 
     if (virNWFilterConfLayerInit(virNWFilterDomainFWUpdateCB,
                                  driverState) < 0)
@@ -251,6 +252,7 @@ error:
 
 err_techdrivers_shutdown:
     virNWFilterTechDriversShutdown();
+err_dhcpsnoop_shutdown:
     virNWFilterDHCPSnoopShutdown();
 err_exit_learnshutdown:
     virNWFilterLearnShutdown();
@@ -327,10 +329,10 @@ nwfilterStateCleanup(void) {
 
     if (driverState->privileged) {
         virNWFilterConfLayerShutdown();
-        virNWFilterTechDriversShutdown();
         virNWFilterDHCPSnoopShutdown();
         virNWFilterLearnShutdown();
         virNWFilterIPAddrMapShutdown();
+        virNWFilterTechDriversShutdown();
 
         nwfilterDriverLock(driverState);
 

diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c
index 8c5cd57b613395556ebf085c830ed4e81c7e503f..5b1fac45b3fea4fb78330d0c2624ee273e62bfcd 100644 (file)
--- a/src/nwfilter/nwfilter_gentech_driver.c
+++ b/src/nwfilter/nwfilter_gentech_driver.c
@@ -55,30 +55,53 @@ static virNWFilterTechDriverPtr filter_tech_drivers[] = {
     NULL
 };
 
+/* Serializes instantiation of filters. This is necessary
+ * to avoid lock ordering deadlocks. eg __virNWFilterInstantiateFilter
+ * will hold a lock on a virNWFilterObjPtr. This in turn invokes
+ * virNWFilterInstantiate which invokes virNWFilterDetermineMissingVarsRec
+ * which invokes virNWFilterObjFindByName. This iterates over every single
+ * virNWFilterObjPtr in the list. So if 2 threads try to instantiate a
+ * filter in parallel, they'll both hold 1 lock at the top level in
+ * __virNWFilterInstantiateFilter which will cause the other thread
+ * to deadlock in virNWFilterObjFindByName.
+ *
+ * XXX better long term solution is to make virNWFilterObjList use a
+ * hash table as is done for virDomainObjList. You can then get
+ * lockless lookup of objects by name.
+ */
+static virMutex updateMutex;
 
-void virNWFilterTechDriversInit(bool privileged) {
+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);
         i++;
     }
+    return 0;
 }
 
 
-void virNWFilterTechDriversShutdown(void) {
+void virNWFilterTechDriversShutdown(void)
+{
     size_t i = 0;
     while (filter_tech_drivers[i]) {
         if ((filter_tech_drivers[i]->flags & TECHDRV_FLAG_INITIALIZED))
             filter_tech_drivers[i]->shutdown();
         i++;
     }
+    virMutexDestroy(&updateMutex);
 }
 
 
 virNWFilterTechDriverPtr
-virNWFilterTechDriverForName(const char *name) {
+virNWFilterTechDriverForName(const char *name)
+{
     size_t i = 0;
     while (filter_tech_drivers[i]) {
        if (STREQ(filter_tech_drivers[i]->name, name)) {
@@ -935,6 +958,8 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver,
     int ifindex;
     int rc;
 
+    virMutexLock(&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
        (while holding the lock) and we don't want to build new ones */
@@ -962,6 +987,8 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver,
                                         foundNewFilter);
 
 cleanup:
+    virMutexUnlock(&updateMutex);
+
     return rc;
 }
 
@@ -981,6 +1008,7 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver,
     bool foundNewFilter = false;
 
     virNWFilterReadLockFilterUpdates();
+    virMutexLock(&updateMutex);
 
     rc = __virNWFilterInstantiateFilter(driver,
                                         vmuuid,
@@ -1006,6 +1034,7 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver,
     }
 
     virNWFilterUnlockFilterUpdates();
+    virMutexUnlock(&updateMutex);
 
     return rc;
 }
@@ -1129,7 +1158,11 @@ _virNWFilterTeardownFilter(const char *ifname)
 int
 virNWFilterTeardownFilter(const virDomainNetDef *net)
 {
-    return _virNWFilterTeardownFilter(net->ifname);
+    int ret;
+    virMutexLock(&updateMutex);
+    ret = _virNWFilterTeardownFilter(net->ifname);
+    virMutexUnlock(&updateMutex);
+    return ret;
 }
 
 

diff --git a/src/nwfilter/nwfilter_gentech_driver.h b/src/nwfilter/nwfilter_gentech_driver.h
index f4789e191b6d5a7edeb61f99ef5e2b9e1243b8a6..d72e0407253d45bdfb97889787f9190ab6ad8d71 100644 (file)
--- a/src/nwfilter/nwfilter_gentech_driver.h
+++ b/src/nwfilter/nwfilter_gentech_driver.h
@@ -31,7 +31,7 @@ virNWFilterTechDriverPtr virNWFilterTechDriverForName(const char *name);
 int virNWFilterRuleInstAddData(virNWFilterRuleInstPtr res,
                                void *data);
 
-void virNWFilterTechDriversInit(bool privileged);
+int virNWFilterTechDriversInit(bool privileged);
 void virNWFilterTechDriversShutdown(void);
 
 enum instCase {