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

[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

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

[Michael S. Tsirkin]

On Tue, Jun 12, 2012 at 04:31:20AM +0000, Ma, Stephen B. wrote:
> 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)
>  {


Yes, I agree. qdev_init did now allocate the device so
it should not free it.


> @@ -150,7 +150,6 @@ int qdev_init(DeviceState *dev)
>  
>      rc = dc->init(dev);
>      if (rc < 0) {
> -        qdev_free(dev);
>          return rc;
>      }
>

Another thing we need to fix is unparent the device in
qdev_free.

-- 
MST

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

[Ma, Stephen B.]

Michael,

Thanks for the review.  I added the unparent to the qdev_free.


---
 hw/qdev.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index d2dc28b..ed1328d 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -264,6 +264,7 @@ void qdev_init_nofail(DeviceState *dev)
 /* Unlink device from bus and free the structure.  */
 void qdev_free(DeviceState *dev)
 {
+    object_unparent(OBJECT(dev));
     object_delete(OBJECT(dev));
 }

--
1.7.1

-----Original Message-----
From: Michael S. Tsirkin [mailto:mst@redhat.com] 
Sent: Tuesday, June 12, 2012 1:27 AM
To: Ma, Stephen B.
Cc: 'qemu-devel@nongnu.org'
Subject: Re: [PATCH] Fix for qemu crash on assertion error when adding PCI passthru device.

On Tue, Jun 12, 2012 at 04:31:20AM +0000, Ma, Stephen B. wrote:
> 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)
>  {


Yes, I agree. qdev_init did now allocate the device so
it should not free it.


> @@ -150,7 +150,6 @@ int qdev_init(DeviceState *dev)
>  
>      rc = dc->init(dev);
>      if (rc < 0) {
> -        qdev_free(dev);
>          return rc;
>      }
>

Another thing we need to fix is unparent the device in
qdev_free.

-- 
MST

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

[Michael S. Tsirkin]

On Sun, Jun 17, 2012 at 06:26:33AM +0000, Ma, Stephen B. wrote:
> 
> Michael,
> 
> Thanks for the review.  I added the unparent to the qdev_free.
> 
> 
> ---
>  hw/qdev.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/qdev.c b/hw/qdev.c
> index d2dc28b..ed1328d 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -264,6 +264,7 @@ void qdev_init_nofail(DeviceState *dev)
>  /* Unlink device from bus and free the structure.  */
>  void qdev_free(DeviceState *dev)
>  {
> +    object_unparent(OBJECT(dev));
>      object_delete(OBJECT(dev));
>  }
> 
> --
> 1.7.1

Anthony, any feedback?

-- 
MST

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

[Anthony Liguori]

On 06/17/2012 03:34 AM, Michael S. Tsirkin wrote:
> On Sun, Jun 17, 2012 at 06:26:33AM +0000, Ma, Stephen B. wrote:
>>
>> Michael,
>>
>> Thanks for the review.  I added the unparent to the qdev_free.
>>
>>
>> ---
>>   hw/qdev.c |    1 +
>>   1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/qdev.c b/hw/qdev.c
>> index d2dc28b..ed1328d 100644
>> --- a/hw/qdev.c
>> +++ b/hw/qdev.c
>> @@ -264,6 +264,7 @@ void qdev_init_nofail(DeviceState *dev)
>>   /* Unlink device from bus and free the structure.  */
>>   void qdev_free(DeviceState *dev)
>>   {
>> +    object_unparent(OBJECT(dev));
>>       object_delete(OBJECT(dev));
>>   }
>>
>> --
>> 1.7.1
>
> Anthony, any feedback?

Yes, this is wrong.

PCI passthrough isn't in qemu.git so it's not clear to me where this is 
happening.  Why would qdev_free be called when adding a PCI passthru device?

Regards,

Anthony Liguori

>

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1118 bytes --]

On 2012-06-17 16:28, Anthony Liguori wrote:
> On 06/17/2012 03:34 AM, Michael S. Tsirkin wrote:
>> On Sun, Jun 17, 2012 at 06:26:33AM +0000, Ma, Stephen B. wrote:
>>>
>>> Michael,
>>>
>>> Thanks for the review.  I added the unparent to the qdev_free.
>>>
>>>
>>> ---
>>>   hw/qdev.c |    1 +
>>>   1 files changed, 1 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/hw/qdev.c b/hw/qdev.c
>>> index d2dc28b..ed1328d 100644
>>> --- a/hw/qdev.c
>>> +++ b/hw/qdev.c
>>> @@ -264,6 +264,7 @@ void qdev_init_nofail(DeviceState *dev)
>>>   /* Unlink device from bus and free the structure.  */
>>>   void qdev_free(DeviceState *dev)
>>>   {
>>> +    object_unparent(OBJECT(dev));
>>>       object_delete(OBJECT(dev));
>>>   }
>>>
>>> -- 
>>> 1.7.1
>>
>> Anthony, any feedback?
> 
> Yes, this is wrong.
> 
> PCI passthrough isn't in qemu.git so it's not clear to me where this is
> happening.  Why would qdev_free be called when adding a PCI passthru
> device?

The bug is reproducible with any in-tree device (at least PCI) that
happens to return != 0 from its init handler.

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

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

[Ma, Stephen B.]

Sorry for taking so long to reply.  I am new to this.  Should this patch be committed or just dropped


-----Original Message-----
From: Jan Kiszka [mailto:jan.kiszka@web.de] 
Sent: Sunday, June 17, 2012 11:25 PM
To: Anthony Liguori
Cc: Michael S. Tsirkin; 'qemu-devel@nongnu.org'; Ma, Stephen B.
Subject: Re: [PATCH] Fix for qemu crash on assertion error when adding PCI passthru device.

On 2012-06-17 16:28, Anthony Liguori wrote:
> On 06/17/2012 03:34 AM, Michael S. Tsirkin wrote:
>> On Sun, Jun 17, 2012 at 06:26:33AM +0000, Ma, Stephen B. wrote:
>>>
>>> Michael,
>>>
>>> Thanks for the review.  I added the unparent to the qdev_free.
>>>
>>>
>>> ---
>>>   hw/qdev.c |    1 +
>>>   1 files changed, 1 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/hw/qdev.c b/hw/qdev.c
>>> index d2dc28b..ed1328d 100644
>>> --- a/hw/qdev.c
>>> +++ b/hw/qdev.c
>>> @@ -264,6 +264,7 @@ void qdev_init_nofail(DeviceState *dev)
>>>   /* Unlink device from bus and free the structure.  */
>>>   void qdev_free(DeviceState *dev)
>>>   {
>>> +    object_unparent(OBJECT(dev));
>>>       object_delete(OBJECT(dev));
>>>   }
>>>
>>> --
>>> 1.7.1
>>
>> Anthony, any feedback?
> 
> Yes, this is wrong.
> 
> PCI passthrough isn't in qemu.git so it's not clear to me where this 
> is happening.  Why would qdev_free be called when adding a PCI 
> passthru device?

The bug is reproducible with any in-tree device (at least PCI) that happens to return != 0 from its init handler.

Jan

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1723 bytes --]

On 2012-07-18 22:42, Ma, Stephen B. wrote:
> Sorry for taking so long to reply.  I am new to this.  Should this patch be committed or just dropped

This bug was fixed by 266ca11a0433643a3cc3146a9837d9f2b0bfbe3b in the
meantime.

Jan

> 
> 
> -----Original Message-----
> From: Jan Kiszka [mailto:jan.kiszka@web.de] 
> Sent: Sunday, June 17, 2012 11:25 PM
> To: Anthony Liguori
> Cc: Michael S. Tsirkin; 'qemu-devel@nongnu.org'; Ma, Stephen B.
> Subject: Re: [PATCH] Fix for qemu crash on assertion error when adding PCI passthru device.
> 
> On 2012-06-17 16:28, Anthony Liguori wrote:
>> On 06/17/2012 03:34 AM, Michael S. Tsirkin wrote:
>>> On Sun, Jun 17, 2012 at 06:26:33AM +0000, Ma, Stephen B. wrote:
>>>>
>>>> Michael,
>>>>
>>>> Thanks for the review.  I added the unparent to the qdev_free.
>>>>
>>>>
>>>> ---
>>>>   hw/qdev.c |    1 +
>>>>   1 files changed, 1 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/hw/qdev.c b/hw/qdev.c
>>>> index d2dc28b..ed1328d 100644
>>>> --- a/hw/qdev.c
>>>> +++ b/hw/qdev.c
>>>> @@ -264,6 +264,7 @@ void qdev_init_nofail(DeviceState *dev)
>>>>   /* Unlink device from bus and free the structure.  */
>>>>   void qdev_free(DeviceState *dev)
>>>>   {
>>>> +    object_unparent(OBJECT(dev));
>>>>       object_delete(OBJECT(dev));
>>>>   }
>>>>
>>>> --
>>>> 1.7.1
>>>
>>> Anthony, any feedback?
>>
>> Yes, this is wrong.
>>
>> PCI passthrough isn't in qemu.git so it's not clear to me where this 
>> is happening.  Why would qdev_free be called when adding a PCI 
>> passthru device?
> 
> The bug is reproducible with any in-tree device (at least PCI) that happens to return != 0 from its init handler.
> 
> Jan
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]