[PATCH v3 0/6] migration: check required entries and sections are loaded

[PATCH v3 0/6] migration: check required entries and sections are loaded

[marcandre.lureau]

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

Hi,

Surprisingly, the migration code doesn't check that required migration entries
and subsections are loaded. Either optional or required sections are both
ignored when missing. According to the documentation a "newer QEMU that knows
about a subsection can (with care) load a stream from an older QEMU that didn't
send the subsection". I propose this behaviour to be limited to "optional"
sections only.

This series has a few preliminary fixes, add new checks that entries are
loaded once and required ones have been loaded, add some tests and
documentation update.

thanks

v3:
 - rebased, drop RFC status
 - switch from tracepoint + returning an error to report for missing
   subsections, as we worry about potential regressions
 - add r-b tags

v2:
 - add "migration: rename vmstate_save_needed->vmstate_section_needed"
 - add "migration: set file error on subsection loading"
 - add subsection tests
 - update the documentation

Marc-André Lureau (6):
  block/fdc: 'phase' is not needed on load
  virtio: make endian_needed() work during loading
  migration: check required subsections are loaded, once
  migration: check required entries are loaded, once
  test-vmstate: add some subsection tests
  docs/migration: reflect the changes about needed subsections

 docs/devel/migration.rst  |  17 +++---
 hw/block/fdc.c            |   5 ++
 hw/virtio/virtio.c        |   6 +-
 migration/savevm.c        |  43 ++++++++++++++
 migration/vmstate.c       |  40 ++++++++++++-
 tests/unit/test-vmstate.c | 116 ++++++++++++++++++++++++++++++++++++++
 6 files changed, 215 insertions(+), 12 deletions(-)

-- 
2.41.0

[PATCH v3 1/6] block/fdc: 'phase' is not needed on load

[marcandre.lureau]

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

It is reconstructed during fdc_post_load()

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/block/fdc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index d7cc4d3ec1..fc71660ba0 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -1005,6 +1005,11 @@ static bool fdc_phase_needed(void *opaque)
 {
     FDCtrl *fdctrl = opaque;
 
+    /* not needed on load */
+    if (fdctrl->phase == FD_PHASE_RECONSTRUCT) {
+        return false;
+    }
+
     return reconstruct_phase(fdctrl) != fdctrl->phase;
 }
 
-- 
2.41.0

[PATCH v3 2/6] virtio: make endian_needed() work during loading

[marcandre.lureau]

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

There is no simple way to distinguish when the callback is used for load
or save, AFAICT.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/virtio.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index e5105571cf..22aeb88235 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2517,7 +2517,11 @@ static bool virtio_device_endian_needed(void *opaque)
 {
     VirtIODevice *vdev = opaque;
 
-    assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
+    /* On load, endian is UNKNOWN. The section might be loaded as required. */
+    if (vdev->device_endian == VIRTIO_DEVICE_ENDIAN_UNKNOWN) {
+        return false;
+    }
+
     if (!virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
         return vdev->device_endian != virtio_default_endian();
     }
-- 
2.41.0

[PATCH v3 3/6] migration: check required subsections are loaded, once

[marcandre.lureau]

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

Check that required subsections have been loaded.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
 migration/vmstate.c | 40 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index b7723a4187..9d7a58d26b 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -452,22 +452,51 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
 }
 
 static const VMStateDescription *
-vmstate_get_subsection(const VMStateDescription **sub, char *idstr)
+vmstate_get_subsection(const VMStateDescription **sub, char *idstr, bool *visited)
 {
+    size_t i = 0;
+
     while (sub && *sub) {
         if (strcmp(idstr, (*sub)->name) == 0) {
+            if (visited[i]) {
+                return NULL;
+            }
+            visited[i] = true;
             return *sub;
         }
+        i++;
         sub++;
     }
     return NULL;
 }
 
+static size_t
+vmstate_get_n_subsections(const VMStateDescription **sub)
+{
+    size_t n = 0;
+
+    if (!sub) {
+        return 0;
+    }
+
+    while (sub[n]) {
+        n++;
+    }
+
+    return n;
+}
+
 static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
                                    void *opaque)
 {
+    size_t i, n;
+    g_autofree bool *visited = NULL;
+
     trace_vmstate_subsection_load(vmsd->name);
 
+    n = vmstate_get_n_subsections(vmsd->subsections);
+    visited = g_new0(bool, n);
+
     while (qemu_peek_byte(f, 0) == QEMU_VM_SUBSECTION) {
         char idstr[256], *idstr_ret;
         int ret;
@@ -493,7 +522,7 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
             /* it doesn't have a valid subsection name */
             return 0;
         }
-        sub_vmsd = vmstate_get_subsection(vmsd->subsections, idstr);
+        sub_vmsd = vmstate_get_subsection(vmsd->subsections, idstr, visited);
         if (sub_vmsd == NULL) {
             trace_vmstate_subsection_load_bad(vmsd->name, idstr, "(lookup)");
             return -ENOENT;
@@ -510,6 +539,13 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
         }
     }
 
+    for (i = 0; i < n; i++) {
+        if (!visited[i] && vmstate_section_needed(vmsd->subsections[i], opaque)) {
+            error_report("Subsection %s %s missing",
+                         vmsd->name, vmsd->subsections[i]->name);
+        }
+    }
+
     trace_vmstate_subsection_load_good(vmsd->name);
     return 0;
 }
-- 
2.41.0

[PATCH v3 4/6] migration: check required entries are loaded, once

[marcandre.lureau]

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

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
 migration/savevm.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/migration/savevm.c b/migration/savevm.c
index bc98c2ea6f..2ae65b8088 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -217,6 +217,7 @@ typedef struct SaveStateEntry {
     void *opaque;
     CompatEntry *compat;
     int is_ram;
+    bool visited;
 } SaveStateEntry;
 
 typedef struct SaveState {
@@ -1787,6 +1788,36 @@ int qemu_save_device_state(QEMUFile *f)
     return qemu_file_get_error(f);
 }
 
+static void savevm_reset_visited(void)
+{
+    SaveStateEntry *se;
+
+    QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
+        se->visited = false;
+    }
+}
+
+static bool loadvm_check_visited(Error **errp)
+{
+    SaveStateEntry *se;
+
+    QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
+        if (se->ops && se->ops->is_active && !se->ops->is_active(se->opaque)) {
+            continue;
+        }
+        if (se->vmsd && !vmstate_section_needed(se->vmsd, se->opaque)) {
+            continue;
+        }
+        if (!se->visited) {
+            error_setg(errp, "Missing entry '%s' while loading VM", se->idstr);
+            return false;
+        }
+    }
+
+    return true;
+}
+
+
 static SaveStateEntry *find_se(const char *idstr, uint32_t instance_id)
 {
     SaveStateEntry *se;
@@ -2592,6 +2623,11 @@ qemu_loadvm_section_start_full(QEMUFile *f, MigrationIncomingState *mis,
                      idstr, instance_id);
         return -EINVAL;
     }
+    if (se->visited) {
+        error_report("error while loading state for instance 0x%"PRIx32" of"
+                     " device '%s'", instance_id, idstr);
+        return -EINVAL;
+    }
 
     /* Validate version */
     if (version_id > se->version_id) {
@@ -2629,6 +2665,8 @@ qemu_loadvm_section_start_full(QEMUFile *f, MigrationIncomingState *mis,
         return -EINVAL;
     }
 
+    se->visited = true;
+
     return 0;
 }
 
@@ -2950,7 +2988,12 @@ int qemu_loadvm_state(QEMUFile *f)
 
     cpu_synchronize_all_pre_loadvm();
 
+    savevm_reset_visited();
     ret = qemu_loadvm_state_main(f, mis);
+    if (!loadvm_check_visited(&local_err)) {
+        error_report_err(local_err);
+        return -EINVAL;
+    }
     qemu_event_set(&mis->main_thread_load_event);
 
     trace_qemu_loadvm_state_post_main(ret);
-- 
2.41.0

[PATCH v3 5/6] test-vmstate: add some subsection tests

[marcandre.lureau]

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

Check subsection support, and optional handling.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
 tests/unit/test-vmstate.c | 116 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 116 insertions(+)

diff --git a/tests/unit/test-vmstate.c b/tests/unit/test-vmstate.c
index 0b7d5ecd68..d60457486c 100644
--- a/tests/unit/test-vmstate.c
+++ b/tests/unit/test-vmstate.c
@@ -1479,6 +1479,118 @@ static void test_tmp_struct(void)
     g_assert_cmpint(obj.f, ==, 8); /* From the child->parent */
 }
 
+static bool sub_optional_needed = true;
+
+static bool sub_optional_needed_cb(void *opaque)
+{
+    return sub_optional_needed;
+}
+
+static const VMStateDescription vmstate_sub_optional_a = {
+    .name = "sub/optional/a",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = sub_optional_needed_cb,
+    .fields = (VMStateField[]) {
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static const VMStateDescription vmstate_sub_optional = {
+    .name = "sub/optional",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_END_OF_LIST()
+    },
+    .subsections = (const VMStateDescription * []) {
+        &vmstate_sub_optional_a,
+    }
+};
+
+static uint8_t wire_sub_optional[] = {
+    QEMU_VM_SUBSECTION,
+    14,
+    's', 'u', 'b', '/', 'o', 'p', 't', 'i', 'o', 'n', 'a', 'l', '/', 'a',
+    0x0, 0x0, 0x0, 1,
+    QEMU_VM_EOF,
+};
+
+static uint8_t wire_sub_optional_missing[] = {
+    QEMU_VM_EOF,
+};
+
+static void test_sub_optional_needed(void)
+{
+    sub_optional_needed = true;
+    save_vmstate(&vmstate_sub_optional, NULL);
+
+    compare_vmstate(wire_sub_optional, sizeof(wire_sub_optional));
+
+    SUCCESS(load_vmstate_one(&vmstate_sub_optional, NULL,
+                             1, wire_sub_optional,
+                             sizeof(wire_sub_optional)));
+
+    /* this will print an error, but succeed nonetheless */
+    load_vmstate_one(&vmstate_sub_optional, NULL,
+                     1, wire_sub_optional_missing,
+                     sizeof(wire_sub_optional_missing));
+}
+
+static void test_sub_optional_missing(void)
+{
+    sub_optional_needed = false;
+    save_vmstate(&vmstate_sub_optional, NULL);
+
+    compare_vmstate(wire_sub_optional_missing, sizeof(wire_sub_optional_missing));
+
+    SUCCESS(load_vmstate_one(&vmstate_sub_optional, NULL,
+                             1, wire_sub_optional,
+                             sizeof(wire_sub_optional)));
+
+    SUCCESS(load_vmstate_one(&vmstate_sub_optional, NULL,
+                             1, wire_sub_optional_missing,
+                             sizeof(wire_sub_optional_missing)));
+}
+
+static uint8_t wire_sub_dup[] = {
+    QEMU_VM_SUBSECTION,
+    14,
+    's', 'u', 'b', '/', 'o', 'p', 't', 'i', 'o', 'n', 'a', 'l', '/', 'a',
+    0x0, 0x0, 0x0, 1,
+    QEMU_VM_SUBSECTION,
+    14,
+    's', 'u', 'b', '/', 'o', 'p', 't', 'i', 'o', 'n', 'a', 'l', '/', 'a',
+    0x0, 0x0, 0x0, 1,
+    QEMU_VM_EOF,
+};
+
+static void test_sub_optional_dup(void)
+{
+    sub_optional_needed = false;
+
+    FAILURE(load_vmstate_one(&vmstate_sub_optional, NULL,
+                             1, wire_sub_dup,
+                             sizeof(wire_sub_dup)));
+}
+
+static uint8_t wire_sub_unknown[] = {
+    QEMU_VM_SUBSECTION,
+    14,
+    's', 'u', 'b', '/', 'o', 'p', 't', 'i', 'o', 'n', 'a', 'l', '/', 'b',
+    0x0, 0x0, 0x0, 1,
+    QEMU_VM_EOF,
+};
+
+static void test_sub_optional_unknown(void)
+{
+    sub_optional_needed = false;
+
+    FAILURE(load_vmstate_one(&vmstate_sub_optional, NULL,
+                             1, wire_sub_unknown,
+                             sizeof(wire_sub_unknown)));
+}
+
 int main(int argc, char **argv)
 {
     g_autofree char *temp_file = g_strdup_printf("%s/vmst.test.XXXXXX",
@@ -1519,6 +1631,10 @@ int main(int argc, char **argv)
     g_test_add_func("/vmstate/qlist/save/saveqlist", test_save_qlist);
     g_test_add_func("/vmstate/qlist/load/loadqlist", test_load_qlist);
     g_test_add_func("/vmstate/tmp_struct", test_tmp_struct);
+    g_test_add_func("/vmstate/subsection/needed", test_sub_optional_needed);
+    g_test_add_func("/vmstate/subsection/missing", test_sub_optional_missing);
+    g_test_add_func("/vmstate/subsection/dup", test_sub_optional_dup);
+    g_test_add_func("/vmstate/subsection/unknown", test_sub_optional_unknown);
     g_test_run();
 
     close(temp_fd);
-- 
2.41.0

[PATCH v3 6/6] docs/migration: reflect the changes about needed subsections

[marcandre.lureau]

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

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
 docs/devel/migration.rst | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/docs/devel/migration.rst b/docs/devel/migration.rst
index 240eb16d90..22875ac40c 100644
--- a/docs/devel/migration.rst
+++ b/docs/devel/migration.rst
@@ -246,17 +246,16 @@ a newer form of device, or adding that state that you previously
 forgot to migrate.  This is best solved using a subsection.
 
 A subsection is "like" a device vmstate, but with a particularity, it
-has a Boolean function that tells if that values are needed to be sent
-or not.  If this functions returns false, the subsection is not sent.
-Subsections have a unique name, that is looked for on the receiving
-side.
+has a Boolean function that tells if that values are needed or not. If
+this functions returns false, the subsection is not sent. Subsections
+have a unique name, that is looked for on the receiving side.
 
 On the receiving side, if we found a subsection for a device that we
-don't understand, we just fail the migration.  If we understand all
-the subsections, then we load the state with success.  There's no check
-that a subsection is loaded, so a newer QEMU that knows about a subsection
-can (with care) load a stream from an older QEMU that didn't send
-the subsection.
+don't understand, we just fail the migration. If we understand all the
+subsections, then we load the state with success. There's no check
+that an optional subsection is loaded, so a newer QEMU that knows
+about a subsection can (with care) load a stream from an older QEMU
+that didn't send the subsection.
 
 If the new data is only needed in a rare case, then the subsection
 can be made conditional on that case and the migration will still
-- 
2.41.0

Re: [PATCH v3 0/6] migration: check required entries and sections are loaded

[Michael S. Tsirkin]

On Mon, Nov 06, 2023 at 03:35:54PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Hi,
> 
> Surprisingly, the migration code doesn't check that required migration entries
> and subsections are loaded. Either optional or required sections are both
> ignored when missing. According to the documentation a "newer QEMU that knows
> about a subsection can (with care) load a stream from an older QEMU that didn't
> send the subsection". I propose this behaviour to be limited to "optional"
> sections only.
> 
> This series has a few preliminary fixes, add new checks that entries are
> loaded once and required ones have been loaded, add some tests and
> documentation update.
> 
> thanks

I think this kind of thing is better deferred to the next release -
unless you have something specific in mind this fixes?

> v3:
>  - rebased, drop RFC status
>  - switch from tracepoint + returning an error to report for missing
>    subsections, as we worry about potential regressions
>  - add r-b tags
> 
> v2:
>  - add "migration: rename vmstate_save_needed->vmstate_section_needed"
>  - add "migration: set file error on subsection loading"
>  - add subsection tests
>  - update the documentation
> 
> Marc-André Lureau (6):
>   block/fdc: 'phase' is not needed on load
>   virtio: make endian_needed() work during loading
>   migration: check required subsections are loaded, once
>   migration: check required entries are loaded, once
>   test-vmstate: add some subsection tests
>   docs/migration: reflect the changes about needed subsections
> 
>  docs/devel/migration.rst  |  17 +++---
>  hw/block/fdc.c            |   5 ++
>  hw/virtio/virtio.c        |   6 +-
>  migration/savevm.c        |  43 ++++++++++++++
>  migration/vmstate.c       |  40 ++++++++++++-
>  tests/unit/test-vmstate.c | 116 ++++++++++++++++++++++++++++++++++++++
>  6 files changed, 215 insertions(+), 12 deletions(-)
> 
> -- 
> 2.41.0

Re: [PATCH v3 0/6] migration: check required entries and sections are loaded

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, Nov 06, 2023 at 03:35:54PM +0400, marcandre.lureau@redhat.com wrote:
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> 
>> Hi,
>> 
>> Surprisingly, the migration code doesn't check that required migration entries
>> and subsections are loaded. Either optional or required sections are both
>> ignored when missing. According to the documentation a "newer QEMU that knows
>> about a subsection can (with care) load a stream from an older QEMU that didn't
>> send the subsection". I propose this behaviour to be limited to "optional"
>> sections only.
>> 
>> This series has a few preliminary fixes, add new checks that entries are
>> loaded once and required ones have been loaded, add some tests and
>> documentation update.
>> 
>> thanks
>
> I think this kind of thing is better deferred to the next release -
> unless you have something specific in mind this fixes?

I agree with you.

Later, Juan.

>> v3:
>>  - rebased, drop RFC status
>>  - switch from tracepoint + returning an error to report for missing
>>    subsections, as we worry about potential regressions
>>  - add r-b tags
>> 
>> v2:
>>  - add "migration: rename vmstate_save_needed->vmstate_section_needed"
>>  - add "migration: set file error on subsection loading"
>>  - add subsection tests
>>  - update the documentation
>> 
>> Marc-André Lureau (6):
>>   block/fdc: 'phase' is not needed on load
>>   virtio: make endian_needed() work during loading
>>   migration: check required subsections are loaded, once
>>   migration: check required entries are loaded, once
>>   test-vmstate: add some subsection tests
>>   docs/migration: reflect the changes about needed subsections
>> 
>>  docs/devel/migration.rst  |  17 +++---
>>  hw/block/fdc.c            |   5 ++
>>  hw/virtio/virtio.c        |   6 +-
>>  migration/savevm.c        |  43 ++++++++++++++
>>  migration/vmstate.c       |  40 ++++++++++++-
>>  tests/unit/test-vmstate.c | 116 ++++++++++++++++++++++++++++++++++++++
>>  6 files changed, 215 insertions(+), 12 deletions(-)
>> 
>> -- 
>> 2.41.0

Re: [PATCH v3 0/6] migration: check required entries and sections are loaded

[Michael S. Tsirkin]

On Mon, Nov 06, 2023 at 03:35:54PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Hi,
> 
> Surprisingly, the migration code doesn't check that required migration entries
> and subsections are loaded. Either optional or required sections are both
> ignored when missing. According to the documentation a "newer QEMU that knows
> about a subsection can (with care) load a stream from an older QEMU that didn't
> send the subsection". I propose this behaviour to be limited to "optional"
> sections only.
> 
> This series has a few preliminary fixes, add new checks that entries are
> loaded once and required ones have been loaded, add some tests and
> documentation update.
> 
> thanks


series:

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

merge through migration tree.

> v3:
>  - rebased, drop RFC status
>  - switch from tracepoint + returning an error to report for missing
>    subsections, as we worry about potential regressions
>  - add r-b tags
> 
> v2:
>  - add "migration: rename vmstate_save_needed->vmstate_section_needed"
>  - add "migration: set file error on subsection loading"
>  - add subsection tests
>  - update the documentation
> 
> Marc-André Lureau (6):
>   block/fdc: 'phase' is not needed on load
>   virtio: make endian_needed() work during loading
>   migration: check required subsections are loaded, once
>   migration: check required entries are loaded, once
>   test-vmstate: add some subsection tests
>   docs/migration: reflect the changes about needed subsections
> 
>  docs/devel/migration.rst  |  17 +++---
>  hw/block/fdc.c            |   5 ++
>  hw/virtio/virtio.c        |   6 +-
>  migration/savevm.c        |  43 ++++++++++++++
>  migration/vmstate.c       |  40 ++++++++++++-
>  tests/unit/test-vmstate.c | 116 ++++++++++++++++++++++++++++++++++++++
>  6 files changed, 215 insertions(+), 12 deletions(-)
> 
> -- 
> 2.41.0