| Message ID | 1541069086-167036-1-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | ivshmem: fix memory backend leak | expand |
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 > >
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 > > > > > >
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); > } >
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); > > } > > >
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>
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
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); }
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(+)