[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[libvirt] [PATCH] Fix potential events deadlock when unref'ing virConnectPtr



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

When the last reference to a virConnectPtr is released by
libvirtd, it was possible for a deadlock to occur in the
virDomainEventState functions. The virDomainEventStatePtr
holds a reference on virConnectPtr for each registered
callback. When removing a callback, the virUnrefConnect
function is run. If this causes the last reference on the
virConnectPtr to be released, then virReleaseConnect can
be run, which in turns calls qemudClose. This function has
a call to virDomainEventStateDeregisterConn which is intended
to remove all callbacks associated with the virConnectPtr
instance.

Since each callback associated with a virConnectPtr holds a
reference on virConnectPtr, it is impossible for the qemudClose
method to be invoked while any callbacks are still registered.
Thus the call to virDomainEventStateDeregisterConn must in fact
be a no-op. Thus it is possible to just remove all trace of
virDomainEventStateDeregisterConn and avoid the deadlock.

* src/conf/domain_event.c, src/conf/domain_event.h,
  src/libvirt_private.syms: Delete virDomainEventStateDeregisterConn
* src/libxl/libxl_driver.c, src/lxc/lxc_driver.c,
  src/qemu/qemu_driver.c, src/uml/uml_driver.c: Remove
  calls to virDomainEventStateDeregisterConn
---
 src/conf/domain_event.c  |   61 ----------------------------------------------
 src/conf/domain_event.h  |    4 ---
 src/libvirt_private.syms |    1 -
 src/libxl/libxl_driver.c |    4 ---
 src/lxc/lxc_driver.c     |    2 --
 src/qemu/qemu_driver.c   |    2 --
 src/uml/uml_driver.c     |    2 --
 7 files changed, 76 deletions(-)

diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c
index 923c58d..4ecc413 100644
--- a/src/conf/domain_event.c
+++ b/src/conf/domain_event.c
@@ -248,45 +248,6 @@ virDomainEventCallbackListRemoveID(virConnectPtr conn,
 }
 
 
-/**
- * virDomainEventCallbackListRemoveConn:
- * @conn: pointer to the connection
- * @cbList: the list
- *
- * Internal function to remove all of a given connection's callback
- * from a virDomainEventCallbackListPtr
- */
-static int
-virDomainEventCallbackListRemoveConn(virConnectPtr conn,
-                                     virDomainEventCallbackListPtr cbList)
-{
-    int old_count = cbList->count;
-    int i;
-    for (i = 0 ; i < cbList->count ; i++) {
-        if (cbList->callbacks[i]->conn == conn) {
-            virFreeCallback freecb = cbList->callbacks[i]->freecb;
-            if (freecb)
-                (*freecb)(cbList->callbacks[i]->opaque);
-            virUnrefConnect(cbList->callbacks[i]->conn);
-            VIR_FREE(cbList->callbacks[i]);
-
-            if (i < (cbList->count - 1))
-                memmove(cbList->callbacks + i,
-                        cbList->callbacks + i + 1,
-                        sizeof(*(cbList->callbacks)) *
-                                (cbList->count - (i + 1)));
-            cbList->count--;
-            i--;
-        }
-    }
-    if (cbList->count < old_count &&
-        VIR_REALLOC_N(cbList->callbacks, cbList->count) < 0) {
-        ; /* Failure to reduce memory allocation isn't fatal */
-    }
-    return 0;
-}
-
-
 static int
 virDomainEventCallbackListMarkDelete(virConnectPtr conn,
                                      virDomainEventCallbackListPtr cbList,
@@ -1608,28 +1569,6 @@ virDomainEventStateDeregisterID(virConnectPtr conn,
 
 
 /**
- * virDomainEventStateDeregisterConn:
- * @conn: connection to associate with callbacks
- * @state: domain event state
- *
- * Remove all callbacks from @state associated with the
- * connection @conn
- *
- * Returns 0 on success, -1 on error
- */
-int
-virDomainEventStateDeregisterConn(virConnectPtr conn,
-                                  virDomainEventStatePtr state)
-{
-    int ret;
-    virDomainEventStateLock(state);
-    ret = virDomainEventCallbackListRemoveConn(conn, state->callbacks);
-    virDomainEventStateUnlock(state);
-    return ret;
-}
-
-
-/**
  * virDomainEventStateEventID:
  * @conn: connection associated with the callback
  * @state: domain event state
diff --git a/src/conf/domain_event.h b/src/conf/domain_event.h
index f7776c7..d381aec 100644
--- a/src/conf/domain_event.h
+++ b/src/conf/domain_event.h
@@ -161,10 +161,6 @@ virDomainEventStateDeregisterID(virConnectPtr conn,
                                 int callbackID)
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
 int
-virDomainEventStateDeregisterConn(virConnectPtr conn,
-                                  virDomainEventStatePtr state)
-    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
-int
 virDomainEventStateEventID(virConnectPtr conn,
                            virDomainEventStatePtr state,
                            int callbackID)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index f5c2184..6c907c4 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -522,7 +522,6 @@ virDomainEventRebootNewFromDom;
 virDomainEventRebootNewFromObj;
 virDomainEventStateDeregister;
 virDomainEventStateDeregisterID;
-virDomainEventStateDeregisterConn;
 virDomainEventStateEventID;
 virDomainEventStateRegister;
 virDomainEventStateRegisterID;
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 45bf1f8..fc90d16 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -1081,10 +1081,6 @@ libxlClose(virConnectPtr conn ATTRIBUTE_UNUSED)
 {
     libxlDriverPrivatePtr driver = conn->privateData;
 
-    libxlDriverLock(driver);
-    virDomainEventStateDeregisterConn(conn,
-                                      driver->domainEventState);
-    libxlDriverUnlock(driver);
     conn->privateData = NULL;
     return 0;
 }
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 4cccd53..9aea556 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -178,8 +178,6 @@ static int lxcClose(virConnectPtr conn)
     lxc_driver_t *driver = conn->privateData;
 
     lxcDriverLock(driver);
-    virDomainEventStateDeregisterConn(conn,
-                                      driver->domainEventState);
     lxcProcessAutoDestroyRun(driver, conn);
     lxcDriverUnlock(driver);
 
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index efbc421..39b27b1 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -962,8 +962,6 @@ static int qemudClose(virConnectPtr conn) {
 
     /* Get rid of callbacks registered for this conn */
     qemuDriverLock(driver);
-    virDomainEventStateDeregisterConn(conn,
-                                      driver->domainEventState);
     qemuDriverCloseCallbackRunAll(driver, conn);
     qemuDriverUnlock(driver);
 
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
index 8a39d73..65f9cbc 100644
--- a/src/uml/uml_driver.c
+++ b/src/uml/uml_driver.c
@@ -1188,8 +1188,6 @@ static int umlClose(virConnectPtr conn) {
     struct uml_driver *driver = conn->privateData;
 
     umlDriverLock(driver);
-    virDomainEventStateDeregisterConn(conn,
-                                      driver->domainEventState);
     umlProcessAutoDestroyRun(driver, conn);
     umlDriverUnlock(driver);
 
-- 
1.7.10.1


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]