On Tue, 2 Jun 2026 18:35:25 +0100 Matt Evans <[email protected]> wrote: > On 29/05/2026 00:15, Alex Williamson wrote: > > On Wed, 27 May 2026 03:23:10 -0700 > > Matt Evans <[email protected]> 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
