Re: [PATCH QEMU HCI-8.1 v2 11/24] 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: migrate_prepare() aborts QEMU on duplicate yank instance
--

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

migration: yank: Move register instance earlier

This patch moves yank_register_instance() from qmp_migrate_finish() into
migrate_prepare(), so that it pairs with the unregister in
migration_cleanup(). The resume case is now detected from the migration
state instead of a resume_requested argument.

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

> diff --git a/migration/migration.c b/migration/migration.c
> index 2f8ef20c0369..fb83e8bc695d 100644
> --- 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: Medium]
Can this abort QEMU when a yank instance is already registered?

Before this patch, a duplicate registration was reported through errp
from qmp_migrate_finish() as "duplicate yank instance".  Now
migrate_prepare() passes &error_abort, so the duplicate check in
yank_register_instance() ends in abort():

util/yank.c:yank_register_instance() {
    if (yank_find_entry(instance)) {
        error_setg(errp, "duplicate yank instance");
        return false;
    }
}

The incoming side registers the same global MIGRATION_YANK_INSTANCE in
qmp_migrate_incoming(), and it keeps it until
migration_incoming_state_destroy():

migration/migration.c:migration_incoming_state_destroy() {
    ...
    cpr_set_incoming_mode(MIG_MODE_NONE);
    yank_unregister_instance(MIGRATION_YANK_INSTANCE);
}

For postcopy, that destroy happens at the end of postcopy from the
listen thread.  If the incoming side is in POSTCOPY_PAUSED, it can wait
indefinitely.  By then the guest is running, so the runstate is no
longer RUN_STATE_INMIGRATE.

The guards earlier in migrate_prepare() are migration_is_running(), which
only looks at the outgoing current_migration, RUN_STATE_INMIGRATE, and
RUN_STATE_POSTMIGRATE.  None of them looks at incoming state.

A QMP migrate issued in that window would pass all of them, run
migrate_init(), and then hit yank_register_instance() with &error_abort.

This needs a nested or chained migration setup and a privileged QMP
command, so it should be rare.  The upstream commit 7e714392437c has the
same &error_abort.  I did not find a guard against the overlap, and
nothing later in this series adds one.  The final series state still has
&error_abort in migrate_prepare(), while qmp_migrate_incoming() and
load_snapshot() still pass errp.

Could the registration here report the failure through errp instead, with
migrate_prepare() returning false?  Alternatively, could the incoming
state be checked before registering?

The commit message also does not mention that a duplicate registration
changes from a recoverable error into an abort.

[ ... ]

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