Skip to contentSkip to conversation

rust / dma_buf: Add abstractions for dma_fence

PATCH Superseded Philipp Stanner 2 participants 9 messages Jun 22 – Jun 23

Subsystems (6)

v4 of 11 Superseded Latest is v11 →

lore
Share & tools
lore

Philipp Stanner

author maintainer
#0 lore raw

v4 cover rust / dma_buf: Add abstractions for dma_fence

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 | 870 +++++++++++++++++++++++++++++++
 rust/kernel/dma_buf/mod.rs       |  14 +
 rust/kernel/error.rs             |   1 +
 rust/kernel/lib.rs               |   1 +
 rust/kernel/sync/aref.rs         |  39 ++
 rust/kernel/sync/rcu.rs          |   6 +
 10 files changed, 986 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: 848bf57e98e1678ce7a49eb4e0bf0502da95dc07

Signature · 1 line
2.54.0
author maintainer
#1 lore raw

PATCH v4 1/5 rust: types: implement ForeignOwnable for ARef<T>

From: Danilo Krummrich <[email protected]>

Implement ForeignOwnable for ARef<T>, making it possible for C code to own an ARef<T>.

Since ARef represents shared ownership, BorrowedMut is &T rather than &mut T, matching the semantics of the underlying reference-counted type.

---
 rust/kernel/sync/aref.rs | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)
rust/kernel/sync/aref.rs +39 -0
index 9989f56d0605..82907383c44b 100644
@@ -17,6 +17,10 @@
 //! [`Arc`]: crate::sync::Arc
 //! [`Arc<T>`]: crate::sync::Arc
 
+use crate::{
+    prelude::*,
+    types::ForeignOwnable, //
+};
 use core::{marker::PhantomData, mem::ManuallyDrop, ops::Deref, ptr::NonNull};
 
 /// Types that are _always_ reference counted.
@@ -183,6 +187,41 @@ fn eq(&self, other: &ARef<U>) -> bool {
 }
 impl<T: AlwaysRefCounted + Eq> Eq for ARef<T> {}
 
+// SAFETY: `into_foreign` returns a pointer from `NonNull::as_ptr`, so it's non-null. The
+// `ARef` invariant guarantees that `ptr` points to a valid `T`, so it's aligned to `T`.
+unsafe impl<T: AlwaysRefCounted + 'static> ForeignOwnable for ARef<T> {
+    const FOREIGN_ALIGN: usize = core::mem::align_of::<T>();
+
+    type Borrowed<'a> = &'a T;
+    type BorrowedMut<'a> = &'a T;
+
+    fn into_foreign(self) -> *mut c_void {
+        ARef::into_raw(self).as_ptr().cast()
+    }
+
+    unsafe fn from_foreign(ptr: *mut c_void) -> Self {
+        // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous
+        // call to `Self::into_foreign`.
+        let ptr = unsafe { NonNull::new_unchecked(ptr.cast()) };
+
+        // SAFETY: `ptr` came from `into_foreign`, which consumed an `ARef` without decrementing
+        // the refcount, so we can transfer the ownership to the new `ARef`.
+        unsafe { ARef::from_raw(ptr) }
+    }
+
+    unsafe fn borrow<'a>(ptr: *mut c_void) -> &'a T {
+        // SAFETY: The safety requirements of this method ensure that the object remains alive and
+        // immutable for the duration of 'a.
+        unsafe { &*ptr.cast() }
+    }
+
+    unsafe fn borrow_mut<'a>(ptr: *mut c_void) -> &'a T {
+        // SAFETY: The safety requirements for `borrow_mut` are a superset of the safety
+        // requirements for `borrow`.
+        unsafe { <Self as ForeignOwnable>::borrow(ptr) }
+    }
+}
+
 impl<T, U> PartialEq<&'_ U> for ARef<T>
 where
     T: AlwaysRefCounted + PartialEq<U>,
Signature · 1 line
2.54.0
#2 lore raw

Re: [PATCH v4 1/5] rust: types: implement ForeignOwnable for ARef<T>

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] Small functions in the `ForeignOwnable` trait implementation for `ARef<T>` are missing `#[inline]`…

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Small functions in the ForeignOwnable trait implementation for ARef<T> are missing #[inline] annotations. --

commit 5f304b2260799e0fe0be8848ec9e82f882fdc711
Author: Danilo Krummrich <[email protected]>

rust: types: implement ForeignOwnable for ARef<T>

This commit implements ForeignOwnable for ARef<T>, making it possible for C code to own an ARef<T>. Since ARef represents shared ownership, BorrowedMut is &T rather than &mut T, matching the semantics of the underlying reference-counted type.

Quoted 4 lines
rust/kernel/sync/aref.rs +0 -0
index 9989f56d06052..82907383c44b5 100644

[ ... ]

Quoted 5 lines
rust/kernel/sync/aref.rs +1 -0
@@ -183,6 +187,41 @@ fn eq(&self, other: &ARef<U>) -> bool {
 }
 impl<T: AlwaysRefCounted + Eq> Eq for ARef<T> {}
 
+// SAFETY: `into_foreign` returns a pointer from `NonNull::as_ptr`, so it's non-null. The

rust/kernel/sync/aref.rs fn eq(&self, other: &ARef<U>) -> bool { · line 200

Quoted 10 lines
rust/kernel/sync/aref.rs +10 -0
+// `ARef` invariant guarantees that `ptr` points to a valid `T`, so it's aligned to `T`.
+unsafe impl<T: AlwaysRefCounted + 'static> ForeignOwnable for ARef<T> {
+    const FOREIGN_ALIGN: usize = core::mem::align_of::<T>();
+
+    type Borrowed<'a> = &'a T;
+    type BorrowedMut<'a> = &'a T;
+
+    fn into_foreign(self) -> *mut c_void {
+        ARef::into_raw(self).as_ptr().cast()
+    }

[Severity: Low]
This isn't a bug, but according to the Rust subsystem guidelines, should this small forwarding function in the abstraction be annotated with #[inline]?

rust/kernel/sync/aref.rs fn eq(&self, other: &ARef<U>) -> bool { · line 210

Quoted 10 lines
rust/kernel/sync/aref.rs +10 -0
+
+    unsafe fn from_foreign(ptr: *mut c_void) -> Self {
+        // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous
+        // call to `Self::into_foreign`.
+        let ptr = unsafe { NonNull::new_unchecked(ptr.cast()) };
+
+        // SAFETY: `ptr` came from `into_foreign`, which consumed an `ARef` without decrementing
+        // the refcount, so we can transfer the ownership to the new `ARef`.
+        unsafe { ARef::from_raw(ptr) }
+    }

[Severity: Low]
This isn't a bug, but should from_foreign() also be annotated with #[inline] since it is a small forwarding function?

rust/kernel/sync/aref.rs fn eq(&self, other: &ARef<U>) -> bool { · line 216

Quoted 6 lines
rust/kernel/sync/aref.rs +6 -0
+
+    unsafe fn borrow<'a>(ptr: *mut c_void) -> &'a T {
+        // SAFETY: The safety requirements of this method ensure that the object remains alive and
+        // immutable for the duration of 'a.
+        unsafe { &*ptr.cast() }
+    }

[Severity: Low]
This isn't a bug, but should borrow() also be annotated with #[inline]?

rust/kernel/sync/aref.rs fn eq(&self, other: &ARef<U>) -> bool { · line 222

Quoted 6 lines
rust/kernel/sync/aref.rs +6 -0
+
+    unsafe fn borrow_mut<'a>(ptr: *mut c_void) -> &'a T {
+        // SAFETY: The safety requirements for `borrow_mut` are a superset of the safety
+        // requirements for `borrow`.
+        unsafe { <Self as ForeignOwnable>::borrow(ptr) }
+    }

[Severity: Low]
This isn't a bug, but should borrow_mut() also be annotated with #[inline]?

Quoted 1 line
rust/kernel/sync/aref.rs +1 -0
+}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
author maintainer
#3 lore raw

PATCH v4 2/5 rust: error: Add ECANCELED error code

In reply to Philipp Stanner

The Rust dma_fence abstractions need the ECANCELED error code.

Add ECANCELED error code.

---
 rust/kernel/error.rs | 1 +
 1 file changed, 1 insertion(+)
rust/kernel/error.rs +1 -0
index 05cf869ac090..e376c60e21ed 100644
@@ -69,6 +69,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
author maintainer
#4 lore raw

PATCH v4 3/5 rust: sync: Add abstraction for rcu_barrier()

In reply to Philipp Stanner

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

Add a safe abstraction for rcu_barrier().

---
 rust/kernel/sync/rcu.rs | 6 ++++++
 1 file changed, 6 insertions(+)
rust/kernel/sync/rcu.rs +6 -0
index a32bef6e490b..9196feb62d6d 100644
@@ -50,3 +50,9 @@ fn drop(&mut self) {
 pub fn read_lock() -> Guard {
     Guard::new()
 }
+
+/// Wait for all pending [`bindings::call_rcu()`] callbacks, if there are any.
+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
#5 lore raw

Re: [PATCH v4 3/5] rust: sync: Add abstraction for rcu_barrier()

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] The `rcu_barrier()` abstraction function is missing the mandatory `#[inline]` attribute. --

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The rcu_barrier() abstraction function is missing the mandatory #[inline] attribute. --

commit 9b4d6fad89b434f3d9d6204b87f3b0eed4df4bb6
Author: Philipp Stanner <[email protected]>

rust: sync: Add abstraction for rcu_barrier()

This commit adds a safe Rust abstraction for the C function rcu_barrier(), wrapping the underlying C binding so it can be called safely from Rust.

rust/kernel/sync/rcu.rs fn drop(&mut self) { · line 55

Quoted 11 lines
rust/kernel/sync/rcu.rs +3 -0
index a32bef6e490b0..9196feb62d6de 100644
@@ -50,3 +50,9 @@ fn drop(&mut self) {
 pub fn read_lock() -> Guard {
     Guard::new()
 }
+
+/// Wait for all pending [`bindings::call_rcu()`] callbacks, if there are any.
+pub fn rcu_barrier() {

[Severity: Low]
This isn't a bug, but according to the Rust subsystem guidelines, functions in abstractions that are small or simply forward to a C binding call should be annotated with #[inline].

Could we add the #[inline] attribute to rcu_barrier() since it just forwards to the C binding?

Quoted 3 lines
rust/kernel/sync/rcu.rs +3 -0
+    // SAFETY: `rcu_barrier()` is always safe to be called. It just might wait for a grace period.
+    unsafe { bindings::rcu_barrier() };
+}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
author maintainer
#6 lore raw

PATCH v4 4/5 rust: Add dma_fence abstractions

In reply to Philipp Stanner

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.

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

48 lines not on this page · raw message

rust/helpers/helpers.c +1 -0
index 4488a87223b9..8e37064cb836 100644
@@ -57,6 +57,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 +870 -0

870 lines not on this page · raw message

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 b72b2fbe046d..a05ccaa7598c 100644
@@ -63,6 +63,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
#7 lore raw

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

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [Critical] Soundness / Use-After-Free due to lifetime erasure in `ForeignOwnable` implementation for…

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Soundness / Use-After-Free due to lifetime erasure in ForeignOwnable implementation for DriverFence.
- [Critical] TOCTOU race condition in FenceCbRegistration::new allows uninitialized memory access.
- [High] Deadlock when calling Fence::is_signaled() from a registered fence callback.
- [High] FenceCtx::drop calls rcu_barrier(), sleeping in atomic context.
- [Medium] Missing dma_fence_begin_signalling() lockdep annotations in DriverFence::signal and DriverFence::drop. --

commit 13ec5041cfbf69d9b4feb4a3709de7bd1157511a
Author: Philipp Stanner <[email protected]>

rust: Add dma_fence abstractions

This commit introduces Rust abstractions for the C dma_fence API, providing a safe interface for drivers to create, signal, and await fences. It handles the memory management, callback registration, and context creation.

Quoted 5 lines
rust/kernel/dma_buf/dma_fence.rs new file +0 -0
new file mode 100644
index 0000000000000..a9d9a684947c5

[ ... ]

rust/kernel/dma_buf/dma_fence.rs line 184

Quoted 9 lines
rust/kernel/dma_buf/dma_fence.rs new file +9 -0
+    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()
+    }

[Severity: Critical]
Could this result in a use-after-free?

When the C backend calls this mandatory callback, it dereferences the fctx pointer. However, the lifetime of the DriverFence's context is erased in the ForeignOwnable implementation. If the underlying FenceCtx is dropped prematurely from safe Rust, could this callback access freed memory?

[ ... ]

rust/kernel/dma_buf/dma_fence.rs line 229

Quoted 6 lines
rust/kernel/dma_buf/dma_fence.rs new file +6 -0
+#[pinned_drop]
+impl<T: Send + Sync> PinnedDrop for FenceCtx<T> {
+    fn drop(self: Pin<&mut Self>) {
+        rcu_barrier();
+    }
+}

[Severity: High]
Does this sleep in atomic context?

FenceCtx is managed by an Arc, meaning its drop executes when the last reference is released. If an Arc<FenceCtx> is dropped in an atomic context like an interrupt handler or while holding a spinlock, unconditionally calling rcu_barrier() will block and sleep, violating the atomic context constraints.

[ ... ]

Quoted 22 lines
rust/kernel/dma_buf/dma_fence.rs new file +22 -0
+    pub fn new<'a>(fence: &'a Fence, callback: T) -> impl PinInit<Self, CallbackError<T>> + 'a
+    where
+        T: 'a,
+    {
+        let mut callback = Some(callback);
+
+        try_pin_init!(Self {
+            cb <- 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) => {

rust/kernel/dma_buf/dma_fence.rs line 352

Quoted 10 lines
rust/kernel/dma_buf/dma_fence.rs new file +10 -0
+                        let cb_back = callback.take().expect("Success path did not run.");
+                        if e == ENOENT {
+                            Err(CallbackError::AlreadySignaled(cb_back))
+                        } else {
+                            Err(CallbackError::Other(e))
+                        }
+                    }
+                }
+            }),
+            callback: callback.take().expect("Error path did not run."),

[Severity: Critical]
Is there a race condition here that allows uninitialized memory access?

The cb field is initialized first, which registers the callback with the C backend via dma_fence_add_callback. Once added, the callback is live. However, the callback field containing the user's T: FenceCb implementation has not yet been initialized.

If a concurrent thread signals the fence in this tight window, the C subsystem will immediately invoke Self::dma_fence_callback, which will cast the pointer back and invoke (*reg).callback.called() on uninitialized memory.

[ ... ]

Quoted 16 lines
rust/kernel/dma_buf/dma_fence.rs new file +16 -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
+        // 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.
+        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 line 464

Quoted 10 lines
rust/kernel/dma_buf/dma_fence.rs new file +10 -0
+        // 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.
+
+        // 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) };

[Severity: High]
Could this cause a deadlock if called from within a registered fence callback?

When the fence signals, the C backend (dma_fence_signal) acquires the fence's spinlock and iterates over the callback list. The callback executes FenceCb::called() synchronously while the spinlock is held.

If the user's called() implementation invokes Fence::is_signaled(), this code will forcefully attempt to acquire the exact same spinlock, leading to an immediate CPU deadlock since spinlocks are not recursive.

[ ... ]

Quoted 9 lines
rust/kernel/dma_buf/dma_fence.rs new file +9 -0
+    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

rust/kernel/dma_buf/dma_fence.rs line 672

Quoted 10 lines
rust/kernel/dma_buf/dma_fence.rs new file +10 -0
+        // 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) };

[Severity: Medium]
Are we missing the dma_fence_begin_signalling() lockdep annotations here?

By calling bindings::dma_fence_signal_locked() directly instead of using the standard dma_fence_signal() C function, we skip the lockdep state tracking helpers. Could this disable lockdep's ability to detect deadlocks between fence signaling and fence waiters?

[ ... ]

Quoted 6 lines
rust/kernel/dma_buf/dma_fence.rs new file +6 -0
+unsafe impl<'b, T: Send + Sync + FenceCtxOps + 'static> ForeignOwnable for DriverFence<'b, T> {
+    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>();
+

rust/kernel/dma_buf/dma_fence.rs line 730

Quoted 10 lines
rust/kernel/dma_buf/dma_fence.rs new file +10 -0
+    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()
+    }

[Severity: Critical]
Does this ForeignOwnable implementation introduce a soundness issue by erasing the lifetime 'b bound to FenceCtx?

When into_foreign() is called, it consumes the DriverFence and returns a raw pointer, effectively erasing the lifetime 'b from the borrow checker's perspective. This allows the borrow on FenceCtx to be released, and safe Rust code can then drop the FenceCtx.

Since the C backend still retains the raw dma_fence pointer, any subsequent use of mandatory callbacks (like get_driver_name) will read from the dangling fctx pointer inside DriverFenceData, causing a use-after-free.

[ ... ]

Quoted 12 lines
rust/kernel/dma_buf/dma_fence.rs new file +12 -0
+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.

rust/kernel/dma_buf/dma_fence.rs line 796

Quoted 10 lines
rust/kernel/dma_buf/dma_fence.rs new file +10 -0
+
+        // 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) };

[Severity: Medium]
Similar to the signal() method above, are we skipping the required lockdep annotations here?

This fallback drop signaling path also invokes dma_fence_signal_locked() directly without wrapping it in dma_fence_begin_signalling() and dma_fence_end_signalling().

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
author maintainer
#8 lore raw

PATCH v4 5/5 MAINTAINERS: Add entry for Rust dma-buf

In reply to Philipp Stanner

Rust does now have abstractions for dma_fence. These abstractions are quite complicated and require expertise with both the C and the Rust side. Therefore, using the existing entry also for maintenance of the Rust code appears reasonable.

Philipp volunteers to help maintain the dma_fence abstractions. Add a corresponding MAINTAINERS entry.

---
 MAINTAINERS | 5 +++++
 1 file changed, 5 insertions(+)
MAINTAINERS +5 -0
index 4ae6919454c3..ef2cb1173c66 100644
@@ -7535,6 +7535,7 @@ F:	fs/dlm/
 DMA BUFFER SHARING FRAMEWORK
 M:	Sumit Semwal <[email protected]>
 M:	Christian König <[email protected]>
+M:	Philipp Stanner <[email protected]>
 L:	[email protected]
 L:	[email protected]
 L:	[email protected] (moderated for non-subscribers)
@@ -7548,6 +7549,8 @@ F:	include/linux/dma-buf.h
 F:	include/linux/dma-buf/
 F:	include/linux/dma-resv.h
 F:	rust/helpers/dma-resv.c
+F:	rust/helpers/dma_fence.c
+F:	rust/kernel/dma_buf/
 K:	\bdma_(?:buf|fence|resv)\b
 
 DMA GENERIC OFFLOAD ENGINE SUBSYSTEM
@@ -8536,7 +8539,9 @@ T:	git https://gitlab.freedesktop.org/drm/rust/kernel.git
 F:	drivers/gpu/drm/nova/
 F:	drivers/gpu/drm/tyr/
 F:	drivers/gpu/nova-core/
+F:	rust/helpers/dma_fence.c
 F:	rust/helpers/gpu.c
+F:	rust/kernel/dma_buf/
 F:	rust/kernel/drm/
 F:	rust/kernel/gpu.rs
 F:	rust/kernel/gpu/
Signature · 1 line
2.54.0