Re: [PATCH QEMU HCI-8.1 v7 26/35] vfio/migration: don't migrate device state during CPR

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] vfio/migration: cpr_is_in_progress() is false on cpr-transfer source during the stream
- [Medium] vfio/migration: cpr_is_in_progress() true before migration starts skips stop/cont device state
--

commit 67f4499a74b760de74e49d3ef3311d53eec749fd
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

vfio/migration: don't migrate device state during CPR

This patch gates the VFIO migration handlers on cpr_is_in_progress() so
that cpr-transfer and cpr-exec no longer save, load or change the state
of a preserved VFIO device. It also resets multifd_transfer in
vfio_multifd_cleanup().

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

[ ... ]

> @@ -674,6 +674,16 @@ static void vfio_save_state(QEMUFile *f, void *opaque)
>      Error *local_err = NULL;
>      int ret;
>  
> +    /*
> +     * .is_active doesn't cover the non-iterable stage, so the section is
> +     * still sent: leave it empty, or new QEMU would load the config into
> +     * the preserved device and re-program its MSI-X.
> +     */
> +    if (cpr_is_in_progress()) {
> +        qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
> +        return;
> +    }
> +

[Severity: High]
Is cpr_is_in_progress() still true on a cpr-transfer source by the time
this runs?

On the source, cpr_is_in_progress() ends with:

    return cpr_state_file != NULL;

For cpr-transfer, qmp_migrate() calls cpr_state_save() and then
cpr_transfer_add_hup_watch(). When the target closes its CPR socket,
qmp_migrate_finish_cb() runs:

    qmp_migrate_finish(addr, NULL);

    cpr_state_close();

qmp_migrate_finish() only starts the asynchronous connect.
cpr_state_close() then does:

    qemu_fclose(cpr_state_file);
    cpr_state_file = NULL;

The migration thread is started later, from socket_outgoing_migration()
via migration_channel_connect(). So by the time save_setup, vm_stop,
vfio_save_state(), the switchover-ack check and save_complete run on the
source, cpr_state_file appears to be NULL. The source has no incoming
mode, so cpr_is_incoming() is false and cpr_is_in_progress() returns
false.

On the target, cpr_is_incoming() is true, so cpr_is_in_progress() returns
true. The two sides then disagree:

vfio_is_active() returns true on the source, so the source sends the full
config section, and vfio_vmstate_change() stops the device.

The target skips load_setup and vfio_vmstate_change(), but
vfio_load_state() has no is_active check, so it may still process the
config and buffer sections. That could re-program MSI-X on the preserved
device, use an uninitialised data_fd, or leave the device stopped and
never restarted.

For fd transport the connect is not asynchronous, so the migration thread
may race with cpr_state_close().

Can the source-side predicate be made to stay true for the whole outgoing
cpr-transfer stream? As far as I can tell, cpr-exec is not affected,
since its cpr_state_file stays open until migration_cleanup(). The
later patches in the series do not appear to change cpr_is_in_progress(),
cpr_state_close() timing or qmp_migrate_finish_cb().

[ ... ]

> @@ -857,6 +884,11 @@ static void vfio_vmstate_change_prepare(void *opaque, bool running,
>      Error *local_err = NULL;
>      int ret;
>  
> +    /* The device is preserved, not a plain stop/cont: keep its state */
> +    if (cpr_is_in_progress()) {
> +        return;
> +    }
> +
>  
> @@ -882,6 +914,10 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state)
>      Error *local_err = NULL;
>      int ret;
>  
> +    if (cpr_is_in_progress()) {
> +        return;
> +    }
> +

[Severity: Medium]
Can these early returns skip a plain stop/cont on the source before the
migration has started?

On the source, cpr_is_in_progress() becomes true as soon as
qmp_migrate() calls cpr_state_save() and sets cpr_state_file. For
cpr-transfer, qmp_migrate() then only adds the HUP watch and returns.
The real migration starts only in qmp_migrate_finish_cb(), after the
target closes the CPR socket. That window is not bounded.

During the window the migration state is still SETUP and no savevm
handlers have run. qmp_cont() still accepts stop and cont. Both
vfio_vmstate_change_prepare() and vfio_vmstate_change() return early.

For example, if the VM is paused, the device is in STOP. The user runs
migrate with cpr-transfer and then cont before the target connects.
vm_start() resumes the vCPUs, but vfio_vmstate_change() skips the
RUNNING transition, so the guest runs against a device left in STOP. If
the migration is then cancelled or fails, cpr_state_close() clears the
predicate, but nothing restores the device state. A stop inside the
window can similarly leave the device RUNNING.

The commit message says "the mode may merely be set, and then a plain
stop/cont must still change the device state". Does that hold for this
window, where cpr_state_file is already set but no migration is
running? The later patches in the series appear to add only a
cpr_is_incoming() branch for the target in vfio_vmstate_change(), so the
source keeps the same early return.

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