Re: [PATCH 1/2] migration: Make all helpers in misc.h safe to use without migration

[Avihai Horon <avihaih@nvidia.com>]

On 22/10/2024 19:07, Peter Xu wrote:
> External email: Use caution opening links or attachments
>
>
> Migration object can be freed before some other device codes run, while we
> do have a bunch of migration helpers exported in migration/misc.h that
> logically can be invoked at any time of QEMU, even during destruction of a
> VM.
>
> Make all these functions safe to be called, especially, not crashing after
> the migration object is freed.
>
> Add a rich comment in the header explaining how to guarantee thread safe on
> using these functions, and we choose BQL because fundamentally that's how
> it's working now.  We can move to other things (e.g. RCU) whenever
> necessary in the future but it's an overkill if we have BQL anyway in
> most/all existing callers.
>
> When at it, update some comments, e.g. migrate_announce_params() is
> exported from options.c now.
>
> Cc: Cédric Le Goater <clg@redhat.com>
> Cc: Avihai Horon <avihaih@nvidia.com>
> Cc: Fabiano Rosas <farosas@suse.de>
> Cc: Dr. David Alan Gilbert <dave@treblig.org>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   include/migration/misc.h | 33 ++++++++++++++++++++++++++++-----
>   migration/migration.c    | 22 +++++++++++++++++++++-
>   2 files changed, 49 insertions(+), 6 deletions(-)
>
> diff --git a/include/migration/misc.h b/include/migration/misc.h
> index bfadc5613b..8d6812b8c7 100644
> --- a/include/migration/misc.h
> +++ b/include/migration/misc.h
> @@ -19,8 +19,26 @@
>   #include "qapi/qapi-types-net.h"
>   #include "migration/client-options.h"
>
> -/* migration/ram.c */
> +/*
> + * Misc migration functions exported to be used in QEMU generic system
> + * code outside migration/.
> + *
> + * By default, BQL is required to use below functions to avoid race
> + * conditions (e.g. concurrent free of the migration object).  It's
> + * caller's responsibility to make sure it's thread safe otherwise when
> + * below helpers are used without BQL held.
> + *
> + * One example of the special case is migration_thread(), who will take a
> + * refcount of the migration object.  The refcount will make sure the
> + * migration object will not be freed concurrently when accessing through
> + * below helpers.
> + *
> + * When unsure, always take BQL first before using the helpers.
> + */
>
> +/*
> + * migration/ram.c
> + */
>   typedef enum PrecopyNotifyReason {
>       PRECOPY_NOTIFY_SETUP = 0,
>       PRECOPY_NOTIFY_BEFORE_BITMAP_SYNC = 1,
> @@ -43,14 +61,19 @@ void ram_mig_init(void);
>   void qemu_guest_free_page_hint(void *addr, size_t len);
>   bool migrate_ram_is_ignored(RAMBlock *block);
>
> -/* migration/block.c */
> -
> +/*
> + * migration/options.c
> + */
>   AnnounceParameters *migrate_announce_params(void);
> -/* migration/savevm.c */
>
> +/*
> + * migration/savevm.c
> + */
>   void dump_vmstate_json_to_file(FILE *out_fp);
>
> -/* migration/migration.c */
> +/*
> + * migration/migration.c
> + */
>   void migration_object_init(void);
>   void migration_shutdown(void);
>   bool migration_is_idle(void);
> diff --git a/migration/migration.c b/migration/migration.c
> index bcb735869b..27341eed50 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1121,6 +1121,10 @@ bool migration_is_setup_or_active(void)
>   {
>       MigrationState *s = current_migration;
>
> +    if (!s) {
> +        return false;
> +    }
> +
>       switch (s->state) {
>       case MIGRATION_STATUS_ACTIVE:
>       case MIGRATION_STATUS_POSTCOPY_ACTIVE:
> @@ -1136,7 +1140,6 @@ bool migration_is_setup_or_active(void)
>
>       default:
>           return false;
> -
>       }
>   }
>
> @@ -1685,6 +1688,10 @@ bool migration_is_active(void)
>   {
>       MigrationState *s = current_migration;
>
> +    if (!s) {
> +        return false;
> +    }
> +
>       return (s->state == MIGRATION_STATUS_ACTIVE ||
>               s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE);
>   }
> @@ -1693,6 +1700,10 @@ bool migration_is_device(void)
>   {
>       MigrationState *s = current_migration;
>
> +    if (!s) {
> +        return false;
> +    }
> +
>       return s->state == MIGRATION_STATUS_DEVICE;
>   }
>
> @@ -1700,6 +1711,11 @@ bool migration_thread_is_self(void)
>   {
>       MigrationState *s = current_migration;
>
> +    /* If no migration object, must not be the migration thread */
> +    if (!s) {
> +        return false;
> +    }
> +
>       return qemu_thread_is_self(&s->thread);
>   }
>
> @@ -3077,6 +3093,10 @@ void migration_file_set_error(int ret, Error *err)
>   {
>       MigrationState *s = current_migration;
>
> +    if (!s) {
> +        return;
> +    }
> +

I think this is not enough because current_migration is never set to 
NULL after it's destroyed.

Can we add "current_migration = NULL;" to migration_instance_finalize()?

Thanks.

>       WITH_QEMU_LOCK_GUARD(&s->qemu_file_lock) {
>           if (s->to_dst_file) {
>               qemu_file_set_error_obj(s->to_dst_file, ret, err);
> --
> 2.45.0
>