Re: [RFC PATCH 3/7] vfio/pci: Support mmap() of a DMABUF

[Christian König]

On 2/26/26 21:21, Matt Evans wrote:
> A VFIO DMABUF can export a subset of a BAR to userspace by fd; add
> support for mmap() of this fd.  This provides another route for a
> process to map BARs, except one where the process can only map a specific
> subset of a BAR represented by the exported DMABUF.
> 
> mmap() support enables userspace driver designs that safely delegate
> access to BAR sub-ranges to other client processes by sharing a DMABUF
> fd, without having to share the (omnipotent) VFIO device fd with them.
> 
> The mmap callback installs vm_ops callbacks for .fault and .huge_fault;
> they find a PFN by searching the DMABUF's physical ranges.  That is,
> DMABUFs with multiple ranges are supported for mmap().

In general sounds like a good idea but this approach here doesn't looks good at all.

Especially how you call unmap_mapping_range() from your DMA-buf cleanup path looks extremely questionable.

...

> +/*
> + * Similar to vfio_pci_core_mmap() for a regular VFIO device fd, but
> + * differs by pre-checks performed and ultimately the vm_ops installed.
> + */
> +static int vfio_pci_dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
> +{
> +       struct vfio_pci_dma_buf *priv = dmabuf->priv;
> +       u64 req_len, req_start;
> +
> +       if (!vfio_pci_dma_buf_is_mappable(dmabuf))
> +               return -ENODEV;
> +       if ((vma->vm_flags & VM_SHARED) == 0)
> +               return -EINVAL;
> +
> +       req_len = vma->vm_end - vma->vm_start;
> +       req_start = vma->vm_pgoff << PAGE_SHIFT;
> +
> +       if (req_start + req_len > priv->size)
> +               return -EINVAL;
> +
> +       vma->vm_private_data = priv;
> +       vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> +       vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
> +
> +       /*
> +        * See comments in vfio_pci_core_mmap() re VM_ALLOW_ANY_UNCACHED.
> +        *
> +        * FIXME: get mapping attributes from dmabuf?
> +        */
> +       vm_flags_set(vma, VM_ALLOW_ANY_UNCACHED | VM_IO | VM_PFNMAP |
> +                    VM_DONTEXPAND | VM_DONTDUMP);
> +       vma->vm_ops = &vfio_pci_dma_buf_mmap_ops;
> +
> +       return 0;

Let's start with this here, it just looks horrible over complicated.

When a DMA-buf just represents a linear piece of BAR which is map-able through the VFIO FD anyway then the right approach is to just re-direct the mapping to this VFIO FD.

Roughly something like this here should do it:

	vma->vm_pgoff += offset_which_your_dma_buf_represents;
	vma_set_file(vma, core_dev->file);
	vfio_pci_core_mmap(core_dev, vma);

It can be that you want additional checks (e.g. if the DMA-buf is revoked) in which case you would need to override the vma->vm_ops, but then just do the access checks and call the vfio_pci_mmap_ops to get the actually page fault handling done.


>+               unmap_mapping_range(priv->dmabuf->file->f_mapping,
>+                                   0, priv->size, 1);

When you need to use unmap_mapping_range() then you usually share the address space object between the file descriptor exporting the DMA-buf and the DMA-buf fd itself.

Otherwise functions like vfio_pci_zap_bars() doesn't work correctly any more and that usually creates a huge bunch of problems.

Regards,
Christian.