[v1] ivshmem: fix memory backend leak

[Qemu-devel] [PATCH] ivshmem: fix memory backend leak

Posted by Igor Mammedov 5 years, 5 months ago

object_new() returns a new backend with refcount == 1 and
then later object_property_add_child() increases refcount to 2
So when ivshmem is desroyed, the backend it has created isn't
destroyed along with it as children cleanup will bring
backend's refcount only to 1, which leaks backend including
resources it is using.

Drop the original reference from object_new() once backend
is attached to its parent.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/misc/ivshmem.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index f88910e..ecfd10a 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -1279,6 +1279,7 @@ static void desugar_shm(IVShmemState *s)
     object_property_set_bool(obj, true, "share", &error_abort);
     object_property_add_child(OBJECT(s), "internal-shm-backend", obj,
                               &error_abort);
+    object_unref(obj);
     user_creatable_complete(obj, &error_abort);
     s->hostmem = MEMORY_BACKEND(obj);
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH] ivshmem: fix memory backend leak

Posted by Marc-André Lureau 5 years, 5 months ago

On Thu, Nov 1, 2018 at 2:53 PM Igor Mammedov <imammedo@redhat.com> wrote:
>
> object_new() returns a new backend with refcount == 1 and
> then later object_property_add_child() increases refcount to 2
> So when ivshmem is desroyed, the backend it has created isn't
> destroyed along with it as children cleanup will bring
> backend's refcount only to 1, which leaks backend including
> resources it is using.
>
> Drop the original reference from object_new() once backend
> is attached to its parent.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

I would rather have the unref in finalize, but that is ok too.

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  hw/misc/ivshmem.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index f88910e..ecfd10a 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -1279,6 +1279,7 @@ static void desugar_shm(IVShmemState *s)
>      object_property_set_bool(obj, true, "share", &error_abort);
>      object_property_add_child(OBJECT(s), "internal-shm-backend", obj,
>                                &error_abort);
> +    object_unref(obj);
>      user_creatable_complete(obj, &error_abort);
>      s->hostmem = MEMORY_BACKEND(obj);
>  }
> --
> 2.7.4
>
>


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] ivshmem: fix memory backend leak

Posted by Igor Mammedov 5 years, 5 months ago

On Thu, 1 Nov 2018 15:02:04 +0400
Marc-André Lureau <marcandre.lureau@gmail.com> wrote:

> On Thu, Nov 1, 2018 at 2:53 PM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > object_new() returns a new backend with refcount == 1 and
> > then later object_property_add_child() increases refcount to 2
> > So when ivshmem is desroyed, the backend it has created isn't
> > destroyed along with it as children cleanup will bring
> > backend's refcount only to 1, which leaks backend including
> > resources it is using.
> >
> > Drop the original reference from object_new() once backend
> > is attached to its parent.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> 
> I would rather have the unref in finalize, but that is ok too.
I followed the pattern we use else where, i.e. drop reference
as soon as we set the parent (virtio-rng/cpus) within the same
scope as object_new().

There is no point in keeping reference until finalize time since
backend is kept alive as child and is destroyed well after all
nonexistent ivshmem::unrealize/finilize() are finished when generic
Object is being destroyed.

>  Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> > ---
> >  hw/misc/ivshmem.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> > index f88910e..ecfd10a 100644
> > --- a/hw/misc/ivshmem.c
> > +++ b/hw/misc/ivshmem.c
> > @@ -1279,6 +1279,7 @@ static void desugar_shm(IVShmemState *s)
> >      object_property_set_bool(obj, true, "share", &error_abort);
> >      object_property_add_child(OBJECT(s), "internal-shm-backend", obj,
> >                                &error_abort);
> > +    object_unref(obj);
> >      user_creatable_complete(obj, &error_abort);
> >      s->hostmem = MEMORY_BACKEND(obj);
> >  }
> > --
> > 2.7.4
> >
> >  
> 
>

Re: [Qemu-devel] [PATCH] ivshmem: fix memory backend leak

Posted by Markus Armbruster 5 years, 5 months ago

Igor Mammedov <imammedo@redhat.com> writes:

> On Thu, 1 Nov 2018 15:02:04 +0400
> Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
>
>> On Thu, Nov 1, 2018 at 2:53 PM Igor Mammedov <imammedo@redhat.com> wrote:
>> >
>> > object_new() returns a new backend with refcount == 1 and
>> > then later object_property_add_child() increases refcount to 2
>> > So when ivshmem is desroyed, the backend it has created isn't
>> > destroyed along with it as children cleanup will bring
>> > backend's refcount only to 1, which leaks backend including
>> > resources it is using.
>> >
>> > Drop the original reference from object_new() once backend
>> > is attached to its parent.
>> >

I believe
Fixes: 5503e285041979dd29698ecb41729b3b22622e8d

>> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
>> 
>> I would rather have the unref in finalize, but that is ok too.
> I followed the pattern we use else where, i.e. drop reference
> as soon as we set the parent (virtio-rng/cpus) within the same
> scope as object_new().
>
> There is no point in keeping reference until finalize time since
> backend is kept alive as child and is destroyed well after all
> nonexistent ivshmem::unrealize/finilize() are finished when generic
> Object is being destroyed.

Concur.

>>  Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

With Philippe's spelling fix:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH] ivshmem: fix memory backend leak

Posted by Philippe Mathieu-Daudé 5 years, 5 months ago

On 1/11/18 11:44, Igor Mammedov wrote:
> object_new() returns a new backend with refcount == 1 and
> then later object_property_add_child() increases refcount to 2
> So when ivshmem is desroyed, the backend it has created isn't

                      ^ "destroyed"

> destroyed along with it as children cleanup will bring
> backend's refcount only to 1, which leaks backend including
> resources it is using.
> 
> Drop the original reference from object_new() once backend
> is attached to its parent.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>   hw/misc/ivshmem.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index f88910e..ecfd10a 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -1279,6 +1279,7 @@ static void desugar_shm(IVShmemState *s)
>       object_property_set_bool(obj, true, "share", &error_abort);
>       object_property_add_child(OBJECT(s), "internal-shm-backend", obj,
>                                 &error_abort);
> +    object_unref(obj);
>       user_creatable_complete(obj, &error_abort);
>       s->hostmem = MEMORY_BACKEND(obj);
>   }
>

Re: [Qemu-devel] [PATCH] ivshmem: fix memory backend leak

Posted by Igor Mammedov 5 years, 5 months ago

On Thu, 1 Nov 2018 15:27:02 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 1/11/18 11:44, Igor Mammedov wrote:
> > object_new() returns a new backend with refcount == 1 and
> > then later object_property_add_child() increases refcount to 2
> > So when ivshmem is desroyed, the backend it has created isn't  
> 
>                       ^ "destroyed"
Thanks, fixed in v2

> 
> > destroyed along with it as children cleanup will bring
> > backend's refcount only to 1, which leaks backend including
> > resources it is using.
> > 
> > Drop the original reference from object_new() once backend
> > is attached to its parent.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >   hw/misc/ivshmem.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> > index f88910e..ecfd10a 100644
> > --- a/hw/misc/ivshmem.c
> > +++ b/hw/misc/ivshmem.c
> > @@ -1279,6 +1279,7 @@ static void desugar_shm(IVShmemState *s)
> >       object_property_set_bool(obj, true, "share", &error_abort);
> >       object_property_add_child(OBJECT(s), "internal-shm-backend", obj,
> >                                 &error_abort);
> > +    object_unref(obj);
> >       user_creatable_complete(obj, &error_abort);
> >       s->hostmem = MEMORY_BACKEND(obj);
> >   }
> >   
>

Re: [Qemu-devel] [PATCH] ivshmem: fix memory backend leak

Posted by Paolo Bonzini 5 years, 5 months ago

On 01/11/2018 11:44, Igor Mammedov wrote:
> object_new() returns a new backend with refcount == 1 and
> then later object_property_add_child() increases refcount to 2
> So when ivshmem is desroyed, the backend it has created isn't
> destroyed along with it as children cleanup will bring
> backend's refcount only to 1, which leaks backend including
> resources it is using.
> 
> Drop the original reference from object_new() once backend
> is attached to its parent.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/misc/ivshmem.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index f88910e..ecfd10a 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -1279,6 +1279,7 @@ static void desugar_shm(IVShmemState *s)
>      object_property_set_bool(obj, true, "share", &error_abort);
>      object_property_add_child(OBJECT(s), "internal-shm-backend", obj,
>                                &error_abort);
> +    object_unref(obj);
>      user_creatable_complete(obj, &error_abort);
>      s->hostmem = MEMORY_BACKEND(obj);
>  }
> 

Queued, thanks.

Paolo