On Wed, 12 Aug 2026 23:39:37 +0100 Matt Evans matt@ozlabs.org wrote:
Hi Alex, Leon, Kevin, Praan,
On 15/07/2026 18:47, Matt Evans wrote:
This helper, vfio_pci_core_mmap_prep_dmabuf(), creates a single-range DMABUF for the purpose of mapping a PCI BAR. This is used in a future commit by VFIO's ordinary mmap() path.
This function transfers ownership of the VFIO device fd to the DMABUF, which fput()s when it's released.
Refactor the existing vfio_pci_core_feature_dma_buf() to split out export code common to the two paths, VFIO_DEVICE_FEATURE_DMA_BUF and this new VFIO_BAR mmap().
Signed-off-by: Matt Evans matt@ozlabs.org Reviewed-by: Kevin Tian kevin.tian@intel.com Reviewed-by: Pranjal Shrivastava praan@google.com
drivers/vfio/pci/vfio_pci_dmabuf.c | 142 +++++++++++++++++++++++------ drivers/vfio/pci/vfio_pci_priv.h | 5 + 2 files changed, 117 insertions(+), 30 deletions(-)
diff --git a/drivers/vfio/pci/vfio_pci_dmabuf.c b/drivers/vfio/pci/vfio_pci_dmabuf.c index 7c047400dfd1..74c02794bfe2 100644 --- a/drivers/vfio/pci/vfio_pci_dmabuf.c +++ b/drivers/vfio/pci/vfio_pci_dmabuf.c @@ -82,6 +82,8 @@ static void vfio_pci_dma_buf_release(struct dma_buf *dmabuf) up_write(&priv->vdev->memory_lock); vfio_device_put_registration(&priv->vdev->vdev); }
- if (priv->vfile)
fput(priv->vfile);} @@ -233,6 +235,45 @@ int vfio_pci_dma_buf_find_pfn(struct vfio_pci_dma_buf *priv, return ret; } +/*
- Create a DMABUF corresponding to priv, add it to vdev->dmabufs list
- for tracking (meaning cleanup or revocation will zap it), and take
- a vfio_device registration.
- */
+static int vfio_pci_dmabuf_export(struct vfio_pci_core_device *vdev,
struct vfio_pci_dma_buf *priv, u32 flags)+{
- DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
- if (!vfio_device_try_get_registration(&vdev->vdev))
return -ENODEV;- exp_info.ops = &vfio_pci_dmabuf_ops;
- exp_info.size = priv->size;
- exp_info.flags = flags;
- exp_info.priv = priv;
- priv->dmabuf = dma_buf_export(&exp_info);
- if (IS_ERR(priv->dmabuf)) {
vfio_device_put_registration(&vdev->vdev);return PTR_ERR(priv->dmabuf);- }
- kref_init(&priv->kref);
- init_completion(&priv->comp);
- /* dma_buf_put() now frees priv */
- INIT_LIST_HEAD(&priv->dmabufs_elm);
- down_write(&vdev->memory_lock);
- dma_resv_lock(priv->dmabuf->resv, NULL);
- priv->revoked = !__vfio_pci_memory_enabled(vdev);
- list_add_tail(&priv->dmabufs_elm, &vdev->dmabufs);
- dma_resv_unlock(priv->dmabuf->resv);
- up_write(&vdev->memory_lock);
It looks like a local Claude review (kreview) genuinely found a problem here. There seems to be a new deadlock scenario because vfio-pci's mmap() now does the DMABUF export and now takes vdev->memory_lock:
nvgrace-gpu forwards mmap() of regular BARs on to vfio_pci_core_mmap(), so it takes vdev->memory_lock for write here with mm->mmap_lock held for write.
But the nvgrace-gpu driver's MMIO accessors, nvgrace_gpu_{read,write}_mem(), rely on holding vdev->memory_lock for read across the device readiness check and the device access, e.g.:
nvgrace_gpu_read_mem(): takes memory_lock(R) nvgrace_gpu_check_device_ready() nvgrace_gpu_map_and_read(): // The copy accesses the device copy_to_user(...) <-- could fault
That fault hits lock_mm_and_find_vma() and tries to take mm->mmap_lock for read. That waits on another thread that's already started an mmap() and holds mm->mmap_lock for write but has blocked on the faulting thread's vdev->memory_lock. ABBA and boom.
Yuck. I'm glad this was found now, at least. :|
A possible way forward:
Please can I have some expert advice on whether the DMABUF export really must hold vdev->memory_lock for _write_ or could relax to hold it for _read_ in the function above:
- It's protecting the __vfio_pci_memory_enabled() test vs adding the
buffer to the list (could be read)
- It's upholding the invariant of priv->revoked not changing without
holding both memory_lock & resv, but no one can see the DMABUF yet
- It's protecting the list-add against a concurrent revoke/cleanup
- It's protecting the list-add against another concurrent export
If vfio_pci_dmabuf_export() could instead hold memory_lock for read, then nvgrace-gpu (or other future vfio-pci variant drivers!) can also hold it for read, and the deadlock is avoided.
The revoke/cleanup paths hold vdev->memory_lock for write, so wouldn't run concurrently, but there'd be a new problem of protecting against another concurrent export. Perhaps a new vdev->export_lock held (only) in this function around vdev->memory_lock could address that.
The other variant drivers seem to be OK in this regard. Solving this in the core seems the right approach; at any rate, I don't think the nvgrace-gpu side can be relaxed.
There'd still be the strong constraint that the drivers must avoid taking vdev->memory_lock for write. How to enforce this?
What are your thoughts on this problem/solution? Am I missing any nuances?
My read is that the vfio dmabuf code is using memory_lock write-lock to serialize the dmabufs list as a matter of convenience since it needs to be held across all the revokes anyway. It's an overloaded use of memory_lock.
I agree with your analysis how we could use read-lock, but I don't particularly like the idea of a separate lock just to serialize between exports while legitimate write-lock paths continue to rely on memory_lock for serialization. I'd rather see a dmabufs_lock mutex added and used consistently for serializing the dmabufs list.
I think the touch points are:
- vfio_pci_dma_buf_release(): list protection only, dmabufs_lock
- vfio_pci_core_feature_dma_buf(): memory_lock(R) for memory enabled, enclosing dmabufs_lock for list
- vfio_pci_dma_buf_move(): add dmabufs_lock guard
- vfio_pci_dma_buf_cleanup(): up_write memory_lock after move, add dmabufs_lock guard around list walk
The cleanup call is on the close_device path, so while it's a bit clunky that we drop and re-aquire dmabufs_lock between move and list pruning, no dmabufs can be added in that gap since the device is closed, ie. no dmabuf feature ioctl access. At least aiui.
For enforcement that a variant driver doesn't take the write-lock, I think in part it's that there really shouldn't be a need for serializing on memory_lock through mmap if we're using the lock correctly, but also lockdep to find it. Thanks,
Alex