nwfilter: fix crash during filter define when lxc driver failed startup

The meat of this patch is just moving the calls to
virNWFilterRegisterCallbackDriver from each hypervisor's "register"
function into its "initialize" function. The rest is just code
movement to allow that, and a new virNWFilterUnRegisterCallbackDriver
function to undo what the register function does.

The long explanation:

There is an array in nwfilter called callbackDrvArray that has
pointers to a table of functions for each hypervisor driver that are
called by nwfilter. One of those function pointers is to a function
that will lock the hypervisor driver. Entries are added to the table
by calling each driver's "register" function, which happens quite
early in libvirtd's startup.

Sometime later, each driver's "initialize" function is called. This
function allocates a driver object and stores a pointer to it in a
static variable that was previously initialized to NULL. (and here's
the important part...) If the "initialize" function fails, the driver
object is freed, and that pointer set back to NULL (but the entry in
nwfilter's callbackDrvArray is still there).

When the "lock the driver" function mentioned above is called, it
assumes that the driver was successfully loaded, so it blindly tries
to call virMutexLock on "driver->lock".

BUT, if the initialize never happened, or if it failed, "driver" is
NULL. And it just happens that "lock" is always the first field in
driver so it is also NULL.

Boom.

To fix this, the call to virNWFilterRegisterCallbackDriver for each
driver shouldn't be called until the end of its (*already guaranteed
successful*) "initialize" function, not during its "register" function
(which is currently the case). This implies that there should also be
a virNWFilterUnregisterCallbackDriver() function that is called in a
driver's "shutdown" function (although in practice, that function is
currently never called).

diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c
index 665ea0cca2b7bbe76d012a450dd340278fc19796..8f8e0530bd11f4cc77a0ad153052b8abb1c10778 100644 (file)
--- a/src/conf/nwfilter_conf.c
+++ b/src/conf/nwfilter_conf.c
@@ -2828,6 +2828,22 @@ virNWFilterRegisterCallbackDriver(virNWFilterCallbackDriverPtr cbd)
     }
 }
 
+void
+virNWFilterUnRegisterCallbackDriver(virNWFilterCallbackDriverPtr cbd)
+{
+    int i = 0;
+
+    while (i < nCallbackDriver && callbackDrvArray[i] != cbd)
+        i++;
+
+    if (i < nCallbackDriver) {
+        memmove(&callbackDrvArray[i], &callbackDrvArray[i+1],
+                (nCallbackDriver - i - 1) * sizeof(callbackDrvArray[i]));
+        callbackDrvArray[i] = 0;
+        nCallbackDriver--;
+    }
+}
+
 void
 virNWFilterCallbackDriversLock(void)
 {

diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h
index ca6bd168af34ca8d01d9290fb969c94f1a6097d5..0b3ff91da3db641448b7483c739bbaf04acc3aab 100644 (file)
--- a/src/conf/nwfilter_conf.h
+++ b/src/conf/nwfilter_conf.h
@@ -745,6 +745,7 @@ struct _virNWFilterCallbackDriver {
 };
 
 void virNWFilterRegisterCallbackDriver(virNWFilterCallbackDriverPtr);
+void virNWFilterUnRegisterCallbackDriver(virNWFilterCallbackDriverPtr);
 void virNWFilterCallbackDriversLock(void);
 void virNWFilterCallbackDriversUnlock(void);
 

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 79b4a183ce84ecf3dbe672abb2b1631b62fd0640..c023dbf4cc68d3224e5dc82d498f381b4c998be0 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -880,6 +880,7 @@ virNWFilterRuleActionTypeToString;
 virNWFilterRuleDirectionTypeToString;
 virNWFilterRuleProtocolTypeToString;
 virNWFilterTestUnassignDef;
+virNWFilterUnRegisterCallbackDriver;
 virNWFilterUnlockFilterUpdates;
 
 

diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index d118dd238801e5b0606eea22f643c95a157b948c..b7b119c9d668f10790a41a85e0d068a682fcb2ff 100644 (file)
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -74,6 +74,35 @@ static int lxcStartup(int privileged);
 static int lxcShutdown(void);
 virLXCDriverPtr lxc_driver = NULL;
 
+/* callbacks for nwfilter */
+static int
+lxcVMFilterRebuild(virConnectPtr conn ATTRIBUTE_UNUSED,
+                   virHashIterator iter, void *data)
+{
+    virHashForEach(lxc_driver->domains.objs, iter, data);
+
+    return 0;
+}
+
+static void
+lxcVMDriverLock(void)
+{
+    lxcDriverLock(lxc_driver);
+}
+
+static void
+lxcVMDriverUnlock(void)
+{
+    lxcDriverUnlock(lxc_driver);
+}
+
+static virNWFilterCallbackDriver lxcCallbackDriver = {
+    .name = "LXC",
+    .vmFilterRebuild = lxcVMFilterRebuild,
+    .vmDriverLock = lxcVMDriverLock,
+    .vmDriverUnlock = lxcVMDriverUnlock,
+};
+
 /* Functions */
 
 static virDrvOpenStatus lxcOpen(virConnectPtr conn,
@@ -1465,6 +1494,7 @@ static int lxcStartup(int privileged)
 
     virLXCProcessAutostartAll(lxc_driver);
 
+    virNWFilterRegisterCallbackDriver(&lxcCallbackDriver);
     return 0;
 
 cleanup:
@@ -1516,6 +1546,7 @@ static int lxcShutdown(void)
         return -1;
 
     lxcDriverLock(lxc_driver);
+    virNWFilterUnRegisterCallbackDriver(&lxcCallbackDriver);
     virDomainObjListDeinit(&lxc_driver->domains);
     virDomainEventStateFree(lxc_driver->domainEventState);
 
@@ -2654,34 +2685,6 @@ lxcListAllDomains(virConnectPtr conn,
     return ret;
 }
 
-static int
-lxcVMFilterRebuild(virConnectPtr conn ATTRIBUTE_UNUSED,
-                   virHashIterator iter, void *data)
-{
-    virHashForEach(lxc_driver->domains.objs, iter, data);
-
-    return 0;
-}
-
-static void
-lxcVMDriverLock(void)
-{
-    lxcDriverLock(lxc_driver);
-}
-
-static void
-lxcVMDriverUnlock(void)
-{
-    lxcDriverUnlock(lxc_driver);
-}
-
-static virNWFilterCallbackDriver lxcCallbackDriver = {
-    .name = "LXC",
-    .vmFilterRebuild = lxcVMFilterRebuild,
-    .vmDriverLock = lxcVMDriverLock,
-    .vmDriverUnlock = lxcVMDriverUnlock,
-};
-
 /* Function Tables */
 static virDriver lxcDriver = {
     .no = VIR_DRV_LXC,
@@ -2761,6 +2764,5 @@ int lxcRegister(void)
 {
     virRegisterDriver(&lxcDriver);
     virRegisterStateDriver(&lxcStateDriver);
-    virNWFilterRegisterCallbackDriver(&lxcCallbackDriver);
     return 0;
 }

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index dee1268c2e7c9a6dd53a01db064dbe962e1fa987..9bf89bbb119e8558b471cde9bcd7359c7132966b 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -148,6 +148,35 @@ static void qemuDomainManagedSaveLoad(void *payload,
 struct qemud_driver *qemu_driver = NULL;
 
 
+static void
+qemuVMDriverLock(void) {
+    qemuDriverLock(qemu_driver);
+};
+
+
+static void
+qemuVMDriverUnlock(void) {
+    qemuDriverUnlock(qemu_driver);
+};
+
+
+static int
+qemuVMFilterRebuild(virConnectPtr conn ATTRIBUTE_UNUSED,
+                    virHashIterator iter, void *data)
+{
+    virHashForEach(qemu_driver->domains.objs, iter, data);
+
+    return 0;
+}
+
+static virNWFilterCallbackDriver qemuCallbackDriver = {
+    .name = QEMU_DRIVER_NAME,
+    .vmFilterRebuild = qemuVMFilterRebuild,
+    .vmDriverLock = qemuVMDriverLock,
+    .vmDriverUnlock = qemuVMDriverUnlock,
+};
+
+
 struct qemuAutostartData {
     struct qemud_driver *driver;
     virConnectPtr conn;
@@ -754,6 +783,7 @@ qemudStartup(int privileged) {
     if (conn)
         virConnectClose(conn);
 
+    virNWFilterRegisterCallbackDriver(&qemuCallbackDriver);
     return 0;
 
 out_of_memory:
@@ -843,6 +873,7 @@ qemudShutdown(void) {
         return -1;
 
     qemuDriverLock(qemu_driver);
+    virNWFilterUnRegisterCallbackDriver(&qemuCallbackDriver);
     pciDeviceListFree(qemu_driver->activePciHostdevs);
     pciDeviceListFree(qemu_driver->inactivePciHostdevs);
     usbDeviceListFree(qemu_driver->activeUsbHostdevs);
@@ -13380,37 +13411,8 @@ static virStateDriver qemuStateDriver = {
     .active = qemudActive,
 };
 
-static void
-qemuVMDriverLock(void) {
-    qemuDriverLock(qemu_driver);
-};
-
-
-static void
-qemuVMDriverUnlock(void) {
-    qemuDriverUnlock(qemu_driver);
-};
-
-
-static int
-qemuVMFilterRebuild(virConnectPtr conn ATTRIBUTE_UNUSED,
-                    virHashIterator iter, void *data)
-{
-    virHashForEach(qemu_driver->domains.objs, iter, data);
-
-    return 0;
-}
-
-static virNWFilterCallbackDriver qemuCallbackDriver = {
-    .name = QEMU_DRIVER_NAME,
-    .vmFilterRebuild = qemuVMFilterRebuild,
-    .vmDriverLock = qemuVMDriverLock,
-    .vmDriverUnlock = qemuVMDriverUnlock,
-};
-
 int qemuRegister(void) {
     virRegisterDriver(&qemuDriver);
     virRegisterStateDriver(&qemuStateDriver);
-    virNWFilterRegisterCallbackDriver(&qemuCallbackDriver);
     return 0;
 }

diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
index a4a92589c760afccf29ed1dded8cc64f521d431e..91698e11bffd13d3930569d4eddd840a604f5857 100644 (file)
--- a/src/uml/uml_driver.c
+++ b/src/uml/uml_driver.c
@@ -146,6 +146,34 @@ static int umlMonitorCommand(const struct uml_driver *driver,
 
 static struct uml_driver *uml_driver = NULL;
 
+static int
+umlVMFilterRebuild(virConnectPtr conn ATTRIBUTE_UNUSED,
+                   virHashIterator iter, void *data)
+{
+    virHashForEach(uml_driver->domains.objs, iter, data);
+
+    return 0;
+}
+
+static void
+umlVMDriverLock(void)
+{
+    umlDriverLock(uml_driver);
+}
+
+static void
+umlVMDriverUnlock(void)
+{
+    umlDriverUnlock(uml_driver);
+}
+
+static virNWFilterCallbackDriver umlCallbackDriver = {
+    .name = "UML",
+    .vmFilterRebuild = umlVMFilterRebuild,
+    .vmDriverLock = umlVMDriverLock,
+    .vmDriverUnlock = umlVMDriverUnlock,
+};
+
 struct umlAutostartData {
     struct uml_driver *driver;
     virConnectPtr conn;
@@ -505,6 +533,7 @@ umlStartup(int privileged)
 
     VIR_FREE(userdir);
 
+    virNWFilterRegisterCallbackDriver(&umlCallbackDriver);
     return 0;
 
 out_of_memory:
@@ -603,6 +632,7 @@ umlShutdown(void) {
         return -1;
 
     umlDriverLock(uml_driver);
+    virNWFilterRegisterCallbackDriver(&umlCallbackDriver);
     if (uml_driver->inotifyWatch != -1)
         virEventRemoveHandle(uml_driver->inotifyWatch);
     VIR_FORCE_CLOSE(uml_driver->inotifyFD);
@@ -2596,15 +2626,6 @@ static virDriver umlDriver = {
     .nodeSuspendForDuration = nodeSuspendForDuration, /* 0.9.8 */
 };
 
-static int
-umlVMFilterRebuild(virConnectPtr conn ATTRIBUTE_UNUSED,
-                   virHashIterator iter, void *data)
-{
-    virHashForEach(uml_driver->domains.objs, iter, data);
-
-    return 0;
-}
-
 static virStateDriver umlStateDriver = {
     .name = "UML",
     .initialize = umlStartup,
@@ -2613,28 +2634,8 @@ static virStateDriver umlStateDriver = {
     .active = umlActive,
 };
 
-static void
-umlVMDriverLock(void)
-{
-    umlDriverLock(uml_driver);
-}
-
-static void
-umlVMDriverUnlock(void)
-{
-    umlDriverUnlock(uml_driver);
-}
-
-static virNWFilterCallbackDriver umlCallbackDriver = {
-    .name = "UML",
-    .vmFilterRebuild = umlVMFilterRebuild,
-    .vmDriverLock = umlVMDriverLock,
-    .vmDriverUnlock = umlVMDriverUnlock,
-};
-
 int umlRegister(void) {
     virRegisterDriver(&umlDriver);
     virRegisterStateDriver(&umlStateDriver);
-    virNWFilterRegisterCallbackDriver(&umlCallbackDriver);
     return 0;
 }