On Wed, Jun 10, 2026 at 04:43:21PM +0100, Matt Evans wrote:
Hi Matt,
[...]
> + *
> + * With the goal of taking vdev->memory_lock in a world where
> + * vdev might not still exist:
> + *
> + * 1. Take the resv lock on the DMABUF:
> + * - If racing cleanup got in first, the buffer is revoked;
> + * stop/exit if so.
> + * - If we got in first, the buffer is not revoked so vdev is
> + * non-NULL, accessible, and cleanup _has not yet put the
> + * VFIO device registration_. So, the device refcount must
> + * be >0.
> + *
> + * 2. Take vfio_device registration (refcount guaranteed >0
> + * hereafter).
> + *
> + * 3. Unlock the DMABUF's resv lock:
> + * - A racing cleanup can now complete.
> + * - But, the device refcount >0, meaning the vfio_device
> + * (and vfio_pcie_core device vdev) have not yet been
> + * freed. vdev is accessible, even if the DMABUF has been
> + * revoked or cleanup has happened, because
> + * vfio_unregister_group_dev() can't complete.
> + *
> + * 4. Take the vdev->memory_lock
> + * - Either the DMABUF is usable, or has been cleaned up.
> + * Whichever, it can no longer change under us.
> + * - Test the DMABUF revocation status again: if it was
> + * revoked between 1 and 4 return a SIGBUS. Otherwise,
> + * return a PFN.
> + * - It's not necessary to also take the resv lock, because
> + * the status/vdev can't change while memory_lock is held.
> + *
> + * 5. Unlock, done.
> */
> +
> + dma_resv_lock(priv->dmabuf->resv, NULL);
> +
> + if (priv->revoked) {
> + pr_debug_ratelimited("%s VA 0x%lx, pgoff 0x%lx: DMABUF
> revoked/cleaned up\n",
> + __func__, vmf->address, vma->vm_pgoff);
> + dma_resv_unlock(priv->dmabuf->resv);
> + return VM_FAULT_SIGBUS;
> + }
> +
> + /* If the buffer isn't revoked, vdev is valid */
> vdev = priv->vdev;
>
> + if (!vfio_device_try_get_registration(&vdev->vdev)) {
> + /*
> + * If vdev != NULL (above), the registration should
> + * already be >0 and so this try_get should never
> + * fail.
> + */
> + dev_warn(&vdev->pdev->dev, "%s: Unexpected registration
> failure\n",
> + __func__);
> + dma_resv_unlock(priv->dmabuf->resv);
> + return VM_FAULT_SIGBUS;
> + }
> + dma_resv_unlock(priv->dmabuf->resv);
> +
> scoped_guard(rwsem_read, &vdev->memory_lock) {
> + /* Revocation status must be re-read, under memory_lock */
> if (!priv->revoked) {
> int pres = vfio_pci_dma_buf_find_pfn(priv, vma,
> vmf->address,
Wait, I noticed that the is_aligned_for_order() check from mainline was
removed here. Was that intentional?
For hugepage faults (order > 0), we must ensure the PFN and address are
properly aligned before calling vfio_pci_vmf_insert_pfn().
In the current upstream code, we have:
if (is_aligned_for_order(vma, addr, pfn, order))
Should we restore that check here?
> @@ -1766,6 +1827,7 @@ static vm_fault_t vfio_pci_mmap_huge_fault(struct
> vm_fault *vmf,
> __func__, order, pfn, vmf->address,
> vma->vm_pgoff, (unsigned int)ret);
>
> + vfio_device_put_registration(&vdev->vdev);
> return ret;
> }
>
> @@ -1774,7 +1836,7 @@ static vm_fault_t vfio_pci_mmap_page_fault(struct
> vm_fault *vmf)
> return vfio_pci_mmap_huge_fault(vmf, 0);
> }
>
> -static const struct vm_operations_struct vfio_pci_mmap_ops = {
> +const struct vm_operations_struct vfio_pci_mmap_ops = {
> .fault = vfio_pci_mmap_page_fault,
Nit: Instead of making this global, should we add a helper? E.g.:
void vfio_pci_set_vma_ops(struct vm_area_struct *vma)
{
vma->vm_ops = &vfio_pci_mmap_ops;
}
[...]
> +
> +static int vfio_pci_dma_buf_mmap(struct dma_buf *dmabuf, struct
> vm_area_struct *vma)
> +{
> + struct vfio_pci_dma_buf *priv = dmabuf->priv;
> +
> + /*
> + * If we observe that the buffer is revoked now then refuse
> + * the mmap(). This is a belt-and-braces early failure to
> + * ease debugging a revoked buffer being used. Userspace
> + * might also race an mmap() against an explicit revocation,
> + * or an action doing a temporary revoke; race scenarios are
> + * still safe because the fault handler ultimately prevents
> + * access to a revoked buffer if it isn't caught here.
> + */
> + if (READ_ONCE(priv->revoked))
> + return -ENODEV;
> + if ((vma->vm_flags & VM_SHARED) == 0)
> + return -EINVAL;
> +
> + /*
> + * dma_buf_mmap_internal() has asserted that the VMA is
> + * contained within the DMABUF size before calling this.
> + */
> +
> + 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. */
> + vm_flags_set(vma, VM_ALLOW_ANY_UNCACHED | VM_IO | VM_PFNMAP |
> + VM_DONTEXPAND | VM_DONTDUMP);
> + vma->vm_private_data = priv;
> + vma->vm_ops = &vfio_pci_mmap_ops;
> +
> + return 0;
> +}
> #endif /* CONFIG_VFIO_PCI_DMABUF */
>
Thanks,
Praan
