Re: [PATCH v2] hotplug: Fix libvirtd crash on qemu-attached guest

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

 



On 25.09.2014 17:00, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1141621

The crash in this case was because the qemu-attach code did not create
aliases for qemu cli args. When the detach-interface code was called
it assumed aliases were set resulting in a core when dereferencing the
still NULL alias.

Adding a call to qemuAssignDeviceAliases() resolves the path for
qemu-attach; however, to prevent future issues an additional check
for a NULL value is made and an error provided in the deatch path.

Add some more verbiage to the virsh man page.

Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
---

v1 is here:
http://www.redhat.com/archives/libvir-list/2014-September/msg01331.html

Changes since v1:
  - Add the call to qemuAssignDeviceAliases() in qemuDomainQemuAttach().
  - Move the check for NULL alias inside the CAPS_DEVICE check and emit
    an error rather than trying to remove as an "else" condition.

  src/qemu/qemu_driver.c  | 3 +++
  src/qemu/qemu_hotplug.c | 7 +++++++
  tools/virsh.pod         | 5 +++--
  3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 117138a..ef4ecd2 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14746,6 +14746,9 @@ static virDomainPtr qemuDomainQemuAttach(virConnectPtr conn,
      if (qemuCanonicalizeMachine(def, qemuCaps) < 0)
          goto cleanup;

+    if (qemuAssignDeviceAliases(def, qemuCaps) < 0)
+        goto cleanup;
+
      if (qemuDomainAssignAddresses(def, qemuCaps, NULL) < 0)
          goto cleanup;

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index d631887..daebe82 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3521,6 +3521,13 @@ qemuDomainDetachNetDevice(virConnectPtr conn,

      qemuDomainObjEnterMonitor(driver, vm);
      if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
+        if (!detach->info.alias) {
+            virReportError(VIR_ERR_OPERATION_FAILED, "%s",
+                           _("device alias not found: cannot delete the "
+                             "net device"));
+            qemuDomainObjExitMonitor(driver, vm);
+            goto cleanup;
+        }

Is there a reason to not check this upfront rather than this late in the process? And I don't think that net devices are the only thing affected here. For instance a virtio disks seems vulnerable too (well, at first glance on the code).

          if (qemuMonitorDelDevice(priv->mon, detach->info.alias) < 0) {

The other possibility would be to make qemuMonitorDelDevice fail on @devalias == NULL.

              qemuDomainObjExitMonitor(driver, vm);
              virDomainAuditNet(vm, detach, NULL, "detach", false);
diff --git a/tools/virsh.pod b/tools/virsh.pod
index eae9195..bd17f68 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -3698,8 +3698,9 @@ using the UNIX driver. Ideally the process will also have had the

  Not all functions of libvirt are expected to work reliably after
  attaching to an externally launched QEMU process. There may be
-issues with the guest ABI changing upon migration, and hotunplug
-may not work.
+issues with the guest ABI changing upon migration and device hotplug
+or hotunplug may not work. The attached environment should be considered
+primarily read-only.

  =item B<qemu-monitor-command> I<domain> { [I<--hmp>] | [I<--pretty>] }
  I<command>...


Michal

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]