[Qemu-devel] [PATCH 0/4] Add section footers to detect corrupted migration streams

[Qemu-devel] [PATCH 0/4] Add section footers to detect corrupted migration streams

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Badly formatted migration streams can go undetected or produce
misleading errors due to a lock of checking at the end of sections.
In particular a section that adds an extra 0x00 at the end
causes what looks like a normal end of stream and thus doesn't produce
any errors, and something that ends in a 0x01..0x04 kind of look
like real section headers and then fail when the section parser tries
to figure out which section they are.  This is made worse by the
choice of 0x00..0x04 being small numbers that are particularly common
in normal section data.

This patch series adds a section footer consisting of a marker (0x7e - ~)
followed by the section-id that was also sent in the header.  If
they mismatch then it throws an error explaining which section was
being loaded.

The footers are tied to new machine types (on both pc types).

Dr. David Alan Gilbert (4):
  Merge section header writing
  Disable section footers on older machine types
  Add a protective section footer
  Teach analyze-migration.py about section footers

 hw/i386/pc_piix.c             |   2 +
 hw/i386/pc_q35.c              |   2 +
 include/migration/migration.h |   2 +
 savevm.c                      | 141 ++++++++++++++++++++++++++++--------------
 scripts/analyze-migration.py  |   5 ++
 5 files changed, 107 insertions(+), 45 deletions(-)

-- 
2.4.1

[Qemu-devel] [PATCH 1/4] Merge section header writing

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

The header writing for device sections is open coded in
a few places, merge it into one.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 savevm.c | 73 +++++++++++++++++++++++++---------------------------------------
 1 file changed, 28 insertions(+), 45 deletions(-)

diff --git a/savevm.c b/savevm.c
index 3b0e222..edb8f33 100644
--- a/savevm.c
+++ b/savevm.c
@@ -602,6 +602,27 @@ static void vmstate_save(QEMUFile *f, SaveStateEntry *se, QJSON *vmdesc)
     vmstate_save_state(f, se->vmsd, se->opaque, vmdesc);
 }
 
+/*
+ * Write the header for device section (QEMU_VM_SECTION START/END/PART/FULL)
+ */
+static void save_section_header(QEMUFile *f, SaveStateEntry *se,
+                                uint8_t section_type)
+{
+    qemu_put_byte(f, section_type);
+    qemu_put_be32(f, se->section_id);
+
+    if (section_type == QEMU_VM_SECTION_FULL ||
+        section_type == QEMU_VM_SECTION_START) {
+        /* ID string */
+        size_t len = strlen(se->idstr);
+        qemu_put_byte(f, len);
+        qemu_put_buffer(f, (uint8_t *)se->idstr, len);
+
+        qemu_put_be32(f, se->instance_id);
+        qemu_put_be32(f, se->version_id);
+    }
+}
+
 bool qemu_savevm_state_blocked(Error **errp)
 {
     SaveStateEntry *se;
@@ -634,8 +655,6 @@ void qemu_savevm_state_begin(QEMUFile *f,
     qemu_put_be32(f, QEMU_VM_FILE_VERSION);
 
     QTAILQ_FOREACH(se, &savevm_handlers, entry) {
-        int len;
-
         if (!se->ops || !se->ops->save_live_setup) {
             continue;
         }
@@ -644,17 +663,7 @@ void qemu_savevm_state_begin(QEMUFile *f,
                 continue;
             }
         }
-        /* Section type */
-        qemu_put_byte(f, QEMU_VM_SECTION_START);
-        qemu_put_be32(f, se->section_id);
-
-        /* ID string */
-        len = strlen(se->idstr);
-        qemu_put_byte(f, len);
-        qemu_put_buffer(f, (uint8_t *)se->idstr, len);
-
-        qemu_put_be32(f, se->instance_id);
-        qemu_put_be32(f, se->version_id);
+        save_section_header(f, se, QEMU_VM_SECTION_START);
 
         ret = se->ops->save_live_setup(f, se->opaque);
         if (ret < 0) {
@@ -689,9 +698,8 @@ int qemu_savevm_state_iterate(QEMUFile *f)
             return 0;
         }
         trace_savevm_section_start(se->idstr, se->section_id);
-        /* Section type */
-        qemu_put_byte(f, QEMU_VM_SECTION_PART);
-        qemu_put_be32(f, se->section_id);
+
+        save_section_header(f, se, QEMU_VM_SECTION_PART);
 
         ret = se->ops->save_live_iterate(f, se->opaque);
         trace_savevm_section_end(se->idstr, se->section_id, ret);
@@ -737,9 +745,8 @@ void qemu_savevm_state_complete(QEMUFile *f)
             }
         }
         trace_savevm_section_start(se->idstr, se->section_id);
-        /* Section type */
-        qemu_put_byte(f, QEMU_VM_SECTION_END);
-        qemu_put_be32(f, se->section_id);
+
+        save_section_header(f, se, QEMU_VM_SECTION_END);
 
         ret = se->ops->save_live_complete(f, se->opaque);
         trace_savevm_section_end(se->idstr, se->section_id, ret);
@@ -753,8 +760,6 @@ void qemu_savevm_state_complete(QEMUFile *f)
     json_prop_int(vmdesc, "page_size", TARGET_PAGE_SIZE);
     json_start_array(vmdesc, "devices");
     QTAILQ_FOREACH(se, &savevm_handlers, entry) {
-        int len;
-
         if ((!se->ops || !se->ops->save_state) && !se->vmsd) {
             continue;
         }
@@ -764,17 +769,7 @@ void qemu_savevm_state_complete(QEMUFile *f)
         json_prop_str(vmdesc, "name", se->idstr);
         json_prop_int(vmdesc, "instance_id", se->instance_id);
 
-        /* Section type */
-        qemu_put_byte(f, QEMU_VM_SECTION_FULL);
-        qemu_put_be32(f, se->section_id);
-
-        /* ID string */
-        len = strlen(se->idstr);
-        qemu_put_byte(f, len);
-        qemu_put_buffer(f, (uint8_t *)se->idstr, len);
-
-        qemu_put_be32(f, se->instance_id);
-        qemu_put_be32(f, se->version_id);
+        save_section_header(f, se, QEMU_VM_SECTION_FULL);
 
         vmstate_save(f, se, vmdesc);
 
@@ -873,8 +868,6 @@ static int qemu_save_device_state(QEMUFile *f)
     cpu_synchronize_all_states();
 
     QTAILQ_FOREACH(se, &savevm_handlers, entry) {
-        int len;
-
         if (se->is_ram) {
             continue;
         }
@@ -882,17 +875,7 @@ static int qemu_save_device_state(QEMUFile *f)
             continue;
         }
 
-        /* Section type */
-        qemu_put_byte(f, QEMU_VM_SECTION_FULL);
-        qemu_put_be32(f, se->section_id);
-
-        /* ID string */
-        len = strlen(se->idstr);
-        qemu_put_byte(f, len);
-        qemu_put_buffer(f, (uint8_t *)se->idstr, len);
-
-        qemu_put_be32(f, se->instance_id);
-        qemu_put_be32(f, se->version_id);
+        save_section_header(f, se, QEMU_VM_SECTION_FULL);
 
         vmstate_save(f, se, NULL);
     }
-- 
2.4.1

[Qemu-devel] [PATCH 2/4] Disable section footers on older machine types

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

The next patch adds section footers; but we don't want to
break migration compatibility so disable them on older
machine types

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/i386/pc_piix.c             | 2 ++
 hw/i386/pc_q35.c              | 2 ++
 include/migration/migration.h | 1 +
 savevm.c                      | 7 +++++++
 4 files changed, 12 insertions(+)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 212e263..dd4c826 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -52,6 +52,7 @@
 #ifdef CONFIG_XEN
 #  include <xen/hvm/hvm_info_table.h>
 #endif
+#include "migration/migration.h"
 
 #define MAX_IDE_BUS 2
 
@@ -312,6 +313,7 @@ static void pc_init_pci(MachineState *machine)
 
 static void pc_compat_2_3(MachineState *machine)
 {
+    savevm_skip_section_footers();
 }
 
 static void pc_compat_2_2(MachineState *machine)
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index e67f2de..3fea169 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -45,6 +45,7 @@
 #include "hw/usb.h"
 #include "hw/cpu/icc_bus.h"
 #include "qemu/error-report.h"
+#include "migration/migration.h"
 
 /* ICH9 AHCI has 6 ports */
 #define MAX_SATA_PORTS     6
@@ -291,6 +292,7 @@ static void pc_q35_init(MachineState *machine)
 
 static void pc_compat_2_3(MachineState *machine)
 {
+    savevm_skip_section_footers();
 }
 
 static void pc_compat_2_2(MachineState *machine)
diff --git a/include/migration/migration.h b/include/migration/migration.h
index a6e025a..9afee02 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -180,4 +180,5 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
                              ram_addr_t offset, size_t size,
                              uint64_t *bytes_sent);
 
+void savevm_skip_section_footers(void);
 #endif
diff --git a/savevm.c b/savevm.c
index edb8f33..415914f 100644
--- a/savevm.c
+++ b/savevm.c
@@ -51,6 +51,8 @@
 #define ARP_PTYPE_IP 0x0800
 #define ARP_OP_REQUEST_REV 0x3
 
+static bool skip_section_footers;
+
 static int announce_self_create(uint8_t *buf,
                                 uint8_t *mac_addr)
 {
@@ -602,6 +604,11 @@ static void vmstate_save(QEMUFile *f, SaveStateEntry *se, QJSON *vmdesc)
     vmstate_save_state(f, se->vmsd, se->opaque, vmdesc);
 }
 
+void savevm_skip_section_footers(void)
+{
+    skip_section_footers = true;
+}
+
 /*
  * Write the header for device section (QEMU_VM_SECTION START/END/PART/FULL)
  */
-- 
2.4.1

[Qemu-devel] [PATCH 3/4] Add a protective section footer

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Badly formatted migration streams can go undetected or produce
misleading errors due to a lock of checking at the end of sections.
In particular a section that adds an extra 0x00 at the end
causes what looks like a normal end of stream and thus doesn't produce
any errors, and something that ends in a 0x01..0x04 kind of look
like real section headers and then fail when the section parser tries
to figure out which section they are.  This is made worse by the
choice of 0x00..0x04 being small numbers that are particularly common
in normal section data.

This patch adds a section footer consisting of a marker (0x7e - ~)
followed by the section-id that was also sent in the header.  If
they mismatch then it throws an error explaining which section was
being loaded.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 include/migration/migration.h |  1 +
 savevm.c                      | 61 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index 9afee02..8c4edf4 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -34,6 +34,7 @@
 #define QEMU_VM_SECTION_FULL         0x04
 #define QEMU_VM_SUBSECTION           0x05
 #define QEMU_VM_VMDESCRIPTION        0x06
+#define QEMU_VM_SECTION_FOOTER       0x7e
 
 struct MigrationParams {
     bool blk;
diff --git a/savevm.c b/savevm.c
index 415914f..792c0fd 100644
--- a/savevm.c
+++ b/savevm.c
@@ -630,6 +630,53 @@ static void save_section_header(QEMUFile *f, SaveStateEntry *se,
     }
 }
 
+/*
+ * Write a footer onto device sections that catches cases misformatted device
+ * sections.
+ */
+static void save_section_footer(QEMUFile *f, SaveStateEntry *se)
+{
+    if (!skip_section_footers) {
+        qemu_put_byte(f, QEMU_VM_SECTION_FOOTER);
+        qemu_put_be32(f, se->section_id);
+    }
+}
+
+/*
+ * Read a footer off the wire and check that it matches the expected section
+ *
+ * Returns: true if the footer was good
+ *          false if there is a problem (and calls error_report to say why)
+ */
+static bool check_section_footer(QEMUFile *f, SaveStateEntry *se)
+{
+    uint8_t read_mark;
+    uint32_t read_section_id;
+
+    if (skip_section_footers) {
+        /* No footer to check */
+        return true;
+    }
+
+    read_mark = qemu_get_byte(f);
+
+    if (read_mark != QEMU_VM_SECTION_FOOTER) {
+        error_report("Missing section footer for %s", se->idstr);
+        return false;
+    }
+
+    read_section_id = qemu_get_be32(f);
+    if (read_section_id != se->section_id) {
+        error_report("Mismatched section id in footer for %s -"
+                     " read 0x%x expected 0x%x",
+                     se->idstr, read_section_id, se->section_id);
+        return false;
+    }
+
+    /* All good */
+    return true;
+}
+
 bool qemu_savevm_state_blocked(Error **errp)
 {
     SaveStateEntry *se;
@@ -673,6 +720,7 @@ void qemu_savevm_state_begin(QEMUFile *f,
         save_section_header(f, se, QEMU_VM_SECTION_START);
 
         ret = se->ops->save_live_setup(f, se->opaque);
+        save_section_footer(f, se);
         if (ret < 0) {
             qemu_file_set_error(f, ret);
             break;
@@ -710,6 +758,7 @@ int qemu_savevm_state_iterate(QEMUFile *f)
 
         ret = se->ops->save_live_iterate(f, se->opaque);
         trace_savevm_section_end(se->idstr, se->section_id, ret);
+        save_section_footer(f, se);
 
         if (ret < 0) {
             qemu_file_set_error(f, ret);
@@ -757,6 +806,7 @@ void qemu_savevm_state_complete(QEMUFile *f)
 
         ret = se->ops->save_live_complete(f, se->opaque);
         trace_savevm_section_end(se->idstr, se->section_id, ret);
+        save_section_footer(f, se);
         if (ret < 0) {
             qemu_file_set_error(f, ret);
             return;
@@ -782,6 +832,7 @@ void qemu_savevm_state_complete(QEMUFile *f)
 
         json_end_object(vmdesc);
         trace_savevm_section_end(se->idstr, se->section_id, 0);
+        save_section_footer(f, se);
     }
 
     qemu_put_byte(f, QEMU_VM_EOF);
@@ -885,6 +936,8 @@ static int qemu_save_device_state(QEMUFile *f)
         save_section_header(f, se, QEMU_VM_SECTION_FULL);
 
         vmstate_save(f, se, NULL);
+
+        save_section_footer(f, se);
     }
 
     qemu_put_byte(f, QEMU_VM_EOF);
@@ -1002,6 +1055,10 @@ int qemu_loadvm_state(QEMUFile *f)
                              " device '%s'", instance_id, idstr);
                 goto out;
             }
+            if (!check_section_footer(f, le->se)) {
+                ret = -EINVAL;
+                goto out;
+            }
             break;
         case QEMU_VM_SECTION_PART:
         case QEMU_VM_SECTION_END:
@@ -1025,6 +1082,10 @@ int qemu_loadvm_state(QEMUFile *f)
                              section_id, le->se->idstr);
                 goto out;
             }
+            if (!check_section_footer(f, le->se)) {
+                ret = -EINVAL;
+                goto out;
+            }
             break;
         default:
             error_report("Unknown savevm section type %d", section_type);
-- 
2.4.1

[Qemu-devel] [PATCH 4/4] Teach analyze-migration.py about section footers

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 scripts/analyze-migration.py | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
index 0c8b22f..f6894be 100755
--- a/scripts/analyze-migration.py
+++ b/scripts/analyze-migration.py
@@ -474,6 +474,7 @@ class MigrationDump(object):
     QEMU_VM_SECTION_FULL  = 0x04
     QEMU_VM_SUBSECTION    = 0x05
     QEMU_VM_VMDESCRIPTION = 0x06
+    QEMU_VM_SECTION_FOOTER= 0x7e
 
     def __init__(self, filename):
         self.section_classes = { ( 'ram', 0 ) : [ RamSection, None ],
@@ -526,6 +527,10 @@ class MigrationDump(object):
             elif section_type == self.QEMU_VM_SECTION_PART or section_type == self.QEMU_VM_SECTION_END:
                 section_id = file.read32()
                 self.sections[section_id].read()
+            elif section_type == self.QEMU_VM_SECTION_FOOTER:
+                read_section_id = file.read32()
+                if read_section_id != section_id:
+                    raise Exception("Mismatched section footer: %x vs %x" % (read_section_id, section_id))
             else:
                 raise Exception("Unknown section type: %d" % section_type)
         file.close()
-- 
2.4.1

Re: [Qemu-devel] [PATCH 0/4] Add section footers to detect corrupted migration streams

[Eric Blake]

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

On 05/19/2015 05:29 AM, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Badly formatted migration streams can go undetected or produce
> misleading errors due to a lock of checking at the end of sections.
> In particular a section that adds an extra 0x00 at the end
> causes what looks like a normal end of stream and thus doesn't produce
> any errors, and something that ends in a 0x01..0x04 kind of look
> like real section headers and then fail when the section parser tries
> to figure out which section they are.  This is made worse by the
> choice of 0x00..0x04 being small numbers that are particularly common
> in normal section data.
> 
> This patch series adds a section footer consisting of a marker (0x7e - ~)
> followed by the section-id that was also sent in the header.  If
> they mismatch then it throws an error explaining which section was
> being loaded.

Good idea.  Is it redundant with the recent addition of self-describing
json that newer machine types send?  Does it let us detect a corrupted
stream earlier in the process?  Or is the main benefit that it gives
better error messages at the point corruption is first detected?

> 
> The footers are tied to new machine types (on both pc types).

Good that you tied it to machine type, but is it enough?  When we added
the optional section for giving the json representation of the stream,
we ended up having to add a knob to turn off that section, so that
backwards migration from a new qemu to an older one did not send it.
I'm wondering if we'll need to expose a knob to turn off footers, again
for the sake of backwards migration in downstream distros.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 0/4] Add section footers to detect corrupted migration streams

[Dr. David Alan Gilbert]

* Eric Blake (eblake@redhat.com) wrote:
> On 05/19/2015 05:29 AM, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Badly formatted migration streams can go undetected or produce
> > misleading errors due to a lock of checking at the end of sections.
> > In particular a section that adds an extra 0x00 at the end
> > causes what looks like a normal end of stream and thus doesn't produce
> > any errors, and something that ends in a 0x01..0x04 kind of look
> > like real section headers and then fail when the section parser tries
> > to figure out which section they are.  This is made worse by the
> > choice of 0x00..0x04 being small numbers that are particularly common
> > in normal section data.
> > 
> > This patch series adds a section footer consisting of a marker (0x7e - ~)
> > followed by the section-id that was also sent in the header.  If
> > they mismatch then it throws an error explaining which section was
> > being loaded.
> 
> Good idea.

> Is it redundant with the recent addition of self-describing
> json that newer machine types send? 

No, that self-describing json goes at the end, and is for parsing
by an external-entity; it doesn't help detect corruption on loading.

> Does it let us detect a corrupted
> stream earlier in the process?  Or is the main benefit that it gives
> better error messages at the point corruption is first detected?

Both; there are two cases that often happen; both triggered by a section
reading too little or too much, and it gets back to the main loop and
we read the next byte:
   1) the next byte on the stream is a 0x00 - that's read as an end-of-migration
      marker, we start the VM  and you get a hung VM with no errors.

   2) the next byte is between 0x01..0x04 - and it looks like a section header,
      then we try and read the next few bytes to figure out which section;
      this could a) result in an error saying it's an unknown section or
      b) Happen to match a section ID and then get an error about a problem
      in that section.  In either case you don't get an error pointing to
      the previous section which was the actual problem.

> > 
> > The footers are tied to new machine types (on both pc types).
> 
> Good that you tied it to machine type, but is it enough?  When we added
> the optional section for giving the json representation of the stream,
> we ended up having to add a knob to turn off that section, so that
> backwards migration from a new qemu to an older one did not send it.
> I'm wondering if we'll need to expose a knob to turn off footers, again
> for the sake of backwards migration in downstream distros.

That knob is already the knob that I've created and tied to the machine
type; the downstream distros will just turn the same knob in their old
machine types.

Dave

> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 


--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 0/4] Add section footers to detect corrupted migration streams

[Eric Blake]

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

On 05/19/2015 08:06 AM, Dr. David Alan Gilbert wrote:

>> Does it let us detect a corrupted
>> stream earlier in the process?  Or is the main benefit that it gives
>> better error messages at the point corruption is first detected?
> 
> Both; there are two cases that often happen; both triggered by a section
> reading too little or too much, and it gets back to the main loop and
> we read the next byte:
>    1) the next byte on the stream is a 0x00 - that's read as an end-of-migration
>       marker, we start the VM  and you get a hung VM with no errors.
> 
>    2) the next byte is between 0x01..0x04 - and it looks like a section header,
>       then we try and read the next few bytes to figure out which section;
>       this could a) result in an error saying it's an unknown section or
>       b) Happen to match a section ID and then get an error about a problem
>       in that section.  In either case you don't get an error pointing to
>       the previous section which was the actual problem.

Probably worth incorporating into the commit body then :)

> 
>>>
>>> The footers are tied to new machine types (on both pc types).
>>
>> Good that you tied it to machine type, but is it enough?  When we added
>> the optional section for giving the json representation of the stream,
>> we ended up having to add a knob to turn off that section, so that
>> backwards migration from a new qemu to an older one did not send it.
>> I'm wondering if we'll need to expose a knob to turn off footers, again
>> for the sake of backwards migration in downstream distros.
> 
> That knob is already the knob that I've created and tied to the machine
> type; the downstream distros will just turn the same knob in their old
> machine types.

I'm not asking about the machine type defaults, but a command line
override: -machine suppress-vmdesc=on, commit 9850c604

[uggh - why'd we give that option an inverted name? Just so we could
have a default of off?  It might have been nicer as -machine
send-vmdesc=off with a default of on for new machine types]


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 0/4] Add section footers to detect corrupted migration streams

[Dr. David Alan Gilbert]

* Eric Blake (eblake@redhat.com) wrote:
> On 05/19/2015 08:06 AM, Dr. David Alan Gilbert wrote:
> 
> >> Does it let us detect a corrupted
> >> stream earlier in the process?  Or is the main benefit that it gives
> >> better error messages at the point corruption is first detected?
> > 
> > Both; there are two cases that often happen; both triggered by a section
> > reading too little or too much, and it gets back to the main loop and
> > we read the next byte:
> >    1) the next byte on the stream is a 0x00 - that's read as an end-of-migration
> >       marker, we start the VM  and you get a hung VM with no errors.
> > 
> >    2) the next byte is between 0x01..0x04 - and it looks like a section header,
> >       then we try and read the next few bytes to figure out which section;
> >       this could a) result in an error saying it's an unknown section or
> >       b) Happen to match a section ID and then get an error about a problem
> >       in that section.  In either case you don't get an error pointing to
> >       the previous section which was the actual problem.
> 
> Probably worth incorporating into the commit body then :)

Well the original text does say:
  Badly formatted migration streams can go undetected or produce
  misleading errors due to a lock of checking at the end of sections.
  In particular a section that adds an extra 0x00 at the end
  causes what looks like a normal end of stream and thus doesn't produce
  any errors, and something that ends in a 0x01..0x04 kind of look
  like real section headers and then fail when the section parser tries
  to figure out which section they are.  This is made worse by the
  choice of 0x00..0x04 being small numbers that are particularly common
  in normal section data.

which is pretty close to that; do you want me to flip that other explanation in?

> > 
> >>>
> >>> The footers are tied to new machine types (on both pc types).
> >>
> >> Good that you tied it to machine type, but is it enough?  When we added
> >> the optional section for giving the json representation of the stream,
> >> we ended up having to add a knob to turn off that section, so that
> >> backwards migration from a new qemu to an older one did not send it.
> >> I'm wondering if we'll need to expose a knob to turn off footers, again
> >> for the sake of backwards migration in downstream distros.
> > 
> > That knob is already the knob that I've created and tied to the machine
> > type; the downstream distros will just turn the same knob in their old
> > machine types.
> 
> I'm not asking about the machine type defaults, but a command line
> override: -machine suppress-vmdesc=on, commit 9850c604

That shouldn't be necessary; VMdesc was 'odd' in that it wrote data after
the end marker which broke some implicit rules about the behaviour
of the streams and the way they interacted with the file buffers.
I'd have sympathy for the opposite direction - i.e. turning on the
footer protection for older machine types when you know you've got
modern qemu's; but it doesn't seem worth the extra boiler plate unless
someone wants to do it (especially since it looks from that like it takes
two functions and ~20 lines of code to add one boolean flag to the machine
type!).

> [uggh - why'd we give that option an inverted name? Just so we could
> have a default of off?  It might have been nicer as -machine
> send-vmdesc=off with a default of on for new machine types]

We seem to do that a lot - and indeed my code does that to keep
it consistent with the other uses.  I think we like to assume 0 initialised
flags do the 'normal' thing;  still nothing like a double or triple negative
to keep you on your guard.

Dave

> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 


--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 0/4] Add section footers to detect corrupted migration streams

[Eric Blake]

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

On 05/19/2015 08:28 AM, Dr. David Alan Gilbert wrote:
> * Eric Blake (eblake@redhat.com) wrote:
>> On 05/19/2015 08:06 AM, Dr. David Alan Gilbert wrote:
>>
>>>> Does it let us detect a corrupted
>>>> stream earlier in the process?  Or is the main benefit that it gives
>>>> better error messages at the point corruption is first detected?
>>>
>>> Both; there are two cases that often happen; both triggered by a section
>>> reading too little or too much, and it gets back to the main loop and
>>> we read the next byte:
>>>    1) the next byte on the stream is a 0x00 - that's read as an end-of-migration
>>>       marker, we start the VM  and you get a hung VM with no errors.
>>>
>>>    2) the next byte is between 0x01..0x04 - and it looks like a section header,
>>>       then we try and read the next few bytes to figure out which section;
>>>       this could a) result in an error saying it's an unknown section or
>>>       b) Happen to match a section ID and then get an error about a problem
>>>       in that section.  In either case you don't get an error pointing to
>>>       the previous section which was the actual problem.
>>
>> Probably worth incorporating into the commit body then :)
> 
> Well the original text does say:
>   Badly formatted migration streams can go undetected or produce
>   misleading errors due to a lock of checking at the end of sections.
>   In particular a section that adds an extra 0x00 at the end
>   causes what looks like a normal end of stream and thus doesn't produce
>   any errors, and something that ends in a 0x01..0x04 kind of look
>   like real section headers and then fail when the section parser tries
>   to figure out which section they are.  This is made worse by the
>   choice of 0x00..0x04 being small numbers that are particularly common
>   in normal section data.
> 
> which is pretty close to that; do you want me to flip that other explanation in?

Up to you, but I found the bullet points a bit more descriptive (for
example, bullet 1 mentions a hung VM, while the original text says "no
errors" but doesn't mention "hung").

>> I'm not asking about the machine type defaults, but a command line
>> override: -machine suppress-vmdesc=on, commit 9850c604
> 
> That shouldn't be necessary; VMdesc was 'odd' in that it wrote data after
> the end marker which broke some implicit rules about the behaviour
> of the streams and the way they interacted with the file buffers.
> I'd have sympathy for the opposite direction - i.e. turning on the
> footer protection for older machine types when you know you've got
> modern qemu's; but it doesn't seem worth the extra boiler plate unless
> someone wants to do it (especially since it looks from that like it takes
> two functions and ~20 lines of code to add one boolean flag to the machine
> type!).

We may still add it later. And since suppress-vmdesc was added later, I
can live with the decision if you don't want to add command-line
override on the initial commit series.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 0/4] Add section footers to detect corrupted migration streams

[Amit Shah]

On (Tue) 19 May 2015 [08:13:52], Eric Blake wrote:
> On 05/19/2015 08:06 AM, Dr. David Alan Gilbert wrote:
> 
> >> Does it let us detect a corrupted
> >> stream earlier in the process?  Or is the main benefit that it gives
> >> better error messages at the point corruption is first detected?
> > 
> > Both; there are two cases that often happen; both triggered by a section
> > reading too little or too much, and it gets back to the main loop and
> > we read the next byte:
> >    1) the next byte on the stream is a 0x00 - that's read as an end-of-migration
> >       marker, we start the VM  and you get a hung VM with no errors.
> > 
> >    2) the next byte is between 0x01..0x04 - and it looks like a section header,
> >       then we try and read the next few bytes to figure out which section;
> >       this could a) result in an error saying it's an unknown section or
> >       b) Happen to match a section ID and then get an error about a problem
> >       in that section.  In either case you don't get an error pointing to
> >       the previous section which was the actual problem.
> 
> Probably worth incorporating into the commit body then :)

How about docs/migration.txt?  The 00/NN message gets lost..



		Amit

Re: [Qemu-devel] [PATCH 0/4] Add section footers to detect corrupted migration streams

[Amit Shah]

On (Tue) 19 May 2015 [12:29:49], Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Badly formatted migration streams can go undetected or produce
> misleading errors due to a lock of checking at the end of sections.

s/lock/lack

in patch 3 commit msg as well.

Otherwise, looks good.

Reviewed-by: Amit Shah <amit.shah@redhat.com>

		Amit

Re: [Qemu-devel] [PATCH 1/4] Merge section header writing

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> The header writing for device sections is open coded in
> a few places, merge it into one.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [Qemu-devel] [PATCH 2/4] Disable section footers on older machine types

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> The next patch adds section footers; but we don't want to
> break migration compatibility so disable them on older
> machine types
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

But ....

Power people
ARM people

Do you care now about cross version migration?

Later, Juan.

Re: [Qemu-devel] [PATCH 3/4] Add a protective section footer

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Badly formatted migration streams can go undetected or produce
> misleading errors due to a lock of checking at the end of sections.
> In particular a section that adds an extra 0x00 at the end
> causes what looks like a normal end of stream and thus doesn't produce
> any errors, and something that ends in a 0x01..0x04 kind of look
> like real section headers and then fail when the section parser tries
> to figure out which section they are.  This is made worse by the
> choice of 0x00..0x04 being small numbers that are particularly common
> in normal section data.
>
> This patch adds a section footer consisting of a marker (0x7e - ~)
> followed by the section-id that was also sent in the header.  If
> they mismatch then it throws an error explaining which section was
> being loaded.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [Qemu-devel] [PATCH 0/4] Add section footers to detect corrupted migration streams

[Juan Quintela]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Eric Blake (eblake@redhat.com) wrote:
>> On 05/19/2015 08:06 AM, Dr. David Alan Gilbert wrote:
>> 
>> >> Does it let us detect a corrupted
>> >> stream earlier in the process?  Or is the main benefit that it gives
>> >> better error messages at the point corruption is first detected?
>> > 
>> > Both; there are two cases that often happen; both triggered by a section
>> > reading too little or too much, and it gets back to the main loop and
>> > we read the next byte:
>> >    1) the next byte on the stream is a 0x00 - that's read as an end-of-migration
>> >       marker, we start the VM  and you get a hung VM with no errors.
>> > 
>> >    2) the next byte is between 0x01..0x04 - and it looks like a section header,
>> >       then we try and read the next few bytes to figure out which section;
>> >       this could a) result in an error saying it's an unknown section or
>> >       b) Happen to match a section ID and then get an error about a problem
>> >       in that section.  In either case you don't get an error pointing to
>> >       the previous section which was the actual problem.
>> 
>> Probably worth incorporating into the commit body then :)
>
> Well the original text does say:
>   Badly formatted migration streams can go undetected or produce
>   misleading errors due to a lock of checking at the end of sections.
>   In particular a section that adds an extra 0x00 at the end
>   causes what looks like a normal end of stream and thus doesn't produce
>   any errors, and something that ends in a 0x01..0x04 kind of look
>   like real section headers and then fail when the section parser tries
>   to figure out which section they are.  This is made worse by the
>   choice of 0x00..0x04 being small numbers that are particularly common
>   in normal section data.
>
> which is pretty close to that; do you want me to flip that other explanation in?
>
>> > 
>> >>>
>> >>> The footers are tied to new machine types (on both pc types).
>> >>
>> >> Good that you tied it to machine type, but is it enough?  When we added
>> >> the optional section for giving the json representation of the stream,
>> >> we ended up having to add a knob to turn off that section, so that
>> >> backwards migration from a new qemu to an older one did not send it.
>> >> I'm wondering if we'll need to expose a knob to turn off footers, again
>> >> for the sake of backwards migration in downstream distros.
>> > 
>> > That knob is already the knob that I've created and tied to the machine
>> > type; the downstream distros will just turn the same knob in their old
>> > machine types.
>> 
>> I'm not asking about the machine type defaults, but a command line
>> override: -machine suppress-vmdesc=on, commit 9850c604
>
> That shouldn't be necessary; VMdesc was 'odd' in that it wrote data after
> the end marker which broke some implicit rules about the behaviour
> of the streams and the way they interacted with the file buffers.
> I'd have sympathy for the opposite direction - i.e. turning on the
> footer protection for older machine types when you know you've got
> modern qemu's; but it doesn't seem worth the extra boiler plate unless
> someone wants to do it (especially since it looks from that like it takes
> two functions and ~20 lines of code to add one boolean flag to the machine
> type!).
>
>> [uggh - why'd we give that option an inverted name? Just so we could
>> have a default of off?  It might have been nicer as -machine
>> send-vmdesc=off with a default of on for new machine types]
>
> We seem to do that a lot - and indeed my code does that to keep
> it consistent with the other uses.  I think we like to assume 0 initialised
> flags do the 'normal' thing;  still nothing like a double or triple negative
> to keep you on your guard.

In my case here, is that we want the "new" functionality to be the
default, and just disable it for the old machine types.

So, for now on, section footers are sent.  Except, if we are on an old
machine type, that we don't sent them then.  So, the only obvious way
that I can think of is naming it as "skip_foo".  Notice that we have
also to accomodate the achitecture that don't care about cross-version
migration, and there, we just want the new behaviour.

If you have a nicer approach on mind, I am all for it.

THanks, Juan.


>
> Dave
>
>> -- 
>> Eric Blake   eblake redhat com    +1-919-301-3266
>> Libvirt virtualization library http://libvirt.org
>> 
>
>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/4] Disable section footers on older machine types

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > The next patch adds section footers; but we don't want to
> > break migration compatibility so disable them on older
> > machine types
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> 
> But ....
> 
> Power people

It looks like Power does:

ppc/spapr.c
1828:static void spapr_compat_2_3(Object *obj)

like the PC ones;  so I guess we need to roll this flag, and your
two flags into a common function somewhere and clal it from there?

Dave

> ARM people
> 
> Do you care now about cross version migration?
> 
> Later, Juan.
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK