Re: [PATCH QEMU HCI-8.1 v5 07/36] migration: don't refuse a blocker over a migration of another mode

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] migration: is_busy() infers running migration mode from mutable migrate_mode()
--

commit 7374a894384272ece1d26f3fa1c922acfae8eea7
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

migration: don't refuse a blocker over a migration of another mode

This patch changes is_busy() so that a running migration only causes a
migration blocker to be refused if the blocker applies to the mode of that
migration. A running snapshot (RUN_STATE_SAVE_VM) is still checked as
before.

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

[ ... ]

> -static bool is_busy(Error **reasonp, Error **errp)
> +static bool is_busy(Error **reasonp, Error **errp, int modes)
>  {
>      ERRP_GUARD();
>  
> -    /* Snapshots are similar to migrations, so check RUN_STATE_SAVE_VM too. */
> -    if (runstate_check(RUN_STATE_SAVE_VM) || migration_is_running()) {
> +    /*
> +     * Snapshots are similar to migrations, so check RUN_STATE_SAVE_VM too.
> +     * A running migration only matters if the blocker applies to its mode.
> +     */
> +    if (runstate_check(RUN_STATE_SAVE_VM) ||
> +        (migration_is_running() && (modes & BIT(migrate_mode())))) {

[Severity: Low]
Can this check pick the wrong mode for the running migration?
migrate_mode() does not return the mode the running migration was
started with:

migration/options.c:migrate_mode() {
    MigMode mode = cpr_get_incoming_mode();

    if (mode == MIG_MODE_NONE) {
        mode = migrate_get_current()->parameters.mode;
    }
    ...
}

It returns either the CPR incoming mode or the live parameter value.
Nothing latches the mode in MigrationState at migrate_init().

The parameter can also be changed while a migration is running:

qmp_migrate_set_parameters()->migrate_params_apply()
    s->parameters.mode = params->mode;

I did not see a migration_is_running() guard on this path.
qmp_migrate_set_capabilities() has such a guard, but the parameters path
does not.

Consider a normal-mode migration that is running when the user sets mode
to cpr-transfer. If migrate_add_blocker_modes() or
migrate_add_blocker_internal() is then called:

  - A blocker for normal mode only is accepted, even though a normal
    migration is running.
  - A blocker for cpr-transfer only is refused with -EBUSY, even though
    the running migration is not a cpr-transfer one.

If cpr_get_incoming_mode() is set while an outgoing migration is running,
migrate_mode() returns the incoming mode, and the same mismatch can occur.

Before this patch is_busy() ignored the mode, so this exposure is new.
migration_is_blocked() already reads migrate_mode() live, so the
convention is not new.

Would it be better to record the mode in MigrationState in
migrate_init() and use that value in is_busy()? Alternatively, could
changes to the mode parameter be rejected while migration_is_running()?

>          error_propagate_prepend(errp, *reasonp,
>                                  "disallowing migration blocker "
>                                  "(migration/snapshot in progress) for: ");

[ ... ]

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