Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

18 participants
7031 discussions

[PATCH v5 0/9] vfio/pci: Add mmap() for DMABUFs

by Matt Evans

Hi all, The goal of this series is to enable userspace driver designs that use VFIO to export DMABUFs representing subsets of PCI device BARs, and "vend" those buffers from a primary process to other subordinate processes by fd. These processes then mmap() the buffers and their access to the device is isolated to the exported ranges. This is an improvement on sharing the VFIO device fd to subordinate processes, which would allow unfettered access. This is achieved by enabling mmap() of vfio-pci DMABUFs, passed by fd to subordinate processes. Second, a new revocation mechanism is added to allow the primary process to forcibly revoke access to previously-shared BAR spans, even if the subordinate processes haven't cleanly exited. (The related topic of safe delegation of iommufd control to the subordinate processes is not addressed here, and is follow-up work.) The background/rationale is covered in more detail in the RFC cover letters. Feedback from the RFCs requested that, instead of creating DMABUF-specific vm_ops and .fault paths, to go the whole way and migrate the existing VFIO PCI BAR mmap() to be backed by a DMABUF too, resulting in a common vm_ops and fault handler for mmap()s of both the VFIO device and explicitly-exported DMABUFs. This will help future iommufd emulation of VFIO Type1 peer-to-peer, making it easier to get a DMABUF for a VFIO BAR as a DMA target. mmap() conversion to use DMABUF underneath has been done for vfio-pci, but not sub-drivers: nvgrace-gpu's mmap() override path is unchanged; I kept this out of scope for now not least because I don't have a thorough test setup for this system. I would prefer to help the nvgrace-gpu maintainers enable BAR mmap() DMABUFs themselves. Notes on patches ================ PCI/P2PDMA: Split pool-related cleanup out of pci_p2pdma_release() PCI/P2PDMA: Add CONFIG_PCI_P2PDMA_CORE Later in the series, vfio-pci's mmap() is going to depend on pcim_p2pdma_provider() which depended on CONFIG_PCI_P2PDMA, which in turn depended on ZONE_DEVICE. That isn't available on 32-bit and some archs, because they lack MEMORY_HOTPLUG and friends. VFIO does _not_ require actual P2P to be present for basic mmap() functionality, only for the optional CONFIG_DMA_SHARED_BUFFER feature. These split out p2pdma_core.c under CONFIG_PCI_P2PDMA_CORE (which currently contains pcim_p2pdma_provider()), and an optional CONFIG_PCI_P2PDMA which depends on ZONE_DEVICE etc. providing P2P functionality in the existing p2pdma.c. The first splits out pool cleanup from the release path, and the second does the refactor/code move to the new file. 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 The first adds a DMABUF VMA fault handler helper to determine arbitrary-sized PFNs from ranges in DMABUF. The second 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. 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. vfio/pci: Provide a user-facing name for BAR mappings 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 mapped. 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 vdev->memory_lock is touched; this holds the device registration so that even if the buffer has been cleaned up, vdev hasn't been freed and so the lock can be safely taken. vfio/pci: Permanently revoke a DMABUF on request This is mostly a rename of `revoked` to an enum, `status`, and adding a third state for a buffer: usable, revoked temporary, revoked permanent. A new VFIO feature is added, VFIO_DEVICE_FEATURE_DMA_BUF_REVOKE, which takes a DMABUF (exported from the same device) and permanently revokes it. Thus a userspace driver can guarantee any downstream consumers of a shared fd are prevented from accessing a BAR range, and that range can be reused. NOTE: This might block userspace, waiting on importers to detach. The code doing revocation in vfio_pci_dma_buf_move() is moved, to a common function for use by ..._move() and this new feature. NOTE: See changelog, by request v4 added a condition to the existing code to elide the unnecessary invalidation/sync on the un-revoke path.) NOTE: Previous versions contained an additional feature patch, "vfio/pci: Add mmap() attributes to DMABUF feature". This has been dropped in v5 because: - The mechanism simply set vma->vm_page_prot. This would be sufficient for arm64 and other architectures. - However, (locally-run claude-opus-4-8) Sashiko flagged that, on x86, additional memtype handling is required to set up the PAT. Without this, the memtype is returned back to UC- by pfnmap_setup_cachemode() upon PTE creation. Most other sources of userspace WC mappings create PTEs eagerly with e.g. io_remap_pfn_range() which memtype_reserve() WC for the range. Getting them with lazy-fault used by vfio-pci is more complicated (e.g. perhaps registering WC for BARs with PAT/MTRRs, and deciding how to deal with aliasing...). Since this feature is not critical for this series to be useful, I've decided for now to drop it in favour of a simpler series now and revisiting this separ*ately. Testing ======= (The [RFC ONLY] userspace test program, for QEMU edu-plus, can be found in the GitHub branch below. It at least illustrates how the export, map, revoke, and close semantics interoperate.) 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. Dear Reviewers, =============== I was grateful for the reviews and Reviewed-Bys on previous versions. Thanks; I've added some Reviewed-Bys/Acks. I have NOT included your tags where the patch has materially changed after your review (or where requested changes ended up more than super-trivial). I hope that's okay. End === This is based on v7.2-rc3. These commits are on GitHub for easier browsing, along with "[RFC ONLY] selftests: vfio: Add standalone vfio_dmabuf_mmap_test": https://github.com/metamev/linux/compare/v7.2-rc3...dev/mev/vfio-dmabuf-mma… Thanks for reading, Matt ================================================================================ Changelog: v5: - Rebased on 7.2-rc3 - Dropped the memattr/WC feature (see explanation above). - "vfio/pci: Convert BAR mmap() to use a DMABUF": Fixed a potentially-nasty bug (which (locally-run) Sashiko found!) whereby the unmap_mapping_range() performed in cleanup was passed a range up from offset zero for the DMABUF size. Initially this was how all DMABUFs were created and an appropriate zap, but a new version kept the VFIO region index encoded in the offset -- for BAR > 0 the unmap span would then mismatch. Instead, pass size 0 to mean an "all" range. Because the goal is to shoot down everything relating to one DMABUF and the address_space can only contain things relating to that DMABUF, this is equivalent and has the bonus of never failing to match mappings... Praan, Kevin, I kept your R-Bs on this fix. - The revoke patch converts vfio_pci_dma_buf_cleanup()'s priv->vdev = NULL to a WRITE_ONCE, corresponding to the revoke function's READ_ONCE (performed to test that the VFIO and DMABUF are related). - Clarified the VFIO_DEVICE_FEATURE_DMA_BUF_REVOKE UAPI comments, documenting previously-missing error cases and their reasons. 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): PCI/P2PDMA: Split pool-related cleanup out of pci_p2pdma_release() PCI/P2PDMA: Add CONFIG_PCI_P2PDMA_CORE 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: Provide a user-facing name for BAR mappings vfio/pci: Clean up BAR zap and revocation vfio/pci: Support mmap() of a VFIO DMABUF vfio/pci: Permanently revoke a DMABUF on request MAINTAINERS | 2 +- drivers/pci/Kconfig | 5 + drivers/pci/Makefile | 1 + drivers/pci/p2pdma.c | 113 +--- drivers/pci/p2pdma.h | 29 + drivers/pci/p2pdma_core.c | 122 +++++ drivers/vfio/pci/Kconfig | 5 +- drivers/vfio/pci/Makefile | 3 +- .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 8 + drivers/vfio/pci/vfio_pci_config.c | 30 +- drivers/vfio/pci/vfio_pci_core.c | 210 +++++-- drivers/vfio/pci/vfio_pci_dmabuf.c | 515 +++++++++++++++--- drivers/vfio/pci/vfio_pci_priv.h | 53 +- include/linux/pci-p2pdma.h | 24 +- include/linux/pci.h | 2 +- include/linux/vfio_pci_core.h | 1 + include/uapi/linux/vfio.h | 25 + 17 files changed, 875 insertions(+), 273 deletions(-) create mode 100644 drivers/pci/p2pdma.h create mode 100644 drivers/pci/p2pdma_core.c -- 2.50.1 (Apple Git-155)

7 hours, 40 minutes

Re: [PATCH v4 03/10] dt-bindings: gpu: host1x: Document memory-regions for NVDEC

by Rob Herring (Arm)

On Fri, 07 Aug 2026 17:54:23 +0200, Thierry Reding wrote: > From: Thierry Reding <treding(a)nvidia.com> > > The video protection region is a reserved memory region that can be used > for secure video playback. NVDEC can access this region to decode images > into securely. > > Signed-off-by: Thierry Reding <treding(a)nvidia.com> > --- > .../devicetree/bindings/gpu/host1x/nvidia,tegra234-nvdec.yaml | 8 ++++++++ > 1 file changed, 8 insertions(+) > Reviewed-by: Rob Herring (Arm) <robh(a)kernel.org>

7 hours, 45 minutes

Re: [PATCH v4 02/10] dt-bindings: display: tegra: Document memory regions

by Rob Herring (Arm)

On Fri, 07 Aug 2026 17:54:22 +0200, Thierry Reding wrote: > From: Thierry Reding <treding(a)nvidia.com> > > Add the memory-region and memory-region-names properties to the bindings > for the display controllers and the host1x engine found on various Tegra > generations. These memory regions are used to access firmware-provided > framebuffer memory as well as the video protection region. > > Signed-off-by: Thierry Reding <treding(a)nvidia.com> > --- > Changes in v4: > - typofix > > Changes in v3: > - document properties for VIC > --- > .../devicetree/bindings/display/tegra/nvidia,tegra124-vic.yaml | 8 ++++++++ > .../devicetree/bindings/display/tegra/nvidia,tegra186-dc.yaml | 10 ++++++++++ > .../devicetree/bindings/display/tegra/nvidia,tegra20-dc.yaml | 10 +++++++++- > .../bindings/display/tegra/nvidia,tegra20-host1x.yaml | 7 +++++++ > 4 files changed, 34 insertions(+), 1 deletion(-) > Reviewed-by: Rob Herring (Arm) <robh(a)kernel.org>

7 hours, 46 minutes

[PATCH] kselftests: dmabuf-heaps: Open heap device files O_RDONLY

by T.J. Mercier

Write permissions on the /dev/dma_heap/* device files are not required to issue ioctls and allocate dmabufs. Applications should be opening these file as O_RDONLY. The BPF dmabuf_iter selftest already does this. [1] Users are pointing to these selftests as examples of how use dmabuf, and encountering permission errors on systems where write permissions are not available on /dev/dma_heap/*. Apply the principle of least privilege to selftests which open dmabuf heaps by removing the write access mode and using O_RDONLY for the open() instead. The same is true for the vgem test using /dev/dri/card. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/too… Signed-off-by: T.J. Mercier <tjmercier(a)google.com> --- tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c b/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c index fc9694fc4e89..45b420e37c97 100644 --- a/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c +++ b/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c @@ -48,7 +48,7 @@ static int open_vgem(void) snprintf(name, 80, "%s%u", drmstr, i); - fd = open(name, O_RDWR); + fd = open(name, O_RDONLY); if (fd < 0) continue; @@ -96,7 +96,7 @@ static int dmabuf_heap_open(char *name) if (ret < 0) ksft_exit_fail_msg("snprintf failed! %d\n", ret); - fd = open(buf, O_RDWR); + fd = open(buf, O_RDONLY); if (fd < 0) ksft_exit_fail_msg("open %s failed: %s\n", buf, strerror(errno)); base-commit: 9a11db68872055e6ead919bad04d6330851c522d -- 2.55.0.679.g6767b8d81c-goog

14 hours, 14 minutes

Re: [PATCH v2 0/5] accel/amdxdna: honour the SYNC_BO range

by Christian König

On 8/12/26 01:13, Taimuraz Kaitmazov wrote: > SYNC_BO carries an offset and a size, but amdxdna_flush_bo() honours them > only on the vmap path. An imported BO is tested for first and flushes its > whole scatterlist, Absolutely clear NAK to that from a DMA-buf maintainer side. Flushing on imported scatterlist of a DMA-buf is a really big NO-GO. If DMA-buf imports are used with the device then the device needs to be able to coherently access the underlying memory. In other words you *CAN'T* call drm_clflush_pages() on imported memory. Regards, Christian. > so a sync costs what the BO is worth rather than what > the caller asked to maintain: on npu4 an imported 64 MiB BO cost 1056 us > to sync at every size from 4 KiB up. Patch 5 reorders the arms so the > vmap path is tried first, and indexes the page-array fallback from the > requested offset. > > The four before it are the ground that has to be solid first. Patch 1 > refuses an I/O memory mapping, which the driver currently stores as if it > were an ordinary kernel address. Patch 2 adds a probe that does not log, > so patch 5 does not make an exporter without a vmap op print on every > ioctl. Patches 3 and 4 fix two ways the ioctl mishandles its own range: a > zero length reaching drm_clflush_virt_range(), and an offset and size > added to the BO address without an overflow check, one level above a > function that checks the same arithmetic. All four stand on their own and > can be taken separately; only patch 5 depends on them. > > v1 did not reach dri-devel, so this is the first version visible there. > It is on lore via the other lists it was copied to: > https://lore.kernel.org/lkml/20260811204556.875037-1-taimuraz@kaitmazov.com/ > > Changes in v2: > - patch 2: take the device from the GEM object rather than abo->client. > amdxdna_gem_obj_close() clears that pointer under abo->lock, which the > pre-split code held across the log and the split did not. > - new patch 3: return early from a zero-length flush. > - new patch 4: check the sync range for overflow on a device BO. > - patch 5: say why the persistent mapping adds no pin. > > The measurements in patch 5 were taken with the equivalent change in > AMD's out-of-tree xdna-driver, where this merged as #1541. That version > and this one differ only in a page-array fallback mainline has no field > for, reached when the mapping fails and the BO is neither imported nor > shmem backed, and in the name of the mapping helper. The flush and the > helper are otherwise identical. This version is compile-tested; it has > not been booted. > > Patch 1 is from inspection rather than a reproducer. The exporter I can > test against is amdgpu, and amdgpu is the case that cannot reach it: it > implements .pin, so a non peer to peer attachment like this driver's > forces the buffer to GTT before anything maps it. Reproducing it needs a > GPU whose exporter has no .pin, which I do not have paired with an NPU > here. > > Taimuraz Kaitmazov (5): > accel/amdxdna: refuse an I/O memory mapping of an imported BO > accel/amdxdna: add a quiet variant of amdxdna_gem_vmap() > accel/amdxdna: return early from a zero-length flush > accel/amdxdna: check the sync range for overflow on a device BO > accel/amdxdna: flush only the requested range in amdxdna_flush_bo > > drivers/accel/amdxdna/amdxdna_gem.c | 66 +++++++++++++++++++++-------- > 1 file changed, 49 insertions(+), 17 deletions(-) > > -- > 2.55.0 >

14 hours, 34 minutes

Re: [PATCH v6 0/4] dma-buf: fix fd leak when copy_to_user() fails after fd_install()

by Sumit Semwal

Hello Baineng, On Fri, 7 Aug 2026 at 15:41, Baineng Shou <shoubaineng(a)gmail.com> wrote: > > Several drivers call dma_buf_fd() — which internally calls fd_install() > — before copy_to_user() returns the fd number to userspace. If > copy_to_user() fails, the fd is already published in the caller's fd > table but the ioctl returns an error, so userspace never learns the fd > number. Worse, the window between fd_install() and copy_to_user() > allows other threads to observe and manipulate the fd (dup, close, > SCM_RIGHTS), making any "close it on the failure path" fix unsafe. > > The fix is to split the allocation into three steps: reserve an fd with > get_unused_fd_flags() (not yet visible to other threads), do > copy_to_user(), and only then publish the fd with fd_install() via the > new dma_buf_fd_install() helper. On copy_to_user() failure, > put_unused_fd() + dma_buf_put() cleanly unwind with no user-visible > side effects. > > Patch 1 introduces dma_buf_fd_install() in dma-buf.c (wrapping > fd_install() together with the DMA_BUF_TRACE call to preserve export > tracing) and applies the fix to dma-heap. > > Patch 2 applies the same fix to fastrpc, which even had a comment > acknowledging the problem could not be fixed before. > > Patch 3 replaces the bare fd_install() in drm_gem_prime_handle_to_fd() > with dma_buf_fd_install() to restore tracepoint coverage for DRM PRIME > exports (suggested by Christian König). > > Patch 4 adds a selftest to tools/testing/selftests/dmabuf-heaps/ that > reproduces the fd-leak scenario (mprotect flip before the ioctl) and > verifies the fd count is unchanged after a failed ioctl (suggested by > Sumit Semwal). Thank you for the series - please feel free to add my Acked-by: Sumit Semwal <sumit.semwal(a)linaro.org> > > v1: https://lore.kernel.org/dri-devel/20260703080922.1838362-1-shoubaineng@gmai… > v2: https://lore.kernel.org/dri-devel/20260710105430.3059661-1-shoubaineng@gmai… > v3: https://lore.kernel.org/dri-devel/20260714114654.3885457-1-shoubaineng@gmai… > > Changes in v6: > - Rework the selftest (patch 4) per review: extract a count_open_fds() > helper, fix the copy_from_user() comment, fail (not skip) when the > ioctl does not return -1, drop the bogus mprotect-race mention, and > reword the result message. > > Changes in v5: > - Add selftest (patch 4) reproducing the fd-leak scenario (Sumit Semwal) > > Changes in v4: > - Add patch 3: drm/prime: use dma_buf_fd_install() (Christian König) > - Add Acked-by: Christian König to patches 1 and 2 > > Changes in v3: > - Split into two patches (dma-heap + fastrpc separately) > - Add dma_buf_fd_install() to preserve trace_dma_buf_fd tracepoint > - Add fastrpc fix using the new helper (T.J. Mercier) > > Baineng Shou (4): > dma-buf: dma-heap: don't publish fd before copy_to_user() succeeds > misc: fastrpc: don't publish fd before copy_to_user() succeeds > drm/prime: use dma_buf_fd_install() to preserve export tracing > selftests: dmabuf-heaps: add fd-leak-on-EFAULT regression test > > drivers/dma-buf/dma-buf.c | 20 ++++ > drivers/dma-buf/dma-heap.c | 80 ++++++------- > drivers/gpu/drm/drm_prime.c | 2 +- > drivers/misc/fastrpc.c | 16 +-- > include/linux/dma-buf.h | 1 + > .../selftests/dmabuf-heaps/dmabuf-heap.c | 113 +++++++++++++++++- > 6 files changed, 180 insertions(+), 52 deletions(-) > > -- > 2.34.1 > Best, Sumit.

14 hours, 53 minutes

Re: [PATCH v5 2/9] PCI/P2PDMA: Add CONFIG_PCI_P2PDMA_CORE

by Matt Evans

Hi Logan, Jason, Alex, On 05/08/2026 21:50, Logan Gunthorpe wrote: > > > On 2026-08-05 10:40, Jason Gunthorpe wrote: >> On Wed, Aug 05, 2026 at 05:28:27PM +0100, Matt Evans wrote: >>> Hi both (Logan thanks for your comments!), >>> >>> On 05/08/2026 01:39, Jason Gunthorpe wrote: >>>> On Tue, Aug 04, 2026 at 10:19:11AM -0600, Logan Gunthorpe wrote: >>>> >>>>> There's a vague convention for this already: the term 'p2pmem' is often >>>>> used for cases where the driver uses the allocator, etc. (I think I had >>>>> this intention when I wrote the code and have since forgotten about >>>>> it). >>>> >>>> I've been calling it the genalloc layer and the core layer. p2pmem >>>> would be OK to refer to the genalloc stuff. So if you want to have >>>> CONFIG_PCI_P2PDMA and CONFIG_PCI_P2PMEM that seem sOk >>>> >>>>> code into it's own file, potentially renaming some functions. Then, in >>>>> the end, we would probably have a pcim_p2pdma_supported() function and a >>>>> pcim_p2pmem_supported() function, the latter being used by existing use >>>>> cases. >>>> >>>> Not quite sure why we need this? >>> >>> ( [1] ) >>> >>>> Matt, the mlx5 stuff is the same as VFIO, it just uses the "core" >>>> layer and does not use the genalloc. So there shouldn't be an issue >>>> here, if the genalloc is off then the mlx5 stuff should still >>>> work. There shouldn't be a case where CONFIG_PCI_P2PDMA=y and mlx5 is >>>> broken? >>> >>> Oh, when CONFIG_PCI_P2PDMA=y it's all good. >>> >>> The issue is when CONFIG_PCI_P2PDMA=n, as mlx5 still seems to permit a >>> DMABUF export solely because pcim_p2pdma_provider() succeeds. (This >>> patch's CONFIG_PCI_P2PDMA_CORE enables that.) mlx5 assumes that getting >>> a provider means P2P DMA is also available. >> >> That's my point, the mlx5 should work fine with CONFIG_PCI_P2PDMA_CORE >> only or it is split wrong. >> >>> Later, DMABUF attach would fail, but it'd be good to keep the original >>> failure mode where UVERBS_METHOD_DMABUF_ALLOC fails early if no P2PDMA. >> >> Why does it fail? It should not fail :) >> >>> I was thinking something trivial like the following would let things >>> like IB fail the DMABUF_ALLOC early still, instead of making the >>> assupmtion that having a provider means having P2P DMA. E.g. OK >>> provider's available, but test for P2P DMA support too: >>> >>> bool pcim_p2pdma_supported(void) >>> { >> >> it seems illogical, if you have a provider you have p2p dma, things >> are split wrong if this is not true. >> >> The split should be only around the genalloc and related >> (sysfs,etc,etc) not anything mlx5 uses. >> >> The only think that should stop working without genalloc (ie >> CONFIG_PCI_P2PDMA=n) is nvme. > > Hmm, seems I made a few mistakes in my reply. Though I'm less certain I > understand the issue anymore. > > I did confuse in my response pcim_p2pdma_provider() and > pcim_p2pdma_supported(). The later choice I really dislike. My thinking > was we'd have two versions of pcim_p2p[dma|mem]_provider()... But I'm > not sure that's necessary. > > I thought there were callers of pcim_p2pdma_provider() in the p2pmem > code (non-core) section. But digging deeper today, it seems like > pci_p2pdma_add_resource() is the one caller, which I'd expect would be > compiled out when CONFIG_PCI_P2PDMA=n. Which seems fine. So I'm not sure > I understand the root issue anymore. Yes, this has gone round in a bit of a circle, apologies. I went back to the original change and concluded vfio-pci _doesn't_ always need a provider, so this work to refactor P2PDMA to provide a provider even when CONFIG_PCI_P2PDMA=n is unnecessary. The original original need was to break the dependency of vfio-pci on P2PDMA, since the series made vfio-pci dependent on CONFIG_VFIO_PCI_DMABUF. I'd missed that the _export_ of a DMABUF doesn't need a provider (after all, the phys_vec has the PFNs). So we can just do this: - VFIO exports DMABUF internally for mmap() - It attempts to get a provider for the export, but if CONFIG_PCI_P2PDMA=n then provider is NULL. - The DMABUF works fine for CPU access through the VMA - If someone were to fish out the DMABUF from the VMA/fds then it cannot be imported unless the provider is valid (dma_buf_map_attachment() returns -EINVAL) - Drop the P2PDMA changes entirely IOW, when CONFIG_PCI_P2PDMA=n you obviously don't get to import DMABUFs into another driver for P2P, so the provider is unused ... and when P2PDMA is supported, the VFIO BAR VMA's DMABUF has a provider, so could be imported for P2P. The struct vfio_pci_dma_buf only needs to have a valid provider if the DMABUF could possibly be imported of course. The behaviour of pcim_p2pdma_provider() is unchanged, as are any other consumers such as mlx5. This is way simpler. I will drop these first two P2PDMA refactor patches from this series. Sorry for the churn and time spent on these; though a cleanup/refactor could have merit separately it sounds like there's discussion needed on exactly what that goal should be. Matt

1 day, 16 hours

Re: [PATCH v9 4/5] rust: Add dma_fence abstractions

by Gary Guo

On Wed Aug 5, 2026 at 3:59 PM BST, Philipp Stanner wrote: > C's dma_fence's are synchronisation primitives that will be needed by all > Rust GPU drivers. > > The dma_fence framework sets a number of rules, notably: > - fences must only be signaled once > - all fences must be signaled at some point > - fence error codes must only be set before signaling > - every pointer to a fence must be backed by a reference > > All those rules are being addressed by these abstractions. > > To cleanly decouple fence issuers and consumers, two types are provided: > - DriverFence: the only fence type that can be signaled and that > carries driver-specific data. > - Fence: the fence type to be shared with other drivers and / or > userspace. The only type callbacks can be registered on. > Cannot be signaled. > > Hereby, a Fence lives in the same chunk of memory as a DriverFence. Both > share the refcount of the underlying C dma_fence. Since this > implementation does not provide a custom dma_fence_backend_ops.release() > function, the memory is freed by the dma_fence backend once the refcount > drops to 0. > > To create a DriverFence, the user must first allocate a > DriverFenceAllocation, so that the creation of the DriverFence later on > can always succeed. Otherwise, deadlocks could occur if fences need to > be created in a GPU job submission path. > > Synchronization is ensured by the dma_fence backend. > > All DriverFence's created through this abstraction must be signaled by > the creator with an error code. In case a DriverFence drops without > being signaled beforehand, it is signaled with -ECANCELLED as its > error and a warning is printed. This allows the Rust abstraction to very > cleanly decouple fence issuer and consumer by relying on the decoupling > mechanisms in the C backend, which ensures through RCU and the > 'signaled' fence-flag that dma_fence_backend_ops functions cannot > access the potentially unloaded driver code anymore. > > Signalling fences on drop thus grants many advantages. Not signaling > fences on drop would risk deadlock and does not grant real advantages: > By definition only the drivers can ensure that a fence always represents > the hardware's state correctly. > > This implementation models a DmaFenceContext object on which fences are > to be created, thereby ensuring correct sequence numbering according to > the timeline. > > dma_fence supports a variety of callbacks. The mandatory callbacks > (get_timeline_name() and get_driver_name()) are implemented in this > patch. For convenience, they store those name parameters in the fence > context, saving the driver from implementing these two callbacks. > > Support for other callbacks (like for hardware signaling) is prepared > for through the fact that both DriverFence and Fence live in the same > allocation, allowing for usage of container_of from the callback to > access the driver-specific data. > > It is expected that other callbacks, added in the future, also mostly > operate on the generic data in the FenceContext. To make this safe, the > implementation ensures through a lifetime that a DriverFence cannot > outlive its FenceContext. > > Synchronization for dma_fence_ops callbacks is ensured by only running the > Rust deconstructor delayed with call_rcu(), which prevents UAF-bugs > should a DriverFence drop while a Fence callback is currently operating > on the associated driver data. Since they can also operate on the > FenceContext's data, its drop implementation also performs the necessary > delay with rcu_barrier(). > > An additional issue discovered during the review process of this code is > that there is (currently) no mechanism in Rust to prevent someone from > circumventing the DriverFence's FenceContext-reference's lifetime by > "forgetting" the fence, e.g. with core::mem::forget(). This would enable > UAF bugs on the FenceContext. Throw a panic if this happens and document > a path towards a more robust solution. > > Add abstractions for dma_fence in Rust. > > Signed-off-by: Philipp Stanner <phasta(a)kernel.org> > Tested-by: Daniel Almeida <daniel.almeida(a)collabora.com> > --- > rust/bindings/bindings_helper.h | 1 + > rust/helpers/dma_fence.c | 48 ++ > rust/helpers/helpers.c | 1 + > rust/kernel/dma_buf/dma_fence.rs | 1002 ++++++++++++++++++++++++++++++ > rust/kernel/dma_buf/mod.rs | 14 + > rust/kernel/lib.rs | 1 + > 6 files changed, 1067 insertions(+) > create mode 100644 rust/helpers/dma_fence.c > create mode 100644 rust/kernel/dma_buf/dma_fence.rs > create mode 100644 rust/kernel/dma_buf/mod.rs > > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h > index 1124785e210b..54b62d952e01 100644 > --- a/rust/bindings/bindings_helper.h > +++ b/rust/bindings/bindings_helper.h > @@ -53,6 +53,7 @@ > #include <linux/debugfs.h> > #include <linux/device/faux.h> > #include <linux/dma-direction.h> > +#include <linux/dma-fence.h> > #include <linux/dma-mapping.h> > #include <linux/dma-resv.h> > #include <linux/errname.h> > diff --git a/rust/helpers/dma_fence.c b/rust/helpers/dma_fence.c > new file mode 100644 > index 000000000000..0e08411098fa > --- /dev/null > +++ b/rust/helpers/dma_fence.c > @@ -0,0 +1,48 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +#include <linux/dma-fence.h> > + > +__rust_helper void rust_helper_dma_fence_get(struct dma_fence *f) > +{ > + dma_fence_get(f); > +} > + > +__rust_helper void rust_helper_dma_fence_put(struct dma_fence *f) > +{ > + dma_fence_put(f); > +} > + > +__rust_helper bool rust_helper_dma_fence_begin_signalling(void) > +{ > + return dma_fence_begin_signalling(); > +} > + > +__rust_helper void rust_helper_dma_fence_end_signalling(bool cookie) > +{ > + dma_fence_end_signalling(cookie); > +} > + > +__rust_helper bool rust_helper_dma_fence_is_signaled(struct dma_fence *f) > +{ > + return dma_fence_is_signaled(f); > +} > + > +__rust_helper bool rust_helper_dma_fence_test_signaled_flag(struct dma_fence *f) > +{ > + return dma_fence_test_signaled_flag(f); > +} > + > +__rust_helper void rust_helper_dma_fence_lock_irqsave(struct dma_fence *f, unsigned long *flags) > +{ > + dma_fence_lock_irqsave(f, *flags); > +} > + > +__rust_helper void rust_helper_dma_fence_unlock_irqrestore(struct dma_fence *f, unsigned long *flags) > +{ > + dma_fence_unlock_irqrestore(f, *flags); > +} > + > +__rust_helper void rust_helper_dma_fence_set_error(struct dma_fence *f, int error) > +{ > + dma_fence_set_error(f, error); > +} > diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c > index 998e31052e66..4ab8aa9da7e7 100644 > --- a/rust/helpers/helpers.c > +++ b/rust/helpers/helpers.c > @@ -58,6 +58,7 @@ > #include "cred.c" > #include "device.c" > #include "dma.c" > +#include "dma_fence.c" > #include "dma-resv.c" > #include "drm.c" > #include "drm_gpuvm.c" > diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs > new file mode 100644 > index 000000000000..e61b4b2d8b8c > --- /dev/null > +++ b/rust/kernel/dma_buf/dma_fence.rs > @@ -0,0 +1,1002 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2025-2026 Red Hat Inc. > + * Author: Philipp Stanner <pstanner(a)redhat.com> > + */ > + > +//! DriverFence support. > +//! > +//! Reference: <https://docs.kernel.org/driver-api/dma-buf.html#c.dma_fence> > +//! > +//! header: [`include/linux/dma-fence.h`](srctree/include/linux/dma-fence.h) > + > +use crate::{ > + alloc::AllocError, > + bindings, > + container_of, > + error::to_result, > + prelude::*, > + types::ForeignOwnable, > + types::Opaque, // > +}; > + > +use core::{ > + marker::PhantomData, > + mem::ManuallyDrop, > + ops::Deref, > + ptr, > + ptr::{ > + drop_in_place, > + NonNull, // > + }, // > +}; > + > +use kernel::{ > + str::CString, > + sync::{ > + aref::{ > + ARef, > + AlwaysRefCounted, // > + }, > + atomic::{ > + Atomic, > + Relaxed, // > + }, > + rcu::rcu_barrier, // > + }, // > +}; > + > +/// VTable for dma_fence backend_ops callbacks. > +// > +// Mandatory dma_fence backend_ops are implemented implicitly through > +// [`FenceContext`]. Additional ones shall get implemented on this trait. > +pub trait FenceContextOps { > + /// The generic payload data for [`DriverFence`]s created on this fctx. > + type FenceDataType: Send + Sync; > +} > + > +/// A dma-fence context. A fence context takes care of associating related fences > +/// with each other, providing each with raising sequence numbers and a common > +/// identifier. > +#[pin_data(PinnedDrop)] > +pub struct FenceContext<T: FenceContextOps + Send + Sync> { > + /// The fence context number. > + nr: u64, > + /// The sequence number for the next fence created. > + seqno: Atomic<u64>, > + // The name parameters can be accessed by the dma_fence backend_ops. UAF > + // errors are prevented by the `call_rcu()` in `drop_driver_fence_data()`. > + /// The name of the driver this FenceContext's fences belong to. > + driver_name: CString, > + /// The name of the timeline this FenceContext's fences belong to. > + timeline_name: CString, > + /// The number of all unsignaled fences on this context. > + // Used to prevent bugs due to forgotten fences. > + // > + // The lifetime on `DriverFence`s should typically prevent this from > + // happening. > + // > + // However, we cannot fully guarantee in Rust that `DriverFence`s will not > + // be forgotten, e.g., through `core::mem::forget()`. This could circumvent > + // the lifetime which intends to enforce that all fences disappear before > + // their context. > + nr_of_unsignaled_fences: Atomic<u64>, This can be `Atomic<usize>` so it doesn't need to go through the generic 64-bit atomic mechanism on 32-bit systems. > + /// The user's data. > + #[pin] > + data: T, > +} > + > +impl<'a, T: Send + Sync + FenceContextOps> FenceContext<T> { > + // This can later be extended as a vtable in case other parties need support > + // for the more "exotic" callbacks. > + const OPS: bindings::dma_fence_ops = bindings::dma_fence_ops { > + get_driver_name: Some(Self::get_driver_name), > + get_timeline_name: Some(Self::get_timeline_name), > + enable_signaling: None, > + signaled: None, > + wait: None, > + release: None, > + set_deadline: None, > + }; > + > + /// Create a new `FenceContext`. > + pub fn new<E>( > + initial_seqno: u64, > + driver_name: CString, > + timeline_name: CString, > + data: impl PinInit<T, E>, > + ) -> impl PinInit<Self, Error> > + where > + Error: From<E>, > + { > + try_pin_init!(Self { > + // SAFETY: `dma_fence_context_alloc()` merely works on a global > + // atomic. Parameter `1` is the number of contexts we want to > + // allocate. > + nr: unsafe { bindings::dma_fence_context_alloc(1) }, > + seqno: Atomic::new(initial_seqno), > + driver_name, > + timeline_name, > + nr_of_unsignaled_fences: Atomic::new(0), > + data <- data, > + }) > + } > + > + fn next_seqno(&self) -> u64 { > + self.seqno.fetch_add(1, Relaxed) > + } > + > + /// Allocate the memory for a [`DriverFence`] and already store `data` inside. > + /// > + /// This is needed because many times, creation of a [`DriverFence`] must not > + /// fail, and allocating might deadlock in some situations. > + /// > + /// The `data` you pass here must not perform any operations that are illegal > + /// in atomic context in its [`Drop`] implementation. > + pub fn new_fence_allocation( > + &self, > + data: T::FenceDataType, > + ) -> Result<DriverFenceAllocation<'_, T>> { > + let fence_data = DriverFenceData { > + rcu_head: Default::default(), > + // `inner` remains uninitialized until a `DriverFence` takes over. > + inner: Fence { > + inner: Opaque::uninit(), > + }, > + fctx: self, > + data, > + }; > + > + // In order to support the C dma_fence callbacks, it is necessary for > + // a `Fence` and a `DriverFence` to live in the same allocation, > + // because the C backend passes a dma_fence, from which the driver most > + // likely wants to be able to access its `data` in `DriverFence`. > + // > + // Hence, we need the manage the memory manually. It will be freed by the > + // C backend automatically once the refcount within `Fence` drops to 0. > + let data = KBox::new(fence_data, GFP_KERNEL | __GFP_ZERO)?; > + > + Ok(DriverFenceAllocation { > + data, > + ops: &Self::OPS, > + }) > + } > + > + extern "C" fn get_driver_name(ptr: *mut bindings::dma_fence) -> *const c_char { > + // SAFETY: The C backend only invokes this callback with `ptr` pointing > + // to a valid, unsignaled `bindings::dma_fence`. All fences created in > + // this module always reside within `Fence` which always resides in a > + // `DriverFenceData`, thus satisfying the function's safety > + // requirements. > + let fctx = unsafe { Self::from_raw_fence(ptr) }; > + > + fctx.driver_name.as_char_ptr() > + } > + > + extern "C" fn get_timeline_name(ptr: *mut bindings::dma_fence) -> *const c_char { > + // SAFETY: The C backend only invokes this callback with `ptr` pointing > + // to a valid, unsignaled `bindings::dma_fence`. All fences created in > + // this module always reside within `Fence` which always resides in a > + // `DriverFenceData`, thus satisfying the function's safety > + // requirements. > + let fctx = unsafe { Self::from_raw_fence(ptr) }; > + > + fctx.timeline_name.as_char_ptr() > + } > + > + /// Create a [`FenceContext`] from an associated [`bindings::dma_fence`]. > + /// > + /// # Safety > + /// > + /// `ptr` must be a valid pointer to a [`bindings::dma_fence`] which resides > + /// within a [`Fence`], which in turn resides in a [`DriverFenceData`]. > + unsafe fn from_raw_fence(ptr: *mut bindings::dma_fence) -> &'a Self { > + let opaque_fence = Opaque::cast_from(ptr); > + > + // SAFETY: Safe due to the function's overall safety requirements. > + let fence_ptr = unsafe { container_of!(opaque_fence, Fence, inner) }; > + > + // CAST: `DriverFenceData` is repr(C) and a `Fence` is its first member. `repr(C)` > + let fence_data_ptr = fence_ptr as *mut DriverFenceData<'a, T>; `*const` should work here too? > + > + // SAFETY: Safe because of the comments directly above. > + let fence_data = unsafe { &*fence_data_ptr }; > + > + fence_data.fctx > + } > +} > + > +// FenceContext's drop() ensures that the driver cannot unload while there are > +// still dma_fence callbacks running. This also prevents UAF problems with > +// `fctx.driver_name` and `fctx.timeline_name`. > +// > +// DriverFence data gets dropped through `call_rcu()` in `DriverFence::drop`. > +// This `rcu_barrier()` also serves to wait for their completion. These should be comment on the drop code itself, not as comment of `PinnedDrop` impl. This can be more detailed about why a UAF problem exists (and this should be commented on the panic part, as something like: // Fence ops callbacks may be called on unsignaled fences, so we may not // leak any driver fences, otherwise `fctx.driver_name` and // `fctx.timeline_name` can be accessed after drop (UAF). if self.nr_of_unsignaled_fences.load(Relaxed) != 0 { // Fence ops callbacks use RCU to sychronize callbacks and thus we use `call_rcu` // to destroy `DriverFence` data. `rcu_barrier` here synchronize with driver // fence's destruction. > +#[pinned_drop] > +impl<T: FenceContextOps + Send + Sync> PinnedDrop for FenceContext<T> { > + fn drop(self: Pin<&mut Self>) { > + // TODO: > + // It would be better if the fence context signals all forgotten fences > + // itself. To do so, it would keep a list of unsignaled fences. That > + // list members would have to be pre-allocated (see > + // `FenceCallback::new_fence_allocation()`). > + if self.nr_of_unsignaled_fences.load(Relaxed) != 0 { > + panic!("Forgotten fences in FenceContext."); > + } > + > + rcu_barrier(); > + } > +} > + > +/// Error type for fence callback registration. > +/// > +/// Generic over `T` so that `AlreadySignaled` can return the callback to the > +/// caller, allowing it to reclaim any resources owned by the callback (e.g., > +/// a fence handle that needs to be signaled). > +#[derive(Debug)] > +pub enum CallbackError<T = ()> { What is this `= ()` used for? > + /// The fence was already signaled. The callback is returned so the caller > + /// can extract owned resources without losing them. > + AlreadySignaled(T), > + /// Some other error occurred during registration. > + Other(Error), > +} > + > +impl<T> From<CallbackError<T>> for Error { #[inline] > + fn from(err: CallbackError<T>) -> Self { > + match err { > + CallbackError::AlreadySignaled(_) => ENOENT, > + CallbackError::Other(e) => e, > + } > + } > +} > + > +impl<T> From<AllocError> for CallbackError<T> { #[inline] > + fn from(e: AllocError) -> Self { > + CallbackError::Other(Error::from(e)) > + } > +} > + > +/// Trait for callbacks that can be registered on fences. > +/// > +/// When the fence signals, the callback will be invoked. > +/// > +/// # Example > +/// > +/// ```rust > +/// use kernel::dma_buf::FenceCallback; > +/// > +/// struct MyCallback { > +/// // Your callback state here > +/// } > +/// > +/// impl FenceCallback for MyCallback { > +/// fn called(&mut self) { > +/// pr_info!("Fence signaled!"); > +/// // Handle fence completion > +/// } > +/// } > +/// ``` > +pub trait FenceCallback: Send + 'static { > + /// Called when the fence is signaled. > + /// > + /// This is called from the fence signaling path, which may be in interrupt > + /// context or with locks held, which is why `self` is only borrowed, so that > + /// it cannot drop. Implementations must not sleep or perform > + /// long-running operations. > + /// > + /// An implementation likely wants to inform itself (e.g., through a work item) > + /// within this callback that the associated [`FenceCallbackRegistration`] > + /// can now be dropped. > + fn called(&mut self); The name feels a bit awkward to me. I think this should either look like an action on the callback, in which case "call" or describe an event on the fence, i.e. "on_signal" or "signaled". Naming it "called" is very weird because it's not a event that is triggered when something is "called". > +} > + > +/// A callback registration on a fence. > +/// > +/// When this object is dropped, the callback is automatically removed if it > +/// hasn't been called yet. > +#[pin_data(PinnedDrop)] > +pub struct FenceCallbackRegistration<T: FenceCallback + 'static> { > + #[pin] > + callback_foreign: Opaque<bindings::dma_fence_cb>, > + callback: ManuallyDrop<T>, > + fence: ARef<Fence>, > +} > + > +impl<T: FenceCallback> FenceCallbackRegistration<T> { > + /// Create a [`PinInit`] closure for registering a callback on a fence. > + /// > + /// The actual attempt at registering the callback will take place once you > + /// call an allocator's `pin_init()` function. > + /// > + /// On success the callback is pinned in place and will fire when the fence > + /// signals. On `AlreadySignaled` the callback is returned to the caller so > + /// that owned resources can be reclaimed. > + pub fn new<'a>(fence: &'a Fence, callback: T) -> impl PinInit<Self, CallbackError<T>> + 'a > + where > + T: 'a, > + { > + try_pin_init!(Self { > + // We need to fully initialize the fence because after > + // `dma_fence_add_callback()` ran, the callback might immediately > + // get invoked. > + callback: ManuallyDrop::new(callback), > + fence: ARef::from(fence), > + callback_foreign <- Opaque::try_ffi_init(|ptr| { > + // SAFETY: `fence.inner.get()` is a valid, initialized `struct > + // dma_fence`. `ptr` points to the `struct dma_fence_cb` field > + // within the pinned allocation, so it remains valid until > + // `dma_fence_remove_callback()` in `PinnedDrop` or until the > + // callback fires. > + let ret = unsafe { > + to_result(bindings::dma_fence_add_callback( > + fence.inner.get(), > + ptr, > + Some(Self::dma_fence_callback), > + )) > + }; > + match ret { > + Ok(()) => Ok(()), > + Err(e) => { > + // SAFETY: We could not register the callback. Thus, > + // C will not use it. So we can just take it back > + // and pass it to the user again. > + let cb_back = unsafe { ManuallyDrop::take(callback) }; > + if e == ENOENT { > + Err(CallbackError::AlreadySignaled(cb_back)) > + } else { > + Err(CallbackError::Other(e)) > + } > + }, > + } > + }), > + }? CallbackError<T>) > + } > + > + /// Raw dma fence callback that is called by the C code. > + /// > + /// # Safety > + /// > + /// This is only called by the dma_fence subsystem with valid pointers. > + unsafe extern "C" fn dma_fence_callback( > + _fence: *mut bindings::dma_fence, > + callback_foreign: *mut bindings::dma_fence_cb, > + ) { > + let ptr = Opaque::cast_from(callback_foreign).cast_mut(); > + > + // SAFETY: All `cb` we can receive here have been created in such a way > + // that they are embedded into a `FenceCallbackRegistration`. The > + // backend ensures synchronisation so whoever holds the registration > + // object cannot drop it while this code is running. See > + // `FenceCallbackRegistration::drop`. > + unsafe { > + let reg: *mut Self = container_of!(ptr, Self, callback_foreign); > + > + (*reg).callback.called(); > + } > + } > + > + /// Returns a reference to the fence this callback is registered on. > + pub fn fence(self: Pin<&Self>) -> &Fence { > + &self.get_ref().fence > + } This can just be `fence(&self) -> &Fence`. > +} > + > +#[pinned_drop] > +impl<T: FenceCallback> PinnedDrop for FenceCallbackRegistration<T> { > + fn drop(self: Pin<&mut Self>) { > + // Always call dma_fence_remove_callback, even if `callback` has already > + // been taken by `dma_fence_callback`. This is necessary for Is this still up-to-date? You're not taking callback anymore in `dma_fence_callback`. > + // synchronization: `dma_fence_remove_callback` acquires `fence->lock`, > + // which ensures that any in-flight `dma_fence_signal` (which calls our > + // callback while holding the same lock) has completed before we free > + // the struct. > + // > + // Without this, Drop can race with a concurrent signal: > + // CPU0 (signal, lock held): take() -> signaled(fence_ref) (in progress) > + // CPU1 (drop): sees is_some()==false -> skips lock -> frees struct > + // CPU0: accesses fence_ref -> use-after-free > + // > + // When the callback has already fired, the signal path detached the > + // list node via INIT_LIST_HEAD, so dma_fence_remove_callback just sees > + // an empty node and returns false — the lock acquisition is the only > + // thing that matters. > + // > + // SAFETY: The fence pointer is valid and the cb was initialized by > + // dma_fence_add_callback during construction. > + unsafe { > + bindings::dma_fence_remove_callback(self.fence.as_raw(), self.callback_foreign.get()); > + } > + > + // SAFETY: This is literally the drop implementation, so no one has > + // dropped this so far; so we can do it now. > + unsafe { ManuallyDrop::<T>::drop(self.project().callback) }; > + } > +} > + > +// SAFETY: FenceCallbackRegistration can be sent between threads. > +unsafe impl<T: FenceCallback> Send for FenceCallbackRegistration<T> {} > + > +// SAFETY: &FenceCallbackRegistration can be shared between threads if &T can. > +unsafe impl<T: FenceCallback> Sync for FenceCallbackRegistration<T> where T: Sync {} > + > +/// The receiving counterpart of a [`DriverFence`]. > +/// > +/// The Rust DMA fence implementation has a dualistic design: [`DriverFence`]s > +/// are the producer-side, intended to be always owned by only one party. That > +/// party has the monopoly on signalling the fence. > +/// > +/// A [`Fence`] is the counterpart for consumers. Thus, [`Fence`]s are always > +/// refcounted and can shared with an arbitrary number of parties, including > +/// userspace. A [`Fence`] can only be used for actions such as checking the > +/// fence's status or for registering callbacks on it. > +/// > +/// Once the associated [`DriverFence`] signals, all > +/// [`FenceCallbackRegistration`]s registered on the [`Fence`] will be executed. > +/// > +/// A [`Fence`] can arbitrarily outlive its [`DriverFence`] and the > +/// [`FenceContext`]. Signalling a [`DriverFence`] decouples it from its > +/// [`Fence`]s. > +#[repr(transparent)] > +pub struct Fence { > + /// The actual dma_fence passed to C. > + inner: Opaque<bindings::dma_fence>, > +} > + > +/// Guard helper for locking within this module. > +/// > +/// Its only purpose for now is to avoid a number of unsafe lock-unlock cycles. > +/// It is never used outside of this module. > +// TODO: This should be made more canonical, probably by basing it on a > +// SpinLockIrqGuard once available. > +struct FenceGuard { > + inner: *mut bindings::dma_fence, > + flags: usize, > +} > + > +impl Deref for FenceGuard { > + type Target = *mut bindings::dma_fence; Why not store and return `&Fence`? > + > + fn deref(&self) -> &Self::Target { > + &self.inner > + } > +} > + > +impl Drop for FenceGuard { > + fn drop(&mut self) { > + // SAFETY: `fence` is valid because `self` is valid. `flag_ptr` is > + // merely a pointer to an integer, which lives as long as this function. > + // When a `FenceGuard` exists, the lock has been taken by definition. > + unsafe { bindings::dma_fence_unlock_irqrestore(self.inner, &raw mut self.flags) }; > + } > +} > + > +// SAFETY: Fences are literally designed to be shared between threads. > +unsafe impl Send for Fence {} > +// SAFETY: Fences are literally designed to be shared between threads. > +unsafe impl Sync for Fence {} > + > +impl Fence { > + /// Check whether the fence was signaled at the moment of the function call. > + /// > + /// Note that this can return `true` for a [`Fence`] whose [`DriverFence`] > + /// has not yet been dropped. The reason is that the fence ops callbacks can > + /// cause the fence to get signaled by the C backend. > + pub fn is_signaled(&self) -> bool { > + // We should not use `dma_fence_is_signaled_locked()` here, because > + // according to the C backend's recommendations, that function is > + // problematic and we should avoid calling that function with a lock > + // held. > + > + // SAFETY: Inner `fence` is valid because `self` is valid. > + let ret = unsafe { bindings::dma_fence_is_signaled(self.as_raw()) }; > + > + // To be as robust as possible for the future we guarantee that an API > + // caller can 100% rely on the signalling being completed (i.e., all > + // fence callbacks ran), so we have to take the lock. > + // > + // The reason is that the C dma_fence backend currently does not > + // carefully synchronize the `dma_fence_is_signaled()` function with the > + // proper spinlock. This can lead to the function returning `true` while > + // fence callbacks are still being executed. This can be mitigated by > + // guarding the entire function with the spinlock. > + // > + // The fundamental reason is that the C backend currently does guard > + // setting of the fence's signaled-bit with the fence's spinlock, but > + // reading is done locklessly. > + // > + // See commit c8a5d5ea3ba6a. > + Extra newline here. > + let _ = self.lock(); > + > + ret > + } > + #[inline] here and many more below. > + fn lock(&self) -> FenceGuard { > + let mut guard = FenceGuard { > + inner: self.as_raw(), > + flags: 0, > + }; > + > + // SAFETY: `fence` is valid because `self` is valid. `flag_ptr` is > + // merely a pointer to an integer, whose lifetime is tied to the guard > + // object. > + unsafe { bindings::dma_fence_lock_irqsave(guard.inner, &raw mut guard.flags) }; > + > + guard > + } > + > + /// Get the fence's sequence number. > + pub fn seqno(&self) -> u64 { > + // SAFETY: Valid because `self` is valid. > + unsafe { (*self.as_raw()).seqno } > + } > + > + fn as_raw(&self) -> *mut bindings::dma_fence { > + self.inner.get() > + } > + > + /// Create a [`Fence`] from a raw C [`bindings::dma_fence`]. > + /// > + /// # Safety > + /// > + /// `ptr` must point to an initialized fence that is embedded into a [`Fence`]. > + pub unsafe fn from_raw<'a>(ptr: *mut bindings::dma_fence) -> &'a Self { > + // SAFETY: Safe as per the function's overall safety requirements. > + unsafe { &*ptr.cast() } > + } > +} > + > +// SAFETY: These implement the C backends refcounting methods which are proven > +// to work correctly. > +unsafe impl AlwaysRefCounted for Fence { > + fn inc_ref(&self) { > + // SAFETY: `self.as_raw()` is a pointer to a valid `struct dma_fence`. > + unsafe { bindings::dma_fence_get(self.as_raw()) } > + } > + > + /// # Safety > + /// > + /// `ptr`must be a valid pointer to a [`DriverFence`]. > + unsafe fn dec_ref(ptr: NonNull<Self>) { > + // SAFETY: `ptr` is never a NULL pointer; and when `dec_ref()` is called > + // the fence is by definition still valid. > + let fence = unsafe { (*ptr.as_ptr()).inner.get() }; > + > + // SAFETY: `fence` was created validly above. When `dec_ref()` is called, > + // there is by definition still a reference alive that can be put. > + unsafe { bindings::dma_fence_put(fence) } > + } > +} > + > +// Necessary to guarantee that `inner` always comes first and can be freed by C. > +// Also useful for using casts instead of container_of(). > +#[repr(C)] > +#[pin_data] > +struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> { > + #[pin] > + /// The inner fence. > + // Must always be the first member so that unsafe casting works; but also > + // necessary so that the C backend can free the allocation (coming from our > + // Rust code) with kfree_rcu(). > + inner: Fence, > + /// Callback head for dropping this in a deferred manner through RCU. > + rcu_head: bindings::callback_head, > + /// Reference to access the FenceContext. Useful for obtaining name parameters. > + fctx: &'a FenceContext<T>, > + /// The API user's data. It is essential that the data only performs > + /// operations legal in atomic context in its [`Drop`] implementation. > + #[pin] > + data: T::FenceDataType, > +} > + > +/// A synchronization primitive mainly for GPU drivers. > +/// > +/// The Rust DMA fence implementation has a dualistic design: [`DriverFence`]s > +/// are the producer-side, intended to be always owned by only one party. That > +/// party has the monopoly on signalling the fence. > +/// > +/// A [`Fence`] is the counterpart for consumers. Thus, [`Fence`]s are always > +/// refcounted and can shared with an arbitrary number of parties, including > +/// userspace. A [`Fence`] can only be used for actions such as checking the > +/// fence's status or for registering callbacks on it. > +/// > +/// Once the associated [`DriverFence`] signals, all > +/// [`FenceCallbackRegistration`]s registered on a [`Fence`] will be executed. > +/// > +/// A [`Fence`] can arbitrarily outlive its [`DriverFence`] and the > +/// [`FenceContext`]. Signalling a [`DriverFence`] decouples it from its > +/// [`Fence`]s. > +/// > +/// It is crucial that a [`DriverFence`] always correctly represents the state > +/// of the associated job on the hardware. Especially, it is strictly necessary > +/// that the owner ensures that all [`DriverFence`]s eventually get signaled. > +/// As a last resort, a [`DriverFence`] will signal itself if it drops > +/// unsignaled and print a warning. > +/// > +/// This design intends to implement the [`bindings::dma_fence_ops`] in such a > +/// way that the driver-data necessary to implement the callback's functionality > +/// resides in the [`FenceContext`]. Thus, a [`DriverFence`] contains a > +/// reference to the context, which can be accessed in the callbacks. The > +/// implementation, therefore, ensures that a [`DriverFence`] cannot outlive its > +/// [`FenceContext`]. Unfortunately, this can be circumvented under certain > +/// circumstances in Rust (e.g., usage of [`core::mem::forget`]). > +/// > +/// In the unlikely case of such violations, error warnings are printed. > +/// > +/// # Examples > +/// > +/// ``` > +/// use kernel::dma_buf::{ > +/// DriverFence, > +/// FenceContext, > +/// FenceContextOps, > +/// FenceCallback, > +/// FenceCallbackRegistration, // > +/// }; > +/// use kernel::str::CString; > +/// use kernel::sync::aref::ARef; > +/// use core::fmt::Display; > +/// > +/// struct CallbackData { } > +/// > +/// impl FenceCallback for CallbackData { > +/// fn called(&mut self) { > +/// pr_info!("DmaFence callback executed.\n"); > +/// } > +/// } > +/// > +/// #[pin_data] > +/// struct FenceContextData {} > +/// > +/// impl FenceContextData { > +/// fn new() -> impl PinInit<Self> { > +/// pin_init!(Self {}) > +/// } > +/// } > +/// > +/// impl FenceContextOps for FenceContextData { > +/// type FenceDataType = FenceData; > +/// } > +/// > +/// let fctx_data = FenceContextData::new(); > +/// > +/// let driver_name = CString::try_from_fmt(fmt!("dummy_driver"))?; > +/// let timeline_name = CString::try_from_fmt(fmt!("dummy_timeline"))?; > +/// > +/// let mut fctx = KBox::pin_init( > +/// FenceContext::new(0, driver_name, timeline_name, fctx_data), GFP_KERNEL)?; > +/// > +/// struct FenceData { > +/// data: CString, > +/// } > +/// > +/// let data = CString::try_from_fmt(fmt!("dummy_data"))?; > +/// let fence_data = FenceData { data }; > +/// > +/// let fence_alloc = fctx.new_fence_allocation(fence_data)?; > +/// let mut fence = fence_alloc.new_fence(); > +/// > +/// let cb_data = CallbackData { }; > +/// let waiting_fence = ARef::from(fence.as_fence()); > +/// let cb_reg = FenceCallbackRegistration::new(&waiting_fence, cb_data); > +/// let cb_reg = KBox::pin_init(cb_reg, GFP_KERNEL)?; > +/// > +/// // TODO signalling guards > +/// fence.signal(Ok(())); > +/// assert_eq!(waiting_fence.is_signaled(), true); > +/// > +/// Ok::<(), Error>(()) > +/// ``` > +pub struct DriverFence<'a, T: Send + Sync + FenceContextOps> { > + /// The actual content of the fence. Lives in a [`NonNull`] so that its > + /// memory can be managed independently. Valid until both the [`DriverFence`] > + /// and all associated [`Fence`]s have disappeared. > + data: NonNull<DriverFenceData<'a, T>>, > +} > + > +/// A pre-prepared DMA fence, carrying the user's data and the memory it and the > +/// fence reside in. Only useful for creating a [`DriverFence`]. Splitting > +/// allocation and full initialization is necessary because fences cannot be > +/// allocated dynamically in some circumstances (deadlock). > +pub struct DriverFenceAllocation<'a, T: Send + Sync + FenceContextOps> { > + /// The memory for the actual content of the fence. > + /// Handed over to a [`DriverFence`], or deallocated once the > + /// [`DriverFenceAllocation`] drops. > + data: KBox<DriverFenceData<'a, T>>, > + /// Pointer for the ops for the associated [`FenceContext`] > + ops: *const bindings::dma_fence_ops, > +} > + > +impl<'a, T: Send + Sync + FenceContextOps> DriverFenceAllocation<'a, T> { > + /// Create a new fence, consuming `data`. There's no `data`. > + /// > + /// This increments the sequence number in the associated [`FenceContext`]. > + pub fn new_fence(self) -> DriverFence<'a, T> { > + // We feed the C dma_fence backend a NULL for the spinlock so that it > + // uses per-fence locks automatically. > + let null_ptr: *mut bindings::spinlock = ptr::null_mut(); > + let seqno = self.data.fctx.next_seqno(); > + let fence_ptr = self.as_raw(); > + // SAFETY: `fence_ptr` has been created directly above. It will live > + // at least as long as `Self`. The same applies to `&Self::OPS`. > + unsafe { > + bindings::dma_fence_init(fence_ptr, self.ops, null_ptr, self.data.fctx.nr, seqno) > + }; > + > + self.data.fctx.nr_of_unsignaled_fences.fetch_add(1, Relaxed); > + > + // A `DriverFenceAllocation`'s purpose is to carry allocated memory, so that > + // `DriverFence`s can always be created without allocating. In this > + // method, ownership over that memory is transferred to the new > + // `DriverFence` and managed through refcounting. The C dma_fence > + // backend will ultimately free the memory once the refcount reaches 0. > + let ptr = KBox::into_raw(self.data); > + // SAFETY: `ptr` was just created validly directly above. > + let ptr = unsafe { NonNull::new_unchecked(ptr) }; > + > + DriverFence { data: ptr } > + } > + > + fn as_raw(&self) -> *mut bindings::dma_fence { > + self.data.inner.inner.get() > + } > +} > + > +impl<'a, T: Send + Sync + FenceContextOps> DriverFence<'a, T> { > + fn as_raw(&self) -> *mut bindings::dma_fence { > + // SAFETY: Valid because `self` is valid. > + let fence_data = unsafe { &*self.data.as_ptr() }; > + > + fence_data.inner.inner.get() > + } > + > + /// Create a [`DriverFence`] from a raw pointer to a [`bindings::dma_fence`]. > + /// > + /// # Safety > + /// > + /// `ptr` must be a valid pointer to a `dma_fence` that was obtained through > + /// a [`DriverFence`] with matching generic data for both fence and associated > + /// [`FenceContext`]. > + unsafe fn from_raw(ptr: *mut bindings::dma_fence) -> Self { > + let opaque_fence = Opaque::cast_from(ptr); > + > + // SAFETY: Safe due to the function's overall safety requirements. > + let fence_ptr = unsafe { container_of!(opaque_fence, Fence, inner) }; > + > + // DriverFenceData is repr(C) and a Fence is its first member. > + let fence_data_ptr = fence_ptr as *mut DriverFenceData<'a, T>; > + > + // SAFETY: `fence_data_ptr` was created validly above. > + let data = unsafe { NonNull::new_unchecked(fence_data_ptr) }; > + > + Self { data } > + } > + > + /// Return the underlying [`Fence`]. > + pub fn as_fence(&self) -> &Fence { > + // SAFETY: `self` is by definition still valid, and it cannot drop until > + // this new reference is gone. > + unsafe { Fence::from_raw(self.as_raw()) } > + } > + > + /// Signal the fence. This will invoke all registered callbacks. > + pub fn signal(self, res: Result) { > + let fence = self.as_fence().lock(); > + > + // SAFETY: `fence` is valid because `self` is valid. The lock must be > + // held, which we acquired directly above. > + if !unsafe { bindings::dma_fence_test_signaled_flag(*fence.deref()) } { These `*fence.deref()` are quite weird as consequence of `FenceGuard` design. If `FenceGuard` just derefs to `&Fence` then this can be `fence.as_raw()`. > + if let Err(err) = res { > + // SAFETY: `fence` is valid because `self` is valid. The fence > + // must not have been signaled yet, which we check directly above. > + unsafe { bindings::dma_fence_set_error(*fence.deref(), err.to_errno()) }; > + } > + // SAFETY: `fence` is valid because `self` is valid. The lock must > + // be held, which we acquired above. > + unsafe { bindings::dma_fence_signal_locked(*fence.deref()) }; > + } > + > + // SAFETY: `self.data` is valid because `self` is valid. > + let fctx = unsafe { self.data.as_ref().fctx }; > + let _ = fctx.nr_of_unsignaled_fences.fetch_sub(1, Relaxed); Drop impl of `self` here will neededlessly take lock again before checking it's signaled already and unlock. > + } > +} > + > +// SAFETY: Fences are literally designed to be shared between threads. > +unsafe impl<'a, T: Send + Sync + FenceContextOps> Send for DriverFence<'a, T> {} > +// SAFETY: Fences are literally designed to be shared between threads. > +unsafe impl<'a, T: Send + Sync + FenceContextOps> Sync for DriverFence<'a, T> {} > + > +impl<'a, T: Send + Sync + FenceContextOps> Deref for DriverFence<'a, T> { > + type Target = T::FenceDataType; > + > + fn deref(&self) -> &Self::Target { > + // SAFETY: Thanks to refcounting, `data` is always valid as long as `self` is. > + let data = unsafe { &*self.data.as_ptr() }; > + > + &data.data > + } > +} > + > +/// A borrow wrapper for [`DriverFence`]. Implements [`Deref`]. > +pub struct DriverFenceBorrow<'a, T: Send + Sync + FenceContextOps> { > + driver_fence: ManuallyDrop<DriverFence<'a, T>>, > + _lifetime: PhantomData<&'a T>, > +} > + > +impl<'a, T: Send + Sync + FenceContextOps> Deref for DriverFenceBorrow<'a, T> { > + type Target = DriverFence<'a, T>; > + > + fn deref(&self) -> &Self::Target { > + self.driver_fence.deref() > + } > +} > + > +// SAFETY: The Rust dma_fence abstractions are already designed around the inner > +// C `dma_fence`, which can serve safely as the identification point when being > +// owned by C. Moreover, safety is ensured by not dropping `DriverFence` and by > +// only allowing operations without side effects on the Borrowed type. > +unsafe impl<T: Send + Sync + FenceContextOps + 'static> ForeignOwnable for DriverFence<'_, T> { The `'static` shouldn't be needed here. > + type Borrowed<'a> > + = DriverFenceBorrow<'a, T> > + where > + Self: 'a; > + type BorrowedMut<'a> > + = DriverFenceBorrow<'a, T> > + where > + Self: 'a; > + > + const FOREIGN_ALIGN: usize = core::mem::align_of::<bindings::dma_fence>(); > + > + fn into_foreign(self) -> *mut c_void { > + let fence = self; > + > + let ptr = fence.as_raw(); > + > + // DriverFence must not drop. > + let _ = ManuallyDrop::new(fence); > + > + ptr.cast() > + } > + > + unsafe fn from_foreign(ptr: *mut c_void) -> Self { > + // SAFETY: Safe because the trait implementation only invokes this with > + // a valid `ptr`, associated to a `DriverFence` with matching generic data. > + unsafe { Self::from_raw(ptr.cast()) } > + } > + > + unsafe fn borrow<'a>(ptr: *mut c_void) -> Self::Borrowed<'a> > + where > + Self: 'a, > + { > + // SAFETY: The trait implementation ensures that `ptr` always resides > + // within a [`Fence`] within a [`DriverFenceData`]. > + let driver_fence = unsafe { Self::from_raw(ptr.cast()) }; > + > + let driver_fence = ManuallyDrop::new(driver_fence); > + > + DriverFenceBorrow { > + driver_fence, > + _lifetime: PhantomData, > + } > + } > + > + unsafe fn borrow_mut<'a>(ptr: *mut c_void) -> Self::BorrowedMut<'a> > + // FIXME: The bound below and the one above in `borrow` should actually be > + // unnecessary since the compiler should be able to completely derive all > + // necessary information automatically. There is currently a compiler bug > + // preventing that, though: > + // > + // https://github.com/rust-lang/rust/issues/155430. > + // > + // (Help to) fix the compiler bug and remove the bounds afterwards. > + where > + Self: 'a, > + { > + // SAFETY: The trait implementation ensures that `ptr` always resides > + // within a [`Fence`] within a [`DriverFenceData`]. > + let driver_fence = unsafe { Self::from_raw(ptr.cast()) }; > + > + let driver_fence = ManuallyDrop::new(driver_fence); > + > + DriverFenceBorrow { > + driver_fence, > + _lifetime: PhantomData, > + } > + } > +} > + > +impl<'a, T: Send + Sync + FenceContextOps> Drop for DriverFence<'a, T> { > + fn drop(&mut self) { > + let guard = self.as_fence().lock(); > + > + // Use dma_fence_test_signaled_flag() instead of > + // dma_fence_is_signaled_locked() because the C backend wants to get rid > + // of the latter. > + > + // SAFETY: `guard` is valid until the `call_rcu()` below. > + let signaled: bool = unsafe { bindings::dma_fence_test_signaled_flag(*guard.deref()) }; > + if !signaled { > + pr_err!("DriverFence drops unsignaled. Danger of memory corruption!\n"); > + // SAFETY: `guard` is valid until the `call_rcu()` below. The fence > + // must not have been signaled yet, which we check directly above. > + unsafe { bindings::dma_fence_set_error(*guard.deref(), ECANCELED.to_errno()) }; > + // SAFETY: `guard` is valid until the `call_rcu()` below. The lock > + // must be held, which we acquired above. > + unsafe { bindings::dma_fence_signal_locked(*guard.deref()) }; > + > + // SAFETY: `self.data` is valid because `self` is valid. > + let fctx = unsafe { self.data.as_ref().fctx }; > + let _ = fctx.nr_of_unsignaled_fences.fetch_sub(1, Relaxed); > + } > + drop(guard); > + > + // SAFETY: Valid because `self` is valid. > + let rcu_head_ptr = unsafe { &raw mut (*self.data.as_ptr()).rcu_head }; > + > + // `DriverFenceData` but could be accessed through some dma_fence > + // callbacks right now. Access is being revoked in principle above by > + // signalling the fence, but since the C backend does not guarantee > + // perfect full synchronization, we have to wait for one grace period to > + // ensure that all accessors of `DriverFenceData` (through the > + // dma_fence_ops accessible through a `Fence`) are gone. > + > + // SAFETY: `call_rcu()` is always safe to be called. `rcu_head_ptr` was > + // created validly above. The module must perform a `synchronize_rcu()` > + // or `rcu_barrier()` call to guard against module unload. > + unsafe { bindings::call_rcu(rcu_head_ptr, Some(drop_driver_fence_data::<T>)) }; I thought at some point it was mentioned that we want a fast path if !mem::needs_drop::<...>() { } ? > + } > +} > + > +// TODO: > +// The entire call_rcu() mechanism in the drop above and the code below would be > +// unnecessary if C's dma_fence_signal() could be reworked in a way that after it > +// ran, the caller knows that no fence_ops callbacks can be running anymore. > +// In other words, if the dma_fence backend would use its spinlock for full > +// synchronization. > +// > +// Then we could move the drop_in_place() and dma_fence_put() upwards into the > +// drop() implementation and call it a day. > + > +/// Finally really drop this `DriverFence<T>` > +/// > +/// # Safety > +/// > +/// `head` references the `rcu_head` field of an `DriverFenceData<T>`. All > +/// accessors to that `DriverFenceData<T>` must be gone by now. This must be > +/// ensured by signalling the associated `DriverFence<T>` and then waiting > +/// for a grace period until calling this function here. > +unsafe extern "C" fn drop_driver_fence_data<T: Send + Sync + FenceContextOps>( > + head: *mut bindings::callback_head, > +) { > + // SAFETY: Caller provides a pointer to the `rcu_head` field of a `DriverFenceData<C>`. > + let fence_data = unsafe { container_of!(head, DriverFenceData<'_, T>, rcu_head) }; > + > + // SAFETY: `fence_data` was created validly above. All the fence's data will > + // only drop below, but the raw pointer to the raw C `dma_fence` remains > + // valid because the reference count is only decremented at the end of the > + // function. > + let fence = unsafe { (*fence_data).inner.inner.get() }; > + > + // SAFETY: `fence_data` was created validly above. A grace period has passed. > + // All callbacks which might have had access to the `fctx` are gone now. > + unsafe { drop_in_place(&raw mut (*fence_data).fctx) }; fctx is just a reference, so this is a no-op. Best, Gary > + > + // SAFETY: `fence_data` was created validly above. The user has already > + // dropped the only conventional accessor to the user data, the `DriverFence`, > + // one grace period ago. All accessors are gone now. > + unsafe { drop_in_place(&raw mut (*fence_data).data) }; > + > + // The inner `Fence` explicitly does not get dropped because there may be > + // many more users / consumers, each holding their own reference. > + > + // SAFETY: Once a `DriverFence` is initialized, the inner `fence` is > + // valid and initialized. It is valid until the refcount drops > + // to 0, which can earliest happen once we drop the `DriverFence`'s reference > + // here. > + unsafe { bindings::dma_fence_put(fence) }; > + > + // The actual memory the data associated with a `DriverFence` lives in > + // gets freed by the C dma_fence backend once the fence's refcount reaches 0. > +} > diff --git a/rust/kernel/dma_buf/mod.rs b/rust/kernel/dma_buf/mod.rs > new file mode 100644 > index 000000000000..4764a828642e > --- /dev/null > +++ b/rust/kernel/dma_buf/mod.rs > @@ -0,0 +1,14 @@ > +// SPDX-License-Identifier: GPL-2.0 OR MIT > + > +//! DMA-buf subsystem abstractions. > + > +pub mod dma_fence; > + > +pub use self::dma_fence::{ > + DriverFence, > + Fence, > + FenceCallback, > + FenceCallbackRegistration, > + FenceContext, > + FenceContextOps, // > +}; > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > index 68f4d9a3425d..6221ebfe71df 100644 > --- a/rust/kernel/lib.rs > +++ b/rust/kernel/lib.rs > @@ -67,6 +67,7 @@ > pub mod device_id; > pub mod devres; > pub mod dma; > +pub mod dma_buf; > pub mod driver; > #[cfg(CONFIG_DRM = "y")] > pub mod drm;

1 day, 20 hours

Re: [PATCH v3 06/11] mm/cma: Allow dynamically creating CMA areas

by T.J. Mercier

On Thu, Aug 6, 2026 at 9:21 AM Thierry Reding <thierry.reding(a)kernel.org> wrote: > > On Wed, Jul 08, 2026 at 04:49:31PM -0700, T.J. Mercier wrote: > > On Wed, Jul 1, 2026 at 9:09 AM Thierry Reding <thierry.reding(a)kernel.org> wrote: > > > > > > From: Thierry Reding <treding(a)nvidia.com> > > > > > > There is no technical reason why there should be a limited number of CMA > > > regions, so extract some code into helpers and use them to create extra > > > functions (cma_create() and cma_free()) that allow creating and freeing, > > > respectively, CMA regions dynamically at runtime. > > > > > > The static array of CMA areas cannot be replaced by dynamically created > > > areas because for many of them, allocation must not fail and some cases > > > may need to initialize them before the slab allocator is even available. > > > To account for this, keep these "early" areas in a separate list and > > > track the dynamic areas in a separate list. > > Huh... going over this thread again I could've sworn that I had replied > to this before, but I can't find any evidence of any reply. > > > Hi, It looks like you'll also need to update the CMA dma-buf heap's > > add_cma_heaps init function so that it adds all the CMA areas, not > > just the early ones. > > I would actually rather not do that. The case that we need this for is > creating a special purpose dma-buf heap that's composed of multiple CMA > areas. These areas must not be used by anyone else. Exposing these to > the CMA dma-buf heap would allow anyone to allocate from individual > chunks and throw off the accounting that we need to do in order to keep > the protected memory from working correctly. > > Besides, given the dynamic nature of this means that the CMA dma-buf > heap might already have probed when these dynamic CMA areas are added. > So we would either need to have some way of notifying the CMA dma-buf > heap of newly created areas or we might end up with an incomplete set > of registered heaps. > > If you really insist, I could look into this, but I think it's actually > a nice feature that the CMA dma-buf heap only registers these "early" > areas and exposes them. > > Thierry No problem, thanks for responding. I see there is now discussion about subdividing a single CMA area for this, and that seems like it could end up simpler in terms of the CMA code for this series. So I'm supportive of that idea.

2 days, 6 hours

Re: [PATCH 3/5] drm/amdgpu/userq: bound the eviction fence rearm retry loop

by Christian König

On 8/10/26 18:13, Junrui Luo via B4 Relay wrote: > From: Junrui Luo <moonafterrain(a)outlook.com> > > amdgpu_userq_ensure_ev_fence() loops until the eviction fence is both > present and unsignaled. The only producer of such a fence is > amdgpu_evf_mgr_rearm(), which runs as the very last step of > amdgpu_userq_vm_validate(). Every failure point ahead of it - the > kzalloc() in the rearm itself, amdgpu_hmm_range_alloc(), the > ttm_bo_validate() calls, the GART binding of the wptr BOs - makes > amdgpu_userq_restore_worker() give up with only a drm_file_err(). > Nothing propagates that back, so the waiting thread reschedules the > worker and flushes it again, forever. > > Both flush_delayed_work() and mutex_lock() sleep in > TASK_UNINTERRUPTIBLE, so the looping task cannot be killed and the OOM > killer cannot reclaim it. An unprivileged render node client > reaches this from both AMDGPU_USERQ and AMDGPU_USERQ_SIGNAL. > > The eviction fence sequence number is already bumped by every > successful rearm, so use it as the loop's progress condition: if a > completed flush of the restore worker did not move it then no rearm > happened and retrying cannot help. Return -ENOMEM in that case and > let both callers report it to userspace. > > Fixes: a242a3e4b5be ("drm/amdgpu: simplify eviction fence suspend/resume") > Reported-by: Yuhao Jiang <danisjiang(a)gmail.com> > Assisted-by: Claude:claude-opus-5 > Cc: stable(a)vger.kernel.org > Signed-off-by: Junrui Luo <moonafterrain(a)outlook.com> Absolutely clear NAK! This functions needs to loop forever should the rearm worker fails to re-arm the fence. The only thing which could potentially get out of that is to kill the process or maybe that the eviction fence is signaled with an error. Regards, Christian. > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 21 +++++++++++++++++++-- > drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h | 4 ++-- > drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c | 10 +++++++++- > 3 files changed, 30 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c > index bec107216811..208b53ae5bd1 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c > @@ -448,12 +448,16 @@ static void amdgpu_userq_cleanup(struct amdgpu_usermode_queue *queue) > * Ensures that a valid and not yet signaled eviction fence is attached to the > * usermode queue before any queue operations proceed. If it is signalled, then > * rearm a new eviction fence. > + * > + * Returns 0 with @uq_mgr->userq_mutex held, or -ENOMEM with the mutex released > + * when the restore worker could not rearm the fence. > */ > -void > +int > amdgpu_userq_ensure_ev_fence(struct amdgpu_userq_mgr *uq_mgr, > struct amdgpu_eviction_fence_mgr *evf_mgr) > { > struct dma_fence *ev_fence; > + int seq, prev_seq = -1; > > retry: > /* Flush any pending resume work to create ev_fence */ > @@ -463,7 +467,16 @@ amdgpu_userq_ensure_ev_fence(struct amdgpu_userq_mgr *uq_mgr, > ev_fence = amdgpu_evf_mgr_get_fence(evf_mgr); > if (dma_fence_is_signaled(ev_fence)) { > dma_fence_put(ev_fence); > + seq = atomic_read(&evf_mgr->ev_fence_seq); > mutex_unlock(&uq_mgr->userq_mutex); > + /* > + * The sequence number is only bumped by a successful rearm, so > + * if the flush above ran the worker without moving it then the > + * restore failed and looping again would never terminate. > + */ > + if (seq == prev_seq) > + return -ENOMEM; > + prev_seq = seq; > /* > * Looks like there was no pending resume work, > * add one now to create a valid eviction fence > @@ -472,6 +485,8 @@ amdgpu_userq_ensure_ev_fence(struct amdgpu_userq_mgr *uq_mgr, > goto retry; > } > dma_fence_put(ev_fence); > + > + return 0; > } > > > @@ -747,7 +762,9 @@ amdgpu_userq_create(struct drm_file *filp, union drm_amdgpu_userq *args) > if (r) > goto clean_mqd; > > - amdgpu_userq_ensure_ev_fence(&fpriv->userq_mgr, &fpriv->evf_mgr); > + r = amdgpu_userq_ensure_ev_fence(&fpriv->userq_mgr, &fpriv->evf_mgr); > + if (r) > + goto erase_doorbell; > > /* don't map the queue if scheduling is halted */ > if (!adev->userq_halt_for_enforce_isolation || > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h > index 6412a7f7b6ef..c35909bf7ceb 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h > @@ -164,8 +164,8 @@ void amdgpu_userq_mgr_fini(struct amdgpu_userq_mgr *userq_mgr); > > void amdgpu_userq_evict(struct amdgpu_userq_mgr *uq_mgr); > > -void amdgpu_userq_ensure_ev_fence(struct amdgpu_userq_mgr *userq_mgr, > - struct amdgpu_eviction_fence_mgr *evf_mgr); > +int amdgpu_userq_ensure_ev_fence(struct amdgpu_userq_mgr *userq_mgr, > + struct amdgpu_eviction_fence_mgr *evf_mgr); > > u32 amdgpu_userq_get_supported_ip_mask(struct amdgpu_device *adev); > bool amdgpu_userq_enabled(struct drm_device *dev); > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c > index 7e80442ec3e5..1c287ce59736 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c > @@ -523,7 +523,15 @@ int amdgpu_userq_signal_ioctl(struct drm_device *dev, void *data, > goto put_queue; > > /* We are here means UQ is active, make sure the eviction fence is valid */ > - amdgpu_userq_ensure_ev_fence(&fpriv->userq_mgr, &fpriv->evf_mgr); > + r = amdgpu_userq_ensure_ev_fence(&fpriv->userq_mgr, &fpriv->evf_mgr); > + if (r) { > + /* The fence is not initialized yet, so unwind it by hand */ > + amdgpu_userq_fence_put_fence_drv_array(fence); > + amdgpu_userq_fence_driver_put(fence->fence_drv); > + kvfree(fence->fence_drv_array); > + kfree(fence); > + goto put_queue; > + } > > /* Create the new fence */ > amdgpu_userq_fence_init(queue, fence, wptr); >

2 days, 12 hours

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig