Re: [PATCH QEMU HCI-8.1 21/21] VZ: migration/cpr-exec: allow cpr-exec with VFIO devices #VSTOR-103860

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] migration: cpr-exec VFIO blocker removed with no check that RAM is shared/memfd-backed
- [Medium] vfio: removing vfio_devices_present() leaves vfio-user devices unblocked for cpr-exec
- [Low] migration: cpr-exec with VFIO has no up-front check for MADV_DOEXEC support
- [Low] migration: stale hw/vfio/vfio-device.h include left in migration.c
--

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

VZ: migration/cpr-exec: allow cpr-exec with VFIO devices #VSTOR-103860

This patch removes the check in migrate_cpr_is_blocked() that rejected
cpr-exec migration whenever any VFIO device was present. It also removes
the now unused vfio_devices_present() helper and its declaration.

> diff --git a/hw/vfio/device.c b/hw/vfio/device.c
> index 5a14a7d5c0a2e..9689f40daa172 100644
> --- a/hw/vfio/device.c
> +++ b/hw/vfio/device.c
> @@ -36,11 +36,6 @@
>  VFIODeviceList vfio_device_list =
>      QLIST_HEAD_INITIALIZER(vfio_device_list);
>  
> -bool vfio_devices_present(void)
> -{
> -    return !QLIST_EMPTY(&vfio_device_list);
> -}
> -
>  /*
>   * We want to differentiate hot reset of multiple in-use devices vs
>   * hot reset of a single in-use device. VFIO_DEVICE_RESET will already

[ ... ]

> diff --git a/migration/migration.c b/migration/migration.c
> index ab1f1641c2170..c513427df4367 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2103,9 +2103,6 @@ static bool migrate_cpr_is_blocked(Error **errp)
>      } else if (cbw_filter_present()) {
>          error_setg(errp, "CPR is prohibited: active block snapshots (CBW "
>                     "filter) present");
> -    } else if (vfio_devices_present()) {
> -        error_setg(errp, "CPR is prohibited: passthrough (VFIO) "
> -                   "devices present");
>      } else {
>          res = false;
>      }

[Severity: Medium]
Can this corrupt guest memory when the guest RAM is anonymous or private?

With the VFIO branch gone, migrate_cpr_is_blocked() accepts cpr-exec for
any RAM backend. cpr_exec_cb() then calls cpr_walk_ramblock(preserve_ram),
and preserve_ram() only does:

migration/cpr-exec.c:preserve_ram() {
    if (qemu_madvise((void *) ram.addr, ram.length, QEMU_MADV_DOEXEC)) {
    ...
}

It does not check that the backing is shared or memfd.  ram_block_add()
in system/physmem.c registers anonymous RAM through cpr_save_ramblock()
without any restriction, and cpr_exec_cb() then calls execvp() with the
VFIO mappings still pinned.

The sibling commit "VZ: physmem: align fd-backed RAM like anonymous RAM"
says anonymous RAM preserved via MADV_DOEXEC and pinned by
VFIO_IOMMU_MAP_DMA is replaced by copies on first guest write after
exec(), while the device keeps doing DMA to the old pinned pages.  Would
that leave device DMA and guest memory incoherent?

The remaining VFIO blockers do not seem to cover this.  In
hw/vfio/cpr-legacy.c, vfio_cpr_supported() only checks VFIO_UPDATE_VADDR
and VFIO_UNMAP_ALL.  In hw/vfio/cpr-iommufd.c, it only checks
IOMMU_IOAS_CHANGE_PROCESS.  The qemu_ram_is_shared() check in
hw/vfio/container.c only selects IOMMU_IOAS_MAP_FILE for iommufd, and the
legacy container has no equivalent.

The sibling commit relies on using memory-backend-memfd for such VMs, but
QEMU does not enforce that.  Should the blanket VFIO blocker be replaced
with one that rejects cpr-exec when a VFIO device is present and any
RAMBlock is not shared and fd-backed?

[Severity: Low]
Is there an up-front check for MADV_DOEXEC support on the host kernel?

With the VFIO check removed, migrate_cpr_is_blocked() is the only gate
and it no longer verifies this.  On a kernel without MADV_DOEXEC support
for shared PFN mappings of device memory, the failure shows up late:

migration/cpr-exec.c:cpr_exec_cb()
  cpr_walk_ramblock(preserve_ram)
    preserve_ram()
      qemu_madvise(..., QEMU_MADV_DOEXEC) fails
      migrate_set_state(..., MIGRATION_STATUS_FAILED)

By then the guest has been stopped and its state has been sent.  The
rollback at the fail: label in cpr_exec_cb() looks correct, but could a
capability probe be added so that the migrate command is rejected up
front?  The dependency appears to be documented only in the commit
message of "VZ: vfio/region: preserve BAR mappings across cpr-exec".

[Severity: Medium]
Does removing vfio_devices_present() leave vfio-user devices unblocked
for cpr-exec?

vfio_user_pci_realize() reaches vfio_device_prepare(), which inserts the
device into vfio_device_list.  That list was what vfio_devices_present()
tested, so it used to reject vfio-user devices.

The other blockers do not appear to apply.  vfio-user attaches through
vfio_device_attach_by_iommu_type(TYPE_VFIO_IOMMU_USER) rather than the
legacy container, so it never reaches the mdev blocker in
hw/vfio/container-legacy.c:

    if (vbasedev->mdev) {
        error_setg(&vbasedev->cpr.mdev_blocker,
                   "CPR does not support vfio mdev %s", vbasedev->name);
        if (migrate_add_blocker_modes(&vbasedev->cpr.mdev_blocker, errp,
                                      MIG_MODE_CPR_TRANSFER, MIG_MODE_CPR_EXEC,
                                      -1) < 0) {

A grep for blocker, cpr and CPR in hw/vfio-user/ finds no matches.  Can a
cpr-exec migration now proceed with a vfio-user-pci device attached,
even though its connection to the external server has no CPR support?
Should a blocker be added for vfio-user devices?

[Severity: Low]
This isn't a bug, but is the include of hw/vfio/vfio-device.h in
migration/migration.c still needed?

It appears to have been used only for vfio_devices_present().  The
remaining VFIO helpers used in that file, migration_populate_vfio_info()
and migration_reset_vfio_bytes_transferred(), are declared in
migration/migration.h.  This has not been confirmed with a build, so
could the include be dropped if it compiles cleanly?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261009-vfio_v1-v1-0-2ae91d5b8048@virtuozzo.com?part=21