From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44596) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wwpjw-0001Sn-Rj for qemu-devel@nongnu.org; Tue, 17 Jun 2014 05:31:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wwpjo-0001Ok-8t for qemu-devel@nongnu.org; Tue, 17 Jun 2014 05:31:40 -0400 Received: from mx-v6.kamp.de ([2a02:248:0:51::16]:34575 helo=mx01.kamp.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wwpjn-0001NY-R9 for qemu-devel@nongnu.org; Tue, 17 Jun 2014 05:31:32 -0400 Message-ID: <53A00AEF.2090304@kamp.de> Date: Tue, 17 Jun 2014 11:31:27 +0200 From: Peter Lieven MIME-Version: 1.0 References: <1402888539-14961-1-git-send-email-quintela@redhat.com> <1402888539-14961-7-git-send-email-quintela@redhat.com> <539F2A0A.40401@redhat.com> In-Reply-To: <539F2A0A.40401@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 6/6] migration: catch unknown flags in ram_load List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , Juan Quintela , qemu-devel@nongnu.org, Amit Shah On 16.06.2014 19:31, Eric Blake wrote: > On 06/15/2014 09:15 PM, Juan Quintela wrote: >> From: Peter Lieven >> >> 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); }