[PATCH 0/5] RFC: migration: check required entries and sections are loaded

[PATCH 0/5] RFC: migration: check required entries and sections are loaded

[marcandre.lureau]

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

Hi,

To my surprise, QEMU didn't seem to check that required migration entries and
subsections are loaded. I took a naive approach to add such check, by adding
some "visited" marks along the loading path, and checking by the end that all
required entries have been loaded.

Note, I haven't thoroughly tested it yet (it passed make check on x86 targets
with the preliminary "fixes" applied).

Comments welcome!

Marc-André Lureau (5):
  block/fdc: 'phase' is not needed on load
  virtio: make endian_needed() work during loading
  net/slirp: use different IDs for each instance
  RFC: migration: check required subsections are loaded, once
  RFC: migration: check required entries are loaded, once

 hw/block/fdc.c      |  5 +++++
 hw/virtio/virtio.c  |  6 +++++-
 migration/savevm.c  | 43 +++++++++++++++++++++++++++++++++++++++++++
 migration/vmstate.c | 40 ++++++++++++++++++++++++++++++++++++++--
 net/slirp.c         |  3 ++-
 5 files changed, 93 insertions(+), 4 deletions(-)

-- 
2.41.0

[PATCH 1/5] 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 2/5] 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 4577f3f5b3..7e77e66e99 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2487,7 +2487,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 */
+    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 3/5] net/slirp: use different IDs for each instance

[marcandre.lureau]

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

Using always 0, QEMU will end up loading the same instance, even if
multiple have been saved.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 net/slirp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/slirp.c b/net/slirp.c
index c33b3e02e7..af1451b60f 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -47,6 +47,7 @@
 #include "util.h"
 #include "migration/register.h"
 #include "migration/qemu-file-types.h"
+#include "migration/vmstate.h"
 
 static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
 {
@@ -659,7 +660,7 @@ static int net_slirp_init(NetClientState *peer, const char *model,
      * specific version?
      */
     g_assert(slirp_state_version() == 4);
-    register_savevm_live("slirp", 0, slirp_state_version(),
+    register_savevm_live("slirp", VMSTATE_INSTANCE_ID_ANY, slirp_state_version(),
                          &savevm_slirp_state, s->slirp);
 
     s->poll_notifier.notify = net_slirp_poll_notify;
-- 
2.41.0

[PATCH 4/5] RFC: 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>
---
 migration/vmstate.c | 40 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index 31842c3afb..5147a8191d 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -426,22 +426,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;
@@ -467,7 +496,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;
@@ -484,6 +513,13 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
         }
     }
 
+    for (i = 0; i < n; i++) {
+        if (!visited[i] && vmstate_save_needed(vmsd->subsections[i], opaque)) {
+            trace_vmstate_subsection_load_bad(vmsd->name, vmsd->subsections[i]->name, "(not visited)");
+            return -ENOENT;
+        }
+    }
+
     trace_vmstate_subsection_load_good(vmsd->name);
     return 0;
 }
-- 
2.41.0

[PATCH 5/5] RFC: 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>
---
 migration/savevm.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/migration/savevm.c b/migration/savevm.c
index bb3e99194c..6a17bb0f73 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 {
@@ -1718,6 +1719,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_save_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;
@@ -2520,6 +2551,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) {
@@ -2546,6 +2582,8 @@ qemu_loadvm_section_start_full(QEMUFile *f, MigrationIncomingState *mis)
         return -EINVAL;
     }
 
+    se->visited = true;
+
     return 0;
 }
 
@@ -2852,7 +2890,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

Re: [PATCH 4/5] RFC: migration: check required subsections are loaded, once

[Peter Xu]

On Tue, Sep 26, 2023 at 07:59:24PM +0400, marcandre.lureau@redhat.com wrote:
> @@ -484,6 +513,13 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
>          }
>      }
>  
> +    for (i = 0; i < n; i++) {
> +        if (!visited[i] && vmstate_save_needed(vmsd->subsections[i], opaque)) {
> +            trace_vmstate_subsection_load_bad(vmsd->name, vmsd->subsections[i]->name, "(not visited)");
> +            return -ENOENT;
> +        }
> +    }

One thing that might be tricky to call needed() on loading side is, IMHO
the needed() hooks normally was designed to be only called on a complete VM
state. IOW, I think it can reference any machine/device state, or whatever
variable assuming all of them contain valid data.

But the load side may not yet contain everything..  we can guarantee here
we loaded the full device state of this one as subsections should be the
last to come, and all we've loaded so far.  But what if it references
something else outside what we've loaded?  It looks possible in some
special .needed() hook we can return something unexpected.

I assume most needed() hooks are fine (and it does look like we can find
bugs with this, which means this might be proved useful already at least in
some form or another). I just worry on something start to break after we
become strict on this.

Maybe.. make the check only throw warnings, but not yet fail the migration?

> +
>      trace_vmstate_subsection_load_good(vmsd->name);
>      return 0;
>  }
> -- 
> 2.41.0
> 

-- 
Peter Xu

Re: [PATCH 0/5] RFC: migration: check required entries and sections are loaded

[Peter Xu]

On Tue, Sep 26, 2023 at 07:59:20PM +0400, marcandre.lureau@redhat.com wrote:
> Marc-André Lureau (5):
>   block/fdc: 'phase' is not needed on load
>   virtio: make endian_needed() work during loading
>   net/slirp: use different IDs for each instance

First 3 patches are bug fixes, am I right?

It'll be great if they can be acked (or even picked up earlier?) by
subsystem maintainers if so, and copy stable if justified proper.

Thanks,

-- 
Peter Xu