| Message ID | 20230728121516.16258-4-farosas@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | Fix segfault on migration return path | expand |
On Fri, Jul 28, 2023 at 09:15:16AM -0300, Fabiano Rosas wrote: > Replace the return path retry logic with finishing and restarting the > thread. This fixes a race when resuming the migration that leads to a > segfault. > > Currently when doing postcopy we consider that an IO error on the > return path file could be due to a network intermittency. We then keep > the thread alive but have it do cleanup of the 'from_dst_file' and > wait on the 'postcopy_pause_rp' semaphore. When the user issues a > migrate resume, a new return path is opened and the thread is allowed > to continue. > > There's a race condition in the above mechanism. It is possible for > the new return path file to be setup *before* the cleanup code in the > return path thread has had a chance to run, leading to the *new* file > being closed and the pointer set to NULL. When the thread is released > after the resume, it tries to dereference 'from_dst_file' and crashes: > > Thread 7 "return path" received signal SIGSEGV, Segmentation fault. > [Switching to Thread 0x7fffd1dbf700 (LWP 9611)] > 0x00005555560e4893 in qemu_file_get_error_obj (f=0x0, errp=0x0) at ../migration/qemu-file.c:154 > 154 return f->last_error; > > (gdb) bt > #0 0x00005555560e4893 in qemu_file_get_error_obj (f=0x0, errp=0x0) at ../migration/qemu-file.c:154 > #1 0x00005555560e4983 in qemu_file_get_error (f=0x0) at ../migration/qemu-file.c:206 > #2 0x0000555555b9a1df in source_return_path_thread (opaque=0x555556e06000) at ../migration/migration.c:1876 > #3 0x000055555602e14f in qemu_thread_start (args=0x55555782e780) at ../util/qemu-thread-posix.c:541 > #4 0x00007ffff38d76ea in start_thread (arg=0x7fffd1dbf700) at pthread_create.c:477 > #5 0x00007ffff35efa6f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95 > > Here's the race (important bit is open_return_path happening before > migration_release_dst_files): > > migration | qmp | return path > --------------------------+-----------------------------+--------------------------------- > qmp_migrate_pause() > shutdown(ms->to_dst_file) > f->last_error = -EIO > migrate_detect_error() > postcopy_pause() > set_state(PAUSED) > wait(postcopy_pause_sem) > qmp_migrate(resume) > migrate_fd_connect() > resume = state == PAUSED > open_return_path <-- TOO SOON! > set_state(RECOVER) > post(postcopy_pause_sem) > (incoming closes to_src_file) > res = qemu_file_get_error(rp) > migration_release_dst_files() > ms->rp_state.from_dst_file = NULL > post(postcopy_pause_rp_sem) > postcopy_pause_return_path_thread() > wait(postcopy_pause_rp_sem) > rp = ms->rp_state.from_dst_file > goto retry > qemu_file_get_error(rp) > SIGSEGV > ------------------------------------------------------------------------------------------- > > We can keep the retry logic without having the thread alive and > waiting. The only piece of data used by it is the 'from_dst_file' and > it is only allowed to proceed after a migrate resume is issued and the > semaphore released at migrate_fd_connect(). > > Move the retry logic to outside the thread by having > open_return_path_on_source() wait for the thread to finish before > creating a new one with the updated 'from_dst_file'. If we can remove that (along with the sync sem) it'll be pretty nice. If you want, and if this works well, not sure whether you're interested in doing similarly to the other threads. Currently we halt all the threads, I'm not sure whether we can do similiar things on dest and whether that can also benefit on sync efforts. Still one comment below. > > Signed-off-by: Fabiano Rosas <farosas@suse.de> > --- > migration/migration.c | 72 +++++++++++++++---------------------------- > migration/migration.h | 1 - > 2 files changed, 25 insertions(+), 48 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c > index d6f4470265..36cdd7bda8 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -97,6 +97,7 @@ static int migration_maybe_pause(MigrationState *s, > int *current_active_state, > int new_state); > static void migrate_fd_cancel(MigrationState *s); > +static int await_return_path_close_on_source(MigrationState *ms); > > static bool migration_needs_multiple_sockets(void) > { > @@ -1764,18 +1765,6 @@ static void migrate_handle_rp_req_pages(MigrationState *ms, const char* rbname, > } > } > > -/* Return true to retry, false to quit */ > -static bool postcopy_pause_return_path_thread(MigrationState *s) > -{ > - trace_postcopy_pause_return_path(); > - > - qemu_sem_wait(&s->postcopy_pause_rp_sem); > - > - trace_postcopy_pause_return_path_continued(); > - > - return true; > -} > - > static int migrate_handle_rp_recv_bitmap(MigrationState *s, char *block_name) > { > RAMBlock *block = qemu_ram_block_by_name(block_name); > @@ -1859,7 +1848,6 @@ static void *source_return_path_thread(void *opaque) > trace_source_return_path_thread_entry(); > rcu_register_thread(); > > -retry: > while (!ms->rp_state.error && !qemu_file_get_error(rp) && > migration_is_setup_or_active(ms->state)) { > trace_source_return_path_thread_loop_top(); > @@ -1981,28 +1969,18 @@ retry: > } > > out: > - res = qemu_file_get_error(rp); > - if (res) { > - if (res && migration_in_postcopy()) { > + if (qemu_file_get_error(rp)) { > + if (migration_in_postcopy()) { > /* > - * Maybe there is something we can do: it looks like a > - * network down issue, and we pause for a recovery. > + * This could be a network issue that would have been > + * detected by the main migration thread and caused the > + * migration to pause. Do cleanup and finish. > */ > - migration_release_dst_files(ms); > - rp = NULL; > - if (postcopy_pause_return_path_thread(ms)) { > - /* > - * 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; > - } > + ms->rp_state.error = false; > + } else { > + trace_source_return_path_thread_bad_end(); > + mark_source_rp_bad(ms); > } > - > - trace_source_return_path_thread_bad_end(); > - mark_source_rp_bad(ms); > } > > trace_source_return_path_thread_end(); > @@ -2012,8 +1990,21 @@ out: > } > > static int open_return_path_on_source(MigrationState *ms, > - bool create_thread) > + bool resume) > { > + if (resume) { > + assert(ms->state == MIGRATION_STATUS_POSTCOPY_PAUSED); > + > + /* > + * We're resuming from a paused postcopy migration. Wait for > + * the thread to do its cleanup before re-opening the return > + * path. > + */ > + if (await_return_path_close_on_source(ms)) { > + return -1; > + } This smells a bit hacky. Can we do this in postcopy_pause(), perhaps before we switch to PAUSED state? Then we know after being PAUSED we're ready to recover. Thanks, > + } > + > ms->rp_state.from_dst_file = qemu_file_get_return_path(ms->to_dst_file); > if (!ms->rp_state.from_dst_file) { > return -1; > @@ -2021,11 +2012,6 @@ static int open_return_path_on_source(MigrationState *ms, > > trace_open_return_path_on_source(); > > - if (!create_thread) { > - /* We're done */ > - return 0; > - } > - > 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; > @@ -2550,12 +2536,6 @@ static MigThrError postcopy_pause(MigrationState *s) > if (s->state == MIGRATION_STATUS_POSTCOPY_RECOVER) { > /* Woken up by a recover procedure. Give it a shot */ > > - /* > - * Firstly, let's wake up the return path now, with a new > - * return path channel. > - */ > - qemu_sem_post(&s->postcopy_pause_rp_sem); > - > /* Do the resume logic */ > if (postcopy_do_resume(s) == 0) { > /* Let's continue! */ > @@ -3243,7 +3223,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in) > * QEMU uses the return path. > */ > if (migrate_postcopy_ram() || migrate_return_path()) { > - if (open_return_path_on_source(s, !resume)) { > + if (open_return_path_on_source(s, resume)) { > error_report("Unable to open return-path for postcopy"); > migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED); > migrate_fd_cleanup(s); > @@ -3304,7 +3284,6 @@ static void migration_instance_finalize(Object *obj) > qemu_sem_destroy(&ms->rate_limit_sem); > qemu_sem_destroy(&ms->pause_sem); > qemu_sem_destroy(&ms->postcopy_pause_sem); > - qemu_sem_destroy(&ms->postcopy_pause_rp_sem); > qemu_sem_destroy(&ms->rp_state.rp_sem); > qemu_sem_destroy(&ms->rp_state.rp_pong_acks); > qemu_sem_destroy(&ms->postcopy_qemufile_src_sem); > @@ -3324,7 +3303,6 @@ static void migration_instance_init(Object *obj) > migrate_params_init(&ms->parameters); > > qemu_sem_init(&ms->postcopy_pause_sem, 0); > - qemu_sem_init(&ms->postcopy_pause_rp_sem, 0); > qemu_sem_init(&ms->rp_state.rp_sem, 0); > qemu_sem_init(&ms->rp_state.rp_pong_acks, 0); > qemu_sem_init(&ms->rate_limit_sem, 0); > diff --git a/migration/migration.h b/migration/migration.h > index b7c8b67542..e78db5361c 100644 > --- a/migration/migration.h > +++ b/migration/migration.h > @@ -382,7 +382,6 @@ struct MigrationState { > > /* Needed by postcopy-pause state */ > QemuSemaphore postcopy_pause_sem; > - QemuSemaphore postcopy_pause_rp_sem; > /* > * Whether we abort the migration if decompression errors are > * detected at the destination. It is left at false for qemu > -- > 2.35.3 >
Peter Xu <peterx@redhat.com> writes: > On Fri, Jul 28, 2023 at 09:15:16AM -0300, Fabiano Rosas wrote: >> Replace the return path retry logic with finishing and restarting the >> thread. This fixes a race when resuming the migration that leads to a >> segfault. >> >> Currently when doing postcopy we consider that an IO error on the >> return path file could be due to a network intermittency. We then keep >> the thread alive but have it do cleanup of the 'from_dst_file' and >> wait on the 'postcopy_pause_rp' semaphore. When the user issues a >> migrate resume, a new return path is opened and the thread is allowed >> to continue. >> >> There's a race condition in the above mechanism. It is possible for >> the new return path file to be setup *before* the cleanup code in the >> return path thread has had a chance to run, leading to the *new* file >> being closed and the pointer set to NULL. When the thread is released >> after the resume, it tries to dereference 'from_dst_file' and crashes: >> >> Thread 7 "return path" received signal SIGSEGV, Segmentation fault. >> [Switching to Thread 0x7fffd1dbf700 (LWP 9611)] >> 0x00005555560e4893 in qemu_file_get_error_obj (f=0x0, errp=0x0) at ../migration/qemu-file.c:154 >> 154 return f->last_error; >> >> (gdb) bt >> #0 0x00005555560e4893 in qemu_file_get_error_obj (f=0x0, errp=0x0) at ../migration/qemu-file.c:154 >> #1 0x00005555560e4983 in qemu_file_get_error (f=0x0) at ../migration/qemu-file.c:206 >> #2 0x0000555555b9a1df in source_return_path_thread (opaque=0x555556e06000) at ../migration/migration.c:1876 >> #3 0x000055555602e14f in qemu_thread_start (args=0x55555782e780) at ../util/qemu-thread-posix.c:541 >> #4 0x00007ffff38d76ea in start_thread (arg=0x7fffd1dbf700) at pthread_create.c:477 >> #5 0x00007ffff35efa6f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95 >> >> Here's the race (important bit is open_return_path happening before >> migration_release_dst_files): >> >> migration | qmp | return path >> --------------------------+-----------------------------+--------------------------------- >> qmp_migrate_pause() >> shutdown(ms->to_dst_file) >> f->last_error = -EIO >> migrate_detect_error() >> postcopy_pause() >> set_state(PAUSED) >> wait(postcopy_pause_sem) >> qmp_migrate(resume) >> migrate_fd_connect() >> resume = state == PAUSED >> open_return_path <-- TOO SOON! >> set_state(RECOVER) >> post(postcopy_pause_sem) >> (incoming closes to_src_file) >> res = qemu_file_get_error(rp) >> migration_release_dst_files() >> ms->rp_state.from_dst_file = NULL >> post(postcopy_pause_rp_sem) >> postcopy_pause_return_path_thread() >> wait(postcopy_pause_rp_sem) >> rp = ms->rp_state.from_dst_file >> goto retry >> qemu_file_get_error(rp) >> SIGSEGV >> ------------------------------------------------------------------------------------------- >> >> We can keep the retry logic without having the thread alive and >> waiting. The only piece of data used by it is the 'from_dst_file' and >> it is only allowed to proceed after a migrate resume is issued and the >> semaphore released at migrate_fd_connect(). >> >> Move the retry logic to outside the thread by having >> open_return_path_on_source() wait for the thread to finish before >> creating a new one with the updated 'from_dst_file'. > > If we can remove that (along with the sync sem) it'll be pretty nice. If > you want, and if this works well, not sure whether you're interested in > doing similarly to the other threads. Currently we halt all the threads, > I'm not sure whether we can do similiar things on dest and whether that can > also benefit on sync efforts. I'm interested but I don't know the postcopy code too well, I'll have to spend some time with it. Next on my list was the multifd p->running flag which seems entirely superfluous to me. > > Still one comment below. > >> >> Signed-off-by: Fabiano Rosas <farosas@suse.de> >> --- >> migration/migration.c | 72 +++++++++++++++---------------------------- >> migration/migration.h | 1 - >> 2 files changed, 25 insertions(+), 48 deletions(-) >> >> diff --git a/migration/migration.c b/migration/migration.c >> index d6f4470265..36cdd7bda8 100644 >> --- a/migration/migration.c >> +++ b/migration/migration.c >> @@ -97,6 +97,7 @@ static int migration_maybe_pause(MigrationState *s, >> int *current_active_state, >> int new_state); >> static void migrate_fd_cancel(MigrationState *s); >> +static int await_return_path_close_on_source(MigrationState *ms); >> >> static bool migration_needs_multiple_sockets(void) >> { >> @@ -1764,18 +1765,6 @@ static void migrate_handle_rp_req_pages(MigrationState *ms, const char* rbname, >> } >> } >> >> -/* Return true to retry, false to quit */ >> -static bool postcopy_pause_return_path_thread(MigrationState *s) >> -{ >> - trace_postcopy_pause_return_path(); >> - >> - qemu_sem_wait(&s->postcopy_pause_rp_sem); >> - >> - trace_postcopy_pause_return_path_continued(); >> - >> - return true; >> -} >> - >> static int migrate_handle_rp_recv_bitmap(MigrationState *s, char *block_name) >> { >> RAMBlock *block = qemu_ram_block_by_name(block_name); >> @@ -1859,7 +1848,6 @@ static void *source_return_path_thread(void *opaque) >> trace_source_return_path_thread_entry(); >> rcu_register_thread(); >> >> -retry: >> while (!ms->rp_state.error && !qemu_file_get_error(rp) && >> migration_is_setup_or_active(ms->state)) { >> trace_source_return_path_thread_loop_top(); >> @@ -1981,28 +1969,18 @@ retry: >> } >> >> out: >> - res = qemu_file_get_error(rp); >> - if (res) { >> - if (res && migration_in_postcopy()) { >> + if (qemu_file_get_error(rp)) { >> + if (migration_in_postcopy()) { >> /* >> - * Maybe there is something we can do: it looks like a >> - * network down issue, and we pause for a recovery. >> + * This could be a network issue that would have been >> + * detected by the main migration thread and caused the >> + * migration to pause. Do cleanup and finish. >> */ >> - migration_release_dst_files(ms); >> - rp = NULL; >> - if (postcopy_pause_return_path_thread(ms)) { >> - /* >> - * 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; >> - } >> + ms->rp_state.error = false; >> + } else { >> + trace_source_return_path_thread_bad_end(); >> + mark_source_rp_bad(ms); >> } >> - >> - trace_source_return_path_thread_bad_end(); >> - mark_source_rp_bad(ms); >> } >> >> trace_source_return_path_thread_end(); >> @@ -2012,8 +1990,21 @@ out: >> } >> >> static int open_return_path_on_source(MigrationState *ms, >> - bool create_thread) >> + bool resume) >> { >> + if (resume) { >> + assert(ms->state == MIGRATION_STATUS_POSTCOPY_PAUSED); >> + >> + /* >> + * We're resuming from a paused postcopy migration. Wait for >> + * the thread to do its cleanup before re-opening the return >> + * path. >> + */ >> + if (await_return_path_close_on_source(ms)) { >> + return -1; >> + } > > This smells a bit hacky. Can we do this in postcopy_pause(), perhaps > before we switch to PAUSED state? Then we know after being PAUSED we're > ready to recover. It looks like we could. I'll move it.
diff --git a/migration/migration.c b/migration/migration.c index d6f4470265..36cdd7bda8 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -97,6 +97,7 @@ static int migration_maybe_pause(MigrationState *s, int *current_active_state, int new_state); static void migrate_fd_cancel(MigrationState *s); +static int await_return_path_close_on_source(MigrationState *ms); static bool migration_needs_multiple_sockets(void) { @@ -1764,18 +1765,6 @@ static void migrate_handle_rp_req_pages(MigrationState *ms, const char* rbname, } } -/* Return true to retry, false to quit */ -static bool postcopy_pause_return_path_thread(MigrationState *s) -{ - trace_postcopy_pause_return_path(); - - qemu_sem_wait(&s->postcopy_pause_rp_sem); - - trace_postcopy_pause_return_path_continued(); - - return true; -} - static int migrate_handle_rp_recv_bitmap(MigrationState *s, char *block_name) { RAMBlock *block = qemu_ram_block_by_name(block_name); @@ -1859,7 +1848,6 @@ static void *source_return_path_thread(void *opaque) trace_source_return_path_thread_entry(); rcu_register_thread(); -retry: while (!ms->rp_state.error && !qemu_file_get_error(rp) && migration_is_setup_or_active(ms->state)) { trace_source_return_path_thread_loop_top(); @@ -1981,28 +1969,18 @@ retry: } out: - res = qemu_file_get_error(rp); - if (res) { - if (res && migration_in_postcopy()) { + if (qemu_file_get_error(rp)) { + if (migration_in_postcopy()) { /* - * Maybe there is something we can do: it looks like a - * network down issue, and we pause for a recovery. + * This could be a network issue that would have been + * detected by the main migration thread and caused the + * migration to pause. Do cleanup and finish. */ - migration_release_dst_files(ms); - rp = NULL; - if (postcopy_pause_return_path_thread(ms)) { - /* - * 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; - } + ms->rp_state.error = false; + } else { + trace_source_return_path_thread_bad_end(); + mark_source_rp_bad(ms); } - - trace_source_return_path_thread_bad_end(); - mark_source_rp_bad(ms); } trace_source_return_path_thread_end(); @@ -2012,8 +1990,21 @@ out: } static int open_return_path_on_source(MigrationState *ms, - bool create_thread) + bool resume) { + if (resume) { + assert(ms->state == MIGRATION_STATUS_POSTCOPY_PAUSED); + + /* + * We're resuming from a paused postcopy migration. Wait for + * the thread to do its cleanup before re-opening the return + * path. + */ + if (await_return_path_close_on_source(ms)) { + return -1; + } + } + ms->rp_state.from_dst_file = qemu_file_get_return_path(ms->to_dst_file); if (!ms->rp_state.from_dst_file) { return -1; @@ -2021,11 +2012,6 @@ static int open_return_path_on_source(MigrationState *ms, trace_open_return_path_on_source(); - if (!create_thread) { - /* We're done */ - return 0; - } - 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; @@ -2550,12 +2536,6 @@ static MigThrError postcopy_pause(MigrationState *s) if (s->state == MIGRATION_STATUS_POSTCOPY_RECOVER) { /* Woken up by a recover procedure. Give it a shot */ - /* - * Firstly, let's wake up the return path now, with a new - * return path channel. - */ - qemu_sem_post(&s->postcopy_pause_rp_sem); - /* Do the resume logic */ if (postcopy_do_resume(s) == 0) { /* Let's continue! */ @@ -3243,7 +3223,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in) * QEMU uses the return path. */ if (migrate_postcopy_ram() || migrate_return_path()) { - if (open_return_path_on_source(s, !resume)) { + if (open_return_path_on_source(s, resume)) { error_report("Unable to open return-path for postcopy"); migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED); migrate_fd_cleanup(s); @@ -3304,7 +3284,6 @@ static void migration_instance_finalize(Object *obj) qemu_sem_destroy(&ms->rate_limit_sem); qemu_sem_destroy(&ms->pause_sem); qemu_sem_destroy(&ms->postcopy_pause_sem); - qemu_sem_destroy(&ms->postcopy_pause_rp_sem); qemu_sem_destroy(&ms->rp_state.rp_sem); qemu_sem_destroy(&ms->rp_state.rp_pong_acks); qemu_sem_destroy(&ms->postcopy_qemufile_src_sem); @@ -3324,7 +3303,6 @@ static void migration_instance_init(Object *obj) migrate_params_init(&ms->parameters); qemu_sem_init(&ms->postcopy_pause_sem, 0); - qemu_sem_init(&ms->postcopy_pause_rp_sem, 0); qemu_sem_init(&ms->rp_state.rp_sem, 0); qemu_sem_init(&ms->rp_state.rp_pong_acks, 0); qemu_sem_init(&ms->rate_limit_sem, 0); diff --git a/migration/migration.h b/migration/migration.h index b7c8b67542..e78db5361c 100644 --- a/migration/migration.h +++ b/migration/migration.h @@ -382,7 +382,6 @@ struct MigrationState { /* Needed by postcopy-pause state */ QemuSemaphore postcopy_pause_sem; - QemuSemaphore postcopy_pause_rp_sem; /* * Whether we abort the migration if decompression errors are * detected at the destination. It is left at false for qemu
Replace the return path retry logic with finishing and restarting the thread. This fixes a race when resuming the migration that leads to a segfault. Currently when doing postcopy we consider that an IO error on the return path file could be due to a network intermittency. We then keep the thread alive but have it do cleanup of the 'from_dst_file' and wait on the 'postcopy_pause_rp' semaphore. When the user issues a migrate resume, a new return path is opened and the thread is allowed to continue. There's a race condition in the above mechanism. It is possible for the new return path file to be setup *before* the cleanup code in the return path thread has had a chance to run, leading to the *new* file being closed and the pointer set to NULL. When the thread is released after the resume, it tries to dereference 'from_dst_file' and crashes: Thread 7 "return path" received signal SIGSEGV, Segmentation fault. [Switching to Thread 0x7fffd1dbf700 (LWP 9611)] 0x00005555560e4893 in qemu_file_get_error_obj (f=0x0, errp=0x0) at ../migration/qemu-file.c:154 154 return f->last_error; (gdb) bt #0 0x00005555560e4893 in qemu_file_get_error_obj (f=0x0, errp=0x0) at ../migration/qemu-file.c:154 #1 0x00005555560e4983 in qemu_file_get_error (f=0x0) at ../migration/qemu-file.c:206 #2 0x0000555555b9a1df in source_return_path_thread (opaque=0x555556e06000) at ../migration/migration.c:1876 #3 0x000055555602e14f in qemu_thread_start (args=0x55555782e780) at ../util/qemu-thread-posix.c:541 #4 0x00007ffff38d76ea in start_thread (arg=0x7fffd1dbf700) at pthread_create.c:477 #5 0x00007ffff35efa6f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95 Here's the race (important bit is open_return_path happening before migration_release_dst_files): migration | qmp | return path --------------------------+-----------------------------+--------------------------------- qmp_migrate_pause() shutdown(ms->to_dst_file) f->last_error = -EIO migrate_detect_error() postcopy_pause() set_state(PAUSED) wait(postcopy_pause_sem) qmp_migrate(resume) migrate_fd_connect() resume = state == PAUSED open_return_path <-- TOO SOON! set_state(RECOVER) post(postcopy_pause_sem) (incoming closes to_src_file) res = qemu_file_get_error(rp) migration_release_dst_files() ms->rp_state.from_dst_file = NULL post(postcopy_pause_rp_sem) postcopy_pause_return_path_thread() wait(postcopy_pause_rp_sem) rp = ms->rp_state.from_dst_file goto retry qemu_file_get_error(rp) SIGSEGV ------------------------------------------------------------------------------------------- We can keep the retry logic without having the thread alive and waiting. The only piece of data used by it is the 'from_dst_file' and it is only allowed to proceed after a migrate resume is issued and the semaphore released at migrate_fd_connect(). Move the retry logic to outside the thread by having open_return_path_on_source() wait for the thread to finish before creating a new one with the updated 'from_dst_file'. Signed-off-by: Fabiano Rosas <farosas@suse.de> --- migration/migration.c | 72 +++++++++++++++---------------------------- migration/migration.h | 1 - 2 files changed, 25 insertions(+), 48 deletions(-)