From af3bbbe98405fe2d274696abe5def679a3b0c673 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Tue, 3 Nov 2020 12:25:58 +0100 Subject: [PATCH 01/11] migration/ram: Fix hexadecimal format string specifier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The '%u' conversion specifier is for decimal notation. When prefixing a format with '0x', we want the hexadecimal specifier ('%x'). Inspired-by: Dov Murik Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Dr. David Alan Gilbert Message-Id: <20201103112558.2554390-5-philmd@redhat.com> Signed-off-by: Dr. David Alan Gilbert --- migration/ram.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/migration/ram.c b/migration/ram.c index add5396a62..7811cde643 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -3741,7 +3741,7 @@ int ram_dirty_bitmap_reload(MigrationState *s, RAMBlock *block) } if (end_mark != RAMBLOCK_RECV_BITMAP_ENDING) { - error_report("%s: ramblock '%s' end mark incorrect: 0x%"PRIu64, + error_report("%s: ramblock '%s' end mark incorrect: 0x%"PRIx64, __func__, block->idstr, end_mark); ret = -EINVAL; goto out; From 136fc6aa2cf38205fa3b47e155ebac11baccc789 Mon Sep 17 00:00:00 2001 From: Peng Liang Date: Thu, 12 Nov 2020 10:06:38 +0800 Subject: [PATCH 02/11] ACPI: Avoid infinite recursion when dump-vmstate There is a field with vmstate_ghes_state as vmsd in vmstate_ghes_state, which will lead to infinite recursion in dump_vmstate_vmsd. Fixes: a08a64627b ("ACPI: Record the Generic Error Status Block address") Reported-by: Euler Robot Signed-off-by: Peng Liang Acked-by: Igor Mammedov Message-Id: <20201112020638.874515-1-liangpeng10@huawei.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Dr. David Alan Gilbert --- hw/acpi/generic_event_device.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c index 6df400e1ee..5454be67d5 100644 --- a/hw/acpi/generic_event_device.c +++ b/hw/acpi/generic_event_device.c @@ -322,6 +322,16 @@ static const VMStateDescription vmstate_ged_state = { } }; +static const VMStateDescription vmstate_ghes = { + .name = "acpi-ghes", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT64(ghes_addr_le, AcpiGhesState), + VMSTATE_END_OF_LIST() + }, +}; + static bool ghes_needed(void *opaque) { AcpiGedState *s = opaque; @@ -335,7 +345,7 @@ static const VMStateDescription vmstate_ghes_state = { .needed = ghes_needed, .fields = (VMStateField[]) { VMSTATE_STRUCT(ghes_state, AcpiGedState, 1, - vmstate_ghes_state, AcpiGhesState), + vmstate_ghes, AcpiGhesState), VMSTATE_END_OF_LIST() } }; From a1af605bd5ade1a6dd571f553a6746b97f3d6869 Mon Sep 17 00:00:00 2001 From: Chuan Zheng Date: Fri, 6 Nov 2020 14:24:53 +0800 Subject: [PATCH 03/11] migration/multifd: fix hangup with TLS-Multifd due to blocking handshake MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The qemu main loop could hang up forever when we enable TLS+Multifd. The Src multifd_send_0 invokes tls handshake, it sends hello to sever and wait response. However, the Dst main qemu loop has been waiting recvmsg() for multifd_recv_1. Both of Src and Dst main qemu loop are blocking and waiting for reponse which results in hanging up forever. Src: (multifd_send_0) Dst: (multifd_recv_1) multifd_channel_connect migration_channel_process_incoming multifd_tls_channel_connect migration_tls_channel_process_incoming multifd_tls_channel_connect qio_channel_tls_handshake_task qio_channel_tls_handshake gnutls_handshake qio_channel_tls_handshake_task ... qcrypto_tls_session_handshake ... gnutls_handshake ... ... ... recvmsg (Blocking I/O waiting for response) recvmsg (Blocking I/O waiting for response) Fix this by offloadinig handshake work to a background thread. Reported-by: Yan Jin Suggested-by: Daniel P. Berrangé Signed-off-by: Chuan Zheng Message-Id: <1604643893-8223-1-git-send-email-zhengchuan@huawei.com> Reviewed-by: Daniel P. Berrangé Signed-off-by: Dr. David Alan Gilbert --- migration/multifd.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/migration/multifd.c b/migration/multifd.c index 68b171fb61..88486b90d6 100644 --- a/migration/multifd.c +++ b/migration/multifd.c @@ -739,6 +739,19 @@ static void multifd_tls_outgoing_handshake(QIOTask *task, multifd_channel_connect(p, ioc, err); } +static void *multifd_tls_handshake_thread(void *opaque) +{ + MultiFDSendParams *p = opaque; + QIOChannelTLS *tioc = QIO_CHANNEL_TLS(p->c); + + qio_channel_tls_handshake(tioc, + multifd_tls_outgoing_handshake, + p, + NULL, + NULL); + return NULL; +} + static void multifd_tls_channel_connect(MultiFDSendParams *p, QIOChannel *ioc, Error **errp) @@ -754,12 +767,10 @@ static void multifd_tls_channel_connect(MultiFDSendParams *p, trace_multifd_tls_outgoing_handshake_start(ioc, tioc, hostname); qio_channel_set_name(QIO_CHANNEL(tioc), "multifd-tls-outgoing"); - qio_channel_tls_handshake(tioc, - multifd_tls_outgoing_handshake, - p, - NULL, - NULL); - + p->c = QIO_CHANNEL(tioc); + qemu_thread_create(&p->thread, "multifd-tls-handshake-worker", + multifd_tls_handshake_thread, p, + QEMU_THREAD_JOINABLE); } static bool multifd_channel_connect(MultiFDSendParams *p, From a24292830b7a356f528760e065c0012ff56e18ab Mon Sep 17 00:00:00 2001 From: Chen Qun Date: Wed, 11 Nov 2020 22:22:03 +0800 Subject: [PATCH 04/11] migration: fix uninitialized variable warning in migrate_send_rp_req_pages() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After the WITH_QEMU_LOCK_GUARD macro is added, the compiler cannot identify that the statements in the macro must be executed. As a result, some variables assignment statements in the macro may be considered as unexecuted by the compiler. When the -Wmaybe-uninitialized capability is enabled on GCC9,the compiler showed warning: migration/migration.c: In function ‘migrate_send_rp_req_pages’: migration/migration.c:384:8: warning: ‘received’ may be used uninitialized in this function [-Wmaybe-uninitialized] 384 | if (received) { | ^ Add a default value for 'received' to prevented the warning. Reported-by: Euler Robot Signed-off-by: Chen Qun Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20201111142203.2359370-6-kuhn.chenqun@huawei.com> Signed-off-by: Dr. David Alan Gilbert --- migration/migration.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/migration/migration.c b/migration/migration.c index 3263aa55a9..f696e22fab 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -365,7 +365,7 @@ int migrate_send_rp_req_pages(MigrationIncomingState *mis, RAMBlock *rb, ram_addr_t start, uint64_t haddr) { void *aligned = (void *)(uintptr_t)(haddr & (-qemu_ram_pagesize(rb))); - bool received; + bool received = false; WITH_QEMU_LOCK_GUARD(&mis->page_request_mutex) { received = ramblock_recv_bitmap_test_byte_offset(rb, start); From a18ed79b19ec63368bf825eaf708e31f49888d40 Mon Sep 17 00:00:00 2001 From: Chuan Zheng Date: Fri, 30 Oct 2020 11:58:01 +0800 Subject: [PATCH 05/11] migration/dirtyrate: simplify includes in dirtyrate.c Remove redundant blank line which is left by Commit 662770af7c6e8c, also take this opportunity to remove redundant includes in dirtyrate.c. Signed-off-by: Chuan Zheng Message-Id: <1604030281-112946-1-git-send-email-zhengchuan@huawei.com> Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Dr. David Alan Gilbert --- migration/dirtyrate.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c index 8f728d2600..ccb98147e8 100644 --- a/migration/dirtyrate.c +++ b/migration/dirtyrate.c @@ -11,17 +11,12 @@ */ #include "qemu/osdep.h" - #include #include "qapi/error.h" #include "cpu.h" -#include "qemu/config-file.h" -#include "exec/memory.h" #include "exec/ramblock.h" -#include "exec/target_page.h" #include "qemu/rcu_queue.h" #include "qapi/qapi-commands-migration.h" -#include "migration.h" #include "ram.h" #include "trace.h" #include "dirtyrate.h" From 9e8424088c5648959e4c5d715290e6cfa96df087 Mon Sep 17 00:00:00 2001 From: Chuan Zheng Date: Wed, 11 Nov 2020 22:26:03 +0800 Subject: [PATCH 06/11] multifd/tls: fix memoryleak of the QIOChannelSocket object when cancelling migration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When creating new tls client, the tioc->master will be referenced which results in socket leaking after multifd_save_cleanup if we cancel migration. Fix it by do object_unref() after tls client creation. Suggested-by: Daniel P. Berrangé Signed-off-by: Chuan Zheng Message-Id: <1605104763-118687-1-git-send-email-zhengchuan@huawei.com> Reviewed-by: Daniel P. Berrangé Signed-off-by: Dr. David Alan Gilbert --- migration/multifd.c | 1 + 1 file changed, 1 insertion(+) diff --git a/migration/multifd.c b/migration/multifd.c index 88486b90d6..45c690aa11 100644 --- a/migration/multifd.c +++ b/migration/multifd.c @@ -765,6 +765,7 @@ static void multifd_tls_channel_connect(MultiFDSendParams *p, return; } + object_unref(OBJECT(ioc)); trace_multifd_tls_outgoing_handshake_start(ioc, tioc, hostname); qio_channel_set_name(QIO_CHANNEL(tioc), "multifd-tls-outgoing"); p->c = QIO_CHANNEL(tioc); From 6ba11211bd616237d028fb5d27f8576fc8cf7b1c Mon Sep 17 00:00:00 2001 From: "Longpeng(Mike)" Date: Thu, 5 Nov 2020 17:17:26 +0800 Subject: [PATCH 07/11] migration: handle CANCELLING state in migration_completion() The following sequence may cause the VM abort during migration: 1. RUN_STATE_RUNNING,MIGRATION_STATUS_ACTIVE 2. before call migration_completion(), we send migrate_cancel QMP command, the state machine is changed to: RUN_STATE_RUNNING,MIGRATION_STATUS_CANCELLING 3. call migration_completion(), and the state machine is switch to: RUN_STATE_RUNNING,MIGRATION_STATUS_COMPLETED 4. call migration_iteration_finish(), because the migration status is COMPLETED, so it will try to set the runstate to POSTMIGRATE, but RUNNING-->POSTMIGRATE is an invalid transition, so abort(). The migration_completion() should not change the migration state to COMPLETED if it is already changed to CANCELLING. Signed-off-by: Longpeng(Mike) Message-Id: <20201105091726.148-1-longpeng2@huawei.com> Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Dr. David Alan Gilbert --- migration/migration.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/migration/migration.c b/migration/migration.c index f696e22fab..87a9b59f83 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -3061,6 +3061,8 @@ static void migration_completion(MigrationState *s) qemu_savevm_state_complete_postcopy(s->to_dst_file); trace_migration_completion_postcopy_end_after_complete(); + } else if (s->state == MIGRATION_STATUS_CANCELLING) { + goto fail; } /* From f26688a911ed4bc122f597333c9d5b45175e683c Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Tue, 3 Nov 2020 17:41:35 +0100 Subject: [PATCH 08/11] virtiofsd: Announce submounts even without statx() Contrary to what the check (and warning) in lo_init() claims, we can announce submounts just fine even without statx() -- the check is based on comparing both the mount ID and st_dev of parent and child. Without statx(), we will not have the mount ID; but we always have st_dev. The only problems we have (without statx() and its mount ID) are: (1) Mounting the same device twice may lead to both trees being treated as exactly the same tree by virtiofsd. But that is a problem that is completely independent of mirroring host submounts in the guest. Both submount roots will still show the FUSE_SUBMOUNT flag, because their st_dev still differs from their respective parent. (2) There is only one exception to (1), and that is if you mount a device inside a mount of itself: Then, its st_dev will be the same as that of its parent, and so without a mount ID, virtiofsd will not be able to recognize the nested mount's root as a submount. However, thanks to virtiofsd then treating both trees as exactly the same tree, it will be caught up in a loop when the guest tries to examine the nested submount, so the guest will always see nothing but an ELOOP there. Therefore, this case is just fully broken without statx(), whether we check for submounts (based on st_dev) or not. All in all, checking for submounts works well even without comparing the mount ID (i.e., without statx()). The only concern is an edge case that, without statx() mount IDs, is utterly broken anyway. Thus, drop said check in lo_init(). Reported-by: Miklos Szeredi Signed-off-by: Max Reitz Message-Id: <20201103164135.169325-1-mreitz@redhat.com> Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Dr. David Alan Gilbert --- tools/virtiofsd/passthrough_ll.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c index ec1008bceb..6c64b03f1a 100644 --- a/tools/virtiofsd/passthrough_ll.c +++ b/tools/virtiofsd/passthrough_ll.c @@ -610,14 +610,6 @@ static void lo_init(void *userdata, struct fuse_conn_info *conn) "does not support it\n"); lo->announce_submounts = false; } - -#ifndef CONFIG_STATX - if (lo->announce_submounts) { - fuse_log(FUSE_LOG_WARNING, "lo_init: Cannot announce submounts, there " - "is no statx()\n"); - lo->announce_submounts = false; - } -#endif } static void lo_getattr(fuse_req_t req, fuse_ino_t ino, From 7fa87944f82d75d21b7166570ac87d7874c151d5 Mon Sep 17 00:00:00 2001 From: Haotian Li Date: Wed, 11 Nov 2020 09:05:56 +0800 Subject: [PATCH 09/11] tools/virtiofsd/buffer.c: check whether buf is NULL in fuse_bufvec_advance func In fuse_bufvec_advance func, calling fuse_bufvec_current func may return NULL, so we should check whether buf is NULL before using it. Signed-off-by: Haotian Li Signed-off-by: Zhiqiang Liu Message-Id: <29fc87c2-b87c-4c34-40d4-75381f228849@huawei.com> Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Dr. David Alan Gilbert --- tools/virtiofsd/buffer.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tools/virtiofsd/buffer.c b/tools/virtiofsd/buffer.c index 27c1377f22..bdc608c221 100644 --- a/tools/virtiofsd/buffer.c +++ b/tools/virtiofsd/buffer.c @@ -246,6 +246,10 @@ static int fuse_bufvec_advance(struct fuse_bufvec *bufv, size_t len) { const struct fuse_buf *buf = fuse_bufvec_current(bufv); + if (!buf) { + return 0; + } + bufv->off += len; assert(bufv->off <= buf->size); if (bufv->off == buf->size) { From db2e026a39d9871217289e5ed5cb97a2b7f476e5 Mon Sep 17 00:00:00 2001 From: Haotian Li Date: Wed, 11 Nov 2020 09:09:12 +0800 Subject: [PATCH 10/11] virtiofsd: check whether lo_map_reserve returns NULL in, main func In main func, func lo_map_reserve is called without NULL check. If reallocing new_elems fails in func lo_map_grow, the func lo_map_reserve may return NULL. We should check whether lo_map_reserve returns NULL before using it. Signed-off-by: Haotian Li Signed-off-by: Zhiqiang Liu Message-Id: <48887813-1c95-048c-6d10-48e3dd2bac71@huawei.com> Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Dr. David Alan Gilbert --- tools/virtiofsd/passthrough_ll.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c index 6c64b03f1a..9545a0d174 100644 --- a/tools/virtiofsd/passthrough_ll.c +++ b/tools/virtiofsd/passthrough_ll.c @@ -3425,6 +3425,7 @@ int main(int argc, char *argv[]) .proc_self_fd = -1, }; struct lo_map_elem *root_elem; + struct lo_map_elem *reserve_elem; int ret = -1; /* Don't mask creation mode, kernel already did that */ @@ -3444,8 +3445,17 @@ int main(int argc, char *argv[]) * [1] Root inode */ lo_map_init(&lo.ino_map); - lo_map_reserve(&lo.ino_map, 0)->in_use = false; + reserve_elem = lo_map_reserve(&lo.ino_map, 0); + if (!reserve_elem) { + fuse_log(FUSE_LOG_ERR, "failed to alloc reserve_elem.\n"); + goto err_out1; + } + reserve_elem->in_use = false; root_elem = lo_map_reserve(&lo.ino_map, lo.root.fuse_ino); + if (!root_elem) { + fuse_log(FUSE_LOG_ERR, "failed to alloc root_elem.\n"); + goto err_out1; + } root_elem->inode = &lo.root; lo_map_init(&lo.dirp_map); From 7632b56c8f880a8f86cf049a3785069e1ffd2997 Mon Sep 17 00:00:00 2001 From: Haotian Li Date: Wed, 11 Nov 2020 09:10:38 +0800 Subject: [PATCH 11/11] virtiofsd: check whether strdup lo.source return NULL in main func In main func, strdup lo.source may fail. So check whether strdup lo.source return NULL before using it. Signed-off-by: Haotian Li Signed-off-by: Zhiqiang Liu Message-Id: Reviewed-by: Dr. David Alan Gilbert Signed-off-by: Dr. David Alan Gilbert --- tools/virtiofsd/passthrough_ll.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c index 9545a0d174..97485b22b4 100644 --- a/tools/virtiofsd/passthrough_ll.c +++ b/tools/virtiofsd/passthrough_ll.c @@ -3517,6 +3517,10 @@ int main(int argc, char *argv[]) } } else { lo.source = strdup("/"); + if (!lo.source) { + fuse_log(FUSE_LOG_ERR, "failed to strdup source\n"); + goto err_out1; + } } if (lo.xattrmap) {