[Qemu-devel] [PATCH 0/2] pci: hotplug bridge fixes

[Qemu-devel] [PATCH 0/2] pci: hotplug bridge fixes

[Jason Baron]

Hi,

While testing pci bridge hotplug via device_add, I ran into a couple of
qemu segfaults.

The first one was caused by having a refcount greater than 0, in the
object_delete() path. Once, I got past that error, I hit a second
segfault due to the fact that pci_bridge_dev_initfn() didn't fully
cleanup its state.

Thanks,

-Jason

Jason Baron (2):
  qdev: release parent properties on dc->init failure
  pci_bridge_dev: fix error path in pci_bridge_dev_initfn()

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

[Qemu-devel] [PATCH 1/2] qdev: release parent properties on dc->init failure

[Jason Baron]

While looking into hot-plugging bridges, I can create a qemu segfault via:

$ device_add pci-bridge

Bridge chassis not specified. Each bridge is required to be assigned a unique chassis id > 0.
**
ERROR:qom/object.c:389:object_delete: assertion failed: (obj->ref == 0)

I'm proposing to fix this by adding a call to 'object_unparent()', before the
call to qdev_free(). I see there is already a precedent for this usage pattern as
seen in qdev_simple_unplug_cb():

/* can be used as ->unplug() callback for the simple cases */
int qdev_simple_unplug_cb(DeviceState *dev)
{
    /* just zap it */
    object_unparent(OBJECT(dev));
    qdev_free(dev);
    return 0;
}

Signed-off-by: Jason Baron <jbaron@redhat.com>
---
 hw/qdev.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 0bcde20..cff7f4c 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -150,6 +150,7 @@ int qdev_init(DeviceState *dev)
 
     rc = dc->init(dev);
     if (rc < 0) {
+        object_unparent(OBJECT(dev));
         qdev_free(dev);
         return rc;
     }
-- 
1.7.1

[Qemu-devel] [PATCH 2/2] pci_bridge_dev: fix error path in pci_bridge_dev_initfn()

[Jason Baron]

Currently, we do not properly cleanup, if pci_bridge_dev_initfn
fails to initialize properly. Make sure to call pci_bridge_exitfn()
in the error path.

Signed-off-by: Jason Baron <jbaron@redhat.com>
---
 hw/pci_bridge_dev.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/hw/pci_bridge_dev.c b/hw/pci_bridge_dev.c
index eccaa58..ad63703 100644
--- a/hw/pci_bridge_dev.c
+++ b/hw/pci_bridge_dev.c
@@ -52,7 +52,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
 {
     PCIBridge *br = DO_UPCAST(PCIBridge, dev, dev);
     PCIBridgeDev *bridge_dev = DO_UPCAST(PCIBridgeDev, bridge, br);
-    int err;
+    int err, ret;
     pci_bridge_map_irq(br, NULL, pci_bridge_dev_map_irq_fn);
     err = pci_bridge_initfn(dev);
     if (err) {
@@ -86,6 +86,8 @@ slotid_error:
     shpc_cleanup(dev, &bridge_dev->bar);
 shpc_error:
     memory_region_destroy(&bridge_dev->bar);
+    ret = pci_bridge_exitfn(dev);
+    assert(!ret);
 bridge_error:
     return err;
 }
-- 
1.7.1

Re: [Qemu-devel] [PATCH 0/2] pci: hotplug bridge fixes

[Michael S. Tsirkin]

On Wed, May 02, 2012 at 10:42:06PM -0400, Jason Baron wrote:
> Hi,
> 
> While testing pci bridge hotplug via device_add, I ran into a couple of
> qemu segfaults.
> 
> The first one was caused by having a refcount greater than 0, in the
> object_delete() path. Once, I got past that error, I hit a second
> segfault due to the fact that pci_bridge_dev_initfn() didn't fully
> cleanup its state.
> 
> Thanks,
> 
> -Jason

Applied, thanks.

> Jason Baron (2):
>   qdev: release parent properties on dc->init failure
>   pci_bridge_dev: fix error path in pci_bridge_dev_initfn()
> 
>  hw/pci_bridge_dev.c |    4 +++-
>  hw/qdev.c           |    1 +
>  2 files changed, 4 insertions(+), 1 deletions(-)