[Qemu-devel] [PATCH] Fix for qemu crash on assertion error when adding PCI passthru device.

[Ma, Stephen B.]

Title: Fix for qemu crash on assertion error when adding PCI passthru device.

Description: On a kernel 3.4.0+ without having interrupt remapping enabled,
starting a VM having a PCI passthrough device fails. ON q qemu-kvm built with
--enable-debug, qemu aborts. Qemu-kvm is executed via virsh.

Command line:

2012-05-31 20:56:03.922+0000: starting up
LC_ALL=C PATH=/sbin:/usr/sbin:/bin:/usr/bin HOME=/root USER=root LOGNAME=root 
QEMU_AUDIO_DRV=none /usr/libexec/qemu-kvm -S -M pc-1.1 -enable-kvm -m 4096 -smp 
2,sockets=2,cores=1,threads=1 -name f1664M -uuid 
507532c6-d287-cf86-c6a0-6895055392b6 -no-user-config -nodefaults -chardev 
socket,id=charmonitor,path=/var/lib/libvirt/qemu/f1664M.monitor,server,nowait 
-mon chardev=charmonitor,id=monitor,mode=control -rtc base=utc -no-shutdown 
-device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 -drive 
file=/var/lib/libvirt/images/f1664M.img,if=none,id=drive-ide0-0-0,format=qcow2,cache=none
 -device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 
-drive if=none,id=drive-ide0-1-0,readonly=on,format=raw -device 
ide-cd,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0 -netdev 
tap,fd=20,id=hostnet0 -device 
rtl8139,netdev=hostnet0,id=net0,mac=52:54:00:00:3e:32,bus=pci.0,addr=0x3 
-netdev tap,fd=21,id=hostnet1,vhost=on,vhostfd=22 -device 
virtio-net-pci,netdev=hostnet1,id=net1,mac=52:54:00:7c:76:bb,bus=pci.0,addr=0x4 
-chardev pty,id=charserial0 -device isa-serial,chardev=charserial0,id=serial0 
-vnc 0.0.0.0:5 -vga cirrus -device intel-hda,id=sound0,bus=pci.0,addr=0x5 
-device hda-duplex,id=sound0-codec0,bus=sound0.0,cad=0 -device 
pci-assign,host=02:06.1,id=hostdev0,configfd=23,bus=pci.0,addr=0x7 -device 
virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x6Domain id=1 is tainted: 
high-privileges
char device redirected to /dev/pts/3
Failed to assign device "hostdev0" : Operation not permitted
**
ERROR:qom/object.c:389:object_delete: assertion failed: (obj->ref == 0)
2012-05-31 20:56:04.375+0000: shutting down

Gdb of the qemu-kvm coredump.

(gdb) backtrace
   from /lib64/libglib-2.0.so.0
    at /home/hpadmin/src/qemu-kvm/hw/qdev.c:268
    at /home/hpadmin/src/qemu-kvm/hw/qdev.c:153
    at /home/hpadmin/src/qemu-kvm/hw/qdev-monitor.c:472
    at /home/hpadmin/src/qemu-kvm/vl.c:1838
    0x7f6c752b616d <device_init_func>, opaque=0x0, abort_on_failure=1)
    at qemu-option.c:1053
    0x7fffc6a5a6e0) at /home/hpadmin/src/qemu-kvm/vl.c:3570
(gdb)

Explanation:
The crash happened the qdev_init routine freed the qdev structure when there
are still references by other objects to the structure. The initialization
using looking for the parent bus of the device. This association is made by
made by qdev_device_add.  In cases other than PCI passthrough devices,
No such associations is made in the callers of qdev_init.

Fix is to
1. In all cases except for pci passthrough, have the caller of qdev_init free
the object being initialized in case of failure.
2. In the case of adding a PCI passthrough device, upon qdev_init failure,
disassociate the device with the parent and then free it.

Signed-off-by: Stephen Ma
---
 hw/grlib.h           |    3 +++
 hw/ide/isa.c         |    5 +++--
 hw/pc.h              |    2 ++
 hw/pci-hotplug.c     |    8 ++++++--
 hw/pci.c             |    4 +++-
 hw/qdev-monitor.c    |    1 +
 hw/qdev.c            |    3 +--
 hw/scsi-bus.c        |    4 +++-
 hw/usb/bus.c         |    1 +
 hw/usb/dev-storage.c |    5 +++--
 10 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/hw/grlib.h b/hw/grlib.h
index e1c4137..2e9742c 100644
--- a/hw/grlib.h
+++ b/hw/grlib.h
@@ -56,6 +56,7 @@ DeviceState *grlib_irqmp_create(target_phys_addr_t   base,
     qdev_prop_set_ptr(dev, "set_pil_in_opaque", env);
 
     if (qdev_init(dev)) {
+        qdev_free(dev);
         return NULL;
     }
 
@@ -88,6 +89,7 @@ DeviceState *grlib_gptimer_create(target_phys_addr_t  base,
     qdev_prop_set_uint32(dev, "irq-line", base_irq);
 
     if (qdev_init(dev)) {
+        qdev_free(dev);
         return NULL;
     }
 
@@ -113,6 +115,7 @@ DeviceState *grlib_apbuart_create(target_phys_addr_t  base,
     qdev_prop_set_chr(dev, "chrdev", serial);
 
     if (qdev_init(dev)) {
+        qdev_free(dev);
         return NULL;
     }
 
diff --git a/hw/ide/isa.c b/hw/ide/isa.c
index 8ab2718..0efaedb 100644
--- a/hw/ide/isa.c
+++ b/hw/ide/isa.c
@@ -83,9 +83,10 @@ ISADevice *isa_ide_init(ISABus *bus, int iobase, int 
iobase2, int isairq,
     qdev_prop_set_uint32(&dev->qdev, "iobase",  iobase);
     qdev_prop_set_uint32(&dev->qdev, "iobase2", iobase2);
     qdev_prop_set_uint32(&dev->qdev, "irq",     isairq);
-    if (qdev_init(&dev->qdev) < 0)
+    if (qdev_init(&dev->qdev) < 0) {
+        qdev_free(&dev->qdev);
         return NULL;
-
+    }
     s = DO_UPCAST(ISAIDEState, dev, dev);
     if (hd0)
         ide_create_drive(&s->bus, 0, hd0);
diff --git a/hw/pc.h b/hw/pc.h
index 74d3369..e378821 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -32,6 +32,7 @@ static inline bool serial_isa_init(ISABus *bus, int index,
     qdev_prop_set_uint32(&dev->qdev, "index", index);
     qdev_prop_set_chr(&dev->qdev, "chardev", chr);
     if (qdev_init(&dev->qdev) < 0) {
+        qdev_free(&dev->qdev);
         return false;
     }
     return true;
@@ -51,6 +52,7 @@ static inline bool parallel_init(ISABus *bus, int index, 
CharDriverState *chr)
     qdev_prop_set_uint32(&dev->qdev, "index", index);
     qdev_prop_set_chr(&dev->qdev, "chardev", chr);
     if (qdev_init(&dev->qdev) < 0) {
+        qdev_free(&dev->qdev);
         return false;
     }
     return true;
diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
index 61257f4..16fecd1 100644
--- a/hw/pci-hotplug.c
+++ b/hw/pci-hotplug.c
@@ -192,8 +192,10 @@ static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon,
     switch (type) {
     case IF_SCSI:
         dev = pci_create(bus, devfn, "lsi53c895a");
-        if (qdev_init(&dev->qdev) < 0)
+        if (qdev_init(&dev->qdev) < 0)   {
+            qdev_free(&dev->qdev);
             dev = NULL;
+        }
         if (dev && dinfo) {
             if (scsi_hot_add(mon, &dev->qdev, dinfo, 0) != 0) {
                 qdev_unplug(&dev->qdev, NULL);
@@ -212,8 +214,10 @@ static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon,
             dev = NULL;
             break;
         }
-        if (qdev_init(&dev->qdev) < 0)
+        if (qdev_init(&dev->qdev) < 0)  {
+            qdev_free(&dev->qdev);
             dev = NULL;
+        }
         break;
     default:
         dev = NULL;
diff --git a/hw/pci.c b/hw/pci.c
index c1ebdde..53cc60b 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1390,8 +1390,10 @@ PCIDevice *pci_nic_init(NICInfo *nd, const char 
*default_model,
     pci_dev = pci_create(bus, devfn, pci_nic_names[i]);
     dev = &pci_dev->qdev;
     qdev_set_nic_properties(dev, nd);
-    if (qdev_init(dev) < 0)
+    if (qdev_init(dev) < 0) {
+        qdev_free(dev);
         return NULL;
+    }
     return pci_dev;
 }
 
diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
index b01ef06..3f940cf 100644
--- a/hw/qdev-monitor.c
+++ b/hw/qdev-monitor.c
@@ -470,6 +470,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
         g_free(name);
     }        
     if (qdev_init(qdev) < 0) {
+        qdev_free(qdev);
         qerror_report(QERR_DEVICE_INIT_FAILED, driver);
         return NULL;
     }
diff --git a/hw/qdev.c b/hw/qdev.c
index 6a8f6bd..d2dc28b 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -139,7 +139,7 @@ DeviceState *qdev_try_create(BusState *bus, const char 
*type)
 /* Initialize a device.  Device properties should be set before calling
    this function.  IRQs and MMIO regions should be connected/mapped after
    calling this function.
-   On failure, destroy the device and return negative value.
+   On failure, return a negative value.
    Return 0 on success.  */
 int qdev_init(DeviceState *dev)
 {
@@ -150,7 +150,6 @@ int qdev_init(DeviceState *dev)
 
     rc = dc->init(dev);
     if (rc < 0) {
-        qdev_free(dev);
         return rc;
     }
 
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index f10f3ec..2b738b0 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -212,8 +212,10 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, 
BlockDriverState *bdrv,
         qdev_free(dev);
         return NULL;
     }
-    if (qdev_init(dev) < 0)
+    if (qdev_init(dev) < 0) {
+        qdev_free(dev);
         return NULL;
+    }
     return SCSI_DEVICE(dev);
 }
 
diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index 2068640..1b0a1cc 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -242,6 +242,7 @@ USBDevice *usb_create_simple(USBBus *bus, const char *name)
     }
     rc = qdev_init(&dev->qdev);
     if (rc < 0) {
+        qdev_free(&dev->qdev);
         error_report("Failed to initialize USB device '%s'", name);
         return NULL;
     }
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index a96c0b9..cf0addb 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -623,9 +623,10 @@ static USBDevice *usb_msd_init(USBBus *bus, const char 
*filename)
         qdev_free(&dev->qdev);
         return NULL;
     }
-    if (qdev_init(&dev->qdev) < 0)
+    if (qdev_init(&dev->qdev) < 0) {
+        qdev_free(&dev->qdev);
         return NULL;
-
+    }
     return dev;
 }
 
-- 
1.7.1