Re: [PATCH hci-dev v5 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 2 potential issue(s) to consider:
- [Medium] vfio/migration: stop helper misses RUNNING device left in a PAUSED VM
- [Low] vfio: stop_after_shutdown() stops RUNNING devices one by one without a P2P round
--

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

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

This patch adds vfio_migration_stop_after_shutdown(). It stops a VFIO
device that is still in RUNNING_P2P, or in RUNNING while the VM is in
SHUTDOWN. The helper is called from a new shutdown notifier and from
vfio_save_setup().

> 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 P2P round of the VM stop has already run, so the
> + * devices are stopped one by one.
> + */
> +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;
> +    }
> +
> +    return vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_STOP,
> +                                    VFIO_DEVICE_STATE_RUNNING, errp);
> +}

[Severity: Medium]
Can a RUNNING device be left behind in a PAUSED or PRELAUNCH VM here?
The helper stops a RUNNING device only when the run state is exactly
RUN_STATE_SHUTDOWN.

One path that seems to reach this state:

    VM is PAUSED, VFIO device is in STOP
    migration_stop_vm()
      vm_stop_force_state(RUN_STATE_FINISH_MIGRATE)
        (VM not live: runstate_set() only, no vmstate-change
         notification, device stays in STOP)
    migration fails or is cancelled
    migration_cleanup()
      migration_call_notifiers(MIG_EVENT_PRECOPY_FAILED)
        vfio_migration_state_notifier()
          vfio_migration_set_state_or_reset(RUNNING)
    migration_iteration_finish()
      if (runstate_is_live(s->vm_old_state)) {
          ...
      } else {
          if (runstate_check(RUN_STATE_FINISH_MIGRATE)) {
              runstate_set(s->vm_old_state);
          }
      }

For a non-live old state, vm_start() is not called. The device ends up
RUNNING in a PAUSED VM.

On a later quit, vfio_migration_shutdown_notifier() calls
vfio_migration_stop_after_shutdown(). The device state is RUNNING, not
RUNNING_P2P, and the run state is PAUSED, not SHUTDOWN. So the helper
returns 0 and the device is closed running.

A retried migration reaches vfio_save_setup(), which calls the same
helper and also returns 0.

The commit message says "Today only a failed migration of such a VM leaves
a device running there", and the helper comment says "or in RUNNING while
the VM is in SHUTDOWN". Both seem to understate this case, because the
same failed-migration path also leaves a RUNNING device in PAUSED and
PRELAUNCH VMs.

The follow-up patch "vfio/migration: don't stop the device on guest
shutdown" only adds an early return in vfio_vmstate_change(). It doesn't
change the PRECOPY_FAILED handler or the run-state check in this helper.

Should the helper cover this case, or should the PRECOPY_FAILED handler
avoid setting RUNNING when the VM is not running?

[Severity: Low]
Is the comment "The P2P round of the VM stop has already run, so the
devices are stopped one by one" accurate for the RUNNING branch?

A device in plain RUNNING in RUN_STATE_SHUTDOWN did not go through
vfio_vmstate_change_prepare(). It is sent straight to STOP with no
all-device RUNNING_P2P round first.

The path to that state is a failed migration of a -no-shutdown VM:

    vfio_migration_state_notifier()
      /* MIG_EVENT_PRECOPY_FAILED */
      vfio_migration_set_state_or_reset(RUNNING)

This moves every device to plain RUNNING while the VM stays in SHUTDOWN.

A later quit or a new save then reaches
vfio_migration_stop_after_shutdown() for each device in turn. With two or
more P2P-capable devices, one device can go to STOP while a peer is still
RUNNING and able to DMA to it.

I haven't shown that this causes a concrete failure. The comment is only
correct for devices found in RUNNING_P2P. The follow-up patch makes
RUNNING_P2P the common case, but it leaves the RUNNING branch and this
comment unchanged.

[ ... ]

> +static void vfio_migration_shutdown_notifier(Notifier *notifier, void *data)
> +{
> +    VFIOMigration *migration = container_of(notifier, VFIOMigration,
> +                                            shutdown);
> +    Error *local_err = NULL;
> +
> +    /* QEMU is quitting: a device left running must be stopped first */
> +    if (qemu_shutdown_action_is_pause()) {
> +        return;
> +    }
> +
> +    if (vfio_migration_stop_after_shutdown(migration->vbasedev, &local_err)) {
> +        error_report_err(local_err);
> +    }
> +}

[Severity: Low]
Each device is stopped individually by its own notifier, with no
coordination between devices, and a failure is only logged with
error_report_err().

Could a device that fails to stop here still be closed running?

[ ... ]

Please let me know if I have misread any of these paths.

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