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

[Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>]

On 11/9/26 22:55, no-reply@sashiko.virtuozzo.com wrote:
> [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.

This is how QEMU behaves today, without this series: the cache says STOP
while the kernel has reset the device. The patch does not change that
sequence. A device that has just been reset is idle, it has no guest
driver and no DMA, so closing or saving it is the same as closing or
saving a freshly started device. The patch is about a device the guest
shutdown left running, not about the cache after a reset.

Updating the cache on reset is not safe either: the NVIDIA vGPU driver
keeps its migration state across a reset, so a paused VM that is reset
and continued would end up with the device in STOP while the guest runs.

> [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.
> [...]
> 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.

That is intended. This patch has to come first, so that no commit in the
series can close a device running. On its own it is a no-op, and the
message says so: "The next patch does it at every guest shutdown."

> 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.

It cannot. vfio_vmstate_change() uses vfio_migration_set_state_or_reset(),
which resets the device when the transition fails and caches RUNNING.
RUNNING_P2P is never left behind by a failure.

> [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.
> [...]
> 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 failure handler itself moves the devices to RUNNING one by one, with
no P2P round, for any stopped VM, today. The VM has been stopped the
whole time, so the guest cannot have started any new DMA. Stopping the
devices one by one afterwards adds nothing to what that path already
does. The comment states exactly this.