Re: [PATCH v2 2/5] migration: Make from_dst_file accesses thread-safe

[Dr. David Alan Gilbert]

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

Yep, with Eric's comments

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