From 544908ecc824823a5e7340bede05c5bee77b20f6 Mon Sep 17 00:00:00 2001

Message-Id: <544908ecc824823a5e7340bede05c5bee77b20f6@dist-git>

From: "Daniel P. Berrange" <berrange@redhat.com>

Date: Mon, 10 Mar 2014 15:05:32 +0000

Subject: [PATCH] Add a mutex to serialize updates to firewall

For

  https://bugzilla.redhat.com/show_bug.cgi?id=1074003

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>

(cherry picked from commit 925de19ed7f13e0d12d0b993496d314bab886589)

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>

---

 src/nwfilter/nwfilter_driver.c         |  6 +++--

 src/nwfilter/nwfilter_gentech_driver.c | 41 ++++++++++++++++++++++++++++++----

 src/nwfilter/nwfilter_gentech_driver.h |  2 +-

 3 files changed, 42 insertions(+), 7 deletions(-)

diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c

index 77fc608..b1bdd3f 100644

--- a/src/nwfilter/nwfilter_driver.c

+++ b/src/nwfilter/nwfilter_driver.c

@@ -202,7 +202,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)

@@ -253,6 +254,7 @@ error:

 err_techdrivers_shutdown:

     virNWFilterTechDriversShutdown();

+err_dhcpsnoop_shutdown:

     virNWFilterDHCPSnoopShutdown();

 err_exit_learnshutdown:

     virNWFilterLearnShutdown();

@@ -329,10 +331,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 203ba0e..21b38ae 100644

--- 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)) {

@@ -934,6 +957,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 */

@@ -961,6 +986,8 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver,

                                         foundNewFilter);

 cleanup:

+    virMutexUnlock(&updateMutex);

+

     return rc;

 }

@@ -980,6 +1007,7 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver,

     bool foundNewFilter = false;

     virNWFilterReadLockFilterUpdates();

+    virMutexLock(&updateMutex);

     rc = __virNWFilterInstantiateFilter(driver,

                                         vmuuid,

@@ -1005,6 +1033,7 @@ virNWFilterInstantiateFilterLate(virNWFilterDriverStatePtr driver,

     }

     virNWFilterUnlockFilterUpdates();

+    virMutexUnlock(&updateMutex);

     return rc;

 }

@@ -1128,7 +1157,11 @@ _virNWFilterTeardownFilter(const char *ifname)

 int

 virNWFilterTeardownFilter(const virDomainNetDefPtr 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 8528e2a..48ee1df 100644

--- a/src/nwfilter/nwfilter_gentech_driver.h

+++ b/src/nwfilter/nwfilter_gentech_driver.h

@@ -30,7 +30,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 {

-- 

1.9.0