Re: [Qemu-devel] [PATCH 6/6] migration: catch unknown flags in ram_load

[Peter Lieven]

On 16.06.2014 19:31, Eric Blake wrote:
> On 06/15/2014 09:15 PM, Juan Quintela wrote:
> > From: Peter Lieven <address@hidden>
> >
> > if a saved vm has unknown flags in the memory data qemu
> > currently simply ignores this flag and continues which
> > yields in an unpredictable result.
> >
> > This patch catches all unknown flags and aborts the
> > loading of the vm. Additionally error reports are thrown
> > if the migration aborts abnormally.
> This patch is a strict improvement, so I'm glad it went in.  However, I
> still feel that we aren't doing a good job of silently ignoring
> unexpected combinations of flag bits, and had suggestions in the
> original thread on further followups that are worth having before the
> 2.1 release.
>
> > -
> > -        if (flags & RAM_SAVE_FLAG_COMPRESS) {
> > +        } else if (flags & RAM_SAVE_FLAG_COMPRESS) {
> >               void *host;
> Among other things, switching from a chain of if-else to a switch might
> make it easier to document explicit supported combinations of flags and
> reject other values from an invalid stream.

Is this what you have in mind?

diff --git a/arch_init.c b/arch_init.c
index 8ddaf35..925cc66 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -1039,7 +1039,7 @@ void ram_handle_compressed(void *host, uint8_t ch, 
uint64_t size)
 static int ram_load(QEMUFile *f, void *opaque, int version_id)
 {
     ram_addr_t addr;
-    int flags, ret = 0;
+    int flags = 0, ret = 0;
     static uint64_t seq_iter;

     seq_iter++;
@@ -1048,97 +1048,96 @@ static int ram_load(QEMUFile *f, void *opaque, int 
version_id)
         ret = -EINVAL;
     }

-    while (!ret) {
+    while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) {
         addr = qemu_get_be64(f);

         flags = addr & ~TARGET_PAGE_MASK;
         addr &= TARGET_PAGE_MASK;

-        if (flags & RAM_SAVE_FLAG_MEM_SIZE) {
-            /* Synchronize RAM block list */
-            char id[256];
-            ram_addr_t length;
-            ram_addr_t total_ram_bytes = addr;
-
-            while (total_ram_bytes) {
-                RAMBlock *block;
-                uint8_t len;
-
-                len = qemu_get_byte(f);
-                qemu_get_buffer(f, (uint8_t *)id, len);
-                id[len] = 0;
-                length = qemu_get_be64(f);
-
-                QTAILQ_FOREACH(block, &ram_list.blocks, next) {
-                    if (!strncmp(id, block->idstr, sizeof(id))) {
-                        if (block->length != length) {
-                            error_report("Length mismatch: %s: " RAM_ADDR_FMT
-                                         " in != " RAM_ADDR_FMT, id, length,
-                                         block->length);
-                            ret =  -EINVAL;
+        if (flags & RAM_SAVE_FLAG_HOOK) {
+            ram_control_load_hook(f, flags);
+        } else {
+            ram_addr_t total_ram_bytes;
+            void *host;
+            uint8_t ch;
+            switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
+            case RAM_SAVE_FLAG_MEM_SIZE:
+                /* Synchronize RAM block list */
+                total_ram_bytes = addr;
+                while (total_ram_bytes) {
+                    RAMBlock *block;
+                    uint8_t len;
+                    char id[256];
+                    ram_addr_t length;
+
+                    len = qemu_get_byte(f);
+                    qemu_get_buffer(f, (uint8_t *)id, len);
+                    id[len] = 0;
+                    length = qemu_get_be64(f);
+
+                    QTAILQ_FOREACH(block, &ram_list.blocks, next) {
+                        if (!strncmp(id, block->idstr, sizeof(id))) {
+                            if (block->length != length) {
+                                error_report("Length mismatch: %s: " 
RAM_ADDR_FMT
+                                             " in != " RAM_ADDR_FMT, id, 
length,
+                                             block->length);
+                                ret =  -EINVAL;
+                            }
+                            continue;
                         }
-                        break;
                     }
+                    if (!block) {
+                        error_report("Unknown ramblock \"%s\", cannot "
+                                     "accept migration", id);
+                        ret = -EINVAL;
+                    }
+                    if (ret) {
+                        continue;
+                    }
+                    total_ram_bytes -= length;
                 }
-
-                if (!block) {
-                    error_report("Unknown ramblock \"%s\", cannot "
-                                 "accept migration", id);
+                break;
+            case RAM_SAVE_FLAG_COMPRESS:
+                host = host_from_stream_offset(f, addr, flags);
+                if (!host) {
+                    error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
                     ret = -EINVAL;
+                    continue;
                 }
-                if (ret) {
-                    break;
-                }
-
-                total_ram_bytes -= length;
-            }
-        } else if (flags & RAM_SAVE_FLAG_COMPRESS) {
-            void *host;
-            uint8_t ch;
-
-            host = host_from_stream_offset(f, addr, flags);
-            if (!host) {
-                error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
-                ret = -EINVAL;
+                ch = qemu_get_byte(f);
+                ram_handle_compressed(host, ch, TARGET_PAGE_SIZE);
                 break;
-            }
-
-            ch = qemu_get_byte(f);
-            ram_handle_compressed(host, ch, TARGET_PAGE_SIZE);
-        } else if (flags & RAM_SAVE_FLAG_PAGE) {
-            void *host;
-
-            host = host_from_stream_offset(f, addr, flags);
-            if (!host) {
-                error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
-                ret = -EINVAL;
+            case RAM_SAVE_FLAG_PAGE:
+                host = host_from_stream_offset(f, addr, flags);
+                if (!host) {
+                    error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
+                    ret = -EINVAL;
+                    continue;
+                }
+                qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
                 break;
-            }
-
-            qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
-        } else if (flags & RAM_SAVE_FLAG_XBZRLE) {
-            void *host = host_from_stream_offset(f, addr, flags);
-            if (!host) {
-                error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
-                ret = -EINVAL;
+            case RAM_SAVE_FLAG_XBZRLE:
+                host = host_from_stream_offset(f, addr, flags);
+                if (!host) {
+                    error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
+                    ret = -EINVAL;
+                    continue;
+                }
+                if (load_xbzrle(f, addr, host) < 0) {
+                    error_report("Failed to decompress XBZRLE page at "
+                                 RAM_ADDR_FMT, addr);
+                    ret = -EINVAL;
+                    continue;
+                }
                 break;
-            }
-
-            if (load_xbzrle(f, addr, host) < 0) {
-                error_report("Failed to decompress XBZRLE page at "
-                             RAM_ADDR_FMT, addr);
+            case RAM_SAVE_FLAG_EOS:
+                /* normal exit */
+                continue;
+            default:
+                error_report("Unknown migration flags: %#x", flags);
                 ret = -EINVAL;
-                break;
+                continue;
             }
-        } else if (flags & RAM_SAVE_FLAG_HOOK) {
-            ram_control_load_hook(f, flags);
-        } else if (flags & RAM_SAVE_FLAG_EOS) {
-            /* normal exit */
-            break;
-        } else {
-            error_report("Unknown migration flags: %#x", flags);
-            ret = -EINVAL;
-            break;
         }
         ret = qemu_file_get_error(f);
     }