Re: [PATCH QEMU HCI-8.1 20/21] VZ: vfio/region: preserve BAR mappings across cpr-exec #VSTOR-103860

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] vfio: region BAR reuse via CPR ramblock is not gated on cpr_exec_is_incoming()
- [Medium] vfio: stale CPR ramblock entry left after region finalize, reused by cpr_find
- [Low] vfio: BAR mappings in the CPR ramblock list get RAM-only mprotect handling
--

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

VZ: vfio/region: preserve BAR mappings across cpr-exec #VSTOR-103860

This patch saves each VFIO BAR mmap in the CPR ramblock list, so
cpr_exec_cb() marks it with MADV_DOEXEC. The new QEMU then finds the entry
by name and reuses the mapping instead of calling mmap() again. This avoids
the NVIDIA vGPU driver refusing to map a BAR that old QEMU still has
recorded as mapped.

> diff --git a/hw/vfio/region.c b/hw/vfio/region.c
> index 1deaef12fd0b..07027ee79f3c 100644
> --- a/hw/vfio/region.c
> +++ b/hw/vfio/region.c

[ ... ]

> +static char *vfio_region_cpr_name(VFIORegion *region, int index)
> +{
> +    g_autofree char *path = qdev_get_dev_path(region->vbasedev->dev);
> +    return g_strdup_printf("%s%s%s mmaps[%d]", path ?: "", path ? "/" : "",
> +                           memory_region_name(region->mem), index);
> +}
> +
> +static void *vfio_region_cpr_find(VFIORegion *region, int index)
> +{
> +    g_autofree char *name = vfio_region_cpr_name(region, index);
> +    cpr_ramblock_t ram = cpr_find_ramblock(name);
> +
> +    if (ram.addr) {
> +        assert(ram.length == region->mmaps[index].size);
> +    }
> +
> +    return (void *)(uintptr_t)ram.addr;
> +}

[Severity: High]
Should vfio_region_cpr_find() be gated on cpr_exec_is_incoming()?

The lookup is by name only. cpr_state_save() serializes the ramblock list
for both cpr-transfer and cpr-exec, and cpr_state_load() loads it in both
modes:

migration/cpr.c:cpr_state_load() {
    ...
    } else if (channel) {
        mode = MIG_MODE_CPR_TRANSFER;
        cpr_set_incoming_mode(mode);
        f = cpr_transfer_input(channel, errp);
    ...
}

In transfer mode cpr_exec_is_incoming() is false, but the entry is still
found. The address from the old process would then be passed to
memory_region_init_ram_device_ptr() in vfio_region_mmap(). Guest BAR
accesses could then fault or hit unrelated memory.

In the opposite case, an exec-incoming QEMU with a missing entry silently
falls back to mmap(). That brings back the NVIDIA "BAR already mapped"
failure with no clear error.

ram_block_add() in system/physmem.c checks cpr_exec_is_incoming() and
reports an explicit error in both mismatch cases:

    if (cpr_exec_is_incoming()) { ... } else {
        if (ram.addr || ram.length) {
            error_report("CPR mode=exec wasn't requested, but ram block %s was saved...");
            exit(1);

Could the VFIO path have an equivalent check?

[Severity: Low]
Also, vfio_region_cpr_save() puts device MMIO BAR mappings into the generic
RAM preservation list:

> +static void vfio_region_cpr_save(VFIORegion *region, int index)
> +{
> +    g_autofree char *name = vfio_region_cpr_name(region, index);
> +    void *addr = region->mmaps[index].mmap;
> +    size_t size = region->mmaps[index].size;
> +
> +    cpr_save_ramblock(name, (cpr_ramblock_t) { (uintptr_t)addr, size });
> +}

Every cpr_walk_ramblock() user now treats these BARs as guest RAM. With the
debug option -cpr-ram-rdonly, cpr_exec_cb() runs:

    if (cpr_ram_rdonly) {
        cpr_walk_ramblock(mark_ram_rdonly);
    }

This mprotects the BAR mappings read-only. Neither the fail: label nor the
execvp failure path restores RW. cpr_ram_restore_rw() only runs on the
incoming side. After a failed cpr-exec, the next guest MMIO store through a
ram_device pointer could SIGSEGV the old QEMU.

On the incoming side, mark_ram_rw() does this:

    if (qemu_mprotect_rw((void *) ram.addr, ram.length)) {
        ...
        abort();
    }

Could that abort QEMU for a BAR mapped without PROT_WRITE? I did not
confirm that the kernel's VFIO mmap path rejects the upgrade, so this part
is speculative. The behavior is the same as guest RAM already has in this
debug mode, but the commit message does not mention the interaction.

>  static void vfio_subregion_unmap(VFIORegion *region, int index)
>  {
> +    g_autofree char *cpr_name = vfio_region_cpr_name(region, index);
> +
> +    cpr_delete_ramblock(cpr_name);

[Severity: Medium]
Can a stale CPR ramblock entry outlive its BAR mapping?

This is the only place the patch deletes the entry. It is not reached from
vfio_region_exit()->vfio_region_finalize(), which munmap()s the mappings
directly:

hw/vfio/region.c:vfio_region_finalize() {
    ...
    for (i = 0; i < region->nr_mmaps; i++) {
        if (region->mmaps[i].mmap) {
            munmap(region->mmaps[i].mmap, region->mmaps[i].size);
        }
    }
    ...
}

Two paths seem affected.

First, vfio_display_region_update() calls:

    vfio_region_exit(&dpy->region.buffer);
    vfio_region_finalize(&dpy->region.buffer);

It then calls vfio_region_setup() and vfio_region_mmap() again under the
same name "display". vfio_region_cpr_find() returns the stale address, so
mmap() is skipped and a dead pointer goes to
memory_region_init_ram_device_ptr(). If the new size differs,
assert(ram.length == region->mmaps[index].size) aborts QEMU.

Second, PCI device unplug and re-hotplug goes through
vfio_pci_bars_exit()/vfio_bars_finalize()->vfio_region_finalize(). The
stale entry stays in cpr_state.ramblocks, and the next cpr-exec applies
MADV_DOEXEC to an unmapped or reused range.

The fallback cleanup in qemu_ram_free() does not seem to cover this. In the
display case, vfio_region_finalize() never unparents the mmaps[i].mem
child, so that MemoryRegion is not finalized and qemu_ram_free() never
runs. On unplug, qemu_ram_free() runs from object_property_del_all() during
object finalize. By then the qdev path is already unset (see the VSTOR-100254
comment in qemu_ram_unset_idstr()). cpr_name() then yields
"<region> mmaps[i]" without the device path, which does not match
vfio_region_cpr_name().

Should vfio_region_finalize() also call cpr_delete_ramblock() for each
mapping?

>  int vfio_region_mmap(VFIORegion *region)
>  {

[ ... ]

> +        region->mmaps[i].mmap = vfio_region_cpr_find(region, i);
> +        if (region->mmaps[i].mmap) {
> +            goto mapped;
> +        }

The reuse decision here depends only on whether an entry is present. It
does not check the incoming CPR mode and does not check that the old
mapping still exists. See the comments above on vfio_region_cpr_find() and
vfio_subregion_unmap().

[ ... ]

> +        vfio_region_cpr_save(region, i);
>  
> +mapped:
>          name = g_strdup_printf("%s mmaps[%d]",
>                                 memory_region_name(region->mem), i);

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