Hi all,
The goal of this series is to enable userspace driver designs that use VFIO to export DMABUFs representing subsets of PCI device BARs, and "vend" those buffers from a primary process to other subordinate processes by fd. These processes then mmap() the buffers and their access to the device is isolated to the exported ranges. This is an improvement on sharing the VFIO device fd to subordinate processes, which would allow unfettered access.
This is achieved by enabling mmap() of vfio-pci DMABUFs, passed by fd to subordinate processes. Second, a new revocation mechanism is added to allow the primary process to forcibly revoke access to previously-shared BAR spans, even if the subordinate processes haven't cleanly exited.
(The related topic of safe delegation of iommufd control to the subordinate processes is not addressed here, and is follow-up work.)
The background/rationale is covered in more detail in the RFC cover letters.
Reviews requested that we migrate the existing VFIO PCI BAR mmap() to be backed by a DMABUF too, resulting in a common vm_ops and fault handler for mmap()s of both the VFIO device and explicitly-exported DMABUFs. This will help future iommufd emulation of VFIO Type1 peer-to-peer, making it easier to get a DMABUF for a VFIO BAR as a DMA target.
mmap() conversion to use DMABUF underneath has been done for vfio-pci, but not sub-drivers:
nvgrace-gpu's mmap() override path is unchanged; I kept this out of scope for now not least because I don't have a thorough test setup for this system. I would prefer to help the nvgrace-gpu maintainers enable BAR mmap() DMABUFs themselves.
Notes on patches ================
vfio/pci: Remove DMABUF export dependency on vdev->memory_lock
In v5 of this series [1] we discover that doing an export from the VFIO mmap() path (fundamental!) adds a dependency between mmap_lock and vdev->memory_lock(W) because export was using memory_lock(W) to protect the vdev->dmabufs list and state within, and a deadlock scenario leapt out to bite. (Details in [1].)
The suggestion was to add a dedicated mutex/rwsem specifically for the DMABUFs/list, which is cleaner than overloading memory_lock(W).
But whilst export could now downgrade to holding memory_lock just for _read_ to test __vfio_pci_memory_enabled(), a very similar deadlock can still arise due to a memory_lock(W) elsewhere depending on a prior memory_lock(R) to be released, and attempting to take memory_lock(R) will queue behind the (W) for fairness (effectively an R->R dependency). I'd overlooked that rwsem cannot guarantee multiple readers.
To be able to export while holding mmap_lock, export cannot hold memory_lock at all. Instead of testing __vfio_pci_memory_enabled() (which tracks PCI_COMMAND.MSE and PM state), this patch tracks device-global DMABUF revocation state in a new vdev->bars_revoked flag updated by vfio_pci_dma_buf_move(). If an mmap() is somehow performed during a period when DMABUFs are all revoked, then the DMABUF is created revoked. Move() already bookends reset, PM transitions etc., so subsequent revocation state changes work as-is. This flag is also protected by the dmabuf_lock and thus memory_lock is not required to export.
vfio/pci: Un-revoke DMABUFs in LOW_POWER_ENTRY_WITH_WAKEUP resume
On LOW_POWER entry, DMABUFs have a move(revoke=true), but the runtime resume path didn't un-revoke. This adds a corresponding move(revoke=false), which will later turn into vfio_pci_unrevoke_bars().
NOTE: the unrevoke is reordered _before_ the eventfd_signal() in vfio_pci_core_runtime_resume(). This removes a window in which a waking waiter could have observed the DMABUF state as still revoked (or pm_runtime_engaged = true). (The UAPI docs state the event means the resume's complete, so observing otherwise seemed unintended.)
Also, when DMABUFs are later mmap()ed, a waiter waking and taking a fault could have seen the unrevoked state and SIGBUS just before taking the memory_lock. (If the handler gets to acquire the lock, though, the resume sequence is complete, and the handler observes pm_runtime_engaged = false.)
This fix is in this series because the issue will impact CPU access to the VMA as well (once they use DMABUFs), and so it's a strict dependency of later commits.
dma-buf: Export dma_buf_set_name()
Makes dma_buf_set_name() available for use (by helper patch), taking a kernel-allocated string. The pre-existing local helper becomes a wrapper copying a __user string for the set-name ioctl.
vfio/pci: Add a helper to look up PFNs for DMABUFs
Adds a DMABUF VMA fault handler helper to determine arbitrary-sized PFNs from ranges in DMABUF.
vfio/pci: Add a helper to create a DMABUF for a BAR-map VMA
Refactors DMABUF export for use by the existing export feature, and adds a helper that creates a DMABUF corresponding to a VFIO BAR mmap() request.
There was a request for decent debug naming in /proc/<pid>/maps etc. comparable to the existing VFIO names: since the VMAs are DMABUFs, they have a "dmabuf:" prefix and can't be 100% identical to before. This is a user-visible change, but this patch at least now gives us extra info on the BDF & BAR being mmap()ed. The name is installed with the new dma_buf_set_name() above. An alternative would be to add a creation-time name field to struct dma_buf_export_info, but a setter function is more useful: the name can be changed at will after init.
vfio/pci: Convert BAR mmap() to use a DMABUF
The vfio-pci core mmap() creates a DMABUF with the helper above, and the vm_ops fault handler uses the other helper to resolve the fault. Because this depends on DMABUF structs/code, CONFIG_VFIO_PCI_CORE needs to depend on CONFIG_DMA_SHARED_BUFFER. The CONFIG_VFIO_PCI_DMABUF still conditionally enables the export support code.
NOTE: The user mmap()s a device fd, but the resulting VMA's vm_file becomes that of the DMABUF. The DMABUF takes ownership of the device file and put()s it on release, which maintains the existing behaviour of a VMA keeping the VFIO device open.
BAR zapping then happens via the existing vfio_pci_dma_buf_move() path, which now needs to unmap PTEs in the DMABUF's address_space.
NOTE: As local LLM reviews did, Sashiko might falsely worry about the DMABUF fd being obtained through /proc/pid/map_files and remapped, but the file is an anon inode and doesn't support the open op (-ENXIO) so AFAICT this is currently impossible.
NOTE: A side-effect of this is that is_mergeable_vma() will be false between adjacent mappings of VFIO BARs; merging would require rebuilding a larger DMABUF representing the union, not just plugging the VMAs together.
The DMABUF backing the BAR is an implicit/internal export, even when the CONFIG_VFIO_PCI_DMABUF feature is not included because CONFIG_PCI_P2PDMA is not available. Without P2PDMA, it's acceptable for the DMABUF to not have a P2PDMA provider. In this configuration, the VFIO DMABUF .attach prohibits any import, which avoids getting as far as a WARN in dma_buf_map_attachment(), which would fail without P2PDMA anyway.
vfio/pci: Clean up BAR zap and revocation
In general (see NOTE!) the vfio_pci_zap_bars() is now obsolete, since it unmaps PTEs in the VFIO device address_space which is now unused. This consolidates all calls (e.g. around reset) with the neighbouring vfio_pci_dma_buf_move()s into new functions, to revoke/unrevoke (making the steps clearer).
NOTE: Because drivers can use their own vm_ops and override .mmap, the core must conservatively assume an overridden .mmap might still add PTEs to the VFIO device address_space and therefore still does the zap. A new flag, zap_bars_on_revoke, enables the zap when .mmap is overridden. A driver that does not need the zap can clear this to opt-out, e.g. if the driver calls down to the common mmap (and so uses DMABUFs). hisi-acc-vfio-pci does just this, and thus sets the opt-out flag.
vfio/pci: Support mmap() of a VFIO DMABUF
Adds mmap() for a DMABUF fd exported from vfio-pci.
It was a goal to keep the VFIO device fd lifetime behaviour unchanged with respect to the DMABUFs. An application can close all device fds, and this will revoke/clean up all DMABUFs; then, no mappings or other access can be performed. When enabling mmap() of the DMABUFs, this means access through the VMA is also revoked. This complicates the fault handler because whilst the DMABUF exists, it has no guarantee that the corresponding VFIO device is still alive. Adds synchronisation ensuring the vdev is available before the locks in vdev are touched; this holds the device registration so that even if the buffer has been cleaned up, vdev hasn't been freed and so the locks can be safely taken.
vfio/pci: Permanently revoke a DMABUF on request
This is mostly a rename of `revoked` to an enum, `status`, and adding a third state for a buffer: usable, revoked temporary, revoked permanent. A new VFIO feature is added, VFIO_DEVICE_FEATURE_DMA_BUF_REVOKE, which takes a DMABUF (exported from the same device) and permanently revokes it. Thus a userspace driver can guarantee any downstream consumers of a shared fd are prevented from accessing a BAR range, and that range can be reused. NOTE: This might block userspace, waiting on importers to detach.
The code doing revocation in vfio_pci_dma_buf_move() is moved, to a common function for use by ..._move() and this new feature.
Testing =======
(The [RFC ONLY] userspace test program, which drives a QEMU bochs-display function, can be found in the GitHub branch below. It at least illustrates how the export, map, revoke, and close semantics interoperate. WIP on a follow-up with a proper vfio-selftests style test based on this -- this won't be part of this series.)
This code has been tested in mapping DMABUFs of single/multiple ranges from multiple BARs, aliasing mmap()s, aliasing ranges across DMABUFs, vm_pgoff > 0, revocation, shutdown/cleanup scenarios, and hugepage mappings. No regressions observed on the VFIO selftests, or on our internal vfio-pci applications. VFIO on i386 has been build-tested. Thanks to Alex Mastro for building a (WIP) testcase for the mmap_lock->memory_lock issue (which is now OK...).
Dear Reviewers, ===============
There was a lot of finessing v5->v6, all patches had fixes/changes/cleanups/rewordings made, and so I have dropped previous R-B tags.
Along the way several related issues came up that warrant more eyes, and I'd be grateful for your input:
1. The mmap fault handler takes a bunch of locks non-interruptibly, and potentially depends on a lot of DMABUF-related activities completing. I had a go at converting them to interruptible/killable forms, but pulling on the thread revealed there seems to be a wider issue if move/revoke doesn't complete in a timely fashion (due to buggy importers). Where I got to was that just updating the fault handler won't fix the user experience of an unkillable task, and move()/revocation will need thought too. I don't intend to fix this here but wanted to start discussion so we can address it in a follow up. There's now a dependency between mmap_lock in the fault handler and the DMABUF resv (which might take a while to resolve).
2. vfio_basic_config_write() has an error path if vfio_default_config_write() fails that releases memory_lock but doesn't un-revoke BARs in the case of PCI_COMMAND.MSE being cleared. When can the write fail, in practice, perhaps surprise removal?
The effect on this series would be: a write of MSE=0 revokes BARs, vconfig[PCI_COMMAND]'s MSE becomes 0, but if the physical write fails then the physical MSE remains 1 and BAR VMAs stay revoked.
This seemed a mess; fixing isn't as simple as un-revoking on the error path since vfio_default_config_write() has already trampled vconfig so that'd need unwinding. It felt like a catastrophic scenario where BARs staying revoked isn't a bad outcome, but want to hear your experience of the likelihood of this issue.
3. The exchange of a VFIO fd mmap()'s vma->vm_file with an implicitly created DMABUF's file has implications on LSM. For example, an mmap will be checked against the policy for a VFIO fd, but a subsequent mprotect() relates to the DMABUF file's policy (which is anon/unique to the mapping). This is pretty confusing.
4. If VFIO fd is opened O_RDONLY, it currently can't be mmap()ed (because PROT_WRITE is rejected in do_mmap(), and PROT_READ alone drops the VM_SHARED so VFIO's mmap rejects it). But it seems we can export a DMABUF from it, and then pass the resulting fd around for P2P writes.
I don't know if this is intentional, relied on, or a known limitation so haven't included a change here, but:
a) We could reject export unless the device fd's f_mode has O_RDWR, to reflect the abilities of P2P
b) Or, instead of just failing if !O_RDWR, we limit the get_dma_buf.open_flags to the VFIO device fd's f_mode, such as:
VFIO device fd perms: Export flags: Result: O_RDWR O_RDWR, O_RDONLY OK O_RDONLY O_RDONLY OK O_RDONLY O_RDWR -EPERM O_WRONLY * -EPERM * O_WRONLY -EPERM
(Skipping WRONLY because a PROT_WRITE-only mmap() won't work, though it probably should be included for P2P.)
If we can do at least (a) that seems good, because the knock-on effect in this series is that we can export a DMABUF RW from an O_RDONLY device fd and then succeed to mmap() the DMABUF with RW. (That said, even if one has an O_RDONLY device fd, the device state can still be changed/reset. But it feels cleaner to least prevent export for a O_RDONLY device fd, and match the device fd mmap() behaviour.)
Maybe (b) is step 2: Allowing finer-grained RD/WR could be useful if there's a future goal to tie DMABUF permissions to, say, iommufd IOMMU_READ/IOMMU_WRITE permissions. I don't htink this is in place today, apologies if I'm missed something.
END ===
This is based on v7.2.
These commits are on GitHub for easier browsing, along with "[RFC ONLY] selftests: vfio: Add standalone vfio_dmabuf_mmap_test":
https://github.com/metamev/linux/compare/v7.2...dev/mev/vfio-dmabuf-mmap-v6
Thanks for reading [this astonishingly-long cover letter],
Matt
[1] https://lore.kernel.org/kvm/9a615f22-c0d4-46ae-9654-db11e94e5fec@ozlabs.org/
================================================================================ Changelog:
v6:
- Dropped patches: PCI/P2PDMA: Split pool-related cleanup out of pci_p2pdma_release() PCI/P2PDMA: Add CONFIG_PCI_P2PDMA_CORE
- New patch, "vfio/pci: Remove DMABUF export dependency on vdev->memory_lock":
memory_lock was overloaded to protect the vdev->dmabufs list, the entries' revoked status, etc. Towards the goal of not needing memory_lock in export (so not creating a mmap_lock -> memory_lock dependency due to exporting from mmap()), add a new vdev->dmabuf_lock which protects the list and (writing) the revocation status. (See details in cover letter.)
- New patch, "vfio/pci: Un-revoke DMABUFs in LOW_POWER_ENTRY_WITH_WAKEUP resume":
Bugfix, as the resume path didn't un-revoke. Previously affected "only" P2P, but since DMABUFs-everywhere it would affect BAR mmap()s too.
- New patch, "dma-buf: Export dma_buf_set_name()":
Change in dma-buf.c's dma_buf_set_name() to take a kernel-allocated string. The original IOCTL-related path wraps this to strdup the user string first.
- "vfio/pci: Add a helper to look up PFNs for DMABUFs":
Commit message reworded explaining vma_pgoff_adjust, return -ERANGE for (non-transient) pagesize failure instead of -EAGAIN. Add dmabuf_lock annotation and assert that the DMABUF isn't revoked (else new -ENODEV error), using a new vdev parameter (caller is expected to safely extract it from priv).
Significant bugfix in the case of a VMA offset exceeding the 1TB stride between VFIO_PCI_OFFSET_MASK-sized regions, which would have silently wrapped due to being masked to 1TB. This is fixed by including the VFIO region index in the high bits of vma_pgoff_adjust for the traditional mmap() path, so the subtraction from vma->vm_pgoff in ...find_pfn() removes the region index without masking. For the DMABUF mmap() path, vma_pgoff_adjust = 0 and (thanks to no masking) large offsets can be used.
- "vfio/pci: Add a helper to create a DMABUF for a BAR-map VMA":
Now permits a NULL p2pdma provider to be installed in the vfio_pci_dma_buf. The regular BAR mmap() path doesn't need a provider for CPU access; the DMABUF backing the VMA exists primarily for the CPU, and removing this check decouples vfio-pci from depending on CONFIG_PCI_P2PDMA (_without_ going through the previous hoops of splitting up P2PDMA into a _CORE subset to provide the provider).
Only DMABUF import/map needs a provider, which is available when P2PDMA is available; the previous series' first two patches (to always provide a provider) are now unnecessary and have been dropped.
Squashed "vfio/pci: Provide a user-facing name for BAR mappings" into this patch, because it's now just very small due to the new dma_buf_set_name(). Comment that, because of careful construction of the name length, the setting of the name can't fail; but in case assumptions change, a check/kfree prevents it leaking. (A new error path doing full unwind of the DMABUF creation is overkill.)
- "vfio/pci: Convert BAR mmap() to use a DMABUF":
Instead of the previous dropped P2PDMA split commits, allows pcim_p2pdma_provider() to return NULL if !CONFIG_VFIO_PCI_DMABUF (meaning !CONFIG_PCI_P2PDMA). The provider isn't used unless the DMABUF is imported; prevent import to make this fail early (rather than at map), by creating an always-fail vfio_pci_dma_buf_attach() stub. (Somewhat belt and braces, but makes clear you need P2PDMA to import a BAR VMA's DMABUF!)
- "vfio/pci: Clean up BAR zap and revocation":
The zap_bars_on_revoke flag is moved back to the bitfield, and is set from vfio_pci_core_init_dev() (instead of registration time). Reworked the hisi driver change to update this earlier.
- "vfio/pci: Support mmap() of a VFIO DMABUF":
Previous versions introduced a build issue (which was fixed by the next patch) due to a READ_ONCE of the priv->revoked bitfield member; removed. It is added by the next patch in the series (s/revoked/status/).
Added an explicit rejection for mmap of a DMABUF with vma_pgoff_adjust > 0. Such buffers can only be created implicitly by the VFIO BAR mmap path, and an fd can't currently be recreated from a VMA (e.g. fished out of /proc/pid/map_files) because the anon inode ops don't support open. But check added if a future mechanism arises and for clarity. Rejecting an mmap of a DMABUF fd having vma_pgoff_adjust > 0 means vfio_pci_dma_buf_find_pfn() doesn't need to cope with the case of vma->vm_pgoff < priv->vma_pgoff_adjust and underflow.
- "vfio/pci: Permanently revoke a DMABUF on request":
Clarified docs for returned error values; re-added READ_ONCE(priv->status) for unlocked reads in both vfio_pci_dma_buf_attach() and vfio_pci_dma_buf_mmap().
v5: https://lore.kernel.org/all/20260715174737.15287-1-matt@ozlabs.org/ v4: https://lore.kernel.org/all/20260701171245.90111-1-matt@ozlabs.org/ v3: https://lore.kernel.org/all/20260610154327.37758-1-matt@ozlabs.org/ v2: https://lore.kernel.org/all/20260527102319.100128-1-mattev@meta.com/ v1: https://lore.kernel.org/kvm/20260416131815.2729131-1-mattev@meta.com/ RFCv2: https://lore.kernel.org/kvm/20260312184613.3710705-1-mattev@meta.com/ RFCv1: https://lore.kernel.org/all/20260226202211.929005-1-mattev@meta.com/ Tech topic: https://lore.kernel.org/linux-iommu/20250918214425.2677057-1-amastro@fb.com/
Matt Evans (9): vfio/pci: Remove DMABUF export dependency on vdev->memory_lock vfio/pci: Un-revoke DMABUFs in LOW_POWER_ENTRY_WITH_WAKEUP resume dma-buf: Export dma_buf_set_name() vfio/pci: Add a helper to look up PFNs for DMABUFs vfio/pci: Add a helper to create a DMABUF for a BAR-map VMA vfio/pci: Convert BAR mmap() to use a DMABUF vfio/pci: Clean up BAR zap and revocation vfio/pci: Support mmap() of a VFIO DMABUF vfio/pci: Permanently revoke a DMABUF on request
drivers/dma-buf/dma-buf.c | 58 +- drivers/vfio/pci/Kconfig | 4 +- drivers/vfio/pci/Makefile | 3 +- .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 14 +- drivers/vfio/pci/vfio_pci_config.c | 30 +- drivers/vfio/pci/vfio_pci_core.c | 222 +++++-- drivers/vfio/pci/vfio_pci_dmabuf.c | 607 +++++++++++++++--- drivers/vfio/pci/vfio_pci_priv.h | 54 +- include/linux/dma-buf.h | 2 + include/linux/vfio_pci_core.h | 3 + include/uapi/linux/vfio.h | 24 + 11 files changed, 851 insertions(+), 170 deletions(-)
The VFIO_DEVICE_FEATURE_DMA_BUF export previously used vdev->memory_lock to protect VFIO's list of exported DMABUFs (vdev->dmabufs) and their revocation status. When adding a new export, memory_lock was taken for write.
A future commit will refactor DMABUF export into a function used from mmap(), which would create a new mmap_lock -> memory_lock dependency.
In preparation, this commit introduces a vdev->dmabuf_lock that protects the dmabufs list, per-DMABUF revocation state, and a new bars_revoked flag. The flag tracks the device-wide BAR revocation state set via vfio_pci_dma_buf_move() under dmabuf_lock, removing the test of __vfio_pci_memory_enabled() and dependency on vdev->memory_lock.
This is a cleanup currently, but allows the future export path to avoid holding memory_lock under mmap_lock and a deadlock scenario (a fault handler attempting to take mmap_lock in a vfio-pci variant driver thread that holds memory_lock).
Signed-off-by: Matt Evans matt@ozlabs.org --- drivers/vfio/pci/vfio_pci_core.c | 2 ++ drivers/vfio/pci/vfio_pci_dmabuf.c | 30 +++++++++++++++++++++++++----- include/linux/vfio_pci_core.h | 2 ++ 3 files changed, 29 insertions(+), 5 deletions(-)
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c index 3f11a9624b9c..7d090be8c9c1 100644 --- a/drivers/vfio/pci/vfio_pci_core.c +++ b/drivers/vfio/pci/vfio_pci_core.c @@ -659,6 +659,7 @@ int vfio_pci_core_enable(struct vfio_pci_core_device *vdev) vdev->has_vga = true;
vfio_pci_core_map_bars(vdev); + vdev->bars_revoked = false;
return 0;
@@ -2195,6 +2196,7 @@ int vfio_pci_core_init_dev(struct vfio_device *core_vdev) return ret; INIT_LIST_HEAD(&vdev->dmabufs); init_rwsem(&vdev->memory_lock); + init_rwsem(&vdev->dmabuf_lock); xa_init(&vdev->ctx);
return 0; diff --git a/drivers/vfio/pci/vfio_pci_dmabuf.c b/drivers/vfio/pci/vfio_pci_dmabuf.c index c16f460c01d6..847ee5cc78e7 100644 --- a/drivers/vfio/pci/vfio_pci_dmabuf.c +++ b/drivers/vfio/pci/vfio_pci_dmabuf.c @@ -90,9 +90,9 @@ static void vfio_pci_dma_buf_release(struct dma_buf *dmabuf) * The refcount prevents both. */ if (priv->vdev) { - down_write(&priv->vdev->memory_lock); + down_write(&priv->vdev->dmabuf_lock); list_del_init(&priv->dmabufs_elm); - up_write(&priv->vdev->memory_lock); + up_write(&priv->vdev->dmabuf_lock); vfio_device_put_registration(&priv->vdev->vdev); } kfree(priv->phys_vec); @@ -305,12 +305,27 @@ int vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags,
/* dma_buf_put() now frees priv */ INIT_LIST_HEAD(&priv->dmabufs_elm); - down_write(&vdev->memory_lock); + + /* + * dmabuf_lock synchronises access (R) or updates (W) to the + * vdev->dmabufs list and to bars_revoked (see below). The + * revocation state of DMABUF elements in the list is written + * holding both dmabuf_lock(W) and resv, and tested with + * either. + * + * dmabuf_lock -> resv + * + * vdev->bars_revoked tracks the BAR revocation status updated + * via vfio_pci_dma_buf_move(), so the initial DMABUF state + * follows the same criteria that later update the DMABUF + * state (BAR zap, etc.). + */ + down_write(&vdev->dmabuf_lock); dma_resv_lock(priv->dmabuf->resv, NULL); - priv->revoked = !__vfio_pci_memory_enabled(vdev); + priv->revoked = vdev->bars_revoked; list_add_tail(&priv->dmabufs_elm, &vdev->dmabufs); dma_resv_unlock(priv->dmabuf->resv); - up_write(&vdev->memory_lock); + up_write(&vdev->dmabuf_lock);
/* * dma_buf_fd() consumes the reference, when the file closes the dmabuf @@ -340,6 +355,8 @@ void vfio_pci_dma_buf_move(struct vfio_pci_core_device *vdev, bool revoked)
lockdep_assert_held_write(&vdev->memory_lock);
+ down_write(&vdev->dmabuf_lock); + vdev->bars_revoked = revoked; list_for_each_entry_safe(priv, tmp, &vdev->dmabufs, dmabufs_elm) { if (!get_file_active(&priv->dmabuf->file)) continue; @@ -375,6 +392,7 @@ void vfio_pci_dma_buf_move(struct vfio_pci_core_device *vdev, bool revoked) } fput(priv->dmabuf->file); } + up_write(&vdev->dmabuf_lock); }
void vfio_pci_dma_buf_cleanup(struct vfio_pci_core_device *vdev) @@ -393,6 +411,7 @@ void vfio_pci_dma_buf_cleanup(struct vfio_pci_core_device *vdev) */ vfio_pci_dma_buf_move(vdev, true);
+ down_write(&vdev->dmabuf_lock); list_for_each_entry_safe(priv, tmp, &vdev->dmabufs, dmabufs_elm) { if (!get_file_active(&priv->dmabuf->file)) continue; @@ -402,5 +421,6 @@ void vfio_pci_dma_buf_cleanup(struct vfio_pci_core_device *vdev) vfio_device_put_registration(&vdev->vdev); fput(priv->dmabuf->file); } + up_write(&vdev->dmabuf_lock); up_write(&vdev->memory_lock); } diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h index 9a1674c152aa..f99b152edcca 100644 --- a/include/linux/vfio_pci_core.h +++ b/include/linux/vfio_pci_core.h @@ -134,6 +134,7 @@ struct vfio_pci_core_device { bool pm_intx_masked; bool pm_runtime_engaged; bool sriov_active; + bool bars_revoked; struct pci_saved_state *pci_saved_state; struct pci_saved_state *pm_save; int ioeventfds_nr; @@ -148,6 +149,7 @@ struct vfio_pci_core_device { struct vfio_pci_core_device *sriov_pf_core_dev; struct notifier_block nb; struct rw_semaphore memory_lock; + struct rw_semaphore dmabuf_lock; struct list_head dmabufs; };
VFIO_DEVICE_FEATURE_LOW_POWER_ENTRY_WITH_WAKEUP correctly revokes DMABUFs on entry and, when paired with a VFIO_DEVICE_FEATURE_LOW_POWER_EXIT, vfio_pci_runtime_pm_exit() correctly un-revokes them.
However, when vfio_pci_core_runtime_resume() signals the eventfd on resume, the bare __vfio_pci_runtime_pm_exit() is used (which does not un-revoke). Add a DMABUF move(false) to the resume path, making it similar to vfio_pci_runtime_pm_exit().
This also reorders the eventfd signal after the un-revoke and __vfio_pci_runtime_pm_exit() to guarantee that woken threads observe the new state.
Fixes: 5d74781ebc86 ("vfio/pci: Add dma-buf export support for MMIO regions") Signed-off-by: Matt Evans matt@ozlabs.org --- drivers/vfio/pci/vfio_pci_core.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c index 7d090be8c9c1..3428322efa6e 100644 --- a/drivers/vfio/pci/vfio_pci_core.c +++ b/drivers/vfio/pci/vfio_pci_core.c @@ -526,8 +526,14 @@ static int vfio_pci_core_runtime_resume(struct device *dev) */ down_write(&vdev->memory_lock); if (vdev->pm_wake_eventfd_ctx) { - eventfd_signal(vdev->pm_wake_eventfd_ctx); + struct eventfd_ctx *ctx = vdev->pm_wake_eventfd_ctx; + + vdev->pm_wake_eventfd_ctx = NULL; __vfio_pci_runtime_pm_exit(vdev); + if (__vfio_pci_memory_enabled(vdev)) + vfio_pci_dma_buf_move(vdev, false); + eventfd_signal(ctx); + eventfd_ctx_put(ctx); } up_write(&vdev->memory_lock);
dma_buf_set_name() originally took a __user string to duplicate for the buffer name. Make this function a generic set-name helper, taking a kernel-allocated string.
Wrap this by a new dma_buf_set_name_user() to support the existing ioctl path for a user-provided string.
Signed-off-by: Matt Evans matt@ozlabs.org --- drivers/dma-buf/dma-buf.c | 58 ++++++++++++++++++++++++++++++--------- include/linux/dma-buf.h | 2 ++ 2 files changed, 47 insertions(+), 13 deletions(-)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index d504c636dc29..8129ea11ff58 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -405,31 +405,29 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll) }
/** - * dma_buf_set_name - Set a name to a specific dma_buf to track the usage. - * It could support changing the name of the dma-buf if the same - * piece of memory is used for multiple purpose between different devices. + * dma_buf_set_name_user - Set a dma_buf's name from a user string + * + * The string is up to DMA_BUF_NAME_LEN long, including the terminator. * * @dmabuf: [in] dmabuf buffer that will be renamed. * @buf: [in] A piece of userspace memory that contains the name of * the dma-buf. * - * Returns 0 on success. If the dma-buf buffer is already attached to - * devices, return -EBUSY. - * + * Returns 0 on success, and any previously-set name is freed. */ -static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf) +static long dma_buf_set_name_user(struct dma_buf *dmabuf, const char __user *buf) { char *name = strndup_user(buf, DMA_BUF_NAME_LEN); + int ret;
if (IS_ERR(name)) return PTR_ERR(name);
- spin_lock(&dmabuf->name_lock); - kfree(dmabuf->name); - dmabuf->name = name; - spin_unlock(&dmabuf->name_lock); + ret = dma_buf_set_name(dmabuf, name); + if (ret) + kfree(name);
- return 0; + return ret; }
#if IS_ENABLED(CONFIG_SYNC_FILE) @@ -578,7 +576,7 @@ static long dma_buf_ioctl(struct file *file,
case DMA_BUF_SET_NAME_A: case DMA_BUF_SET_NAME_B: - return dma_buf_set_name(dmabuf, (const char __user *)arg); + return dma_buf_set_name_user(dmabuf, (const char __user *)arg);
#if IS_ENABLED(CONFIG_SYNC_FILE) case DMA_BUF_IOCTL_EXPORT_SYNC_FILE: @@ -854,6 +852,40 @@ void dma_buf_put(struct dma_buf *dmabuf) } EXPORT_SYMBOL_NS_GPL(dma_buf_put, "DMA_BUF");
+/** + * dma_buf_set_name - Set a dma_buf's name + * It could support changing the name of the dma-buf if the same piece + * of memory is used for multiple purpose between different devices. + * + * @dmabuf: [in] dmabuf buffer that will be renamed. + * @name: [in] The name of the dma-buf, allocated with kmalloc() or + * similar. This takes ownership of the allocation + * on success, which will be kfree()d when the + * dmabuf is released or a new name assigned. + * + * Returns 0 on success, -EINVAL if the name is NULL, or -E2BIG if the + * name exceeds DMA_BUF_NAME_LEN. + */ +int dma_buf_set_name(struct dma_buf *dmabuf, char *name) +{ + if (!name) + return -EINVAL; + + /* dmabuffs_dname() won't use the string if the length + * (including terminator) exceeds DMA_BUF_NAME_LEN: + */ + if (strlen(name) >= DMA_BUF_NAME_LEN) + return -E2BIG; + + spin_lock(&dmabuf->name_lock); + kfree(dmabuf->name); + dmabuf->name = name; + spin_unlock(&dmabuf->name_lock); + + return 0; +} +EXPORT_SYMBOL_NS_GPL(dma_buf_set_name, "DMA_BUF"); + static int dma_buf_wrap_sg_table(struct sg_table **sg_table) { struct scatterlist *to_sg, *from_sg; diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index d1203da56fc5..14d3950b63c8 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -570,6 +570,8 @@ int dma_buf_fd(struct dma_buf *dmabuf, int flags); struct dma_buf *dma_buf_get(int fd); void dma_buf_put(struct dma_buf *dmabuf);
+int dma_buf_set_name(struct dma_buf *dmabuf, char *name); + struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *, enum dma_data_direction); void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table *,
On 9/11/26 23:41, Matt Evans wrote:
dma_buf_set_name() originally took a __user string to duplicate for the buffer name. Make this function a generic set-name helper, taking a kernel-allocated string.
Wrap this by a new dma_buf_set_name_user() to support the existing ioctl path for a user-provided string.
That just describes what the patch does and not why, in other words this needs a bit more justification for the change.
For example something like "Allow exporters/importers to set the name of buffers from pre-existing information".
It is also important to describe which role should set a name (exporter, importer or both). When an exporter gives a standard name for it's buffers that is most likely harmless, but when an importer bluntly overwrites a name previously set by an exporter or userspace then we really need a good reason for that.
Signed-off-by: Matt Evans matt@ozlabs.org
drivers/dma-buf/dma-buf.c | 58 ++++++++++++++++++++++++++++++--------- include/linux/dma-buf.h | 2 ++ 2 files changed, 47 insertions(+), 13 deletions(-)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index d504c636dc29..8129ea11ff58 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -405,31 +405,29 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll) } /**
- dma_buf_set_name - Set a name to a specific dma_buf to track the usage.
- It could support changing the name of the dma-buf if the same
- piece of memory is used for multiple purpose between different devices.
- dma_buf_set_name_user - Set a dma_buf's name from a user string
- The string is up to DMA_BUF_NAME_LEN long, including the terminator.
- @dmabuf: [in] dmabuf buffer that will be renamed.
- @buf: [in] A piece of userspace memory that contains the name of
the dma-buf.
- Returns 0 on success. If the dma-buf buffer is already attached to
- devices, return -EBUSY.
*/
- Returns 0 on success, and any previously-set name is freed.
Kerneldoc for a static function is usually overkill.
And since this is used only once and very complicated I would completely merge the logic into dma_buf_ioctl().
Regards, Christian.
-static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf) +static long dma_buf_set_name_user(struct dma_buf *dmabuf, const char __user *buf) { char *name = strndup_user(buf, DMA_BUF_NAME_LEN);
- int ret;
if (IS_ERR(name)) return PTR_ERR(name);
- spin_lock(&dmabuf->name_lock);
- kfree(dmabuf->name);
- dmabuf->name = name;
- spin_unlock(&dmabuf->name_lock);
- ret = dma_buf_set_name(dmabuf, name);
- if (ret)
kfree(name);
- return 0;
- return ret;
} #if IS_ENABLED(CONFIG_SYNC_FILE) @@ -578,7 +576,7 @@ static long dma_buf_ioctl(struct file *file, case DMA_BUF_SET_NAME_A: case DMA_BUF_SET_NAME_B:
return dma_buf_set_name(dmabuf, (const char __user *)arg);
return dma_buf_set_name_user(dmabuf, (const char __user *)arg);#if IS_ENABLED(CONFIG_SYNC_FILE) case DMA_BUF_IOCTL_EXPORT_SYNC_FILE: @@ -854,6 +852,40 @@ void dma_buf_put(struct dma_buf *dmabuf) } EXPORT_SYMBOL_NS_GPL(dma_buf_put, "DMA_BUF"); +/**
- dma_buf_set_name - Set a dma_buf's name
- It could support changing the name of the dma-buf if the same piece
- of memory is used for multiple purpose between different devices.
- @dmabuf: [in] dmabuf buffer that will be renamed.
- @name: [in] The name of the dma-buf, allocated with kmalloc() or
similar. This takes ownership of the allocation
on success, which will be kfree()d when the
dmabuf is released or a new name assigned.
- Returns 0 on success, -EINVAL if the name is NULL, or -E2BIG if the
- name exceeds DMA_BUF_NAME_LEN.
- */
+int dma_buf_set_name(struct dma_buf *dmabuf, char *name) +{
- if (!name)
return -EINVAL;- /* dmabuffs_dname() won't use the string if the length
* (including terminator) exceeds DMA_BUF_NAME_LEN:*/- if (strlen(name) >= DMA_BUF_NAME_LEN)
return -E2BIG;- spin_lock(&dmabuf->name_lock);
- kfree(dmabuf->name);
- dmabuf->name = name;
- spin_unlock(&dmabuf->name_lock);
- return 0;
+} +EXPORT_SYMBOL_NS_GPL(dma_buf_set_name, "DMA_BUF");
static int dma_buf_wrap_sg_table(struct sg_table **sg_table) { struct scatterlist *to_sg, *from_sg; diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index d1203da56fc5..14d3950b63c8 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -570,6 +570,8 @@ int dma_buf_fd(struct dma_buf *dmabuf, int flags); struct dma_buf *dma_buf_get(int fd); void dma_buf_put(struct dma_buf *dmabuf); +int dma_buf_set_name(struct dma_buf *dmabuf, char *name);
struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *, enum dma_data_direction); void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table *,
Hi Christian,
On 14/09/2026 12:06, Christian König wrote:
On 9/11/26 23:41, Matt Evans wrote:
dma_buf_set_name() originally took a __user string to duplicate for the buffer name. Make this function a generic set-name helper, taking a kernel-allocated string.
Wrap this by a new dma_buf_set_name_user() to support the existing ioctl path for a user-provided string.
That just describes what the patch does and not why, in other words this needs a bit more justification for the change.
For example something like "Allow exporters/importers to set the name of buffers from pre-existing information".
Fair enough, am adding missing rationale.
It is also important to describe which role should set a name (exporter, importer or both). When an exporter gives a standard name for it's buffers that is most likely harmless, but when an importer bluntly overwrites a name previously set by an exporter or userspace then we really need a good reason for that.
Good point; my expectation was that exporters would use this to either set or update the name, and importers would never do this. I will make this clear in the commit message/comments, thanks.
Signed-off-by: Matt Evans matt@ozlabs.org
drivers/dma-buf/dma-buf.c | 58 ++++++++++++++++++++++++++++++--------- include/linux/dma-buf.h | 2 ++ 2 files changed, 47 insertions(+), 13 deletions(-)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index d504c636dc29..8129ea11ff58 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -405,31 +405,29 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll) } /**
- dma_buf_set_name - Set a name to a specific dma_buf to track the usage.
- It could support changing the name of the dma-buf if the same
- piece of memory is used for multiple purpose between different devices.
- dma_buf_set_name_user - Set a dma_buf's name from a user string
- The string is up to DMA_BUF_NAME_LEN long, including the terminator.
- @dmabuf: [in] dmabuf buffer that will be renamed.
- @buf: [in] A piece of userspace memory that contains the name of
the dma-buf.
- Returns 0 on success. If the dma-buf buffer is already attached to
- devices, return -EBUSY.
*/
- Returns 0 on success, and any previously-set name is freed.
Kerneldoc for a static function is usually overkill.
The original static function had some so I kept it, but no worries. Removed.
And since this is used only once and very complicated I would completely merge the logic into dma_buf_ioctl().
Will do (no function, no kerneldoc).
Many thanks,
Matt
Regards, Christian.
-static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf) +static long dma_buf_set_name_user(struct dma_buf *dmabuf, const char __user *buf) { char *name = strndup_user(buf, DMA_BUF_NAME_LEN);
- int ret;
if (IS_ERR(name)) return PTR_ERR(name);
- spin_lock(&dmabuf->name_lock);
- kfree(dmabuf->name);
- dmabuf->name = name;
- spin_unlock(&dmabuf->name_lock);
- ret = dma_buf_set_name(dmabuf, name);
- if (ret)
kfree(name);
- return 0;
- return ret;
} #if IS_ENABLED(CONFIG_SYNC_FILE) @@ -578,7 +576,7 @@ static long dma_buf_ioctl(struct file *file, case DMA_BUF_SET_NAME_A: case DMA_BUF_SET_NAME_B:
return dma_buf_set_name(dmabuf, (const char __user *)arg);
return dma_buf_set_name_user(dmabuf, (const char __user *)arg);#if IS_ENABLED(CONFIG_SYNC_FILE) case DMA_BUF_IOCTL_EXPORT_SYNC_FILE: @@ -854,6 +852,40 @@ void dma_buf_put(struct dma_buf *dmabuf) } EXPORT_SYMBOL_NS_GPL(dma_buf_put, "DMA_BUF"); +/**
- dma_buf_set_name - Set a dma_buf's name
- It could support changing the name of the dma-buf if the same piece
- of memory is used for multiple purpose between different devices.
- @dmabuf: [in] dmabuf buffer that will be renamed.
- @name: [in] The name of the dma-buf, allocated with kmalloc() or
similar. This takes ownership of the allocation
on success, which will be kfree()d when the
dmabuf is released or a new name assigned.
- Returns 0 on success, -EINVAL if the name is NULL, or -E2BIG if the
- name exceeds DMA_BUF_NAME_LEN.
- */
+int dma_buf_set_name(struct dma_buf *dmabuf, char *name) +{
- if (!name)
return -EINVAL;- /* dmabuffs_dname() won't use the string if the length
* (including terminator) exceeds DMA_BUF_NAME_LEN:*/- if (strlen(name) >= DMA_BUF_NAME_LEN)
return -E2BIG;- spin_lock(&dmabuf->name_lock);
- kfree(dmabuf->name);
- dmabuf->name = name;
- spin_unlock(&dmabuf->name_lock);
- return 0;
+} +EXPORT_SYMBOL_NS_GPL(dma_buf_set_name, "DMA_BUF");
static int dma_buf_wrap_sg_table(struct sg_table **sg_table) { struct scatterlist *to_sg, *from_sg; diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index d1203da56fc5..14d3950b63c8 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -570,6 +570,8 @@ int dma_buf_fd(struct dma_buf *dmabuf, int flags); struct dma_buf *dma_buf_get(int fd); void dma_buf_put(struct dma_buf *dmabuf); +int dma_buf_set_name(struct dma_buf *dmabuf, char *name);
struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *, enum dma_data_direction); void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table *,
Add vfio_pci_dma_buf_find_pfn(), which a VMA fault handler can use to find a PFN.
This supports multi-range DMABUFs, which typically would be used to represent scattered spans but might even represent overlapping or aliasing spans of PFNs.
Because this is intended to be used in vfio_pci_core.c, we also need to expose the struct vfio_pci_dma_buf in the vfio_pci_priv.h header.
This move also adds a new member, vma_pgoff_adjust; this is consumed by vfio_pci_dma_buf_find_pfn(), but will be set in a future commit. Add it now to reduce the number of functions touched by later commits.
Signed-off-by: Matt Evans matt@ozlabs.org --- drivers/vfio/pci/vfio_pci_dmabuf.c | 166 ++++++++++++++++++++++++++--- drivers/vfio/pci/vfio_pci_priv.h | 21 ++++ 2 files changed, 174 insertions(+), 13 deletions(-)
diff --git a/drivers/vfio/pci/vfio_pci_dmabuf.c b/drivers/vfio/pci/vfio_pci_dmabuf.c index 847ee5cc78e7..9f10b10fc436 100644 --- a/drivers/vfio/pci/vfio_pci_dmabuf.c +++ b/drivers/vfio/pci/vfio_pci_dmabuf.c @@ -9,19 +9,6 @@
MODULE_IMPORT_NS("DMA_BUF");
-struct vfio_pci_dma_buf { - struct dma_buf *dmabuf; - struct vfio_pci_core_device *vdev; - struct list_head dmabufs_elm; - size_t size; - struct phys_vec *phys_vec; - struct p2pdma_provider *provider; - u32 nr_ranges; - struct kref kref; - struct completion comp; - u8 revoked : 1; -}; - static int vfio_pci_dma_buf_attach(struct dma_buf *dmabuf, struct dma_buf_attachment *attachment) { @@ -106,6 +93,159 @@ static const struct dma_buf_ops vfio_pci_dmabuf_ops = { .release = vfio_pci_dma_buf_release, };
+int vfio_pci_dma_buf_find_pfn(struct vfio_pci_core_device *vdev, + struct vfio_pci_dma_buf *priv, + struct vm_area_struct *vma, + unsigned long fault_addr, + unsigned int order, + unsigned long *out_pfn) +{ + /* + * Given a VMA (start, end, pgoffs) and a fault address, + * search the corresponding DMABUF's phys_vec[] to find the + * range representing the address's offset into the VMA, and + * its PFN. vdev must be the device that the DMABUF priv was + * exported from; vdev->dmabuf_lock must be held, and priv + * must not be revoked. + * + * The phys_vec[] ranges represent contiguous spans of VAs + * upwards from the buffer offset 0; the actual PFNs might be + * in any order, overlap/alias, etc. Calculate an offset of + * the desired page given VMA start/pgoff and address, then + * search upwards from 0 to find which span contains it. + * + * On success, a valid PFN for a page sized by 'order' is + * returned into out_pfn. + * + * Failure occurs if: + * - A hugepage would cross the edge of the VMA, + * - A hugepage isn't entirely contained within a range + * (including where it straddles the boundary between + * ranges), + * - We find a range, but the final PFN isn't aligned to the + * requested order. + * + * Upon failure, -ERANGE is returned and the caller is + * expected to try again with a smaller order, which will + * eventually succeed. + * + * It's suboptimal if DMABUFs are created with neighbouring + * ranges that are physically contiguous, since hugepages + * can't straddle range boundaries. (The construction of the + * ranges should merge them in this case.) + * + * Finally, vma_pgoff_adjust is used with a DMABUF created for + * a VFIO BAR mmap: a BAR mapped with vm_pgoff > 0 creates a + * DMABUF such that byte 0 of the VMA corresponds to byte 0 of + * the DMABUF and byte 'vm_pgoff << PAGE_SHIFT' into the BAR. + * To avoid double-offsetting in this scenario, subtracting + * vma_pgoff_adjust from this (non-zero) vm_pgoff generates + * the effective offset. This also removes the VFIO region + * index encoded in vm_pgoff for VFIO BAR mmaps. + */ + + const unsigned long pagesize = PAGE_SIZE << order; + unsigned long vma_off = (vma->vm_pgoff - priv->vma_pgoff_adjust) << + PAGE_SHIFT; + unsigned long rounded_page_addr = ALIGN_DOWN(fault_addr, pagesize); + unsigned long rounded_page_end = rounded_page_addr + pagesize; + unsigned long fault_offset; + unsigned long fault_offset_end; + unsigned long range_start_offset = 0; + unsigned int i; + int ret; + + if (unlikely(!vdev)) + return -ENODEV; + + /* This prevents the dmabuf revocation state from changing under us */ + lockdep_assert_held(&vdev->dmabuf_lock); + + if (unlikely(priv->vdev != vdev || priv->revoked)) + return -ENODEV; + + if (rounded_page_addr < vma->vm_start || rounded_page_end > vma->vm_end) { + if (order > 0) + return -ERANGE; + + /* A fault address outside of the VMA is absurd. */ + dev_warn_ratelimited( + &vdev->pdev->dev, + "Fault addr 0x%lx outside VMA 0x%lx-0x%lx\n", + fault_addr, vma->vm_start, vma->vm_end); + return -EFAULT; + } + + /* + * fault_offset[_end] is the span within the DMABUF + * corresponding to the faulting page: + */ + if (unlikely(check_add_overflow(rounded_page_addr - vma->vm_start, + vma_off, &fault_offset) || + check_add_overflow(fault_offset, pagesize, + &fault_offset_end))) + return -EFAULT; + + /* + * Iterate over ranges in the buffer, summing their lengths: + * range_start_offset represents the current range's starting + * offset in the buffer (from 0 upwards). + * + * A failure for order == 0 is unexpected, and triggers a + * fault/warn. + */ + ret = (order == 0) ? -EFAULT : -ERANGE; + + for (i = 0; i < priv->nr_ranges; i++) { + size_t range_len = priv->phys_vec[i].len; + + /* Early exit if range starts after the page end */ + if (fault_offset_end <= range_start_offset) + break; + + if (fault_offset >= range_start_offset && + fault_offset_end <= range_start_offset + range_len) { + /* + * The faulting page is wholly contained + * within the span represented by this range, + * so validate PFN alignment for the order. + * The if() condition ensures the pfn + * arithmetic won't overflow. + */ + unsigned long pfn = + ((fault_offset - range_start_offset) + + priv->phys_vec[i].paddr) >> PAGE_SHIFT; + + if (IS_ALIGNED(pfn, 1 << order)) { + *out_pfn = pfn; + ret = 0; + } + /* + * Else order > 0; ERANGE retries with smaller + * order + */ + break; + } + range_start_offset += range_len; + } + + if (order == 0 && ret != 0) + /* + * The address fell outside of the span represented by + * the (concatenated) ranges. As setup of a mapping + * ensures that the VMA is <= the total size of the + * ranges this should never happen. If it does, warn + * and SIGBUS. + */ + dev_warn_ratelimited( + &vdev->pdev->dev, + "No range for addr 0x%lx, order %d: VMA 0x%lx-0x%lx pgoff 0x%lx, %u ranges, size 0x%zx\n", + fault_addr, order, vma->vm_start, vma->vm_end, + vma->vm_pgoff, priv->nr_ranges, priv->size); + + return ret; +} + /* * This is a temporary "private interconnect" between VFIO DMABUF and iommufd. * It allows the two co-operating drivers to exchange the physical address of diff --git a/drivers/vfio/pci/vfio_pci_priv.h b/drivers/vfio/pci/vfio_pci_priv.h index fca9d0dfac90..48d9f574a3df 100644 --- a/drivers/vfio/pci/vfio_pci_priv.h +++ b/drivers/vfio/pci/vfio_pci_priv.h @@ -23,6 +23,20 @@ struct vfio_pci_ioeventfd { bool test_mem; };
+struct vfio_pci_dma_buf { + struct dma_buf *dmabuf; + struct vfio_pci_core_device *vdev; + struct list_head dmabufs_elm; + size_t size; + struct phys_vec *phys_vec; + struct p2pdma_provider *provider; + u32 nr_ranges; + struct kref kref; + struct completion comp; + unsigned long vma_pgoff_adjust; + u8 revoked : 1; +}; + bool vfio_pci_intx_mask(struct vfio_pci_core_device *vdev); void vfio_pci_intx_unmask(struct vfio_pci_core_device *vdev);
@@ -114,6 +128,13 @@ static inline bool vfio_pci_is_vga(struct pci_dev *pdev) return (pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA; }
+int vfio_pci_dma_buf_find_pfn(struct vfio_pci_core_device *vdev, + struct vfio_pci_dma_buf *priv, + struct vm_area_struct *vma, + unsigned long address, + unsigned int order, + unsigned long *out_pfn); + #ifdef CONFIG_VFIO_PCI_DMABUF int vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags, struct vfio_device_feature_dma_buf __user *arg,
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().
By exchanging the VMA file, we lose the original device path in /proc/<pid>/maps, lsof, etc. Generate a debug-oriented synthetic 'filename' for BAR mappings based on the cdev, plus BDF, plus resource index. (This does not apply to explicitly-exported DMABUFs which are named by DMA_BUF_SET_NAME.)
Signed-off-by: Matt Evans matt@ozlabs.org --- drivers/vfio/pci/vfio_pci_dmabuf.c | 211 +++++++++++++++++++++++------ drivers/vfio/pci/vfio_pci_priv.h | 5 + 2 files changed, 171 insertions(+), 45 deletions(-)
diff --git a/drivers/vfio/pci/vfio_pci_dmabuf.c b/drivers/vfio/pci/vfio_pci_dmabuf.c index 9f10b10fc436..faa9239e66f8 100644 --- a/drivers/vfio/pci/vfio_pci_dmabuf.c +++ b/drivers/vfio/pci/vfio_pci_dmabuf.c @@ -3,6 +3,7 @@ */ #include <linux/dma-buf-mapping.h> #include <linux/pci-p2pdma.h> +#include <linux/dma-buf.h> #include <linux/dma-resv.h>
#include "vfio_pci_priv.h" @@ -82,6 +83,8 @@ static void vfio_pci_dma_buf_release(struct dma_buf *dmabuf) up_write(&priv->vdev->dmabuf_lock); vfio_device_put_registration(&priv->vdev->vdev); } + if (priv->vfile) + fput(priv->vfile); kfree(priv->phys_vec); kfree(priv); } @@ -246,6 +249,167 @@ int vfio_pci_dma_buf_find_pfn(struct vfio_pci_core_device *vdev, 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); + + /* + * dmabuf_lock synchronises access (R) or updates (W) to the + * vdev->dmabufs list and to bars_revoked (see below). The + * revocation state of DMABUF elements in the list is written + * holding both dmabuf_lock(W) and resv, and tested with + * either. + * + * (memory_lock, if held ->) dmabuf_lock -> resv + * + * NOTE: memory_lock is strictly avoided here, to avoid a + * dependency on memory_lock when mmap_lock is held, when + * mmap() leads to export. vfio-pci variant drivers are + * permitted to hold memory_lock across actions that might + * fault (such as user access); a deadlock could result when + * that fault path attempts to take mmap_lock (if held by an + * export waiting for memory_lock). + * + * vdev->bars_revoked tracks the BAR revocation status updated + * via vfio_pci_dma_buf_move(), so the initial DMABUF state + * follows the same criteria that later update the DMABUF + * state (BAR zap, etc.). + */ + lockdep_assert_not_held(&vdev->memory_lock); + + down_write(&vdev->dmabuf_lock); + dma_resv_lock(priv->dmabuf->resv, NULL); + priv->revoked = vdev->bars_revoked; + list_add_tail(&priv->dmabufs_elm, &vdev->dmabufs); + dma_resv_unlock(priv->dmabuf->resv); + up_write(&vdev->dmabuf_lock); + + return 0; +} + +int vfio_pci_core_mmap_prep_dmabuf(struct vfio_pci_core_device *vdev, + struct vm_area_struct *vma, + u64 phys_start, u64 req_len, + unsigned int res_index) +{ + struct vfio_pci_dma_buf *priv; + unsigned long vma_pgoff = vma->vm_pgoff & (VFIO_PCI_OFFSET_MASK >> PAGE_SHIFT); + char *bufname; + int ret; + + priv = kzalloc_obj(*priv); + if (!priv) + return -ENOMEM; + + priv->phys_vec = kzalloc_obj(*priv->phys_vec); + if (!priv->phys_vec) { + ret = -ENOMEM; + goto err_free_priv; + } + + /* + * Maximum size of the friendly debug name is + * vfio1048575:ffff:ff:1f.7/5 = 26. This fits within + * DMA_BUF_NAME_LEN, so dma_buf_set_name() below won't fail. + */ + bufname = kasprintf(GFP_KERNEL, "%s:%s/%x", + dev_name(&vdev->vdev.device), pci_name(vdev->pdev), + res_index); + + if (!bufname) { + ret = -ENOMEM; + goto err_free_phys; + } + + /* + * The DMABUF begins from the mmap()'s BAR offset, i.e. the + * start of the VMA corresponds to byte 0 of the DMABUF and + * byte (vma_pgoff << PAGE_SHIFT) of the BAR. + * + * vfio_pci_dma_buf_find_pfn() reverses this offset using + * vma_pgoff_adjust, so that ultimately a fault's offset from + * the start of the _VMA_ has a consistent usage whether the + * VMA originates from an mmap() of the VFIO device here or a + * direct DMABUF mmap(). Note vma_pgoff_adjust also includes + * the encoded VFIO region index, which cancels out the index + * encoded in vm_pgoff. + */ + priv->vdev = vdev; + priv->size = req_len; + priv->nr_ranges = 1; + priv->vma_pgoff_adjust = vma->vm_pgoff; + + priv->provider = pcim_p2pdma_provider(vdev->pdev, res_index); + if (!priv->provider) { + ret = -EINVAL; + goto err_free_name; + } + + priv->phys_vec[0].paddr = phys_start + ((u64)vma_pgoff << PAGE_SHIFT); + priv->phys_vec[0].len = priv->size; + + ret = vfio_pci_dmabuf_export(vdev, priv, O_RDWR); + if (ret) + goto err_free_name; + + if (dma_buf_set_name(priv->dmabuf, bufname)) { + /* Shouldn't happen, but don't leak if it does: */ + dev_dbg_ratelimited(&vdev->pdev->dev, + "Failed to set map name '%s'\n", + bufname); + kfree(bufname); + } + + /* + * Ownership of the DMABUF file transfers to the VMA so that + * other users can locate the DMABUF via a VA. Ownership of + * the original VFIO device file being mmap()ed transfers to + * priv, and is put when the DMABUF is released. This + * intentionally does not use get_file()/vma_set_file() + * because the references are already held, and ownership + * moves. + */ + priv->vfile = vma->vm_file; + vma->vm_file = priv->dmabuf->file; + vma->vm_private_data = priv; + + return 0; + +err_free_name: + kfree(bufname); +err_free_phys: + kfree(priv->phys_vec); +err_free_priv: + kfree(priv); + return ret; +} + /* * This is a temporary "private interconnect" between VFIO DMABUF and iommufd. * It allows the two co-operating drivers to exchange the physical address of @@ -364,7 +528,6 @@ int vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags, { struct vfio_device_feature_dma_buf get_dma_buf = {}; struct vfio_region_dma_range *dma_ranges; - DEFINE_DMA_BUF_EXPORT_INFO(exp_info); struct vfio_pci_dma_buf *priv; size_t length; int ret; @@ -424,49 +587,9 @@ int vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags, kfree(dma_ranges); dma_ranges = NULL;
- if (!vfio_device_try_get_registration(&vdev->vdev)) { - ret = -ENODEV; + ret = vfio_pci_dmabuf_export(vdev, priv, get_dma_buf.open_flags); + if (ret) goto err_free_phys; - } - - exp_info.ops = &vfio_pci_dmabuf_ops; - exp_info.size = priv->size; - exp_info.flags = get_dma_buf.open_flags; - exp_info.priv = priv; - - priv->dmabuf = dma_buf_export(&exp_info); - if (IS_ERR(priv->dmabuf)) { - ret = PTR_ERR(priv->dmabuf); - goto err_dev_put; - } - - kref_init(&priv->kref); - init_completion(&priv->comp); - - /* dma_buf_put() now frees priv */ - INIT_LIST_HEAD(&priv->dmabufs_elm); - - /* - * dmabuf_lock synchronises access (R) or updates (W) to the - * vdev->dmabufs list and to bars_revoked (see below). The - * revocation state of DMABUF elements in the list is written - * holding both dmabuf_lock(W) and resv, and tested with - * either. - * - * dmabuf_lock -> resv - * - * vdev->bars_revoked tracks the BAR revocation status updated - * via vfio_pci_dma_buf_move(), so the initial DMABUF state - * follows the same criteria that later update the DMABUF - * state (BAR zap, etc.). - */ - down_write(&vdev->dmabuf_lock); - dma_resv_lock(priv->dmabuf->resv, NULL); - priv->revoked = vdev->bars_revoked; - list_add_tail(&priv->dmabufs_elm, &vdev->dmabufs); - dma_resv_unlock(priv->dmabuf->resv); - up_write(&vdev->dmabuf_lock); - /* * dma_buf_fd() consumes the reference, when the file closes the dmabuf * will be released. @@ -477,8 +600,6 @@ int vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags,
return ret;
-err_dev_put: - vfio_device_put_registration(&vdev->vdev); err_free_phys: kfree(priv->phys_vec); err_free_priv: diff --git a/drivers/vfio/pci/vfio_pci_priv.h b/drivers/vfio/pci/vfio_pci_priv.h index 48d9f574a3df..3ec676e12e21 100644 --- a/drivers/vfio/pci/vfio_pci_priv.h +++ b/drivers/vfio/pci/vfio_pci_priv.h @@ -30,6 +30,7 @@ struct vfio_pci_dma_buf { size_t size; struct phys_vec *phys_vec; struct p2pdma_provider *provider; + struct file *vfile; u32 nr_ranges; struct kref kref; struct completion comp; @@ -134,6 +135,10 @@ int vfio_pci_dma_buf_find_pfn(struct vfio_pci_core_device *vdev, unsigned long address, unsigned int order, unsigned long *out_pfn); +int vfio_pci_core_mmap_prep_dmabuf(struct vfio_pci_core_device *vdev, + struct vm_area_struct *vma, + u64 phys_start, u64 req_len, + unsigned int res_index);
#ifdef CONFIG_VFIO_PCI_DMABUF int vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags,
Convert the VFIO device fd fops->mmap to create a DMABUF representing the BAR mapping, and make the VMA fault handler look up PFNs from the corresponding DMABUF. This supports future code mmap()ing BAR DMABUFs, and iommufd work to support Type1 P2P.
First, vfio_pci_core_mmap() uses the new vfio_pci_core_mmap_prep_dmabuf() helper to export a DMABUF representing a single BAR range. Then, the vfio_pci_mmap_huge_fault() callback is updated to understand revoked buffers, and uses the new vfio_pci_dma_buf_find_pfn() helper to determine the PFN for a given fault address.
Now that the VFIO DMABUFs can be mmap()ed, vfio_pci_dma_buf_move() zaps PTEs (used on the revocation and cleanup paths).
CONFIG_VFIO_PCI_CORE now unconditionally selects CONFIG_DMA_SHARED_BUFFER. The CONFIG_VFIO_PCI_DMABUF feature conditionally includes support for VFIO_DEVICE_FEATURE_DMA_BUF, depending on the availability of CONFIG_PCI_P2PDMA.
When the DMABUF feature is not present, DMABUFs are used internally for the BAR mappings. In this case, DMABUF attach is prohibited (and the requirement for a P2PDMA provider is relaxed).
Signed-off-by: Matt Evans matt@ozlabs.org --- drivers/vfio/pci/Kconfig | 4 +- drivers/vfio/pci/Makefile | 3 +- drivers/vfio/pci/vfio_pci_core.c | 71 ++++++++++++++++++------------ drivers/vfio/pci/vfio_pci_dmabuf.c | 24 +++++++++- drivers/vfio/pci/vfio_pci_priv.h | 11 +---- 5 files changed, 73 insertions(+), 40 deletions(-)
diff --git a/drivers/vfio/pci/Kconfig b/drivers/vfio/pci/Kconfig index 296bf01e185e..c6d6fb09af86 100644 --- a/drivers/vfio/pci/Kconfig +++ b/drivers/vfio/pci/Kconfig @@ -6,6 +6,7 @@ config VFIO_PCI_CORE tristate select VFIO_VIRQFD select IRQ_BYPASS_MANAGER + select DMA_SHARED_BUFFER
config VFIO_PCI_INTX def_bool y if !S390 @@ -56,7 +57,8 @@ config VFIO_PCI_ZDEV_KVM To enable s390x KVM vfio-pci extensions, say Y.
config VFIO_PCI_DMABUF - def_bool y if VFIO_PCI_CORE && PCI_P2PDMA && DMA_SHARED_BUFFER + def_bool y if PCI_P2PDMA + depends on VFIO_PCI_CORE
source "drivers/vfio/pci/mlx5/Kconfig"
diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile index 6138f1bf241d..881452ea89be 100644 --- a/drivers/vfio/pci/Makefile +++ b/drivers/vfio/pci/Makefile @@ -1,8 +1,7 @@ # SPDX-License-Identifier: GPL-2.0-only
-vfio-pci-core-y := vfio_pci_core.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o +vfio-pci-core-y := vfio_pci_core.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o vfio_pci_dmabuf.o vfio-pci-core-$(CONFIG_VFIO_PCI_ZDEV_KVM) += vfio_pci_zdev.o -vfio-pci-core-$(CONFIG_VFIO_PCI_DMABUF) += vfio_pci_dmabuf.o obj-$(CONFIG_VFIO_PCI_CORE) += vfio-pci-core.o
vfio-pci-y := vfio_pci.o diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c index 3428322efa6e..8dd69948e0b7 100644 --- a/drivers/vfio/pci/vfio_pci_core.c +++ b/drivers/vfio/pci/vfio_pci_core.c @@ -1746,18 +1746,6 @@ void vfio_pci_memory_unlock_and_restore(struct vfio_pci_core_device *vdev, u16 c up_write(&vdev->memory_lock); }
-static unsigned long vma_to_pfn(struct vm_area_struct *vma) -{ - struct vfio_pci_core_device *vdev = vma->vm_private_data; - int index = vma->vm_pgoff >> (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT); - u64 pgoff; - - pgoff = vma->vm_pgoff & - ((1U << (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT)) - 1); - - return (pci_resource_start(vdev->pdev, index) >> PAGE_SHIFT) + pgoff; -} - vm_fault_t vfio_pci_vmf_insert_pfn(struct vfio_pci_core_device *vdev, struct vm_fault *vmf, unsigned long pfn, @@ -1785,23 +1773,46 @@ static vm_fault_t vfio_pci_mmap_huge_fault(struct vm_fault *vmf, unsigned int order) { struct vm_area_struct *vma = vmf->vma; - struct vfio_pci_core_device *vdev = vma->vm_private_data; - unsigned long addr = vmf->address & ~((PAGE_SIZE << order) - 1); - unsigned long pgoff = (addr - vma->vm_start) >> PAGE_SHIFT; - unsigned long pfn = vma_to_pfn(vma) + pgoff; - vm_fault_t ret = VM_FAULT_FALLBACK; - - if (is_aligned_for_order(vma, addr, pfn, order)) { - scoped_guard(rwsem_read, &vdev->memory_lock) - ret = vfio_pci_vmf_insert_pfn(vdev, vmf, pfn, order); + struct vfio_pci_dma_buf *priv = vma->vm_private_data; + struct vfio_pci_core_device *vdev; + unsigned long pfn = 0; + vm_fault_t ret = VM_FAULT_SIGBUS; + + /* + * We can rely on the existence of both a DMABUF (priv) and + * the VFIO device it was exported from (vdev). This fault's + * VMA was established using vfio_pci_core_mmap_prep_dmabuf() + * which transfers ownership of the VFIO device fd to the + * DMABUF, and so the VFIO device is held open because the + * VMA's vm_file (DMABUF) is open. + * + * Since vfio_pci_dma_buf_cleanup() cannot have happened, + * vdev must be valid; we can take vdev locks. + */ + vdev = priv->vdev; + + /* memory_lock for vfio_pci_vmf_insert_pfn() */ + down_read(&vdev->memory_lock); + /* Test revocation status under dmabuf_lock */ + down_read(&vdev->dmabuf_lock); + if (!priv->revoked) { + int pres = vfio_pci_dma_buf_find_pfn(vdev, priv, vma, + vmf->address, + order, &pfn); + + if (pres == 0) + ret = vfio_pci_vmf_insert_pfn(vdev, vmf, + pfn, order); + else if (pres == -ERANGE) + ret = VM_FAULT_FALLBACK; } + up_read(&vdev->dmabuf_lock); + up_read(&vdev->memory_lock);
dev_dbg_ratelimited(&vdev->pdev->dev, - "%s(,order = %d) BAR %ld page offset 0x%lx: 0x%x\n", - __func__, order, - vma->vm_pgoff >> - (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT), - pgoff, (unsigned int)ret); + "%s(order = %d) PFN 0x%lx, VA 0x%lx, pgoff 0x%lx: 0x%x\n", + __func__, order, pfn, vmf->address, + vma->vm_pgoff, (unsigned int)ret);
return ret; } @@ -1826,6 +1837,7 @@ int vfio_pci_core_mmap(struct vfio_device *core_vdev, struct vm_area_struct *vma unsigned int index; u64 phys_len, req_len, pgoff, req_start; void __iomem *bar_io; + int ret;
index = vma->vm_pgoff >> (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT);
@@ -1865,7 +1877,12 @@ int vfio_pci_core_mmap(struct vfio_device *core_vdev, struct vm_area_struct *vma if (IS_ERR(bar_io)) return PTR_ERR(bar_io);
- vma->vm_private_data = vdev; + ret = vfio_pci_core_mmap_prep_dmabuf(vdev, vma, + pci_resource_start(pdev, index), + req_len, index); + if (ret) + return ret; + vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot); vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
diff --git a/drivers/vfio/pci/vfio_pci_dmabuf.c b/drivers/vfio/pci/vfio_pci_dmabuf.c index faa9239e66f8..0126a0582072 100644 --- a/drivers/vfio/pci/vfio_pci_dmabuf.c +++ b/drivers/vfio/pci/vfio_pci_dmabuf.c @@ -10,6 +10,7 @@
MODULE_IMPORT_NS("DMA_BUF");
+#ifdef CONFIG_VFIO_PCI_DMABUF static int vfio_pci_dma_buf_attach(struct dma_buf *dmabuf, struct dma_buf_attachment *attachment) { @@ -26,6 +27,18 @@ static int vfio_pci_dma_buf_attach(struct dma_buf *dmabuf,
return 0; } +#else +static int vfio_pci_dma_buf_attach(struct dma_buf *dmabuf, + struct dma_buf_attachment *attachment) +{ + /* + * Explicit export can't occur without the DMABUF feature, but + * DMABUFs are implicitly created for BAR mappings. An + * .attach that fails prevents dma_buf_attach(). + */ + return -EOPNOTSUPP; +} +#endif /* CONFIG_VFIO_PCI_DMABUF */
static void vfio_pci_dma_buf_done(struct kref *kref) { @@ -365,8 +378,13 @@ int vfio_pci_core_mmap_prep_dmabuf(struct vfio_pci_core_device *vdev, priv->nr_ranges = 1; priv->vma_pgoff_adjust = vma->vm_pgoff;
+ /* + * The provider can be NULL _iff_ the DMABUF feature isn't + * supported, because it's only used by DMABUF import and + * attach is prohibited if the feature isn't present. + */ priv->provider = pcim_p2pdma_provider(vdev->pdev, res_index); - if (!priv->provider) { + if (IS_ENABLED(CONFIG_VFIO_PCI_DMABUF) && !priv->provider) { ret = -EINVAL; goto err_free_name; } @@ -410,6 +428,7 @@ int vfio_pci_core_mmap_prep_dmabuf(struct vfio_pci_core_device *vdev, return ret; }
+#ifdef CONFIG_VFIO_PCI_DMABUF /* * This is a temporary "private interconnect" between VFIO DMABUF and iommufd. * It allows the two co-operating drivers to exchange the physical address of @@ -608,6 +627,7 @@ int vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags, kfree(dma_ranges); return ret; } +#endif /* CONFIG_VFIO_PCI_DMABUF */
void vfio_pci_dma_buf_move(struct vfio_pci_core_device *vdev, bool revoked) { @@ -634,6 +654,8 @@ void vfio_pci_dma_buf_move(struct vfio_pci_core_device *vdev, bool revoked) if (revoked) { kref_put(&priv->kref, vfio_pci_dma_buf_done); wait_for_completion(&priv->comp); + unmap_mapping_range(priv->dmabuf->file->f_mapping, + 0, 0, true); /* * Re-arm the registered kref reference and the * completion so the post-revoke state matches the diff --git a/drivers/vfio/pci/vfio_pci_priv.h b/drivers/vfio/pci/vfio_pci_priv.h index 3ec676e12e21..c62fbbf1625e 100644 --- a/drivers/vfio/pci/vfio_pci_priv.h +++ b/drivers/vfio/pci/vfio_pci_priv.h @@ -139,13 +139,13 @@ int vfio_pci_core_mmap_prep_dmabuf(struct vfio_pci_core_device *vdev, struct vm_area_struct *vma, u64 phys_start, u64 req_len, unsigned int res_index); +void vfio_pci_dma_buf_cleanup(struct vfio_pci_core_device *vdev); +void vfio_pci_dma_buf_move(struct vfio_pci_core_device *vdev, bool revoked);
#ifdef CONFIG_VFIO_PCI_DMABUF int vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags, struct vfio_device_feature_dma_buf __user *arg, size_t argsz); -void vfio_pci_dma_buf_cleanup(struct vfio_pci_core_device *vdev); -void vfio_pci_dma_buf_move(struct vfio_pci_core_device *vdev, bool revoked); #else static inline int vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags, @@ -154,13 +154,6 @@ vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags, { return -ENOTTY; } -static inline void vfio_pci_dma_buf_cleanup(struct vfio_pci_core_device *vdev) -{ -} -static inline void vfio_pci_dma_buf_move(struct vfio_pci_core_device *vdev, - bool revoked) -{ -} #endif
#endif
Previously, vfio_pci_zap_bars() (and the wrapper vfio_pci_zap_and_down_write_memory_lock()) calls were paired with calls to vfio_pci_dma_buf_move().
This commit replaces them with a unified new function, vfio_pci_revoke_bars() containing both the vfio_pci_dma_buf_move() and the unmap_mapping_range(), making it harder for callers to omit one. It adds a wrapper, vfio_pci_lock_revoke_bars(), which takes the write memory_lock before zapping, and adds a new vfio_pci_unrevoke_bars() for the re-enable path.
As of "vfio/pci: Convert BAR mmap() to use a DMABUF", the zap via unmap_mapping_range() is no longer performed for vfio-pci since the DMABUFs used for BAR mappings already zap PTEs when the vfio_pci_dma_buf_move() occurs.
However, it must be assumed that VFIO drivers which override the .mmap op could create mappings _not_ backed by DMABUFs. So, the zap is still performed on revoke if .mmap is overridden, using a new zap_bars_on_revoke flag. A driver can explicitly opt out; the flag is cleared by the hisi_acc_vfio_pci driver, since its .mmap just wraps vfio_pci_core_mmap() and so still uses DMABUFs.
Signed-off-by: Matt Evans matt@ozlabs.org --- .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 14 +++- drivers/vfio/pci/vfio_pci_config.c | 30 +++----- drivers/vfio/pci/vfio_pci_core.c | 75 +++++++++++++------ drivers/vfio/pci/vfio_pci_priv.h | 3 +- include/linux/vfio_pci_core.h | 1 + 5 files changed, 82 insertions(+), 41 deletions(-)
diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c index 86362ec424a5..14622556355e 100644 --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c @@ -1564,6 +1564,7 @@ static int hisi_acc_vfio_pci_migrn_init_dev(struct vfio_device *core_vdev) struct hisi_acc_vf_core_device *hisi_acc_vdev = hisi_acc_get_vf_dev(core_vdev); struct pci_dev *pdev = to_pci_dev(core_vdev->dev); struct hisi_qm *pf_qm = hisi_acc_get_pf_qm(pdev); + int ret;
hisi_acc_vdev->vf_id = pci_iov_vf_id(pdev) + 1; hisi_acc_vdev->pf_qm = pf_qm; @@ -1575,7 +1576,18 @@ static int hisi_acc_vfio_pci_migrn_init_dev(struct vfio_device *core_vdev) core_vdev->migration_flags = VFIO_MIGRATION_STOP_COPY | VFIO_MIGRATION_PRE_COPY; core_vdev->mig_ops = &hisi_acc_vfio_pci_migrn_state_ops;
- return vfio_pci_core_init_dev(core_vdev); + ret = vfio_pci_core_init_dev(core_vdev); + if (ret) + return ret; + /* + * hisi_acc_vfio_pci_mmap() calls down to + * vfio_pci_core_mmap(), so BAR mappings are still + * DMABUF-backed. They don't require a zap on revoke, so opt + * out: + */ + hisi_acc_vdev->core_device.zap_bars_on_revoke = false; + + return 0; }
static const struct vfio_device_ops hisi_acc_vfio_pci_migrn_ops = { diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c index a10ed733f0e3..bc6ccb2e135c 100644 --- a/drivers/vfio/pci/vfio_pci_config.c +++ b/drivers/vfio/pci/vfio_pci_config.c @@ -590,12 +590,10 @@ static int vfio_basic_config_write(struct vfio_pci_core_device *vdev, int pos, virt_mem = !!(le16_to_cpu(*virt_cmd) & PCI_COMMAND_MEMORY); new_mem = !!(new_cmd & PCI_COMMAND_MEMORY);
- if (!new_mem) { - vfio_pci_zap_and_down_write_memory_lock(vdev); - vfio_pci_dma_buf_move(vdev, true); - } else { + if (!new_mem) + vfio_pci_lock_revoke_bars(vdev); + else down_write(&vdev->memory_lock); - }
/* * If the user is writing mem/io enable (new_mem/io) and we @@ -631,7 +629,7 @@ static int vfio_basic_config_write(struct vfio_pci_core_device *vdev, int pos, *virt_cmd |= cpu_to_le16(new_cmd & mask);
if (__vfio_pci_memory_enabled(vdev)) - vfio_pci_dma_buf_move(vdev, false); + vfio_pci_unrevoke_bars(vdev); up_write(&vdev->memory_lock); }
@@ -712,16 +710,14 @@ static int __init init_pci_cap_basic_perm(struct perm_bits *perm) static void vfio_lock_and_set_power_state(struct vfio_pci_core_device *vdev, pci_power_t state) { - if (state >= PCI_D3hot) { - vfio_pci_zap_and_down_write_memory_lock(vdev); - vfio_pci_dma_buf_move(vdev, true); - } else { + if (state >= PCI_D3hot) + vfio_pci_lock_revoke_bars(vdev); + else down_write(&vdev->memory_lock); - }
vfio_pci_set_power_state(vdev, state); if (__vfio_pci_memory_enabled(vdev)) - vfio_pci_dma_buf_move(vdev, false); + vfio_pci_unrevoke_bars(vdev); up_write(&vdev->memory_lock); }
@@ -908,11 +904,10 @@ static int vfio_exp_config_write(struct vfio_pci_core_device *vdev, int pos, &cap);
if (!ret && (cap & PCI_EXP_DEVCAP_FLR)) { - vfio_pci_zap_and_down_write_memory_lock(vdev); - vfio_pci_dma_buf_move(vdev, true); + vfio_pci_lock_revoke_bars(vdev); pci_try_reset_function(vdev->pdev); if (__vfio_pci_memory_enabled(vdev)) - vfio_pci_dma_buf_move(vdev, false); + vfio_pci_unrevoke_bars(vdev); up_write(&vdev->memory_lock); } } @@ -993,11 +988,10 @@ static int vfio_af_config_write(struct vfio_pci_core_device *vdev, int pos, &cap);
if (!ret && (cap & PCI_AF_CAP_FLR) && (cap & PCI_AF_CAP_TP)) { - vfio_pci_zap_and_down_write_memory_lock(vdev); - vfio_pci_dma_buf_move(vdev, true); + vfio_pci_lock_revoke_bars(vdev); pci_try_reset_function(vdev->pdev); if (__vfio_pci_memory_enabled(vdev)) - vfio_pci_dma_buf_move(vdev, false); + vfio_pci_unrevoke_bars(vdev); up_write(&vdev->memory_lock); } } diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c index 8dd69948e0b7..760a816e6de0 100644 --- a/drivers/vfio/pci/vfio_pci_core.c +++ b/drivers/vfio/pci/vfio_pci_core.c @@ -375,8 +375,7 @@ static int vfio_pci_runtime_pm_entry(struct vfio_pci_core_device *vdev, * The vdev power related flags are protected with 'memory_lock' * semaphore. */ - vfio_pci_zap_and_down_write_memory_lock(vdev); - vfio_pci_dma_buf_move(vdev, true); + vfio_pci_lock_revoke_bars(vdev);
if (vdev->pm_runtime_engaged) { up_write(&vdev->memory_lock); @@ -462,7 +461,7 @@ static void vfio_pci_runtime_pm_exit(struct vfio_pci_core_device *vdev) down_write(&vdev->memory_lock); __vfio_pci_runtime_pm_exit(vdev); if (__vfio_pci_memory_enabled(vdev)) - vfio_pci_dma_buf_move(vdev, false); + vfio_pci_unrevoke_bars(vdev); up_write(&vdev->memory_lock); }
@@ -531,7 +530,7 @@ static int vfio_pci_core_runtime_resume(struct device *dev) vdev->pm_wake_eventfd_ctx = NULL; __vfio_pci_runtime_pm_exit(vdev); if (__vfio_pci_memory_enabled(vdev)) - vfio_pci_dma_buf_move(vdev, false); + vfio_pci_unrevoke_bars(vdev); eventfd_signal(ctx); eventfd_ctx_put(ctx); } @@ -1319,6 +1318,8 @@ static int vfio_pci_ioctl_set_irqs(struct vfio_pci_core_device *vdev, return ret; }
+static void vfio_pci_revoke_bars(struct vfio_pci_core_device *vdev); + static int vfio_pci_ioctl_reset(struct vfio_pci_core_device *vdev, void __user *arg) { @@ -1327,7 +1328,7 @@ static int vfio_pci_ioctl_reset(struct vfio_pci_core_device *vdev, if (!vdev->reset_works) return -EINVAL;
- vfio_pci_zap_and_down_write_memory_lock(vdev); + down_write(&vdev->memory_lock);
/* * This function can be invoked while the power state is non-D0. If @@ -1337,13 +1338,18 @@ static int vfio_pci_ioctl_reset(struct vfio_pci_core_device *vdev, * have NoSoftRst-, the reset function can cause the PCI config space * reset without restoring the original state (saved locally in * 'vdev->pm_save'). + * + * The zap is done after making the device accessible in D0, + * because a DMABUF importer could access the device as part + * of its revocation cleanup. */ vfio_pci_set_power_state(vdev, PCI_D0);
- vfio_pci_dma_buf_move(vdev, true); + vfio_pci_revoke_bars(vdev); + ret = pci_try_reset_function(vdev->pdev); if (__vfio_pci_memory_enabled(vdev)) - vfio_pci_dma_buf_move(vdev, false); + vfio_pci_unrevoke_bars(vdev); up_write(&vdev->memory_lock);
return ret; @@ -1711,20 +1717,37 @@ ssize_t vfio_pci_core_write(struct vfio_device *core_vdev, const char __user *bu } EXPORT_SYMBOL_GPL(vfio_pci_core_write);
-static void vfio_pci_zap_bars(struct vfio_pci_core_device *vdev) +static void vfio_pci_revoke_bars(struct vfio_pci_core_device *vdev) { - struct vfio_device *core_vdev = &vdev->vdev; - loff_t start = VFIO_PCI_INDEX_TO_OFFSET(VFIO_PCI_BAR0_REGION_INDEX); - loff_t end = VFIO_PCI_INDEX_TO_OFFSET(VFIO_PCI_ROM_REGION_INDEX); - loff_t len = end - start; + lockdep_assert_held_write(&vdev->memory_lock); + vfio_pci_dma_buf_move(vdev, true);
- unmap_mapping_range(core_vdev->inode->i_mapping, start, len, true); + /* + * If a driver could possibly create BAR mappings in the + * vdev's address_space, do an additional zap on revoke. See + * vfio_pci_core_init_dev(). + */ + if (vdev->zap_bars_on_revoke) { + struct vfio_device *core_vdev = &vdev->vdev; + loff_t start = VFIO_PCI_INDEX_TO_OFFSET(VFIO_PCI_BAR0_REGION_INDEX); + loff_t end = VFIO_PCI_INDEX_TO_OFFSET(VFIO_PCI_ROM_REGION_INDEX); + loff_t len = end - start; + + unmap_mapping_range(core_vdev->inode->i_mapping, + start, len, true); + } }
-void vfio_pci_zap_and_down_write_memory_lock(struct vfio_pci_core_device *vdev) +void vfio_pci_lock_revoke_bars(struct vfio_pci_core_device *vdev) { down_write(&vdev->memory_lock); - vfio_pci_zap_bars(vdev); + vfio_pci_revoke_bars(vdev); +} + +void vfio_pci_unrevoke_bars(struct vfio_pci_core_device *vdev) +{ + lockdep_assert_held_write(&vdev->memory_lock); + vfio_pci_dma_buf_move(vdev, false); }
u16 vfio_pci_memory_lock_and_enable(struct vfio_pci_core_device *vdev) @@ -2222,6 +2245,16 @@ int vfio_pci_core_init_dev(struct vfio_device *core_vdev) init_rwsem(&vdev->dmabuf_lock); xa_init(&vdev->ctx);
+ /* + * If a driver overrides .mmap, it has to be assumed that it + * might not use the DMABUF-backed core mmap; this flag + * enables a zap at revoke time. A driver can opt out by + * clearing this flag at init, if their .mmap override calls + * down to vfio_pci_core_mmap(). + */ + if (vdev->vdev.ops->mmap != vfio_pci_core_mmap) + vdev->zap_bars_on_revoke = true; + return 0; } EXPORT_SYMBOL_GPL(vfio_pci_core_init_dev); @@ -2589,9 +2622,10 @@ static int vfio_pci_dev_set_hot_reset(struct vfio_device_set *dev_set, }
/* - * Take the memory write lock for each device and zap BAR - * mappings to prevent the user accessing the device while in - * reset. Locking multiple devices is prone to deadlock, + * Take the memory write lock for each device and + * zap/revoke BAR mappings to prevent the user (or + * peers) accessing the device while in reset. + * Locking multiple devices is prone to deadlock, * runaway and unwind if we hit contention. */ if (!down_write_trylock(&vdev->memory_lock)) { @@ -2599,8 +2633,7 @@ static int vfio_pci_dev_set_hot_reset(struct vfio_device_set *dev_set, break; }
- vfio_pci_dma_buf_move(vdev, true); - vfio_pci_zap_bars(vdev); + vfio_pci_revoke_bars(vdev); }
if (!list_entry_is_head(vdev, @@ -2630,7 +2663,7 @@ static int vfio_pci_dev_set_hot_reset(struct vfio_device_set *dev_set, list_for_each_entry_from_reverse(vdev, &dev_set->device_list, vdev.dev_set_list) { if (vdev->vdev.open_count && __vfio_pci_memory_enabled(vdev)) - vfio_pci_dma_buf_move(vdev, false); + vfio_pci_unrevoke_bars(vdev); up_write(&vdev->memory_lock); }
diff --git a/drivers/vfio/pci/vfio_pci_priv.h b/drivers/vfio/pci/vfio_pci_priv.h index c62fbbf1625e..b0e0ac295db4 100644 --- a/drivers/vfio/pci/vfio_pci_priv.h +++ b/drivers/vfio/pci/vfio_pci_priv.h @@ -83,7 +83,8 @@ void vfio_config_free(struct vfio_pci_core_device *vdev); int vfio_pci_set_power_state(struct vfio_pci_core_device *vdev, pci_power_t state);
-void vfio_pci_zap_and_down_write_memory_lock(struct vfio_pci_core_device *vdev); +void vfio_pci_lock_revoke_bars(struct vfio_pci_core_device *vdev); +void vfio_pci_unrevoke_bars(struct vfio_pci_core_device *vdev); u16 vfio_pci_memory_lock_and_enable(struct vfio_pci_core_device *vdev); void vfio_pci_memory_unlock_and_restore(struct vfio_pci_core_device *vdev, u16 cmd); diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h index f99b152edcca..44891fdb7c76 100644 --- a/include/linux/vfio_pci_core.h +++ b/include/linux/vfio_pci_core.h @@ -129,6 +129,7 @@ struct vfio_pci_core_device { bool disable_idle_d3:1; bool nointxmask:1; bool disable_vga:1; + bool zap_bars_on_revoke:1; /* Flags modified at runtime - dedicated storage unit */ bool needs_reset; bool pm_intx_masked;
A VFIO DMABUF can export a subset of a BAR to userspace by fd; add support for mmap() of this fd. This provides another route for a process to map BARs, in which the process can only map a specific subset of a BAR represented by the exported DMABUF.
mmap() support enables userspace driver designs that safely delegate access to BAR sub-ranges to other client processes by sharing a DMABUF fd, without having to share the (omnipotent) VFIO device fd with them.
Since the main VFIO BAR mmap() is now DMABUF-aware, the new mmap() reuses the existing vm_ops.
The lifecycle of an exported DMABUF remains decoupled from that of the device fd it came from, i.e. the device fd could be closed with DMABUF VMAs present, meaning a fault on a VMA could happen concurrently with vfio_pci_dma_buf_cleanup().
To deal with this scenario, the fault handler now temporarily takes a VFIO device registration to ensure the vdev remains valid, and then vdev locks can be taken.
Signed-off-by: Matt Evans matt@ozlabs.org --- drivers/vfio/pci/vfio_pci_core.c | 84 ++++++++++++++++++++++++++---- drivers/vfio/pci/vfio_pci_dmabuf.c | 47 +++++++++++++++++ drivers/vfio/pci/vfio_pci_priv.h | 1 + 3 files changed, 123 insertions(+), 9 deletions(-)
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c index 760a816e6de0..711ef54fd91e 100644 --- a/drivers/vfio/pci/vfio_pci_core.c +++ b/drivers/vfio/pci/vfio_pci_core.c @@ -13,6 +13,8 @@ #include <linux/aperture.h> #include <linux/debugfs.h> #include <linux/device.h> +#include <linux/dma-buf.h> +#include <linux/dma-resv.h> #include <linux/eventfd.h> #include <linux/file.h> #include <linux/interrupt.h> @@ -1802,21 +1804,79 @@ static vm_fault_t vfio_pci_mmap_huge_fault(struct vm_fault *vmf, vm_fault_t ret = VM_FAULT_SIGBUS;
/* - * We can rely on the existence of both a DMABUF (priv) and - * the VFIO device it was exported from (vdev). This fault's - * VMA was established using vfio_pci_core_mmap_prep_dmabuf() - * which transfers ownership of the VFIO device fd to the - * DMABUF, and so the VFIO device is held open because the - * VMA's vm_file (DMABUF) is open. + * The only thing this can rely on is that the DMABUF relating + * to the VMA's vm_file exists (priv). * - * Since vfio_pci_dma_buf_cleanup() cannot have happened, - * vdev must be valid; we can take vdev locks. + * A DMABUF for a VFIO device fd mmap() holds a reference to + * the original VFIO device fd, but an explicitly-exported + * DMABUF does not. The original fd might have closed, + * meaning this fault can race with + * vfio_pci_dma_buf_cleanup(), meaning the buffer could have + * been revoked (in which case priv->vdev might be NULL), and + * the VFIO device registration might have been dropped. + * + * With the goal of taking vdev locks 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 then vdev->dmabuf_lock: + * - Either the DMABUF is usable, or has been cleaned up. + * - It's not necessary to also take the resv lock, because + * the status/vdev can't change while dmabuf_lock is held. + * - Test the DMABUF revocation status again: if it was + * revoked between 1 and 4, return a SIGBUS. Otherwise, + * return a PFN. + * + * 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_ratelimited(&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); + /* memory_lock for vfio_pci_vmf_insert_pfn() */ down_read(&vdev->memory_lock); - /* Test revocation status under dmabuf_lock */ + /* Re-test revocation status under dmabuf_lock */ down_read(&vdev->dmabuf_lock); if (!priv->revoked) { int pres = vfio_pci_dma_buf_find_pfn(vdev, priv, vma, @@ -1837,6 +1897,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; }
@@ -1852,6 +1913,11 @@ static const struct vm_operations_struct vfio_pci_mmap_ops = { #endif };
+void vfio_pci_set_vma_ops(struct vm_area_struct *vma) +{ + vma->vm_ops = &vfio_pci_mmap_ops; +} + int vfio_pci_core_mmap(struct vfio_device *core_vdev, struct vm_area_struct *vma) { struct vfio_pci_core_device *vdev = diff --git a/drivers/vfio/pci/vfio_pci_dmabuf.c b/drivers/vfio/pci/vfio_pci_dmabuf.c index 0126a0582072..1c85c15290d4 100644 --- a/drivers/vfio/pci/vfio_pci_dmabuf.c +++ b/drivers/vfio/pci/vfio_pci_dmabuf.c @@ -27,6 +27,50 @@ 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; + + /* + * dma_buf_mmap_internal() has asserted that the VMA is + * contained within the DMABUF size before calling this. + * + * Also, 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 (priv->revoked) + return -ENODEV; + /* + * Make clear that anything with an offset adjustment is + * explicitly unsupported, as vfio_pci_dma_buf_find_pfn() + * maths would underflow; this doesn't happen through the + * regular DMABUF export path used with this mmap(). A DMABUF + * implicitly created for BAR mmap could have adjust > 0, but + * these can't currently be re-opened and mmap()ed again. + * Catch here in case that assumption ever changes. + */ + if (priv->vma_pgoff_adjust) + return -EINVAL; + if ((vma->vm_flags & VM_SHARED) == 0) + return -EINVAL; + + 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; + vfio_pci_set_vma_ops(vma); + + return 0; +} #else static int vfio_pci_dma_buf_attach(struct dma_buf *dmabuf, struct dma_buf_attachment *attachment) @@ -104,6 +148,9 @@ static void vfio_pci_dma_buf_release(struct dma_buf *dmabuf)
static const struct dma_buf_ops vfio_pci_dmabuf_ops = { .attach = vfio_pci_dma_buf_attach, +#ifdef CONFIG_VFIO_PCI_DMABUF + .mmap = vfio_pci_dma_buf_mmap, +#endif .map_dma_buf = vfio_pci_dma_buf_map, .unmap_dma_buf = vfio_pci_dma_buf_unmap, .release = vfio_pci_dma_buf_release, diff --git a/drivers/vfio/pci/vfio_pci_priv.h b/drivers/vfio/pci/vfio_pci_priv.h index b0e0ac295db4..cd432f52c38f 100644 --- a/drivers/vfio/pci/vfio_pci_priv.h +++ b/drivers/vfio/pci/vfio_pci_priv.h @@ -142,6 +142,7 @@ int vfio_pci_core_mmap_prep_dmabuf(struct vfio_pci_core_device *vdev, unsigned int res_index); void vfio_pci_dma_buf_cleanup(struct vfio_pci_core_device *vdev); void vfio_pci_dma_buf_move(struct vfio_pci_core_device *vdev, bool revoked); +void vfio_pci_set_vma_ops(struct vm_area_struct *vma);
#ifdef CONFIG_VFIO_PCI_DMABUF int vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags,
Expand the VFIO DMABUF revocation state to three states: Not revoked, temporarily revoked, and permanently revoked.
The first two are for existing transient revocation, e.g. across a function reset, and the DMABUF is put into the last in response to a new VFIO feature VFIO_DEVICE_FEATURE_DMA_BUF_REVOKE.
VFIO_DEVICE_FEATURE_DMA_BUF_REVOKE passes a DMABUF by fd and requests that the DMABUF is permanently revoked. On success, it's guaranteed that the buffer can never be imported/attached/mmap()ed in future, and that dynamic imports have been cleanly unmapped.
This is useful for lifecycle management, to reclaim VFIO PCI BAR ranges previously delegated to a subordinate client process: by revoking, the driver process can ensure that the loaned resources are made inaccessible when the client is deemed "done". The original DMABUF is defunct, and BAR resources can then be safely re-exported for use by new clients.
Refactor the revocation code out of vfio_pci_dma_buf_move() to a function common to move and the new feature request path. Note: this now only calls dma_buf_invalidate_mappings()/dma_resv_wait_timeout() on the revoke path, whereas vfio_pci_dma_buf_move() originally called them for both revoke and (unnecessarily) un-revoke.
Signed-off-by: Matt Evans matt@ozlabs.org --- drivers/vfio/pci/vfio_pci_core.c | 6 +- drivers/vfio/pci/vfio_pci_dmabuf.c | 177 ++++++++++++++++++++++------- drivers/vfio/pci/vfio_pci_priv.h | 19 +++- include/uapi/linux/vfio.h | 24 ++++ 4 files changed, 184 insertions(+), 42 deletions(-)
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c index 711ef54fd91e..911e248aa764 100644 --- a/drivers/vfio/pci/vfio_pci_core.c +++ b/drivers/vfio/pci/vfio_pci_core.c @@ -1640,6 +1640,8 @@ int vfio_pci_core_ioctl_feature(struct vfio_device *device, u32 flags, return vfio_pci_core_feature_token(vdev, flags, arg, argsz); case VFIO_DEVICE_FEATURE_DMA_BUF: return vfio_pci_core_feature_dma_buf(vdev, flags, arg, argsz); + case VFIO_DEVICE_FEATURE_DMA_BUF_REVOKE: + return vfio_pci_core_feature_dma_buf_revoke(vdev, flags, arg, argsz); default: return -ENOTTY; } @@ -1850,7 +1852,7 @@ static vm_fault_t vfio_pci_mmap_huge_fault(struct vm_fault *vmf,
dma_resv_lock(priv->dmabuf->resv, NULL);
- if (priv->revoked) { + if (priv->status != VFIO_PCI_DMABUF_OK) { 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); @@ -1878,7 +1880,7 @@ static vm_fault_t vfio_pci_mmap_huge_fault(struct vm_fault *vmf, down_read(&vdev->memory_lock); /* Re-test revocation status under dmabuf_lock */ down_read(&vdev->dmabuf_lock); - if (!priv->revoked) { + if (priv->status == VFIO_PCI_DMABUF_OK) { int pres = vfio_pci_dma_buf_find_pfn(vdev, priv, vma, vmf->address, order, &pfn); diff --git a/drivers/vfio/pci/vfio_pci_dmabuf.c b/drivers/vfio/pci/vfio_pci_dmabuf.c index 1c85c15290d4..7cda2bd00d25 100644 --- a/drivers/vfio/pci/vfio_pci_dmabuf.c +++ b/drivers/vfio/pci/vfio_pci_dmabuf.c @@ -19,7 +19,7 @@ static int vfio_pci_dma_buf_attach(struct dma_buf *dmabuf, if (!attachment->peer2peer) return -EOPNOTSUPP;
- if (priv->revoked) + if (READ_ONCE(priv->status) != VFIO_PCI_DMABUF_OK) return -ENODEV;
if (!dma_buf_attach_revocable(attachment)) @@ -44,7 +44,7 @@ static int vfio_pci_dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct * * still safe because the fault handler ultimately prevents * access to a revoked buffer if it isn't caught here. */ - if (priv->revoked) + if (READ_ONCE(priv->status) != VFIO_PCI_DMABUF_OK) return -ENODEV; /* * Make clear that anything with an offset adjustment is @@ -101,7 +101,7 @@ vfio_pci_dma_buf_map(struct dma_buf_attachment *attachment,
dma_resv_assert_held(priv->dmabuf->resv);
- if (priv->revoked) + if (priv->status != VFIO_PCI_DMABUF_OK) return ERR_PTR(-ENODEV);
ret = dma_buf_phys_vec_to_sgt(attachment, priv->provider, @@ -224,7 +224,7 @@ int vfio_pci_dma_buf_find_pfn(struct vfio_pci_core_device *vdev, /* This prevents the dmabuf revocation state from changing under us */ lockdep_assert_held(&vdev->dmabuf_lock);
- if (unlikely(priv->vdev != vdev || priv->revoked)) + if (unlikely(priv->vdev != vdev || priv->status != VFIO_PCI_DMABUF_OK)) return -ENODEV;
if (rounded_page_addr < vma->vm_start || rounded_page_end > vma->vm_end) { @@ -365,7 +365,8 @@ static int vfio_pci_dmabuf_export(struct vfio_pci_core_device *vdev,
down_write(&vdev->dmabuf_lock); dma_resv_lock(priv->dmabuf->resv, NULL); - priv->revoked = vdev->bars_revoked; + priv->status = vdev->bars_revoked ? VFIO_PCI_DMABUF_TEMP_REVOKED : + VFIO_PCI_DMABUF_OK; list_add_tail(&priv->dmabufs_elm, &vdev->dmabufs); dma_resv_unlock(priv->dmabuf->resv); up_write(&vdev->dmabuf_lock); @@ -498,7 +499,7 @@ int vfio_pci_dma_buf_iommufd_map(struct dma_buf_attachment *attachment, return -EOPNOTSUPP;
priv = attachment->dmabuf->priv; - if (priv->revoked) + if (priv->status != VFIO_PCI_DMABUF_OK) return -ENODEV;
/* More than one range to iommufd will require proper DMABUF support */ @@ -676,6 +677,63 @@ int vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags, } #endif /* CONFIG_VFIO_PCI_DMABUF */
+/* Set the DMABUF's revocation status (OK or temporarily/permanently revoked) */ +static void vfio_pci_dma_buf_set_status(struct vfio_pci_dma_buf *priv, + enum vfio_pci_dma_buf_status new_status) +{ + bool was_revoked; + + /* + * Changes to the DMABUF's revocation status are synchronised + * using dmabuf_lock: + */ + lockdep_assert_held_write(&priv->vdev->dmabuf_lock); + + if (priv->status == VFIO_PCI_DMABUF_PERM_REVOKED || + priv->status == new_status) + return; + + dma_resv_lock(priv->dmabuf->resv, NULL); + was_revoked = (priv->status == VFIO_PCI_DMABUF_TEMP_REVOKED); + + if (new_status != VFIO_PCI_DMABUF_OK) { + priv->status = new_status; /* Temp or permanently revoked */ + + if (was_revoked) { + /* + * TEMP_REVOKED is being upgraded to + * PERM_REVOKED. The buffer is already gone, + * don't wait on it again. + */ + dma_resv_unlock(priv->dmabuf->resv); + return; + } + dma_buf_invalidate_mappings(priv->dmabuf); + dma_resv_wait_timeout(priv->dmabuf->resv, + DMA_RESV_USAGE_BOOKKEEP, false, + MAX_SCHEDULE_TIMEOUT); + dma_resv_unlock(priv->dmabuf->resv); + kref_put(&priv->kref, vfio_pci_dma_buf_done); + wait_for_completion(&priv->comp); + unmap_mapping_range(priv->dmabuf->file->f_mapping, + 0, 0, true); + /* + * Re-arm the registered kref reference and the + * completion so the post-revoke state matches the + * post-creation state. An un-revoke followed by a + * new mapping needs the kref to be non-zero before + * kref_get(), and vfio_pci_dma_buf_cleanup() + * delegates its drain back through this revoke + * path on a possibly-already-revoked dma-buf. + */ + kref_init(&priv->kref); + reinit_completion(&priv->comp); + } else { + priv->status = VFIO_PCI_DMABUF_OK; + dma_resv_unlock(priv->dmabuf->resv); + } +} + void vfio_pci_dma_buf_move(struct vfio_pci_core_device *vdev, bool revoked) { struct vfio_pci_dma_buf *priv; @@ -688,38 +746,9 @@ void vfio_pci_dma_buf_move(struct vfio_pci_core_device *vdev, bool revoked) list_for_each_entry_safe(priv, tmp, &vdev->dmabufs, dmabufs_elm) { if (!get_file_active(&priv->dmabuf->file)) continue; - - if (priv->revoked != revoked) { - dma_resv_lock(priv->dmabuf->resv, NULL); - if (revoked) - priv->revoked = true; - dma_buf_invalidate_mappings(priv->dmabuf); - dma_resv_wait_timeout(priv->dmabuf->resv, - DMA_RESV_USAGE_BOOKKEEP, false, - MAX_SCHEDULE_TIMEOUT); - dma_resv_unlock(priv->dmabuf->resv); - if (revoked) { - kref_put(&priv->kref, vfio_pci_dma_buf_done); - wait_for_completion(&priv->comp); - unmap_mapping_range(priv->dmabuf->file->f_mapping, - 0, 0, true); - /* - * Re-arm the registered kref reference and the - * completion so the post-revoke state matches the - * post-creation state. An un-revoke followed by a - * new mapping needs the kref to be non-zero before - * kref_get(), and vfio_pci_dma_buf_cleanup() - * delegates its drain back through this revoke - * path on a possibly-already-revoked dma-buf. - */ - kref_init(&priv->kref); - reinit_completion(&priv->comp); - } else { - dma_resv_lock(priv->dmabuf->resv, NULL); - priv->revoked = false; - dma_resv_unlock(priv->dmabuf->resv); - } - } + vfio_pci_dma_buf_set_status(priv, revoked ? + VFIO_PCI_DMABUF_TEMP_REVOKED : + VFIO_PCI_DMABUF_OK); fput(priv->dmabuf->file); } up_write(&vdev->dmabuf_lock); @@ -747,10 +776,80 @@ void vfio_pci_dma_buf_cleanup(struct vfio_pci_core_device *vdev) continue;
list_del_init(&priv->dmabufs_elm); - priv->vdev = NULL; + WRITE_ONCE(priv->vdev, NULL); vfio_device_put_registration(&vdev->vdev); fput(priv->dmabuf->file); } up_write(&vdev->dmabuf_lock); up_write(&vdev->memory_lock); } + +#ifdef CONFIG_VFIO_PCI_DMABUF +int vfio_pci_core_feature_dma_buf_revoke( + struct vfio_pci_core_device *vdev, u32 flags, + struct vfio_device_feature_dma_buf_revoke __user *arg, + size_t argsz) +{ + struct vfio_device_feature_dma_buf_revoke db_revoke; + struct vfio_pci_dma_buf *priv; + struct dma_buf *dmabuf; + int ret; + + if (!vdev->pci_ops || !vdev->pci_ops->get_dmabuf_phys) + return -EOPNOTSUPP; + + ret = vfio_check_feature(flags, argsz, + VFIO_DEVICE_FEATURE_SET, + sizeof(db_revoke)); + if (ret != 1) + return ret; + + if (copy_from_user(&db_revoke, arg, sizeof(db_revoke))) + return -EFAULT; + + dmabuf = dma_buf_get(db_revoke.dmabuf_fd); + if (IS_ERR(dmabuf)) + return PTR_ERR(dmabuf); + + priv = dmabuf->priv; + /* + * Sanity-check the DMABUF is really a vfio_pci_dma_buf _and_ + * relates to the VFIO device it was provided with. + * + * If the DMABUF relates to this vdev then priv->vdev is + * stable because this open fd prevents cleanup. + * + * If it relates to a different vdev, reading priv->vdev might + * race with a concurrent cleanup on that device. But if so, + * it points to a non-matching vdev or NULL and is unusable + * either way. + */ + if (dmabuf->ops != &vfio_pci_dmabuf_ops || + READ_ONCE(priv->vdev) != vdev) { + ret = -ENODEV; + goto out_put_buf; + } + + /* + * memory_lock(R) is taken to stop vfio_pci_dev_set_hot_reset() + * from getting it and then blocking all devices in the dev_set behind + * this revoke's drain. + */ + down_read(&vdev->memory_lock); + down_write(&vdev->dmabuf_lock); + if (priv->status == VFIO_PCI_DMABUF_PERM_REVOKED) { + ret = -EBADFD; + } else { + vfio_pci_dma_buf_set_status(priv, + VFIO_PCI_DMABUF_PERM_REVOKED); + ret = 0; + } + up_write(&vdev->dmabuf_lock); + up_read(&vdev->memory_lock); + +out_put_buf: + dma_buf_put(dmabuf); + + return ret; +} +#endif /* CONFIG_VFIO_PCI_DMABUF */ diff --git a/drivers/vfio/pci/vfio_pci_priv.h b/drivers/vfio/pci/vfio_pci_priv.h index cd432f52c38f..9cf66c19f798 100644 --- a/drivers/vfio/pci/vfio_pci_priv.h +++ b/drivers/vfio/pci/vfio_pci_priv.h @@ -23,6 +23,12 @@ struct vfio_pci_ioeventfd { bool test_mem; };
+enum vfio_pci_dma_buf_status { + VFIO_PCI_DMABUF_OK = 0, + VFIO_PCI_DMABUF_TEMP_REVOKED = 1, + VFIO_PCI_DMABUF_PERM_REVOKED = 2, +}; + struct vfio_pci_dma_buf { struct dma_buf *dmabuf; struct vfio_pci_core_device *vdev; @@ -35,7 +41,7 @@ struct vfio_pci_dma_buf { struct kref kref; struct completion comp; unsigned long vma_pgoff_adjust; - u8 revoked : 1; + enum vfio_pci_dma_buf_status status; };
bool vfio_pci_intx_mask(struct vfio_pci_core_device *vdev); @@ -148,6 +154,10 @@ void vfio_pci_set_vma_ops(struct vm_area_struct *vma); int vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags, struct vfio_device_feature_dma_buf __user *arg, size_t argsz); +int vfio_pci_core_feature_dma_buf_revoke( + struct vfio_pci_core_device *vdev, u32 flags, + struct vfio_device_feature_dma_buf_revoke __user *arg, + size_t argsz); #else static inline int vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags, @@ -156,6 +166,13 @@ vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags, { return -ENOTTY; } +static inline int vfio_pci_core_feature_dma_buf_revoke( + struct vfio_pci_core_device *vdev, u32 flags, + struct vfio_device_feature_dma_buf_revoke __user *arg, + size_t argsz) +{ + return -ENOTTY; +} #endif
#endif diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h index 5de618a3a5ee..8c1d50275a41 100644 --- a/include/uapi/linux/vfio.h +++ b/include/uapi/linux/vfio.h @@ -1534,6 +1534,30 @@ struct vfio_device_feature_dma_buf { */ #define VFIO_DEVICE_FEATURE_MIG_PRECOPY_INFOv2 12
+/** + * Given a dma_buf fd previously exported from the same device by + * VFIO_DEVICE_FEATURE_DMA_BUF, a SET of this feature requests that + * access to the corresponding DMABUF is immediately and permanently + * revoked. On successful return, the buffer is not accessible + * through any mmap() or dma-buf import. The buffer is permanently + * disabled, and VFIO refuses all map, mmap, attach, etc. requests. + * + * Return: 0 on success, -1 and errno is set on failure: + * + * EBADF, EINVAL: dmabuf_fd is not a DMABUF fd. + * EOPNOTSUPP: The VFIO device does not support DMABUF export. + * ENODEV: The DMABUF was not exported from this device. + * EBADFD: The DMABUF is already permanently revoked. + * + * Additionally, common errors can occur: EFAULT accessing the struct, + * or EINVAL requesting an unsupported feature op. + */ +#define VFIO_DEVICE_FEATURE_DMA_BUF_REVOKE 13 + +struct vfio_device_feature_dma_buf_revoke { + __s32 dmabuf_fd; +}; + /* -------- API for Type1 VFIO IOMMU -------- */
/**
On Fri, Sep 11, 2026 at 10:41:57PM +0100, Matt Evans wrote:
Expand the VFIO DMABUF revocation state to three states: Not revoked, temporarily revoked, and permanently revoked.
The thing is that "temporarily revoked" is actually the standard invalidate_mappings/move_notify mechanism of DMABUF, which wasn't good for VFIO.
Thanks
On Sun, Sep 13, 2026 at 07:52:44PM +0300, Leon Romanovsky wrote:
On Fri, Sep 11, 2026 at 10:41:57PM +0100, Matt Evans wrote:
Expand the VFIO DMABUF revocation state to three states: Not revoked, temporarily revoked, and permanently revoked.
The thing is that "temporarily revoked" is actually the standard invalidate_mappings/move_notify mechanism of DMABUF, which wasn't good for VFIO.
I think temporarily revokes here means it is revoked from a dmabuf perspective
Just that VFIO can make it's internal dmabuf work again, there won't be a notification to any importer or an expectation that something like iommufd will re-establish mapping automatically.
This is principally a kernel self protection mechanism where the userspace was expected to have removed the dmabuf before issuing a reset/etc. If they didn't then the kernel plonks it and userspace gets a mess to clean up.
Jason
On Mon, Sep 14, 2026 at 08:36:47AM -0300, Jason Gunthorpe wrote:
On Sun, Sep 13, 2026 at 07:52:44PM +0300, Leon Romanovsky wrote:
On Fri, Sep 11, 2026 at 10:41:57PM +0100, Matt Evans wrote:
Expand the VFIO DMABUF revocation state to three states: Not revoked, temporarily revoked, and permanently revoked.
The thing is that "temporarily revoked" is actually the standard invalidate_mappings/move_notify mechanism of DMABUF, which wasn't good for VFIO.
I think temporarily revokes here means it is revoked from a dmabuf perspective
My guess is that this is more of a "change owner" operation than a revoke operation.
The main issue here is that we have to guess the semantics instead of having a properly named and documented operation. As Christian pointed out, the patches describe what they are doing well, but they do not explain why they are doing it.
Just that VFIO can make it's internal dmabuf work again, there won't be a notification to any importer or an expectation that something like iommufd will re-establish mapping automatically.
This is principally a kernel self protection mechanism where the userspace was expected to have removed the dmabuf before issuing a reset/etc. If they didn't then the kernel plonks it and userspace gets a mess to clean up.
Revoke/invalidate means that the importer should stop accessing the buffer provided by the exporter. It does not specify what the exporter should do afterwards.
So I still think that "temporarily revoked" is closer to invalidate_mappings, with some additional dma-buf documentation to lose the expectation that the buffer will become available again.
Thanks
Jason
On Mon, Sep 14, 2026 at 02:54:56PM +0300, Leon Romanovsky wrote:
So I still think that "temporarily revoked" is closer to invalidate_mappings, with some additional dma-buf documentation to lose the expectation that the buffer will become available again.
It really isn't, there is no expectation her that the importer has to put things back like with move.
From a DMABUF API this is very much just plain revoke.
Jason
On Mon, Sep 14, 2026 at 08:58:26AM -0300, Jason Gunthorpe wrote:
On Mon, Sep 14, 2026 at 02:54:56PM +0300, Leon Romanovsky wrote:
So I still think that "temporarily revoked" is closer to invalidate_mappings, with some additional dma-buf documentation to lose the expectation that the buffer will become available again.
It really isn't, there is no expectation her that the importer has to put things back like with move.
As we discussed with Christian, "move" doesn't have such expectation. https://lore.kernel.org/all/afb19db0-4154-443c-9d39-f905e7d17806@amd.com/
Thanks
From a DMABUF API this is very much just plain revoke.
Jason
On Mon, Sep 14, 2026 at 03:06:18PM +0300, Leon Romanovsky wrote:
On Mon, Sep 14, 2026 at 08:58:26AM -0300, Jason Gunthorpe wrote:
On Mon, Sep 14, 2026 at 02:54:56PM +0300, Leon Romanovsky wrote:
So I still think that "temporarily revoked" is closer to invalidate_mappings, with some additional dma-buf documentation to lose the expectation that the buffer will become available again.
It really isn't, there is no expectation her that the importer has to put things back like with move.
As we discussed with Christian, "move" doesn't have such expectation. https://lore.kernel.org/all/afb19db0-4154-443c-9d39-f905e7d17806@amd.com/
"the exporter can move the buffer back when a mapping is created" is exactly what I'm saying is not required here, iommufd will not implement this, it only does permanent revoke.
Jason
Hi Leon,
On 14/09/2026 12:54, Leon Romanovsky wrote:
On Mon, Sep 14, 2026 at 08:36:47AM -0300, Jason Gunthorpe wrote:
On Sun, Sep 13, 2026 at 07:52:44PM +0300, Leon Romanovsky wrote:
On Fri, Sep 11, 2026 at 10:41:57PM +0100, Matt Evans wrote:
Expand the VFIO DMABUF revocation state to three states: Not revoked, temporarily revoked, and permanently revoked.
The thing is that "temporarily revoked" is actually the standard invalidate_mappings/move_notify mechanism of DMABUF, which wasn't good for VFIO.
I think temporarily revokes here means it is revoked from a dmabuf perspective
My guess is that this is more of a "change owner" operation than a revoke operation.
The main issue here is that we have to guess the semantics instead of having a properly named and documented operation.
Apologies if the cover letter for the series and patch commit message (which cover this) are unclear about the motivations and semantics. On the commit message, can you suggest clarifications:
"This is useful for lifecycle management, to reclaim VFIO PCI BAR ranges previously delegated to a subordinate client process: by revoking, the driver process can ensure that the loaned resources are made inaccessible when the client is deemed "done". The original DMABUF is defunct, and BAR resources can then be safely re-exported for use by new clients."
Given what I'll explain below, do give suggestions please. There is more context in the cover letter (the volume of which I didn't think appropriate for the commit message).
As Christian pointed out, the patches describe what they are doing well, but they do not explain why they are doing it.
His comment may be warranted on the other, new, patch for DMABUF name, but I don't think this applies here.
Just that VFIO can make it's internal dmabuf work again, there won't be a notification to any importer or an expectation that something like iommufd will re-establish mapping automatically.
This is principally a kernel self protection mechanism where the userspace was expected to have removed the dmabuf before issuing a reset/etc. If they didn't then the kernel plonks it and userspace gets a mess to clean up.
Revoke/invalidate means that the importer should stop accessing the buffer provided by the exporter. It does not specify what the exporter should do afterwards.
So I still think that "temporarily revoked" is closer to invalidate_mappings, with some additional dma-buf documentation to lose the expectation that the buffer will become available again.
You are right in indicating in your previous email that the "temporarily revoked" state is the old "revoked" state.
From the DMABUF importer interactions, there's _still_ only "revoked or not revoked" state. No changes there.
From this VFIO exporter's perspective, the change is to add a "sticky" revoked state.
The thing (as per UAPI docs, again I hope there's no guessing needed) that the new third state gives is that a) userspace can explicitly request it via the new ioctl/feature, and b) it is guaranteed not to be spuriously or intentionally undone by anyone else.
E.g. reset: temporarily revoke, then un-revoke. New ioctl: permanently revoke, such that something like a subsequent reset isn't going to undo it and make the DMABUF usable again.
The usage scenario is:
1. Process A exports DMABUF from VFIO device. (Proc A is the "orchestrator" here.) 2. Process A sends fd to process B. 3. Process B maps it, or hands it on, etc. 4. Process A eventually decides the buffer is finished with/lifecycle done/wants to reuse that BAR range for something else. It permanently revokes the DMABUF. 5. Hopefully this isn't a surprise to B (some cooperation has given it up first) but either way, B now cannot access the BAR range anymore. This does not _need_ cooperation, so if B becomes malicious, or crashes, etc. A can still revoke it. 6. That BAR range is reused for something unrelated.
HTH,
Matt
On Mon, Sep 14, 2026 at 01:13:36PM +0100, Matt Evans wrote:
Hi Leon,
On 14/09/2026 12:54, Leon Romanovsky wrote:
On Mon, Sep 14, 2026 at 08:36:47AM -0300, Jason Gunthorpe wrote:
On Sun, Sep 13, 2026 at 07:52:44PM +0300, Leon Romanovsky wrote:
On Fri, Sep 11, 2026 at 10:41:57PM +0100, Matt Evans wrote:
Expand the VFIO DMABUF revocation state to three states: Not revoked, temporarily revoked, and permanently revoked.
The thing is that "temporarily revoked" is actually the standard invalidate_mappings/move_notify mechanism of DMABUF, which wasn't good for VFIO.
I think temporarily revokes here means it is revoked from a dmabuf perspective
My guess is that this is more of a "change owner" operation than a revoke operation.
The main issue here is that we have to guess the semantics instead of having a properly named and documented operation.
Apologies if the cover letter for the series and patch commit message (which cover this) are unclear about the motivations and semantics. On the commit message, can you suggest clarifications:
"This is useful for lifecycle management, to reclaim VFIO PCI BAR ranges previously delegated to a subordinate client process: by revoking, the driver process can ensure that the loaned resources are made inaccessible when the client is deemed "done". The original DMABUF is defunct, and BAR resources can then be safely re-exported for use by new clients."
Given what I'll explain below, do give suggestions please. There is more context in the cover letter (the volume of which I didn't think appropriate for the commit message).
1. Do not mix "driver" and "client" in the same description. For a non-native English speaker, "driver" has a very specific meaning in the context of the Linux kernel.
2. Explain the lifecycle in the commit message, and why "revoke", which is effectively what the importer does, is not sufficient.
3. The more you put in the cover letter, the less likely people are to read it.
4. Commit messages should describe the patches themselves, since they are what remains visible in the git log, unlike the cover letter.
5. I would call what you describe as "temporarily revoke" is actually "reclaim".
Thanks
On 9/15/26 09:20, Leon Romanovsky wrote:
On Mon, Sep 14, 2026 at 01:13:36PM +0100, Matt Evans wrote:
Hi Leon,
On 14/09/2026 12:54, Leon Romanovsky wrote:
On Mon, Sep 14, 2026 at 08:36:47AM -0300, Jason Gunthorpe wrote:
On Sun, Sep 13, 2026 at 07:52:44PM +0300, Leon Romanovsky wrote:
On Fri, Sep 11, 2026 at 10:41:57PM +0100, Matt Evans wrote:
Expand the VFIO DMABUF revocation state to three states: Not revoked, temporarily revoked, and permanently revoked.
The thing is that "temporarily revoked" is actually the standard invalidate_mappings/move_notify mechanism of DMABUF, which wasn't good for VFIO.
I think temporarily revokes here means it is revoked from a dmabuf perspective
My guess is that this is more of a "change owner" operation than a revoke operation.
The main issue here is that we have to guess the semantics instead of having a properly named and documented operation.
Apologies if the cover letter for the series and patch commit message (which cover this) are unclear about the motivations and semantics. On the commit message, can you suggest clarifications:
"This is useful for lifecycle management, to reclaim VFIO PCI BAR ranges previously delegated to a subordinate client process: by revoking, the driver process can ensure that the loaned resources are made inaccessible when the client is deemed "done". The original DMABUF is defunct, and BAR resources can then be safely re-exported for use by new clients."
Given what I'll explain below, do give suggestions please. There is more context in the cover letter (the volume of which I didn't think appropriate for the commit message).
- Do not mix "driver" and "client" in the same description. For a
non-native English speaker, "driver" has a very specific meaning in the context of the Linux kernel.
In the context of DMA-buf it has also proven vital to clearly use the terms importer and exporter to describe the different roles a driver can have.
I can't count how often there was confusion because people (me included) just used "driver" and it wasn't clear which role was meant.
- Explain the lifecycle in the commit message, and why "revoke", which
is effectively what the importer does, is not sufficient.
- The more you put in the cover letter, the less likely people are to
read it.
- Commit messages should describe the patches themselves, since they
are what remains visible in the git log, unlike the cover letter.
- I would call what you describe as "temporarily revoke" is actually "reclaim".
+1
Regards, Christian.
Thanks
Hi Christian,
On 15/09/2026 12:13, Christian König wrote:
On 9/15/26 09:20, Leon Romanovsky wrote:
On Mon, Sep 14, 2026 at 01:13:36PM +0100, Matt Evans wrote:
Hi Leon,
On 14/09/2026 12:54, Leon Romanovsky wrote:
On Mon, Sep 14, 2026 at 08:36:47AM -0300, Jason Gunthorpe wrote:
On Sun, Sep 13, 2026 at 07:52:44PM +0300, Leon Romanovsky wrote:
On Fri, Sep 11, 2026 at 10:41:57PM +0100, Matt Evans wrote: > Expand the VFIO DMABUF revocation state to three states: > Not revoked, temporarily revoked, and permanently revoked.
The thing is that "temporarily revoked" is actually the standard invalidate_mappings/move_notify mechanism of DMABUF, which wasn't good for VFIO.
I think temporarily revokes here means it is revoked from a dmabuf perspective
My guess is that this is more of a "change owner" operation than a revoke operation.
The main issue here is that we have to guess the semantics instead of having a properly named and documented operation.
Apologies if the cover letter for the series and patch commit message (which cover this) are unclear about the motivations and semantics. On the commit message, can you suggest clarifications:
"This is useful for lifecycle management, to reclaim VFIO PCI BAR ranges previously delegated to a subordinate client process: by revoking, the driver process can ensure that the loaned resources are made inaccessible when the client is deemed "done". The original DMABUF is defunct, and BAR resources can then be safely re-exported for use by new clients."
Given what I'll explain below, do give suggestions please. There is more context in the cover letter (the volume of which I didn't think appropriate for the commit message).
- Do not mix "driver" and "client" in the same description. For a
non-native English speaker, "driver" has a very specific meaning in the context of the Linux kernel.
In the context of DMA-buf it has also proven vital to clearly use the terms importer and exporter to describe the different roles a driver can have.
I can't count how often there was confusion because people (me included) just used "driver" and it wasn't clear which role was meant.
:) I see where the quote can be clearer. FWIW the "driver process" was referring to a userspace driver (which is a legitimate use of the D-word with VFIO, but still). I'll clarify that, and will indicate this mechanism is used by userspace to influence the VFIO _exporter_ behaviour.
- Explain the lifecycle in the commit message, and why "revoke", which
is effectively what the importer does, is not sufficient.
- The more you put in the cover letter, the less likely people are to
read it.
- Commit messages should describe the patches themselves, since they
are what remains visible in the git log, unlike the cover letter.
- I would call what you describe as "temporarily revoke" is actually "reclaim".
+1
I don't follow here, sorry. Would you please elaborate?
Currently VFIO uses the priv->revoked flag to track whether it-the-exporter had previously done dma_buf_invalidate_mappings() on a buffer and is now causing all .attach requests to fail. Do you mean that (even without this series) you want to call that concept priv->reclaimed instead?
Or do you mean that you don't like the words "temporary"/"permanent" and are looking for another name for a temporarily unavailable buffer? (If so, I find "not revoked", "reclaimed", "revoked" much less clear than not/temp/perm revoked, as such names give no hint as to what to expect. But I may have misunderstood what you're getting at.)
From the importer side, there is no change from this patch: they might observe an invalidate_mappings() and find attach() of a given buffer now fails, same as before this patch. This is only about guaranteeing the impossibility of an importer ever being able to re-attach in future.
Thanks,
Matt
PS: A hypothetical alternative way of doing what this patch is doing is instead to have:
- existing priv->revoked - new priv->revoked_flag_is_immutable
Then, the ioctl triggers an invalidate_mappings() for a targeted DMABUF, sets revoked = true, and revoked_flag_is_immutable = true. The new flag prevents a future vfio_pci_dma_buf_move(false) from clearing revoked. For example, a reset doing `move(true); reset; move(false);` can make all DMABUFs available to attach again, except for those marked immutably-revoked. (That is equivalent to this new "permanent" state.)
On 9/15/26 16:22, Matt Evans wrote:
Hi Christian,
On 15/09/2026 12:13, Christian König wrote:
On 9/15/26 09:20, Leon Romanovsky wrote:
On Mon, Sep 14, 2026 at 01:13:36PM +0100, Matt Evans wrote:
Hi Leon,
On 14/09/2026 12:54, Leon Romanovsky wrote:
On Mon, Sep 14, 2026 at 08:36:47AM -0300, Jason Gunthorpe wrote:
On Sun, Sep 13, 2026 at 07:52:44PM +0300, Leon Romanovsky wrote: > On Fri, Sep 11, 2026 at 10:41:57PM +0100, Matt Evans wrote: >> Expand the VFIO DMABUF revocation state to three states: >> Not revoked, temporarily revoked, and permanently revoked. > > The thing is that "temporarily revoked" is actually the standard > invalidate_mappings/move_notify mechanism of DMABUF, which wasn't good > for VFIO.
I think temporarily revokes here means it is revoked from a dmabuf perspective
My guess is that this is more of a "change owner" operation than a revoke operation.
The main issue here is that we have to guess the semantics instead of having a properly named and documented operation.
Apologies if the cover letter for the series and patch commit message (which cover this) are unclear about the motivations and semantics. On the commit message, can you suggest clarifications:
"This is useful for lifecycle management, to reclaim VFIO PCI BAR ranges previously delegated to a subordinate client process: by revoking, the driver process can ensure that the loaned resources are made inaccessible when the client is deemed "done". The original DMABUF is defunct, and BAR resources can then be safely re-exported for use by new clients."
Given what I'll explain below, do give suggestions please. There is more context in the cover letter (the volume of which I didn't think appropriate for the commit message).
- Do not mix "driver" and "client" in the same description. For a
non-native English speaker, "driver" has a very specific meaning in the context of the Linux kernel.
In the context of DMA-buf it has also proven vital to clearly use the terms importer and exporter to describe the different roles a driver can have.
I can't count how often there was confusion because people (me included) just used "driver" and it wasn't clear which role was meant.
:) I see where the quote can be clearer. FWIW the "driver process" was referring to a userspace driver (which is a legitimate use of the D-word with VFIO, but still). I'll clarify that, and will indicate this mechanism is used by userspace to influence the VFIO _exporter_ behaviour.
- Explain the lifecycle in the commit message, and why "revoke", which
is effectively what the importer does, is not sufficient.
- The more you put in the cover letter, the less likely people are to
read it.
- Commit messages should describe the patches themselves, since they
are what remains visible in the git log, unlike the cover letter.
- I would call what you describe as "temporarily revoke" is actually "reclaim".
+1
I don't follow here, sorry. Would you please elaborate?
Currently VFIO uses the priv->revoked flag to track whether it-the-exporter had previously done dma_buf_invalidate_mappings() on a buffer and is now causing all .attach requests to fail. Do you mean that (even without this series) you want to call that concept priv->reclaimed instead?
That sounds like a permanent revoke.
Or do you mean that you don't like the words "temporary"/"permanent" and are looking for another name for a temporarily unavailable buffer? (If so, I find "not revoked", "reclaimed", "revoked" much less clear than not/temp/perm revoked, as such names give no hint as to what to expect. But I may have misunderstood what you're getting at.)
Yeah it's pretty much the naming I would clarify.
The original idea of notifying the importer that it need to re-create the mapping was resource reclaim.
But when you have some IOCTL or sysfs or whatever to disable a DMA-buf permanently I would call that revoke.
I'm still not 100% sure what a temporary revoke should be. If you have some IOCTL/sysfs/whatever to temporary say to importers "You can't use that resource" then that most likely won't fly.
From the importer side, there is no change from this patch: they might observe an invalidate_mappings() and find attach() of a given buffer now fails, same as before this patch. This is only about guaranteeing the impossibility of an importer ever being able to re-attach in future.
That sounds reasonable.
When a resource becomes unavailable you seriously need completely destroy it, re-create it and then import it again into other drivers which want to use it should it ever become available again.
Regards, Christian.
Thanks,
Matt
PS: A hypothetical alternative way of doing what this patch is doing is instead to have:
- existing priv->revoked
- new priv->revoked_flag_is_immutable
Then, the ioctl triggers an invalidate_mappings() for a targeted DMABUF, sets revoked = true, and revoked_flag_is_immutable = true. The new flag prevents a future vfio_pci_dma_buf_move(false) from clearing revoked. For example, a reset doing `move(true); reset; move(false);` can make all DMABUFs available to attach again, except for those marked immutably-revoked. (That is equivalent to this new "permanent" state.)
Hi both,
On 16/09/2026 15:19, Christian König wrote:
On 9/15/26 16:22, Matt Evans wrote:
Hi Christian,
On 15/09/2026 12:13, Christian König wrote:
On 9/15/26 09:20, Leon Romanovsky wrote:
On Mon, Sep 14, 2026 at 01:13:36PM +0100, Matt Evans wrote:
Hi Leon,
On 14/09/2026 12:54, Leon Romanovsky wrote:
On Mon, Sep 14, 2026 at 08:36:47AM -0300, Jason Gunthorpe wrote: > On Sun, Sep 13, 2026 at 07:52:44PM +0300, Leon Romanovsky wrote: >> On Fri, Sep 11, 2026 at 10:41:57PM +0100, Matt Evans wrote: >>> Expand the VFIO DMABUF revocation state to three states: >>> Not revoked, temporarily revoked, and permanently revoked. >> >> The thing is that "temporarily revoked" is actually the standard >> invalidate_mappings/move_notify mechanism of DMABUF, which wasn't good >> for VFIO. > > I think temporarily revokes here means it is revoked from a dmabuf > perspective
My guess is that this is more of a "change owner" operation than a revoke operation.
The main issue here is that we have to guess the semantics instead of having a properly named and documented operation.
Apologies if the cover letter for the series and patch commit message (which cover this) are unclear about the motivations and semantics. On the commit message, can you suggest clarifications:
"This is useful for lifecycle management, to reclaim VFIO PCI BAR ranges previously delegated to a subordinate client process: by revoking, the driver process can ensure that the loaned resources are made inaccessible when the client is deemed "done". The original DMABUF is defunct, and BAR resources can then be safely re-exported for use by new clients."
Given what I'll explain below, do give suggestions please. There is more context in the cover letter (the volume of which I didn't think appropriate for the commit message).
- Do not mix "driver" and "client" in the same description. For a
non-native English speaker, "driver" has a very specific meaning in the context of the Linux kernel.
In the context of DMA-buf it has also proven vital to clearly use the terms importer and exporter to describe the different roles a driver can have.
I can't count how often there was confusion because people (me included) just used "driver" and it wasn't clear which role was meant.
:) I see where the quote can be clearer. FWIW the "driver process" was referring to a userspace driver (which is a legitimate use of the D-word with VFIO, but still). I'll clarify that, and will indicate this mechanism is used by userspace to influence the VFIO _exporter_ behaviour.
- Explain the lifecycle in the commit message, and why "revoke", which
is effectively what the importer does, is not sufficient.
- The more you put in the cover letter, the less likely people are to
read it.
- Commit messages should describe the patches themselves, since they
are what remains visible in the git log, unlike the cover letter.
- I would call what you describe as "temporarily revoke" is actually "reclaim".
Leon, please see *** below.
+1
I don't follow here, sorry. Would you please elaborate?
Currently VFIO uses the priv->revoked flag to track whether it-the-exporter had previously done dma_buf_invalidate_mappings() on a buffer and is now causing all .attach requests to fail. Do you mean that (even without this series) you want to call that concept priv->reclaimed instead?
That sounds like a permanent revoke.
Not quite; the priv->revoked flag tracks temporary periods of inaccessibility. An example is VFIO resetting a function; the BAR mappings as seen by the CPU and DMABUFs made from the BARs are all made inaccessible before the reset, and made accessible again after the reset.
Or do you mean that you don't like the words "temporary"/"permanent" and are looking for another name for a temporarily unavailable buffer? (If so, I find "not revoked", "reclaimed", "revoked" much less clear than not/temp/perm revoked, as such names give no hint as to what to expect. But I may have misunderstood what you're getting at.)
Yeah it's pretty much the naming I would clarify.
The original idea of notifying the importer that it need to re-create the mapping was resource reclaim.
I feel the (existing) VFIO function reset sequenece is a counterexample, as the resource isn't being reclaimed/repurposed but users are "held off" for a period of time across the reset. (It's then the same resource that a later access can touch.)
But when you have some IOCTL or sysfs or whatever to disable a DMA-buf permanently I would call that revoke.
I see where you're coming from now, thanks.
If we were to just call VFIO's "permanently inacessible" state _revoked_ and "temporarily inaccessible" _reclaimed_, then it doesn't help clarity unfortunately. Those words are ~synonymous and don't convey permanent vs momentary.
***
Leon, would you please explain this comment #5? Were you suggesting that (even ignoring this patch!) you want to rename priv->revoked to priv->reclaimed? If so, why? (For context, this patch's state = TEMP_REVOKED is identical to the existing revoked = true.)
In short: TEMP_${X} vs PERM_${X} most clearly highlights the meaning/behaviour of the current + new state, regardless of whether X=REVOKED or X=RECLAIMED. Although I prefer to use the exisitng "revoked" I'm not wed to it, but mixing both "reclaimed" & "revoked" makes the distinction (and semantics, expectations of what can happen next) much less clear.
I'm still not 100% sure what a temporary revoke should be. If you have some IOCTL/sysfs/whatever to temporary say to importers "You can't use that resource" then that most likely won't fly.
This patch only provides an ioctl action to "make this inacessible forever". There is no direct userspace manipulation to/from the temporarily-inacessible state, nor need for that.
The only things that lead to a temporary revoke (as a side effect) are the preexisting VFIO reset ioctls etc.
State is an enum, but this isn't a complex FSM, just an extra "sticky" state. And to labour the point, the new temp/perm distinction is only about VFIO's internal tracking of why it revoked the buffer using the existing mechanisms (meaning, whether it permits un-revoke); it does not change the importer.
From the importer side, there is no change from this patch: they might observe an invalidate_mappings() and find attach() of a given buffer now fails, same as before this patch. This is only about guaranteeing the impossibility of an importer ever being able to re-attach in future.
That sounds reasonable.
When a resource becomes unavailable you seriously need completely destroy it, re-create it and then import it again into other drivers which want to use it should it ever become available again.
Yep, quite.
Thanks,
Matt
PS: I should've rebased (eyeballed master and didn't spot the tiny conflict), so no Sashiko review on v6; I want to resolve this for a v7 soon.
Regards, Christian.
Thanks,
Matt
PS: A hypothetical alternative way of doing what this patch is doing is instead to have:
- existing priv->revoked
- new priv->revoked_flag_is_immutable
Then, the ioctl triggers an invalidate_mappings() for a targeted DMABUF, sets revoked = true, and revoked_flag_is_immutable = true. The new flag prevents a future vfio_pci_dma_buf_move(false) from clearing revoked. For example, a reset doing `move(true); reset; move(false);` can make all DMABUFs available to attach again, except for those marked immutably-revoked. (That is equivalent to this new "permanent" state.)
On Mon, Sep 21, 2026 at 02:08:47PM +0100, Matt Evans wrote:
Not quite; the priv->revoked flag tracks temporary periods of inaccessibility. An example is VFIO resetting a function; the BAR mappings as seen by the CPU and DMABUFs made from the BARs are all made inaccessible before the reset, and made accessible again after the reset.
From the importer perspective this is a permanent revoke.
The right way to view this flow is VFIO permanently revokes the DMABUF FD. Then instead of forcing a new FD to be obtained it replaces the existing FD with a working one.
From an importer perspective it sees the revoke happen and then that's it, the dmabuf never does anything further. The importer has to unmap and start from scratch, get a FD and map it.
That VFIO re-used the FD to make a new live export isn't visible to the importer at all.
I think that is where this "temporary revoke" language gets confusing.
Call it "VFIO reuses the FD to create a new live mapping" is clearer than calling it "temporarily revoke" which sounds too much like move.
Jason
On 9/21/26 15:22, Jason Gunthorpe wrote:
On Mon, Sep 21, 2026 at 02:08:47PM +0100, Matt Evans wrote:
Not quite; the priv->revoked flag tracks temporary periods of inaccessibility. An example is VFIO resetting a function; the BAR mappings as seen by the CPU and DMABUFs made from the BARs are all made inaccessible before the reset, and made accessible again after the reset.
From the importer perspective this is a permanent revoke.
Yes, exactly that.
The right way to view this flow is VFIO permanently revokes the DMABUF FD. Then instead of forcing a new FD to be obtained it replaces the existing FD with a working one.
Well that is a little bit problematic. It could result in a race when you have multiple importers.
From an importer perspective it sees the revoke happen and then that's it, the dmabuf never does anything further. The importer has to unmap and start from scratch, get a FD and map it.
It could also be possible to wait for the reset to finish before giving out a new mapping.
That VFIO re-used the FD to make a new live export isn't visible to the importer at all.
I think that is where this "temporary revoke" language gets confusing.
Call it "VFIO reuses the FD to create a new live mapping" is clearer than calling it "temporarily revoke" which sounds too much like move.
I would avoid that and just re-create the DMA-buf fd from scratch. The extra overhead is negligible and one way state transmissions are usually much easier to handle.
Christian.
Jason
On Mon, Sep 21, 2026 at 03:45:28PM +0200, Christian König wrote:
The right way to view this flow is VFIO permanently revokes the DMABUF FD. Then instead of forcing a new FD to be obtained it replaces the existing FD with a working one.
Well that is a little bit problematic. It could result in a race when you have multiple importers.
This is all a "kernel protection" scheme for VFIO. VFIO userspace is expected to have unmapped things and is not expected to do mappings while also doing FLR.
The only kernel goal here is to maintain kernel integrity, not to have well defined semantics for illegal racy userspace behavior.
So from that perspective the races are OK.
That VFIO re-used the FD to make a new live export isn't visible to the importer at all.
I think that is where this "temporary revoke" language gets confusing.
Call it "VFIO reuses the FD to create a new live mapping" is clearer than calling it "temporarily revoke" which sounds too much like move.
I would avoid that and just re-create the DMA-buf fd from scratch. The extra overhead is negligible and one way state transmissions are usually much easier to handle.
Yeah, maybe we should have done that. Might be too late now.
Jason
On 9/21/26 15:49, Jason Gunthorpe wrote:
On Mon, Sep 21, 2026 at 03:45:28PM +0200, Christian König wrote:
The right way to view this flow is VFIO permanently revokes the DMABUF FD. Then instead of forcing a new FD to be obtained it replaces the existing FD with a working one.
Well that is a little bit problematic. It could result in a race when you have multiple importers.
This is all a "kernel protection" scheme for VFIO. VFIO userspace is expected to have unmapped things and is not expected to do mappings while also doing FLR.
The only kernel goal here is to maintain kernel integrity, not to have well defined semantics for illegal racy userspace behavior.
So from that perspective the races are OK.
Yeah as long as userspace can only shoot themselves in the foot it is probably ok.
In this case we just need to make sure that the flag is properly protected by some lock inside the kernel.
That VFIO re-used the FD to make a new live export isn't visible to the importer at all.
I think that is where this "temporary revoke" language gets confusing.
Call it "VFIO reuses the FD to create a new live mapping" is clearer than calling it "temporarily revoke" which sounds too much like move.
I would avoid that and just re-create the DMA-buf fd from scratch. The extra overhead is negligible and one way state transmissions are usually much easier to handle.
Yeah, maybe we should have done that. Might be too late now.
It's already uAPI?
Christian.
Jason
On Mon, Sep 21, 2026 at 04:09:18PM +0200, Christian König wrote:
I would avoid that and just re-create the DMA-buf fd from scratch. The extra overhead is negligible and one way state transmissions are usually much easier to handle.
Yeah, maybe we should have done that. Might be too late now.
It's already uAPI?
Yeah, but Matt is making some changes here so maybe new stuff can avoid this. I'm not sure.
Jason
On Tue, Sep 22, 2026 at 09:41:37AM -0300, Jason Gunthorpe wrote:
On Mon, Sep 21, 2026 at 04:09:18PM +0200, Christian König wrote:
I would avoid that and just re-create the DMA-buf fd from scratch. The extra overhead is negligible and one way state transmissions are usually much easier to handle.
Yeah, maybe we should have done that. Might be too late now.
It's already uAPI?
Yeah, but Matt is making some changes here so maybe new stuff can avoid this. I'm not sure.
Jason, the proposed semantics is not UAPI yet.
Matt only added the new struct in this patch. https://lore.kernel.org/all/20260911214200.33793-10-matt@ozlabs.org/#Z31incl...
Thanks
Jason
On Tue, Sep 22, 2026 at 03:46:37PM +0300, Leon Romanovsky wrote:
On Tue, Sep 22, 2026 at 09:41:37AM -0300, Jason Gunthorpe wrote:
On Mon, Sep 21, 2026 at 04:09:18PM +0200, Christian König wrote:
I would avoid that and just re-create the DMA-buf fd from scratch. The extra overhead is negligible and one way state transmissions are usually much easier to handle.
Yeah, maybe we should have done that. Might be too late now.
It's already uAPI?
Yeah, but Matt is making some changes here so maybe new stuff can avoid this. I'm not sure.
Jason, the proposed semantics is not UAPI yet.
What I'm talking about is, the revoke/unrevoke flow for a single FD was added from the start. For example vfio_pci_ioctl_reset() does it:
+ vfio_pci_dma_buf_move(vdev, true); ret = pci_try_reset_function(vdev->pdev); + if (__vfio_pci_memory_enabled(vdev)) + vfio_pci_dma_buf_move(vdev, false); up_write(&vdev->memory_lock);
The false restores the exsting dmabuf fds back to normal operation.
Matt's version is one that defeates the above false so once revoked it stays revoked forever.
Jason
On Mon, Sep 21, 2026 at 02:08:47PM +0100, Matt Evans wrote:
Hi both,
On 16/09/2026 15:19, Christian König wrote:
On 9/15/26 16:22, Matt Evans wrote:
Hi Christian,
On 15/09/2026 12:13, Christian König wrote:
On 9/15/26 09:20, Leon Romanovsky wrote:
On Mon, Sep 14, 2026 at 01:13:36PM +0100, Matt Evans wrote:
Hi Leon,
On 14/09/2026 12:54, Leon Romanovsky wrote: > On Mon, Sep 14, 2026 at 08:36:47AM -0300, Jason Gunthorpe wrote: >> On Sun, Sep 13, 2026 at 07:52:44PM +0300, Leon Romanovsky wrote: >>> On Fri, Sep 11, 2026 at 10:41:57PM +0100, Matt Evans wrote: >>>> Expand the VFIO DMABUF revocation state to three states: >>>> Not revoked, temporarily revoked, and permanently revoked. >>> >>> The thing is that "temporarily revoked" is actually the standard >>> invalidate_mappings/move_notify mechanism of DMABUF, which wasn't good >>> for VFIO. >> >> I think temporarily revokes here means it is revoked from a dmabuf >> perspective > > My guess is that this is more of a "change owner" operation than a > revoke operation. > > The main issue here is that we have to guess the semantics instead of > having a properly named and documented operation.
Apologies if the cover letter for the series and patch commit message (which cover this) are unclear about the motivations and semantics. On the commit message, can you suggest clarifications:
"This is useful for lifecycle management, to reclaim VFIO PCI BAR ranges previously delegated to a subordinate client process: by revoking, the driver process can ensure that the loaned resources are made inaccessible when the client is deemed "done". The original DMABUF is defunct, and BAR resources can then be safely re-exported for use by new clients."
Given what I'll explain below, do give suggestions please. There is more context in the cover letter (the volume of which I didn't think appropriate for the commit message).
- Do not mix "driver" and "client" in the same description. For a
non-native English speaker, "driver" has a very specific meaning in the context of the Linux kernel.
In the context of DMA-buf it has also proven vital to clearly use the terms importer and exporter to describe the different roles a driver can have.
I can't count how often there was confusion because people (me included) just used "driver" and it wasn't clear which role was meant.
:) I see where the quote can be clearer. FWIW the "driver process" was referring to a userspace driver (which is a legitimate use of the D-word with VFIO, but still). I'll clarify that, and will indicate this mechanism is used by userspace to influence the VFIO _exporter_ behaviour.
- Explain the lifecycle in the commit message, and why "revoke", which
is effectively what the importer does, is not sufficient.
- The more you put in the cover letter, the less likely people are to
read it.
- Commit messages should describe the patches themselves, since they
are what remains visible in the git log, unlike the cover letter.
- I would call what you describe as "temporarily revoke" is actually "reclaim".
Leon, please see *** below.
I would like to see clearer terminology here. Terms such as "temporarily revoke" and "permanently revoke" are misleading and require the reader to be very familiar with the underlying details.
The exact terminology matters less to me, as long as we avoid overloading the existing meaning of "revoke".
Thanks
On Mon, Sep 14, 2026 at 01:13:36PM +0100, Matt Evans wrote:
"This is useful for lifecycle management, to reclaim VFIO PCI BAR ranges previously delegated to a subordinate client process: by revoking, the driver process can ensure that the loaned resources are made inaccessible when the client is deemed "done". The original DMABUF is defunct, and BAR resources can then be safely re-exported for use by new clients."
This makes the point clear, you want a way to permanently revoke a specific dmabuf fd and render it forever unable to access any underlying memory.
You want to do this so the vendor can fence the vendee without requiring its co-operation.
"temporary revoke" ie what VFIO already does around reset is a different thing.
Jason
Hi Jason,
On 15/09/2026 13:35, Jason Gunthorpe wrote:
On Mon, Sep 14, 2026 at 01:13:36PM +0100, Matt Evans wrote:
"This is useful for lifecycle management, to reclaim VFIO PCI BAR ranges previously delegated to a subordinate client process: by revoking, the driver process can ensure that the loaned resources are made inaccessible when the client is deemed "done". The original DMABUF is defunct, and BAR resources can then be safely re-exported for use by new clients."
This makes the point clear, you want a way to permanently revoke a specific dmabuf fd and render it forever unable to access any underlying memory.
You want to do this so the vendor can fence the vendee without requiring its co-operation.
Exactly! (I'll reword this commit message still, re other requests in this thread, but you got the concept.)
"temporary revoke" ie what VFIO already does around reset is a different thing.
Yes, right. From the DMABUF importer (and now mmap) perspective the mechanism is unchanged. priv->revoked was always "temporary"/transient (OK, splitting hairs, but at VFIO shutdown an exported DMABUF becomes permanently inaccessible).
Here the only change is adding another cause of becoming inaccessible (direct request from userspace) and the length of time (it can never be re-attached, for the reasons you mentioned).
Cheers,
Matt
linaro-mm-sig@lists.linaro.org
-
Christian König -
Jason Gunthorpe -
Leon Romanovsky -
Matt Evans