Re: [PATCH v2 7/9] vfio/pci: Support mmap() of a VFIO DMABUF

[Alex Williamson <alex@shazbot.org>]

On Tue, 2 Jun 2026 18:35:25 +0100
Matt Evans <mattev@meta.com> wrote:
> On 29/05/2026 00:15, Alex Williamson wrote:
> > On Wed, 27 May 2026 03:23:10 -0700
> > Matt Evans <mattev@meta.com> wrote:
> >> diff --git a/drivers/vfio/pci/vfio_pci_dmabuf.c b/drivers/vfio/pci/vfio_pci_dmabuf.c
> >> index 733607371082..4b3b15655f1d 100644
> >> --- a/drivers/vfio/pci/vfio_pci_dmabuf.c
> >> +++ b/drivers/vfio/pci/vfio_pci_dmabuf.c
> >> @@ -27,6 +27,32 @@ static int vfio_pci_dma_buf_attach(struct dma_buf *dmabuf,
> >>   
> >>   	return 0;
> >>   }
> >> +
> >> +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 (priv->revoked)
> >> +		return -ENODEV;  
> > 
> > Questionable validity to testing revoked without a lock, but doesn't
> > this also fail to follow the "map regardless, sort it out on fault"
> > paradigm used elsewhere in vfio-pci?  Thanks,  
> 
> Kind of, though this was intentional.  (I had removed a crappy comment 
> here from the RFC, but should re-add something better...)
> 
> You're right that, if we don't test this here, the fault handler ensures 
> safety.
> 
> In the revoke ioctl^Wfeature header UAPI comment I'd suggested a 
> revocation prevents a new map (in addition to prevents access to an 
> existing map) with the intention of making userspace easier to debug:
> 
>   - I'd rather trace an mmap() failure than a SIGBUS (eg. if another
>     process has revoked my DMABUF and I didn't get the memo).  For a
>     prior revoke this is a reliable & useful test.
> 
>   - Even if there's an active race such as userspace doing an
>     ioctl(RESET) at the same time as an mmap() (!) then obviously this
>     check won't always catch it, but when it does then that's useful.
> 
> I'd prefer to keep this belt-and-braces check to early-fail (with 
> comment), if you don't mind much.  It _is_ testing revoked outside of 
> any synchronisation as anything we can lock against here doesn't have a 
> useful effect AFAICT.  (Taking the example of a concurrent ioctl(RESET), 
> a revoke still manifests "before" (caught here) or "after" (caught in 
> fault handler).)  Maybe READ_ONCE() wouldn't hurt, if only to indicate this.

Hey Matt,

I think only the permanently revoked semantics are up for
interpretation here, mmap with memory disabled or across reset is
currently allowed and better debug'ability is not sufficient to risk
breaking compatibility.  Thanks,

Alex