[Qemu-devel] [PATCH] virtio-pci: fix virtio-net child refcount in transports

[Qemu-devel] [PATCH] virtio-pci: fix virtio-net child refcount in transports

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

object_initialize() leaves the object with a refcount of 1.
object_property_add_child() adds its own reference which is dropped
again when the property is deleted.

The upshot of this is that we always have a refcount >= 1.  Upon hot
unplug the virtio-net child is not finalized!

Drop our reference after the child property has been added to the
parent.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
Stefan had post virtio-blk in commit c5d49db4, but virtio-net has 
the same problem. Maybe the other virtio devices have too.
---
 hw/virtio/virtio-pci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index ddb5da1..78dcd68 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1456,6 +1456,7 @@ static void virtio_net_pci_instance_init(Object *obj)
     VirtIONetPCI *dev = VIRTIO_NET_PCI(obj);
     object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_NET);
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
+    object_unref(OBJECT(&dev->vdev));
 }
 
 static const TypeInfo virtio_net_pci_info = {
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH] virtio-pci: fix virtio-net child refcount in transports

[Gonglei (Arei)]

Hi,

CC'ing Stefan and qemu-stable@ for more attention. :)


Best regards,
-Gonglei


> -----Original Message-----
> From: Gonglei (Arei)
> Sent: Thursday, September 04, 2014 7:42 PM
> To: qemu-devel@nongnu.org
> Cc: mst@redhat.com; Huangweidong (C); Gonglei (Arei)
> Subject: [PATCH] virtio-pci: fix virtio-net child refcount in transports
> 
> From: Gonglei <arei.gonglei@huawei.com>
> 
> object_initialize() leaves the object with a refcount of 1.
> object_property_add_child() adds its own reference which is dropped
> again when the property is deleted.
> 
> The upshot of this is that we always have a refcount >= 1.  Upon hot
> unplug the virtio-net child is not finalized!
> 
> Drop our reference after the child property has been added to the
> parent.
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
> Stefan had post virtio-blk in commit c5d49db4, but virtio-net has
> the same problem. Maybe the other virtio devices have too.
> ---
>  hw/virtio/virtio-pci.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index ddb5da1..78dcd68 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1456,6 +1456,7 @@ static void virtio_net_pci_instance_init(Object *obj)
>      VirtIONetPCI *dev = VIRTIO_NET_PCI(obj);
>      object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_NET);
>      object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev),
> NULL);
> +    object_unref(OBJECT(&dev->vdev));
>  }
> 
>  static const TypeInfo virtio_net_pci_info = {
> --
> 1.7.12.4
> 

Re: [Qemu-devel] [PATCH] virtio-pci: fix virtio-net child refcount in transports

[Michael S. Tsirkin]

On Thu, Sep 04, 2014 at 07:41:32PM +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> object_initialize() leaves the object with a refcount of 1.
> object_property_add_child() adds its own reference which is dropped
> again when the property is deleted.
> 
> The upshot of this is that we always have a refcount >= 1.  Upon hot
> unplug the virtio-net child is not finalized!
> 
> Drop our reference after the child property has been added to the
> parent.
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>

Aren't other virtio devices affected? what about virtio-scsi?

> ---
> Stefan had post virtio-blk in commit c5d49db4, but virtio-net has 
> the same problem. Maybe the other virtio devices have too.
> ---
>  hw/virtio/virtio-pci.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index ddb5da1..78dcd68 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1456,6 +1456,7 @@ static void virtio_net_pci_instance_init(Object *obj)
>      VirtIONetPCI *dev = VIRTIO_NET_PCI(obj);
>      object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_NET);
>      object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
> +    object_unref(OBJECT(&dev->vdev));
>  }
>  
>  static const TypeInfo virtio_net_pci_info = {
> -- 
> 1.7.12.4
> 

Re: [Qemu-devel] [PATCH] virtio-pci: fix virtio-net child refcount in transports

[Gonglei]

Hi,

> Subject: Re: [Qemu-devel] [PATCH] virtio-pci: fix virtio-net child refcount in
> transports
> 
> On Thu, Sep 04, 2014 at 07:41:32PM +0800, arei.gonglei@huawei.com wrote:
> > From: Gonglei <arei.gonglei@huawei.com>
> >
> > object_initialize() leaves the object with a refcount of 1.
> > object_property_add_child() adds its own reference which is dropped
> > again when the property is deleted.
> >
> > The upshot of this is that we always have a refcount >= 1.  Upon hot
> > unplug the virtio-net child is not finalized!
> >
> > Drop our reference after the child property has been added to the
> > parent.
> >
> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> 
> Aren't other virtio devices affected? what about virtio-scsi?
> 
Maybe, but I haven't tested other virtio devices TBH. :)

Best regards,
-Gonglei

> > ---
> > Stefan had post virtio-blk in commit c5d49db4, but virtio-net has
> > the same problem. Maybe the other virtio devices have too.
> > ---
> >  hw/virtio/virtio-pci.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index ddb5da1..78dcd68 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -1456,6 +1456,7 @@ static void virtio_net_pci_instance_init(Object
> *obj)
> >      VirtIONetPCI *dev = VIRTIO_NET_PCI(obj);
> >      object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_NET);
> >      object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev),
> NULL);
> > +    object_unref(OBJECT(&dev->vdev));
> >  }
> >
> >  static const TypeInfo virtio_net_pci_info = {
> > --
> > 1.7.12.4
> >

Re: [Qemu-devel] [PATCH] virtio-pci: fix virtio-net child refcount in transports

[Stefan Hajnoczi]

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

On Thu, Sep 04, 2014 at 07:41:32PM +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> object_initialize() leaves the object with a refcount of 1.
> object_property_add_child() adds its own reference which is dropped
> again when the property is deleted.
> 
> The upshot of this is that we always have a refcount >= 1.  Upon hot
> unplug the virtio-net child is not finalized!
> 
> Drop our reference after the child property has been added to the
> parent.
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
> Stefan had post virtio-blk in commit c5d49db4, but virtio-net has 
> the same problem. Maybe the other virtio devices have too.
> ---
>  hw/virtio/virtio-pci.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index ddb5da1..78dcd68 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1456,6 +1456,7 @@ static void virtio_net_pci_instance_init(Object *obj)
>      VirtIONetPCI *dev = VIRTIO_NET_PCI(obj);
>      object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_NET);
>      object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
> +    object_unref(OBJECT(&dev->vdev));
>  }
>  
>  static const TypeInfo virtio_net_pci_info = {

You just copied my commit but didn't consider all the work I did before
to make virtio-blk safe.  Similar work is necessary for virtio-net.

Before it is okay to finalize the virtio-net (dev->vdev) object, you
need to audit virtio-pci and virtio-net to make sure all resources and
qdev properties are freed once and only once.

In the case of virtio-net, the qdev properties are duplicated so both
virtio-net-pci and virtio-net point qdev properties at the same fields
in virtio-net.  For example, the "tx" string property will be
double-freed with your patch.

You can use the alias properties that I introduced to avoid duplicating
qdev properties for virtio-net.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] virtio-pci: fix virtio-net child refcount in transports

[Gonglei]

Hi,

> Subject: Re: [Qemu-devel] [PATCH] virtio-pci: fix virtio-net child refcount in
> transports
> 
> On Thu, Sep 04, 2014 at 07:41:32PM +0800, arei.gonglei@huawei.com wrote:
> > From: Gonglei <arei.gonglei@huawei.com>
> >
> > object_initialize() leaves the object with a refcount of 1.
> > object_property_add_child() adds its own reference which is dropped
> > again when the property is deleted.
> >
> > The upshot of this is that we always have a refcount >= 1.  Upon hot
> > unplug the virtio-net child is not finalized!
> >
> > Drop our reference after the child property has been added to the
> > parent.
> >
> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > ---
> > Stefan had post virtio-blk in commit c5d49db4, but virtio-net has
> > the same problem. Maybe the other virtio devices have too.
> > ---
> >  hw/virtio/virtio-pci.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index ddb5da1..78dcd68 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -1456,6 +1456,7 @@ static void virtio_net_pci_instance_init(Object
> *obj)
> >      VirtIONetPCI *dev = VIRTIO_NET_PCI(obj);
> >      object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_NET);
> >      object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev),
> NULL);
> > +    object_unref(OBJECT(&dev->vdev));
> >  }
> >
> >  static const TypeInfo virtio_net_pci_info = {
> 
> You just copied my commit but didn't consider all the work I did before
> to make virtio-blk safe.  Similar work is necessary for virtio-net.
> 
> Before it is okay to finalize the virtio-net (dev->vdev) object, 

Sorry? In my case the virtio-net object will not be finalized at present
because its obj->ref is non-1 when we hot-unplug a virtio-net-pci device.

> you need to audit virtio-pci and virtio-net to make sure all resources and
> qdev properties are freed once and only once.
> 
Yes, indeed.

> In the case of virtio-net, the qdev properties are duplicated so both
> virtio-net-pci and virtio-net point qdev properties at the same fields
> in virtio-net.  For example, the "tx" string property will be
> double-freed with your patch.
> 
OK. Got it!

> You can use the alias properties that I introduced to avoid duplicating
> qdev properties for virtio-net.
> 
> Stefan

OK, Thanks for your point and suggestion! I will rework it more carefully
next week.

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH] virtio-pci: fix virtio-net child refcount in transports

[Stefan Hajnoczi]

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

On Fri, Sep 05, 2014 at 11:46:32PM +0800, Gonglei wrote:
> Hi,
> 
> > Subject: Re: [Qemu-devel] [PATCH] virtio-pci: fix virtio-net child refcount in
> > transports
> > 
> > On Thu, Sep 04, 2014 at 07:41:32PM +0800, arei.gonglei@huawei.com wrote:
> > > From: Gonglei <arei.gonglei@huawei.com>
> > >
> > > object_initialize() leaves the object with a refcount of 1.
> > > object_property_add_child() adds its own reference which is dropped
> > > again when the property is deleted.
> > >
> > > The upshot of this is that we always have a refcount >= 1.  Upon hot
> > > unplug the virtio-net child is not finalized!
> > >
> > > Drop our reference after the child property has been added to the
> > > parent.
> > >
> > > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > > ---
> > > Stefan had post virtio-blk in commit c5d49db4, but virtio-net has
> > > the same problem. Maybe the other virtio devices have too.
> > > ---
> > >  hw/virtio/virtio-pci.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > > index ddb5da1..78dcd68 100644
> > > --- a/hw/virtio/virtio-pci.c
> > > +++ b/hw/virtio/virtio-pci.c
> > > @@ -1456,6 +1456,7 @@ static void virtio_net_pci_instance_init(Object
> > *obj)
> > >      VirtIONetPCI *dev = VIRTIO_NET_PCI(obj);
> > >      object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_NET);
> > >      object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev),
> > NULL);
> > > +    object_unref(OBJECT(&dev->vdev));
> > >  }
> > >
> > >  static const TypeInfo virtio_net_pci_info = {
> > 
> > You just copied my commit but didn't consider all the work I did before
> > to make virtio-blk safe.  Similar work is necessary for virtio-net.
> > 
> > Before it is okay to finalize the virtio-net (dev->vdev) object, 
> 
> Sorry? In my case the virtio-net object will not be finalized at present
> because its obj->ref is non-1 when we hot-unplug a virtio-net-pci device.

"Before" refers to making virtio-pci/virtio-net fixes first and then
object_unref() second.  It does not mean "previously" here.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] virtio-pci: fix virtio-net child refcount in transports

[Gonglei]

> Subject: Re: [Qemu-devel] [PATCH] virtio-pci: fix virtio-net child refcount in
> transports
> 
> On Fri, Sep 05, 2014 at 11:46:32PM +0800, Gonglei wrote:
> > Hi,
> >
> > > Subject: Re: [Qemu-devel] [PATCH] virtio-pci: fix virtio-net child refcount in
> > > transports
> > >
> > > On Thu, Sep 04, 2014 at 07:41:32PM +0800, arei.gonglei@huawei.com
> wrote:
> > > > From: Gonglei <arei.gonglei@huawei.com>
> > > >
> > > > object_initialize() leaves the object with a refcount of 1.
> > > > object_property_add_child() adds its own reference which is dropped
> > > > again when the property is deleted.
> > > >
> > > > The upshot of this is that we always have a refcount >= 1.  Upon hot
> > > > unplug the virtio-net child is not finalized!
> > > >
> > > > Drop our reference after the child property has been added to the
> > > > parent.
> > > >
> > > > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > > > ---
> > > > Stefan had post virtio-blk in commit c5d49db4, but virtio-net has
> > > > the same problem. Maybe the other virtio devices have too.
> > > > ---
> > > >  hw/virtio/virtio-pci.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > > > index ddb5da1..78dcd68 100644
> > > > --- a/hw/virtio/virtio-pci.c
> > > > +++ b/hw/virtio/virtio-pci.c
> > > > @@ -1456,6 +1456,7 @@ static void virtio_net_pci_instance_init(Object
> > > *obj)
> > > >      VirtIONetPCI *dev = VIRTIO_NET_PCI(obj);
> > > >      object_initialize(&dev->vdev, sizeof(dev->vdev),
> TYPE_VIRTIO_NET);
> > > >      object_property_add_child(obj, "virtio-backend",
> OBJECT(&dev->vdev),
> > > NULL);
> > > > +    object_unref(OBJECT(&dev->vdev));
> > > >  }
> > > >
> > > >  static const TypeInfo virtio_net_pci_info = {
> > >
> > > You just copied my commit but didn't consider all the work I did before
> > > to make virtio-blk safe.  Similar work is necessary for virtio-net.
> > >
> > > Before it is okay to finalize the virtio-net (dev->vdev) object,
> >
> > Sorry? In my case the virtio-net object will not be finalized at present
> > because its obj->ref is non-1 when we hot-unplug a virtio-net-pci device.
> 
> "Before" refers to making virtio-pci/virtio-net fixes first and then
> object_unref() second.  It does not mean "previously" here.

OK. Thanks. :)

Best regards,
-Gonglei