mirrors/qemu
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

migration: Make from_dst_file accesses thread-safe

Browse Source 
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>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Lukas Straub <lukasstraub2@web.de>
Message-Id: <20210722175841.938739-3-peterx@redhat.com>
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
  with Peter's fixup
This commit is contained in:
Peter Xu 2021-07-22 13:58:38 -04:00 committed by Dr. David Alan Gilbert
parent 53021ea165
commit 43044ac0ee
3 changed files with 35 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

33
migration/migration.c
View File

@ -1879,10 +1879,12 @@ static void migrate_fd_cancel(MigrationState *s)
QEMUFile *f = migrate_get_current()->to_dst_file;
trace_migrate_fd_cancel();
WITH_QEMU_LOCK_GUARD(&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);
}
}
do {
old_state = s->state;
@ -2686,6 +2688,23 @@ 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;
WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
/*
* Reset the from_dst_file pointer first before releasing it, as we
* can't block within lock section
*/
file = ms->rp_state.from_dst_file;
ms->rp_state.from_dst_file = NULL;
}
qemu_fclose(file);
}
/*
* Handles messages sent on the return path towards the source VM
*
@ -2827,11 +2846,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 safe 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 +2864,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 +2872,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 +3765,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);

8
migration/migration.h
View File

@ -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;

1
migration/ram.c
View File

@ -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);