Re: [PATCH QEMU HCI-8.1 10/21] migration: yank: Move register instance earlier

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] migration: yank instance leaked when qmp_migrate_finish() fails synchronously, aborting next migrate
- [Medium] migration: unconditional yank_unregister_instance() in qmp_migrate() out: path on resume
--

commit 8b6d573d4b1ea369ae6968e2938e2e1b1c4366c3
Author: Fabiano Rosas <farosas@suse.de>

migration: yank: Move register instance earlier

This commit 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 drops the resume_requested argument from
qmp_migrate_finish() and adds a yank_unregister_instance() call to the
local_err path at the end of qmp_migrate().

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

> diff --git a/migration/migration.c b/migration/migration.c
> --- a/migration/migration.c
> +++ b/migration/migration.c

[ ... ]

> @@ -2235,11 +2235,12 @@ static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
>          return false;
>      }
>  
> +    yank_register_instance(MIGRATION_YANK_INSTANCE, &error_abort);
> +
>      return true;
>  }

[Severity: High]
Can this call abort QEMU on a retry after a failed migrate command?

If a previous attempt failed synchronously in qmp_migrate_finish(), the
instance registered here is never unregistered (see the comments below).
The state is then FAILED, so migration_is_running() is false and the next
migrate passes the checks in migrate_prepare().

yank_register_instance() then finds the stale entry via yank_find_entry()
and fails with "duplicate yank instance".  Because &error_abort is passed,
QEMU aborts.

A privileged QMP client can reach this with an fd: URI naming a
nonexistent fd name, an exec: command that cannot be spawned, or a file:
target that cannot be opened, followed by any further migrate.

[ ... ]

> @@ -2354,28 +2353,22 @@ 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) {
> +        yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>          migration_connect_set_error(s, local_err);
>          error_propagate(errp, local_err);
>      }
>  }

[Severity: High]
Is this unregister reached when qmp_migrate_finish() fails?

qmp_migrate_finish() is passed errp rather than &local_err, so only a
cpr_state_save() failure sets local_err in qmp_migrate().  Failures from
qmp_migrate_finish() never reach this yank_unregister_instance() call.

[Severity: Medium]
Separately, this unregister is not conditioned on this call having
registered the instance.  When resume is requested, migrate_prepare()
takes the resume branch and returns true before yank_register_instance().

cpr_state_save() looks only at migrate_mode() and not at the resume flag.
If mode is cpr-exec or cpr-transfer and cpr_state_save() fails (for
example cpr_exec_output() failing), the out: block calls
yank_unregister_instance() on the instance owned by the paused postcopy
migration.

Would the later migration_cleanup() then call yank_unregister_instance()
again and hit assert(entry)?  The old code guarded both register and
unregister with !resume_requested.  Should the unregister here be skipped
for resume?

> -static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
> -                               Error **errp)
> +static void qmp_migrate_finish(MigrationAddress *addr, Error **errp)
>  {
>      MigrationState *s = migrate_get_current();
>      Error *local_err = NULL;
>  
> -    if (!resume_requested) {
> -        if (!yank_register_instance(MIGRATION_YANK_INSTANCE, errp)) {
> -            return;
> -        }
> -    }
> -
>      if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {

[ ... ]

>      if (local_err) {
> -        if (!resume_requested) {
> -            yank_unregister_instance(MIGRATION_YANK_INSTANCE);
> -        }
>          migration_connect_set_error(s, local_err);
>          error_propagate(errp, local_err);
>          return;

[Severity: High]
Does this error path now leak the yank instance?

The removed yank_unregister_instance() was the only release on this path.
migration_connect_set_error() only calls migrate_set_state() to FAILED and
migrate_set_error(); it does not call migration_cleanup().

When the start function fails before any channel exists, for example
fd_start_outgoing_migration() returning after monitor_get_fd() == -1,
neither migration_channel_connect() nor migration_connect() runs, so
migration_cleanup() is never reached.  Does the instance registered in
migrate_prepare() then stay registered?

The same applies to the cpr-transfer deferred path:

qmp_migrate_finish_cb()
    qmp_migrate_finish(addr, NULL);
    cpr_state_close();
    migrate_hup_delete(migrate_get_current());

No yank unregister is done there after a start failure either.

The later patches in the series change the HUP watch handling and
migration_cancel(), but do not change the yank handling in
qmp_migrate_finish() or qmp_migrate_finish_cb().  The issue is therefore
not fixed within the series.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261009-vfio_v1-v1-0-2ae91d5b8048@virtuozzo.com?part=10