Re: [PATCH QEMU HCI-8.1 v3 17/28] migration: Fix crash on second migration when cancel early

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] migration: migration_cancel() asserts on mode that migrate-set-parameters can change
--

commit 1a45829d1f41fe592f4ff72499c683f6dac53858
Author: Peter Xu <peterx@redhat.com>

migration: Fix crash on second migration when cancel early

This commit changes migration_cancel() to call migration_cleanup() when
the cpr-transfer HUP gsource is still pending. The goal is to avoid a
crash on a second migration after an early cancel. It also clears
hup_source as soon as the HUP callback runs, and adds assertions.

Link: https://lore.kernel.org/r/20260417184742.293061-1-marcandre.lureau@redhat.com
Link: https://lore.kernel.org/r/20260421175820.302795-1-peterx@redhat.com

> diff --git a/migration/migration.c b/migration/migration.c
> --- a/migration/migration.c
> +++ b/migration/migration.c

[ ... ]

> @@ -1653,14 +1653,19 @@ void migration_cancel(void)
>      }
>  
>      /*
> -     * If qmp_migrate_finish has not been called, then there is no path that
> -     * will complete the cancellation.  Do it now.
> +     * This is cpr-transfer specific processing.
> +     *
> +     * If this is true, it means cpr-transfer migration is waiting for the
> +     * destination to send HUP event on CPR channel to continue the next
> +     * phase.  If so, do the cleanup proactively to avoid get stuck in
> +     * CANCELLING state.
>       */
> -    if (setup && !s->to_dst_file) {
> -        migrate_set_state(&s->state, MIGRATION_STATUS_CANCELLING,
> -                          MIGRATION_STATUS_CANCELLED);
> -        cpr_state_close();
> -        cpr_transfer_source_destroy(s);
> +    if (cpr_transfer_source_active(s)) {
> +        assert(migrate_mode() == MIG_MODE_CPR_TRANSFER);

[Severity: Medium]
Can this assert abort QEMU? migrate_mode() on the source falls back to
s->parameters.mode, because cpr_get_incoming_mode() returns
MIG_MODE_NONE there. That value can be changed while hup_source is still
set.

The sequence would be:

qmp_migrate() with mode=cpr-transfer
  -> cpr_transfer_add_hup_watch()
     (state stays SETUP, hup_source set until the destination closes
     the CPR channel)

qmp_migrate_set_parameters() with mode=normal
  -> migrate_params_check()
  -> migrate_params_apply()

migrate_cancel, or migration_shutdown() at QEMU exit
  -> migration_cancel()
  -> cpr_transfer_source_active(s) is true
  -> assert(migrate_mode() == MIG_MODE_CPR_TRANSFER) fails

As far as I can tell, neither migrate_params_check() nor the apply step
rejects a mode change while a migration is running or while hup_source
is set. The migration_is_running() check in options.c is in the
capabilities path, not the parameters path.

The wait for the destination to close the CPR channel has no upper bound,
so this window can stay open for a long time. Before this patch, the
cancel path in this window did not assert on the mode.

Would it make sense to reject a mode change while a migration is
running, or to drop the mode assertion here?

> +        assert(setup && !s->to_dst_file);
> +        migration_cleanup(s);
> +        /* Now all things should have been released */
> +        assert(!cpr_transfer_source_active(s));
>      }
>  }

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261016-vfio_v1-v3-0-47487cb5a8e5@virtuozzo.com?part=17