[RFC PATCH v2 0/4] migration: Fix multifd qemu_mutex_destroy race

[RFC PATCH v2 0/4] migration: Fix multifd qemu_mutex_destroy race

[Fabiano Rosas]

changes:
- dropped the Error patch
- removed p->running
- joined the TLS thread

v1:
https://lore.kernel.org/r/20231109165856.15224-1-farosas@suse.de

We're calling qemu_sem_post() in threads other than the multifd
channel and the migration thread. This is vulnerable to a race with
multifd_save_cleanup() which calls qemu_sem_destroy(). If we attempt
to destroy the semaphore mutex with the lock taken, the code asserts.

We're hitting this in the current master and we've had reports of this
in the past already:

[PATCH] migrate/multifd: fix coredump when the multifd thread cleanup
https://lore.kernel.org/r/20230621081826.3203053-1-zhangjianguo18@huawei.com

Fabiano Rosas (4):
  migration/multifd: Stop setting p->ioc before connecting
  migration/multifd: Join the TLS thread
  migration/multifd: Remove p->running
  migration/multifd: Move semaphore release into main thread

 migration/migration.c |  4 +-
 migration/multifd.c   | 87 +++++++++++++++++++++++--------------------
 migration/multifd.h   |  9 ++---
 3 files changed, 53 insertions(+), 47 deletions(-)

-- 
2.35.3

[RFC PATCH v2 1/4] migration/multifd: Stop setting p->ioc before connecting

[Fabiano Rosas]

This is being shadowed but the assignments at
multifd_channel_connect() and multifd_tls_channel_connect() .

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/multifd.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index ec58c58082..409460684f 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -883,8 +883,7 @@ static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
 
     trace_multifd_new_send_channel_async(p->id);
     if (!qio_task_propagate_error(task, &local_err)) {
-        p->c = ioc;
-        qio_channel_set_delay(p->c, false);
+        qio_channel_set_delay(ioc, false);
         p->running = true;
         if (multifd_channel_connect(p, ioc, &local_err)) {
             return;
-- 
2.35.3

[RFC PATCH v2 2/4] migration/multifd: Join the TLS thread

[Fabiano Rosas]

We're currently leaking the resources of the TLS thread by not joining
it and also overwriting the p->thread pointer altogether.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/multifd.c | 10 +++++++++-
 migration/multifd.h |  1 +
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index 409460684f..d632dbc095 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -525,6 +525,10 @@ void multifd_save_cleanup(void)
     for (i = 0; i < migrate_multifd_channels(); i++) {
         MultiFDSendParams *p = &multifd_send_state->params[i];
 
+        if (p->tls_thread) {
+            qemu_thread_join(p->tls_thread);
+        }
+
         if (p->running) {
             qemu_thread_join(&p->thread);
         }
@@ -552,6 +556,8 @@ void multifd_save_cleanup(void)
         p->iov = NULL;
         g_free(p->normal);
         p->normal = NULL;
+        g_free(p->tls_thread);
+        p->tls_thread = NULL;
         multifd_send_state->ops->send_cleanup(p, &local_err);
         if (local_err) {
             migrate_set_error(migrate_get_current(), local_err);
@@ -826,7 +832,9 @@ static bool 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");
     p->c = QIO_CHANNEL(tioc);
-    qemu_thread_create(&p->thread, "multifd-tls-handshake-worker",
+
+    p->tls_thread = g_new0(QemuThread, 1);
+    qemu_thread_create(p->tls_thread, "multifd-tls-handshake-worker",
                        multifd_tls_handshake_thread, p,
                        QEMU_THREAD_JOINABLE);
     return true;
diff --git a/migration/multifd.h b/migration/multifd.h
index a835643b48..4ff78e9863 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -75,6 +75,7 @@ typedef struct {
     char *name;
     /* channel thread id */
     QemuThread thread;
+    QemuThread *tls_thread;
     /* communication channel */
     QIOChannel *c;
     /* is the yank function registered */
-- 
2.35.3

[RFC PATCH v2 3/4] migration/multifd: Remove p->running

[Fabiano Rosas]

We currently only need p->running to avoid calling qemu_thread_join()
on a non existent thread if the thread has never been created. We
could turn the QemuThread into a pointer and check for NULL instead
and get rid of the p->running flag. Testing the pointer directly is
more precise and less prone to misuse.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/multifd.c | 41 ++++++++++++++++++++---------------------
 migration/multifd.h |  8 ++------
 2 files changed, 22 insertions(+), 27 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index d632dbc095..639505dd10 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -529,8 +529,8 @@ void multifd_save_cleanup(void)
             qemu_thread_join(p->tls_thread);
         }
 
-        if (p->running) {
-            qemu_thread_join(&p->thread);
+        if (p->thread) {
+            qemu_thread_join(p->thread);
         }
     }
     for (i = 0; i < migrate_multifd_channels(); i++) {
@@ -558,6 +558,8 @@ void multifd_save_cleanup(void)
         p->normal = NULL;
         g_free(p->tls_thread);
         p->tls_thread = NULL;
+        g_free(p->thread);
+        p->thread = NULL;
         multifd_send_state->ops->send_cleanup(p, &local_err);
         if (local_err) {
             migrate_set_error(migrate_get_current(), local_err);
@@ -762,10 +764,6 @@ out:
         error_free(local_err);
     }
 
-    qemu_mutex_lock(&p->mutex);
-    p->running = false;
-    qemu_mutex_unlock(&p->mutex);
-
     rcu_unregister_thread();
     migration_threads_remove(thread);
     trace_multifd_send_thread_end(p->id, p->num_packets, p->total_normal_pages);
@@ -860,7 +858,9 @@ static bool multifd_channel_connect(MultiFDSendParams *p,
         migration_ioc_register_yank(ioc);
         p->registered_yank = true;
         p->c = ioc;
-        qemu_thread_create(&p->thread, p->name, multifd_send_thread, p,
+
+        p->thread = g_new0(QemuThread, 1);
+        qemu_thread_create(p->thread, p->name, multifd_send_thread, p,
                            QEMU_THREAD_JOINABLE);
     }
     return true;
@@ -892,7 +892,6 @@ static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
     trace_multifd_new_send_channel_async(p->id);
     if (!qio_task_propagate_error(task, &local_err)) {
         qio_channel_set_delay(ioc, false);
-        p->running = true;
         if (multifd_channel_connect(p, ioc, &local_err)) {
             return;
         }
@@ -1034,15 +1033,15 @@ void multifd_load_cleanup(void)
     for (i = 0; i < migrate_multifd_channels(); i++) {
         MultiFDRecvParams *p = &multifd_recv_state->params[i];
 
-        if (p->running) {
-            /*
-             * multifd_recv_thread may hung at MULTIFD_FLAG_SYNC handle code,
-             * however try to wakeup it without harm in cleanup phase.
-             */
-            qemu_sem_post(&p->sem_sync);
-        }
+        /*
+         * multifd_recv_thread may hung at MULTIFD_FLAG_SYNC handle code,
+         * however try to wakeup it without harm in cleanup phase.
+         */
+        qemu_sem_post(&p->sem_sync);
 
-        qemu_thread_join(&p->thread);
+        if (p->thread) {
+            qemu_thread_join(p->thread);
+        }
     }
     for (i = 0; i < migrate_multifd_channels(); i++) {
         MultiFDRecvParams *p = &multifd_recv_state->params[i];
@@ -1061,6 +1060,8 @@ void multifd_load_cleanup(void)
         p->iov = NULL;
         g_free(p->normal);
         p->normal = NULL;
+        g_free(p->thread);
+        p->thread = NULL;
         multifd_recv_state->ops->recv_cleanup(p);
     }
     qemu_sem_destroy(&multifd_recv_state->sem_sync);
@@ -1152,9 +1153,6 @@ static void *multifd_recv_thread(void *opaque)
         multifd_recv_terminate_threads(local_err);
         error_free(local_err);
     }
-    qemu_mutex_lock(&p->mutex);
-    p->running = false;
-    qemu_mutex_unlock(&p->mutex);
 
     rcu_unregister_thread();
     trace_multifd_recv_thread_end(p->id, p->num_packets, p->total_normal_pages);
@@ -1198,6 +1196,7 @@ int multifd_load_setup(Error **errp)
         p->normal = g_new0(ram_addr_t, page_count);
         p->page_count = page_count;
         p->page_size = qemu_target_page_size();
+        p->thread = g_new0(QemuThread, 1);
     }
 
     for (i = 0; i < thread_count; i++) {
@@ -1264,8 +1263,8 @@ void multifd_recv_new_channel(QIOChannel *ioc, Error **errp)
     /* initial packet */
     p->num_packets = 1;
 
-    p->running = true;
-    qemu_thread_create(&p->thread, p->name, multifd_recv_thread, p,
+    p->thread = g_new0(QemuThread, 1);
+    qemu_thread_create(p->thread, p->name, multifd_recv_thread, p,
                        QEMU_THREAD_JOINABLE);
     qatomic_inc(&multifd_recv_state->count);
 }
diff --git a/migration/multifd.h b/migration/multifd.h
index 4ff78e9863..d21edaeaae 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -74,7 +74,7 @@ typedef struct {
     /* channel thread name */
     char *name;
     /* channel thread id */
-    QemuThread thread;
+    QemuThread *thread;
     QemuThread *tls_thread;
     /* communication channel */
     QIOChannel *c;
@@ -96,8 +96,6 @@ typedef struct {
 
     /* this mutex protects the following parameters */
     QemuMutex mutex;
-    /* is this channel thread running */
-    bool running;
     /* should this thread finish */
     bool quit;
     /* multifd flags for each packet */
@@ -144,7 +142,7 @@ typedef struct {
     /* channel thread name */
     char *name;
     /* channel thread id */
-    QemuThread thread;
+    QemuThread *thread;
     /* communication channel */
     QIOChannel *c;
     /* packet allocated len */
@@ -159,8 +157,6 @@ typedef struct {
 
     /* this mutex protects the following parameters */
     QemuMutex mutex;
-    /* is this channel thread running */
-    bool running;
     /* should this thread finish */
     bool quit;
     /* multifd flags for each packet */
-- 
2.35.3

[RFC PATCH v2 4/4] migration/multifd: Move semaphore release into main thread

[Fabiano Rosas]

We cannot operate on the multifd semaphores outside of the multifd
channel thread because multifd_save_cleanup() can run in parallel and
attempt to destroy the mutexes, which causes an assert.

Looking at the places where we use the semaphores aside from the
migration thread, there's only the TLS handshake thread and the
initial multifd_channel_connect() in the main thread. These are places
where creating the multifd channel cannot fail, so releasing the
semaphores at these places only serves the purpose of avoiding a
deadlock when an error happens before the channel(s) have started, but
after the migration thread has already called
multifd_send_sync_main().

Instead of attempting to release the semaphores at those places, move
the release into multifd_save_cleanup(). This puts the semaphore usage
in the same thread that does the cleanup, eliminating the race.

Move the call to multifd_save_cleanup() before joining the migration
thread so we release the semaphores before and avoid deadlock.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration.c |  4 +++-
 migration/multifd.c   | 33 +++++++++++++++++----------------
 2 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 28a34c9068..5a07f5318d 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1293,6 +1293,9 @@ static void migrate_fd_cleanup(MigrationState *s)
         QEMUFile *tmp;
 
         trace_migrate_fd_cleanup();
+
+        multifd_save_cleanup();
+
         qemu_mutex_unlock_iothread();
         if (s->migration_thread_running) {
             qemu_thread_join(&s->thread);
@@ -1300,7 +1303,6 @@ static void migrate_fd_cleanup(MigrationState *s)
         }
         qemu_mutex_lock_iothread();
 
-        multifd_save_cleanup();
         qemu_mutex_lock(&s->qemu_file_lock);
         tmp = s->to_dst_file;
         s->to_dst_file = NULL;
diff --git a/migration/multifd.c b/migration/multifd.c
index 639505dd10..a66be2d4e4 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -531,6 +531,15 @@ void multifd_save_cleanup(void)
 
         if (p->thread) {
             qemu_thread_join(p->thread);
+        } else {
+            /*
+             * The migration thread might be waiting on these. It is
+             * the responsibility of the channel thread to release
+             * them, unless it has never been created, then do it
+             * here.
+             */
+            qemu_sem_post(&p->sem_sync);
+            qemu_sem_post(&multifd_send_state->channels_ready);
         }
     }
     for (i = 0; i < migrate_multifd_channels(); i++) {
@@ -784,20 +793,15 @@ static void multifd_tls_outgoing_handshake(QIOTask *task,
 
     if (!qio_task_propagate_error(task, &err)) {
         trace_multifd_tls_outgoing_handshake_complete(ioc);
-        if (multifd_channel_connect(p, ioc, &err)) {
-            return;
-        }
+        multifd_channel_connect(p, ioc, &error_abort);
+    } else {
+        /*
+         * The multifd client could already be waiting to queue data,
+         * so let it know that we didn't even start.
+         */
+        p->quit = true;
+        trace_multifd_tls_outgoing_handshake_error(ioc, error_get_pretty(err));
     }
-
-    trace_multifd_tls_outgoing_handshake_error(ioc, error_get_pretty(err));
-
-    /*
-     * Error happen, mark multifd_send_thread status as 'quit' although it
-     * is not created, and then tell who pay attention to me.
-     */
-    p->quit = true;
-    qemu_sem_post(&multifd_send_state->channels_ready);
-    qemu_sem_post(&p->sem_sync);
 }
 
 static void *multifd_tls_handshake_thread(void *opaque)
@@ -870,9 +874,6 @@ static void multifd_new_send_channel_cleanup(MultiFDSendParams *p,
                                              QIOChannel *ioc, Error *err)
 {
      migrate_set_error(migrate_get_current(), err);
-     /* Error happen, we need to tell who pay attention to me */
-     qemu_sem_post(&multifd_send_state->channels_ready);
-     qemu_sem_post(&p->sem_sync);
      /*
       * Although multifd_send_thread is not created, but main migration
       * thread need to judge whether it is running, so we need to mark
-- 
2.35.3

Re: [RFC PATCH v2 2/4] migration/multifd: Join the TLS thread

[Peter Xu]

On Fri, Nov 10, 2023 at 05:02:39PM -0300, Fabiano Rosas wrote:
> @@ -826,7 +832,9 @@ static bool 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");
>      p->c = QIO_CHANNEL(tioc);
> -    qemu_thread_create(&p->thread, "multifd-tls-handshake-worker",
> +
> +    p->tls_thread = g_new0(QemuThread, 1);
> +    qemu_thread_create(p->tls_thread, "multifd-tls-handshake-worker",
>                         multifd_tls_handshake_thread, p,
>                         QEMU_THREAD_JOINABLE);

I understand your way of doing this, but personally I prefer bool and make
QemuThread not a pointer but still statically allocated.

Same comment to the next patch.

IMHO we can even add support for QemuThread in general to remember the bool
itself:

        struct QemuThread {
                pthread_t thread;
                bool thread_created;
        };

Changing qemu_thread_create() to set the bool, and join() to skip the real
join if it's not even created; clearing the bool otherwise after join()ed.
I _think_ it'll work transparently to existing callers, but start to allow
join() to be bypassed if thread not even created.

Thanks,

-- 
Peter Xu

Re: [RFC PATCH v2 1/4] migration/multifd: Stop setting p->ioc before connecting

[Peter Xu]

On Fri, Nov 10, 2023 at 05:02:38PM -0300, Fabiano Rosas wrote:
> This is being shadowed but the assignments at
> multifd_channel_connect() and multifd_tls_channel_connect() .
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [RFC PATCH v2 1/4] migration/multifd: Stop setting p->ioc before connecting

[Juan Quintela]

Fabiano Rosas <farosas@suse.de> wrote:
> This is being shadowed but the assignments at
> multifd_channel_connect() and multifd_tls_channel_connect() .
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Juan Quintela <quintela@redhat.com>