Re: [PATCH 1/5] migration: Fix missing join() of rp_thread

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> 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>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@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
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK