Re: [PATCH hci-dev v6 1/2] vfio/migration: stop a device left running after a guest shutdown #VSTOR-153976

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] vfio/migration: cached device_state is stale after reset of a stopped VM
- [Low] vfio/migration: commit message and comment describe RUNNING_P2P state that only exists after patch 2
- [Low] vfio: stop_after_shutdown() stops RUNNING devices to STOP with no P2P quiesce round
--

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

vfio/migration: stop a device left running after a guest shutdown

This commit adds a helper that stops a VFIO device left in RUNNING_P2P, or
in RUNNING while the VM is in SHUTDOWN. The helper is called from a new
shutdown notifier and at the start of vfio_save_setup(), so a device is
not closed or saved while running after a -no-shutdown guest shutdown.

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

[ ... ]

> +/*
> + * Stop a device that a guest shutdown left running, before it is closed or
> + * saved. Only that leaves a device in RUNNING_P2P, or in RUNNING while the
> + * VM is in SHUTDOWN. The devices are stopped one by one: a device in
> + * RUNNING_P2P has been through the P2P round of the VM stop, a device in
> + * RUNNING was set there by a failed migration, which has no such round.
> + */
> +static int vfio_migration_stop_after_shutdown(VFIODevice *vbasedev,
> +                                              Error **errp)
> +{
> +    VFIOMigration *migration = vbasedev->migration;
> +
> +    if (migration->device_state != VFIO_DEVICE_STATE_RUNNING_P2P &&
> +        !(runstate_check(RUN_STATE_SHUTDOWN) &&
> +          migration->device_state == VFIO_DEVICE_STATE_RUNNING)) {
> +        return 0;
> +    }

[Severity: Medium]
Can a device still be closed or saved running after a reset of the stopped
VM? The stop decision uses only the cached migration->device_state.

At this commit, a VM stop with RUN_STATE_SHUTDOWN runs
vfio_vmstate_change(), which moves the device to STOP and caches STOP.

A later reset goes through qemu_system_reset() -> qemu_devices_reset() ->
vfio_pci_reset() -> ioctl(VFIO_DEVICE_RESET). This path does not go through
vm_state_notify() and does not update migration->device_state.

By contrast, the reset_device path in vfio_migration_set_state() does
refresh the cache:

    if (ioctl(vbasedev->fd, VFIO_DEVICE_RESET)) {
        hw_error(...);
    }

    vfio_migration_set_device_state(vbasedev, VFIO_DEVICE_STATE_RUNNING);

If the reset returns the device to RUNNING in the kernel, the cache would
still say STOP. vfio_migration_stop_after_shutdown() would then match
neither RUNNING_P2P nor (SHUTDOWN and RUNNING), since the run state is
PRELAUNCH, and return 0. The device would then be closed or saved running,
which is what the patch aims to prevent.

The kernel-side behaviour of VFIO_DEVICE_RESET on a migration-capable
device was not verified here.

[Severity: Low]
Is the commit message accurate for this patch on its own? It says:

    A reset of the stopped VM leaves the device in RUNNING_P2P. Nothing
    else does, so a device in that state is stopped in any run state.
    Today only a failed migration of such a VM leaves a device running
    there. The next patch does it at every guest shutdown.

The code comment says "Only that leaves a device in RUNNING_P2P".

At this commit, vm_state_notify(false) runs vfio_vmstate_change_prepare(),
which sets RUNNING_P2P, and then vfio_vmstate_change(), which sets STOP in
the same pass:

    new_state =
        (vfio_device_state_is_precopy(vbasedev) &&
         (state == RUN_STATE_FINISH_MIGRATE || state == RUN_STATE_PAUSED)) ?
            VFIO_DEVICE_STATE_STOP_COPY :
            VFIO_DEVICE_STATE_STOP;

A reset therefore does not find the device in RUNNING_P2P here. The
RUNNING_P2P arm of the new helper is unreachable until the follow-up patch
adds an early return in vfio_vmstate_change() for the guest-shutdown case.

Also, "Only that leaves a device in RUNNING_P2P" does not seem strictly
true, since a failed vfio_vmstate_change() on VM start can leave
RUNNING_P2P with the VM running.

Could the commit message and comment be reworded, or this patch be ordered
after the one that makes the RUNNING_P2P case reachable?

> +
> +    return vfio_migration_set_state_or_reset(vbasedev, VFIO_DEVICE_STATE_STOP,
> +                                             errp);
> +}

[Severity: Low]
Is it safe to move a device in RUNNING directly to STOP here? This bypasses
the RUNNING -> RUNNING_P2P step that vfio_vmstate_change_prepare() runs for
all devices before any device is stopped.

The helper is called per device, from vfio_migration_shutdown_notifier()
and from each device's vfio_save_setup(), so other devices may still be
RUNNING when one is stopped.

For example, with -no-shutdown, a migration of a SHUTDOWN VM fails.
vfio_migration_state_notifier() then sets every device to RUNNING with no
P2P round. A following quit or a new save moves one RUNNING device straight
to STOP while its peers are still RUNNING, so a running peer could issue
P2P DMA to a stopped device.

The "one by one" comment does not address this. The next patch only adds a
skip in vfio_vmstate_change() and leaves this RUNNING branch unchanged.

Would moving all RUNNING devices to RUNNING_P2P first, as
vfio_vmstate_change_prepare() does, be needed before stopping any of them?

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261109204518.220617-1-andrey.drobyshev@virtuozzo.com?part=1