Re: [PATCH QEMU HCI-8.1 v5 03/36] vfio/region: create BAR dma-bufs with O_CLOEXEC

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] vfio: BAR dma-buf fd stored in RAMBlock->fd is never closed
--

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

vfio/region: create BAR dma-bufs with O_CLOEXEC

This patch adds O_CLOEXEC to the open_flags passed when
vfio_region_create_dma_buf() requests a dma-buf fd for a BAR region. The
aim is to stop those fds from leaking into exec'ed programs, for example
with cpr-exec.

> diff --git a/hw/vfio/region.c b/hw/vfio/region.c
> index 1deaef12fd0b8..b0332f27fcd79 100644
> --- a/hw/vfio/region.c
> +++ b/hw/vfio/region.c

[ ... ]

> @@ -258,7 +258,7 @@ static bool vfio_region_create_dma_buf(VFIORegion *region, Error **errp)
>      dma_buf = (void *)feature->data;
>      *dma_buf = (struct vfio_device_feature_dma_buf) {
>          .region_index = region->nr,
> -        .open_flags = O_RDWR,
> +        .open_flags = O_RDWR | O_CLOEXEC,
>          .nr_ranges = region->nr_mmaps,
>      };

[Severity: Medium]
The commit message says "With cpr-exec, every update leaks one fd per
mmapped BAR". Does this change fix the leak completely? It looks like the
dma-buf fd is still never closed inside the running process.

vfio_region_create_dma_buf() stores the single returned fd in every
sub-region RAMBlock:

    ram_block->fd = ret;
    ram_block->fd_offset = region->mmaps[i].offset;

These RAMBlocks come from memory_region_init_ram_device_ptr() via
qemu_ram_alloc_from_ptr(), so they have RAM_PREALLOC set. In
reclaim_ramblock() that case takes the empty branch, so close(block->fd)
is never reached:

system/physmem.c:reclaim_ramblock() {
    if (block->flags & RAM_PREALLOC) {
        ;
    } else if (xen_enabled()) {
        ...
    } else if (block->fd >= 0) {
        qemu_ram_munmap(block->fd, block->host, block->max_length);
        close(block->fd);
    ...
}

The region teardown paths do not close it either. vfio_subregion_unmap(),
vfio_region_exit() and vfio_region_finalize() only munmap() the BAR:

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

hw/vfio/region.c has no close() call for this fd. Each mmap/unmap cycle,
device unplug or realize/unrealize therefore appears to leak one fd per
region, together with the kernel dma-buf object behind it. O_CLOEXEC does
not change that.

The one fd is also shared by all nr_mmaps RAMBlocks, so it has no single
owner. If RAM_PREALLOC were ever dropped, reclaim_ramblock() would call
close(block->fd) once per sub-region, which would be a double close when
nr_mmaps > 1.

Should the fd be closed explicitly when the region is unmapped or
finalized, and should the commit message be limited to the leak across
exec?

The series patch "VZ: vfio/region: preserve BAR mappings across cpr-exec"
only adds cpr ramblock save/delete of the mmap addresses. It does not
close the dma-buf fd either.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261023-vfio_v1-v5-0-ba0bf3c6d122@virtuozzo.com?part=3