[Qemu-devel] [PATCH v2 0.15 0/4] Fix subsection ambiguity in the migration format

[Qemu-devel] [PATCH v2 0.15 0/4] Fix subsection ambiguity in the migration format

[Paolo Bonzini]

With the current migration format, VMS_STRUCTs with subsections
are ambiguous.  The protocol cannot tell whether a 0x5 byte after
the VMS_STRUCT is a subsection or part of the parent data stream.
In the past QEMU assumed it was always a part of a subsection; after
commit eb60260 (savevm: fix corruption in vmstate_subsection_load(),
2011-02-03) the choice depends on whether the VMS_STRUCT has subsections
defined.

Unfortunately, this means that if a destination has no subsections
defined for the struct, it will happily read subsection data into
its own fields.  And if you are "lucky" enough to stumble on a
zero byte at the right time, it will be interpreted as QEMU_VM_EOF
and migration will be interrupted with half-loaded state.

There is no way out of this except defining an incompatible
migration protocol.  Not-so-long-term we should really try to define
one that is not a joke, but the bug is serious so we need a solution
for 0.15.  A sentinel at the end of embedded structs does remove the
ambiguity.

Of course, this can be restricted to new machine models, and this
is what the patch series does.  (And note that only patch 3 is specific
to the short-term solution, everything else is entirely generic).

I am still proposing this for 0.15.  Tested new on new, 0.14 on new
pc-0.14, new pc-0.14 on 0.14; also for v1 the same combinations on RHEL.

v1->v2:
        added qemu_current_migration_format() and
        QEMU_VM_FILE_VERSION_0_14.

Paolo Bonzini (4):
  add support for machine models to specify their migration format
  add pc-0.14 machine
  savevm: define new unambiguous migration format
  Partially revert "savevm: fix corruption in
    vmstate_subsection_load()."

 cpu-common.h  |    3 ---
 hw/boards.h   |    4 ++++
 hw/pc_piix.c  |   15 ++++++++++++++-
 qemu-common.h |    3 +++
 savevm.c      |   46 ++++++++++++++++++++++++++++++++--------------
 5 files changed, 53 insertions(+), 18 deletions(-)

-- 
1.7.6

[Qemu-devel] [PATCH v2 0.15 1/4] add support for machine models to specify their migration format

[Paolo Bonzini]

We need to provide a new migration format, and not break migration
in old machine models.  So add a migration_format field to QEMUMachine.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpu-common.h  |    3 ---
 hw/boards.h   |    1 +
 qemu-common.h |    3 +++
 savevm.c      |   16 ++++++++++++++--
 4 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/cpu-common.h b/cpu-common.h
index 44b04b3..8c61b18 100644
--- a/cpu-common.h
+++ b/cpu-common.h
@@ -26,9 +26,6 @@ enum device_endian {
     DEVICE_LITTLE_ENDIAN,
 };
 
-/* address in the RAM (different from a physical address) */
-typedef unsigned long ram_addr_t;
-
 /* memory API */
 
 typedef void CPUWriteMemoryFunc(void *opaque, target_phys_addr_t addr, uint32_t value);
diff --git a/hw/boards.h b/hw/boards.h
index 716fd7b..560dbaf 100644
--- a/hw/boards.h
+++ b/hw/boards.h
@@ -19,6 +19,7 @@ typedef struct QEMUMachine {
     QEMUMachineInitFunc *init;
     int use_scsi;
     int max_cpus;
+    unsigned migration_format;
     unsigned int no_serial:1,
         no_parallel:1,
         use_virtcon:1,
diff --git a/qemu-common.h b/qemu-common.h
index ba55719..24330be 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -91,6 +91,9 @@ static inline char *realpath(const char *path, char *resolved_path)
 }
 #endif
 
+/* address in the RAM (different from a physical address) */
+typedef unsigned long ram_addr_t;
+
 /* FIXME: Remove NEED_CPU_H.  */
 #ifndef NEED_CPU_H
 
diff --git a/savevm.c b/savevm.c
index 8139bc7..3049aa1 100644
--- a/savevm.c
+++ b/savevm.c
@@ -72,6 +72,7 @@
 #include "qemu-common.h"
 #include "hw/hw.h"
 #include "hw/qdev.h"
+#include "hw/boards.h"
 #include "net.h"
 #include "monitor.h"
 #include "sysemu.h"
@@ -1461,6 +1462,16 @@ bool qemu_savevm_state_blocked(Monitor *mon)
     return false;
 }
 
+static inline int qemu_current_migration_format(void)
+{
+    if (current_machine->migration_format) {
+        return current_machine->migration_format;
+    }
+
+    /* No format specified, default to the latest.  */
+    return QEMU_VM_FILE_VERSION;
+}
+
 int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable,
                             int shared)
 {
@@ -1474,7 +1484,7 @@ int qemu_savevm_state_begin(Monitor *mon, QEMUFile *f, int blk_enable,
     }
     
     qemu_put_be32(f, QEMU_VM_FILE_MAGIC);
-    qemu_put_be32(f, QEMU_VM_FILE_VERSION);
+    qemu_put_be32(f, qemu_current_migration_format());
 
     QTAILQ_FOREACH(se, &savevm_handlers, entry) {
         int len;
@@ -1747,8 +1757,10 @@ int qemu_loadvm_state(QEMUFile *f)
         fprintf(stderr, "SaveVM v2 format is obsolete and don't work anymore\n");
         return -ENOTSUP;
     }
-    if (v != QEMU_VM_FILE_VERSION)
+    if (v != qemu_current_migration_format()) {
+        fprintf(stderr, "Mismatching SaveVM format v%d\n", v);
         return -ENOTSUP;
+    }
 
     while ((section_type = qemu_get_byte(f)) != QEMU_VM_EOF) {
         uint32_t instance_id, version_id, section_id;
-- 
1.7.6

[Qemu-devel] [PATCH v2 0.15 2/4] add pc-0.14 machine

[Paolo Bonzini]

The new pc-0.15 machine will have a different migration format, so
define the compatibility one right now.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/pc_piix.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index c5c16b4..61f8cbb 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -258,7 +258,7 @@ static void pc_xen_hvm_init(ram_addr_t ram_size,
 #endif
 
 static QEMUMachine pc_machine = {
-    .name = "pc-0.14",
+    .name = "pc-0.15",
     .alias = "pc",
     .desc = "Standard PC",
     .init = pc_init_pci,
@@ -266,6 +266,13 @@ static QEMUMachine pc_machine = {
     .is_default = 1,
 };
 
+static QEMUMachine pc_machine_v0_14 = {
+    .name = "pc-0.14",
+    .desc = "Standard PC",
+    .init = pc_init_pci,
+    .max_cpus = 255,
+};
+
 static QEMUMachine pc_machine_v0_13 = {
     .name = "pc-0.13",
     .desc = "Standard PC",
@@ -482,6 +489,7 @@ static QEMUMachine xenfv_machine = {
 static void pc_machine_init(void)
 {
     qemu_register_machine(&pc_machine);
+    qemu_register_machine(&pc_machine_v0_14);
     qemu_register_machine(&pc_machine_v0_13);
     qemu_register_machine(&pc_machine_v0_12);
     qemu_register_machine(&pc_machine_v0_11);
-- 
1.7.6

[Qemu-devel] [PATCH v2 0.15 3/4] savevm: define new unambiguous migration format

[Paolo Bonzini]

With the current migration format, VMS_STRUCTs with subsections
are ambiguous.  The protocol cannot tell whether a 0x5 byte after
the VMS_STRUCT is a subsection or part of the parent data stream.
In the past QEMU assumed it was always a part of a subsection; after
commit eb60260 (savevm: fix corruption in vmstate_subsection_load().,
2011-02-03) the choice depends on whether the VMS_STRUCT has subsections
defined.

Unfortunately, this means that if a destination has no subsections
defined for the struct, it will happily read subsection data into
its own fields.  And if you are "lucky" enough to stumble on a
zero byte at the right time, it will be interpreted as QEMU_VM_EOF
and migration will be interrupted.

There is no way out of this except defining an incompatible
migration protocol with a sentinel at the end of embedded structs.
Of course, this is restricted to new machine models.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/boards.h  |    3 +++
 hw/pc_piix.c |    5 +++++
 savevm.c     |   26 ++++++++++++++++++--------
 3 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/hw/boards.h b/hw/boards.h
index 560dbaf..1cca3ce 100644
--- a/hw/boards.h
+++ b/hw/boards.h
@@ -5,6 +5,9 @@
 
 #include "qdev.h"
 
+#define QEMU_VM_FILE_VERSION_0_14    0x00000003
+#define QEMU_VM_FILE_VERSION         0x00000004
+
 typedef void QEMUMachineInitFunc(ram_addr_t ram_size,
                                  const char *boot_device,
                                  const char *kernel_filename,
diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 61f8cbb..38818de 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -271,6 +271,7 @@ static QEMUMachine pc_machine_v0_14 = {
     .desc = "Standard PC",
     .init = pc_init_pci,
     .max_cpus = 255,
+    .migration_format = QEMU_VM_FILE_VERSION_0_14,
 };
 
 static QEMUMachine pc_machine_v0_13 = {
@@ -278,6 +279,7 @@ static QEMUMachine pc_machine_v0_13 = {
     .desc = "Standard PC",
     .init = pc_init_pci_no_kvmclock,
     .max_cpus = 255,
+    .migration_format = QEMU_VM_FILE_VERSION_0_14,
     .compat_props = (GlobalProperty[]) {
         {
             .driver   = "virtio-9p-pci",
@@ -317,6 +319,7 @@ static QEMUMachine pc_machine_v0_12 = {
     .desc = "Standard PC",
     .init = pc_init_pci_no_kvmclock,
     .max_cpus = 255,
+    .migration_format = QEMU_VM_FILE_VERSION_0_14,
     .compat_props = (GlobalProperty[]) {
         {
             .driver   = "virtio-serial-pci",
@@ -360,6 +363,7 @@ static QEMUMachine pc_machine_v0_11 = {
     .desc = "Standard PC, qemu 0.11",
     .init = pc_init_pci_no_kvmclock,
     .max_cpus = 255,
+    .migration_format = QEMU_VM_FILE_VERSION_0_14,
     .compat_props = (GlobalProperty[]) {
         {
             .driver   = "virtio-blk-pci",
@@ -411,6 +415,7 @@ static QEMUMachine pc_machine_v0_10 = {
     .desc = "Standard PC, qemu 0.10",
     .init = pc_init_pci_no_kvmclock,
     .max_cpus = 255,
+    .migration_format = QEMU_VM_FILE_VERSION_0_14,
     .compat_props = (GlobalProperty[]) {
         {
             .driver   = "virtio-blk-pci",
diff --git a/savevm.c b/savevm.c
index 3049aa1..197af4b 100644
--- a/savevm.c
+++ b/savevm.c
@@ -158,6 +158,14 @@ void qemu_announce_self(void)
 
 #define IO_BUF_SIZE 32768
 
+#define QEMU_VM_EOF                  0x00
+#define QEMU_VM_SECTION_START        0x01
+#define QEMU_VM_SECTION_PART         0x02
+#define QEMU_VM_SECTION_END          0x03
+#define QEMU_VM_SECTION_FULL         0x04
+#define QEMU_VM_SUBSECTION           0x05
+#define QEMU_VM_SUBSECTIONS_END      0x06
+
 struct QEMUFile {
     QEMUFilePutBufferFunc *put_buffer;
     QEMUFileGetBufferFunc *get_buffer;
@@ -1348,6 +1356,12 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
                 }
                 if (field->flags & VMS_STRUCT) {
                     ret = vmstate_load_state(f, field->vmsd, addr, field->vmsd->version_id);
+                    if (!current_machine->migration_format ||
+                        current_machine->migration_format >= 4) {
+                        if (qemu_get_byte(f) != QEMU_VM_SUBSECTIONS_END) {
+                            return -EINVAL;
+                        }
+                    }
                 } else {
                     ret = field->info->get(f, addr, size);
 
@@ -1410,6 +1424,10 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
                 }
                 if (field->flags & VMS_STRUCT) {
                     vmstate_save_state(f, field->vmsd, addr);
+                    if (!current_machine->migration_format ||
+                        current_machine->migration_format >= 4) {
+                        qemu_put_byte(f, QEMU_VM_SUBSECTIONS_END);
+                    }
                 } else {
                     field->info->put(f, addr, size);
                 }
@@ -1439,14 +1457,6 @@ static void vmstate_save(QEMUFile *f, SaveStateEntry *se)
 
 #define QEMU_VM_FILE_MAGIC           0x5145564d
 #define QEMU_VM_FILE_VERSION_COMPAT  0x00000002
-#define QEMU_VM_FILE_VERSION         0x00000003
-
-#define QEMU_VM_EOF                  0x00
-#define QEMU_VM_SECTION_START        0x01
-#define QEMU_VM_SECTION_PART         0x02
-#define QEMU_VM_SECTION_END          0x03
-#define QEMU_VM_SECTION_FULL         0x04
-#define QEMU_VM_SUBSECTION           0x05
 
 bool qemu_savevm_state_blocked(Monitor *mon)
 {
-- 
1.7.6

[Qemu-devel] [PATCH v2 0.15 4/4] Partially revert "savevm: fix corruption in vmstate_subsection_load()."

[Paolo Bonzini]

This reverts the additional check in commit eb60260d (but not the
assertions).

The new format does not require the check, and with the old format
it traded one kind of bogus failure for a different kind of silent
failure.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 savevm.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/savevm.c b/savevm.c
index 197af4b..c849914 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1687,10 +1687,6 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
 {
     const VMStateSubsection *sub = vmsd->subsections;
 
-    if (!sub || !sub->needed) {
-        return 0;
-    }
-
     while (qemu_peek_byte(f) == QEMU_VM_SUBSECTION) {
         char idstr[256];
         int ret;
-- 
1.7.6

Re: [Qemu-devel] [PATCH v2 0.15 0/4] Fix subsection ambiguity in the migration format

[Anthony Liguori]

On 07/29/2011 10:33 AM, Paolo Bonzini wrote:
> With the current migration format, VMS_STRUCTs with subsections
> are ambiguous.  The protocol cannot tell whether a 0x5 byte after
> the VMS_STRUCT is a subsection or part of the parent data stream.
> In the past QEMU assumed it was always a part of a subsection; after
> commit eb60260 (savevm: fix corruption in vmstate_subsection_load(),
> 2011-02-03) the choice depends on whether the VMS_STRUCT has subsections
> defined.
>
> Unfortunately, this means that if a destination has no subsections
> defined for the struct, it will happily read subsection data into
> its own fields.  And if you are "lucky" enough to stumble on a
> zero byte at the right time, it will be interpreted as QEMU_VM_EOF
> and migration will be interrupted with half-loaded state.
>
> There is no way out of this except defining an incompatible
> migration protocol.  Not-so-long-term we should really try to define
> one that is not a joke, but the bug is serious so we need a solution
> for 0.15.  A sentinel at the end of embedded structs does remove the
> ambiguity.

I've thought about this very carefully now.  I just don't feel 
comfortable making a protocol change in an rc window for a series that 
hasn't spent any time in master.

This issue needs to be fixed for 0.15, but there's a simpler solution as 
we currently only have two uses of subsections in the tree today.  I'll 
send out a patch that bumps those two migration states to a new version 
and eliminates the subsection usage entirely.

If we can agree on that for 0.15, I'm happy to take this series into 
master but we should also consider other possibilities too for fixing 
the problem.

Regards,

Anthony Liguori

>
> Of course, this can be restricted to new machine models, and this
> is what the patch series does.  (And note that only patch 3 is specific
> to the short-term solution, everything else is entirely generic).
>
> I am still proposing this for 0.15.  Tested new on new, 0.14 on new
> pc-0.14, new pc-0.14 on 0.14; also for v1 the same combinations on RHEL.
>
> v1->v2:
>          added qemu_current_migration_format() and
>          QEMU_VM_FILE_VERSION_0_14.
>
> Paolo Bonzini (4):
>    add support for machine models to specify their migration format
>    add pc-0.14 machine
>    savevm: define new unambiguous migration format
>    Partially revert "savevm: fix corruption in
>      vmstate_subsection_load()."
>
>   cpu-common.h  |    3 ---
>   hw/boards.h   |    4 ++++
>   hw/pc_piix.c  |   15 ++++++++++++++-
>   qemu-common.h |    3 +++
>   savevm.c      |   46 ++++++++++++++++++++++++++++++++--------------
>   5 files changed, 53 insertions(+), 18 deletions(-)
>