Re: [PATCH QEMU HCI-8.1 v6 21/34] 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: assert(migrate_mode() == CPR_TRANSFER) in migration_cancel() can abort QEMU
--

commit 4600a9bb3fca173c79b21943fa97310ae3af2659
Author: Peter Xu <peterx@redhat.com>

migration: Fix crash on second migration when cancel early

This patch makes migration_cancel() call migration_cleanup() when the
cpr-transfer HUP gsource is still armed, rather than special-casing
the SETUP state. It also detaches hup_source at the start of
qmp_migrate_finish_cb() and adds several 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

[ ... ]

> @@ -1661,14 +1661,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() is not latched for the running migration. It returns
cpr_get_incoming_mode() if that is not NONE, and otherwise returns the
live s->parameters.mode:

migrate_mode()
    mode = cpr_get_incoming_mode();
    if (mode == MIG_MODE_NONE) {
        mode = migrate_get_current()->parameters.mode;
    }

As far as I can tell, qmp_migrate_set_parameters() and
migrate_params_check() do not reject a change of mode while a migration
is in SETUP. Consider this sequence:

1. qmp_migrate() with mode=cpr-transfer runs migrate_prepare(), then
   cpr_state_save(), then cpr_transfer_add_hup_watch(). hup_source is
   now set and the state stays SETUP, waiting for the destination HUP.

2. Before the HUP arrives, the client sends migrate-set-parameters
   with mode=normal.

3. The client sends migrate_cancel, or QEMU quits and
   migration_shutdown() calls migration_cancel().

In that case cpr_transfer_source_active(s) is true but migrate_mode()
is MIG_MODE_NORMAL, so assert(migrate_mode() == MIG_MODE_CPR_TRANSFER)
fails and the whole source QEMU aborts.

The old code in this window did not depend on the mode. Would it be
better to drop this assert, since hup_source being set already implies
that cpr-transfer armed it?

> +        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/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=21