Re: [PATCH QEMU HCI-8.1 v5 18/36] migration: yank: Move register instance earlier

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] migration: yank instance leaked when cpr-transfer is cancelled during HUP wait
--

commit 19f402394221e6bd3b3aceafbab4a2479e855a39
Author: Fabiano Rosas <farosas@suse.de>

migration: yank: Move register instance earlier

This moves yank_register_instance() from qmp_migrate_finish() into
migrate_prepare(), so that it can be paired with the unregister in
migration_cleanup(). It also adds unregister calls on the qmp_migrate()
error path and refuses migrate while an incoming postcopy is in progress.

Link: https://lore.kernel.org/qemu-devel/20260123141656.6765-11-farosas@suse.de

> diff --git a/migration/migration.c b/migration/migration.c
> index d5a21da3129a..cc88f70ae593 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c

[ ... ]

> @@ -2243,15 +2252,19 @@ static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
>          }
>      }
>  
> +    if (!yank_register_instance(MIGRATION_YANK_INSTANCE, errp)) {
> +        return false;
> +    }
> +
>      if (migrate_init(s, errp)) {
> +        yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>          return false;
>      }
>  
>      return true;
>  }

[ ... ]

> @@ -2366,27 +2377,26 @@ void qmp_migrate(const char *uri, bool has_channels,
>                          QAPI_CLONE(MigrationAddress, addr));
>  
>      } else {
> -        qmp_migrate_finish(addr, resume_requested, errp);
> +        qmp_migrate_finish(addr, errp);
>      }
>  
>  out:
>      if (local_err) {
> +        /* A resume didn't register it, see migrate_prepare() */
> +        if (s->state != MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP) {
> +            yank_unregister_instance(MIGRATION_YANK_INSTANCE);
> +        }
>          migration_connect_set_error(s, local_err);
>          error_propagate(errp, local_err);
>      }
>  }

[Severity: Medium]
Can this leak the yank instance when a cpr-transfer migration is
cancelled while qmp_migrate() is waiting for the HUP event?

In cpr-transfer mode the call chain is:

qmp_migrate()
    migrate_prepare()
        yank_register_instance(MIGRATION_YANK_INSTANCE, errp)
        migrate_init()           /* state is now SETUP */
    cpr_state_save()
    migrate_hup_add(s, cpr_state_ioc(), qmp_migrate_finish_cb, ...)
    /* returns, qmp_migrate_finish() not called yet */

The instance is now registered during the whole HUP wait. Before this
patch it was registered in qmp_migrate_finish(), so nothing was held
during that window.

If the user then issues migrate_cancel, migration_cancel() sees setup
true and s->to_dst_file == NULL, and takes this branch:

migration/migration.c:migration_cancel() {
    ...
    if (setup && !s->to_dst_file) {
        migrate_set_state(&s->state, MIGRATION_STATUS_CANCELLING,
                          MIGRATION_STATUS_CANCELLED);
        cpr_state_close();
        migrate_hup_delete(s);
    }
    ...
}

This branch does not call migration_cleanup(), and migrate_hup_delete()
destroys the source, so qmp_migrate_finish_cb() never runs either. None
of the unregister sites reached from here apply: the migrate_init()
failure branch in migrate_prepare(), the out: label in qmp_migrate(), the
error branch in qmp_migrate_finish(), and the unregister in
migration_cleanup().

The registration then stays in place, so every later migrate command
would fail in migrate_prepare() with "duplicate yank instance" until QEMU
is restarted. This also means the pairing with migration_cleanup() that
the commit message describes does not hold on this path.

This appears to be fixed later in the series. At the series end,
migration_cancel() checks cpr_transfer_source_active(s) and calls
migration_cleanup(s), which ends with yank_unregister_instance(). That
comes from the follow-up patches "migration: Move CPR HUP watch to
cpr-transfer.c" and "migration: Fix crash on second migration when cancel
early".

At this commit, though, cpr-transfer cancel during the HUP wait leaks the
instance, which affects bisectability. Could the cancel path be made to
unregister the instance here, or could these patches be reordered?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261023-vfio_v1-v5-0-ba0bf3c6d122@virtuozzo.com?part=18