From 2122aadba5eb30ae466a3e825bbe9ed75b8d9735 Mon Sep 17 00:00:00 2001

Message-Id: <2122aadba5eb30ae466a3e825bbe9ed75b8d9735@dist-git>

From: Erik Skultety <eskultet@redhat.com>

Date: Thu, 4 May 2017 13:50:00 +0200

Subject: [PATCH] mdev: Fix daemon crash on domain shutdown after reconnect

The problem resides in virHostdevUpdateActiveMediatedDevices which gets

called during qemuProcessReconnect. The issue here is that

virMediatedDeviceListAdd takes a pointer to the item to be added to the

list to which VIR_APPEND_ELEMENT is used, which also clears the pointer.

However, in this case only the local copy of the pointer got cleared,

leaving the original pointing to valid memory. To sum it up, during

cleanup phase, the original pointer is freed and the daemon crashes

basically any time it would access it.

Backtrace:

0x00007ffff3ccdeba in __strcmp_sse2_unaligned

0x00007ffff72a444a in virMediatedDeviceListFindIndex

0x00007ffff7241446 in virHostdevReAttachMediatedDevices

0x00007fffc60215d9 in qemuHostdevReAttachMediatedDevices

0x00007fffc60216dc in qemuHostdevReAttachDomainDevices

0x00007fffc6046e6f in qemuProcessStop

0x00007fffc6091596 in processMonitorEOFEvent

0x00007fffc6091793 in qemuProcessEventHandler

0x00007ffff7294bf5 in virThreadPoolWorker

0x00007ffff7294184 in virThreadHelper

0x00007ffff3fdc3c4 in start_thread () from /lib64/libpthread.so.0

0x00007ffff3d269cf in clone () from /lib64/libc.so.6

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

Signed-off-by: Erik Skultety <eskultet@redhat.com>

Reviewed-by: Laine Stump <laine@laine.org>

(cherry picked from commit 92e30a4dace54d06433f763e1acba0a81bb5c82e)

Signed-off-by: Erik Skultety <eskultet@redhat.com>

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

---

 src/util/virhostdev.c |  4 ++--

 src/util/virmdev.c    | 13 ++++++++-----

 src/util/virmdev.h    |  2 +-

 3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c

index 2c557f5bb..579563c3f 100644

--- a/src/util/virhostdev.c

+++ b/src/util/virhostdev.c

@@ -1294,7 +1294,7 @@ virHostdevUpdateActiveMediatedDevices(virHostdevManagerPtr mgr,

         virMediatedDeviceSetUsedBy(mdev, drv_name, dom_name);

-        if (virMediatedDeviceListAdd(mgr->activeMediatedHostdevs, mdev) < 0)

+        if (virMediatedDeviceListAdd(mgr->activeMediatedHostdevs, &mdev) < 0)

             goto cleanup;

     }

@@ -1790,7 +1790,7 @@ virHostdevPrepareMediatedDevices(virHostdevManagerPtr mgr,

         if (!(mdev = virMediatedDeviceNew(src->uuidstr, src->model)))

             goto cleanup;

-        if (virMediatedDeviceListAdd(list, mdev) < 0) {

+        if (virMediatedDeviceListAdd(list, &mdev) < 0) {

             virMediatedDeviceFree(mdev);

             goto cleanup;

         }

diff --git a/src/util/virmdev.c b/src/util/virmdev.c

index c1499d238..c861d21c9 100644

--- a/src/util/virmdev.c

+++ b/src/util/virmdev.c

@@ -312,16 +312,19 @@ virMediatedDeviceListDispose(void *obj)

 }

+/* The reason for @dev to be double pointer is that VIR_APPEND_ELEMENT clears

+ * the pointer and we need to clear the original not a copy on the stack

+ */

 int

 virMediatedDeviceListAdd(virMediatedDeviceListPtr list,

-                         virMediatedDevicePtr dev)

+                         virMediatedDevicePtr *dev)

 {

-    if (virMediatedDeviceListFind(list, dev)) {

+    if (virMediatedDeviceListFind(list, *dev)) {

         virReportError(VIR_ERR_INTERNAL_ERROR,

-                       _("device %s is already in use"), dev->path);

+                       _("device %s is already in use"), (*dev)->path);

         return -1;

     }

-    return VIR_APPEND_ELEMENT(list->devs, list->count, dev);

+    return VIR_APPEND_ELEMENT(list->devs, list->count, *dev);

 }

@@ -457,7 +460,7 @@ virMediatedDeviceListMarkDevices(virMediatedDeviceListPtr dst,

          * - caller is responsible for NOT freeing devices in @src on success

          * - we're responsible for performing a rollback on failure

          */

-        if (virMediatedDeviceListAdd(dst, mdev) < 0)

+        if (virMediatedDeviceListAdd(dst, &mdev) < 0)

             goto rollback;

         VIR_DEBUG("'%s' added to list of active mediated devices used by '%s'",

diff --git a/src/util/virmdev.h b/src/util/virmdev.h

index 2f3d6bb84..8bb46b9c5 100644

--- a/src/util/virmdev.h

+++ b/src/util/virmdev.h

@@ -86,7 +86,7 @@ virMediatedDeviceListNew(void);

 int

 virMediatedDeviceListAdd(virMediatedDeviceListPtr list,

-                         virMediatedDevicePtr dev);

+                         virMediatedDevicePtr *dev);

 virMediatedDevicePtr

 virMediatedDeviceListGet(virMediatedDeviceListPtr list,

-- 

2.13.0