[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PATCH v2 2/5] migration: Make from_dst_file accesses thread-safe
From: |
Peter Xu |
Subject: |
[PATCH v2 2/5] migration: Make from_dst_file accesses thread-safe |
Date: |
Wed, 21 Jul 2021 15:34:06 -0400 |
Accessing from_dst_file is potentially racy in current code base like below:
if (s->from_dst_file)
do_something(s->from_dst_file);
Because from_dst_file can be reset right after the check in another
thread (rp_thread). One example is migrate_fd_cancel().
Use the same qemu_file_lock to protect it too, just like to_dst_file.
When it's safe to access without lock, comment it.
There's one special reference in migration_thread() that can be replaced by
the newly introduced rp_thread_created flag.
Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
migration/migration.c | 32 +++++++++++++++++++++++++-------
migration/migration.h | 8 +++++---
migration/ram.c | 1 +
3 files changed, 31 insertions(+), 10 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 21b94f75a3..fa70400f98 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1879,10 +1879,12 @@ static void migrate_fd_cancel(MigrationState *s)
QEMUFile *f = migrate_get_current()->to_dst_file;
trace_migrate_fd_cancel();
+ qemu_mutex_lock(&s->qemu_file_lock);
if (s->rp_state.from_dst_file) {
/* shutdown the rp socket, so causing the rp thread to shutdown */
qemu_file_shutdown(s->rp_state.from_dst_file);
}
+ qemu_mutex_unlock(&s->qemu_file_lock);
do {
old_state = s->state;
@@ -2686,6 +2688,22 @@ static int migrate_handle_rp_resume_ack(MigrationState
*s, uint32_t value)
return 0;
}
+/* Release ms->rp_state.from_dst_file in a safe way */
+static void migration_release_from_dst_file(MigrationState *ms)
+{
+ QEMUFile *file = ms->rp_state.from_dst_file;
+
+ qemu_mutex_lock(&ms->qemu_file_lock);
+ /*
+ * Reset the from_dst_file pointer first before releasing it, as we can't
+ * block within lock section
+ */
+ ms->rp_state.from_dst_file = NULL;
+ qemu_mutex_unlock(&ms->qemu_file_lock);
+
+ qemu_fclose(file);
+}
+
/*
* Handles messages sent on the return path towards the source VM
*
@@ -2827,11 +2845,13 @@ out:
* Maybe there is something we can do: it looks like a
* network down issue, and we pause for a recovery.
*/
- qemu_fclose(rp);
- ms->rp_state.from_dst_file = NULL;
+ migration_release_from_dst_file(ms);
rp = NULL;
if (postcopy_pause_return_path_thread(ms)) {
- /* Reload rp, reset the rest */
+ /*
+ * Reload rp, reset the rest. Referencing it is save since
+ * it's reset only by us above, or when migration completes
+ */
rp = ms->rp_state.from_dst_file;
ms->rp_state.error = false;
goto retry;
@@ -2843,8 +2863,7 @@ out:
}
trace_source_return_path_thread_end();
- ms->rp_state.from_dst_file = NULL;
- qemu_fclose(rp);
+ migration_release_from_dst_file(ms);
rcu_unregister_thread();
return NULL;
}
@@ -2852,7 +2871,6 @@ out:
static int open_return_path_on_source(MigrationState *ms,
bool create_thread)
{
-
ms->rp_state.from_dst_file = qemu_file_get_return_path(ms->to_dst_file);
if (!ms->rp_state.from_dst_file) {
return -1;
@@ -3746,7 +3764,7 @@ static void *migration_thread(void *opaque)
* If we opened the return path, we need to make sure dst has it
* opened as well.
*/
- if (s->rp_state.from_dst_file) {
+ if (s->rp_state.rp_thread_created) {
/* Now tell the dest that it should open its end so it can reply */
qemu_savevm_send_open_return_path(s->to_dst_file);
diff --git a/migration/migration.h b/migration/migration.h
index c302879fad..7a5aa8c2fd 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -154,12 +154,13 @@ struct MigrationState {
QemuThread thread;
QEMUBH *vm_start_bh;
QEMUBH *cleanup_bh;
+ /* Protected by qemu_file_lock */
QEMUFile *to_dst_file;
QIOChannelBuffer *bioc;
/*
- * Protects to_dst_file pointer. We need to make sure we won't
- * yield or hang during the critical section, since this lock will
- * be used in OOB command handler.
+ * Protects to_dst_file/from_dst_file pointers. We need to make sure we
+ * won't yield or hang during the critical section, since this lock will be
+ * used in OOB command handler.
*/
QemuMutex qemu_file_lock;
@@ -192,6 +193,7 @@ struct MigrationState {
/* State related to return path */
struct {
+ /* Protected by qemu_file_lock */
QEMUFile *from_dst_file;
QemuThread rp_thread;
bool error;
diff --git a/migration/ram.c b/migration/ram.c
index b5fc454b2f..f728f5072f 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4012,6 +4012,7 @@ static void ram_dirty_bitmap_reload_notify(MigrationState
*s)
int ram_dirty_bitmap_reload(MigrationState *s, RAMBlock *block)
{
int ret = -EINVAL;
+ /* from_dst_file is always valid because we're within rp_thread */
QEMUFile *file = s->rp_state.from_dst_file;
unsigned long *le_bitmap, nbits = block->used_length >> TARGET_PAGE_BITS;
uint64_t local_size = DIV_ROUND_UP(nbits, 8);
--
2.31.1