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. This is achieved by allowing the processes to mmap() the DMABUFs; their access to the device is isolated to the exported ranges. The primary userspace driver process can forcibly revoke access to previously-shared buffers upon cleanup (without requiring cooperation from the subordinate processes). This is an improvement on sharing the VFIO device fd to subordinate processes, which would allow global access. See the RFCs for background.
The existing VFIO PCI BAR mmap() becomes backed by a DMABUF too, keeping common vm_ops and fault handler for VMAs from 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.
Below are per-patch notes & background info, and at the bottom are several related questions that reviewers may like to consider (worth at least skipping to #1, a possible bug).
Notes on patches ================
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.
vfio/pci: Remove DMABUF export dependency on vdev->memory_lock
In v5 of this series 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:
https://lore.kernel.org/kvm/9a615f22-c0d4-46ae-9654-db11e94e5fec@ozlabs.org/
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(R) 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() to remove 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: Provide 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.
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: Revoke a DMABUF on request from userspace
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 used 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 prior mmap_lock->memory_lock issue.
Dear Reviewers, ===============
Along the way several related issues came up that warrant more eyes, and I'd be grateful for your input:
1. 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/a known limitation, or a bug?
a) We could reject export w/ -EPERM unless the device fd's f_mode has O_RDWR, to reflect the RW abilities of P2P
If we agree it's a bug, I want to do this fix (a), as we can now export a DMABUF RW from an O_RDONLY device fd and then succeed to mmap() the DMABUF with RW. (That said, even with an O_RDONLY device fd, the device state can still be changed/reset. But it feels cleaner to prevent export for a O_RDONLY device fd, and match the device fd mmap() behaviour.)
In future, we could consider finer-grained RD/WR if there's a future goal to tie DMABUF permissions to, say, iommufd IOMMU_READ/IOMMU_WRITE permissions:
b) Instead of just failing if !O_RDWR, we could 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.)
2. The mmap fault handler takes a bunch of locks non-interruptibly, and potentially depends on a lot of DMABUF-related activities completing. I'd had a go at converting them to interruptible/killable forms, but that 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), though revocation will be rare in practice.
3. 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.
4. 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 policy of the DMABUF file (which is anon/unique to the mapping). This is pretty confusing.
END ===
This is based on v7.3-rc4
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.3-rc4...dev/mev/vfio-dmabuf-mmap...
Thanks for reading,
Matt
================================================================================
Changelog:
v7: - Rebased, v7.3-rc4
- "dma-buf: Export dma_buf_set_name()" is now "dma-buf: Provide dma_buf_set_name()": reworded commit message with rationale, and indicate it is only expected to be used by exporters. Remove static helper for user ioctl & copy from user inline in the ioctl.
- "vfio/pci: Permanently revoke a DMABUF on request" is now "vfio/pci: Revoke a DMABUF on request from userspace", with a clarified commit message, and removal of "temporary/permanent" language. This still replaces the priv->revoked flag with an enum/third state, named OK/REVOKED for the existing two. The new state, DEAD, is "sticky" from the VFIO-internal perspective, meaning it prevents any of the move-style transitions from REVOKED back to OK, guaranteeing that a DEAD buffer cannot ever be attached/mapped by a new or existing importer. Clarified language around attach vs map to not imply a racing import will be detached; it might attach, but cannot map (and has no existing maps) after the ioctl returns.
- Reworded several commit messages for more clarity/brevity.
- "vfio/pci: Add a helper to create a DMABUF for a BAR-map VMA": As of the recent e8efdf02d3a97 ("vfio: Enable cdev noiommu mode under iommufd") dev_name() can be much larger (e.g. noiommu_vfio1048575) and the discovery that drivers/pci/controller/vmd.c (others?) could create a domain up to MAX_INT means there isn't a nice way to guarantee a debug name is available across all cdev names and maximum sizes of all properties (e.g. 1M cdevs, 4G domains). So, removed the cdev name from the debug name construction, which now looks like 'vfio:0001:02:03.4/5'. The cdev can be fished out of sysfs given the domain+BDF, and it's still useful debug. Adjusted comments & commit message.
v6: https://lore.kernel.org/all/20260911214200.33793-1-matt@ozlabs.org/ 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: Provide 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: Revoke a DMABUF on request from userspace
drivers/dma-buf/dma-buf.c | 78 ++- 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 | 609 +++++++++++++++--- 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, 856 insertions(+), 187 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 therefore avoid a deadlock scenario (a fault handler attempting to take mmap_lock in a vfio-pci variant driver thread that holds memory_lock, vs another thread doing an mmap/export holding them both in the other order).
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 6757054e9d87..1f6066107831 100644 --- a/drivers/vfio/pci/vfio_pci_core.c +++ b/drivers/vfio/pci/vfio_pci_core.c @@ -663,6 +663,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;
@@ -2197,6 +2198,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 vfio_pci_dma_buf_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 1f6066107831..e170cdc7ac92 100644 --- a/drivers/vfio/pci/vfio_pci_core.c +++ b/drivers/vfio/pci/vfio_pci_core.c @@ -527,8 +527,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);
Allow exporters to set the name of buffers, during export or for subsequent updates. This is useful to express buffer-specific debug information, indicate state particular to the exporter's usage, etc. Only exporters and userspace are expected to set names; it isn't sensible for importers to do it.
Originally, the static dma_buf_set_name() was used for DMA_BUF_SET_NAME ioctls, taking a __user string parameter. Export this function as a generic set-name helper, taking a kernel-allocated string. The SET_NAME ioctls now locally duplicate the __user string, and use the new function to set the name.
Signed-off-by: Matt Evans matt@ozlabs.org --- drivers/dma-buf/dma-buf.c | 78 ++++++++++++++++++++++++--------------- include/linux/dma-buf.h | 2 + 2 files changed, 50 insertions(+), 30 deletions(-)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 4c9add51f9ef..e4ec504e9288 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -404,34 +404,6 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll) return events; }
-/** - * 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. - * - * @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. - * - */ -static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf) -{ - char *name = strndup_user(buf, DMA_BUF_NAME_LEN); - - if (IS_ERR(name)) - return PTR_ERR(name); - - spin_lock(&dmabuf->name_lock); - kfree(dmabuf->name); - dmabuf->name = name; - spin_unlock(&dmabuf->name_lock); - - return 0; -} - #if IS_ENABLED(CONFIG_SYNC_FILE) static long dma_buf_export_sync_file(struct dma_buf *dmabuf, void __user *user_data) @@ -577,8 +549,19 @@ static long dma_buf_ioctl(struct file *file, return ret;
case DMA_BUF_SET_NAME_A: - case DMA_BUF_SET_NAME_B: - return dma_buf_set_name(dmabuf, (const char __user *)arg); + case DMA_BUF_SET_NAME_B: { + char *name = strndup_user((const char __user *)arg, + DMA_BUF_NAME_LEN); + + if (IS_ERR(name)) + return PTR_ERR(name); + + ret = dma_buf_set_name(dmabuf, name); + if (ret) + kfree(name); + + return ret; + }
#if IS_ENABLED(CONFIG_SYNC_FILE) case DMA_BUF_IOCTL_EXPORT_SYNC_FILE: @@ -874,6 +857,41 @@ void dma_buf_put(struct dma_buf *dmabuf) } EXPORT_SYMBOL_NS_GPL(dma_buf_put, "DMA_BUF");
+/** + * dma_buf_set_name - Set a dmabuf's name + * Intended to be used by the exporter to set a name for debug + * purposes. This can also change an existing name if the same piece + * of memory is used for multiple purposes over time. + * + * @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 (including terminator) 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 d15b2b31d3c9..952a2c196ad4 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -571,6 +571,8 @@ void dma_buf_fd_install(struct dma_buf *dmabuf, int fd); 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/24/26 17:21, Matt Evans wrote:
Allow exporters to set the name of buffers, during export or for subsequent updates. This is useful to express buffer-specific debug information, indicate state particular to the exporter's usage, etc. Only exporters and userspace are expected to set names; it isn't sensible for importers to do it.
Originally, the static dma_buf_set_name() was used for DMA_BUF_SET_NAME ioctls, taking a __user string parameter. Export this function as a generic set-name helper, taking a kernel-allocated string. The SET_NAME ioctls now locally duplicate the __user string, and use the new function to set the name.
Signed-off-by: Matt Evans matt@ozlabs.org
drivers/dma-buf/dma-buf.c | 78 ++++++++++++++++++++++++--------------- include/linux/dma-buf.h | 2 + 2 files changed, 50 insertions(+), 30 deletions(-)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 4c9add51f9ef..e4ec504e9288 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -404,34 +404,6 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll) return events; } -/**
- 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.
- @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.
- */
-static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf) -{
- char *name = strndup_user(buf, DMA_BUF_NAME_LEN);
- if (IS_ERR(name))
return PTR_ERR(name);- spin_lock(&dmabuf->name_lock);
- kfree(dmabuf->name);
- dmabuf->name = name;
- spin_unlock(&dmabuf->name_lock);
- return 0;
-}
#if IS_ENABLED(CONFIG_SYNC_FILE) static long dma_buf_export_sync_file(struct dma_buf *dmabuf, void __user *user_data) @@ -577,8 +549,19 @@ static long dma_buf_ioctl(struct file *file, return ret; case DMA_BUF_SET_NAME_A:
- case DMA_BUF_SET_NAME_B:
return dma_buf_set_name(dmabuf, (const char __user *)arg);
- case DMA_BUF_SET_NAME_B: {
char *name = strndup_user((const char __user *)arg,DMA_BUF_NAME_LEN);if (IS_ERR(name))return PTR_ERR(name);ret = dma_buf_set_name(dmabuf, name);if (ret)kfree(name);return ret;- }
#if IS_ENABLED(CONFIG_SYNC_FILE) case DMA_BUF_IOCTL_EXPORT_SYNC_FILE: @@ -874,6 +857,41 @@ void dma_buf_put(struct dma_buf *dmabuf) } EXPORT_SYMBOL_NS_GPL(dma_buf_put, "DMA_BUF"); +/**
- dma_buf_set_name - Set a dmabuf's name
- Intended to be used by the exporter to set a name for debug
- purposes. This can also change an existing name if the same piece
- of memory is used for multiple purposes over time.
Add something like "Importers should *NOT* use this function to overwrite the name." here and add some note under "DOC: locking convention" how this function is supposed to be called.
Apart from that looks good to me, Christian.
- @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 (including terminator) 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 d15b2b31d3c9..952a2c196ad4 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -571,6 +571,8 @@ void dma_buf_fd_install(struct dma_buf *dmabuf, int fd); 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 25/09/2026 09:39, Christian König wrote:
On 9/24/26 17:21, Matt Evans wrote:
Allow exporters to set the name of buffers, during export or for subsequent updates. This is useful to express buffer-specific debug information, indicate state particular to the exporter's usage, etc. Only exporters and userspace are expected to set names; it isn't sensible for importers to do it.
Originally, the static dma_buf_set_name() was used for DMA_BUF_SET_NAME ioctls, taking a __user string parameter. Export this function as a generic set-name helper, taking a kernel-allocated string. The SET_NAME ioctls now locally duplicate the __user string, and use the new function to set the name.
Signed-off-by: Matt Evans matt@ozlabs.org
drivers/dma-buf/dma-buf.c | 78 ++++++++++++++++++++++++--------------- include/linux/dma-buf.h | 2 + 2 files changed, 50 insertions(+), 30 deletions(-)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 4c9add51f9ef..e4ec504e9288 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -404,34 +404,6 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll) return events; } -/**
- 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.
- @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.
- */
-static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf) -{
- char *name = strndup_user(buf, DMA_BUF_NAME_LEN);
- if (IS_ERR(name))
return PTR_ERR(name);- spin_lock(&dmabuf->name_lock);
- kfree(dmabuf->name);
- dmabuf->name = name;
- spin_unlock(&dmabuf->name_lock);
- return 0;
-}
- #if IS_ENABLED(CONFIG_SYNC_FILE) static long dma_buf_export_sync_file(struct dma_buf *dmabuf, void __user *user_data)
@@ -577,8 +549,19 @@ static long dma_buf_ioctl(struct file *file, return ret; case DMA_BUF_SET_NAME_A:
- case DMA_BUF_SET_NAME_B:
return dma_buf_set_name(dmabuf, (const char __user *)arg);
- case DMA_BUF_SET_NAME_B: {
char *name = strndup_user((const char __user *)arg,DMA_BUF_NAME_LEN);if (IS_ERR(name))return PTR_ERR(name);ret = dma_buf_set_name(dmabuf, name);if (ret)kfree(name);return ret;- }
#if IS_ENABLED(CONFIG_SYNC_FILE) case DMA_BUF_IOCTL_EXPORT_SYNC_FILE: @@ -874,6 +857,41 @@ void dma_buf_put(struct dma_buf *dmabuf) } EXPORT_SYMBOL_NS_GPL(dma_buf_put, "DMA_BUF"); +/**
- dma_buf_set_name - Set a dmabuf's name
- Intended to be used by the exporter to set a name for debug
- purposes. This can also change an existing name if the same piece
- of memory is used for multiple purposes over time.
Add something like "Importers should *NOT* use this function to overwrite the name." here and add some note under "DOC: locking convention" how this function is supposed to be called.
Apart from that looks good to me,
Thanks, Christian. Good point, done.
Re the locking convention, it appears dma_buf_set_name() is agnostic to whether resv is held or not. However, rather than adding that as a new category to the 'DOC: locking convention', I've put this function in the existing "don't hold resv" section.
Whilst conservative, this matches existing (and this series's) usage, and can then be relaxed in future. If you think a new "resv is permitted but not required to be held" category of API doc is better, do say.
Thanks,
Matt
Christian.
- @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 (including terminator) 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 d15b2b31d3c9..952a2c196ad4 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -571,6 +571,8 @@ void dma_buf_fd_install(struct dma_buf *dmabuf, int fd); 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 4e7162234a2e..786df3a81d9d 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);
@@ -123,6 +137,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 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 | 209 ++++++++++++++++++++++------- drivers/vfio/pci/vfio_pci_priv.h | 5 + 2 files changed, 169 insertions(+), 45 deletions(-)
diff --git a/drivers/vfio/pci/vfio_pci_dmabuf.c b/drivers/vfio/pci/vfio_pci_dmabuf.c index 9f10b10fc436..fd3ee2f5c5e8 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,165 @@ 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; + } + + /* + * Debug name: The absolute maximum size of the name + * ('vfio:ffffffff:ff:1f.7/5') fits within DMA_BUF_NAME_LEN. + */ + bufname = kasprintf(GFP_KERNEL, "vfio:%s/%x", + 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)) { + 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 +526,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 +585,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 +598,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 786df3a81d9d..66a5ae19082d 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; @@ -143,6 +144,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 e170cdc7ac92..cb0e4865d110 100644 --- a/drivers/vfio/pci/vfio_pci_core.c +++ b/drivers/vfio/pci/vfio_pci_core.c @@ -1748,18 +1748,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, @@ -1787,23 +1775,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 = linear_page_delta(vma, addr); - 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; } @@ -1828,6 +1839,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);
@@ -1867,7 +1879,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 fd3ee2f5c5e8..617afb7de00f 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) { @@ -364,8 +377,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; } @@ -408,6 +426,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 @@ -606,6 +625,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) { @@ -632,6 +652,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 66a5ae19082d..ef6aabb9f5a4 100644 --- a/drivers/vfio/pci/vfio_pci_priv.h +++ b/drivers/vfio/pci/vfio_pci_priv.h @@ -148,13 +148,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, @@ -163,13 +163,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 9914f3ac69ae..cef337f4e8f2 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 cb0e4865d110..986852b1d44f 100644 --- a/drivers/vfio/pci/vfio_pci_core.c +++ b/drivers/vfio/pci/vfio_pci_core.c @@ -376,8 +376,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); @@ -463,7 +462,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); }
@@ -532,7 +531,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; @@ -1713,20 +1719,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) @@ -2224,6 +2247,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); @@ -2591,9 +2624,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)) { @@ -2601,8 +2635,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, @@ -2632,7 +2665,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 ef6aabb9f5a4..f11b31f8953c 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 drivers to safely delegate BAR sub-ranges to other 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 986852b1d44f..d38210d1c5c4 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> @@ -1804,21 +1806,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, @@ -1839,6 +1899,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; }
@@ -1854,6 +1915,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 617afb7de00f..c0a48d17c829 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 causing a 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 f11b31f8953c..32d520cac52f 100644 --- a/drivers/vfio/pci/vfio_pci_priv.h +++ b/drivers/vfio/pci/vfio_pci_priv.h @@ -151,6 +151,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,
Replace the VFIO exporter's priv->revoked flag with a status enum of OK, REVOKED, and DEAD. OK and REVOKED are equivalent to the existing flag, which tracks whether invalidate_mappings & unmap has occurred and whether attach/map are permitted.
The DMABUF is marked DEAD in response to a new VFIO feature VFIO_DEVICE_FEATURE_DMA_BUF_REVOKE. This passes a DMABUF by fd and requests that the DMABUF is immediately revoked. On success, it's guaranteed that PTEs are zapped, imports have been cleanly unmapped, and that the buffer can never be mapped/attached or mmap()ed anymore. Existing VFIO move semantics could permit a buffer to be mapped again in future (e.g. revoke/unrevoke around a reset), but the DEAD state can never become OK again.
This enables a userspace driver process to securely reclaim VFIO PCI BAR resources previously delegated to a subordinate process. Using the new ioctl, the userspace driver process can ensure that resources are inaccessible when the subordinate is deemed "done". The originally-exported DMABUF becomes unusable, and BAR resources can then be safely re-exported for use elsewhere.
Refactor the revocation code out of vfio_pci_dma_buf_move() to a function common to move and the new feature request path. This function calls dma_buf_invalidate_mappings() and dma_resv_wait_timeout() only on the revoke path (vfio_pci_dma_buf_move() originally called them for both revoke and, unnecessarily, for 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 | 181 ++++++++++++++++++++++------- drivers/vfio/pci/vfio_pci_priv.h | 19 ++- include/uapi/linux/vfio.h | 24 ++++ 4 files changed, 188 insertions(+), 42 deletions(-)
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c index d38210d1c5c4..8860185cff49 100644 --- a/drivers/vfio/pci/vfio_pci_core.c +++ b/drivers/vfio/pci/vfio_pci_core.c @@ -1642,6 +1642,8 @@ int vfio_pci_core_ioctl_feature(struct vfio_device *device, u32 flags, return vfio_pci_core_feature_dma_buf(vdev, flags, arg, argsz); case VFIO_DEVICE_FEATURE_ZPCI_ERROR: return vfio_pci_zdev_feature_err(device, 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; } @@ -1852,7 +1854,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); @@ -1880,7 +1882,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 c0a48d17c829..b57bfaefd9fa 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 * * 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_REVOKED : + VFIO_PCI_DMABUF_OK; list_add_tail(&priv->dmabufs_elm, &vdev->dmabufs); dma_resv_unlock(priv->dmabuf->resv); up_write(&vdev->dmabuf_lock); @@ -496,7 +497,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 */ @@ -674,6 +675,68 @@ 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, REVOKED, DEAD): DEAD gives + * the guarantee that all future map/attach attempts will fail no + * matter what, whereas REVOKED can transition back to OK. + */ +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 DEAD, state can no longer change */ + if (priv->status == VFIO_PCI_DMABUF_DEAD || + priv->status == new_status) + return; + + dma_resv_lock(priv->dmabuf->resv, NULL); + was_revoked = (priv->status == VFIO_PCI_DMABUF_REVOKED); + + if (new_status != VFIO_PCI_DMABUF_OK) { + priv->status = new_status; + + if (was_revoked) { + /* + * A REVOKED buffer is being marked DEAD. + * invalidate_mappings/unmap wait happened + * when it became REVOKED, don't wait 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; @@ -686,38 +749,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_REVOKED : + VFIO_PCI_DMABUF_OK); fput(priv->dmabuf->file); } up_write(&vdev->dmabuf_lock); @@ -745,10 +779,79 @@ 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_DEAD) { + ret = -EBADFD; + } else { + vfio_pci_dma_buf_set_status(priv, VFIO_PCI_DMABUF_DEAD); + 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 32d520cac52f..ca12221af555 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_REVOKED = 1, + VFIO_PCI_DMABUF_DEAD = 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); @@ -157,6 +163,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, @@ -165,6 +175,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 e41437fa17ad..d3c6057983e0 100644 --- a/include/uapi/linux/vfio.h +++ b/include/uapi/linux/vfio.h @@ -1555,6 +1555,30 @@ struct vfio_device_feature_zpci_err {
#define VFIO_DEVICE_FEATURE_ZPCI_ERROR 13
+/** + * Given a DMABUF 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 revoked. On + * successful return, the buffer is no longer accessible through any + * VMA or DMABUF import. Thereafter, VFIO also refuses all future + * mmap()s and map/attach requests from any new/existing importer. + * + * 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 revoked by this feature. + * + * Additionally, common errors can occur: EFAULT accessing the struct, + * or EINVAL requesting an unsupported feature op. + */ +#define VFIO_DEVICE_FEATURE_DMA_BUF_REVOKE 14 + +struct vfio_device_feature_dma_buf_revoke { + __s32 dmabuf_fd; +}; + /* -------- API for Type1 VFIO IOMMU -------- */
/**
On Thu, 24 Sep 2026 16:21:43 +0100 Matt Evans matt@ozlabs.org wrote:
Dear Reviewers,
Along the way several related issues came up that warrant more eyes, and I'd be grateful for your input:
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/a known limitation, or a bug?
Seems like a bug. In practice it's probably not very meaningful, the user can still potentially change the device power state and trigger a reset, but being able to source a writable dmabuf to a region on the device fd that isn't itself writable seems semantically wrong.
a) We could reject export w/ -EPERM unless the device fd's f_mode has O_RDWR, to reflect the RW abilities of P2P
This seems sufficient...
If we agree it's a bug, I want to do this fix (a), as we can now export a DMABUF RW from an O_RDONLY device fd and then succeed to mmap() the DMABUF with RW. (That said, even with an O_RDONLY device fd, the device state can still be changed/reset. But it feels cleaner to prevent export for a O_RDONLY device fd, and match the device fd mmap() behaviour.)
In future, we could consider finer-grained RD/WR if there's a future goal to tie DMABUF permissions to, say, iommufd IOMMU_READ/IOMMU_WRITE permissions:
b) Instead of just failing if !O_RDWR, we could 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.)
Certainly more complete, but I'm not sure there's a use case here that really warrants the effort.
Another angle to the dmabuf permissions is the region permissions themselves. We don't currently hit this since we're only exporting PCI BARs, but for instance Manish wants to protect the HDM decoder range in the vfio-cxl series[1]. Again, there's probably a simple solution to simply consult the excluded ranges list, added in that series, and reject dmabuf exports overlapping it. I don't expect any action item for this series though.
[1]https://lore.kernel.org/all/20260916183540.3813685-1-mhonap@nvidia.com/
The mmap fault handler takes a bunch of locks non-interruptibly, and potentially depends on a lot of DMABUF-related activities completing. I'd had a go at converting them to interruptible/killable forms, but that 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), though revocation will be rare in practice.
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.
Our vfio-pci story around surprise removal and DPC is pretty weak, I'm hoping that some of our parallel error handling work will shut down the device when this occurs. For now, leaving the dmabuf revoked seems like a reasonable thing to do. In practice, I don't really see this coming up other than in testing surprise removal of NVMe drives.
- 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 policy of the DMABUF file (which is anon/unique to the mapping). This is pretty confusing.
Maybe I'm not seeing the issue, but this seems like correct behavior to me. Policy decisions are made at creation time and later changing protections on one object doesn't affect the other. Thanks,
Alex
Hi Alex,
On 1 Oct 2026, at 22:38, Alex Williamson alex@shazbot.org wrote:
On Thu, 24 Sep 2026 16:21:43 +0100 Matt Evans matt@ozlabs.org wrote:
Dear Reviewers,
Along the way several related issues came up that warrant more eyes, and I'd be grateful for your input:
- 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/a known limitation, or a bug?
Seems like a bug. In practice it's probably not very meaningful, the user can still potentially change the device power state and trigger a reset, but being able to source a writable dmabuf to a region on the device fd that isn't itself writable seems semantically wrong.
a) We could reject export w/ -EPERM unless the device fd's f_mode has O_RDWR, to reflect the RW abilities of P2P
This seems sufficient...
Thanks, I’ll take a look then. I should’ve said this is cdev-centric (an explicit open O_RDONLY), as VFIO_GROUP_GET_DEVICE_FD always generates the fd with RDWR perms. The path from container/group open through to device fd generation and then export is similarly semantically weird, but maybe less clear on effort vs benefit to fix.
So, for cdev open it looks like some plumbing, and that it won’t conflict with this series, so I can look at that separately.
If we agree it's a bug, I want to do this fix (a), as we can now export a DMABUF RW from an O_RDONLY device fd and then succeed to mmap() the DMABUF with RW. (That said, even with an O_RDONLY device fd, the device state can still be changed/reset. But it feels cleaner to prevent export for a O_RDONLY device fd, and match the device fd mmap() behaviour.)
In future, we could consider finer-grained RD/WR if there's a future goal to tie DMABUF permissions to, say, iommufd IOMMU_READ/IOMMU_WRITE permissions:
b) Instead of just failing if !O_RDWR, we could 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.)Certainly more complete, but I'm not sure there's a use case here that really warrants the effort.
Another angle to the dmabuf permissions is the region permissions themselves. We don't currently hit this since we're only exporting PCI BARs, but for instance Manish wants to protect the HDM decoder range in the vfio-cxl series[1]. Again, there's probably a simple solution to simply consult the excluded ranges list, added in that series, and reject dmabuf exports overlapping it. I don't expect any action item for this series though.
Interesting, thanks. Reading.
[1]https://lore.kernel.org/all/20260916183540.3813685-1-mhonap@nvidia.com/
- The mmap fault handler takes a bunch of locks non-interruptibly,
and potentially depends on a lot of DMABUF-related activities completing. I'd had a go at converting them to interruptible/killable forms, but that 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), though revocation will be rare in practice.
- 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.
Our vfio-pci story around surprise removal and DPC is pretty weak, I'm hoping that some of our parallel error handling work will shut down the device when this occurs. For now, leaving the dmabuf revoked seems like a reasonable thing to do. In practice, I don't really see this coming up other than in testing surprise removal of NVMe drives.
OK, cheers.
(Maybe a couple of these bullets are ~rhetorical, just to get a “known wrinkle” searchable somewhere.)
- 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 policy of the DMABUF file (which is anon/unique to the mapping). This is pretty confusing.
Maybe I'm not seeing the issue, but this seems like correct behavior to me. Policy decisions are made at creation time and later changing protections on one object doesn't affect the other. Thanks,
I think this is a change in behaviour re LSM rule, should anyone have applied any to VFIO in any fine-grained way, so just flagging that.
I’ll send v8 shortly, resolving Christian’s request. As ever, thanks for the input!
Matt
On Thu, Oct 01, 2026 at 03:38:34PM -0600, Alex Williamson wrote:
On Thu, 24 Sep 2026 16:21:43 +0100 Matt Evans matt@ozlabs.org wrote:
Dear Reviewers,
Along the way several related issues came up that warrant more eyes, and I'd be grateful for your input:
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/a known limitation, or a bug?
Seems like a bug. In practice it's probably not very meaningful, the user can still potentially change the device power state and trigger a reset, but being able to source a writable dmabuf to a region on the device fd that isn't itself writable seems semantically wrong.
I'm shocked O_RDONLY even did *anything*, I never expected this.
IMHO with these kinds of cdev's we shouldn't do anything in response to O_RDONLY. If the core code does something then fine, but I don't think we should try to define a semantic what "Read only vfio" even should mean.
a) We could reject export w/ -EPERM unless the device fd's f_mode has O_RDWR, to reflect the RW abilities of P2P
This seems sufficient...
+1
Jason
linaro-mm-sig@lists.linaro.org
-
Alex Williamson -
Christian König -
Jason Gunthorpe -
Matt Evans