render / rpms / libvirt

Forked from rpms/libvirt 10 months ago
Clone
c401cc
From 544908ecc824823a5e7340bede05c5bee77b20f6 Mon Sep 17 00:00:00 2001
c401cc
Message-Id: <544908ecc824823a5e7340bede05c5bee77b20f6@dist-git>
c401cc
From: "Daniel P. Berrange" <berrange@redhat.com>
c401cc
Date: Mon, 10 Mar 2014 15:05:32 +0000
c401cc
Subject: [PATCH] Add a mutex to serialize updates to firewall
c401cc
c401cc
For
c401cc
c401cc
  https://bugzilla.redhat.com/show_bug.cgi?id=1074003
c401cc
c401cc
The nwfilter conf update mutex previously serialized
c401cc
updates to the internal data structures for firewall
c401cc
rules, and updates to the firewall itself. The latter
c401cc
was recently turned into a read/write lock, and filter
c401cc
instantiation allowed to proceed in parallel. It was
c401cc
believed that this was ok, since each filter is created
c401cc
on a separate iptables/ebtables chain.
c401cc
c401cc
It turns out that there is a subtle lock ordering problem
c401cc
on virNWFilterObjPtr instances. __virNWFilterInstantiateFilter
c401cc
will hold a lock on the virNWFilterObjPtr it is instantiating.
c401cc
This in turn invokes virNWFilterInstantiate which then invokes
c401cc
virNWFilterDetermineMissingVarsRec which then invokes
c401cc
virNWFilterObjFindByName. This iterates over every single
c401cc
virNWFilterObjPtr in the list, locking them and checking their
c401cc
name. So if 2 or more threads try to instantiate a filter in
c401cc
parallel, they'll all hold 1 lock at the top level in the
c401cc
__virNWFilterInstantiateFilter method which will cause the
c401cc
other thread to deadlock in virNWFilterObjFindByName.
c401cc
c401cc
The fix is to add an exclusive mutex to serialize the
c401cc
execution of __virNWFilterInstantiateFilter.
c401cc
c401cc
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
c401cc
(cherry picked from commit 925de19ed7f13e0d12d0b993496d314bab886589)
c401cc
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
c401cc
---
c401cc
 src/nwfilter/nwfilter_driver.c         |  6 +++--
c401cc
 src/nwfilter/nwfilter_gentech_driver.c | 41 ++++++++++++++++++++++++++++++----
c401cc
 src/nwfilter/nwfilter_gentech_driver.h |  2 +-
c401cc
 3 files changed, 42 insertions(+), 7 deletions(-)
c401cc
c401cc
diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
c401cc
index 77fc608..b1bdd3f 100644
c401cc
--- a/src/nwfilter/nwfilter_driver.c
c401cc
+++ b/src/nwfilter/nwfilter_driver.c
c401cc
@@ -202,7 +202,8 @@ nwfilterStateInitialize(bool privileged,
c401cc
     if (virNWFilterDHCPSnoopInit() < 0)
c401cc
         goto err_exit_learnshutdown;
c401cc
 
c401cc
-    virNWFilterTechDriversInit(privileged);
c401cc
+    if (virNWFilterTechDriversInit(privileged) < 0)
c401cc
+        goto err_dhcpsnoop_shutdown;
c401cc
 
c401cc
     if (virNWFilterConfLayerInit(virNWFilterDomainFWUpdateCB,
c401cc
                                  driverState) < 0)
c401cc
@@ -253,6 +254,7 @@ error:
c401cc
 
c401cc
 err_techdrivers_shutdown:
c401cc
     virNWFilterTechDriversShutdown();
c401cc
+err_dhcpsnoop_shutdown:
c401cc
     virNWFilterDHCPSnoopShutdown();
c401cc
 err_exit_learnshutdown:
c401cc
     virNWFilterLearnShutdown();
c401cc
@@ -329,10 +331,10 @@ nwfilterStateCleanup(void) {
c401cc
 
c401cc
     if (driverState->privileged) {
c401cc
         virNWFilterConfLayerShutdown();
c401cc
-        virNWFilterTechDriversShutdown();
c401cc
         virNWFilterDHCPSnoopShutdown();
c401cc
         virNWFilterLearnShutdown();
c401cc
         virNWFilterIPAddrMapShutdown();
c401cc
+        virNWFilterTechDriversShutdown();
c401cc
 
c401cc
         nwfilterDriverLock(driverState);
c401cc
 
c401cc
diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c
c401cc
index 203ba0e..21b38ae 100644
c401cc
--- a/src/nwfilter/nwfilter_gentech_driver.c
c401cc
+++ b/src/nwfilter/nwfilter_gentech_driver.c
c401cc
@@ -55,30 +55,53 @@ static virNWFilterTechDriverPtr filter_tech_drivers[] = {
c401cc
     NULL
c401cc
 };
c401cc
 
c401cc
+/* Serializes instantiation of filters. This is necessary
c401cc
+ * to avoid lock ordering deadlocks. eg __virNWFilterInstantiateFilter
c401cc
+ * will hold a lock on a virNWFilterObjPtr. This in turn invokes
c401cc
+ * virNWFilterInstantiate which invokes virNWFilterDetermineMissingVarsRec
c401cc
+ * which invokes virNWFilterObjFindByName. This iterates over every single
c401cc
+ * virNWFilterObjPtr in the list. So if 2 threads try to instantiate a
c401cc
+ * filter in parallel, they'll both hold 1 lock at the top level in
c401cc
+ * __virNWFilterInstantiateFilter which will cause the other thread
c401cc
+ * to deadlock in virNWFilterObjFindByName.
c401cc
+ *
c401cc
+ * XXX better long term solution is to make virNWFilterObjList use a
c401cc
+ * hash table as is done for virDomainObjList. You can then get
c401cc
+ * lockless lookup of objects by name.
c401cc
+ */
c401cc
+static virMutex updateMutex;
c401cc
 
c401cc
-void virNWFilterTechDriversInit(bool privileged) {
c401cc
+int virNWFilterTechDriversInit(bool privileged)
c401cc
+{
c401cc
     size_t i = 0;
c401cc
     VIR_DEBUG("Initializing NWFilter technology drivers");
c401cc
+    if (virMutexInitRecursive(&updateMutex) < 0)
c401cc
+        return -1;
c401cc
+
c401cc
     while (filter_tech_drivers[i]) {
c401cc
         if (!(filter_tech_drivers[i]->flags & TECHDRV_FLAG_INITIALIZED))
c401cc
             filter_tech_drivers[i]->init(privileged);
c401cc
         i++;
c401cc
     }
c401cc
+    return 0;
c401cc
 }
c401cc
 
c401cc
 
c401cc
-void virNWFilterTechDriversShutdown(void) {
c401cc
+void virNWFilterTechDriversShutdown(void)
c401cc
+{
c401cc
     size_t i = 0;
c401cc
     while (filter_tech_drivers[i]) {
c401cc
         if ((filter_tech_drivers[i]->flags & TECHDRV_FLAG_INITIALIZED))
c401cc
             filter_tech_drivers[i]->shutdown();
c401cc
         i++;
c401cc
     }
c401cc
+    virMutexDestroy(&updateMutex);
c401cc
 }
c401cc
 
c401cc
 
c401cc
 virNWFilterTechDriverPtr
c401cc
-virNWFilterTechDriverForName(const char *name) {
c401cc
+virNWFilterTechDriverForName(const char *name)
c401cc
+{
c401cc
     size_t i = 0;
c401cc
     while (filter_tech_drivers[i]) {
c401cc
        if (STREQ(filter_tech_drivers[i]->name, name)) {
c401cc
@@ -934,6 +957,8 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver,
c401cc
     int ifindex;
c401cc
     int rc;
c401cc
 
c401cc
+    virMutexLock(&updateMutex);
c401cc
+
c401cc
     /* after grabbing the filter update lock check for the interface; if
c401cc
        it's not there anymore its filters will be or are being removed
c401cc
        (while holding the lock) and we don't want to build new ones */
c401cc
@@ -961,6 +986,8 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver,
c401cc
                                         foundNewFilter);
c401cc
 
c401cc
 cleanup:
c401cc
+    virMutexUnlock(&updateMutex);
c401cc
+
c401cc
     return rc;
c401cc
 }
c401cc
 
c401cc
@@ -980,6 +1007,7 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver,
c401cc
     bool foundNewFilter = false;
c401cc
 
c401cc
     virNWFilterReadLockFilterUpdates();
c401cc
+    virMutexLock(&updateMutex);
c401cc
 
c401cc
     rc = __virNWFilterInstantiateFilter(driver,
c401cc
                                         vmuuid,
c401cc
@@ -1005,6 +1033,7 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver,
c401cc
     }
c401cc
 
c401cc
     virNWFilterUnlockFilterUpdates();
c401cc
+    virMutexUnlock(&updateMutex);
c401cc
 
c401cc
     return rc;
c401cc
 }
c401cc
@@ -1128,7 +1157,11 @@ _virNWFilterTeardownFilter(const char *ifname)
c401cc
 int
c401cc
 virNWFilterTeardownFilter(const virDomainNetDefPtr net)
c401cc
 {
c401cc
-    return _virNWFilterTeardownFilter(net->ifname);
c401cc
+    int ret;
c401cc
+    virMutexLock(&updateMutex);
c401cc
+    ret = _virNWFilterTeardownFilter(net->ifname);
c401cc
+    virMutexUnlock(&updateMutex);
c401cc
+    return ret;
c401cc
 }
c401cc
 
c401cc
 
c401cc
diff --git a/src/nwfilter/nwfilter_gentech_driver.h b/src/nwfilter/nwfilter_gentech_driver.h
c401cc
index 8528e2a..48ee1df 100644
c401cc
--- a/src/nwfilter/nwfilter_gentech_driver.h
c401cc
+++ b/src/nwfilter/nwfilter_gentech_driver.h
c401cc
@@ -30,7 +30,7 @@ virNWFilterTechDriverPtr virNWFilterTechDriverForName(const char *name);
c401cc
 int virNWFilterRuleInstAddData(virNWFilterRuleInstPtr res,
c401cc
                                void *data);
c401cc
 
c401cc
-void virNWFilterTechDriversInit(bool privileged);
c401cc
+int virNWFilterTechDriversInit(bool privileged);
c401cc
 void virNWFilterTechDriversShutdown(void);
c401cc
 
c401cc
 enum instCase {
c401cc
-- 
c401cc
1.9.0
c401cc