Re: [PATCH QEMU HCI-8.1 v2 14/24] migration: Fix crash on second migration when cancel early

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] migration: cancel in CPR HUP wait fires FAILED without SETUP, tripping notify assert
- [Medium] migration: migration_cancel() asserts on live migrate_mode() while hup_source is set
--

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

migration: Fix crash on second migration when cancel early

This patch changes migration_cancel() so that, when a cpr-transfer
migration is waiting for the HUP event on the CPR channel, it calls
migration_cleanup() instead of directly setting the CANCELLED state. It
also detaches hup_source at the start of qmp_migrate_finish_cb() so the
wait window can be detected, 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
> index 14ff16fa6a4e7..2e4be2db65ddd 100644
> --- 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() returns the live value of
migrate_get_current()->parameters.mode, since cpr_get_incoming_mode() is
NONE on the source.

Consider this sequence:

  qmp_migrate() with mode=cpr-transfer
    cpr_transfer_add_hup_watch()   /* sets s->hup_source, state is SETUP */
  migrate-set-parameters mode=normal
    qmp_migrate_set_parameters()->migrate_params_apply()
        s->parameters.mode = params->mode;
  migrate-cancel
    migration_cancel()
      cpr_transfer_source_active(s) is true
      assert(migrate_mode() == MIG_MODE_CPR_TRANSFER) fails

As far as I can tell, none of migrate_params_test_apply(),
migrate_params_check() or migrate_params_apply() reject a mode change
while a migration is running.

The same assert can be reached at exit, because migration_shutdown()
calls migration_cancel().

Since hup_source being set already implies that cpr-transfer was chosen
when the migration started, is this assertion on the mutable global mode
needed?  Before this patch, migration_cancel() had no such assertion.

The second assertion, assert(setup && !s->to_dst_file), did not appear
to fire in the cases I checked.

> +        assert(setup && !s->to_dst_file);
> +        migration_cleanup(s);

[Severity: High]
Can this path abort QEMU when a VFIO PCI device is present?
migration_cleanup() ends with:

  migration_call_notifiers(MIG_EVENT_PRECOPY_FAILED, NULL);

but in the HUP-wait window migration_connect() has not run yet, so
MIG_EVENT_PRECOPY_SETUP was never delivered:

  qmp_migrate()
    cpr_transfer_add_hup_watch()   /* no SETUP notification yet */
  migrate_cancel
    migration_cancel()
      migration_cleanup()
        migration_call_notifiers(MIG_EVENT_PRECOPY_FAILED, NULL)
          vfio_cpr_pci_notifier()
            vfio_cpr_set_msi_virq(vdev, errp, true)

The FAILED handler in hw/vfio/cpr.c re-adds irqfds that only the SETUP
handler removes:

    if (e->type == MIG_EVENT_PRECOPY_SETUP) {
        return vfio_cpr_set_msi_virq(vdev, errp, false);
    } else if (e->type == MIG_EVENT_PRECOPY_FAILED) {
        return vfio_cpr_set_msi_virq(vdev, errp, true);
    }

With SETUP skipped, the irqfds are still assigned, so a second
KVM_IRQFD assign may fail.  I did not find this in the repository.  It
is from my recollection that KVM rejects a duplicate eventfd with
-EBUSY.

If so, the notifier returns nonzero and migration_call_notifiers() hits:

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

The type is FAILED, so the assert fails.

Can the cancel path avoid sending FAILED to notifiers when SETUP was
never sent?  Other notifiers (virtio-net failover, scsi-block,
vfio-migration) seem to tolerate an unpaired FAILED and do not return
errors.

The same unpaired FAILED already exists in the migration_connect()
error_in path.  This patch adds a new route to it that a user can
trigger easily.

The later patches in the series do not appear to address this.  The
series later removes the VFIO CPR block in migrate_cpr_is_blocked(),
which makes the VFIO plus cpr-transfer configuration reachable.

> +        /* Now all things should have been released */
> +        assert(!cpr_transfer_source_active(s));
>      }
>  }

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261013-vfio_v1-v2-0-433455c0a294@virtuozzo.com?part=14