[v3] Fix memory leak of some device state in migration

[PATCH v3 3/3] vmstate: Fix memory leak in vmstate_handle_alloc()

Posted by Jinhao Gao 4 years, 10 months ago

Some memory allocated for fields having a flag of VMS_ALLOC in SaveState
may not free before VM load vmsd in migration. So we pre-free memory before
allocation in vmstate_handle_alloc() to avoid memleaks.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Jinhao Gao <gaojinhao@huawei.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 migration/vmstate.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index e9d2aef66b..873f76739f 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -70,6 +70,7 @@ static void vmstate_handle_alloc(void *ptr, const VMStateField *field,
         gsize size = vmstate_size(opaque, field);
         size *= vmstate_n_elems(opaque, field);
         if (size) {
+            g_free(*(void **)ptr);
             *(void **)ptr = g_malloc(size);
         }
     }
-- 
2.23.0

Re: [PATCH v3 3/3] vmstate: Fix memory leak in vmstate_handle_alloc()

Posted by Dr. David Alan Gilbert 4 years, 10 months ago

* Jinhao Gao (gaojinhao@huawei.com) wrote:
> Some memory allocated for fields having a flag of VMS_ALLOC in SaveState
> may not free before VM load vmsd in migration. So we pre-free memory before
> allocation in vmstate_handle_alloc() to avoid memleaks.
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Jinhao Gao <gaojinhao@huawei.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Yes, I think that's OK; it's actually pretty rare for this to happen;
normally inwards migrations either succeed or fail and exit; doing
multiple loads from snapshots is valid and I guess COLO hits this as well.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/vmstate.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index e9d2aef66b..873f76739f 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -70,6 +70,7 @@ static void vmstate_handle_alloc(void *ptr, const VMStateField *field,
>          gsize size = vmstate_size(opaque, field);
>          size *= vmstate_n_elems(opaque, field);
>          if (size) {
> +            g_free(*(void **)ptr);
>              *(void **)ptr = g_malloc(size);
>          }
>      }
> -- 
> 2.23.0
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

RE: [PATCH v3 3/3] vmstate: Fix memory leak in vmstate_handle_alloc()

Posted by gaojinhao 4 years, 10 months ago

Thank you for review!

Jinhao Gao

-----Original Message-----
From: Dr. David Alan Gilbert [mailto:dgilbert@redhat.com] 
Sent: 2021年1月5日 19:18
To: gaojinhao <gaojinhao@huawei.com>
Cc: qemu-ppc@nongnu.org; qemu-devel@nongnu.org; Michael S . Tsirkin <mst@redhat.com>; David Gibson <david@gibson.dropbear.id.au>; Greg Kurz <groug@kaod.org>; Marc-André Lureau <marcandre.lureau@redhat.com>; Stefan Berger <stefanb@linux.vnet.ibm.com>; Jason Wang <jasowang@redhat.com>; Juan Quintela <quintela@redhat.com>; Wanghaibin (D) <wanghaibin.wang@huawei.com>; zhukeqian <zhukeqian1@huawei.com>
Subject: Re: [PATCH v3 3/3] vmstate: Fix memory leak in vmstate_handle_alloc()

* Jinhao Gao (gaojinhao@huawei.com) wrote:
> Some memory allocated for fields having a flag of VMS_ALLOC in 
> SaveState may not free before VM load vmsd in migration. So we 
> pre-free memory before allocation in vmstate_handle_alloc() to avoid memleaks.
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Jinhao Gao <gaojinhao@huawei.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Yes, I think that's OK; it's actually pretty rare for this to happen; normally inwards migrations either succeed or fail and exit; doing multiple loads from snapshots is valid and I guess COLO hits this as well.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/vmstate.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/migration/vmstate.c b/migration/vmstate.c index 
> e9d2aef66b..873f76739f 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -70,6 +70,7 @@ static void vmstate_handle_alloc(void *ptr, const VMStateField *field,
>          gsize size = vmstate_size(opaque, field);
>          size *= vmstate_n_elems(opaque, field);
>          if (size) {
> +            g_free(*(void **)ptr);
>              *(void **)ptr = g_malloc(size);
>          }
>      }
> --
> 2.23.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v3 3/3] vmstate: Fix memory leak in vmstate_handle_alloc()

Posted by Dr. David Alan Gilbert 4 years, 9 months ago

* Dr. David Alan Gilbert (dgilbert@redhat.com) wrote:
> * Jinhao Gao (gaojinhao@huawei.com) wrote:
> > Some memory allocated for fields having a flag of VMS_ALLOC in SaveState
> > may not free before VM load vmsd in migration. So we pre-free memory before
> > allocation in vmstate_handle_alloc() to avoid memleaks.
> > 
> > Reported-by: Euler Robot <euler.robot@huawei.com>
> > Signed-off-by: Jinhao Gao <gaojinhao@huawei.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Yes, I think that's OK; it's actually pretty rare for this to happen;
> normally inwards migrations either succeed or fail and exit; doing
> multiple loads from snapshots is valid and I guess COLO hits this as well.
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

I'm having to unqueue this because it's triggering a seg fault on Power
in iotest 267 (just run make check).

#2  0x0000000116d0d4c8 in vmstate_handle_alloc (opaque=<optimized out>, field=0x11799e0c8 <__compound_literal.1+312>, ptr=0x1001f8f14b0) at ../qemu/migration/vmstate.c:73
#3  0x0000000116d0d4c8 in vmstate_load_state (f=0x1001f6d0000, vmsd=0x117928730 <vmstate_spapr_tce_table>, opaque=0x1001f8f1400, version_id=<optimized out>) at ../qemu/migration/vmstate.c:122
#4  0x0000000116fb4a4c in vmstate_load (f=0x1001f6d0000, se=0x1001fc7bc40) at ../qemu/migration/savevm.c:910
#5  0x0000000116fb5010 in qemu_loadvm_section_start_full (f=f@entry=0x1001f6d0000, mis=<optimized out>) at ../qemu/migration/savevm.c:2433

It's the mig_nb_table that Power is doing some special
handling with; so it needs some more checking before
we can fix this.

Dave

> > ---
> >  migration/vmstate.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/migration/vmstate.c b/migration/vmstate.c
> > index e9d2aef66b..873f76739f 100644
> > --- a/migration/vmstate.c
> > +++ b/migration/vmstate.c
> > @@ -70,6 +70,7 @@ static void vmstate_handle_alloc(void *ptr, const VMStateField *field,
> >          gsize size = vmstate_size(opaque, field);
> >          size *= vmstate_n_elems(opaque, field);
> >          if (size) {
> > +            g_free(*(void **)ptr);
> >              *(void **)ptr = g_malloc(size);
> >          }
> >      }
> > -- 
> > 2.23.0
> > 
> -- 
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

RE: [PATCH v3 3/3] vmstate: Fix memory leak in vmstate_handle_alloc()

Posted by gaojinhao 4 years, 9 months ago

Hi, Dave,
I will check the code about vmstate_spapr_tce_table to figure out the problem of seg fault.
Thank you for your check.

Jinhao Gao

-----Original Message-----
From: Dr. David Alan Gilbert [mailto:dgilbert@redhat.com] 
Sent: 2021年2月8日 18:53
To: gaojinhao <gaojinhao@huawei.com>
Cc: qemu-ppc@nongnu.org; qemu-devel@nongnu.org; Michael S . Tsirkin <mst@redhat.com>; David Gibson <david@gibson.dropbear.id.au>; Greg Kurz <groug@kaod.org>; Marc-André Lureau <marcandre.lureau@redhat.com>; Stefan Berger <stefanb@linux.vnet.ibm.com>; Jason Wang <jasowang@redhat.com>; Juan Quintela <quintela@redhat.com>; Wanghaibin (D) <wanghaibin.wang@huawei.com>; zhukeqian <zhukeqian1@huawei.com>
Subject: Re: [PATCH v3 3/3] vmstate: Fix memory leak in vmstate_handle_alloc()

* Dr. David Alan Gilbert (dgilbert@redhat.com) wrote:
> * Jinhao Gao (gaojinhao@huawei.com) wrote:
> > Some memory allocated for fields having a flag of VMS_ALLOC in 
> > SaveState may not free before VM load vmsd in migration. So we 
> > pre-free memory before allocation in vmstate_handle_alloc() to avoid memleaks.
> > 
> > Reported-by: Euler Robot <euler.robot@huawei.com>
> > Signed-off-by: Jinhao Gao <gaojinhao@huawei.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Yes, I think that's OK; it's actually pretty rare for this to happen; 
> normally inwards migrations either succeed or fail and exit; doing 
> multiple loads from snapshots is valid and I guess COLO hits this as well.
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

I'm having to unqueue this because it's triggering a seg fault on Power in iotest 267 (just run make check).

#2  0x0000000116d0d4c8 in vmstate_handle_alloc (opaque=<optimized out>, field=0x11799e0c8 <__compound_literal.1+312>, ptr=0x1001f8f14b0) at ../qemu/migration/vmstate.c:73
#3  0x0000000116d0d4c8 in vmstate_load_state (f=0x1001f6d0000, vmsd=0x117928730 <vmstate_spapr_tce_table>, opaque=0x1001f8f1400, version_id=<optimized out>) at ../qemu/migration/vmstate.c:122
#4  0x0000000116fb4a4c in vmstate_load (f=0x1001f6d0000, se=0x1001fc7bc40) at ../qemu/migration/savevm.c:910
#5  0x0000000116fb5010 in qemu_loadvm_section_start_full (f=f@entry=0x1001f6d0000, mis=<optimized out>) at ../qemu/migration/savevm.c:2433

It's the mig_nb_table that Power is doing some special handling with; so it needs some more checking before we can fix this.

Dave

> > ---
> >  migration/vmstate.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/migration/vmstate.c b/migration/vmstate.c index 
> > e9d2aef66b..873f76739f 100644
> > --- a/migration/vmstate.c
> > +++ b/migration/vmstate.c
> > @@ -70,6 +70,7 @@ static void vmstate_handle_alloc(void *ptr, const VMStateField *field,
> >          gsize size = vmstate_size(opaque, field);
> >          size *= vmstate_n_elems(opaque, field);
> >          if (size) {
> > +            g_free(*(void **)ptr);
> >              *(void **)ptr = g_malloc(size);
> >          }
> >      }
> > --
> > 2.23.0
> > 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK