[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PATCH 1/5] migration: Fix missing join() of rp_thread
From: |
Peter Xu |
Subject: |
[PATCH 1/5] migration: Fix missing join() of rp_thread |
Date: |
Tue, 20 Jul 2021 21:21:30 -0400 |
It's possible that the migration thread skip the join() of the rp_thread in
below race and crash on src right at finishing migration:
migration_thread rp_thread
---------------- ---------
migration_completion()
(before rp_thread quits)
from_dst_file=NULL
[thread got scheduled out]
s->rp_state.from_dst_file==NULL
(skip join() of rp_thread)
migrate_fd_cleanup()
qemu_fclose(s->to_dst_file)
yank_unregister_instance()
assert(yank_find_entry()) <------- crash
It could mostly happen with postcopy, but that shouldn't be required, e.g., I
think it could also trigger with MIGRATION_CAPABILITY_RETURN_PATH set.
It's suspected that above race could be the root cause of a recent (but rare)
migration-test break reported by either Dave or PMM:
https://lore.kernel.org/qemu-devel/YPamXAHwan%2FPPXLf@work-vm/
The issue is: from_dst_file is reset in the rp_thread, so if the thread reset
it to NULL fast enough then the migration thread will assume there's no
rp_thread at all.
This could potentially cause more severe issue (e.g. crash) after the yank code.
Fix it by using a boolean to keep "whether we've created rp_thread".
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
migration/migration.c | 4 +++-
migration/migration.h | 7 +++++++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/migration/migration.c b/migration/migration.c
index 2d306582eb..21b94f75a3 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2867,6 +2867,7 @@ static int open_return_path_on_source(MigrationState *ms,
qemu_thread_create(&ms->rp_state.rp_thread, "return path",
source_return_path_thread, ms, QEMU_THREAD_JOINABLE);
+ ms->rp_state.rp_thread_created = true;
trace_open_return_path_on_source_continue();
@@ -2891,6 +2892,7 @@ static int
await_return_path_close_on_source(MigrationState *ms)
}
trace_await_return_path_close_on_source_joining();
qemu_thread_join(&ms->rp_state.rp_thread);
+ ms->rp_state.rp_thread_created = false;
trace_await_return_path_close_on_source_close();
return ms->rp_state.error;
}
@@ -3170,7 +3172,7 @@ static void migration_completion(MigrationState *s)
* it will wait for the destination to send it's status in
* a SHUT command).
*/
- if (s->rp_state.from_dst_file) {
+ if (s->rp_state.rp_thread_created) {
int rp_error;
trace_migration_return_path_end_before();
rp_error = await_return_path_close_on_source(s);
diff --git a/migration/migration.h b/migration/migration.h
index 2ebb740dfa..c302879fad 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -195,6 +195,13 @@ struct MigrationState {
QEMUFile *from_dst_file;
QemuThread rp_thread;
bool error;
+ /*
+ * We can also check non-zero of rp_thread, but there's no "official"
+ * way to do this, so this bool makes it slightly more elegant.
+ * Checking from_dst_file for this is racy because from_dst_file will
+ * be cleared in the rp_thread!
+ */
+ bool rp_thread_created;
QemuSemaphore rp_sem;
} rp_state;
--
2.31.1
[PATCH 3/5] migration: Introduce migration_ioc_[un]register_yank(), Peter Xu, 2021/07/20
[PATCH 4/5] migration: Teach QEMUFile to be QIOChannel-aware, Peter Xu, 2021/07/20