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

[libvirt] [PATCH 03/14] Fix crash when deleting monitor while a command is in progress



If QEMU shuts down while we're in the middle of processing a
monitor command, the monitor will be freed, and upon cleaning
up we attempt to do  qemuMonitorUnlock(priv->mon) when priv->mon
is NULL.

To address this we introduce proper reference counting into
the qemuMonitorPtr object, and hold an extra reference whenever
executing a command.

* src/qemu/qemu_driver.c: Hold a reference on the monitor while
  executing commands, and only NULL-ify the priv->mon field when
  the last reference is released
* src/qemu/qemu_monitor.h, src/qemu/qemu_monitor.c: Add reference
  counting to handle safe deletion of monitor objects
---
 src/qemu/qemu_driver.c  |   13 ++++++--
 src/qemu/qemu_monitor.c |   81 +++++++++++++++++++++++++++++------------------
 src/qemu/qemu_monitor.h |    5 ++-
 3 files changed, 64 insertions(+), 35 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c9b5ac2..3befe3d 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -271,6 +271,7 @@ static void qemuDomainObjEnterMonitor(virDomainObjPtr obj)
     qemuDomainObjPrivatePtr priv = obj->privateData;
 
     qemuMonitorLock(priv->mon);
+    qemuMonitorRef(priv->mon);
     virDomainObjUnlock(obj);
 }
 
@@ -281,10 +282,15 @@ static void qemuDomainObjEnterMonitor(virDomainObjPtr obj)
  */
 static void qemuDomainObjExitMonitor(virDomainObjPtr obj)
 {
+    int refs;
     qemuDomainObjPrivatePtr priv = obj->privateData;
 
+    refs = qemuMonitorUnref(priv->mon);
     qemuMonitorUnlock(priv->mon);
     virDomainObjLock(obj);
+
+    if (refs == 0)
+        priv->mon = NULL;
 }
 
 
@@ -301,6 +307,7 @@ static void qemuDomainObjEnterMonitorWithDriver(struct qemud_driver *driver, vir
     qemuDomainObjPrivatePtr priv = obj->privateData;
 
     qemuMonitorLock(priv->mon);
+    qemuMonitorRef(priv->mon);
     virDomainObjUnlock(obj);
     qemuDriverUnlock(driver);
 }
@@ -315,6 +322,7 @@ static void qemuDomainObjExitMonitorWithDriver(struct qemud_driver *driver, virD
 {
     qemuDomainObjPrivatePtr priv = obj->privateData;
 
+    qemuMonitorUnref(priv->mon);
     qemuMonitorUnlock(priv->mon);
     qemuDriverLock(driver);
     virDomainObjLock(obj);
@@ -2399,10 +2407,9 @@ static void qemudShutdownVMDaemon(virConnectPtr conn,
                              _("Failed to send SIGTERM to %s (%d)"),
                              vm->def->name, vm->pid);
 
-    if (priv->mon) {
-        qemuMonitorClose(priv->mon);
+    if (priv->mon &&
+        qemuMonitorClose(priv->mon) == 0)
         priv->mon = NULL;
-    }
 
     if (vm->monitor_chr) {
         if (vm->monitor_chr->type == VIR_DOMAIN_CHR_TYPE_UNIX)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index f0ef81b..3829e7a 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -42,6 +42,8 @@ struct _qemuMonitor {
     virMutex lock;
     virCond notify;
 
+    int refs;
+
     virDomainObjPtr dom;
 
     int fd;
@@ -67,12 +69,14 @@ struct _qemuMonitor {
      * the next monitor msg */
     int lastErrno;
 
-    /* If the monitor callback is currently active */
+    /* If the monitor EOF callback is currently active (stops more commands being run) */
     unsigned eofcb: 1;
-    /* If the monitor callback should free the closed monitor */
+    /* If the monitor is in process of shutting down */
     unsigned closed: 1;
+
 };
 
+
 void qemuMonitorLock(qemuMonitorPtr mon)
 {
     virMutexLock(&mon->lock);
@@ -84,21 +88,33 @@ void qemuMonitorUnlock(qemuMonitorPtr mon)
 }
 
 
-static void qemuMonitorFree(qemuMonitorPtr mon, int lockDomain)
+static void qemuMonitorFree(qemuMonitorPtr mon)
 {
-    VIR_DEBUG("mon=%p, lockDomain=%d", mon, lockDomain);
-    if (mon->vm) {
-        if (lockDomain)
-            virDomainObjLock(mon->vm);
-        if (!virDomainObjUnref(mon->vm) && lockDomain)
-            virDomainObjUnlock(mon->vm);
-    }
+    VIR_DEBUG("mon=%p", mon);
+    if (mon->vm)
+        virDomainObjUnref(mon->vm);
     if (virCondDestroy(&mon->notify) < 0)
     {}
     virMutexDestroy(&mon->lock);
     VIR_FREE(mon);
 }
 
+int qemuMonitorRef(qemuMonitorPtr mon)
+{
+    mon->refs++;
+    return mon->refs;
+}
+
+int qemuMonitorUnref(qemuMonitorPtr mon)
+{
+    mon->refs--;
+
+    if (mon->refs == 0)
+        qemuMonitorFree(mon);
+
+    return mon->refs;
+}
+
 
 static int
 qemuMonitorOpenUnix(const char *monitor)
@@ -346,8 +362,10 @@ static void
 qemuMonitorIO(int watch, int fd, int events, void *opaque) {
     qemuMonitorPtr mon = opaque;
     int quit = 0, failed = 0;
+    virDomainObjPtr vm;
 
     qemuMonitorLock(mon);
+    qemuMonitorRef(mon);
     VIR_DEBUG("Monitor %p I/O on watch %d fd %d events %d", mon, watch, fd, events);
 
     if (mon->fd != fd || mon->watch != watch) {
@@ -409,22 +427,20 @@ qemuMonitorIO(int watch, int fd, int events, void *opaque) {
     if (failed || quit) {
         /* Make sure anyone waiting wakes up now */
         virCondSignal(&mon->notify);
-        mon->eofcb = 1;
         qemuMonitorUnlock(mon);
         VIR_DEBUG("Triggering EOF callback error? %d", failed);
         mon->eofCB(mon, mon->vm, failed);
 
         qemuMonitorLock(mon);
-        if (mon->closed) {
-            qemuMonitorUnlock(mon);
-            VIR_DEBUG("Delayed free of monitor %p", mon);
-            qemuMonitorFree(mon, 1);
-        } else {
-            qemuMonitorUnlock(mon);
-        }
-    } else {
-        qemuMonitorUnlock(mon);
     }
+
+    /* Safe 'vm' to an intermediate ptr, since 'mon' may be
+     * freed after the Unref call */
+    vm = mon->vm;
+    virDomainObjLock(vm);
+    if (qemuMonitorUnref(mon) > 0)
+        qemuMonitorUnlock(mon);
+    virDomainObjUnlock(vm);
 }
 
 
@@ -453,6 +469,7 @@ qemuMonitorOpen(virDomainObjPtr vm,
         return NULL;
     }
     mon->fd = -1;
+    mon->refs = 1;
     mon->vm = vm;
     mon->eofCB = eofCB;
     qemuMonitorLock(mon);
@@ -512,10 +529,14 @@ cleanup:
 }
 
 
-void qemuMonitorClose(qemuMonitorPtr mon)
+int qemuMonitorClose(qemuMonitorPtr mon)
 {
+    int refs;
+
     if (!mon)
-        return;
+        return 0;
+
+    VIR_DEBUG("mon=%p", mon);
 
     qemuMonitorLock(mon);
     if (!mon->closed) {
@@ -523,18 +544,17 @@ void qemuMonitorClose(qemuMonitorPtr mon)
             virEventRemoveHandle(mon->watch);
         if (mon->fd != -1)
             close(mon->fd);
-        /* NB: don't reset  fd / watch fields, since active
-         * callback may still want them */
+        /* NB: ordinarily one might immediately set mon->watch to -1
+         * and mon->fd to -1, but there may be a callback active
+         * that is still relying on these fields being valid. So
+         * we merely close them, but not clear their values and
+         * use this explicit 'closed' flag to track this state */
         mon->closed = 1;
     }
 
-    if (mon->eofcb) {
-        VIR_DEBUG("Mark monitor to be deleted %p", mon);
+    if ((refs = qemuMonitorUnref(mon)) > 0)
         qemuMonitorUnlock(mon);
-    } else {
-        VIR_DEBUG("Delete monitor now %p", mon);
-        qemuMonitorFree(mon, 0);
-    }
+    return refs;
 }
 
 
@@ -552,7 +572,6 @@ int qemuMonitorSend(qemuMonitorPtr mon,
 
     if (mon->eofcb) {
         msg->lastErrno = EIO;
-        qemuMonitorUnlock(mon);
         return -1;
     }
 
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 71688cb..27b43b7 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -78,11 +78,14 @@ typedef int (*qemuMonitorDiskSecretLookup)(qemuMonitorPtr mon,
 qemuMonitorPtr qemuMonitorOpen(virDomainObjPtr vm,
                                qemuMonitorEOFNotify eofCB);
 
-void qemuMonitorClose(qemuMonitorPtr mon);
+int qemuMonitorClose(qemuMonitorPtr mon);
 
 void qemuMonitorLock(qemuMonitorPtr mon);
 void qemuMonitorUnlock(qemuMonitorPtr mon);
 
+int qemuMonitorRef(qemuMonitorPtr mon);
+int qemuMonitorUnref(qemuMonitorPtr mon);
+
 void qemuMonitorRegisterDiskSecretLookup(qemuMonitorPtr mon,
                                          qemuMonitorDiskSecretLookup secretCB);
 
-- 
1.6.5.2


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