Re: [PATCH QEMU HCI-8.1 v4 19/31] migration: Fix crash on second migration when cancel early

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] migration: cancel in cpr-transfer HUP window sends FAILED without SETUP, can abort via VFIO notifier
- [Medium] migration: migration_cancel() asserts on mode that migrate-set-parameters can change
- [Low] migration: qmp_migrate_finish_cb() comment refers to a nonexistent redundant destroy
--

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

migration: Fix crash on second migration when cancel early

This commit makes migration_cancel() call migration_cleanup() when the
cpr-transfer HUP gsource is still pending. It also detaches hup_source
at entry to qmp_migrate_finish_cb(), and adds some assertions. This
fixes a crash when a cancelled early-setup migration is retried.

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
> index b7db5c94b0d9..3732751e66c7 100644
> --- 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);
> +        assert(setup && !s->to_dst_file);
> +        migration_cleanup(s);
> +        /* Now all things should have been released */
> +        assert(!cpr_transfer_source_active(s));
>      }
>  }

[Severity: High]
Can this migration_cleanup() call abort QEMU through a VFIO notifier?

In the cpr-transfer HUP-wait window, qmp_migrate() has only called
cpr_transfer_add_hup_watch() and returned. migration_connect() has not
run, so migration_call_notifiers(MIG_EVENT_PRECOPY_SETUP) has not been
delivered. The SETUP handler vfio_cpr_pci_notifier(), which disables the
MSI/INTx irqfds, has therefore not run.

If migrate_cancel arrives in this window, migration_cancel() calls
migration_cleanup(), which ends with:

    type = migration_has_failed(s) ? MIG_EVENT_PRECOPY_FAILED :
                                     MIG_EVENT_PRECOPY_DONE;
    migration_call_notifiers(type, NULL);

For MIG_EVENT_PRECOPY_FAILED, vfio_cpr_pci_notifier() calls
vfio_cpr_set_msi_virq(vdev, errp, true). That re-adds irqfds which were
never removed. If KVM_IRQFD assign rejects the duplicate, the notifier
returns non-zero. I have not verified the kernel behaviour for a
duplicate assign.

migration_call_notifiers() then reaches:

    ret = notifier->notify(notifier, &e, errp);
    if (ret) {
        assert(type == MIG_EVENT_PRECOPY_SETUP);

That assert would fire for a FAILED event and abort the running source
QEMU. The old code in this path sent no notifier events.

Is it intended that FAILED is delivered here without a preceding SETUP,
and is vfio_cpr_pci_notifier() expected to tolerate that? The commit
message does not mention this side effect.

[Severity: Medium]
Can assert(migrate_mode() == MIG_MODE_CPR_TRANSFER) fire here?

migrate_mode() reads the live s->parameters.mode. migrate_params_apply()
applies has_mode with no check for a running migration:

    if (params->has_mode) {
        s->parameters.mode = params->mode;
    }

qmp_migrate() samples the mode only once, when it calls
cpr_transfer_add_hup_watch(), and does not record it. Consider this
sequence:

    qmp_migrate()                      /* cpr-transfer, hup_source set */
    migrate-set-parameters mode=normal
    migrate_cancel  (or migration_shutdown() at exit)
      migration_cancel()
        cpr_transfer_source_active(s)  /* true */
        assert(migrate_mode() == MIG_MODE_CPR_TRANSFER)  /* fails */

That would abort QEMU in a case where the old code just cancelled and
cleaned up. The cleanup itself does not depend on the mode. Is this
assertion needed?

> @@ -2266,11 +2271,22 @@ static gboolean qmp_migrate_finish_cb(QIOChannel *channel,
>                                        void *opaque)
>  {
>      MigrationAddress *addr = opaque;
> +    MigrationState *s = migrate_get_current();
> +
> +    /*
> +     * Detach and release the GSource right after use.  We rely on this to
> +     * detect this small cpr-transfer window of "waiting for HUP event".
> +     */
> +    cpr_transfer_source_destroy(s);
>  
>      qmp_migrate_finish(addr, NULL);
>  
>      cpr_state_close();
> -    cpr_transfer_source_destroy(migrate_get_current());
> +
> +    /*
> +     * This is redundant as we do cpr_transfer_source_destroy() at the
> +     * entry, but it's benign; glib will just skip the detach.
> +     */
>      return G_SOURCE_REMOVE;
>  }

[Severity: Low]
This isn't a bug, but the new comment before return G_SOURCE_REMOVE says
"This is redundant as we do cpr_transfer_source_destroy() at the entry".
No second cpr_transfer_source_destroy() call remains in
qmp_migrate_finish_cb(), because the patch removes the trailing one.

The comment seems to describe code that is not present in this backport.
Could it be reworded or dropped?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261020-vfio_v1-v4-0-83a5668230e5@virtuozzo.com?part=19