From a5d35dc7e06896a33216610cc86527258735b0e3 Mon Sep 17 00:00:00 2001 From: Peter Xu Date: Tue, 14 Mar 2023 16:54:29 -0400 Subject: [PATCH 1/7] migration: Wait on preempt channel in preempt thread MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit QEMU main thread will wait until dest preempt channel established during processing the LISTEN command (within the whole postcopy PACKAGED data), by waiting on the semaphore postcopy_qemufile_dst_done. That's racy, because it's possible that the dest QEMU main thread hasn't yet accept()ed the new connection when processing the LISTEN event. The sem_wait() will yield the main thread without being able to run anything else including the accept() of the new socket, which can cause deadlock within the main thread. To avoid the race, move the "wait channel" from main thread to the preempt thread right at the start. Reported-by: Peter Maydell Fixes: 5655aab079 ("migration: Postpone postcopy preempt channel to be after main") Reviewed-by: Daniel P. Berrangé Reviewed-by: Juan Quintela Signed-off-by: Peter Xu Signed-off-by: Juan Quintela --- migration/postcopy-ram.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c index f54f44d899..41c0713650 100644 --- a/migration/postcopy-ram.c +++ b/migration/postcopy-ram.c @@ -1197,11 +1197,6 @@ int postcopy_ram_incoming_setup(MigrationIncomingState *mis) } if (migrate_postcopy_preempt()) { - /* - * The preempt channel is established in asynchronous way. Wait - * for its completion. - */ - qemu_sem_wait(&mis->postcopy_qemufile_dst_done); /* * This thread needs to be created after the temp pages because * it'll fetch RAM_CHANNEL_POSTCOPY PostcopyTmpPage immediately. @@ -1668,6 +1663,12 @@ void *postcopy_preempt_thread(void *opaque) qemu_sem_post(&mis->thread_sync_sem); + /* + * The preempt channel is established in asynchronous way. Wait + * for its completion. + */ + qemu_sem_wait(&mis->postcopy_qemufile_dst_done); + /* Sending RAM_SAVE_FLAG_EOS to terminate this thread */ qemu_mutex_lock(&mis->postcopy_prio_thread_mutex); while (1) { From a5382214d8b099bb2fe60b206097567762861800 Mon Sep 17 00:00:00 2001 From: "Dr. David Alan Gilbert" Date: Tue, 14 Mar 2023 17:15:58 +0000 Subject: [PATCH 2/7] migration/rdma: Fix return-path case The RDMA code has return-path handling code, but it's only enabled if postcopy is enabled; if the 'return-path' migration capability is enabled, the return path is NOT setup but the core migration code still tries to use it and breaks. Enable the RDMA return path if either postcopy or the return-path capability is enabled. bz: https://bugzilla.redhat.com/show_bug.cgi?id=2063615 Signed-off-by: Dr. David Alan Gilbert Reviewed-by: Juan Quintela Reviewed-by: Li Zhijian Signed-off-by: Juan Quintela --- migration/rdma.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/migration/rdma.c b/migration/rdma.c index 288eadc2d2..9d70e9885b 100644 --- a/migration/rdma.c +++ b/migration/rdma.c @@ -3373,7 +3373,8 @@ static int qemu_rdma_accept(RDMAContext *rdma) * initialize the RDMAContext for return path for postcopy after first * connection request reached. */ - if (migrate_postcopy() && !rdma->is_return_path) { + if ((migrate_postcopy() || migrate_use_return_path()) + && !rdma->is_return_path) { rdma_return_path = qemu_rdma_data_init(rdma->host_port, NULL); if (rdma_return_path == NULL) { rdma_ack_cm_event(cm_event); @@ -3455,7 +3456,8 @@ static int qemu_rdma_accept(RDMAContext *rdma) } /* Accept the second connection request for return path */ - if (migrate_postcopy() && !rdma->is_return_path) { + if ((migrate_postcopy() || migrate_use_return_path()) + && !rdma->is_return_path) { qemu_set_fd_handler(rdma->channel->fd, rdma_accept_incoming_migration, NULL, (void *)(intptr_t)rdma->return_path); @@ -4192,7 +4194,7 @@ void rdma_start_outgoing_migration(void *opaque, } /* RDMA postcopy need a separate queue pair for return path */ - if (migrate_postcopy()) { + if (migrate_postcopy() || migrate_use_return_path()) { rdma_return_path = qemu_rdma_data_init(host_port, errp); if (rdma_return_path == NULL) { From d84a78d15d3af9ff28ceec6906a4b101bd545b55 Mon Sep 17 00:00:00 2001 From: Matheus Tavares Bernardino Date: Mon, 13 Mar 2023 15:58:19 -0300 Subject: [PATCH 3/7] migration/xbzrle: use ctz64 to avoid undefined result __builtin_ctzll() produces undefined results when the argument is 0. This can be seen through test-xbzrle, which produces the following warning: ../migration/xbzrle.c:265: runtime error: passing zero to ctz(), which is not a valid argument Replace __builtin_ctzll() with our ctz64() wrapper which properly handles 0. Signed-off-by: Matheus Tavares Bernardino Reviewed-by: Dr. David Alan Gilbert Reviewed-by: Juan Quintela Signed-off-by: Juan Quintela --- migration/xbzrle.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/migration/xbzrle.c b/migration/xbzrle.c index 05366e86c0..21b92d4eae 100644 --- a/migration/xbzrle.c +++ b/migration/xbzrle.c @@ -12,6 +12,7 @@ */ #include "qemu/osdep.h" #include "qemu/cutils.h" +#include "qemu/host-utils.h" #include "xbzrle.h" /* @@ -233,7 +234,7 @@ int xbzrle_encode_buffer_avx512(uint8_t *old_buf, uint8_t *new_buf, int slen, break; } never_same = false; - num = __builtin_ctzll(~comp); + num = ctz64(~comp); num = (num < bytes_to_check) ? num : bytes_to_check; zrun_len += num; bytes_to_check -= num; @@ -262,7 +263,7 @@ int xbzrle_encode_buffer_avx512(uint8_t *old_buf, uint8_t *new_buf, int slen, nzrun_len += 64; break; } - num = __builtin_ctzll(comp); + num = ctz64(comp); num = (num < bytes_to_check) ? num : bytes_to_check; nzrun_len += num; bytes_to_check -= num; From 1776b70f55c75541e9cab3423650a59b085162a9 Mon Sep 17 00:00:00 2001 From: Matheus Tavares Bernardino Date: Mon, 13 Mar 2023 15:58:20 -0300 Subject: [PATCH 4/7] migration/xbzrle: fix out-of-bounds write with axv512 xbzrle_encode_buffer_avx512() checks for overflows too scarcely in its outer loop, causing out-of-bounds writes: $ ../configure --target-list=aarch64-softmmu --enable-sanitizers --enable-avx512bw $ make tests/unit/test-xbzrle && ./tests/unit/test-xbzrle ==5518==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x62100000b100 at pc 0x561109a7714d bp 0x7ffed712a440 sp 0x7ffed712a430 WRITE of size 1 at 0x62100000b100 thread T0 #0 0x561109a7714c in uleb128_encode_small ../util/cutils.c:831 #1 0x561109b67f6a in xbzrle_encode_buffer_avx512 ../migration/xbzrle.c:275 #2 0x5611099a7428 in test_encode_decode_overflow ../tests/unit/test-xbzrle.c:153 #3 0x7fb2fb65a58d (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7a58d) #4 0x7fb2fb65a333 (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7a333) #5 0x7fb2fb65aa79 in g_test_run_suite (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7aa79) #6 0x7fb2fb65aa94 in g_test_run (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7aa94) #7 0x5611099a3a23 in main ../tests/unit/test-xbzrle.c:218 #8 0x7fb2fa78c082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082) #9 0x5611099a608d in _start (/qemu/build/tests/unit/test-xbzrle+0x28408d) 0x62100000b100 is located 0 bytes to the right of 4096-byte region [0x62100000a100,0x62100000b100) allocated by thread T0 here: #0 0x7fb2fb823a06 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:153 #1 0x7fb2fb637ef0 in g_malloc0 (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x57ef0) Fix that by performing the overflow check in the inner loop, instead. Signed-off-by: Matheus Tavares Bernardino Reviewed-by: Dr. David Alan Gilbert Reviewed-by: Juan Quintela Signed-off-by: Juan Quintela --- migration/xbzrle.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/migration/xbzrle.c b/migration/xbzrle.c index 21b92d4eae..c6f8b20917 100644 --- a/migration/xbzrle.c +++ b/migration/xbzrle.c @@ -197,10 +197,6 @@ int xbzrle_encode_buffer_avx512(uint8_t *old_buf, uint8_t *new_buf, int slen, __m512i r = _mm512_set1_epi32(0); while (count512s) { - if (d + 2 > dlen) { - return -1; - } - int bytes_to_check = 64; uint64_t mask = 0xffffffffffffffff; if (count512s == 1) { @@ -216,6 +212,9 @@ int xbzrle_encode_buffer_avx512(uint8_t *old_buf, uint8_t *new_buf, int slen, bool is_same = (comp & 0x1); while (bytes_to_check) { + if (d + 2 > dlen) { + return -1; + } if (is_same) { if (nzrun_len) { d += uleb128_encode_small(dst + d, nzrun_len); From bf0274192a6c1aac67ca6ee6a96b5f295cfc8d0a Mon Sep 17 00:00:00 2001 From: Li Zhijian Date: Thu, 16 Mar 2023 10:55:00 +0100 Subject: [PATCH 5/7] migration/rdma: Remove deprecated variable rdma_return_path It's no longer needed since commit 44bcfd45e98 ("migration/rdma: destination: create the return patch after the first accept") Signed-off-by: Li Zhijian Reviewed-by: Juan Quintela Signed-off-by: Juan Quintela --- migration/rdma.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/migration/rdma.c b/migration/rdma.c index 9d70e9885b..df646be35e 100644 --- a/migration/rdma.c +++ b/migration/rdma.c @@ -4111,7 +4111,7 @@ static void rdma_accept_incoming_migration(void *opaque) void rdma_start_incoming_migration(const char *host_port, Error **errp) { int ret; - RDMAContext *rdma, *rdma_return_path = NULL; + RDMAContext *rdma; Error *local_err = NULL; trace_rdma_start_incoming_migration(); @@ -4157,7 +4157,6 @@ err: g_free(rdma->host_port); } g_free(rdma); - g_free(rdma_return_path); } void rdma_start_outgoing_migration(void *opaque, From ff1585d1d8ae5b15d88327e59859d352829caebb Mon Sep 17 00:00:00 2001 From: Wei Wang Date: Thu, 9 Mar 2023 18:29:56 +0800 Subject: [PATCH 6/7] migration/multifd: correct multifd_send_thread to trace the flags The p->flags could be updated via the send_prepare callback, e.g. OR-ed with MULTIFD_FLAG_ZLIB via zlib_send_prepare. Assign p->flags to the local "flags" before the send_prepare callback could only get partial of p->flags. Fix it by moving the assignment of p->flags to the local flags after the callback, so that the correct flags can be traced. Fixes: ab7cbb0b9a3b ("multifd: Make no compression operations into its own structure") Signed-off-by: Wei Wang Reviewed-by: Juan Quintela Signed-off-by: Juan Quintela --- migration/multifd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/migration/multifd.c b/migration/multifd.c index 5e85c3ea9b..cbc0dfe39b 100644 --- a/migration/multifd.c +++ b/migration/multifd.c @@ -677,7 +677,7 @@ static void *multifd_send_thread(void *opaque) if (p->pending_job) { uint64_t packet_num = p->packet_num; - uint32_t flags = p->flags; + uint32_t flags; p->normal_num = 0; if (use_zero_copy_send) { @@ -699,6 +699,7 @@ static void *multifd_send_thread(void *opaque) } } multifd_send_fill_packet(p); + flags = p->flags; p->flags = 0; p->num_packets++; p->total_normal_pages += p->normal_num; From fa76c854ae837328187bef41d80af5d1ad36681f Mon Sep 17 00:00:00 2001 From: Steve Sistare Date: Tue, 7 Feb 2023 10:47:03 -0800 Subject: [PATCH 7/7] migration: fix populate_vfio_info MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Include CONFIG_DEVICES so that populate_vfio_info is instantiated for CONFIG_VFIO. Without it, the 'info migrate' command never returns info about vfio. Fixes: 43bd0bf30f ("migration: Move populate_vfio_info() into a separate file") Signed-off-by: Steve Sistare Reviewed-by: Marc-André Lureau Reviewed-by: Thomas Huth Reviewed-by: Dr. David Alan Gilbert Reviewed-by: Juan Quintela Signed-off-by: Juan Quintela --- migration/target.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/migration/target.c b/migration/target.c index 907ebf0a0a..00ca007f97 100644 --- a/migration/target.c +++ b/migration/target.c @@ -8,6 +8,7 @@ #include "qemu/osdep.h" #include "qapi/qapi-types-migration.h" #include "migration.h" +#include CONFIG_DEVICES #ifdef CONFIG_VFIO #include "hw/vfio/vfio-common.h" @@ -17,7 +18,6 @@ void populate_vfio_info(MigrationInfo *info) { #ifdef CONFIG_VFIO if (vfio_mig_active()) { - info->has_vfio = true; info->vfio = g_malloc0(sizeof(*info->vfio)); info->vfio->transferred = vfio_mig_bytes_transferred(); }