Re: [PATCH QEMU HCI-8.1 v5 21/36] 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 migrate_mode()==CPR_TRANSFER, but mode is mutable
--

commit d8e2c80dd170f3a8f2343c0a4b7abbebb7c94fc5
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. Before, it used a special case
that checked setup && !s->to_dst_file. It also makes
qmp_migrate_finish_cb() detach the HUP gsource on entry, and it 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 on a legitimate cancel? assert() is g_assert()
here, and migrate_mode() reads s->parameters.mode, which can be changed
while the migration is running.

migrate_params_apply() writes s->parameters.mode whenever has_mode is set.
Neither qmp_migrate_set_parameters() nor migrate_params_check() rejects
that while a migration is running.

The hup_source is armed in qmp_migrate() while migrate_mode() is
MIG_MODE_CPR_TRANSFER, and the mode is not latched at that point:

    if (migrate_mode() == MIG_MODE_CPR_TRANSFER) {
        cpr_transfer_add_hup_watch(s, qmp_migrate_finish_cb, addr);
    }

A possible sequence is:

    migrate (mode=cpr-transfer)    -> hup_source armed, state is SETUP
    migrate-set-parameters mode=normal (or cpr-exec)
    migrate_cancel                 -> migration_cancel()

migration_cancel() sees cpr_transfer_source_active(s) true, but
migrate_mode() no longer returns MIG_MODE_CPR_TRANSFER, so the assert
fails.

The same assert would also run from migration_shutdown() at QEMU exit,
and from the ram.c resize-during-precopy path, if the mode was changed
in the window. The window lasts until the destination closes the CPR
channel.

The old check was only setup && !s->to_dst_file. The new check makes the
cancel path depend on a user-mutable parameter.

Would it be better to drop this assert, or to record the mode when
hup_source is armed and check that instead?

> +        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/20261023-vfio_v1-v5-0-ba0bf3c6d122@virtuozzo.com?part=21