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

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

[g00517791]

From: Jinhao Gao <gaojinhao@huawei.com>

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 allocation
of memory or after load of memory to avoid memory leak.




Jinhao Gao (8):
  vmbus: Fix memory leak of vmstate_gpadl
  virtio-net: Fix memory leak of vmstate_virtio_net_rss
  spapr: Fix memory leak of vmstate_spapr_event_entry
  spapr_pci: Fix memory leak of vmstate_spapr_pci
  savevm: Fix memory leak of vmstate_configuration
  vmbus: Fix memory leak of vmstate_vmbus_chan_req
  tpm_emulator: Fix memory leak of vmstate_tpm_emulator
  dbus-vmstate: Fix memory leak of dbus_vmstate

 backends/dbus-vmstate.c     | 11 +++++++++++
 backends/tpm/tpm_emulator.c | 13 +++++++++++++
 hw/hyperv/vmbus.c           | 22 ++++++++++++++++++++++
 hw/net/virtio-net.c         | 11 +++++++++++
 hw/ppc/spapr.c              | 12 ++++++++++++
 hw/ppc/spapr_pci.c          | 11 +++++++++++
 migration/savevm.c          | 31 +++++++++++++++++++++++++++----
 7 files changed, 107 insertions(+), 4 deletions(-)

-- 
2.23.0

[PATCH 1/8] vmbus: Fix memory leak of vmstate_gpadl

[g00517791]

From: Jinhao Gao <gaojinhao@huawei.com>

When VM migrate VMState of vmbus/gpadl, the field(gfns) of vmbus/
gpadl having a flag of VMS_ALLOC needs to allocate memory. If the
dst doesn't free memory which has been allocated for SaveStateEntry
of vmbus/gpadl before dst loads device state, it may result that
the pointer of gfns is overlaid when vm loads. We add the pre_load
func to free memory, which prevents memory leak.

Signed-off-by: Jinhao Gao <gaojinhao@huawei.com>
---
 hw/hyperv/vmbus.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/hyperv/vmbus.c b/hw/hyperv/vmbus.c
index 896e981f85..a91623aaac 100644
--- a/hw/hyperv/vmbus.c
+++ b/hw/hyperv/vmbus.c
@@ -519,10 +519,21 @@ void vmbus_unmap_sgl(VMBusChanReq *req, DMADirection dir, struct iovec *iov,
     }
 }
 
+static int vmbus_gpadl_pre_load(void *opaque)
+{
+    VMBusGpadl *gpadl = VMBusGpadl(opaque);
+
+    g_free(gpadl->gfns);
+    gpadl->gfns = NULL;
+    gpadl->num_gfns =0;
+    return 0;
+}
+
 static const VMStateDescription vmstate_gpadl = {
     .name = "vmbus/gpadl",
     .version_id = 0,
     .minimum_version_id = 0,
+    .pre_load = vmbus_gpadl_pre_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(id, VMBusGpadl),
         VMSTATE_UINT32(child_relid, VMBusGpadl),
-- 
2.23.0

[PATCH 2/8] virtio-net: Fix memory leak of vmstate_virtio_net_rss

[g00517791]

From: Jinhao Gao <gaojinhao@huawei.com>

When VM migrate VMState of virtio-net-device/rss, the field(rss_data.
indirections_table) of virtio-net-device/rss having a flag of VMS_ALLOC
needs to allocate memory. If the dst doesn't free memory which has bee
n allocated for SaveStateEntry of virtio-net-device/rss before dst loads
device state, it may result that the pointer of rss_data.indirections_table
is overlaid when vm loads. We add the pre_load func to free memory, which
prevents memory leak.

Signed-off-by: Jinhao Gao <gaojinhao@huawei.com>
---
 hw/net/virtio-net.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 044ac95f6f..102e51f32f 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -2957,11 +2957,22 @@ static bool virtio_net_rss_needed(void *opaque)
     return VIRTIO_NET(opaque)->rss_data.enabled;
 }
 
+static int virtio_net_rss_pre_load(void *opaque)
+{
+    VirtIONet *n = VIRTIO_NET(opaque);
+
+    g_free(n->rss_data.indirections_table);
+    n->rss_data.indirections_table = NULL;
+    n->rss_data.indirections_len = 0;
+    return 0;
+}
+
 static const VMStateDescription vmstate_virtio_net_rss = {
     .name      = "virtio-net-device/rss",
     .version_id = 1,
     .minimum_version_id = 1,
     .needed = virtio_net_rss_needed,
+    .pre_load = virtio_net_rss_pre_load,
     .fields = (VMStateField[]) {
         VMSTATE_BOOL(rss_data.enabled, VirtIONet),
         VMSTATE_BOOL(rss_data.redirect, VirtIONet),
-- 
2.23.0

[PATCH 3/8] spapr: Fix memory leak of vmstate_spapr_event_entry

[g00517791]

From: Jinhao Gao <gaojinhao@huawei.com>

When VM migrate VMState of spapr_event_log_entry, the field(extended_log)
of spapr_event_log_entry having a flag of VMS_ALLOC needs to allocate
memory. If the dst doesn't free memory which has been allocated for
SaveStateEntry of spapr_event_log_entry before dst loads device state,
it may result that the pointer of extended_log is overlaid when vm loads.
We add the pre_load func to free memory, which prevents memory leak.

Signed-off-by: Jinhao Gao <gaojinhao@huawei.com>
---
 hw/ppc/spapr.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 489cefcb81..ddfed1e7ca 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1799,10 +1799,22 @@ static bool spapr_pending_events_needed(void *opaque)
     return !QTAILQ_EMPTY(&spapr->pending_events);
 }
 
+static int spapr_event_log_entry_pre_load(void *opaque)
+{
+    SpaprEventLogEntry *entry = opaque;
+
+    g_free(entry->extended_log);
+    entry->extended_log = NULL;
+    entry->extended_length = 0;
+
+    return 0;
+}
+
 static const VMStateDescription vmstate_spapr_event_entry = {
     .name = "spapr_event_log_entry",
     .version_id = 1,
     .minimum_version_id = 1,
+    .pre_load = spapr_event_log_entry_pre_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(summary, SpaprEventLogEntry),
         VMSTATE_UINT32(extended_length, SpaprEventLogEntry),
-- 
2.23.0

[PATCH 4/8] spapr_pci: Fix memory leak of vmstate_spapr_pci

[g00517791]

From: Jinhao Gao <gaojinhao@huawei.com>

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.

Signed-off-by: Jinhao Gao <gaojinhao@huawei.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 5/8] savevm: Fix memory leak of vmstate_configuration

[g00517791]

From: Jinhao Gao <gaojinhao@huawei.com>

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.

Signed-off-by: Jinhao Gao <gaojinhao@huawei.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 6/8] vmbus: Fix memory leak of vmstate_vmbus_chan_req

[g00517791]

From: Jinhao Gao <gaojinhao@huawei.com>

When VM migrate VMState of vmbus/vmbus_chan_req, the field(msg) of
vmbus/vmbus_chan_req having a flag of VMS_ALLOC needs to allocate
memory. If the dst doesn't free memory which has been allocated for
SaveStateEntry of vmbus/vmbus_chan_req before dst loads device state,
it may result that the pointer of msg is overlaid when vm loads.
We add the pre_load func to free memory, which prevents memory leak.

Signed-off-by: Jinhao Gao <gaojinhao@huawei.com>
---
 hw/hyperv/vmbus.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/hyperv/vmbus.c b/hw/hyperv/vmbus.c
index a91623aaac..9eda2341f3 100644
--- a/hw/hyperv/vmbus.c
+++ b/hw/hyperv/vmbus.c
@@ -1303,10 +1303,21 @@ typedef struct VMBusChanReqSave {
     ScatterGatherEntry *sgl;
 } VMBusChanReqSave;
 
+static int vmbus_chan_req_pre_load(void *opaque)
+{
+    VMBusChanReqSave *req_save = VMBusChanReqSave(opaque);
+
+    g_free(req_save.msg);
+    req_save.msg = NULL;
+    req_save.msglen = 0;
+    return 0;
+}
+
 static const VMStateDescription vmstate_vmbus_chan_req = {
     .name = "vmbus/vmbus_chan_req",
     .version_id = 0,
     .minimum_version_id = 0,
+    .pre_load = vmbus_chan_req_pre_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT16(chan_idx, VMBusChanReqSave),
         VMSTATE_UINT16(pkt_type, VMBusChanReqSave),
-- 
2.23.0

[PATCH 7/8] tpm_emulator: Fix memory leak of vmstate_tpm_emulator

[g00517791]

From: Jinhao Gao <gaojinhao@huawei.com>

When VM migrate VMState of tpm-emulator, the fields(state_blobs.
permanent.buffer, state_blobs.volatil.buffer and state_blobs.savestate.
buffer) of tpm-emulator having a flag of VMS_ALLOC need to allocate
memory. If the dst doesn't free memory which has been allocated for
SaveStateEntry of tpm-emulator before dst loads device state, it may
result that the pointers of state_blobs.permanent.buffer, state_blobs.
volatil.buffer and state_blobs.savestate.buffer are overlaid when vm
loads. We add the pre_load func to free memory, which prevents memory
leak.

Signed-off-by: Jinhao Gao <gaojinhao@huawei.com>
---
 backends/tpm/tpm_emulator.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
index a012adc193..7ffa95dbce 100644
--- a/backends/tpm/tpm_emulator.c
+++ b/backends/tpm/tpm_emulator.c
@@ -857,6 +857,18 @@ static int tpm_emulator_pre_save(void *opaque)
     return tpm_emulator_get_state_blobs(tpm_emu);
 }
 
+static int tpm_emulator_pre_load(void *opaque)
+{
+    TPMBackend *tb = opaque;
+    TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
+    TPMBlobBuffers *state_blobs = &tpm_emu->state_blobs;
+
+    tpm_sized_buffer_reset(&state_blobs->volatil);
+    tpm_sized_buffer_reset(&state_blobs->permanent);
+    tpm_sized_buffer_reset(&state_blobs->savestate);
+    return 0;
+}
+
 /*
  * Load the TPM state blobs into the TPM.
  *
@@ -883,6 +895,7 @@ static const VMStateDescription vmstate_tpm_emulator = {
     .name = "tpm-emulator",
     .version_id = 0,
     .pre_save = tpm_emulator_pre_save,
+    .pre_load = tpm_emulator_pre_load,
     .post_load = tpm_emulator_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(state_blobs.permanent_flags, TPMEmulator),
-- 
2.23.0

[PATCH 8/8] dbus-vmstate: Fix memory leak of dbus_vmstate

[g00517791]

From: Jinhao Gao <gaojinhao@huawei.com>

When VM migrate VMState of dbus_vmstate, the field(data) of
dbus_vmstate having a flag of VMS_ALLOC needs to allocate memory.
If the dst doesn't free memory which has been allocated for
SaveStateEntry of dbus_vmstate before dst loads device state, it may
result that the pointer of data is overlaid when vm loads. We add
the pre_load func to free memory, which prevents memory leak.

Signed-off-by: Jinhao Gao <gaojinhao@huawei.com>
---
 backends/dbus-vmstate.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/backends/dbus-vmstate.c b/backends/dbus-vmstate.c
index bd050e8e9c..c13fbee760 100644
--- a/backends/dbus-vmstate.c
+++ b/backends/dbus-vmstate.c
@@ -366,10 +366,21 @@ static int dbus_vmstate_pre_save(void *opaque)
     return 0;
 }
 
+static int dbus_vmstate_pre_load(void *opaque)
+{
+    DBusVMState *self = DBUS_VMSTATE(opaque);
+
+    g_free(self->data);
+    self->data = NULL;
+    self->data_size = 0;
+    return 0;
+}
+
 static const VMStateDescription dbus_vmstate = {
     .name = TYPE_DBUS_VMSTATE,
     .version_id = 0,
     .pre_save = dbus_vmstate_pre_save,
+    .pre_load = dbus_vmstate_pre_load,
     .post_load = dbus_vmstate_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(data_size, DBusVMState),
-- 
2.23.0

Re: [PATCH 0/8] Fix memory leak of some device state in migration

[no-reply]

Patchew URL: https://patchew.org/QEMU/20201226103347.868-1-gaojinhao@huawei.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20201226103347.868-1-gaojinhao@huawei.com
Subject: [PATCH 0/8] Fix memory leak of some device state in migration

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20201226103347.868-1-gaojinhao@huawei.com -> patchew/20201226103347.868-1-gaojinhao@huawei.com
Switched to a new branch 'test'
0a34e05 dbus-vmstate: Fix memory leak of dbus_vmstate
1e7ce1c tpm_emulator: Fix memory leak of vmstate_tpm_emulator
e5e20f9 vmbus: Fix memory leak of vmstate_vmbus_chan_req
0cfbfe6 savevm: Fix memory leak of vmstate_configuration
0d767d2 spapr_pci: Fix memory leak of vmstate_spapr_pci
729d773 spapr: Fix memory leak of vmstate_spapr_event_entry
bd9decd virtio-net: Fix memory leak of vmstate_virtio_net_rss
a7debfa vmbus: Fix memory leak of vmstate_gpadl

=== OUTPUT BEGIN ===
1/8 Checking commit a7debfa33be6 (vmbus: Fix memory leak of vmstate_gpadl)
ERROR: spaces required around that '=' (ctx:WxV)
#31: FILE: hw/hyperv/vmbus.c:528:
+    gpadl->num_gfns =0;
                     ^

total: 1 errors, 0 warnings, 21 lines checked

Patch 1/8 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/8 Checking commit bd9decdc8d81 (virtio-net: Fix memory leak of vmstate_virtio_net_rss)
3/8 Checking commit 729d7739521c (spapr: Fix memory leak of vmstate_spapr_event_entry)
4/8 Checking commit 0d767d2f6f68 (spapr_pci: Fix memory leak of vmstate_spapr_pci)
5/8 Checking commit 0cfbfe6f37d9 (savevm: Fix memory leak of vmstate_configuration)
6/8 Checking commit e5e20f91e7ae (vmbus: Fix memory leak of vmstate_vmbus_chan_req)
7/8 Checking commit 1e7ce1c84602 (tpm_emulator: Fix memory leak of vmstate_tpm_emulator)
8/8 Checking commit 0a34e0598fb9 (dbus-vmstate: Fix memory leak of dbus_vmstate)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20201226103347.868-1-gaojinhao@huawei.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 0/8] Fix memory leak of some device state in migration

[Michael S. Tsirkin]

On Sat, Dec 26, 2020 at 06:33:39PM +0800, g00517791 wrote:
> From: Jinhao Gao <gaojinhao@huawei.com>
> 
> 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 allocation
> of memory or after load of memory to avoid memory leak.
> 

Isn't there a way to handle it centrally?
IIUC the issue is repeated loads in case a load fails, right?
So can't we do something along the lines of:

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

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);
         }
     }

-- 
MST

Re: [PATCH 3/8] spapr: Fix memory leak of vmstate_spapr_event_entry

[David Gibson]

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

On Sat, Dec 26, 2020 at 06:33:42PM +0800, g00517791 wrote:
> From: Jinhao Gao <gaojinhao@huawei.com>
> 
> When VM migrate VMState of spapr_event_log_entry, the field(extended_log)
> of spapr_event_log_entry having a flag of VMS_ALLOC needs to allocate
> memory. If the dst doesn't free memory which has been allocated for
> SaveStateEntry of spapr_event_log_entry before dst loads device state,
> it may result that the pointer of extended_log is overlaid when vm loads.
> We add the pre_load func to free memory, which prevents memory leak.
> 
> Signed-off-by: Jinhao Gao <gaojinhao@huawei.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/ppc/spapr.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 489cefcb81..ddfed1e7ca 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1799,10 +1799,22 @@ static bool spapr_pending_events_needed(void *opaque)
>      return !QTAILQ_EMPTY(&spapr->pending_events);
>  }
>  
> +static int spapr_event_log_entry_pre_load(void *opaque)
> +{
> +    SpaprEventLogEntry *entry = opaque;
> +
> +    g_free(entry->extended_log);
> +    entry->extended_log = NULL;
> +    entry->extended_length = 0;
> +
> +    return 0;
> +}
> +
>  static const VMStateDescription vmstate_spapr_event_entry = {
>      .name = "spapr_event_log_entry",
>      .version_id = 1,
>      .minimum_version_id = 1,
> +    .pre_load = spapr_event_log_entry_pre_load,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT32(summary, SpaprEventLogEntry),
>          VMSTATE_UINT32(extended_length, SpaprEventLogEntry),

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/8] spapr_pci: Fix memory leak of vmstate_spapr_pci

[David Gibson]

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

On Sat, Dec 26, 2020 at 06:33:43PM +0800, g00517791 wrote:
> From: Jinhao Gao <gaojinhao@huawei.com>
> 
> 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.
> 
> Signed-off-by: Jinhao Gao <gaojinhao@huawei.com>

Not really a memory leak, since it will get freed on the next
pre_save.  But, we might as well free it earlier if we can ,so

Acked-by: David Gibson <david@gibson.dropbear.id.au>

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

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH 0/8] Fix memory leak of some device state in migration

[gaojinhao]

Thank you for you review. I will modify patches according to your opinion.

Jinhao Gao

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

On Sat, Dec 26, 2020 at 06:33:39PM +0800, g00517791 wrote:
> From: Jinhao Gao <gaojinhao@huawei.com>
> 
> 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 
> allocation of memory or after load of memory to avoid memory leak.
> 

Isn't there a way to handle it centrally?
IIUC the issue is repeated loads in case a load fails, right?
So can't we do something along the lines of:

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

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);
         }
     }

--
MST

RE: [PATCH 4/8] spapr_pci: Fix memory leak of vmstate_spapr_pci

[gaojinhao]

Hi David,
Firstly, thank you for you review. And then for your review, I worry that a memory leak will occur if QEMU exits after saves vmsd. So, we free it in post_save func.

Jinhao Gao

-----Original Message-----
From: David Gibson [mailto:david@gibson.dropbear.id.au] 
Sent: 2020-12-28 14:58
To: gaojinhao <gaojinhao@huawei.com>
Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org; Marc-André Lureau <marcandre.lureau@redhat.com>; Stefan Berger <stefanb@linux.vnet.ibm.com>; Michael S . Tsirkin <mst@redhat.com>; Jason Wang <jasowang@redhat.com>; Greg Kurz <groug@kaod.org>; Juan Quintela <quintela@redhat.com>; Dr . David Alan Gilbert <dgilbert@redhat.com>; Wanghaibin (D) <wanghaibin.wang@huawei.com>; zhukeqian <zhukeqian1@huawei.com>
Subject: Re: [PATCH 4/8] spapr_pci: Fix memory leak of vmstate_spapr_pci

On Sat, Dec 26, 2020 at 06:33:43PM +0800, g00517791 wrote:
> From: Jinhao Gao <gaojinhao@huawei.com>
> 
> 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.
> 
> Signed-off-by: Jinhao Gao <gaojinhao@huawei.com>

Not really a memory leak, since it will get freed on the next pre_save.  But, we might as well free it earlier if we can ,so

Acked-by: David Gibson <david@gibson.dropbear.id.au>

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

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 4/8] spapr_pci: Fix memory leak of vmstate_spapr_pci

[David Gibson]

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

On Mon, Dec 28, 2020 at 08:10:31AM +0000, gaojinhao wrote:
> Hi David,
> Firstly, thank you for you review. And then for your review, I worry
> that a memory leak will occur if QEMU exits after saves vmsd. So, we
> free it in post_save func.

If qemu exits, all its memory will be freed, so we don't care.

> 
> Jinhao Gao
> 
> -----Original Message-----
> From: David Gibson [mailto:david@gibson.dropbear.id.au] 
> Sent: 2020-12-28 14:58
> To: gaojinhao <gaojinhao@huawei.com>
> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org; Marc-André Lureau <marcandre.lureau@redhat.com>; Stefan Berger <stefanb@linux.vnet.ibm.com>; Michael S . Tsirkin <mst@redhat.com>; Jason Wang <jasowang@redhat.com>; Greg Kurz <groug@kaod.org>; Juan Quintela <quintela@redhat.com>; Dr . David Alan Gilbert <dgilbert@redhat.com>; Wanghaibin (D) <wanghaibin.wang@huawei.com>; zhukeqian <zhukeqian1@huawei.com>
> Subject: Re: [PATCH 4/8] spapr_pci: Fix memory leak of vmstate_spapr_pci
> 
> On Sat, Dec 26, 2020 at 06:33:43PM +0800, g00517791 wrote:
> > From: Jinhao Gao <gaojinhao@huawei.com>
> > 
> > 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.
> > 
> > Signed-off-by: Jinhao Gao <gaojinhao@huawei.com>
> 
> Not really a memory leak, since it will get freed on the next pre_save.  But, we might as well free it earlier if we can ,so
> 
> Acked-by: David Gibson <david@gibson.dropbear.id.au>
> 
> > ---
> >  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),
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH 4/8] spapr_pci: Fix memory leak of vmstate_spapr_pci

[gaojinhao]

Thank you for you reply, I understand.

Jinhao Gao

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

On Mon, Dec 28, 2020 at 08:10:31AM +0000, gaojinhao wrote:
> Hi David,
> Firstly, thank you for you review. And then for your review, I worry 
> that a memory leak will occur if QEMU exits after saves vmsd. So, we 
> free it in post_save func.

If qemu exits, all its memory will be freed, so we don't care.

> 
> Jinhao Gao
> 
> -----Original Message-----
> From: David Gibson [mailto:david@gibson.dropbear.id.au]
> Sent: 2020-12-28 14:58
> To: gaojinhao <gaojinhao@huawei.com>
> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org; Marc-André Lureau 
> <marcandre.lureau@redhat.com>; Stefan Berger 
> <stefanb@linux.vnet.ibm.com>; Michael S . Tsirkin <mst@redhat.com>; 
> Jason Wang <jasowang@redhat.com>; Greg Kurz <groug@kaod.org>; Juan 
> Quintela <quintela@redhat.com>; Dr . David Alan Gilbert 
> <dgilbert@redhat.com>; Wanghaibin (D) <wanghaibin.wang@huawei.com>; 
> zhukeqian <zhukeqian1@huawei.com>
> Subject: Re: [PATCH 4/8] spapr_pci: Fix memory leak of 
> vmstate_spapr_pci
> 
> On Sat, Dec 26, 2020 at 06:33:43PM +0800, g00517791 wrote:
> > From: Jinhao Gao <gaojinhao@huawei.com>
> > 
> > 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.
> > 
> > Signed-off-by: Jinhao Gao <gaojinhao@huawei.com>
> 
> Not really a memory leak, since it will get freed on the next 
> pre_save.  But, we might as well free it earlier if we can ,so
> 
> Acked-by: David Gibson <david@gibson.dropbear.id.au>
> 
> > ---
> >  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),
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson