[PATCH v3 0/3] Fix memory leak of some device state in migration

[PATCH v3 0/3] Fix memory leak of some device state in migration

[Jinhao Gao]

For some device state having some fields of VMS_ALLOC flag, they
don't free memory allocated for the fields in vmstate_save_state
and vmstate_load_state. We add funcs or sentences of free memory
before and after VM saves or loads device state to avoid memory leak.

v2
 - Drop patch1-3,6-8 of v1
 - Address Michael's comment (free memory before load vmsd centrally)
 - Add David's Acked-by and Michael's Signed-off-by

v3
 - Add Euler's Reported-by and Michael's Reviewed-by

Jinhao Gao (3):
  spapr_pci: Fix memory leak of vmstate_spapr_pci
  savevm: Fix memory leak of vmstate_configuration
  vmstate: Fix memory leak in vmstate_handle_alloc()

 hw/ppc/spapr_pci.c  | 11 +++++++++++
 migration/savevm.c  | 31 +++++++++++++++++++++++++++----
 migration/vmstate.c |  1 +
 3 files changed, 39 insertions(+), 4 deletions(-)

-- 
2.23.0

[PATCH v3 1/3] spapr_pci: Fix memory leak of vmstate_spapr_pci

[Jinhao Gao]

When VM migrate VMState of spapr_pci, the field(msi_devs) of spapr_pci
having a flag of VMS_ALLOC need to allocate memory. If the src doesn't free
memory of msi_devs in SaveStateEntry of spapr_pci after QEMUFile save
VMState of spapr_pci, it may result in memory leak of msi_devs. We add the
post_save func to free memory, which prevents memory leak.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Jinhao Gao <gaojinhao@huawei.com>
Acked-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/ppc/spapr_pci.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 76d7c91e9c..1b2b940606 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -2173,6 +2173,16 @@ static int spapr_pci_pre_save(void *opaque)
     return 0;
 }
 
+static int spapr_pci_post_save(void *opaque)
+{
+    SpaprPhbState *sphb = opaque;
+
+    g_free(sphb->msi_devs);
+    sphb->msi_devs = NULL;
+    sphb->msi_devs_num = 0;
+    return 0;
+}
+
 static int spapr_pci_post_load(void *opaque, int version_id)
 {
     SpaprPhbState *sphb = opaque;
@@ -2205,6 +2215,7 @@ static const VMStateDescription vmstate_spapr_pci = {
     .version_id = 2,
     .minimum_version_id = 2,
     .pre_save = spapr_pci_pre_save,
+    .post_save = spapr_pci_post_save,
     .post_load = spapr_pci_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT64_EQUAL(buid, SpaprPhbState, NULL),
-- 
2.23.0

[PATCH v3 2/3] savevm: Fix memory leak of vmstate_configuration

[Jinhao Gao]

When VM migrate VMState of configuration, the fields(name and capabilities)
of configuration having a flag of VMS_ALLOC need to allocate memory. If the
src doesn't free memory of capabilities in SaveState after save VMState of
configuration, or the dst doesn't free memory of name and capabilities in post
load of configuration, it may result in memory leak of name and capabilities.
We free memory in configuration_post_save and configuration_post_load func,
which prevents memory leak.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Jinhao Gao <gaojinhao@huawei.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
---
 migration/savevm.c | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 5f937a2762..13f1a5dab7 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -314,6 +314,16 @@ static int configuration_pre_save(void *opaque)
     return 0;
 }
 
+static int configuration_post_save(void *opaque)
+{
+    SaveState *state = opaque;
+
+    g_free(state->capabilities);
+    state->capabilities = NULL;
+    state->caps_count = 0;
+    return 0;
+}
+
 static int configuration_pre_load(void *opaque)
 {
     SaveState *state = opaque;
@@ -364,24 +374,36 @@ static int configuration_post_load(void *opaque, int version_id)
 {
     SaveState *state = opaque;
     const char *current_name = MACHINE_GET_CLASS(current_machine)->name;
+    int ret = 0;
 
     if (strncmp(state->name, current_name, state->len) != 0) {
         error_report("Machine type received is '%.*s' and local is '%s'",
                      (int) state->len, state->name, current_name);
-        return -EINVAL;
+        ret = -EINVAL;
+        goto out;
     }
 
     if (state->target_page_bits != qemu_target_page_bits()) {
         error_report("Received TARGET_PAGE_BITS is %d but local is %d",
                      state->target_page_bits, qemu_target_page_bits());
-        return -EINVAL;
+        ret = -EINVAL;
+        goto out;
     }
 
     if (!configuration_validate_capabilities(state)) {
-        return -EINVAL;
+        ret = -EINVAL;
+        goto out;
     }
 
-    return 0;
+out:
+    g_free((void *)state->name);
+    state->name = NULL;
+    state->len = 0;
+    g_free(state->capabilities);
+    state->capabilities = NULL;
+    state->caps_count = 0;
+
+    return ret;
 }
 
 static int get_capability(QEMUFile *f, void *pv, size_t size,
@@ -515,6 +537,7 @@ static const VMStateDescription vmstate_configuration = {
     .pre_load = configuration_pre_load,
     .post_load = configuration_post_load,
     .pre_save = configuration_pre_save,
+    .post_save = configuration_post_save,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(len, SaveState),
         VMSTATE_VBUFFER_ALLOC_UINT32(name, SaveState, 0, NULL, len),
-- 
2.23.0

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

[Jinhao Gao]

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()

[Dr. David Alan Gilbert]

* 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()

[gaojinhao]

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 0/3] Fix memory leak of some device state in migration

[Dr. David Alan Gilbert]

* Jinhao Gao (gaojinhao@huawei.com) wrote:
> For some device state having some fields of VMS_ALLOC flag, they
> don't free memory allocated for the fields in vmstate_save_state
> and vmstate_load_state. We add funcs or sentences of free memory
> before and after VM saves or loads device state to avoid memory leak.

Queued

> 
> v2
>  - Drop patch1-3,6-8 of v1
>  - Address Michael's comment (free memory before load vmsd centrally)
>  - Add David's Acked-by and Michael's Signed-off-by
> 
> v3
>  - Add Euler's Reported-by and Michael's Reviewed-by
> 
> Jinhao Gao (3):
>   spapr_pci: Fix memory leak of vmstate_spapr_pci
>   savevm: Fix memory leak of vmstate_configuration
>   vmstate: Fix memory leak in vmstate_handle_alloc()
> 
>  hw/ppc/spapr_pci.c  | 11 +++++++++++
>  migration/savevm.c  | 31 +++++++++++++++++++++++++++----
>  migration/vmstate.c |  1 +
>  3 files changed, 39 insertions(+), 4 deletions(-)
> 
> -- 
> 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()

[Dr. David Alan Gilbert]

* 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()

[gaojinhao]

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