Re: [PATCH QEMU HCI-8.1 v3 14/28] 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: stale incoming_postcopy_state makes migrate_prepare() refuse all later outgoing migrations
- [Medium] migration: migrate_prepare() registers yank with &error_abort, aborting QEMU if cleanup is pending
--

Commit ce0aafa8f0f4f6ebc0a21db9c39794339fc1b277
Author: Fabiano Rosas <farosas@suse.de>

migration: yank: Move register instance earlier

This commit moves yank_register_instance() into migrate_prepare() so it
pairs with the unregister in migration_cleanup(). It also adds a guard
that refuses a non-resume migrate while an incoming postcopy migration is
in progress, and reworks the error-path unregister logic in
qmp_migrate() and qmp_migrate_finish().

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..0b8309565323 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c

[ ... ]

> @@ -2174,6 +2174,15 @@ static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
>          return false;
>      }
>  
> +    /*
> +     * The guest already runs during incoming postcopy, but the incoming
> +     * migration still holds the yank instance registered below.
> +     */
> +    if (migration_incoming_postcopy_advised()) {
> +        error_setg(errp, "Incoming postcopy migration is still in progress");
> +        return false;
> +    }
> +

[Severity: High]
Can this guard reject every later outgoing migrate on a VM whose incoming
migration has already finished?

migration_incoming_postcopy_advised() evaluates:

    PostcopyState ps = postcopy_state_get();

    return ps >= POSTCOPY_INCOMING_ADVISE && ps < POSTCOPY_INCOMING_END;

Only the postcopy listen thread ever sets POSTCOPY_INCOMING_END.  Consider
this sequence on the destination with postcopy-ram enabled:

loadvm_postcopy_handle_advise()
    postcopy_state_set(POSTCOPY_INCOMING_ADVISE);
    (the len == 0 case with !migrate_postcopy_ram() also returns 0
     without resetting the state)

Precopy then completes without postcopy starting, so LISTEN and the
listen thread never run:

process_incoming_migration_co()
    if (ps == POSTCOPY_INCOMING_ADVISE) {
        postcopy_ram_incoming_cleanup(mis);
    }
    ...
process_incoming_migration_bh()
    migration_incoming_state_destroy()   <- yank instance released here

postcopy_ram_incoming_cleanup() does not reset the state, and none of the
other postcopy_state_set() callers run on this path or on the ret < 0
fail: path.  The state therefore stays at ADVISE (or DISCARD, LISTENING or
RUNNING after some failures).

After that, migration_incoming_postcopy_advised() stays true, and every
non-resume QMP migrate returns "Incoming postcopy migration is still in
progress", even though the incoming side no longer holds the yank
instance.  Chained migration (A->B->C) looks like a realistic trigger.
The refusal appears to last until a new incoming migration starts or QEMU
is restarted.

I did not find anything later in the series that resets the state or
narrows this check.

Should the guard test whether the incoming side still holds the yank
instance, or should the postcopy state be reset when incoming migration
finishes or fails?

[ ... ]

> @@ -2235,11 +2244,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 instead of returning a QMP error when the previous
outgoing migration's cleanup has not run yet?

The unregister is the last step of migration_cleanup(), which runs only
from the bottom half scheduled in migration_iteration_finish():

migration_thread()
    s->state set to FAILED (or COMPLETED)
migration_iteration_finish()
    bql_lock();
    ...
    migration_bh_schedule(migration_cleanup_bh, s);
    bql_unlock();

Between the terminal state change and the bottom half running, the main
loop can process a QMP migrate command.  In that window
migration_is_running() returns false, since FAILED, CANCELLED and
COMPLETED fall into its default case:

    case MIGRATION_STATUS_CANCELLING:
    case MIGRATION_STATUS_COLO:
        return true;
    default:
        return false;

The RUN_STATE_POSTMIGRATE check does not catch it either, as that is set
later in migration_iteration_finish().  migrate_init() does not look at a
pending cleanup.  So migrate_prepare() can reach:

    yank_register_instance(MIGRATION_YANK_INSTANCE, &error_abort);

while the old instance is still registered.  yank_register_instance() then
does error_setg(errp, "duplicate yank instance"), which with &error_abort
calls abort().

Before this patch the registration in qmp_migrate_finish() used the
caller's errp, so the same race produced a QMP error.

The commit message adds a guard for the incoming postcopy case for this
reason, but nothing similar covers the outgoing cleanup-pending window.
The later patches through the series end do not change this.

Could this use a non-fatal errp and return an error from migrate_prepare()
instead, or could migrate_prepare() reject the request while cleanup is
still pending?

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261016-vfio_v1-v3-0-47487cb5a8e5@virtuozzo.com?part=14