[Qemu-devel] [PATCH 0/3] UUID validation during migration

[Qemu-devel] [PATCH 0/3] UUID validation during migration

[Yury Kotov]

Hi,

This series adds an UUID validation at the start of the migration
on the target side. The idea is to identify the source of migration.

Possible case of problem:
1. There are 3 servers: A, B and C
2. Server A has a VM 1, server B has a VM 2
3. VM 1 and VM 2 want to migrate to the server C
4. Target of VM 1 starts on the server C and dies too quickly for some reason
5. Target of VM 2 starts just after that and listen the same tcp port X, which
   the target of VM 1 wanted to use
6. Source of VM 1 connects to the tcp port X, and migrates to VM 2 source
7. It's possible that migration might be successful (e.g., devices are the same)
8. So, the target of VM 2 is in undefined state

The series adds a capability to prevent successful (by mistake) migration.

The new capability x-validate-uuid only affects the source so that it sends
its UUID to the target. The target will validate the received UUID and stop
the migration if UUIDs are not equal.

Regards,
Yury

Yury Kotov (3):
  migration: Add x-validate-uuid capability
  tests/libqtest: Allow to set expected exit status
  tests/migration: Add a test for x-validate-uuid capability

 migration/migration.c  |   9 +++
 migration/migration.h  |   1 +
 migration/savevm.c     |  45 +++++++++++++
 qapi/migration.json    |   5 +-
 tests/libqtest.c       |  14 ++++-
 tests/libqtest.h       |   9 +++
 tests/migration-test.c | 140 ++++++++++++++++++++++++++++++++---------
 7 files changed, 189 insertions(+), 34 deletions(-)

-- 
2.22.0

[Qemu-devel] [PATCH 1/3] migration: Add x-validate-uuid capability

[Yury Kotov]

This capability realizes simple source validation by UUID.
It's useful for live migration between hosts.

Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
---
 migration/migration.c |  9 +++++++++
 migration/migration.h |  1 +
 migration/savevm.c    | 45 +++++++++++++++++++++++++++++++++++++++++++
 qapi/migration.json   |  5 ++++-
 4 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index 8b9f2fe30a..910e11b7d7 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2140,6 +2140,15 @@ bool migrate_ignore_shared(void)
     return s->enabled_capabilities[MIGRATION_CAPABILITY_X_IGNORE_SHARED];
 }
 
+bool migrate_validate_uuid(void)
+{
+    MigrationState *s;
+
+    s = migrate_get_current();
+
+    return s->enabled_capabilities[MIGRATION_CAPABILITY_X_VALIDATE_UUID];
+}
+
 bool migrate_use_events(void)
 {
     MigrationState *s;
diff --git a/migration/migration.h b/migration/migration.h
index 3e1ea2b5dc..4f2fe193dc 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -290,6 +290,7 @@ bool migrate_postcopy_ram(void);
 bool migrate_zero_blocks(void);
 bool migrate_dirty_bitmaps(void);
 bool migrate_ignore_shared(void);
+bool migrate_validate_uuid(void);
 
 bool migrate_auto_converge(void);
 bool migrate_use_multifd(void);
diff --git a/migration/savevm.c b/migration/savevm.c
index 4a86128ac4..493dc24fd2 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -256,6 +256,7 @@ typedef struct SaveState {
     uint32_t target_page_bits;
     uint32_t caps_count;
     MigrationCapability *capabilities;
+    QemuUUID uuid;
 } SaveState;
 
 static SaveState savevm_state = {
@@ -307,6 +308,7 @@ static int configuration_pre_save(void *opaque)
             state->capabilities[j++] = i;
         }
     }
+    state->uuid = qemu_uuid;
 
     return 0;
 }
@@ -464,6 +466,48 @@ static const VMStateDescription vmstate_capabilites = {
     }
 };
 
+static bool vmstate_uuid_needed(void *opaque)
+{
+    return qemu_uuid_set && migrate_validate_uuid();
+}
+
+static int vmstate_uuid_post_load(void *opaque, int version_id)
+{
+    SaveState *state = opaque;
+    char uuid_src[UUID_FMT_LEN + 1];
+    char uuid_dst[UUID_FMT_LEN + 1];
+
+    if (!qemu_uuid_set) {
+        /*
+         * It's warning because user might not know UUID in some cases,
+         * e.g. load an old snapshot
+         */
+        qemu_uuid_unparse(&state->uuid, uuid_src);
+        warn_report("UUID is received %s, but local uuid isn't set",
+                     uuid_src);
+        return 0;
+    }
+    if (!qemu_uuid_is_equal(&state->uuid, &qemu_uuid)) {
+        qemu_uuid_unparse(&state->uuid, uuid_src);
+        qemu_uuid_unparse(&qemu_uuid, uuid_dst);
+        error_report("UUID received is %s and local is %s", uuid_src, uuid_dst);
+        return -EINVAL;
+    }
+    return 0;
+}
+
+static const VMStateDescription vmstate_uuid = {
+    .name = "configuration/uuid",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = vmstate_uuid_needed,
+    .post_load = vmstate_uuid_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8_ARRAY_V(uuid.data, SaveState, sizeof(QemuUUID), 1),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_configuration = {
     .name = "configuration",
     .version_id = 1,
@@ -478,6 +522,7 @@ static const VMStateDescription vmstate_configuration = {
     .subsections = (const VMStateDescription*[]) {
         &vmstate_target_page_bits,
         &vmstate_capabilites,
+        &vmstate_uuid,
         NULL
     }
 };
diff --git a/qapi/migration.json b/qapi/migration.json
index 9cfbaf8c6c..b7a8064745 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -415,6 +415,9 @@
 #
 # @x-ignore-shared: If enabled, QEMU will not migrate shared memory (since 4.0)
 #
+# @x-validate-uuid: Check whether the UUID is the same on both sides or not.
+#                   (since 4.2)
+#
 # Since: 1.2
 ##
 { 'enum': 'MigrationCapability',
@@ -422,7 +425,7 @@
            'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram',
            'block', 'return-path', 'pause-before-switchover', 'multifd',
            'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
-           'x-ignore-shared' ] }
+           'x-ignore-shared', 'x-validate-uuid' ] }
 
 ##
 # @MigrationCapabilityStatus:
-- 
2.22.0

[Qemu-devel] [PATCH 2/3] tests/libqtest: Allow to set expected exit status

[Yury Kotov]

Add qtest_set_expected_status function to set expected exit status of
child process. By default expected exit status is 0.

Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
---
 tests/libqtest.c | 14 +++++++++++---
 tests/libqtest.h |  9 +++++++++
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 2713b86cf7..118d779c1b 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -43,6 +43,7 @@ struct QTestState
     int qmp_fd;
     pid_t qemu_pid;  /* our child QEMU process */
     int wstatus;
+    int expected_status;
     bool big_endian;
     bool irq_level[MAX_IRQ];
     GString *rx;
@@ -113,6 +114,11 @@ bool qtest_probe_child(QTestState *s)
     return false;
 }
 
+void qtest_set_expected_status(QTestState *s, int status)
+{
+    s->expected_status = status;
+}
+
 static void kill_qemu(QTestState *s)
 {
     pid_t pid = s->qemu_pid;
@@ -130,11 +136,12 @@ static void kill_qemu(QTestState *s)
      * fishy and should be logged with as much detail as possible.
      */
     wstatus = s->wstatus;
-    if (wstatus) {
+    if (WEXITSTATUS(wstatus) != s->expected_status) {
         if (WIFEXITED(wstatus)) {
             fprintf(stderr, "%s:%d: kill_qemu() tried to terminate QEMU "
-                    "process but encountered exit status %d\n",
-                    __FILE__, __LINE__, WEXITSTATUS(wstatus));
+                    "process but encountered exit status %d (expected %d)\n",
+                    __FILE__, __LINE__, WEXITSTATUS(wstatus),
+                    s->expected_status);
         } else if (WIFSIGNALED(wstatus)) {
             int sig = WTERMSIG(wstatus);
             const char *signame = strsignal(sig) ?: "unknown ???";
@@ -248,6 +255,7 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
     g_test_message("starting QEMU: %s", command);
 
     s->wstatus = 0;
+    s->expected_status = 0;
     s->qemu_pid = fork();
     if (s->qemu_pid == 0) {
         setenv("QEMU_AUDIO_DRV", "none", true);
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 07ea35867c..c00bca94af 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -997,4 +997,13 @@ void qmp_assert_error_class(QDict *rsp, const char *class);
  */
 bool qtest_probe_child(QTestState *s);
 
+/**
+ * qtest_set_expected_status:
+ * @s: QTestState instance to operate on.
+ * @status: an expected exit status.
+ *
+ * Set expected exit status of the child.
+ */
+void qtest_set_expected_status(QTestState *s, int status);
+
 #endif
-- 
2.22.0

[Qemu-devel] [PATCH 3/3] tests/migration: Add a test for x-validate-uuid capability

[Yury Kotov]

Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
---
 tests/migration-test.c | 140 ++++++++++++++++++++++++++++++++---------
 1 file changed, 110 insertions(+), 30 deletions(-)

diff --git a/tests/migration-test.c b/tests/migration-test.c
index b87ba99a9e..adac1c01a2 100644
--- a/tests/migration-test.c
+++ b/tests/migration-test.c
@@ -512,7 +512,8 @@ static void migrate_postcopy_start(QTestState *from, QTestState *to)
 
 static int test_migrate_start(QTestState **from, QTestState **to,
                                const char *uri, bool hide_stderr,
-                               bool use_shmem)
+                               bool use_shmem, const char *opts_src,
+                               const char *opts_dst)
 {
     gchar *cmd_src, *cmd_dst;
     char *bootpath = NULL;
@@ -521,6 +522,9 @@ static int test_migrate_start(QTestState **from, QTestState **to,
     const char *arch = qtest_get_arch();
     const char *accel = "kvm:tcg";
 
+    opts_src = opts_src ? opts_src : "";
+    opts_dst = opts_dst ? opts_dst : "";
+
     if (use_shmem) {
         if (!g_file_test("/dev/shm", G_FILE_TEST_IS_DIR)) {
             g_test_skip("/dev/shm is not supported");
@@ -539,16 +543,16 @@ static int test_migrate_start(QTestState **from, QTestState **to,
         cmd_src = g_strdup_printf("-machine accel=%s -m 150M"
                                   " -name source,debug-threads=on"
                                   " -serial file:%s/src_serial"
-                                  " -drive file=%s,format=raw %s",
+                                  " -drive file=%s,format=raw %s %s",
                                   accel, tmpfs, bootpath,
-                                  extra_opts ? extra_opts : "");
+                                  extra_opts ? extra_opts : "", opts_src);
         cmd_dst = g_strdup_printf("-machine accel=%s -m 150M"
                                   " -name target,debug-threads=on"
                                   " -serial file:%s/dest_serial"
                                   " -drive file=%s,format=raw"
-                                  " -incoming %s %s",
+                                  " -incoming %s %s %s",
                                   accel, tmpfs, bootpath, uri,
-                                  extra_opts ? extra_opts : "");
+                                  extra_opts ? extra_opts : "", opts_dst);
         start_address = X86_TEST_MEM_START;
         end_address = X86_TEST_MEM_END;
     } else if (g_str_equal(arch, "s390x")) {
@@ -556,15 +560,15 @@ static int test_migrate_start(QTestState **from, QTestState **to,
         extra_opts = use_shmem ? get_shmem_opts("128M", shmem_path) : NULL;
         cmd_src = g_strdup_printf("-machine accel=%s -m 128M"
                                   " -name source,debug-threads=on"
-                                  " -serial file:%s/src_serial -bios %s %s",
+                                  " -serial file:%s/src_serial -bios %s %s %s",
                                   accel, tmpfs, bootpath,
-                                  extra_opts ? extra_opts : "");
+                                  extra_opts ? extra_opts : "", opts_src);
         cmd_dst = g_strdup_printf("-machine accel=%s -m 128M"
                                   " -name target,debug-threads=on"
                                   " -serial file:%s/dest_serial -bios %s"
-                                  " -incoming %s %s",
+                                  " -incoming %s %s %s",
                                   accel, tmpfs, bootpath, uri,
-                                  extra_opts ? extra_opts : "");
+                                  extra_opts ? extra_opts : "", opts_dst);
         start_address = S390_TEST_MEM_START;
         end_address = S390_TEST_MEM_END;
     } else if (strcmp(arch, "ppc64") == 0) {
@@ -575,14 +579,15 @@ static int test_migrate_start(QTestState **from, QTestState **to,
                                   " -prom-env 'use-nvramrc?=true' -prom-env "
                                   "'nvramrc=hex .\" _\" begin %x %x "
                                   "do i c@ 1 + i c! 1000 +loop .\" B\" 0 "
-                                  "until' %s",  accel, tmpfs, end_address,
-                                  start_address, extra_opts ? extra_opts : "");
+                                  "until' %s %s",  accel, tmpfs, end_address,
+                                  start_address, extra_opts ? extra_opts : "",
+                                  opts_src);
         cmd_dst = g_strdup_printf("-machine accel=%s -m 256M"
                                   " -name target,debug-threads=on"
                                   " -serial file:%s/dest_serial"
-                                  " -incoming %s %s",
+                                  " -incoming %s %s %s",
                                   accel, tmpfs, uri,
-                                  extra_opts ? extra_opts : "");
+                                  extra_opts ? extra_opts : "", opts_dst);
 
         start_address = PPC_TEST_MEM_START;
         end_address = PPC_TEST_MEM_END;
@@ -592,16 +597,16 @@ static int test_migrate_start(QTestState **from, QTestState **to,
         cmd_src = g_strdup_printf("-machine virt,accel=%s,gic-version=max "
                                   "-name vmsource,debug-threads=on -cpu max "
                                   "-m 150M -serial file:%s/src_serial "
-                                  "-kernel %s %s",
+                                  "-kernel %s %s %s",
                                   accel, tmpfs, bootpath,
-                                  extra_opts ? extra_opts : "");
+                                  extra_opts ? extra_opts : "", opts_src);
         cmd_dst = g_strdup_printf("-machine virt,accel=%s,gic-version=max "
                                   "-name vmdest,debug-threads=on -cpu max "
                                   "-m 150M -serial file:%s/dest_serial "
                                   "-kernel %s "
-                                  "-incoming %s %s",
+                                  "-incoming %s %s %s",
                                   accel, tmpfs, bootpath, uri,
-                                  extra_opts ? extra_opts : "");
+                                  extra_opts ? extra_opts : "", opts_dst);
 
         start_address = ARM_TEST_MEM_START;
         end_address = ARM_TEST_MEM_END;
@@ -731,7 +736,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
     char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
     QTestState *from, *to;
 
-    if (test_migrate_start(&from, &to, uri, hide_error, false)) {
+    if (test_migrate_start(&from, &to, uri, hide_error, false, NULL, NULL)) {
         return -1;
     }
 
@@ -841,20 +846,16 @@ static void test_postcopy_recovery(void)
     migrate_postcopy_complete(from, to);
 }
 
-static void test_baddest(void)
+static void wait_for_migration_fail(QTestState *from, bool allow_active)
 {
-    QTestState *from, *to;
     QDict *rsp_return;
     char *status;
     bool failed;
 
-    if (test_migrate_start(&from, &to, "tcp:0:0", true, false)) {
-        return;
-    }
-    migrate(from, "tcp:0:0", "{}");
     do {
         status = migrate_query_status(from);
-        g_assert(!strcmp(status, "setup") || !(strcmp(status, "failed")));
+        g_assert(!strcmp(status, "setup") || !strcmp(status, "failed") ||
+                 (allow_active && !strcmp(status, "active")));
         failed = !strcmp(status, "failed");
         g_free(status);
     } while (!failed);
@@ -864,7 +865,17 @@ static void test_baddest(void)
     g_assert(qdict_haskey(rsp_return, "running"));
     g_assert(qdict_get_bool(rsp_return, "running"));
     qobject_unref(rsp_return);
+}
+
+static void test_baddest(void)
+{
+    QTestState *from, *to;
 
+    if (test_migrate_start(&from, &to, "tcp:0:0", true, false, NULL, NULL)) {
+        return;
+    }
+    migrate(from, "tcp:0:0", "{}");
+    wait_for_migration_fail(from, false);
     test_migrate_end(from, to, false);
 }
 
@@ -873,7 +884,7 @@ static void test_precopy_unix(void)
     char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
     QTestState *from, *to;
 
-    if (test_migrate_start(&from, &to, uri, false, false)) {
+    if (test_migrate_start(&from, &to, uri, false, false, NULL, NULL)) {
         return;
     }
 
@@ -916,7 +927,7 @@ static void test_ignore_shared(void)
     char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
     QTestState *from, *to;
 
-    if (test_migrate_start(&from, &to, uri, false, true)) {
+    if (test_migrate_start(&from, &to, uri, false, true, NULL, NULL)) {
         return;
     }
 
@@ -951,7 +962,7 @@ static void test_xbzrle(const char *uri)
 {
     QTestState *from, *to;
 
-    if (test_migrate_start(&from, &to, uri, false, false)) {
+    if (test_migrate_start(&from, &to, uri, false, false, NULL, NULL)) {
         return;
     }
 
@@ -1003,7 +1014,8 @@ static void test_precopy_tcp(void)
     char *uri;
     QTestState *from, *to;
 
-    if (test_migrate_start(&from, &to, "tcp:127.0.0.1:0", false, false)) {
+    if (test_migrate_start(&from, &to, "tcp:127.0.0.1:0", false, false,
+                           NULL, NULL)) {
         return;
     }
 
@@ -1049,7 +1061,7 @@ static void test_migrate_fd_proto(void)
     QDict *rsp;
     const char *error_desc;
 
-    if (test_migrate_start(&from, &to, "defer", false, false)) {
+    if (test_migrate_start(&from, &to, "defer", false, false, NULL, NULL)) {
         return;
     }
 
@@ -1125,6 +1137,68 @@ static void test_migrate_fd_proto(void)
     test_migrate_end(from, to, true);
 }
 
+static void do_test_validate_uuid(const char *uuid_arg_src,
+                                  const char *uuid_arg_dst,
+                                  bool should_fail, bool hide_stderr)
+{
+    char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
+    QTestState *from, *to;
+
+    if (test_migrate_start(&from, &to, uri, hide_stderr, false,
+                           uuid_arg_src, uuid_arg_dst)) {
+        return;
+    }
+
+    /*
+     * UUID validation is at the begin of migration. So, the main process of
+     * migration is not interesting for us here. Thus, set huge downtime for
+     * very fast migration.
+     */
+    migrate_set_parameter_int(from, "downtime-limit", 1000000);
+    migrate_set_capability(from, "x-validate-uuid", true);
+
+    /* Wait for the first serial output from the source */
+    wait_for_serial("src_serial");
+
+    migrate(from, uri, "{}");
+
+    if (should_fail) {
+        qtest_set_expected_status(to, 1);
+        wait_for_migration_fail(from, true);
+    } else {
+        wait_for_migration_complete(from);
+    }
+
+    test_migrate_end(from, to, false);
+    g_free(uri);
+}
+
+static void test_validate_uuid(void)
+{
+    do_test_validate_uuid("-uuid 11111111-1111-1111-1111-111111111111",
+                          "-uuid 11111111-1111-1111-1111-111111111111",
+                          false, false);
+}
+
+static void test_validate_uuid_error(void)
+{
+    do_test_validate_uuid("-uuid 11111111-1111-1111-1111-111111111111",
+                          "-uuid 22222222-2222-2222-2222-222222222222",
+                          true, true);
+}
+
+static void test_validate_uuid_src_not_set(void)
+{
+    do_test_validate_uuid(NULL, "-uuid 11111111-1111-1111-1111-111111111111",
+                          false, true);
+}
+
+static void test_validate_uuid_dst_not_set(void)
+{
+    do_test_validate_uuid("-uuid 11111111-1111-1111-1111-111111111111", NULL,
+                          false, true);
+}
+
 int main(int argc, char **argv)
 {
     char template[] = "/tmp/migration-test-XXXXXX";
@@ -1180,6 +1254,12 @@ int main(int argc, char **argv)
     /* qtest_add_func("/migration/ignore_shared", test_ignore_shared); */
     qtest_add_func("/migration/xbzrle/unix", test_xbzrle_unix);
     qtest_add_func("/migration/fd_proto", test_migrate_fd_proto);
+    qtest_add_func("/migration/validate_uuid", test_validate_uuid);
+    qtest_add_func("/migration/validate_uuid_error", test_validate_uuid_error);
+    qtest_add_func("/migration/validate_uuid_src_not_set",
+                   test_validate_uuid_src_not_set);
+    qtest_add_func("/migration/validate_uuid_dst_not_set",
+                   test_validate_uuid_dst_not_set);
 
     ret = g_test_run();
 
-- 
2.22.0

Re: [Qemu-devel] [PATCH 2/3] tests/libqtest: Allow to set expected exit status

[Marc-André Lureau]

Hi

On Tue, Aug 27, 2019 at 4:09 PM Yury Kotov <yury-kotov@yandex-team.ru> wrote:
>
> Add qtest_set_expected_status function to set expected exit status of
> child process. By default expected exit status is 0.
>
> Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
> ---
>  tests/libqtest.c | 14 +++++++++++---
>  tests/libqtest.h |  9 +++++++++
>  2 files changed, 20 insertions(+), 3 deletions(-)
>

I sent a vary similar patch with v1 of dbus-vmstate, and dropped it
because it no longer needs it in v2 (for now) "tests: add
qtest_set_exit_status()".

Your function is better named already.


> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 2713b86cf7..118d779c1b 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -43,6 +43,7 @@ struct QTestState
>      int qmp_fd;
>      pid_t qemu_pid;  /* our child QEMU process */
>      int wstatus;
> +    int expected_status;
>      bool big_endian;
>      bool irq_level[MAX_IRQ];
>      GString *rx;
> @@ -113,6 +114,11 @@ bool qtest_probe_child(QTestState *s)
>      return false;
>  }
>
> +void qtest_set_expected_status(QTestState *s, int status)
> +{
> +    s->expected_status = status;
> +}
> +
>  static void kill_qemu(QTestState *s)
>  {
>      pid_t pid = s->qemu_pid;
> @@ -130,11 +136,12 @@ static void kill_qemu(QTestState *s)
>       * fishy and should be logged with as much detail as possible.
>       */
>      wstatus = s->wstatus;
> -    if (wstatus) {
> +    if (WEXITSTATUS(wstatus) != s->expected_status) {

Shouldn't it check WEXITSTATUS value only when WIFEXITED ?

>          if (WIFEXITED(wstatus)) {
>              fprintf(stderr, "%s:%d: kill_qemu() tried to terminate QEMU "
> -                    "process but encountered exit status %d\n",
> -                    __FILE__, __LINE__, WEXITSTATUS(wstatus));
> +                    "process but encountered exit status %d (expected %d)\n",
> +                    __FILE__, __LINE__, WEXITSTATUS(wstatus),
> +                    s->expected_status);
>          } else if (WIFSIGNALED(wstatus)) {
>              int sig = WTERMSIG(wstatus);
>              const char *signame = strsignal(sig) ?: "unknown ???";
> @@ -248,6 +255,7 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
>      g_test_message("starting QEMU: %s", command);
>
>      s->wstatus = 0;
> +    s->expected_status = 0;
>      s->qemu_pid = fork();
>      if (s->qemu_pid == 0) {
>          setenv("QEMU_AUDIO_DRV", "none", true);
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index 07ea35867c..c00bca94af 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -997,4 +997,13 @@ void qmp_assert_error_class(QDict *rsp, const char *class);
>   */
>  bool qtest_probe_child(QTestState *s);
>
> +/**
> + * qtest_set_expected_status:
> + * @s: QTestState instance to operate on.
> + * @status: an expected exit status.
> + *
> + * Set expected exit status of the child.
> + */
> +void qtest_set_expected_status(QTestState *s, int status);
> +
>  #endif
> --
> 2.22.0
>
>


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 1/3] migration: Add x-validate-uuid capability

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 2018 bytes --]

On 8/27/19 7:02 AM, Yury Kotov wrote:
> This capability realizes simple source validation by UUID.
> It's useful for live migration between hosts.
> 
> Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
> ---
>  migration/migration.c |  9 +++++++++
>  migration/migration.h |  1 +
>  migration/savevm.c    | 45 +++++++++++++++++++++++++++++++++++++++++++
>  qapi/migration.json   |  5 ++++-
>  4 files changed, 59 insertions(+), 1 deletion(-)

Any reason why this is marked experimental?  It seems useful enough that
we could probably just add it as a fully-supported feature (dropping the
x- prefix) - but I'll leave that up to the migration maintainers.

In fact, do we even need this to be a tunable feature?  Why not just
always enable it?  As long as the UUID is sent in a way that new->old
doesn't break the old qemu from receiving the migration stream, and that
old->new copes with UUID being absent, then new->new will always benefit
from the additional safety check.

> +++ b/qapi/migration.json
> @@ -415,6 +415,9 @@
>  #
>  # @x-ignore-shared: If enabled, QEMU will not migrate shared memory (since 4.0)
>  #
> +# @x-validate-uuid: Check whether the UUID is the same on both sides or not.
> +#                   (since 4.2)

Maybe:

@x-validate-uuid: Send the UUID of the source to allow the destination
to ensure it is the same.

if we even need a tunable capability.

> +#
>  # Since: 1.2
>  ##
>  { 'enum': 'MigrationCapability',
> @@ -422,7 +425,7 @@
>             'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram',
>             'block', 'return-path', 'pause-before-switchover', 'multifd',
>             'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
> -           'x-ignore-shared' ] }
> +           'x-ignore-shared', 'x-validate-uuid' ] }
>  
>  ##
>  # @MigrationCapabilityStatus:
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 2/3] tests/libqtest: Allow to set expected exit status

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 886 bytes --]

On 8/27/19 7:02 AM, Yury Kotov wrote:

In the subject: 'Allow to $verb' is not idiomatic; either 'Allow
$subject to $verb' or 'Allow ${verb}ing' sound better.  In this case,
I'd go with:

tests/libqtest: Allow setting expected exit status

> Add qtest_set_expected_status function to set expected exit status of
> child process. By default expected exit status is 0.
> 

> @@ -130,11 +136,12 @@ static void kill_qemu(QTestState *s)
>       * fishy and should be logged with as much detail as possible.
>       */
>      wstatus = s->wstatus;
> -    if (wstatus) {
> +    if (WEXITSTATUS(wstatus) != s->expected_status) {
>          if (WIFEXITED(wstatus)) {

Wrong ordering. WEXITSTATUS() is not reliable unless WIFEXITED() is true.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 2/3] tests/libqtest: Allow to set expected exit status

[Yury Kotov]

27.08.2019, 16:53, "Marc-André Lureau" <marcandre.lureau@gmail.com>:
> Hi
>
> On Tue, Aug 27, 2019 at 4:09 PM Yury Kotov <yury-kotov@yandex-team.ru> wrote:
>>  Add qtest_set_expected_status function to set expected exit status of
>>  child process. By default expected exit status is 0.
>>
>>  Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
>>  ---
>>   tests/libqtest.c | 14 +++++++++++---
>>   tests/libqtest.h | 9 +++++++++
>>   2 files changed, 20 insertions(+), 3 deletions(-)
>
> I sent a vary similar patch with v1 of dbus-vmstate, and dropped it
> because it no longer needs it in v2 (for now) "tests: add
> qtest_set_exit_status()".
>
> Your function is better named already.
>

Thanks, I'll look at this realization

>>  diff --git a/tests/libqtest.c b/tests/libqtest.c
>>  index 2713b86cf7..118d779c1b 100644
>>  --- a/tests/libqtest.c
>>  +++ b/tests/libqtest.c
>>  @@ -43,6 +43,7 @@ struct QTestState
>>       int qmp_fd;
>>       pid_t qemu_pid; /* our child QEMU process */
>>       int wstatus;
>>  + int expected_status;
>>       bool big_endian;
>>       bool irq_level[MAX_IRQ];
>>       GString *rx;
>>  @@ -113,6 +114,11 @@ bool qtest_probe_child(QTestState *s)
>>       return false;
>>   }
>>
>>  +void qtest_set_expected_status(QTestState *s, int status)
>>  +{
>>  + s->expected_status = status;
>>  +}
>>  +
>>   static void kill_qemu(QTestState *s)
>>   {
>>       pid_t pid = s->qemu_pid;
>>  @@ -130,11 +136,12 @@ static void kill_qemu(QTestState *s)
>>        * fishy and should be logged with as much detail as possible.
>>        */
>>       wstatus = s->wstatus;
>>  - if (wstatus) {
>>  + if (WEXITSTATUS(wstatus) != s->expected_status) {
>
> Shouldn't it check WEXITSTATUS value only when WIFEXITED ?
>

Oh, it seems that you're right. I'll fix it in v2

>>           if (WIFEXITED(wstatus)) {
>>               fprintf(stderr, "%s:%d: kill_qemu() tried to terminate QEMU "
>>  - "process but encountered exit status %d\n",
>>  - __FILE__, __LINE__, WEXITSTATUS(wstatus));
>>  + "process but encountered exit status %d (expected %d)\n",
>>  + __FILE__, __LINE__, WEXITSTATUS(wstatus),
>>  + s->expected_status);
>>           } else if (WIFSIGNALED(wstatus)) {
>>               int sig = WTERMSIG(wstatus);
>>               const char *signame = strsignal(sig) ?: "unknown ???";
>>  @@ -248,6 +255,7 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
>>       g_test_message("starting QEMU: %s", command);
>>
>>       s->wstatus = 0;
>>  + s->expected_status = 0;
>>       s->qemu_pid = fork();
>>       if (s->qemu_pid == 0) {
>>           setenv("QEMU_AUDIO_DRV", "none", true);
>>  diff --git a/tests/libqtest.h b/tests/libqtest.h
>>  index 07ea35867c..c00bca94af 100644
>>  --- a/tests/libqtest.h
>>  +++ b/tests/libqtest.h
>>  @@ -997,4 +997,13 @@ void qmp_assert_error_class(QDict *rsp, const char *class);
>>    */
>>   bool qtest_probe_child(QTestState *s);
>>
>>  +/**
>>  + * qtest_set_expected_status:
>>  + * @s: QTestState instance to operate on.
>>  + * @status: an expected exit status.
>>  + *
>>  + * Set expected exit status of the child.
>>  + */
>>  +void qtest_set_expected_status(QTestState *s, int status);
>>  +
>>   #endif
>>  --
>>  2.22.0
>
> --
> Marc-André Lureau

Regards,
Yury

Re: [Qemu-devel] [PATCH 2/3] tests/libqtest: Allow to set expected exit status

[Yury Kotov]

27.08.2019, 17:04, "Eric Blake" <eblake@redhat.com>:
> On 8/27/19 7:02 AM, Yury Kotov wrote:
>
> In the subject: 'Allow to $verb' is not idiomatic; either 'Allow
> $subject to $verb' or 'Allow ${verb}ing' sound better. In this case,
> I'd go with:
>
> tests/libqtest: Allow setting expected exit status
>

Ok, thanks. I'll fix it in v2

>>  Add qtest_set_expected_status function to set expected exit status of
>>  child process. By default expected exit status is 0.
>
>>  @@ -130,11 +136,12 @@ static void kill_qemu(QTestState *s)
>>        * fishy and should be logged with as much detail as possible.
>>        */
>>       wstatus = s->wstatus;
>>  - if (wstatus) {
>>  + if (WEXITSTATUS(wstatus) != s->expected_status) {
>>           if (WIFEXITED(wstatus)) {
>
> Wrong ordering. WEXITSTATUS() is not reliable unless WIFEXITED() is true.
>

Yes, it's a bug.. I'll fix it in v2

> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc. +1-919-301-3226
> Virtualization: qemu.org | libvirt.org

Regards,
Yury

Re: [Qemu-devel] [PATCH 1/3] migration: Add x-validate-uuid capability

[Yury Kotov]

27.08.2019, 17:02, "Eric Blake" <eblake@redhat.com>:
> On 8/27/19 7:02 AM, Yury Kotov wrote:
>>  This capability realizes simple source validation by UUID.
>>  It's useful for live migration between hosts.
>>
>>  Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>
>>  ---
>>   migration/migration.c | 9 +++++++++
>>   migration/migration.h | 1 +
>>   migration/savevm.c | 45 +++++++++++++++++++++++++++++++++++++++++++
>>   qapi/migration.json | 5 ++++-
>>   4 files changed, 59 insertions(+), 1 deletion(-)
>
> Any reason why this is marked experimental? It seems useful enough that
> we could probably just add it as a fully-supported feature (dropping the
> x- prefix) - but I'll leave that up to the migration maintainers.
>

I thought that all new capabilities have x- prefix... May be it's really
unnecessary here, I'm not sure.

> In fact, do we even need this to be a tunable feature? Why not just
> always enable it? As long as the UUID is sent in a way that new->old
> doesn't break the old qemu from receiving the migration stream, and that
> old->new copes with UUID being absent, then new->new will always benefit
> from the additional safety check.
>

In such case we couldn't migrate from e.g. 4.2 to 3.1
If it's not a problem then we can always use it.

>>  +++ b/qapi/migration.json
>>  @@ -415,6 +415,9 @@
>>   #
>>   # @x-ignore-shared: If enabled, QEMU will not migrate shared memory (since 4.0)
>>   #
>>  +# @x-validate-uuid: Check whether the UUID is the same on both sides or not.
>>  +# (since 4.2)
>
> Maybe:
>
> @x-validate-uuid: Send the UUID of the source to allow the destination
> to ensure it is the same.
>
> if we even need a tunable capability.
>

Yes, it sounds better. Thanks!

>>  +#
>>   # Since: 1.2
>>   ##
>>   { 'enum': 'MigrationCapability',
>>  @@ -422,7 +425,7 @@
>>              'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram',
>>              'block', 'return-path', 'pause-before-switchover', 'multifd',
>>              'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
>>  - 'x-ignore-shared' ] }
>>  + 'x-ignore-shared', 'x-validate-uuid' ] }
>>
>>   ##
>>   # @MigrationCapabilityStatus:
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc. +1-919-301-3226
> Virtualization: qemu.org | libvirt.org

Regards,
Yury

Re: [Qemu-devel] [PATCH 1/3] migration: Add x-validate-uuid capability

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 1979 bytes --]

On 8/27/19 10:36 AM, Yury Kotov wrote:
> 27.08.2019, 17:02, "Eric Blake" <eblake@redhat.com>:
>> On 8/27/19 7:02 AM, Yury Kotov wrote:
>>>  This capability realizes simple source validation by UUID.
>>>  It's useful for live migration between hosts.
>>>

>>
>> Any reason why this is marked experimental? It seems useful enough that
>> we could probably just add it as a fully-supported feature (dropping the
>> x- prefix) - but I'll leave that up to the migration maintainers.
>>
> 
> I thought that all new capabilities have x- prefix... May be it's really
> unnecessary here, I'm not sure.

New features that need some testing or possible changes to behavior need
x- to mark them as experimental, so we can make those changes without
worrying about breaking back-compat.  But new features that are outright
useful and presumably in their final form, with no further
experimentation needed, can skip going through an x- phase.

> 
>> In fact, do we even need this to be a tunable feature? Why not just
>> always enable it? As long as the UUID is sent in a way that new->old
>> doesn't break the old qemu from receiving the migration stream, and that
>> old->new copes with UUID being absent, then new->new will always benefit
>> from the additional safety check.
>>
> 
> In such case we couldn't migrate from e.g. 4.2 to 3.1

I don't know the migration format enough to know if there is a way for
4.2 to unconditionally send a UUID as a subsection such that a receiving
3.1 will ignore the unknown subsection. If so, then you don't need a
knob; if not, then you need something to say whether sending the
subsection is safe (perhaps default on in new machine types, but default
off for machine types that might still be migrated back to 3.1).  That's
where I'm hoping the migration experts will chime in.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 0/3] UUID validation during migration

[Dr. David Alan Gilbert]

* Yury Kotov (yury-kotov@yandex-team.ru) wrote:
> Hi,
> 
> This series adds an UUID validation at the start of the migration
> on the target side. The idea is to identify the source of migration.
> 
> Possible case of problem:
> 1. There are 3 servers: A, B and C
> 2. Server A has a VM 1, server B has a VM 2
> 3. VM 1 and VM 2 want to migrate to the server C
> 4. Target of VM 1 starts on the server C and dies too quickly for some reason
> 5. Target of VM 2 starts just after that and listen the same tcp port X, which
>    the target of VM 1 wanted to use
> 6. Source of VM 1 connects to the tcp port X, and migrates to VM 2 source

That shouldn't be possible in practice; you specify the destination tcp
port when you start the destination qemu; so unless the management code
that starts the migration is very broken it should know which port it's
migrating to.
However, if it is very broken then this is a good check.

Dave

> 7. It's possible that migration might be successful (e.g., devices are the same)
> 8. So, the target of VM 2 is in undefined state
> 
> The series adds a capability to prevent successful (by mistake) migration.
> 
> The new capability x-validate-uuid only affects the source so that it sends
> its UUID to the target. The target will validate the received UUID and stop
> the migration if UUIDs are not equal.
> 
> Regards,
> Yury
> 
> Yury Kotov (3):
>   migration: Add x-validate-uuid capability
>   tests/libqtest: Allow to set expected exit status
>   tests/migration: Add a test for x-validate-uuid capability
> 
>  migration/migration.c  |   9 +++
>  migration/migration.h  |   1 +
>  migration/savevm.c     |  45 +++++++++++++
>  qapi/migration.json    |   5 +-
>  tests/libqtest.c       |  14 ++++-
>  tests/libqtest.h       |   9 +++
>  tests/migration-test.c | 140 ++++++++++++++++++++++++++++++++---------
>  7 files changed, 189 insertions(+), 34 deletions(-)
> 
> -- 
> 2.22.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 1/3] migration: Add x-validate-uuid capability

[Dr. David Alan Gilbert]

* Eric Blake (eblake@redhat.com) wrote:
> On 8/27/19 10:36 AM, Yury Kotov wrote:
> > 27.08.2019, 17:02, "Eric Blake" <eblake@redhat.com>:
> >> On 8/27/19 7:02 AM, Yury Kotov wrote:
> >>>  This capability realizes simple source validation by UUID.
> >>>  It's useful for live migration between hosts.
> >>>
> 
> >>
> >> Any reason why this is marked experimental? It seems useful enough that
> >> we could probably just add it as a fully-supported feature (dropping the
> >> x- prefix) - but I'll leave that up to the migration maintainers.
> >>
> > 
> > I thought that all new capabilities have x- prefix... May be it's really
> > unnecessary here, I'm not sure.
> 
> New features that need some testing or possible changes to behavior need
> x- to mark them as experimental, so we can make those changes without
> worrying about breaking back-compat.  But new features that are outright
> useful and presumably in their final form, with no further
> experimentation needed, can skip going through an x- phase.
> 
> > 
> >> In fact, do we even need this to be a tunable feature? Why not just
> >> always enable it? As long as the UUID is sent in a way that new->old
> >> doesn't break the old qemu from receiving the migration stream, and that
> >> old->new copes with UUID being absent, then new->new will always benefit
> >> from the additional safety check.
> >>
> > 
> > In such case we couldn't migrate from e.g. 4.2 to 3.1
> 
> I don't know the migration format enough to know if there is a way for
> 4.2 to unconditionally send a UUID as a subsection such that a receiving
> 3.1 will ignore the unknown subsection. If so, then you don't need a
> knob; if not, then you need something to say whether sending the
> subsection is safe (perhaps default on in new machine types, but default
> off for machine types that might still be migrated back to 3.1).  That's
> where I'm hoping the migration experts will chime in.

Right; the migration format can't ignore chunks of data; so it does need
to know somehow; the choice is either a capability or wiring it to the
machine type;  my preference is to wire it to the machine type; the
arguments are:
    a) Machine type
       Pro: libvirt doesn't need to do anything
       Con: It doesn't protect old machine types
            It's not really part of the guest state

    b) Capability
       Pro: Works on all machine types
       Con: Libvirt needs to enable it

So, no strong preference but I think I prefer (a).

Dave

> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
> 



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

Re: [Qemu-devel] [PATCH 1/3] migration: Add x-validate-uuid capability

[Dr. David Alan Gilbert]

* Yury Kotov (yury-kotov@yandex-team.ru) wrote:
> This capability realizes simple source validation by UUID.
> It's useful for live migration between hosts.
> 
> Signed-off-by: Yury Kotov <yury-kotov@yandex-team.ru>

So, ignoring the question of whether it should be a capaibility or not,
I'm actually OK with this; I would remove the x- - it's simple enough
not to need to go through experimental I think.

Dave

> ---
>  migration/migration.c |  9 +++++++++
>  migration/migration.h |  1 +
>  migration/savevm.c    | 45 +++++++++++++++++++++++++++++++++++++++++++
>  qapi/migration.json   |  5 ++++-
>  4 files changed, 59 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 8b9f2fe30a..910e11b7d7 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2140,6 +2140,15 @@ bool migrate_ignore_shared(void)
>      return s->enabled_capabilities[MIGRATION_CAPABILITY_X_IGNORE_SHARED];
>  }
>  
> +bool migrate_validate_uuid(void)
> +{
> +    MigrationState *s;
> +
> +    s = migrate_get_current();
> +
> +    return s->enabled_capabilities[MIGRATION_CAPABILITY_X_VALIDATE_UUID];
> +}
> +
>  bool migrate_use_events(void)
>  {
>      MigrationState *s;
> diff --git a/migration/migration.h b/migration/migration.h
> index 3e1ea2b5dc..4f2fe193dc 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -290,6 +290,7 @@ bool migrate_postcopy_ram(void);
>  bool migrate_zero_blocks(void);
>  bool migrate_dirty_bitmaps(void);
>  bool migrate_ignore_shared(void);
> +bool migrate_validate_uuid(void);
>  
>  bool migrate_auto_converge(void);
>  bool migrate_use_multifd(void);
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 4a86128ac4..493dc24fd2 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -256,6 +256,7 @@ typedef struct SaveState {
>      uint32_t target_page_bits;
>      uint32_t caps_count;
>      MigrationCapability *capabilities;
> +    QemuUUID uuid;
>  } SaveState;
>  
>  static SaveState savevm_state = {
> @@ -307,6 +308,7 @@ static int configuration_pre_save(void *opaque)
>              state->capabilities[j++] = i;
>          }
>      }
> +    state->uuid = qemu_uuid;
>  
>      return 0;
>  }
> @@ -464,6 +466,48 @@ static const VMStateDescription vmstate_capabilites = {
>      }
>  };
>  
> +static bool vmstate_uuid_needed(void *opaque)
> +{
> +    return qemu_uuid_set && migrate_validate_uuid();
> +}
> +
> +static int vmstate_uuid_post_load(void *opaque, int version_id)
> +{
> +    SaveState *state = opaque;
> +    char uuid_src[UUID_FMT_LEN + 1];
> +    char uuid_dst[UUID_FMT_LEN + 1];
> +
> +    if (!qemu_uuid_set) {
> +        /*
> +         * It's warning because user might not know UUID in some cases,
> +         * e.g. load an old snapshot
> +         */
> +        qemu_uuid_unparse(&state->uuid, uuid_src);
> +        warn_report("UUID is received %s, but local uuid isn't set",
> +                     uuid_src);
> +        return 0;
> +    }
> +    if (!qemu_uuid_is_equal(&state->uuid, &qemu_uuid)) {
> +        qemu_uuid_unparse(&state->uuid, uuid_src);
> +        qemu_uuid_unparse(&qemu_uuid, uuid_dst);
> +        error_report("UUID received is %s and local is %s", uuid_src, uuid_dst);
> +        return -EINVAL;
> +    }
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_uuid = {
> +    .name = "configuration/uuid",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = vmstate_uuid_needed,
> +    .post_load = vmstate_uuid_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8_ARRAY_V(uuid.data, SaveState, sizeof(QemuUUID), 1),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_configuration = {
>      .name = "configuration",
>      .version_id = 1,
> @@ -478,6 +522,7 @@ static const VMStateDescription vmstate_configuration = {
>      .subsections = (const VMStateDescription*[]) {
>          &vmstate_target_page_bits,
>          &vmstate_capabilites,
> +        &vmstate_uuid,
>          NULL
>      }
>  };
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 9cfbaf8c6c..b7a8064745 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -415,6 +415,9 @@
>  #
>  # @x-ignore-shared: If enabled, QEMU will not migrate shared memory (since 4.0)
>  #
> +# @x-validate-uuid: Check whether the UUID is the same on both sides or not.
> +#                   (since 4.2)
> +#
>  # Since: 1.2
>  ##
>  { 'enum': 'MigrationCapability',
> @@ -422,7 +425,7 @@
>             'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram',
>             'block', 'return-path', 'pause-before-switchover', 'multifd',
>             'dirty-bitmaps', 'postcopy-blocktime', 'late-block-activate',
> -           'x-ignore-shared' ] }
> +           'x-ignore-shared', 'x-validate-uuid' ] }
>  
>  ##
>  # @MigrationCapabilityStatus:
> -- 
> 2.22.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 0/3] UUID validation during migration

[Daniel P. Berrangé]

On Tue, Sep 03, 2019 at 12:21:40PM +0100, Dr. David Alan Gilbert wrote:
> * Yury Kotov (yury-kotov@yandex-team.ru) wrote:
> > Hi,
> > 
> > This series adds an UUID validation at the start of the migration
> > on the target side. The idea is to identify the source of migration.
> > 
> > Possible case of problem:
> > 1. There are 3 servers: A, B and C
> > 2. Server A has a VM 1, server B has a VM 2
> > 3. VM 1 and VM 2 want to migrate to the server C
> > 4. Target of VM 1 starts on the server C and dies too quickly for some reason
> > 5. Target of VM 2 starts just after that and listen the same tcp port X, which
> >    the target of VM 1 wanted to use
> > 6. Source of VM 1 connects to the tcp port X, and migrates to VM 2 source
> 
> That shouldn't be possible in practice; you specify the destination tcp
> port when you start the destination qemu; so unless the management code
> that starts the migration is very broken it should know which port it's
> migrating to.
> However, if it is very broken then this is a good check.

In some, but not all, cases allowing the wrong source QEMU to connect to
a target QEMU could be considered a serious security flaw.

Historically live migration security was pretty minimal, to non-existant,
but we do now have the ability todo much better with TLS.

With the way libvirt currently uses TLS for migration, we're just protecting
against a rogue host connecting, as any host with a valid cert gets allowed.

We could do better by using the new ACLs feature to whitelist just the
particular virt host that we know the source VM is on.

This still allows for an accident if libvirt is migrating 2 VMs on the
same source host at the same time.

What's needed is a unique identity for each individual migration operation.

The use of the UUID here is aiming to serve that role.

Assuming libvirt has protected its TLS certificates on the source host,
then this solution is secure. An attacker would need to become root on
the source host to access libvirt's TLS certs, at which point no other
strategy libvirt used instead of UUID would be secure either.


So I think from a security POV, the use of UUID is acceptable.


An alternative would be to not use TLS certificates, and instead use the
TLS pre-shared  keys credential type, generating a new set of PSK creds
for each migration operation.  In this case, UUID would not be required
at all. I don't see this as a reason to reject the UUID check though.
It is reasonable for mgmt apps to have a choice in which approach they
pick.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 1/3] migration: Add x-validate-uuid capability

[Yury Kotov]

03.09.2019, 14:25, "Dr. David Alan Gilbert" <dgilbert@redhat.com>:
> * Eric Blake (eblake@redhat.com) wrote:
>>  On 8/27/19 10:36 AM, Yury Kotov wrote:
>>  > 27.08.2019, 17:02, "Eric Blake" <eblake@redhat.com>:
>>  >> On 8/27/19 7:02 AM, Yury Kotov wrote:
>>  >>>  This capability realizes simple source validation by UUID.
>>  >>>  It's useful for live migration between hosts.
>>  >>>
>>
>>  >>
>>  >> Any reason why this is marked experimental? It seems useful enough that
>>  >> we could probably just add it as a fully-supported feature (dropping the
>>  >> x- prefix) - but I'll leave that up to the migration maintainers.
>>  >>
>>  >
>>  > I thought that all new capabilities have x- prefix... May be it's really
>>  > unnecessary here, I'm not sure.
>>
>>  New features that need some testing or possible changes to behavior need
>>  x- to mark them as experimental, so we can make those changes without
>>  worrying about breaking back-compat. But new features that are outright
>>  useful and presumably in their final form, with no further
>>  experimentation needed, can skip going through an x- phase.
>>
>>  >
>>  >> In fact, do we even need this to be a tunable feature? Why not just
>>  >> always enable it? As long as the UUID is sent in a way that new->old
>>  >> doesn't break the old qemu from receiving the migration stream, and that
>>  >> old->new copes with UUID being absent, then new->new will always benefit
>>  >> from the additional safety check.
>>  >>
>>  >
>>  > In such case we couldn't migrate from e.g. 4.2 to 3.1
>>
>>  I don't know the migration format enough to know if there is a way for
>>  4.2 to unconditionally send a UUID as a subsection such that a receiving
>>  3.1 will ignore the unknown subsection. If so, then you don't need a
>>  knob; if not, then you need something to say whether sending the
>>  subsection is safe (perhaps default on in new machine types, but default
>>  off for machine types that might still be migrated back to 3.1). That's
>>  where I'm hoping the migration experts will chime in.
>
> Right; the migration format can't ignore chunks of data; so it does need
> to know somehow; the choice is either a capability or wiring it to the
> machine type; my preference is to wire it to the machine type; the
> arguments are:
>     a) Machine type
>        Pro: libvirt doesn't need to do anything
>        Con: It doesn't protect old machine types
>             It's not really part of the guest state
>
>     b) Capability
>        Pro: Works on all machine types
>        Con: Libvirt needs to enable it
>
> So, no strong preference but I think I prefer (a).

IIUC the (a) option requires to add a piece of code to every machine type.
This is much more complicated than adding a capability.
If you don't mind, I suggest to keep the current version.

>
> Dave
>
>>  --
>>  Eric Blake, Principal Software Engineer
>>  Red Hat, Inc. +1-919-301-3226
>>  Virtualization: qemu.org | libvirt.org
>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Regards,
Yury

Re: [Qemu-devel] [PATCH 1/3] migration: Add x-validate-uuid capability

[Dr. David Alan Gilbert]

* Yury Kotov (yury-kotov@yandex-team.ru) wrote:
> 03.09.2019, 14:25, "Dr. David Alan Gilbert" <dgilbert@redhat.com>:
> > * Eric Blake (eblake@redhat.com) wrote:
> >>  On 8/27/19 10:36 AM, Yury Kotov wrote:
> >>  > 27.08.2019, 17:02, "Eric Blake" <eblake@redhat.com>:
> >>  >> On 8/27/19 7:02 AM, Yury Kotov wrote:
> >>  >>>  This capability realizes simple source validation by UUID.
> >>  >>>  It's useful for live migration between hosts.
> >>  >>>
> >>
> >>  >>
> >>  >> Any reason why this is marked experimental? It seems useful enough that
> >>  >> we could probably just add it as a fully-supported feature (dropping the
> >>  >> x- prefix) - but I'll leave that up to the migration maintainers.
> >>  >>
> >>  >
> >>  > I thought that all new capabilities have x- prefix... May be it's really
> >>  > unnecessary here, I'm not sure.
> >>
> >>  New features that need some testing or possible changes to behavior need
> >>  x- to mark them as experimental, so we can make those changes without
> >>  worrying about breaking back-compat. But new features that are outright
> >>  useful and presumably in their final form, with no further
> >>  experimentation needed, can skip going through an x- phase.
> >>
> >>  >
> >>  >> In fact, do we even need this to be a tunable feature? Why not just
> >>  >> always enable it? As long as the UUID is sent in a way that new->old
> >>  >> doesn't break the old qemu from receiving the migration stream, and that
> >>  >> old->new copes with UUID being absent, then new->new will always benefit
> >>  >> from the additional safety check.
> >>  >>
> >>  >
> >>  > In such case we couldn't migrate from e.g. 4.2 to 3.1
> >>
> >>  I don't know the migration format enough to know if there is a way for
> >>  4.2 to unconditionally send a UUID as a subsection such that a receiving
> >>  3.1 will ignore the unknown subsection. If so, then you don't need a
> >>  knob; if not, then you need something to say whether sending the
> >>  subsection is safe (perhaps default on in new machine types, but default
> >>  off for machine types that might still be migrated back to 3.1). That's
> >>  where I'm hoping the migration experts will chime in.
> >
> > Right; the migration format can't ignore chunks of data; so it does need
> > to know somehow; the choice is either a capability or wiring it to the
> > machine type; my preference is to wire it to the machine type; the
> > arguments are:
> >     a) Machine type
> >        Pro: libvirt doesn't need to do anything
> >        Con: It doesn't protect old machine types
> >             It's not really part of the guest state
> >
> >     b) Capability
> >        Pro: Works on all machine types
> >        Con: Libvirt needs to enable it
> >
> > So, no strong preference but I think I prefer (a).
> 
> IIUC the (a) option requires to add a piece of code to every machine type.
> This is much more complicated than adding a capability.

Actually it doesn't - you just add a property, default it to true and
then add an entry in hw_compat_4_1 to turn it off for older types.

> If you don't mind, I suggest to keep the current version.

That's OK.

Dave

> >
> > Dave
> >
> >>  --
> >>  Eric Blake, Principal Software Engineer
> >>  Red Hat, Inc. +1-919-301-3226
> >>  Virtualization: qemu.org | libvirt.org
> >
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> Regards,
> Yury
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK