rust / dma_buf: Add abstractions for dma_fence

Changes since v4:
- Fix an uninitialized memory bug for FenceCbRegistration with ManuallyDrop.
- Return FenceCtx as impl PinInit
- Make FenceCtx return an impl PinInit<T, Error> (Danilo)
- Reformat some comments
- Adjust the docu for rcu_barrier(), so that it matches the C side's docu and our docu for rcu::synchronize_rcu().

Changes since v3:
- Add a FIXME for an encountered Rust compiler bug. (Gary)
- Add new Rust files also to DRM drivers & common infrastructure MAINTAINERS file. (Danilo)
- Reposition ECANCELED error code. (Miguel)
- Replace refcounted FenceCtx in DriverFenceData with a reference plus life time. (Boris)
- Re-add rcu_barrier() patch, since we now can use it for dropping the fence context. (Danilo)
- Add forgotten R-b from Alice, and Acks for MAINTAINERS from Christian and Sumit.

Changes since v2:
- Don't drop DriverFenceData as a whole, but only the members we really want to drop. Gives more robustness. (Gary).
- Break apart large pin_init_from_closure(). (Danilo, Onur)
- Remove rcu_barrier() and synchronize_rcu() from FenceCtx::drop(). FenceCtx might drop in atomic context, where you must not perform those operations. With the current way C dma_fence is designed, the driver must wait for a grace period manually until it unloads.
- Repair the DriverFenceBorrow implementation, properly injecting a life time into it. (Danilo)
- Fix memory layout bug for rcu_head. (Onur)
- Drop RCU patches, since this series doesn't need them anymore.

Changes since v1:
- Remove unnecessary mutable references (Alice)
- Split up unsafe comments where possible (Danilo)
- Remove PhantomData + implement FenceCtx ops trait (Boris)
- Consistently call FenceCtx generic data T. FenceDataType is derived from that. (Boris)
- Add abstractions for call_rcu() and synchronize_rcu() (Danilo)
- Add ECANCELED error code in Rust (Alice)
- Remove the rcu_barrier() from FenceCtx::drop() – because we now use call_rcu(), there can be no UAF access to the FenceCtx anymore. In any case, it is illegal to use either call_rcu() or synchronize_rcu() in FenceCtx::drop(), because our new drop_driver_fence_data() can run in atomic context and might put the last fence_ctx reference.
So we now only have to guard against module unload, which it seems either the driver or Rust driver-core / module unload infrastructure must solve.
- Minor formatting etc. changes
- Add C helpers to MAINTAINERS. (Danilo)
- Ensure that Fence::is_signaled() is fully synchronized, i.e., all callbacks really have run. See [1] and [2]. (Myself, Christian König)

Changes since the RFCs:
- Include support for ForeignOwnable for ARef, so that a Fence can be stuffed into an XArray et al. (Code by Danilo)
- Implement ForeignOwnable (with new borrow type) for DriverFence, so that it can be stuffed into an XArray.
- Include the rcu::RcuBox data type to defer dropping data with RCU (Cody by Alice)
- Port DmaFence to RcuBox to make UAF bugs through later, new dma_fence callbacks (backend_ops) impossible.
- Force users to pass their fence data in an RcuBox (or have it not need drop()) through a Sealed trait.
- Document the rules for the user's DriverFence::data's drop implementation very clearly (deadlock danger).
- rustfmt, Clippy.
- Various style suggestions, safety comments, etc. (Önur)
- Add __rust_helper prefix to helper functions. (Önur)

Changes in RFC v3:
- Omit JobQueue patches for now
- Completely redesign the memory layout: Instead of a Fence refcounting a DriverFence, both now live in the same allocation to allow for future support the dma_fence backend_ops callbacks which need to do container_of. (mostly Boris's feedback)
- Allow for pre-allocating fences to avoid deadlocks when submitting jobs to a GPU. (Boris)
- Simultaneously, allow for pre-preparing fence callback objects, so the driver can allocate them when it sees fit. (code largely stolen and inspired by Daniel).
- Signal fences on drop, ensure synchronization.
- Force users to set an error code when signalling.
- Write more documentation
- A ton of minor other changes.

[1] https://lore.kernel.org/dri-devel/[email protected]/
[2] https://lore.kernel.org/dri-devel/[email protected]/

Alright, so since the last RFCs did not reveal significant design issues, I decided to transition this series to a v1 and hope that we can get it upstream.

This now includes code for more common infrastructure that dma_fence needs, contributed by Danilo and Alice.

---

Old cover letter for RFC:

So, this is the spiritual successor of the first / second RFC [1]. v2 also contained code for drm::JobQueue, but mostly to show how the fence code would be used. JobQueue is under heavy rework right now, so I don't want to bother your eyes with it. The docstring examples should show how Rust fences are supposed to be used, though.

This v3 contains a huge amount of highly valuable feedback from a variety of people, notably Boris, but also from Alice, Gary and Danilo.

There are some TODOs open (a better trait for fence backend_ops and RCU support), but my hope is that this effort is now finally approaching its end.

I would greatly appreciate feedback and especially more information about what might be missing to make this usable, which is obviously where Daniel's and Boris's feedback will be valuable once more.

Please regard this patch just as what it's titled: an RFC, to discuss a bit more and to inform a broader community about what the current state is and where this is heading at.

Many regards,
Philipp

[1] https://lore.kernel.org/rust-for-linux/[email protected]/

Danilo Krummrich (1):
rust: types: implement ForeignOwnable for ARef<T>

Philipp Stanner (4):
rust: error: Add ECANCELED error code
rust: sync: Add abstraction for rcu_barrier()
rust: Add dma_fence abstractions
MAINTAINERS: Add entry for Rust dma-buf

MAINTAINERS                      |   5 +
rust/bindings/bindings_helper.h  |   1 +
rust/helpers/dma_fence.c         |  48 ++
rust/helpers/helpers.c           |   1 +
rust/kernel/dma_buf/dma_fence.rs | 894 +++++++++++++++++++++++++++++++
rust/kernel/dma_buf/mod.rs       |  14 +
rust/kernel/error.rs             |   1 +
rust/kernel/lib.rs               |   1 +
rust/kernel/sync/aref.rs         |  40 ++
rust/kernel/sync/rcu.rs          |  20 +
10 files changed, 1025 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

base-commit: a73a398a68ca9b9e5116a617562471f16b8310c4

Signature · 1 line

2.54.0

rust/kernel/dma_buf/dma_fence.rs new file +29 -0

+// 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 + FenceCtxOps> {
+    #[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 FenceCtx. Useful for obtaining name parameters.
+    fctx: &'a FenceCtx<T>,
+    /// The API user's data. This must either not need drop, or must delay its
+    /// drop by a grace period. It is essential that the data only performs
+    /// operations legal in atomic context in its [`Drop`] implementation.
+    #[pin]
+    data: T::FenceDataType,
+}
+
+pub struct DriverFence<'a, T: Send + Sync + FenceCtxOps> {
+    /// The actual content of the fence. Lives in a raw pointer 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>>,
+}
+

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 signalled once
- all fences must be signalled at some point
- fence error codes must only be set before signalling
- 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 signalled 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 signalled.

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 signalled by the creator with an error code. In case a DriverFence drops without being signalled beforehand, it is signalled 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 'signalled' 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 signalling 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 DmaFenceCtx (fence context) 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 signalling) 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.

Synchronization for backend_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.

Add abstractions for dma_fence in Rust.

Signed-off-by Philipp Stanner <[email protected]>

---

rust/bindings/bindings_helper.h  |   1 +
rust/helpers/dma_fence.c         |  48 ++
rust/helpers/helpers.c           |   1 +
rust/kernel/dma_buf/dma_fence.rs | 894 +++++++++++++++++++++++++++++++
rust/kernel/dma_buf/mod.rs       |  14 +
rust/kernel/lib.rs               |   1 +
6 files changed, 959 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

rust/bindings/bindings_helper.h +1 -0

index 1124785e210b..54b62d952e01 100644
@@ -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>

rust/helpers/dma_fence.c new file +48 -0

new file mode 100644
index 000000000000..0e08411098fa
@@ -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);
+}

rust/helpers/helpers.c +1 -0

index 998e31052e66..4ab8aa9da7e7 100644
@@ -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"

rust/kernel/dma_buf/dma_fence.rs new file +0 -0

new file mode 100644
index 000000000000..cbe8f447a603

rust/kernel/dma_buf/dma_fence.rs new file +86 -0

+
+use crate::{
+    alloc::AllocError,
+    bindings,
+    container_of,
+    error::to_result,
+    prelude::*,
+    types::ForeignOwnable,
+    types::Opaque,
+    warn_on, //
+};
+
+use core::{
+    marker::PhantomData,
+    mem::ManuallyDrop,
+    ops::Deref,
+    ptr,
+    ptr::{
+        drop_in_place,
+        NonNull, //
+    },
+    sync::atomic::{
+        AtomicU64,
+        Ordering, //
+    }, //
+};
+
+use kernel::{
+    str::CString,
+    sync::{
+        aref::{
+            ARef,
+            AlwaysRefCounted, //
+        },
+        rcu::rcu_barrier, //
+    }, //
+};
+
+/// VTable for dma_fence backend_ops callbacks.
+//
+// Mandatory dma_fence backend_ops are implemented implicitly through
+// [`FenceCtx`]. Additional ones shall get implemented on this trait, which then
+// shall be demanded for the fence context data.
+pub trait FenceCtxOps {
+    /// 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 FenceCtx<T: Send + Sync> {
+    /// The fence context number.
+    nr: u64,
+    /// The sequence number for the next fence created.
+    seqno: AtomicU64,
+    // 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 FenceCtx's fences belong to.
+    driver_name: CString,
+    /// The name of the timeline this FenceCtx's fences belong to.
+    timeline_name: CString,
+    /// The user's data.
+    #[pin]
+    data: T,
+}
+
+#[allow(unused_unsafe)]
+impl<'a, T: Send + Sync + FenceCtxOps> FenceCtx<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 `FenceCtx`.
+    pub fn new<E>(
+        driver_name: CString,
+        timeline_name: CString,

rust/kernel/dma_buf/dma_fence.rs new file +38 -0

+            driver_name,
+            timeline_name,
+            data <- data,
+        })
+    }
+
+    fn get_next_fence_seqno(&self) -> u64 {
+        self.seqno.fetch_add(1, Ordering::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.

rust/kernel/dma_buf/dma_fence.rs new file +117 -0

+        let seqno: u64 = self.get_next_fence_seqno();
+
+        // 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 fence_ptr = memory.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.nr, seqno) };
+
+        // 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(memory.data);
+        // SAFETY: `ptr` was just created validly directly above.
+        let ptr = unsafe { NonNull::new_unchecked(ptr) };
+
+        DriverFence { data: ptr }
+    }
+
+    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 [`FenceCtx`] from an associated [`bindings::dma_fence`].
+    ///
+    /// # Safety
+    ///
+    /// `ptr` must be a valid pointer to a 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) };
+
+        // DriverFenceData is repr(C) and a Fence is its first member.
+        let fence_data_ptr = fence_ptr as *mut DriverFenceData<'a, T>;
+
+        // SAFETY: Safe because of the safety comment directly above.
+        let fence_data = unsafe { &*fence_data_ptr };
+
+        fence_data.fctx
+    }
+}
+
+// FenceCtx'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.
+#[pinned_drop]
+impl<T: Send + Sync> PinnedDrop for FenceCtx<T> {
+    fn drop(self: Pin<&mut Self>) {
+        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 = ()> {
+    /// 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 {
+    fn from(err: CallbackError<T>) -> Self {
+        match err {
+            CallbackError::AlreadySignaled(_) => ENOENT,
+            CallbackError::Other(e) => e,
+        }
+    }
+}
+
+impl<T> From<AllocError> for CallbackError<T> {
+    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::FenceCb;
+///
+/// struct MyCallback {
+///     // Your callback state here

rust/kernel/dma_buf/dma_fence.rs new file +126 -0

+}
+
+/// A callback registration on a fence.
+///
+/// When this object is dropped, the callback is automatically removed if it
+/// hasn't been called yet.
+///
+/// # Invariants
+///
+/// If `callback` is `Some`, then `cb` is registered with the fence and the
+/// callback hasn't been invoked yet. If `None`, the callback has been invoked
+/// or the fence was already signaled when we tried to register.
+#[pin_data(PinnedDrop)]
+pub struct FenceCbRegistration<T: FenceCb + 'static> {
+    #[pin]
+    callback_foreign: Opaque<bindings::dma_fence_cb>,
+    callback: ManuallyDrop<T>,
+    fence: ARef<Fence>,
+}
+
+impl<T: FenceCb> FenceCbRegistration<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 `FenceCbRegistration`. The backend
+        // ensures synchronisation so whoever holds the registration object
+        // cannot drop it while this code is running. See `FenceCbRegistration::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
+    }
+}
+
+#[pinned_drop]
+impl<T: FenceCb> PinnedDrop for FenceCbRegistration<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
+        // 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());
+        }
+
+        unsafe { ManuallyDrop::<T>::drop(self.project().callback) };
+    }
+}

rust/kernel/dma_buf/dma_fence.rs new file +13 -0

+#[repr(transparent)]
+pub struct Fence {
+    /// The actual dma_fence passed to C.
+    inner: Opaque<bindings::dma_fence>,
+}
+
+// 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 signalled at the moment of the function call.

rust/kernel/dma_buf/dma_fence.rs new file +6 -0

+        let ret = unsafe { bindings::dma_fence_is_signaled(fence) };
+
+        // To guarantee that an API caller can 100% rely on the signalling being
+        // completed (i.e., all fence callbacks ran), we have to take the lock.
+        //
+        // The reason is that the C dma_fence backend currently does not carefully

rust/kernel/dma_buf/dma_fence.rs new file +48 -0

+        // SAFETY: `fence` is valid because `self` is valid. `flag_ptr` is
+        // merely a pointer to an integer, which lives as long as this function.
+        // The lock was taken above.
+        unsafe { bindings::dma_fence_unlock_irqrestore(fence, flag_ptr) };
+
+        ret
+    }
+
+    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 + FenceCtxOps> {

rust/kernel/dma_buf/dma_fence.rs new file +114 -0

+    /// The API user's data. This must either not need drop, or must delay its
+    /// drop by a grace period. 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.
+///
+/// Fences are always reference counted. The typical use case is that one side registers
+/// callbacks on the fence which will perform a certain action (such as queueing work) once the
+/// other side signals the fence.
+///
+/// # Examples
+///
+/// ```
+/// use kernel::dma_buf::{
+///     DriverFence,
+///     FenceCtx,
+///     FenceCtxOps,
+///     FenceCb,
+///     FenceCbRegistration, //
+/// };
+/// use kernel::str::CString;
+/// use kernel::sync::aref::ARef;
+/// use core::fmt::Display;
+///
+/// struct CallbackData { }
+///
+/// impl FenceCb for CallbackData {
+///     fn called(&mut self) {
+///         pr_info!("DmaFence callback executed.\n");
+///     }
+/// }
+///
+/// #[pin_data]
+/// struct FenceCtxData {}
+///
+/// impl FenceCtxData {
+///     fn new() -> impl PinInit<Self> {
+///         pin_init!(Self {})
+///     }
+/// }
+///
+/// impl FenceCtxOps for FenceCtxData {
+///     type FenceDataType = FenceData;
+/// }
+///
+/// let fctx_data = FenceCtxData::new();
+///
+/// let driver_name = CString::try_from_fmt(fmt!("dummy_driver"))?;
+/// let timeline_name = CString::try_from_fmt(fmt!("dummy_timeline"))?;
+///
+/// let fctx = KBox::pin_init(FenceCtx::new(driver_name, timeline_name, fctx_data), GFP_KERNEL)?;
+///
+/// struct FenceData {
+///     data: CString,
+/// }
+///
+/// let fence_data = CString::try_from_fmt(fmt!("dummy_data"))?;
+/// let fence_data = FenceData { data: fence_data };
+///
+/// let fence_alloc = fctx.new_fence_allocation(fence_data)?;
+/// let mut fence = fctx.new_fence(fence_alloc);
+///
+/// let cb_data = CallbackData { };
+/// let waiting_fence = ARef::from(fence.as_fence());
+/// let cb_reg = FenceCbRegistration::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 + FenceCtxOps> {
+    /// The actual content of the fence. Lives in a raw pointer 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 + FenceCtxOps> {
+    /// 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>>,
+}
+
+impl<'a, T: Send + Sync + FenceCtxOps> DriverFenceAllocation<'a, T> {
+    fn as_raw(&self) -> *mut bindings::dma_fence {
+        self.data.inner.inner.get()
+    }
+}
+
+impl<'a, T: Send + Sync + FenceCtxOps> 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

rust/kernel/dma_buf/dma_fence.rs new file +52 -0

+
+        // 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_raw();
+        let mut fence_flags: usize = 0;
+        let flag_ptr = &raw mut fence_flags;
+
+        // SAFETY: `fence` is valid because `self` is valid. `flag_ptr` is
+        // merely a pointer to an integer, which lives as long as this function.
+        unsafe { bindings::dma_fence_lock_irqsave(fence, flag_ptr) };
+        // 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) } {
+            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, 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) };
+        }
+        // SAFETY: `fence` is valid because `self` is valid. `flag_ptr` is
+        // merely a pointer to an integer, which lives as long as this function.
+        // The lock must be held, which we acquired above.
+        unsafe { bindings::dma_fence_unlock_irqrestore(fence, flag_ptr) };
+    }
+}
+
+// SAFETY: Fences are literally designed to be shared between threads.
+unsafe impl<'a, T: Send + Sync + FenceCtxOps> Send for DriverFence<'a, T> {}
+// SAFETY: Fences are literally designed to be shared between threads.
+unsafe impl<'a, T: Send + Sync + FenceCtxOps> Sync for DriverFence<'a, T> {}
+
+impl<'a, T: Send + Sync + FenceCtxOps> 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.

rust/kernel/dma_buf/dma_fence.rs new file +11 -0

+}
+
+impl<'a, T: Send + Sync + FenceCtxOps> 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

rust/kernel/dma_buf/dma_fence.rs new file +159 -0

+    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,
+            _life_time: 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,
+            _life_time: PhantomData,
+        }
+    }
+}
+
+impl<'a, T: Send + Sync + FenceCtxOps> Drop for DriverFence<'a, T> {
+    fn drop(&mut self) {
+        let fence = self.as_raw();
+        let mut fence_flags: usize = 0;
+        let flag_ptr = &raw mut fence_flags;
+
+        // SAFETY: `fence` is valid until the `call_rcu()` below. `flag_ptr` is
+        // merely a pointer to an integer, which lives as long as this function.
+        unsafe { bindings::dma_fence_lock_irqsave(fence, flag_ptr) };
+
+        // 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: `fence` is valid until the `call_rcu()` below.
+        let signaled: bool = unsafe { bindings::dma_fence_test_signaled_flag(fence) };
+        if warn_on!(!signaled) {
+            // SAFETY: `fence` 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(fence, ECANCELED.to_errno()) };
+            // SAFETY: `fence` is valid until the `call_rcu()` below. The lock must
+            // be held, which we acquired above.
+            unsafe { bindings::dma_fence_signal_locked(fence) };
+        }
+        // SAFETY: `fence` is valid until the `call_rcu()` below. `flag_ptr` is
+        // merely a pointer to an integer, which lives as long as this function.
+        // The lock must be held, which we acquired above.
+        unsafe { bindings::dma_fence_unlock_irqrestore(fence, flag_ptr) };
+
+        // 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>)) };
+    }
+}
+
+// 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 + FenceCtxOps>(
+    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) };
+
+    // 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.
+}

rust/kernel/dma_buf/mod.rs new file +14 -0

new file mode 100644
index 000000000000..fb353ce042ce
@@ -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,
+    FenceCb,
+    FenceCbRegistration,
+    FenceCtx,
+    FenceCtxOps, //
+};

rust/kernel/lib.rs +1 -0

index 9512af7156df..dc54d8af471f 100644
@@ -64,6 +64,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;

Signature · 1 line

2.54.0

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 signalled once
- all fences must be signalled at some point
- fence error codes must only be set before signalling
- 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 signalled 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 signalled.

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 signalled by the creator with an error code. In case a DriverFence drops without being signalled beforehand, it is signalled 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 'signalled' 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 signalling 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 DmaFenceCtx (fence context) 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 signalling) 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.

Synchronization for backend_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.

Add abstractions for dma_fence in Rust.

Signed-off-by Philipp Stanner <[email protected]>

---
 rust/bindings/bindings_helper.h  |   1 +
 rust/helpers/dma_fence.c         |  48 ++
 rust/helpers/helpers.c           |   1 +
 rust/kernel/dma_buf/dma_fence.rs | 894 +++++++++++++++++++++++++++++++
 rust/kernel/dma_buf/mod.rs       |  14 +
 rust/kernel/lib.rs               |   1 +
 6 files changed, 959 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

rust/bindings/bindings_helper.h +1 -0

index 1124785e210b..54b62d952e01 100644
@@ -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>

rust/helpers/dma_fence.c new file +48 -0

new file mode 100644
index 000000000000..0e08411098fa
@@ -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);
+}

rust/helpers/helpers.c +1 -0

index 998e31052e66..4ab8aa9da7e7 100644
@@ -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"

rust/kernel/dma_buf/dma_fence.rs new file +69 -0

new file mode 100644
index 000000000000..cbe8f447a603
@@ -0,0 +1,894 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (T) 2025, 2026 Red Hat Inc.:
+//   - Philipp Stanner <[email protected]>
+
+//! DriverFence support.
+//!
+//! Reference: <https://docs.kernel.org/driver-api/dma-buf.html#c.dma_fence>
+//!
+//! T 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,
+    warn_on, //
+};
+
+use core::{
+    marker::PhantomData,
+    mem::ManuallyDrop,
+    ops::Deref,
+    ptr,
+    ptr::{
+        drop_in_place,
+        NonNull, //
+    },
+    sync::atomic::{
+        AtomicU64,
+        Ordering, //
+    }, //
+};
+
+use kernel::{
+    str::CString,
+    sync::{
+        aref::{
+            ARef,
+            AlwaysRefCounted, //
+        },
+        rcu::rcu_barrier, //
+    }, //
+};
+
+/// VTable for dma_fence backend_ops callbacks.
+//
+// Mandatory dma_fence backend_ops are implemented implicitly through
+// [`FenceCtx`]. Additional ones shall get implemented on this trait, which then
+// shall be demanded for the fence context data.
+pub trait FenceCtxOps {
+    /// 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 FenceCtx<T: Send + Sync> {
+    /// The fence context number.
+    nr: u64,
+    /// The sequence number for the next fence created.
+    seqno: AtomicU64,
+    // 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()`.

rust/kernel/dma_buf/dma_fence.rs new file +20 -0

+impl<'a, T: Send + Sync + FenceCtxOps> FenceCtx<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 `FenceCtx`.
+    pub fn new<E>(
+        driver_name: CString,
+        timeline_name: CString,
+        data: impl PinInit<T, E>,
+    ) -> impl PinInit<Self, Error>
+    where

rust/kernel/dma_buf/dma_fence.rs new file +5 -0

+    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.
+}

rust/kernel/dma_buf/mod.rs new file +14 -0

new file mode 100644
index 000000000000..fb353ce042ce
@@ -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,
+    FenceCb,
+    FenceCbRegistration,
+    FenceCtx,
+    FenceCtxOps, //
+};

rust/kernel/lib.rs +1 -0

index 9512af7156df..dc54d8af471f 100644
@@ -64,6 +64,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;

Signature · 1 line

2.54.0

rcu_barrier() is a frequently used C function which is always safe to be called.

Add a safe abstraction for rcu_barrier().

Signed-off-by Philipp Stanner <[email protected]>

---
 rust/kernel/sync/rcu.rs | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

rust/kernel/sync/rcu.rs +20 -0

index a32bef6e490b..7031ca5d2473 100644
@@ -50,3 +50,23 @@ fn drop(&mut self) {
 pub fn read_lock() -> Guard {
     Guard::new()
 }
+
+/// Wait until all in-flight call_rcu() callbacks complete.
+///
+/// Note that this primitive does not necessarily wait for an RCU grace period
+/// to complete.  For example, if there are no RCU callbacks queued anywhere
+/// in the system, then rcu_barrier() is within its rights to return
+/// immediately, without waiting for anything, much less an RCU grace period.
+/// In fact, rcu_barrier() will normally not result in any RCU grace periods
+/// beyond those that were already destined to be executed.
+///
+/// In kernels built with CONFIG_RCU_LAZY=y, this function also hurries all
+/// pending lazy RCU callbacks.
+///
+/// Note that this is one of the RCU primitives which must not be called in
+/// atomic context.
+#[inline]
+pub fn rcu_barrier() {
+    // SAFETY: `rcu_barrier()` is always safe to be called. It just might wait for a grace period.
+    unsafe { bindings::rcu_barrier() };
+}

Signature · 1 line

2.54.0

The Rust dma_fence abstractions need the ECANCELED error code.

Add ECANCELED error code.

Signed-off-by Philipp Stanner <[email protected]>

---
 rust/kernel/error.rs | 1 +
 1 file changed, 1 insertion(+)

rust/kernel/error.rs +1 -0

index a56ba6309594..cb666f0d42ee 100644
@@ -67,6 +67,7 @@ macro_rules! declare_err {
     declare_err!(EOVERFLOW, "Value too large for defined data type.");
     declare_err!(EMSGSIZE, "Message too long.");
     declare_err!(ETIMEDOUT, "Connection timed out.");
+    declare_err!(ECANCELED, "Request has been canceled.");
     declare_err!(ERESTARTSYS, "Restart the system call.");
     declare_err!(ERESTARTNOINTR, "System call was interrupted by a signal and will be restarted.");
     declare_err!(ERESTARTNOHAND, "Restart if no handler.");

Signature · 1 line

2.54.0

rust/kernel/dma_buf/dma_fence.rs line 79

Quoted 3 lines · On Fri, 03 Jul 2026 09:31:40 +0200 Philipp Stanner <[email protected]> wrote:

rust/kernel/dma_buf/dma_fence.rs new file +3 -0

+}
+
+#[allow(unused_unsafe)]

Why is this needed?

Quoted 4 lines

+impl<'a, T: Send + Sync + FenceCtxOps> FenceCtx<T> {
+    // This can later be extended as a vtable in case other parties need support +    // for the more "exotic" callbacks.

Hi Phillipp,

Quoted 2 lines

rust/kernel/dma_buf/dma_fence.rs line 4

Quoted 9 lines

new file mode 100644
index 000000000000..cbe8f447a603

rust/kernel/dma_buf/dma_fence.rs new file +4 -0

@@ -0,0 +1,894 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (T) 2025, 2026 Red Hat Inc.:
+//   - Philipp Stanner <[email protected]>

SPDX-copyrightText?

rust/kernel/dma_buf/dma_fence.rs line 6

Quoted 3 lines

rust/kernel/dma_buf/dma_fence.rs new file +2 -0

+
+//! DriverFence support.

This contains, Fence, DriverFence and FenceContext, along with other miscellaneous. I would spend a bit more time fleshing this out slightly.

rust/kernel/dma_buf/dma_fence.rs line 10

Quoted 5 lines

rust/kernel/dma_buf/dma_fence.rs new file +4 -0

+//!
+//! Reference: <https://docs.kernel.org/driver-api/dma-buf.html#c.dma_fence>
+//!
+//! T header: [`include/linux/dma-fence.h`](srctree/include/linux/dma-fence.h)

T?

Quoted 3 lines

rust/kernel/dma_buf/dma_fence.rs new file +2 -0

+
+

Quoted 5 lines

+        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: AtomicU64::new(0),

Do we really need to force a 0 here? i.e.: can’t we take the initial seqno as an argument?

Quoted 8 lines

+            driver_name,
+            timeline_name,
+            data <- data,
+        })
+    }
+

Quoted 5 lines

+    /// Create a new fence, consuming data.
+    ///
+    /// The fence will increment the refcount of the fence context associated with this +    /// [FenceCtx].
+    pub fn new_fence(&self, memory: DriverFenceAllocation<'a, T>) -> DriverFence<'a, T> {

Do we ever check if the allocation was really made by “self” ? Apparently not :/

Quoted 2 lines

Quoted 1 line

+pub trait FenceCb: Send + 'static {

IMHO these acronyms make the code harder to read for no gain. I don’t think we have to carry that over from C.

FenceCb -> FenceCallback

Quoted 12 lines

+    /// 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 [FenceCbRegistration] can now be +    /// dropped.
+    fn called(&mut self);

I think “signaled” is more descriptive than “called”.

Quoted 4 lines

rust/kernel/dma_buf/dma_fence.rs new file +2 -0

+}
+

rust/kernel/dma_buf/dma_fence.rs line 428

Quoted 4 lines

rust/kernel/dma_buf/dma_fence.rs new file +4 -0

+
+/// The receiving counterpart of a [`DriverFence`], designed to register callbacks
+/// on, check the signalled state etc. A [`Fence`] cannot be signalled.
+/// A [`Fence`] is always refcounted.

I would explain this a tad better.

Quoted 18 lines

rust/kernel/dma_buf/dma_fence.rs new file +17 -0

+#[repr(transparent)]
+pub struct Fence {
+    /// The actual dma_fence passed to C.
+    inner: Opaque<bindings::dma_fence>,
+}
+
+// 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 signalled 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.

Quoted 6 lines

+    pub fn is_signaled(&self) -> bool {
+        let fence = self.as_raw();
+        let mut fence_flags: usize = 0;
+        let flag_ptr = &raw mut fence_flags;
+
+        // We shouuld not use dma_fence_is_signaled_locked() here, because

typo

Quoted 5 lines

+        // according to the C backend's recommendations, that function is problematic +        // and we should avoid calling that function with a lock held. +
+        // SAFETY: self is valid by definition. We take the spinlock above.

Where?

Quoted 13 lines

+        let ret = unsafe { bindings::dma_fence_is_signaled(fence) }; +
+        // To guarantee that an API caller can 100% rely on the signalling being +        // completed (i.e., all fence callbacks ran), 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.
+        //
+        // See commit c8a5d5ea3ba6a.

Quoted 4 lines

+
+        // SAFETY: fence is valid because self is valid. flag_ptr is +        // merely a pointer to an integer, which lives as long as this function. +        unsafe { bindings::dma_fence_lock_irqsave(fence, flag_ptr) };

Shouldn’t this be before the “is_signaled” ffi call? Or is this only about ensuring all callbacks have run? i.e.: is “ret” valid even though it was computed before taking the lock?

Quoted 2 lines

Quoted 13 lines

rust/kernel/dma_buf/dma_fence.rs new file +13 -0

+}
+// 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 + FenceCtxOps> {
+    #[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.

Quoted 3 lines

+    rcu_head: bindings::callback_head,
+    /// Reference to access the FenceCtx. Useful for obtaining name parameters. +    fctx: &'a FenceCtx<T>,

This creates a self-referential borrow in the JobQueue, since it holds both (a) the context to mint new seqnos from and (b) the xarray with fences which borrow from that same context. This issue does not exist with the Arc and we should really fix it before moving with this series.

Quoted 10 lines

+    /// The API user's data. This must either not need drop, or must delay its +    /// drop by a grace period. It is essential that the data only performs +    /// operations legal in atomic context in its [Drop] implementation. +    #[pin]
+    data: T::FenceDataType,

rust/kernel/dma_buf/dma_fence.rs new file +2 -0

+}
+

Quoted 4 lines

+
+        // DriverFenceData is repr(C) and a Fence is its first member.

+        let fence_data_ptr = fence_ptr as *mut DriverFenceData<'a, T>;

Without a “CAST:” keyword, I think this will trigger the linter?

Quoted 3 lines

+

lifetime is a single word.

Quoted 16 lines

rust/kernel/dma_buf/dma_fence.rs new file +15 -0

+}
+
+impl<'a, T: Send + Sync + FenceCtxOps> 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<'b, T: Send + Sync + FenceCtxOps + 'static> ForeignOwnable for DriverFence<'b, T> {

Quoted 6 lines

+    type Borrowed<'a>
+        = DriverFenceBorrow<'a, T>
+    where
+        Self: 'a;
+    type BorrowedMut<'a>
+        = DriverFenceBorrow<'a, T>

We should have a separate type for mutable borrows, IMHO.

Quoted 15 lines

Quoted 15 lines

rust/kernel/dma_buf/dma_fence.rs new file +13 -0

+}
+// 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 + FenceCtxOps> {
+    #[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.

Quoted 8 lines

rust/kernel/dma_buf/dma_fence.rs line 529

Quoted 3 lines

rust/kernel/dma_buf/dma_fence.rs new file +3 -0

+    rcu_head: bindings::callback_head,
+    /// Reference to access the FenceCtx. Useful for obtaining name parameters.
+    fctx: &'a FenceCtx<T>,

This creates a self-referential borrow in the JobQueue, since it holds both (a) the context to mint new seqnos from and (b) the xarray with fences which borrow from that same context. This issue does not exist with the Arc and we should really fix it before moving with this series.

Can you detail that a bit more. So your JobQueue version has a `data:
T` where T holds both the xarray and a jobqueue. The xarray contains fences that hold references to the FenceCtx. But shouldn't that be fine since the life time ensures that the drop order is correct?

btw we are recently coming up with a proposal on how JobQueue could store DriverFences for the driver. I want to present some RFC for that soonish.

Quoted 9 lines

rust/kernel/dma_buf/dma_fence.rs line 106

Quoted 6 lines

rust/kernel/dma_buf/dma_fence.rs new file +5 -0

+        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: AtomicU64::new(0),

Do we really need to force a 0 here? i.e.: can’t we take the initial seqno as an argument?

We could. What would that be useful for?

Quoted 9 lines

Quoted 9 lines

rust/kernel/dma_buf/dma_fence.rs new file +6 -0

+            driver_name,
+            timeline_name,
+            data <- data,
+        })
+    }
+

[…]

Quoted 7 lines

rust/kernel/dma_buf/dma_fence.rs line 154

Quoted 5 lines

rust/kernel/dma_buf/dma_fence.rs new file +5 -0

+    /// Create a new fence, consuming `data`.
+    ///
+    /// The fence will increment the refcount of the fence context associated with this
+    /// [`FenceCtx`].
+    pub fn new_fence(&self, memory: DriverFenceAllocation<'a, T>) -> DriverFence<'a, T> {

Do we ever check if the allocation was really made by “self” ? Apparently not :/

Hm, no, we don't.

For the most part that's irrelevant, since all critical components then only get set in new_fence(). Correct typization is enforced through T.

The notable exception is the fence_ctx reference itself.

What should we do about it?

We could keep the fctx field as a MaybeUninit and set it later. Or we check through the fctx identifier number whether it's the correct one in new_fence(), but then new_fence() could fail with some error, and it's probably better to have it be completely fail-free.

Quoted 3 lines

Quoted 3 lines

[…]

Quoted 2 lines

Quoted 1 line

+pub trait FenceCb: Send + 'static {

Quoted 4 lines

IMHO these acronyms make the code harder to read for no gain. I don’t think we have to carry that over from C.

FenceCb -> FenceCallback

No principle objections from me. But do you believe *all* of them are bad? I really like let fctx = … for instance.

Quoted 15 lines

Quoted 14 lines

rust/kernel/dma_buf/dma_fence.rs new file +11 -0

+    /// 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 [`FenceCbRegistration`] can now be
+    /// dropped.
+    fn called(&mut self);

Quoted 1 line

I think “signaled” is more descriptive than “called”.

Here I disagree. A callback does not get signaled. It gets called once the fence gets signaled.

Quoted 5 lines

Quoted 5 lines

(quoted diff) +2 -0

+}
+

[…]

Quoted 6 lines

rust/kernel/dma_buf/dma_fence.rs line 428

Quoted 4 lines

rust/kernel/dma_buf/dma_fence.rs new file +4 -0

+
+/// The receiving counterpart of a [`DriverFence`], designed to register callbacks
+/// on, check the signalled state etc. A [`Fence`] cannot be signalled.
+/// A [`Fence`] is always refcounted.

I would explain this a tad better.

What exactly? The refcounting? The dualism between DriverFence and Fence? :)

Quoted 19 lines

Quoted 19 lines

rust/kernel/dma_buf/dma_fence.rs new file +17 -0

+#[repr(transparent)]
+pub struct Fence {
+    /// The actual dma_fence passed to C.
+    inner: Opaque<bindings::dma_fence>,
+}
+
+// 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 signalled 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.

Quoted 8 lines

rust/kernel/dma_buf/dma_fence.rs line 451

Quoted 6 lines

rust/kernel/dma_buf/dma_fence.rs new file +6 -0

+    pub fn is_signaled(&self) -> bool {
+        let fence = self.as_raw();
+        let mut fence_flags: usize = 0;
+        let flag_ptr = &raw mut fence_flags;
+
+        // We shouuld not use `dma_fence_is_signaled_locked()` here, because

typo

ACK.

Quoted 7 lines

rust/kernel/dma_buf/dma_fence.rs line 455

Quoted 5 lines

rust/kernel/dma_buf/dma_fence.rs new file +4 -0

+        // according to the C backend's recommendations, that function is problematic
+        // and we should avoid calling that function with a lock held.
+
+        // SAFETY: `self` is valid by definition. We take the spinlock above.

Where?

The safety comment is wrong / outdated. See below.

Quoted 13 lines

Quoted 13 lines

rust/kernel/dma_buf/dma_fence.rs new file +12 -0

+        let ret = unsafe { bindings::dma_fence_is_signaled(fence) };
+
+        // To guarantee that an API caller can 100% rely on the signalling being
+        // completed (i.e., all fence callbacks ran), 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.
+        //
+        // See commit c8a5d5ea3ba6a.

Quoted 8 lines

rust/kernel/dma_buf/dma_fence.rs line 471

Quoted 4 lines

rust/kernel/dma_buf/dma_fence.rs new file +4 -0

+
+        // SAFETY: `fence` is valid because `self` is valid. `flag_ptr` is
+        // merely a pointer to an integer, which lives as long as this function.
+        unsafe { bindings::dma_fence_lock_irqsave(fence, flag_ptr) };

Shouldn’t this be before the “is_signaled” ffi call? Or is this only about ensuring all callbacks have run? i.e.: is “ret” valid even though it was computed before taking the lock?

OK, this is where it gets ugly.

So during the last weeks I've been struggling to get the C backend into better shape. One issue from my POV is that the C dma_fence spinlock does not protect the fence state; there is insistence that the lock shall only protect the callback list.

The function dma_fence_is_signaled() has an unlocked fast path check:

https://elixir.bootlin.com/linux/v7.2-rc3/source/include/linux/dma-fence.h#L551

whereas setting of that bit is done under lock-protection:

https://elixir.bootlin.com/linux/v7.2-rc3/source/drivers/dma-buf/dma-fence.c#L362

This can lead to funny races like in the commit mentioned in the comment block above (c8a5d5ea3ba6a).

And it also leads to weird hacks like this:

https://elixir.bootlin.com/linux/v7.2-rc3/source/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c#L2775

Now, in principle I agree with you that a pattern like this:

dma_fence_lock_irqsave(…);
let signaled = dma_fence_is_signaled_locked(…);
dma_fence_unlock_irqrestore(…);

would be better.

However, lengthy discussions with Christian seem to settle at the point where Christian sees the very strict requirement of never calling fence callbacks under lock protection, and where he views dma_fence_is_signaled_locked() as a broken function that should be removed.

He's currently working on removing all bits where fence callbacks are invoked under lock protection:

https://lore.kernel.org/dri-devel/[email protected]/

There's been a ton of discussions and proposals about that in recent weeks

https://lore.kernel.org/dri-devel/[email protected]/
https://lore.kernel.org/dri-devel/[email protected]/

So tl;dr: The weird code you're commenting on above ensures that

a) the fence->ops->is_signaled() callback is not called under lock protection and
b) taking and releasing the lock guarantees that all callbacks are really finished, i.e. they have run.

(I continue to believe that setting the bit under lock protection and reading it without lock is fundamentally broken and needs to be fixed, but fixes are being rejected because of claimed performance regressions years ago when this was tried, because checking the bit is some sort of fast path check for.. parties that spin on dma_fence_is_signaled() ??)

Quoted 3 lines

Quoted 3 lines

[…]

Quoted 10 lines

Quoted 10 lines

rust/kernel/dma_buf/dma_fence.rs new file +7 -0

+    /// The API user's data. This must either not need drop, or must delay its
+    /// drop by a grace period. It is essential that the data only performs
+    /// operations legal in atomic context in its [`Drop`] implementation.
+    #[pin]
+    data: T::FenceDataType,
+}
+

[…]

Quoted 3 lines

Quoted 2 lines

(quoted diff) +2 -0

+
+        // DriverFenceData is repr(C) and a Fence is its first member.

Quoted 4 lines

Quoted 1 line

+ let fence_data_ptr = fence_ptr as *mut DriverFenceData<'a, T>;

Without a “CAST:” keyword, I think this will trigger the linter?

Didn't see a complaint from clippy nor compiler.

Quoted 4 lines

Quoted 4 lines

+

[…]

Quoted 2 lines

lifetime is a single word.

ACK.

Quoted 16 lines

Quoted 16 lines

rust/kernel/dma_buf/dma_fence.rs new file +15 -0

+}
+
+impl<'a, T: Send + Sync + FenceCtxOps> 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<'b, T: Send + Sync + FenceCtxOps + 'static> ForeignOwnable for DriverFence<'b, T> {

Quoted 8 lines

rust/kernel/dma_buf/dma_fence.rs line 735

Quoted 6 lines

rust/kernel/dma_buf/dma_fence.rs new file +6 -0

+    type Borrowed<'a>
+        = DriverFenceBorrow<'a, T>
+    where
+        Self: 'a;
+    type BorrowedMut<'a>
+        = DriverFenceBorrow<'a, T>

We should have a separate type for mutable borrows, IMHO.

No hard objections. But because it's convention, or because you see other advantages?

Thanks

P.

Quoted 1 line

Quoted 10 lines

Quoted 9 lines

Quoted 6 lines

+        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: AtomicU64::new(0),

Do we really need to force a 0 here? i.e.: can’t we take the initial seqno as an argument?

Quoted 1 line

We could. What would that be useful for?

On Mali, the hardware syncobj starts at 0. If you see a 0, is this the default state, or should you signal seqno 0?

This problem goes away if we can have seqnos starting at a custom value, like
1. It seems like the C machinery also special-cases 0 in a few other places too.

Quoted 7 lines

Hm, no, we don't.

For the most part that's irrelevant, since all critical components then only get set in new_fence(). Correct typization is enforced through T.

Quoted 8 lines

The notable exception is the fence_ctx reference itself.

What should we do about it?

We could keep the fctx field as a MaybeUninit and set it later. Or we check through the fctx identifier number whether it's the correct one in new_fence(), but then new_fence() could fail with some error, and it's probably better to have it be completely fail-free.

Agree about the fail-free part.

The problem I see here is that new_fence() will use "seqno" and "nr" from whatever context called new_fence(), but DriverFenceAllocation has some other (possibly unrelated) context as its DriverFenceData::fctx.

The lifetimes are apparently broken too, because 'a is the lifetime of the context where new_fence_allocation was called, meaning that the context that actually called new_fence() can drop, even though it provided the state for dma_fence_init().

I guess this can be solved by moving new_fence() to impl DriverFenceAllocation? That already has a context, and most importantly, the right context.

Quoted 1 line

Quoted 9 lines

Quoted 6 lines

rust/kernel/dma_buf/dma_fence.rs line 428

Quoted 4 lines

rust/kernel/dma_buf/dma_fence.rs new file +4 -0

+
+/// The receiving counterpart of a [`DriverFence`], designed to register callbacks
+/// on, check the signalled state etc. A [`Fence`] cannot be signalled.
+/// A [`Fence`] is always refcounted.

I would explain this a tad better.

What exactly? The refcounting? The dualism between DriverFence and Fence? :)

For example, you say “a Fence cannot be signaled”. A person seeing this code for the first time might ask why. Specially if they start by reading the docs for Fence first.

I think explaining a bit more about the DriverFence/Fence/refcounting as you said is already enough to settle it.

Quoted 1 line

Quoted 1 line

Quoted 79 lines

Quoted 12 lines

Quoted 12 lines

+        let ret = unsafe { bindings::dma_fence_is_signaled(fence) }; +
+        // To guarantee that an API caller can 100% rely on the signalling being +        // completed (i.e., all fence callbacks ran), 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.
+        //
+        // See commit c8a5d5ea3ba6a.

Quoted 8 lines

Quoted 4 lines

+
+        // SAFETY: fence is valid because self is valid. flag_ptr is +        // merely a pointer to an integer, which lives as long as this function. +        unsafe { bindings::dma_fence_lock_irqsave(fence, flag_ptr) };

Shouldn’t this be before the “is_signaled” ffi call? Or is this only about ensuring all callbacks have run? i.e.: is “ret” valid even though it was computed before taking the lock?

OK, this is where it gets ugly.

So during the last weeks I've been struggling to get the C backend into better shape. One issue from my POV is that the C dma_fence spinlock does not protect the fence state; there is insistence that the lock shall only protect the callback list.

The function dma_fence_is_signaled() has an unlocked fast path check:

https://elixir.bootlin.com/linux/v7.2-rc3/source/include/linux/dma-fence.h#L551

whereas setting of that bit is done under lock-protection:

https://elixir.bootlin.com/linux/v7.2-rc3/source/drivers/dma-buf/dma-fence.c#L362

This can lead to funny races like in the commit mentioned in the comment block above (c8a5d5ea3ba6a).

And it also leads to weird hacks like this:

https://elixir.bootlin.com/linux/v7.2-rc3/source/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c#L2775

Now, in principle I agree with you that a pattern like this:

dma_fence_lock_irqsave(…);
let signaled = dma_fence_is_signaled_locked(…);
dma_fence_unlock_irqrestore(…);

would be better.

However, lengthy discussions with Christian seem to settle at the point where Christian sees the very strict requirement of never calling fence callbacks under lock protection, and where he views dma_fence_is_signaled_locked() as a broken function that should be removed.

He's currently working on removing all bits where fence callbacks are invoked under lock protection:

https://lore.kernel.org/dri-devel/[email protected]/

There's been a ton of discussions and proposals about that in recent weeks

https://lore.kernel.org/dri-devel/[email protected]/
https://lore.kernel.org/dri-devel/[email protected]/

So tl;dr: The weird code you're commenting on above ensures that

a) the fence->ops->is_signaled() callback is not called under lock protection and
b) taking and releasing the lock guarantees that all callbacks are really finished, i.e. they have run.

Quoted 5 lines

(I continue to believe that setting the bit under lock protection and reading it without lock is fundamentally broken and needs to be fixed, but fixes are being rejected because of claimed performance regressions years ago when this was tried, because checking the bit is some sort of fast path check for.. parties that spin on dma_fence_is_signaled() ??)

I see, there is a lot more context on this then. Can you merely add a comment saying it’s ok to call dma_fence_is_signaled() without the locks? Otherwise people might try to “fix” this down the line...

Quoted 24 lines

Quoted 4 lines

Quoted 4 lines

[…]

Quoted 10 lines

Quoted 10 lines

+    /// The API user's data. This must either not need drop, or must delay its +    /// drop by a grace period. It is essential that the data only performs +    /// operations legal in atomic context in its [Drop] implementation. +    #[pin]
+    data: T::FenceDataType,

(quoted diff) +2 -0

+}
+

[…]

Quoted 5 lines

Quoted 4 lines

+
+        // DriverFenceData is repr(C) and a Fence is its first member.

+        let fence_data_ptr = fence_ptr as *mut DriverFenceData<'a, T>;

Quoted 4 lines

Quoted 2 lines

Without a “CAST:” keyword, I think this will trigger the linter?

Didn't see a complaint from clippy nor compiler.

I recommend the CAST thing anyways. It’s being adopted in other parts of the kernel crate.