Re: [PATCH v5 4/9] vfio/pci: Add a helper to create a DMABUF for a BAR-map VMA

[Alex Williamson <alex@shazbot.org>]

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);
> >  	kfree(priv->phys_vec);
> >  	kfree(priv);
> >  }
> > @@ -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