[libvirt] [PATCH 0/2] Fix daemon crash caused by virHostdevUpdateActiveMediatedDevices
Erik Skultety (2): util: mdev: Use a local variable instead of an invalid pointer reference mdev: Fix daemon crash on domain shutdown after reconnect src/util/virhostdev.c | 4 ++-- src/util/virmdev.c | 18 +++++++++++------- src/util/virmdev.h | 2 +- 3 files changed, 14 insertions(+), 10 deletions(-) -- 2.12.2
virMediatedDeviceListAdd calls VIR_APPEND_ELEMENT which normally clears the element to be added, thus the pointer must not be used afterwards, but because the pointer here is passed by value, what gets cleared is a copy of the original pointer that got created on the stack automatically when calling the function. The fact that it works now is just a coincidence and a bug in virMediatedDeviceListAdd that needs to be fixed in a separate commit. Therefore, use a local variable to hold data, rather than accessing the pointer later. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/util/virmdev.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/util/virmdev.c b/src/util/virmdev.c index 2a1ade739..c1499d238 100644 --- a/src/util/virmdev.c +++ b/src/util/virmdev.c @@ -447,20 +447,21 @@ virMediatedDeviceListMarkDevices(virMediatedDeviceListPtr dst, virObjectLock(dst); for (i = 0; i < count; i++) { virMediatedDevicePtr mdev = virMediatedDeviceListGet(src, i); + const char *mdev_path = mdev->path; if (virMediatedDeviceIsUsed(mdev, dst) || virMediatedDeviceSetUsedBy(mdev, drvname, domname) < 0) goto cleanup; /* Copy mdev references to the driver list: - * - caller is responsible for NOT freeing devices in @list on success + * - 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) goto rollback; VIR_DEBUG("'%s' added to list of active mediated devices used by '%s'", - mdev->path, domname); + mdev_path, domname); } ret = 0; -- 2.12.2
On 04/28/2017 03:46 AM, Erik Skultety wrote:
virMediatedDeviceListAdd calls VIR_APPEND_ELEMENT which normally clears the element to be added, thus the pointer must not be used afterwards, but because the pointer here is passed by value, what gets cleared is a copy of the original pointer that got created on the stack automatically when calling the function. The fact that it works now is just a coincidence and a bug in virMediatedDeviceListAdd that needs to be fixed in a separate commit. Therefore, use a local variable to hold data, rather than accessing the pointer later.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Laine Stump <laine@laine.org> (Oh, and "safe for freeze", in case that wasn't assumed. Same goes for 2/2)
On Fri, Apr 28, 2017 at 09:46:32AM +0200, Erik Skultety wrote:
virMediatedDeviceListAdd calls VIR_APPEND_ELEMENT which normally clears the element to be added, thus the pointer must not be used afterwards, but because the pointer here is passed by value, what gets cleared is a copy of the original pointer that got created on the stack automatically when calling the function. The fact that it works now is just a coincidence and a bug in virMediatedDeviceListAdd that needs to be fixed in a separate commit. Therefore, use a local variable to hold data, rather than accessing the pointer later.
I don't thing that this patch is required. What happens here is that in this function we get a pointer to an existing memory that stores virMediatedDevice, if the mdev is not used we mark it as used and store that pointer to *dst* list. That's what virMediatedDeviceListAdd does, it only stores the pointer to an allocated into the list so it doesn't clear the real data, it only operates with pointers. This means that the memory addressed by local *mdev* variable still points to a valid memory and everything is OK, there is no issue at all apart from a fact, that setting the pointer in virMediatedDeviceListAdd is pointless. Pavel
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/util/virmdev.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/util/virmdev.c b/src/util/virmdev.c index 2a1ade739..c1499d238 100644 --- a/src/util/virmdev.c +++ b/src/util/virmdev.c @@ -447,20 +447,21 @@ virMediatedDeviceListMarkDevices(virMediatedDeviceListPtr dst, virObjectLock(dst); for (i = 0; i < count; i++) { virMediatedDevicePtr mdev = virMediatedDeviceListGet(src, i); + const char *mdev_path = mdev->path;
if (virMediatedDeviceIsUsed(mdev, dst) || virMediatedDeviceSetUsedBy(mdev, drvname, domname) < 0) goto cleanup;
/* Copy mdev references to the driver list: - * - caller is responsible for NOT freeing devices in @list on success + * - 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) goto rollback;
VIR_DEBUG("'%s' added to list of active mediated devices used by '%s'", - mdev->path, domname); + mdev_path, domname); }
ret = 0; -- 2.12.2
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, May 03, 2017 at 05:35:47PM +0200, Pavel Hrdina wrote:
On Fri, Apr 28, 2017 at 09:46:32AM +0200, Erik Skultety wrote:
virMediatedDeviceListAdd calls VIR_APPEND_ELEMENT which normally clears the element to be added, thus the pointer must not be used afterwards, but because the pointer here is passed by value, what gets cleared is a copy of the original pointer that got created on the stack automatically when calling the function. The fact that it works now is just a coincidence and a bug in virMediatedDeviceListAdd that needs to be fixed in a separate commit. Therefore, use a local variable to hold data, rather than accessing the pointer later.
I don't thing that this patch is required. What happens here is that in this function we get a pointer to an existing memory that stores virMediatedDevice, if the mdev is not used we mark it as used and store that pointer to *dst* list. That's what virMediatedDeviceListAdd does, it only stores the pointer to an allocated into the list so it doesn't clear the real data, it only operates with pointers. This means that the memory addressed by local *mdev* variable still points to a valid memory and everything is OK, there is no issue at all apart from a fact, that setting the pointer in virMediatedDeviceListAdd is pointless.
Pavel
After reading the second patch, this one will be required, however I would change the commit message because there is no issue right now, this change is only necessary for the following patch. Pavel
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/util/virmdev.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/util/virmdev.c b/src/util/virmdev.c index 2a1ade739..c1499d238 100644 --- a/src/util/virmdev.c +++ b/src/util/virmdev.c @@ -447,20 +447,21 @@ virMediatedDeviceListMarkDevices(virMediatedDeviceListPtr dst, virObjectLock(dst); for (i = 0; i < count; i++) { virMediatedDevicePtr mdev = virMediatedDeviceListGet(src, i); + const char *mdev_path = mdev->path;
if (virMediatedDeviceIsUsed(mdev, dst) || virMediatedDeviceSetUsedBy(mdev, drvname, domname) < 0) goto cleanup;
/* Copy mdev references to the driver list: - * - caller is responsible for NOT freeing devices in @list on success + * - 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) goto rollback;
VIR_DEBUG("'%s' added to list of active mediated devices used by '%s'", - mdev->path, domname); + mdev_path, domname); }
ret = 0; -- 2.12.2
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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> --- 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..55ec8db9f 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 no 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.12.2
On 04/28/2017 03:46 AM, Erik Skultety wrote:
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> or ACK, or whatever is the new accepted hotness.
On Wed, May 03, 2017 at 11:25:30AM -0400, Laine Stump wrote:
On 04/28/2017 03:46 AM, Erik Skultety wrote:
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>
or ACK, or whatever is the new accepted hotness.
Thanks, I pushed them both to master and v3.2-maint. Erik
participants (3)
-
Erik Skultety -
Laine Stump -
Pavel Hrdina