Re: [PATCH QEMU HCI-8.1 v7 08/35] 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() keys off the runtime-mutable migrate_mode() parameter
--

commit 0bf28cc9f7ab6b39aca902a416aa850e785856d5
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 when the blocker covers the mode of that
migration. Snapshots (RUN_STATE_SAVE_VM) are still checked as before.

> diff --git a/migration/migration.c b/migration/migration.c
> index 7c2fd704890e..d5a21da3129a 100644
> --- 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 be fooled by a mode change while a migration is running?

is_busy() now takes the mode from migrate_mode(), which returns
migrate_get_current()->parameters.mode unless cpr_get_incoming_mode() is
set. That is the current user-settable parameter. It is not necessarily the
mode the running migration was started with.

migrate_params_apply() in migration/options.c copies the mode whenever
has_mode is set:

    if (params->has_mode) {
        s->parameters.mode = params->mode;
    }

Unlike the capabilities path, this has no migration_is_running() guard.

Suppose a migration of mode X is running and migrate-set-parameters then
changes the mode to Y. A caller of migrate_add_blocker_modes() whose modes
mask contains X but not Y gets (modes & BIT(Y)) == 0. is_busy() then returns
false, and add_blockers() installs the blocker even though a migration of
mode X is in flight. Before this patch, any running migration caused -EBUSY.

Should the check use a mode recorded when the migration was started, rather
than the mutable parameter?

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

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261103-vfio_v1-v7-0-4467fdccbbe6@virtuozzo.com?part=8