* [PATCH 0/4] rust / dma_buf: Add abstractions for dma_fence
@ 2026-05-30 14:35 Philipp Stanner
2026-05-30 14:35 ` [PATCH 1/4] rust: types: implement ForeignOwnable for ARef<T> Philipp Stanner
` (3 more replies)
0 siblings, 4 replies; 35+ messages in thread
From: Philipp Stanner @ 2026-05-30 14:35 UTC (permalink / raw)
To: Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Sumit Semwal, Christian König,
Philipp Stanner, Paul E. McKenney, Frederic Weisbecker,
Neeraj Upadhyay, Joel Fernandes, Josh Triplett, Uladzislau Rezki,
Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, Zqiang,
Daniel Almeida, Greg Kroah-Hartman, Igor Korotin, Lorenzo Stoakes,
Alexandre Courbot, FUJITA Tomonori, Krishna Ketan Rai,
Shankari Anand, manos, Boris Brezillon
Cc: linux-kernel, rust-for-linux, linux-media, dri-devel,
linaro-mm-sig, rcu
Changes since the RFC:
- 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.
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/20260203081403.68733-2-phasta@kernel.org/
Alice Ryhl (1):
rust: rcu: add RcuBox type
Danilo Krummrich (1):
rust: types: implement ForeignOwnable for ARef<T>
Philipp Stanner (2):
rust: Add dma_fence abstractions
MAINTAINERS: Add entry for Rust dma-buf
MAINTAINERS | 2 +
rust/bindings/bindings_helper.h | 2 +
rust/helpers/dma_fence.c | 48 ++
rust/helpers/helpers.c | 1 +
rust/kernel/dma_buf/dma_fence.rs | 821 +++++++++++++++++++++++++++++++
rust/kernel/dma_buf/mod.rs | 13 +
rust/kernel/lib.rs | 1 +
rust/kernel/sync/aref.rs | 39 ++
rust/kernel/sync/rcu.rs | 31 +-
rust/kernel/sync/rcu/rcu_box.rs | 145 ++++++
10 files changed, 1102 insertions(+), 1 deletion(-)
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
create mode 100644 rust/kernel/sync/rcu/rcu_box.rs
--
2.54.0
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 1/4] rust: types: implement ForeignOwnable for ARef<T>
2026-05-30 14:35 [PATCH 0/4] rust / dma_buf: Add abstractions for dma_fence Philipp Stanner
@ 2026-05-30 14:35 ` Philipp Stanner
2026-05-30 14:45 ` sashiko-bot
2026-06-01 9:46 ` Alice Ryhl
2026-05-30 14:35 ` [PATCH 2/4] rust: rcu: add RcuBox type Philipp Stanner
` (2 subsequent siblings)
3 siblings, 2 replies; 35+ messages in thread
From: Philipp Stanner @ 2026-05-30 14:35 UTC (permalink / raw)
To: Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Sumit Semwal, Christian König,
Philipp Stanner, Paul E. McKenney, Frederic Weisbecker,
Neeraj Upadhyay, Joel Fernandes, Josh Triplett, Uladzislau Rezki,
Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, Zqiang,
Daniel Almeida, Greg Kroah-Hartman, Igor Korotin, Lorenzo Stoakes,
Alexandre Courbot, FUJITA Tomonori, Krishna Ketan Rai,
Shankari Anand, manos, Boris Brezillon
Cc: linux-kernel, rust-for-linux, linux-media, dri-devel,
linaro-mm-sig, rcu
From: Danilo Krummrich <dakr@kernel.org>
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.
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
rust/kernel/sync/aref.rs | 39 +++++++++++++++++++++++++++++++++++++++
1 file changed, 39 insertions(+)
diff --git a/rust/kernel/sync/aref.rs b/rust/kernel/sync/aref.rs
index 9989f56d0605..82907383c44b 100644
--- a/rust/kernel/sync/aref.rs
+++ b/rust/kernel/sync/aref.rs
@@ -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>,
--
2.54.0
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 2/4] rust: rcu: add RcuBox type
2026-05-30 14:35 [PATCH 0/4] rust / dma_buf: Add abstractions for dma_fence Philipp Stanner
2026-05-30 14:35 ` [PATCH 1/4] rust: types: implement ForeignOwnable for ARef<T> Philipp Stanner
@ 2026-05-30 14:35 ` Philipp Stanner
2026-05-30 14:54 ` sashiko-bot
2026-05-30 15:08 ` Boqun Feng
2026-05-30 14:35 ` [PATCH 3/4] rust: Add dma_fence abstractions Philipp Stanner
2026-05-30 14:35 ` [PATCH 4/4] MAINTAINERS: Add entry for Rust dma-buf Philipp Stanner
3 siblings, 2 replies; 35+ messages in thread
From: Philipp Stanner @ 2026-05-30 14:35 UTC (permalink / raw)
To: Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Sumit Semwal, Christian König,
Philipp Stanner, Paul E. McKenney, Frederic Weisbecker,
Neeraj Upadhyay, Joel Fernandes, Josh Triplett, Uladzislau Rezki,
Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, Zqiang,
Daniel Almeida, Greg Kroah-Hartman, Igor Korotin, Lorenzo Stoakes,
Alexandre Courbot, FUJITA Tomonori, Krishna Ketan Rai,
Shankari Anand, manos, Boris Brezillon
Cc: linux-kernel, rust-for-linux, linux-media, dri-devel,
linaro-mm-sig, rcu
From: Alice Ryhl <aliceryhl@google.com>
This adds an RcuBox container, which is like KBox except that the value
is freed with kfree_rcu.
To allow containers to rely on the rcu properties of RcuBox, an
extension of ForeignOwnable is added.
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
rust/bindings/bindings_helper.h | 1 +
rust/kernel/sync/rcu.rs | 31 ++++++-
rust/kernel/sync/rcu/rcu_box.rs | 145 ++++++++++++++++++++++++++++++++
3 files changed, 176 insertions(+), 1 deletion(-)
create mode 100644 rust/kernel/sync/rcu/rcu_box.rs
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 446dbeaf0866..2011645c7cfb 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -80,6 +80,7 @@
#include <linux/property.h>
#include <linux/pwm.h>
#include <linux/random.h>
+#include <linux/rcupdate.h>
#include <linux/refcount.h>
#include <linux/regulator/consumer.h>
#include <linux/sched.h>
diff --git a/rust/kernel/sync/rcu.rs b/rust/kernel/sync/rcu.rs
index a32bef6e490b..7234fe3e79ee 100644
--- a/rust/kernel/sync/rcu.rs
+++ b/rust/kernel/sync/rcu.rs
@@ -4,7 +4,16 @@
//!
//! C header: [`include/linux/rcupdate.h`](srctree/include/linux/rcupdate.h)
-use crate::{bindings, types::NotThreadSafe};
+use crate::{
+ bindings,
+ types::{
+ ForeignOwnable,
+ NotThreadSafe, //
+ }, //
+};
+
+mod rcu_box;
+pub use self::rcu_box::RcuBox;
/// Evidence that the RCU read side lock is held on the current thread/CPU.
///
@@ -50,3 +59,23 @@ fn drop(&mut self) {
pub fn read_lock() -> Guard {
Guard::new()
}
+
+/// Declares that a pointer type is rcu safe.
+pub trait ForeignOwnableRcu: ForeignOwnable {
+ /// Type used to immutably borrow an rcu-safe value that is currently foreign-owned.
+ type RcuBorrowed<'a>;
+
+ /// Borrows a foreign-owned object immutably for an rcu grace period.
+ ///
+ /// This method provides a way to access a foreign-owned rcu-safe value from Rust immutably.
+ ///
+ /// # Safety
+ ///
+ /// * The provided pointer must have been returned by a previous call to [`into_foreign`].
+ /// * If [`from_foreign`] is called, then `'a` must not end after the call to `from_foreign`
+ /// plus one rcu grace period.
+ ///
+ /// [`into_foreign`]: ForeignOwnable::into_foreign
+ /// [`from_foreign`]: ForeignOwnable::from_foreign
+ unsafe fn rcu_borrow<'a>(ptr: *mut ffi::c_void) -> Self::RcuBorrowed<'a>;
+}
diff --git a/rust/kernel/sync/rcu/rcu_box.rs b/rust/kernel/sync/rcu/rcu_box.rs
new file mode 100644
index 000000000000..2508fdb609ec
--- /dev/null
+++ b/rust/kernel/sync/rcu/rcu_box.rs
@@ -0,0 +1,145 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2026 Google LLC.
+
+//! Provides the `RcuBox` type for Rust allocations that live for a grace period.
+
+use core::{ops::Deref, ptr::NonNull};
+
+use kernel::{
+ alloc::{self, AllocError},
+ bindings,
+ ffi::c_void,
+ prelude::*,
+ sync::rcu::{ForeignOwnableRcu, Guard},
+ types::ForeignOwnable,
+};
+
+/// A box that is freed with rcu.
+///
+/// The value must be `Send`, as rcu may drop it on another thread.
+///
+/// # Invariants
+///
+/// * The pointer is valid and references a pinned `RcuBoxInner<T>` allocated with `kmalloc`.
+/// * This `RcuBox` holds exclusive permissions to rcu free the allocation.
+pub struct RcuBox<T: Send>(NonNull<RcuBoxInner<T>>);
+
+struct RcuBoxInner<T> {
+ value: T,
+ rcu_head: bindings::callback_head,
+}
+
+// Note that `T: Sync` is required since when moving an `RcuBox<T>`, the previous owner may still
+// access `&T` for one grace period.
+//
+// SAFETY: Ownership of the `RcuBox<T>` allows for `&T` and dropping the `T`, so `T: Send + Sync`
+// implies `RcuBox<T>: Send`.
+unsafe impl<T: Send + Sync> Send for RcuBox<T> {}
+
+// SAFETY: `&RcuBox<T>` allows for no operations other than those permitted by `&T`, so `T: Sync`
+// implies `RcuBox<T>: Sync`.
+unsafe impl<T: Send + Sync> Sync for RcuBox<T> {}
+
+impl<T: Send> RcuBox<T> {
+ /// Create a new `RcuBox`.
+ pub fn new(x: T, flags: alloc::Flags) -> Result<Self, AllocError> {
+ let b = KBox::new(
+ RcuBoxInner {
+ value: x,
+ rcu_head: Default::default(),
+ },
+ flags,
+ )?;
+
+ // INVARIANT:
+ // * The pointer contains a valid `RcuBoxInner` allocated with `kmalloc`.
+ // * We just allocated it, so we own free permissions.
+ Ok(RcuBox(NonNull::from(KBox::leak(b))))
+ }
+
+ /// Access the value for a grace period.
+ pub fn with_rcu<'rcu>(&self, _read_guard: &'rcu Guard) -> &'rcu T {
+ // SAFETY: The `RcuBox` has not been dropped yet, so the value is valid for at least one
+ // grace period.
+ unsafe { &(*self.0.as_ptr()).value }
+ }
+}
+
+impl<T: Send> Deref for RcuBox<T> {
+ type Target = T;
+ fn deref(&self) -> &T {
+ // SAFETY: While the `RcuBox<T>` exists, the value remains valid.
+ unsafe { &(*self.0.as_ptr()).value }
+ }
+}
+
+// SAFETY:
+// * The `RcuBoxInner<T>` was allocated with `kmalloc`.
+// * `NonNull::as_ptr` returns a non-null pointer.
+unsafe impl<T: Send + 'static> ForeignOwnable for RcuBox<T> {
+ const FOREIGN_ALIGN: usize = <KBox<RcuBoxInner<T>> as ForeignOwnable>::FOREIGN_ALIGN;
+
+ type Borrowed<'a> = &'a T;
+ type BorrowedMut<'a> = &'a T;
+
+ fn into_foreign(self) -> *mut c_void {
+ self.0.as_ptr().cast()
+ }
+
+ unsafe fn from_foreign(ptr: *mut c_void) -> Self {
+ // INVARIANT: Pointer returned by `into_foreign` carries same invariants as `RcuBox<T>`.
+ // SAFETY: `into_foreign` never returns a null pointer.
+ Self(unsafe { NonNull::new_unchecked(ptr.cast()) })
+ }
+
+ unsafe fn borrow<'a>(ptr: *mut c_void) -> &'a T {
+ // SAFETY: Caller ensures that `'a` is short enough.
+ unsafe { &(*ptr.cast::<RcuBoxInner<T>>()).value }
+ }
+
+ unsafe fn borrow_mut<'a>(ptr: *mut c_void) -> &'a T {
+ // SAFETY: `borrow_mut` has strictly stronger preconditions than `borrow`.
+ unsafe { Self::borrow(ptr) }
+ }
+}
+
+impl<T: Send + 'static> ForeignOwnableRcu for RcuBox<T> {
+ type RcuBorrowed<'a> = &'a T;
+
+ unsafe fn rcu_borrow<'a>(ptr: *mut c_void) -> &'a T {
+ // SAFETY: `RcuBox::drop` can only run after `from_foreign` is called, and the value is
+ // valid until `RcuBox::drop` plus one grace period.
+ unsafe { &(*ptr.cast::<RcuBoxInner<T>>()).value }
+ }
+}
+
+impl<T: Send> Drop for RcuBox<T> {
+ fn drop(&mut self) {
+ // SAFETY: The `rcu_head` field is in-bounds of a valid allocation.
+ let rcu_head = unsafe { &raw mut (*self.0.as_ptr()).rcu_head };
+ if core::mem::needs_drop::<T>() {
+ // SAFETY: `rcu_head` is the `rcu_head` field of `RcuBoxInner<T>`. All users will be
+ // gone in an rcu grace period. This is the destructor, so we may pass ownership of the
+ // allocation.
+ unsafe { bindings::call_rcu(rcu_head, Some(drop_rcu_box::<T>)) };
+ } else {
+ // SAFETY: All users will be gone in an rcu grace period.
+ unsafe { bindings::kvfree_call_rcu(rcu_head, self.0.as_ptr().cast()) };
+ }
+ }
+}
+
+/// Free this `RcuBoxInner<T>`.
+///
+/// # Safety
+///
+/// `head` references the `rcu_head` field of an `RcuBoxInner<T>` that has no references to it.
+/// Ownership of the `KBox<RcuBoxInner<T>>` must be passed.
+unsafe extern "C" fn drop_rcu_box<T>(head: *mut bindings::callback_head) {
+ // SAFETY: Caller provides a pointer to the `rcu_head` field of a `RcuBoxInner<T>`.
+ let box_inner = unsafe { crate::container_of!(head, RcuBoxInner<T>, rcu_head) };
+
+ // SAFETY: Caller ensures exclusive access and passed ownership.
+ drop(unsafe { KBox::from_raw(box_inner) });
+}
--
2.54.0
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 3/4] rust: Add dma_fence abstractions
2026-05-30 14:35 [PATCH 0/4] rust / dma_buf: Add abstractions for dma_fence Philipp Stanner
2026-05-30 14:35 ` [PATCH 1/4] rust: types: implement ForeignOwnable for ARef<T> Philipp Stanner
2026-05-30 14:35 ` [PATCH 2/4] rust: rcu: add RcuBox type Philipp Stanner
@ 2026-05-30 14:35 ` Philipp Stanner
2026-05-30 15:06 ` sashiko-bot
` (2 more replies)
2026-05-30 14:35 ` [PATCH 4/4] MAINTAINERS: Add entry for Rust dma-buf Philipp Stanner
3 siblings, 3 replies; 35+ messages in thread
From: Philipp Stanner @ 2026-05-30 14:35 UTC (permalink / raw)
To: Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Sumit Semwal, Christian König,
Philipp Stanner, Paul E. McKenney, Frederic Weisbecker,
Neeraj Upadhyay, Joel Fernandes, Josh Triplett, Uladzislau Rezki,
Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, Zqiang,
Daniel Almeida, Greg Kroah-Hartman, Igor Korotin, Lorenzo Stoakes,
Alexandre Courbot, FUJITA Tomonori, Krishna Ketan Rai,
Shankari Anand, manos, Boris Brezillon
Cc: linux-kernel, rust-for-linux, linux-media, dri-devel,
linaro-mm-sig, rcu
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 through 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 <phasta@kernel.org>
---
rust/bindings/bindings_helper.h | 1 +
rust/helpers/dma_fence.c | 48 ++
rust/helpers/helpers.c | 1 +
rust/kernel/dma_buf/dma_fence.rs | 821 +++++++++++++++++++++++++++++++
rust/kernel/dma_buf/mod.rs | 13 +
rust/kernel/lib.rs | 1 +
6 files changed, 885 insertions(+)
create mode 100644 rust/helpers/dma_fence.c
create mode 100644 rust/kernel/dma_buf/dma_fence.rs
create mode 100644 rust/kernel/dma_buf/mod.rs
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 2011645c7cfb..69daeb790f77 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -52,6 +52,7 @@
#include <linux/debugfs.h>
#include <linux/device/faux.h>
#include <linux/dma-direction.h>
+#include <linux/dma-fence.h>
#include <linux/dma-mapping.h>
#include <linux/dma-resv.h>
#include <linux/errname.h>
diff --git a/rust/helpers/dma_fence.c b/rust/helpers/dma_fence.c
new file mode 100644
index 000000000000..6244a5a61038
--- /dev/null
+++ b/rust/helpers/dma_fence.c
@@ -0,0 +1,48 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/dma-fence.h>
+
+__rust_helper void rust_helper_dma_fence_get(struct dma_fence *f)
+{
+ dma_fence_get(f);
+}
+
+__rust_helper void rust_helper_dma_fence_put(struct dma_fence *f)
+{
+ dma_fence_put(f);
+}
+
+__rust_helper bool rust_helper_dma_fence_begin_signalling(void)
+{
+ return dma_fence_begin_signalling();
+}
+
+__rust_helper void rust_helper_dma_fence_end_signalling(bool cookie)
+{
+ dma_fence_end_signalling(cookie);
+}
+
+__rust_helper bool rust_helper_dma_fence_is_signaled(struct dma_fence *f)
+{
+ return dma_fence_is_signaled(f);
+}
+
+__rust_helper bool rust_helper_dma_fence_is_signaled_locked(struct dma_fence *f)
+{
+ return dma_fence_is_signaled_locked(f);
+}
+
+__rust_helper void rust_helper_dma_fence_lock_irqsave(struct dma_fence *f, unsigned long *flags)
+{
+ dma_fence_lock_irqsave(f, *flags);
+}
+
+__rust_helper void rust_helper_dma_fence_unlock_irqrestore(struct dma_fence *f, unsigned long *flags)
+{
+ dma_fence_unlock_irqrestore(f, *flags);
+}
+
+__rust_helper void rust_helper_dma_fence_set_error(struct dma_fence *f, int error)
+{
+ dma_fence_set_error(f, error);
+}
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 625921e27dfb..d9114d0b3c8f 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -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 "err.c"
diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
new file mode 100644
index 000000000000..7dc1f5c16b02
--- /dev/null
+++ b/rust/kernel/dma_buf/dma_fence.rs
@@ -0,0 +1,821 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (C) 2025, 2026 Red Hat Inc.:
+// - Philipp Stanner <pstanner@redhat.com>
+
+//! DriverFence support.
+//!
+//! Reference: <https://docs.kernel.org/driver-api/dma-buf.html#c.dma_fence>
+//!
+//! C header: [`include/linux/dma-fence.h`](srctree/include/linux/dma-fence.h)
+
+use crate::{
+ alloc::AllocError,
+ bindings,
+ container_of,
+ error::to_result,
+ prelude::*,
+ sync::rcu::RcuBox,
+ types::ForeignOwnable,
+ types::Opaque,
+ warn_on, //
+};
+
+use pin_init::pin_init_from_closure;
+
+use core::{
+ marker::PhantomData, //
+ ops::Deref,
+ ptr,
+ ptr::{
+ drop_in_place,
+ NonNull, //
+ },
+ sync::atomic::{
+ AtomicU64,
+ Ordering, //
+ },
+};
+
+use bindings::ECANCELED;
+
+use kernel::str::CString;
+use kernel::sync::{
+ aref::{
+ ARef,
+ AlwaysRefCounted, //
+ },
+ Arc,
+ ArcBorrow, //
+};
+
+/// 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 {}
+
+/// 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<F: Send + Sync, C: Send + Sync> {
+ /// The fence context number.
+ nr: u64,
+ /// The sequence number for the next fence created.
+ seqno: AtomicU64,
+ // The name parameters live in RcuBox because they can be accessed by the
+ // dma_fence backend_ops. Those accesses are guarded by the rcu_read_lock(),
+ // so dropping them must be delayed by a grace period.
+ /// 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,
+ #[pin]
+ data: C,
+ fence_type: PhantomData<F>,
+}
+
+#[allow(unused_unsafe)]
+impl<F: Send + Sync + DriverFenceAllowedData, C: Send + Sync> FenceCtx<F, C> {
+ // 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(
+ driver_name: CString,
+ timeline_name: CString,
+ data: impl PinInit<C>,
+ ) -> Result<Arc<Self>> {
+ let ctx = 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),
+ driver_name,
+ timeline_name,
+ data <- data,
+ fence_type: PhantomData,
+ });
+
+ Arc::pin_init(ctx, GFP_KERNEL)
+ }
+
+ 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: ArcBorrow<'_, Self>,
+ data: F,
+ ) -> Result<DriverFenceAllocation<F, C>> {
+ let fctx = Arc::<Self>::from(self);
+
+ DriverFenceAllocation::new(fctx, data)
+ }
+
+ /// 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<F, C>) -> DriverFence<F, C> {
+ 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<'a>(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<F, C>;
+
+ // 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<F: Send + Sync, C: Send + Sync> PinnedDrop for FenceCtx<F, C> {
+ fn drop(self: Pin<&mut Self>) {
+ // SAFETY: `rcu_barrier()` is always safe to be called.
+ unsafe { bindings::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
+/// }
+///
+/// impl FenceCb for MyCallback {
+/// fn called(&mut self) {
+/// pr_info!("Fence signaled!");
+/// // Handle fence completion
+/// }
+/// }
+/// ```
+pub trait FenceCb: Send + 'static {
+ /// Called when the fence is signaled.
+ ///
+ /// This is called from the fence signaling path, which may be in interrupt
+ /// context or with locks held, which is why `self` is only borrowed, so that
+ /// it cannot drop. Implementations must not sleep or perform
+ /// long-running operations.
+ ///
+ /// An implementation likely wants to inform itself (e.g., through a work item)
+ /// within this callback that the associated [`FenceCbRegistration`] can now be
+ /// dropped.
+ fn called(&mut self);
+}
+
+/// 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]
+ cb: Opaque<bindings::dma_fence_cb>,
+ callback: T,
+ fence: ARef<Fence>,
+}
+
+impl<T: FenceCb> FenceCbRegistration<T> {
+ /// Register a callback on a fence.
+ ///
+ /// 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,
+ {
+ // Uses `pin_init_from_closure` instead of `try_pin_init!` so that on
+ // `-ENOENT` (already signaled) the callback can be read back from the
+ // partially-initialized slot and returned through the error.
+ //
+ // SAFETY: `pin_init_from_closure` requires:
+ // - On `Ok(())`: the slot is fully initialized and valid for `Drop`.
+ // - On `Err(_)`: the slot is clean, i.e.: no partially-initialized fields
+ // remain, and the slot can be deallocated without dropping.
+ //
+ // We uphold this as follows:
+ // - On success: all three fields are initialized. Ok(()) is returned.
+ // - On ENOENT (already signaled): `callback` and `fence` are read back
+ // from the slot via `ptr::read`, leaving the slot clean. `cb` was
+ // initialized by `dma_fence_add_callback` (it calls
+ // `INIT_LIST_HEAD(&cb->node)` even on error), but `cb` is
+ // `Opaque<dma_fence_cb>` which has no `Drop`, so not dropping it is
+ // fine. The callback is returned through `AlreadySignaled(T)`.
+ // - On other errors: same cleanup as ENOENT, error returned as
+ // `Other(e)`.
+ unsafe {
+ pin_init_from_closure(move |slot: *mut Self| {
+ let slot_callback = &raw mut (*slot).callback;
+ let slot_fence = &raw mut (*slot).fence;
+ let slot_cb = &raw mut (*slot).cb;
+
+ // Write callback and fence first — must be visible before
+ // dma_fence_add_callback makes the registration live.
+ core::ptr::write(slot_callback, callback);
+ core::ptr::write(slot_fence, ARef::from(fence));
+
+ let ret = to_result(bindings::dma_fence_add_callback(
+ fence.inner.get(),
+ Opaque::cast_into(slot_cb),
+ Some(Self::dma_fence_callback),
+ ));
+
+ match ret {
+ Ok(()) => Ok(()),
+ Err(e) => {
+ // Read back what we wrote to leave the slot clean.
+ let cb_back = core::ptr::read(slot_callback);
+ let _fence_back = core::ptr::read(slot_fence);
+
+ if e.to_errno() == ENOENT.to_errno() {
+ Err(CallbackError::AlreadySignaled(cb_back))
+ } else {
+ Err(CallbackError::Other(e))
+ }
+ }
+ }
+ })
+ }
+ }
+
+ /// 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,
+ cb: *mut bindings::dma_fence_cb,
+ ) {
+ let ptr = Opaque::cast_from(cb).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, cb);
+
+ (*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.cb.get());
+ }
+ }
+}
+
+// SAFETY: FenceCbRegistration can be sent between threads
+unsafe impl<T: FenceCb> Send for FenceCbRegistration<T> {}
+
+// SAFETY: &FenceCbRegistration can be shared between threads if &T can.
+unsafe impl<T: FenceCb> Sync for FenceCbRegistration<T> where T: Sync {}
+
+/// 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.
+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.
+ pub fn is_signaled(&self) -> bool {
+ // SAFETY: self is by definition still valid. The backend ensures proper
+ // locking.
+ unsafe { bindings::dma_fence_is_signaled(self.as_raw()) }
+ }
+
+ 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: Valid because `fence` was created validly above.
+ unsafe { bindings::dma_fence_put(fence) }
+ }
+}
+
+#[repr(C)] // Necessary to guarantee that `inner` always comes first so that we can cast.
+#[pin_data]
+struct DriverFenceData<F: Send + Sync, C: Send + Sync> {
+ #[pin]
+ /// The inner fence.
+ inner: Fence,
+ /// Pointer to access the FenceCtx. Useful for obtaining name parameters.
+ // The FenceCtx lives as long as at least all its fences, hence this is safe.
+ fctx: Arc<FenceCtx<F, C>>,
+ /// The API user's data. As required by [`DriverFenceAllowedData`], this either
+ /// does not need drop, or must live in a [`rcu::RcuBox`]. It is essential
+ /// that the data only performs operations legal in atomic context in its
+ /// [`Drop`] implementation.
+ #[pin]
+ data: F,
+}
+
+/// A trait to enforce that all data in a [`DriverFence`] either does not need
+/// drop, or lives in a [`RcuBox`].
+pub trait DriverFenceAllowedData: private::Sealed {}
+
+mod private {
+ pub trait Sealed {}
+}
+
+impl<F: Copy> DriverFenceAllowedData for F {}
+impl<F: Send> DriverFenceAllowedData for RcuBox<F> {}
+
+impl<F: Copy> private::Sealed for F {}
+impl<F: Send> private::Sealed for RcuBox<F> {}
+
+/// 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, FenceCb, FenceCbRegistration};
+/// use kernel::str::CString;
+/// use kernel::sync::{
+/// aref::ARef,
+/// rcu::RcuBox, //
+/// };
+/// use core::ops::Deref;
+/// use core::fmt::Display;
+///
+/// struct CallbackData { }
+///
+/// impl FenceCb for CallbackData {
+/// fn called(&mut self) {
+/// pr_info!("DmaFence callback executed.\n");
+/// }
+/// }
+///
+/// let driver_name = CString::try_from_fmt(fmt!("dummy_driver"))?;
+/// let timeline_name = CString::try_from_fmt(fmt!("dummy_timeline"))?;
+///
+/// let fctx = FenceCtx::new(driver_name, timeline_name, ())?;
+///
+/// let fence_data = CString::try_from_fmt(fmt!("dummy_data"))?;
+/// // DriverFence::data must either not need drop, or live in an RcuBox.
+/// let fence_data = RcuBox::new(fence_data, GFP_KERNEL)?;
+///
+/// let fence_alloc = fctx.as_arc_borrow().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)?;
+///
+/// // DriverFence implements Deref.
+/// // FIXME: unit test claims that CString does not implement Display. Why?
+/// // pr_info!("Fence's inner data is: {}", fence.deref().deref());
+///
+/// // TODO begin_signalling
+/// fence.signal(Ok(()));
+/// assert_eq!(waiting_fence.is_signaled(), true);
+///
+/// Ok::<(), Error>(())
+/// ```
+pub struct DriverFence<F: Send + Sync, C: Send + Sync> {
+ /// 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<F, C>>,
+}
+
+/// 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<F: Send + Sync, C: Send + Sync> {
+ /// The memory for the actual content of the fence.
+ /// Handed over to a [`DriverFence`], or deallocated once the
+ /// [`DriverFenceAllocation`] drops.
+ data: KBox<DriverFenceData<F, C>>,
+}
+
+impl<F: Send + Sync + DriverFenceAllowedData, C: Send + Sync> DriverFenceAllocation<F, C> {
+ /// Create a new allocation slot that can later be used to create a fully
+ /// initialized [`DriverFence`] without the need to allocate.
+ pub fn new(fctx: Arc<FenceCtx<F, C>>, data: F) -> Result<Self> {
+ let fence_data = DriverFenceData {
+ // `inner` remains uninitialized until a [`DriverFence`] takes over.
+ inner: Fence {
+ inner: Opaque::uninit(),
+ },
+ fctx,
+ data,
+ };
+
+ // In order to support the C dma_fence callbacks, it is necessary for
+ // a `Fence` and a `DriverFence` to live in the same allocation,
+ // because the C backend passes a dma_fence, from which the driver most
+ // likely wants to be able to access its `data` in `DriverFence`.
+ //
+ // Hence, we need the manage the memory manually. It will be freed by the
+ // C backend automatically once the refcount within `Fence` drops to 0.
+ let data = KBox::new(fence_data, GFP_KERNEL | __GFP_ZERO)?;
+
+ Ok(Self { data })
+ }
+
+ fn as_raw(&self) -> *mut bindings::dma_fence {
+ self.data.inner.inner.get()
+ }
+}
+
+impl<F: Send + Sync, C: Send + Sync> DriverFence<F, C> {
+ fn as_raw(&self) -> *mut bindings::dma_fence {
+ // SAFETY: Valid because `self` is valid.
+ let fence_data = unsafe { &mut *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
+ /// [`FenceCtx`].
+ unsafe fn from_raw(ptr: *mut bindings::dma_fence) -> Self {
+ let opaque_fence = Opaque::cast_from(ptr);
+
+ // SAFETY: Safe due to the function's overall safety requirements.
+ let fence_ptr = unsafe { container_of!(opaque_fence, Fence, inner) };
+
+ // DriverFenceData is repr(C) and a Fence is its first member.
+ let fence_data_ptr = fence_ptr as *mut DriverFenceData<F, C>;
+
+ // 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: 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 the `DriverFence` has been dropped.
+ unsafe {
+ bindings::dma_fence_lock_irqsave(fence, flag_ptr);
+ if !bindings::dma_fence_is_signaled_locked(fence) {
+ if let Err(err) = res {
+ bindings::dma_fence_set_error(fence, err.to_errno());
+ }
+ bindings::dma_fence_signal_locked(fence);
+ }
+ bindings::dma_fence_unlock_irqrestore(fence, flag_ptr);
+ }
+ }
+}
+
+// SAFETY: Fences are literally designed to be shared between threads.
+unsafe impl<F: Send + Sync, C: Send + Sync> Send for DriverFence<F, C> {}
+
+impl<F: Send + Sync, C: Send + Sync> Deref for DriverFence<F, C> {
+ type Target = F;
+
+ fn deref(&self) -> &Self::Target {
+ // SAFETY: Thanks to refcounting, `data` is always valid as long as `self` is.
+ let data = unsafe { &*self.data.as_ptr() };
+
+ &data.data
+ }
+}
+
+/// A borrowed [`DriverFence`]. All you can do with it is access your user data
+/// and obtain a [`Fence`].
+pub struct DriverFenceBorrow<F: Send + Sync, C: Send + Sync> {
+ /// 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<F, C>>,
+}
+
+impl<F: Send + Sync, C: Send + Sync> Deref for DriverFenceBorrow<F, C> {
+ type Target = F;
+
+ fn deref(&self) -> &Self::Target {
+ // SAFETY: Thanks to refcounting, `data` is always valid as long as `self` is.
+ let data = unsafe { &*self.data.as_ptr() };
+
+ &data.data
+ }
+}
+
+impl<F: Send + Sync, C: Send + Sync> DriverFenceBorrow<F, C> {
+ fn as_raw(&self) -> *mut bindings::dma_fence {
+ // SAFETY: Valid because `self` is valid.
+ let fence_data = unsafe { &mut *self.data.as_ptr() };
+
+ fence_data.inner.inner.get()
+ }
+
+ /// 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()) }
+ }
+
+ /// Get a [`DriverFenceBorrow`] from a raw pointer.
+ ///
+ /// # Safety
+ ///
+ /// `ptr` must point to a raw dma_fence within a [`Fence`] within a [`DriverFenceData`].
+ unsafe fn from_raw(ptr: *mut bindings::dma_fence) -> Self {
+ let opaque_fence = Opaque::cast_from(ptr);
+
+ // SAFETY: Safe due to the function's overall safety requirements.
+ let fence_ptr = unsafe { container_of!(opaque_fence, Fence, inner) };
+
+ // DriverFenceData is repr(C) and a Fence is its first member.
+ let fence_data_ptr = fence_ptr as *mut DriverFenceData<F, C>;
+
+ // SAFETY: `fence_data_ptr` was created validly above.
+ let data = unsafe { NonNull::new_unchecked(fence_data_ptr) };
+
+ Self { data }
+ }
+}
+
+// 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<F: Send + Sync + 'static, C: Send + Sync + 'static> ForeignOwnable
+ for DriverFence<F, C>
+{
+ // `DriverFence` is merely a wrapper around a raw pointer. Thus, we can just
+ // use it directly.
+ type Borrowed<'a> = DriverFenceBorrow<F, C>;
+ type BorrowedMut<'a> = DriverFenceBorrow<F, C>;
+
+ 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.
+ core::mem::forget(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> {
+ // SAFETY: The trait implementation ensures that `ptr` always resides
+ // within a [`Fence`] within a [`DriverFenceData`].
+ unsafe { DriverFenceBorrow::from_raw(ptr.cast()) }
+ }
+
+ unsafe fn borrow_mut<'a>(ptr: *mut c_void) -> Self::BorrowedMut<'a> {
+ // SAFETY: The trait implementation ensures that `ptr` always resides
+ // within a [`Fence`] within a [`DriverFenceData`].
+ unsafe { DriverFenceBorrow::from_raw(ptr.cast()) }
+ }
+}
+
+impl<F: Send + Sync, C: Send + Sync> Drop for DriverFence<F, C> {
+ fn drop(&mut self) {
+ let fence = self.as_raw();
+ let mut fence_flags: usize = 0;
+ let flag_ptr = &raw mut fence_flags;
+
+ // 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 the `DriverFence` has been dropped.
+ unsafe {
+ bindings::dma_fence_lock_irqsave(fence, flag_ptr);
+ #[allow(unused_unsafe)]
+ if warn_on!(!bindings::dma_fence_is_signaled_locked(fence)) {
+ bindings::dma_fence_set_error(fence, ECANCELED as i32);
+ bindings::dma_fence_signal_locked(fence);
+ }
+ bindings::dma_fence_unlock_irqrestore(fence, flag_ptr);
+ }
+
+ // SAFETY: `self.data` is owned by the DriverFence, but could be accessed
+ // through some dma_fence callbacks right now. Access is being revoked
+ // above by signalling the fence. The DriverFenceAllowedData trait
+ // ensures that the data either does not need drop, or if it does it
+ // lives in a RcuBox which will delay dropping by one grace period, hence
+ // ensuring that all readers have disappeared.
+ unsafe { drop_in_place(self.data.as_ptr()) };
+
+ // 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 the `DriverFence` has been dropped.
+ unsafe {
+ bindings::dma_fence_put(fence);
+ }
+
+ // The actual memory the data associated with a `DriverFence` lives in
+ // gets freed by the C dma_fence backend once the fence's refcount reaches 0.
+ }
+}
diff --git a/rust/kernel/dma_buf/mod.rs b/rust/kernel/dma_buf/mod.rs
new file mode 100644
index 000000000000..d9da3dc57fce
--- /dev/null
+++ b/rust/kernel/dma_buf/mod.rs
@@ -0,0 +1,13 @@
+// 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, //
+};
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index b72b2fbe046d..a05ccaa7598c 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -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;
--
2.54.0
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 4/4] MAINTAINERS: Add entry for Rust dma-buf
2026-05-30 14:35 [PATCH 0/4] rust / dma_buf: Add abstractions for dma_fence Philipp Stanner
` (2 preceding siblings ...)
2026-05-30 14:35 ` [PATCH 3/4] rust: Add dma_fence abstractions Philipp Stanner
@ 2026-05-30 14:35 ` Philipp Stanner
2026-05-30 15:20 ` Danilo Krummrich
3 siblings, 1 reply; 35+ messages in thread
From: Philipp Stanner @ 2026-05-30 14:35 UTC (permalink / raw)
To: Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, Sumit Semwal, Christian König,
Philipp Stanner, Paul E. McKenney, Frederic Weisbecker,
Neeraj Upadhyay, Joel Fernandes, Josh Triplett, Uladzislau Rezki,
Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, Zqiang,
Daniel Almeida, Greg Kroah-Hartman, Igor Korotin, Lorenzo Stoakes,
Alexandre Courbot, FUJITA Tomonori, Krishna Ketan Rai,
Shankari Anand, manos, Boris Brezillon
Cc: linux-kernel, rust-for-linux, linux-media, dri-devel,
linaro-mm-sig, rcu
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.
Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
Just as a suggestion, I don't want to force myself in here. Would also
be perfectly happy with other approaches; there are certainly a few
people who could maintain or co-maintain it.
---
MAINTAINERS | 2 ++
1 file changed, 2 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 2e8d160babc2..31fc595d5c6b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7521,6 +7521,7 @@ F: fs/dlm/
DMA BUFFER SHARING FRAMEWORK
M: Sumit Semwal <sumit.semwal@linaro.org>
M: Christian König <christian.koenig@amd.com>
+M: Philipp Stanner <phasta@kernel.org>
L: linux-media@vger.kernel.org
L: dri-devel@lists.freedesktop.org
L: linaro-mm-sig@lists.linaro.org (moderated for non-subscribers)
@@ -7529,6 +7530,7 @@ T: git https://gitlab.freedesktop.org/drm/misc/kernel.git
F: Documentation/driver-api/dma-buf.rst
F: Documentation/userspace-api/dma-buf-alloc-exchange.rst
F: drivers/dma-buf/
+F: rust/kernel/dma_buf/
F: include/linux/*fence.h
F: include/linux/dma-buf.h
F: include/linux/dma-buf/
--
2.54.0
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 1/4] rust: types: implement ForeignOwnable for ARef<T>
2026-05-30 14:35 ` [PATCH 1/4] rust: types: implement ForeignOwnable for ARef<T> Philipp Stanner
@ 2026-05-30 14:45 ` sashiko-bot
2026-06-01 9:46 ` Alice Ryhl
1 sibling, 0 replies; 35+ messages in thread
From: sashiko-bot @ 2026-05-30 14:45 UTC (permalink / raw)
To: Philipp Stanner; +Cc: linux-media, ojeda
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Missing `#[inline]` annotations on small forwarding trait methods in `ForeignOwnable` implementation for `ARef<T>`.
--
commit 62543d419c62c8206acbd8b76f07e5198b71442e
Author: Danilo Krummrich <dakr@kernel.org>
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>. It sets BorrowedMut to &T to match the shared
ownership semantics of the underlying reference-counted type.
> diff --git a/rust/kernel/sync/aref.rs b/rust/kernel/sync/aref.rs
> index 9989f56d06052..82907383c44b5 100644
> --- a/rust/kernel/sync/aref.rs
> +++ b/rust/kernel/sync/aref.rs
[ ... ]
> + type Borrowed<'a> = &'a T;
> + type BorrowedMut<'a> = &'a T;
> +
> + fn into_foreign(self) -> *mut c_void {
[Severity: Low]
This isn't a bug, but should this method be annotated with #[inline]?
According to the Rust subsystem guidelines, small forwarding functions in
abstractions should be annotated with #[inline].
> + ARef::into_raw(self).as_ptr().cast()
> + }
> +
> + unsafe fn from_foreign(ptr: *mut c_void) -> Self {
[Severity: Low]
This isn't a bug, but should from_foreign() also have an #[inline]
annotation since it is a small wrapper method?
> + // 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 {
[Severity: Low]
This isn't a bug, but similar to the above, should borrow() be annotated
with #[inline]?
> + // 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 {
[Severity: Low]
This isn't a bug, but does borrow_mut() need an #[inline] annotation here
as well?
> + // SAFETY: The safety requirements for `borrow_mut` are a superset of the safety
> + // requirements for `borrow`.
> + unsafe { <Self as ForeignOwnable>::borrow(ptr) }
> + }
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260530143541.229628-2-phasta@kernel.org?part=1
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/4] rust: rcu: add RcuBox type
2026-05-30 14:35 ` [PATCH 2/4] rust: rcu: add RcuBox type Philipp Stanner
@ 2026-05-30 14:54 ` sashiko-bot
2026-05-30 15:08 ` Boqun Feng
1 sibling, 0 replies; 35+ messages in thread
From: sashiko-bot @ 2026-05-30 14:54 UTC (permalink / raw)
To: Philipp Stanner; +Cc: ojeda, linux-media
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] `RcuBox::into_foreign` consumes `self` by value without preventing its destructor from running, causing immediate RCU freeing of the active allocation.
- [High] `RcuBox` defers the execution of the inner value's destructor using RCU but fails to restrict the value to the `'static` lifetime, allowing use-after-free on stack references.
- [High] `ForeignOwnableRcu` implementation for `RcuBox<T>` does not require `T: Sync`, allowing data races on `!Sync` types.
--
commit 81791c9c93af881931b7e9c9ed0d885ba31c73cb
Author: Alice Ryhl <aliceryhl@google.com>
rust: rcu: add RcuBox type
This adds an RcuBox container, which is like KBox except that the value
is freed with kfree_rcu. To allow containers to rely on the rcu properties
of RcuBox, an extension of ForeignOwnable is added.
> diff --git a/rust/kernel/sync/rcu/rcu_box.rs b/rust/kernel/sync/rcu/rcu_box.rs
> new file mode 100644
> index 0000000000000..2508fdb609ecc
> --- /dev/null
> +++ b/rust/kernel/sync/rcu/rcu_box.rs
[ ... ]
> +/// A box that is freed with rcu.
> +///
> +/// The value must be `Send`, as rcu may drop it on another thread.
> +///
> +/// # Invariants
> +///
> +/// * The pointer is valid and references a pinned `RcuBoxInner<T>` allocated with `kmalloc`.
> +/// * This `RcuBox` holds exclusive permissions to rcu free the allocation.
> +pub struct RcuBox<T: Send>(NonNull<RcuBoxInner<T>>);
[Severity: High]
Does RcuBox need a 'static bound on T here?
RcuBox defers the execution of the inner value's destructor using RCU via
call_rcu, but the definition lacks a 'static bound on T. This means the Rust
compiler would permit short-lived local references (e.g., &'a mut i32) to be
stored inside types wrapped by RcuBox.
Since the actual destructor of T is executed asynchronously in the RCU
callback, it may occur after the local variables it references have been
popped from the stack. If T's destructor accesses those references, it will
read or write to freed stack memory.
[ ... ]
> +// SAFETY:
> +// * The `RcuBoxInner<T>` was allocated with `kmalloc`.
> +// * `NonNull::as_ptr` returns a non-null pointer.
> +unsafe impl<T: Send + 'static> ForeignOwnable for RcuBox<T> {
> + const FOREIGN_ALIGN: usize = <KBox<RcuBoxInner<T>> as ForeignOwnable>::FOREIGN_ALIGN;
> +
> + type Borrowed<'a> = &'a T;
> + type BorrowedMut<'a> = &'a T;
> +
> + fn into_foreign(self) -> *mut c_void {
> + self.0.as_ptr().cast()
> + }
[Severity: Critical]
Does this code inadvertently run the destructor of RcuBox during
into_foreign()?
Since this method takes self by value and doesn't explicitly wrap it in
core::mem::ManuallyDrop or call core::mem::forget(self), the object will
be dropped at the end of the method's scope.
This would invoke RcuBox::drop immediately, enqueuing a call to
bindings::call_rcu or bindings::kvfree_call_rcu before returning the
pointer. Any subsequent access by C code reading the published pointer
would result in a use-after-free, and calling from_foreign() and dropping
it again would cause a double-free.
> +
> + unsafe fn from_foreign(ptr: *mut c_void) -> Self {
[ ... ]
> +impl<T: Send + 'static> ForeignOwnableRcu for RcuBox<T> {
[Severity: High]
Should this trait implementation also require T: Sync?
The ForeignOwnableRcu trait is designed to allow concurrent RCU readers to
safely borrow the underlying data, with rcu_borrow() returning &'a T.
Because RCU allows multiple threads to execute read-side critical sections
concurrently, multiple CPUs can concurrently call rcu_borrow on the same
pointer. If T is not Sync (e.g., Cell<i32>), this could result in multiple
&T references to a !Sync type across different threads, allowing concurrent
mutation through shared references.
> + type RcuBorrowed<'a> = &'a T;
> +
> + unsafe fn rcu_borrow<'a>(ptr: *mut c_void) -> &'a T {
> + // SAFETY: `RcuBox::drop` can only run after `from_foreign` is called, and the value is
> + // valid until `RcuBox::drop` plus one grace period.
> + unsafe { &(*ptr.cast::<RcuBoxInner<T>>()).value }
> + }
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260530143541.229628-2-phasta@kernel.org?part=2
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/4] rust: Add dma_fence abstractions
2026-05-30 14:35 ` [PATCH 3/4] rust: Add dma_fence abstractions Philipp Stanner
@ 2026-05-30 15:06 ` sashiko-bot
2026-06-01 10:20 ` Alice Ryhl
2026-05-30 15:16 ` Danilo Krummrich
2026-06-01 10:36 ` Alice Ryhl
2 siblings, 1 reply; 35+ messages in thread
From: sashiko-bot @ 2026-05-30 15:06 UTC (permalink / raw)
To: Philipp Stanner; +Cc: linux-media, ojeda
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] DriverFence::drop eagerly drops FenceCtx and user data before the C dma_fence refcount reaches 0, leading to a Use-After-Free.
- [High] FenceCtx::drop unconditionally calls rcu_barrier(), causing a 'scheduling while atomic' panic when a DriverFence is dropped from an interrupt handler or under a spinlock.
- [High] DriverFence::drop() passes a positive error code (ECANCELED) to dma_fence_set_error(), triggering a WARN_ON and misconfiguring the error state.
- [High] DriverFence::as_raw and DriverFenceBorrow::as_raw create exclusive mutable references (&mut) to actively shared data, causing Rust aliasing Undefined Behavior.
--
commit bf1d1a15337629cfe74eaeb7d34f6878de552428
Author: Philipp Stanner <phasta@kernel.org>
Subject: rust: Add dma_fence abstractions
This commit introduces Rust abstractions for the dma_fence framework. It
provides DriverFence, Fence, and FenceCtx types to decouple fence issuers
from consumers and safely manage synchronization primitives required by
GPU drivers.
> diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
> new file mode 100644
> --- /dev/null
> +++ b/rust/kernel/dma_buf/dma_fence.rs
[ ... ]
> +#[pinned_drop]
> +impl<F: Send + Sync, C: Send + Sync> PinnedDrop for FenceCtx<F, C> {
> + fn drop(self: Pin<&mut Self>) {
> + // SAFETY: `rcu_barrier()` is always safe to be called.
> + unsafe { bindings::rcu_barrier() };
[Severity: High]
Does calling rcu_barrier() here risk a scheduling-while-atomic panic?
Since DriverFence::signal() consumes self, the DriverFence will be dropped
at the end of the method. If this happens in an atomic context (such as an
interrupt handler or under a spinlock), and it holds the last reference to
the FenceCtx, dropping it triggers FenceCtx::drop().
Because rcu_barrier() blocks and sleeps until all pending RCU callbacks
complete, this could attempt to sleep in atomic context.
> + }
> +}
[ ... ]
> +impl<F: Send + Sync, C: Send + Sync> DriverFence<F, C> {
> + fn as_raw(&self) -> *mut bindings::dma_fence {
> + // SAFETY: Valid because `self` is valid.
> + let fence_data = unsafe { &mut *self.data.as_ptr() };
[Severity: High]
Does this create an exclusive mutable reference to actively shared memory?
DriverFenceData can be accessed concurrently by other threads holding Fence
references (for instance, when checking if the fence is signaled). Creating
a mutable reference (&mut) in Rust asserts exclusive access and violates
aliasing rules, which allows the compiler to make invalid optimization
assumptions.
Could this use an immutable reference &*self.data.as_ptr() instead?
> +
> + fence_data.inner.inner.get()
> + }
[ ... ]
> +impl<F: Send + Sync, C: Send + Sync> DriverFenceBorrow<F, C> {
> + fn as_raw(&self) -> *mut bindings::dma_fence {
> + // SAFETY: Valid because `self` is valid.
> + let fence_data = unsafe { &mut *self.data.as_ptr() };
[Severity: High]
Similar to DriverFence::as_raw(), does this also incorrectly create a
mutable reference to shared data?
> +
> + fence_data.inner.inner.get()
> + }
[ ... ]
> +impl<F: Send + Sync, C: Send + Sync> Drop for DriverFence<F, C> {
> + fn drop(&mut self) {
> + let fence = self.as_raw();
> + let mut fence_flags: usize = 0;
> + let flag_ptr = &raw mut fence_flags;
> +
> + // 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 the `DriverFence` has been dropped.
> + unsafe {
> + bindings::dma_fence_lock_irqsave(fence, flag_ptr);
> + #[allow(unused_unsafe)]
> + if warn_on!(!bindings::dma_fence_is_signaled_locked(fence)) {
> + bindings::dma_fence_set_error(fence, ECANCELED as i32);
[Severity: High]
Is it intentional to pass a positive error code here?
bindings::ECANCELED evaluates to a positive integer, but
dma_fence_set_error() expects a negative error code (and triggers a WARN_ON
for positive values). A positive value might also be interpreted as success
by callers checking for values < 0.
Should this be - (bindings::ECANCELED as i32)?
> + bindings::dma_fence_signal_locked(fence);
> + }
> + bindings::dma_fence_unlock_irqrestore(fence, flag_ptr);
> + }
> +
> + // SAFETY: `self.data` is owned by the DriverFence, but could be accessed
> + // through some dma_fence callbacks right now. Access is being revoked
> + // above by signalling the fence. The DriverFenceAllowedData trait
> + // ensures that the data either does not need drop, or if it does it
> + // lives in a RcuBox which will delay dropping by one grace period, hence
> + // ensuring that all readers have disappeared.
> + unsafe { drop_in_place(self.data.as_ptr()) };
[Severity: Critical]
Could this eagerly drop FenceCtx and user data before the C dma_fence
refcount reaches 0?
The C dma_fence object can remain alive as long as consumers (like sync_file
or the GPU scheduler) hold references to it. If the backend later invokes
callbacks like ops->get_driver_name(), FenceCtx::get_driver_name() would
attempt to read fctx.driver_name from the already-dropped FenceCtx:
FenceCtx::get_driver_name() {
...
fctx.driver_name.as_char_ptr()
}
Can this result in a use-after-free?
> +
> + // 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 the `DriverFence` has been dropped.
> + unsafe {
> + bindings::dma_fence_put(fence);
> + }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260530143541.229628-2-phasta@kernel.org?part=3
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/4] rust: rcu: add RcuBox type
2026-05-30 14:35 ` [PATCH 2/4] rust: rcu: add RcuBox type Philipp Stanner
2026-05-30 14:54 ` sashiko-bot
@ 2026-05-30 15:08 ` Boqun Feng
2026-05-30 15:27 ` Danilo Krummrich
2026-06-01 7:56 ` Philipp Stanner
1 sibling, 2 replies; 35+ messages in thread
From: Boqun Feng @ 2026-05-30 15:08 UTC (permalink / raw)
To: Philipp Stanner
Cc: Miguel Ojeda, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich,
Sumit Semwal, Christian König, Paul E. McKenney,
Frederic Weisbecker, Neeraj Upadhyay, Joel Fernandes,
Josh Triplett, Uladzislau Rezki, Steven Rostedt,
Mathieu Desnoyers, Lai Jiangshan, Zqiang, Daniel Almeida,
Greg Kroah-Hartman, Igor Korotin, Lorenzo Stoakes,
Alexandre Courbot, FUJITA Tomonori, Krishna Ketan Rai,
Shankari Anand, manos, Boris Brezillon, linux-kernel,
rust-for-linux, linux-media, dri-devel, linaro-mm-sig, rcu
On Sat, May 30, 2026 at 04:35:10PM +0200, Philipp Stanner wrote:
> From: Alice Ryhl <aliceryhl@google.com>
>
> This adds an RcuBox container, which is like KBox except that the value
> is freed with kfree_rcu.
>
> To allow containers to rely on the rcu properties of RcuBox, an
> extension of ForeignOwnable is added.
>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
I have the following on top of Alice's patch. @Alice, @Danilo, thoughts?
Then we can have:
type RcuKBox<T> = RcuBox<T, Kmalloc>;
type RcuVBox<T> = RcuBox<T, Vmalloc>;
and Philipp can use the `RcuKBox` in this patchset. We also need to impl
InPlaceInit for RcuBox, but that can be added later.
Regards,
Boqun
------------->8
Subject: [PATCH] rust: rcu: Make RcuBox generic over Allocator
To support RCU-protected vmalloc allocation, we need to make `RcuBox`
generic over `Allocator`. Currently this works since all `Allocator`s
are either kmalloc() or vmalloc(), and kvfree_call_rcu() works with both
allocations.
While we are at it, add some basic test cases.
Signed-off-by: Boqun Feng <boqun@kernel.org>
---
rust/kernel/sync/rcu/rcu_box.rs | 96 +++++++++++++++++++++++----------
1 file changed, 67 insertions(+), 29 deletions(-)
diff --git a/rust/kernel/sync/rcu/rcu_box.rs b/rust/kernel/sync/rcu/rcu_box.rs
index 2508fdb609ec..5c344d82c0d9 100644
--- a/rust/kernel/sync/rcu/rcu_box.rs
+++ b/rust/kernel/sync/rcu/rcu_box.rs
@@ -4,47 +4,59 @@
//! Provides the `RcuBox` type for Rust allocations that live for a grace period.
-use core::{ops::Deref, ptr::NonNull};
+use core::{
+ marker::PhantomData,
+ ops::Deref,
+ ptr::NonNull, //
+};
use kernel::{
- alloc::{self, AllocError},
+ alloc::{
+ self,
+ AllocError,
+ Allocator, //
+ },
bindings,
ffi::c_void,
prelude::*,
- sync::rcu::{ForeignOwnableRcu, Guard},
types::ForeignOwnable,
};
+use super::{
+ ForeignOwnableRcu,
+ Guard, //
+};
+
/// A box that is freed with rcu.
///
/// The value must be `Send`, as rcu may drop it on another thread.
///
/// # Invariants
///
-/// * The pointer is valid and references a pinned `RcuBoxInner<T>` allocated with `kmalloc`.
+/// * The pointer is valid and references a pinned `RcuBoxInner<T>` allocated with `A`.
/// * This `RcuBox` holds exclusive permissions to rcu free the allocation.
-pub struct RcuBox<T: Send>(NonNull<RcuBoxInner<T>>);
+pub struct RcuBox<T: Send, A: Allocator>(NonNull<RcuBoxInner<T>>, PhantomData<A>);
struct RcuBoxInner<T> {
value: T,
rcu_head: bindings::callback_head,
}
-// Note that `T: Sync` is required since when moving an `RcuBox<T>`, the previous owner may still
-// access `&T` for one grace period.
+// Note that `T: Sync` is required since when moving an `RcuBox<T, A>`, the previous owner may
+// still access `&T` for one grace period.
//
-// SAFETY: Ownership of the `RcuBox<T>` allows for `&T` and dropping the `T`, so `T: Send + Sync`
-// implies `RcuBox<T>: Send`.
-unsafe impl<T: Send + Sync> Send for RcuBox<T> {}
+// SAFETY: Ownership of the `RcuBox<T, A>` allows for `&T` and dropping the `T`, so `T: Send +
+// Sync` implies `RcuBox<T, A>: Send`.
+unsafe impl<T: Send + Sync, A: Allocator> Send for RcuBox<T, A> {}
-// SAFETY: `&RcuBox<T>` allows for no operations other than those permitted by `&T`, so `T: Sync`
-// implies `RcuBox<T>: Sync`.
-unsafe impl<T: Send + Sync> Sync for RcuBox<T> {}
+// SAFETY: `&RcuBox<T, A>` allows for no operations other than those permitted by `&T`, so `T:
+// Sync` implies `RcuBox<T, A>: Sync`.
+unsafe impl<T: Send + Sync, A: Allocator> Sync for RcuBox<T, A> {}
-impl<T: Send> RcuBox<T> {
+impl<T: Send, A: Allocator> RcuBox<T, A> {
/// Create a new `RcuBox`.
pub fn new(x: T, flags: alloc::Flags) -> Result<Self, AllocError> {
- let b = KBox::new(
+ let b = Box::<_, A>::new(
RcuBoxInner {
value: x,
rcu_head: Default::default(),
@@ -53,9 +65,9 @@ pub fn new(x: T, flags: alloc::Flags) -> Result<Self, AllocError> {
)?;
// INVARIANT:
- // * The pointer contains a valid `RcuBoxInner` allocated with `kmalloc`.
+ // * The pointer contains a valid `RcuBoxInner` allocated with `A`.
// * We just allocated it, so we own free permissions.
- Ok(RcuBox(NonNull::from(KBox::leak(b))))
+ Ok(RcuBox(NonNull::from(Box::leak(b)), PhantomData))
}
/// Access the value for a grace period.
@@ -66,7 +78,7 @@ pub fn with_rcu<'rcu>(&self, _read_guard: &'rcu Guard) -> &'rcu T {
}
}
-impl<T: Send> Deref for RcuBox<T> {
+impl<T: Send, A: Allocator> Deref for RcuBox<T, A> {
type Target = T;
fn deref(&self) -> &T {
// SAFETY: While the `RcuBox<T>` exists, the value remains valid.
@@ -75,10 +87,10 @@ fn deref(&self) -> &T {
}
// SAFETY:
-// * The `RcuBoxInner<T>` was allocated with `kmalloc`.
+// * The `RcuBoxInner<T>` was allocated with `A`.
// * `NonNull::as_ptr` returns a non-null pointer.
-unsafe impl<T: Send + 'static> ForeignOwnable for RcuBox<T> {
- const FOREIGN_ALIGN: usize = <KBox<RcuBoxInner<T>> as ForeignOwnable>::FOREIGN_ALIGN;
+unsafe impl<T: Send + 'static, A: Allocator> ForeignOwnable for RcuBox<T, A> {
+ const FOREIGN_ALIGN: usize = <Box<RcuBoxInner<T>, A> as ForeignOwnable>::FOREIGN_ALIGN;
type Borrowed<'a> = &'a T;
type BorrowedMut<'a> = &'a T;
@@ -88,9 +100,9 @@ fn into_foreign(self) -> *mut c_void {
}
unsafe fn from_foreign(ptr: *mut c_void) -> Self {
- // INVARIANT: Pointer returned by `into_foreign` carries same invariants as `RcuBox<T>`.
+ // INVARIANT: Pointer returned by `into_foreign, A` carries same invariants as `RcuBox<T>`.
// SAFETY: `into_foreign` never returns a null pointer.
- Self(unsafe { NonNull::new_unchecked(ptr.cast()) })
+ Self(unsafe { NonNull::new_unchecked(ptr.cast()) }, PhantomData)
}
unsafe fn borrow<'a>(ptr: *mut c_void) -> &'a T {
@@ -104,7 +116,7 @@ unsafe fn borrow_mut<'a>(ptr: *mut c_void) -> &'a T {
}
}
-impl<T: Send + 'static> ForeignOwnableRcu for RcuBox<T> {
+impl<T: Send + 'static, A: Allocator> ForeignOwnableRcu for RcuBox<T, A> {
type RcuBorrowed<'a> = &'a T;
unsafe fn rcu_borrow<'a>(ptr: *mut c_void) -> &'a T {
@@ -114,7 +126,7 @@ unsafe fn rcu_borrow<'a>(ptr: *mut c_void) -> &'a T {
}
}
-impl<T: Send> Drop for RcuBox<T> {
+impl<T: Send, A: Allocator> Drop for RcuBox<T, A> {
fn drop(&mut self) {
// SAFETY: The `rcu_head` field is in-bounds of a valid allocation.
let rcu_head = unsafe { &raw mut (*self.0.as_ptr()).rcu_head };
@@ -122,9 +134,11 @@ fn drop(&mut self) {
// SAFETY: `rcu_head` is the `rcu_head` field of `RcuBoxInner<T>`. All users will be
// gone in an rcu grace period. This is the destructor, so we may pass ownership of the
// allocation.
- unsafe { bindings::call_rcu(rcu_head, Some(drop_rcu_box::<T>)) };
+ unsafe { bindings::call_rcu(rcu_head, Some(drop_rcu_box::<T, A>)) };
} else {
// SAFETY: All users will be gone in an rcu grace period.
+ // TODO: We are luckily since `kvfree_call_rcu()` works on both kmalloc and vmalloc,
+ // maybe a new `Allocator` method is needed.
unsafe { bindings::kvfree_call_rcu(rcu_head, self.0.as_ptr().cast()) };
}
}
@@ -135,11 +149,35 @@ fn drop(&mut self) {
/// # Safety
///
/// `head` references the `rcu_head` field of an `RcuBoxInner<T>` that has no references to it.
-/// Ownership of the `KBox<RcuBoxInner<T>>` must be passed.
-unsafe extern "C" fn drop_rcu_box<T>(head: *mut bindings::callback_head) {
+/// Ownership of the `Box<RcuBoxInner<T>, A>` must be passed.
+unsafe extern "C" fn drop_rcu_box<T, A: Allocator>(head: *mut bindings::callback_head) {
// SAFETY: Caller provides a pointer to the `rcu_head` field of a `RcuBoxInner<T>`.
let box_inner = unsafe { crate::container_of!(head, RcuBoxInner<T>, rcu_head) };
// SAFETY: Caller ensures exclusive access and passed ownership.
- drop(unsafe { KBox::from_raw(box_inner) });
+ drop(unsafe { Box::<_, A>::from_raw(box_inner) });
+}
+
+#[kunit_tests(rust_rcu_box)]
+mod tests {
+ use super::*;
+
+ #[test]
+ fn rcu_box_basic() -> Result {
+ let rb = RcuBox::<_, alloc::allocator::Kmalloc>::new(42i32, alloc::flags::GFP_KERNEL)?;
+
+ assert_eq!(*rb, 42);
+ assert_eq!(*rb.with_rcu(&Guard::new()), 42);
+
+ drop(rb);
+
+ let rb = RcuBox::<_, alloc::allocator::Vmalloc>::new(42i32, alloc::flags::GFP_KERNEL)?;
+
+ assert_eq!(*rb, 42);
+ assert_eq!(*rb.with_rcu(&Guard::new()), 42);
+
+ drop(rb);
+
+ Ok(())
+ }
}
--
2.50.1 (Apple Git-155)
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 3/4] rust: Add dma_fence abstractions
2026-05-30 14:35 ` [PATCH 3/4] rust: Add dma_fence abstractions Philipp Stanner
2026-05-30 15:06 ` sashiko-bot
@ 2026-05-30 15:16 ` Danilo Krummrich
2026-06-01 8:46 ` Philipp Stanner
2026-06-01 10:36 ` Alice Ryhl
2 siblings, 1 reply; 35+ messages in thread
From: Danilo Krummrich @ 2026-05-30 15:16 UTC (permalink / raw)
To: Philipp Stanner
Cc: Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Sumit Semwal, Christian König, Paul E. McKenney,
Frederic Weisbecker, Neeraj Upadhyay, Joel Fernandes,
Josh Triplett, Uladzislau Rezki, Steven Rostedt,
Mathieu Desnoyers, Lai Jiangshan, Zqiang, Daniel Almeida,
Greg Kroah-Hartman, Igor Korotin, Lorenzo Stoakes,
Alexandre Courbot, FUJITA Tomonori, Krishna Ketan Rai,
Shankari Anand, manos, Boris Brezillon, linux-kernel,
rust-for-linux, linux-media, dri-devel, linaro-mm-sig, rcu
(Not a full review, but a few drive-by comments.)
On Sat May 30, 2026 at 4:35 PM CEST, Philipp Stanner wrote:
> +#[allow(unused_unsafe)]
What is this needed for?
> +impl<F: Send + Sync + DriverFenceAllowedData, C: Send + Sync> FenceCtx<F, C> {
<snip>
> +impl<F: Send + Sync, C: Send + Sync> PinnedDrop for FenceCtx<F, C> {
> + fn drop(self: Pin<&mut Self>) {
> + // SAFETY: `rcu_barrier()` is always safe to be called.
> + unsafe { bindings::rcu_barrier() };
We should probably add a safe function for this.
> +impl<T: FenceCb> FenceCbRegistration<T> {
> + /// Register a callback on a fence.
> + ///
> + /// 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,
> + {
> + // Uses `pin_init_from_closure` instead of `try_pin_init!` so that on
> + // `-ENOENT` (already signaled) the callback can be read back from the
> + // partially-initialized slot and returned through the error.
Seems a bit odd that this needs pin_init_from_closure(). You can still use
try_pin_init!() with &this in Self an a _: initializer at the end in the worst
case. But the fence and callback fields should be fine to initialize "normally"?
> + //
> + // SAFETY: `pin_init_from_closure` requires:
> + // - On `Ok(())`: the slot is fully initialized and valid for `Drop`.
> + // - On `Err(_)`: the slot is clean, i.e.: no partially-initialized fields
> + // remain, and the slot can be deallocated without dropping.
> + //
> + // We uphold this as follows:
> + // - On success: all three fields are initialized. Ok(()) is returned.
> + // - On ENOENT (already signaled): `callback` and `fence` are read back
> + // from the slot via `ptr::read`, leaving the slot clean. `cb` was
> + // initialized by `dma_fence_add_callback` (it calls
> + // `INIT_LIST_HEAD(&cb->node)` even on error), but `cb` is
> + // `Opaque<dma_fence_cb>` which has no `Drop`, so not dropping it is
> + // fine. The callback is returned through `AlreadySignaled(T)`.
> + // - On other errors: same cleanup as ENOENT, error returned as
> + // `Other(e)`.
> + unsafe {
> + pin_init_from_closure(move |slot: *mut Self| {
> + let slot_callback = &raw mut (*slot).callback;
> + let slot_fence = &raw mut (*slot).fence;
> + let slot_cb = &raw mut (*slot).cb;
> +
> + // Write callback and fence first — must be visible before
> + // dma_fence_add_callback makes the registration live.
> + core::ptr::write(slot_callback, callback);
> + core::ptr::write(slot_fence, ARef::from(fence));
> +
> + let ret = to_result(bindings::dma_fence_add_callback(
> + fence.inner.get(),
> + Opaque::cast_into(slot_cb),
> + Some(Self::dma_fence_callback),
> + ));
> +
> + match ret {
> + Ok(()) => Ok(()),
> + Err(e) => {
> + // Read back what we wrote to leave the slot clean.
> + let cb_back = core::ptr::read(slot_callback);
> + let _fence_back = core::ptr::read(slot_fence);
What's the purpose of _fence_back?
> +
> + if e.to_errno() == ENOENT.to_errno() {
> + Err(CallbackError::AlreadySignaled(cb_back))
> + } else {
> + Err(CallbackError::Other(e))
> + }
> + }
> + }
> + })
> + }
> + }
> + /// 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: 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 the `DriverFence` has been dropped.
> + unsafe {
> + bindings::dma_fence_lock_irqsave(fence, flag_ptr);
> + if !bindings::dma_fence_is_signaled_locked(fence) {
> + if let Err(err) = res {
> + bindings::dma_fence_set_error(fence, err.to_errno());
> + }
> + bindings::dma_fence_signal_locked(fence);
> + }
> + bindings::dma_fence_unlock_irqrestore(fence, flag_ptr);
> + }
Please use a single unsafe block per unsafe function call, here and in a few
other places.
> + }
> +}
> +
> +// SAFETY: Fences are literally designed to be shared between threads.
> +unsafe impl<F: Send + Sync, C: Send + Sync> Send for DriverFence<F, C> {}
> +
> +impl<F: Send + Sync, C: Send + Sync> Deref for DriverFence<F, C> {
> + type Target = F;
> +
> + fn deref(&self) -> &Self::Target {
> + // SAFETY: Thanks to refcounting, `data` is always valid as long as `self` is.
> + let data = unsafe { &*self.data.as_ptr() };
> +
> + &data.data
> + }
> +}
> +
> +/// A borrowed [`DriverFence`]. All you can do with it is access your user data
> +/// and obtain a [`Fence`].
> +pub struct DriverFenceBorrow<F: Send + Sync, C: Send + Sync> {
This misses the lifetime bound, which is the purpose of this struct.
> + /// 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<F, C>>,
Why not use ManuallyDrop<DriverFence>? This way you would only need a Deref impl
to &'a DriverFence.
This way you basically reimplement the DriverFence type just without the
destructor.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 4/4] MAINTAINERS: Add entry for Rust dma-buf
2026-05-30 14:35 ` [PATCH 4/4] MAINTAINERS: Add entry for Rust dma-buf Philipp Stanner
@ 2026-05-30 15:20 ` Danilo Krummrich
0 siblings, 0 replies; 35+ messages in thread
From: Danilo Krummrich @ 2026-05-30 15:20 UTC (permalink / raw)
To: Philipp Stanner
Cc: Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Sumit Semwal, Christian König, Paul E. McKenney,
Frederic Weisbecker, Neeraj Upadhyay, Joel Fernandes,
Josh Triplett, Uladzislau Rezki, Steven Rostedt,
Mathieu Desnoyers, Lai Jiangshan, Zqiang, Daniel Almeida,
Greg Kroah-Hartman, Igor Korotin, Lorenzo Stoakes,
Alexandre Courbot, FUJITA Tomonori, Krishna Ketan Rai,
Shankari Anand, manos, Boris Brezillon, linux-kernel,
rust-for-linux, linux-media, dri-devel, linaro-mm-sig, rcu
On Sat May 30, 2026 at 4:35 PM CEST, Philipp Stanner wrote:
> @@ -7529,6 +7530,7 @@ T: git https://gitlab.freedesktop.org/drm/misc/kernel.git
> F: Documentation/driver-api/dma-buf.rst
> F: Documentation/userspace-api/dma-buf-alloc-exchange.rst
> F: drivers/dma-buf/
> +F: rust/kernel/dma_buf/
Please also add rust/helpers/dma_fence.c.
Given that dma-buf goes through drm-misc, we should probably also add those file
to the drm-rust entry.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/4] rust: rcu: add RcuBox type
2026-05-30 15:08 ` Boqun Feng
@ 2026-05-30 15:27 ` Danilo Krummrich
2026-06-01 7:56 ` Philipp Stanner
1 sibling, 0 replies; 35+ messages in thread
From: Danilo Krummrich @ 2026-05-30 15:27 UTC (permalink / raw)
To: Boqun Feng
Cc: Philipp Stanner, Miguel Ojeda, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Sumit Semwal, Christian König, Paul E. McKenney,
Frederic Weisbecker, Neeraj Upadhyay, Joel Fernandes,
Josh Triplett, Uladzislau Rezki, Steven Rostedt,
Mathieu Desnoyers, Lai Jiangshan, Zqiang, Daniel Almeida,
Greg Kroah-Hartman, Igor Korotin, Lorenzo Stoakes,
Alexandre Courbot, FUJITA Tomonori, Krishna Ketan Rai,
Shankari Anand, manos, Boris Brezillon, linux-kernel,
rust-for-linux, linux-media, dri-devel, linaro-mm-sig, rcu
On Sat May 30, 2026 at 5:08 PM CEST, Boqun Feng wrote:
> type RcuKBox<T> = RcuBox<T, Kmalloc>;
> type RcuVBox<T> = RcuBox<T, Vmalloc>;
type RcuKVBox<T> = RcuBox<T, KVmalloc>;
> To support RCU-protected vmalloc allocation, we need to make `RcuBox`
> generic over `Allocator`. Currently this works since all `Allocator`s
> are either kmalloc() or vmalloc(), and kvfree_call_rcu() works with both
> allocations.
I think we can add Allocator::free_call_rcu().
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/4] rust: rcu: add RcuBox type
2026-05-30 15:08 ` Boqun Feng
2026-05-30 15:27 ` Danilo Krummrich
@ 2026-06-01 7:56 ` Philipp Stanner
2026-06-01 13:41 ` Boqun Feng
1 sibling, 1 reply; 35+ messages in thread
From: Philipp Stanner @ 2026-06-01 7:56 UTC (permalink / raw)
To: Boqun Feng, Philipp Stanner
Cc: Miguel Ojeda, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich,
Sumit Semwal, Christian König, Paul E. McKenney,
Frederic Weisbecker, Neeraj Upadhyay, Joel Fernandes,
Josh Triplett, Uladzislau Rezki, Steven Rostedt,
Mathieu Desnoyers, Lai Jiangshan, Zqiang, Daniel Almeida,
Greg Kroah-Hartman, Igor Korotin, Lorenzo Stoakes,
Alexandre Courbot, FUJITA Tomonori, Krishna Ketan Rai,
Shankari Anand, manos, Boris Brezillon, linux-kernel,
rust-for-linux, linux-media, dri-devel, linaro-mm-sig, rcu
On Sat, 2026-05-30 at 08:08 -0700, Boqun Feng wrote:
> On Sat, May 30, 2026 at 04:35:10PM +0200, Philipp Stanner wrote:
> > From: Alice Ryhl <aliceryhl@google.com>
> >
> > This adds an RcuBox container, which is like KBox except that the value
> > is freed with kfree_rcu.
> >
> > To allow containers to rely on the rcu properties of RcuBox, an
> > extension of ForeignOwnable is added.
> >
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > ---
>
> I have the following on top of Alice's patch. @Alice, @Danilo, thoughts?
>
> Then we can have:
>
> type RcuKBox<T> = RcuBox<T, Kmalloc>;
> type RcuVBox<T> = RcuBox<T, Vmalloc>;
No objections by me.
I just think we have to decide how the treat the namespaces, though.
Probably Alice wrote it like that so that it's very apparent that this
is not a normal box. It still breaks the naming convention in my
opinion.
rcu::Box vs rcu::RcuBox
With all other subsystems, naming like that seems not allowed.
dma::Fence vs dma::DmaFence
I probably would allow the user to decide whether he wants to just use
it as `rcu::Box` in all his code.
But no hard feelings.
>
> and Philipp can use the `RcuKBox` in this patchset. We also need to impl
> InPlaceInit for RcuBox, but that can be added later.
So shall we merge my series with Alice's patch, and later we add your
patch and other features, or would you prefer to have the additional
boxes from your patch from the get-go?
P.
>
> Regards,
> Boqun
>
> ------------->8
> Subject: [PATCH] rust: rcu: Make RcuBox generic over Allocator
>
> To support RCU-protected vmalloc allocation, we need to make `RcuBox`
> generic over `Allocator`. Currently this works since all `Allocator`s
> are either kmalloc() or vmalloc(), and kvfree_call_rcu() works with both
> allocations.
>
> While we are at it, add some basic test cases.
>
> Signed-off-by: Boqun Feng <boqun@kernel.org>
> ---
> rust/kernel/sync/rcu/rcu_box.rs | 96 +++++++++++++++++++++++----------
> 1 file changed, 67 insertions(+), 29 deletions(-)
>
> diff --git a/rust/kernel/sync/rcu/rcu_box.rs b/rust/kernel/sync/rcu/rcu_box.rs
> index 2508fdb609ec..5c344d82c0d9 100644
> --- a/rust/kernel/sync/rcu/rcu_box.rs
> +++ b/rust/kernel/sync/rcu/rcu_box.rs
> @@ -4,47 +4,59 @@
>
> //! Provides the `RcuBox` type for Rust allocations that live for a grace period.
>
> -use core::{ops::Deref, ptr::NonNull};
> +use core::{
> + marker::PhantomData,
> + ops::Deref,
> + ptr::NonNull, //
> +};
>
> use kernel::{
> - alloc::{self, AllocError},
> + alloc::{
> + self,
> + AllocError,
> + Allocator, //
> + },
> bindings,
> ffi::c_void,
> prelude::*,
> - sync::rcu::{ForeignOwnableRcu, Guard},
> types::ForeignOwnable,
> };
>
> +use super::{
> + ForeignOwnableRcu,
> + Guard, //
> +};
> +
> /// A box that is freed with rcu.
> ///
> /// The value must be `Send`, as rcu may drop it on another thread.
> ///
> /// # Invariants
> ///
> -/// * The pointer is valid and references a pinned `RcuBoxInner<T>` allocated with `kmalloc`.
> +/// * The pointer is valid and references a pinned `RcuBoxInner<T>` allocated with `A`.
> /// * This `RcuBox` holds exclusive permissions to rcu free the allocation.
> -pub struct RcuBox<T: Send>(NonNull<RcuBoxInner<T>>);
> +pub struct RcuBox<T: Send, A: Allocator>(NonNull<RcuBoxInner<T>>, PhantomData<A>);
>
> struct RcuBoxInner<T> {
> value: T,
> rcu_head: bindings::callback_head,
> }
>
> -// Note that `T: Sync` is required since when moving an `RcuBox<T>`, the previous owner may still
> -// access `&T` for one grace period.
> +// Note that `T: Sync` is required since when moving an `RcuBox<T, A>`, the previous owner may
> +// still access `&T` for one grace period.
> //
> -// SAFETY: Ownership of the `RcuBox<T>` allows for `&T` and dropping the `T`, so `T: Send + Sync`
> -// implies `RcuBox<T>: Send`.
> -unsafe impl<T: Send + Sync> Send for RcuBox<T> {}
> +// SAFETY: Ownership of the `RcuBox<T, A>` allows for `&T` and dropping the `T`, so `T: Send +
> +// Sync` implies `RcuBox<T, A>: Send`.
> +unsafe impl<T: Send + Sync, A: Allocator> Send for RcuBox<T, A> {}
>
> -// SAFETY: `&RcuBox<T>` allows for no operations other than those permitted by `&T`, so `T: Sync`
> -// implies `RcuBox<T>: Sync`.
> -unsafe impl<T: Send + Sync> Sync for RcuBox<T> {}
> +// SAFETY: `&RcuBox<T, A>` allows for no operations other than those permitted by `&T`, so `T:
> +// Sync` implies `RcuBox<T, A>: Sync`.
> +unsafe impl<T: Send + Sync, A: Allocator> Sync for RcuBox<T, A> {}
>
> -impl<T: Send> RcuBox<T> {
> +impl<T: Send, A: Allocator> RcuBox<T, A> {
> /// Create a new `RcuBox`.
> pub fn new(x: T, flags: alloc::Flags) -> Result<Self, AllocError> {
> - let b = KBox::new(
> + let b = Box::<_, A>::new(
> RcuBoxInner {
> value: x,
> rcu_head: Default::default(),
> @@ -53,9 +65,9 @@ pub fn new(x: T, flags: alloc::Flags) -> Result<Self, AllocError> {
> )?;
>
> // INVARIANT:
> - // * The pointer contains a valid `RcuBoxInner` allocated with `kmalloc`.
> + // * The pointer contains a valid `RcuBoxInner` allocated with `A`.
> // * We just allocated it, so we own free permissions.
> - Ok(RcuBox(NonNull::from(KBox::leak(b))))
> + Ok(RcuBox(NonNull::from(Box::leak(b)), PhantomData))
> }
>
> /// Access the value for a grace period.
> @@ -66,7 +78,7 @@ pub fn with_rcu<'rcu>(&self, _read_guard: &'rcu Guard) -> &'rcu T {
> }
> }
>
> -impl<T: Send> Deref for RcuBox<T> {
> +impl<T: Send, A: Allocator> Deref for RcuBox<T, A> {
> type Target = T;
> fn deref(&self) -> &T {
> // SAFETY: While the `RcuBox<T>` exists, the value remains valid.
> @@ -75,10 +87,10 @@ fn deref(&self) -> &T {
> }
>
> // SAFETY:
> -// * The `RcuBoxInner<T>` was allocated with `kmalloc`.
> +// * The `RcuBoxInner<T>` was allocated with `A`.
> // * `NonNull::as_ptr` returns a non-null pointer.
> -unsafe impl<T: Send + 'static> ForeignOwnable for RcuBox<T> {
> - const FOREIGN_ALIGN: usize = <KBox<RcuBoxInner<T>> as ForeignOwnable>::FOREIGN_ALIGN;
> +unsafe impl<T: Send + 'static, A: Allocator> ForeignOwnable for RcuBox<T, A> {
> + const FOREIGN_ALIGN: usize = <Box<RcuBoxInner<T>, A> as ForeignOwnable>::FOREIGN_ALIGN;
>
> type Borrowed<'a> = &'a T;
> type BorrowedMut<'a> = &'a T;
> @@ -88,9 +100,9 @@ fn into_foreign(self) -> *mut c_void {
> }
>
> unsafe fn from_foreign(ptr: *mut c_void) -> Self {
> - // INVARIANT: Pointer returned by `into_foreign` carries same invariants as `RcuBox<T>`.
> + // INVARIANT: Pointer returned by `into_foreign, A` carries same invariants as `RcuBox<T>`.
> // SAFETY: `into_foreign` never returns a null pointer.
> - Self(unsafe { NonNull::new_unchecked(ptr.cast()) })
> + Self(unsafe { NonNull::new_unchecked(ptr.cast()) }, PhantomData)
> }
>
> unsafe fn borrow<'a>(ptr: *mut c_void) -> &'a T {
> @@ -104,7 +116,7 @@ unsafe fn borrow_mut<'a>(ptr: *mut c_void) -> &'a T {
> }
> }
>
> -impl<T: Send + 'static> ForeignOwnableRcu for RcuBox<T> {
> +impl<T: Send + 'static, A: Allocator> ForeignOwnableRcu for RcuBox<T, A> {
> type RcuBorrowed<'a> = &'a T;
>
> unsafe fn rcu_borrow<'a>(ptr: *mut c_void) -> &'a T {
> @@ -114,7 +126,7 @@ unsafe fn rcu_borrow<'a>(ptr: *mut c_void) -> &'a T {
> }
> }
>
> -impl<T: Send> Drop for RcuBox<T> {
> +impl<T: Send, A: Allocator> Drop for RcuBox<T, A> {
> fn drop(&mut self) {
> // SAFETY: The `rcu_head` field is in-bounds of a valid allocation.
> let rcu_head = unsafe { &raw mut (*self.0.as_ptr()).rcu_head };
> @@ -122,9 +134,11 @@ fn drop(&mut self) {
> // SAFETY: `rcu_head` is the `rcu_head` field of `RcuBoxInner<T>`. All users will be
> // gone in an rcu grace period. This is the destructor, so we may pass ownership of the
> // allocation.
> - unsafe { bindings::call_rcu(rcu_head, Some(drop_rcu_box::<T>)) };
> + unsafe { bindings::call_rcu(rcu_head, Some(drop_rcu_box::<T, A>)) };
> } else {
> // SAFETY: All users will be gone in an rcu grace period.
> + // TODO: We are luckily since `kvfree_call_rcu()` works on both kmalloc and vmalloc,
> + // maybe a new `Allocator` method is needed.
> unsafe { bindings::kvfree_call_rcu(rcu_head, self.0.as_ptr().cast()) };
> }
> }
> @@ -135,11 +149,35 @@ fn drop(&mut self) {
> /// # Safety
> ///
> /// `head` references the `rcu_head` field of an `RcuBoxInner<T>` that has no references to it.
> -/// Ownership of the `KBox<RcuBoxInner<T>>` must be passed.
> -unsafe extern "C" fn drop_rcu_box<T>(head: *mut bindings::callback_head) {
> +/// Ownership of the `Box<RcuBoxInner<T>, A>` must be passed.
> +unsafe extern "C" fn drop_rcu_box<T, A: Allocator>(head: *mut bindings::callback_head) {
> // SAFETY: Caller provides a pointer to the `rcu_head` field of a `RcuBoxInner<T>`.
> let box_inner = unsafe { crate::container_of!(head, RcuBoxInner<T>, rcu_head) };
>
> // SAFETY: Caller ensures exclusive access and passed ownership.
> - drop(unsafe { KBox::from_raw(box_inner) });
> + drop(unsafe { Box::<_, A>::from_raw(box_inner) });
> +}
> +
> +#[kunit_tests(rust_rcu_box)]
> +mod tests {
> + use super::*;
> +
> + #[test]
> + fn rcu_box_basic() -> Result {
> + let rb = RcuBox::<_, alloc::allocator::Kmalloc>::new(42i32, alloc::flags::GFP_KERNEL)?;
> +
> + assert_eq!(*rb, 42);
> + assert_eq!(*rb.with_rcu(&Guard::new()), 42);
> +
> + drop(rb);
> +
> + let rb = RcuBox::<_, alloc::allocator::Vmalloc>::new(42i32, alloc::flags::GFP_KERNEL)?;
> +
> + assert_eq!(*rb, 42);
> + assert_eq!(*rb.with_rcu(&Guard::new()), 42);
> +
> + drop(rb);
> +
> + Ok(())
> + }
> }
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/4] rust: Add dma_fence abstractions
2026-05-30 15:16 ` Danilo Krummrich
@ 2026-06-01 8:46 ` Philipp Stanner
2026-06-01 10:13 ` Danilo Krummrich
0 siblings, 1 reply; 35+ messages in thread
From: Philipp Stanner @ 2026-06-01 8:46 UTC (permalink / raw)
To: Danilo Krummrich, Philipp Stanner
Cc: Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Sumit Semwal, Christian König, Paul E. McKenney,
Frederic Weisbecker, Neeraj Upadhyay, Joel Fernandes,
Josh Triplett, Uladzislau Rezki, Steven Rostedt,
Mathieu Desnoyers, Lai Jiangshan, Zqiang, Daniel Almeida,
Greg Kroah-Hartman, Igor Korotin, Lorenzo Stoakes,
Alexandre Courbot, FUJITA Tomonori, Krishna Ketan Rai,
Shankari Anand, manos, Boris Brezillon, linux-kernel,
rust-for-linux, linux-media, dri-devel, linaro-mm-sig, rcu
On Sat, 2026-05-30 at 17:16 +0200, Danilo Krummrich wrote:
> (Not a full review, but a few drive-by comments.)
>
> On Sat May 30, 2026 at 4:35 PM CEST, Philipp Stanner wrote:
> > +#[allow(unused_unsafe)]
>
> What is this needed for?
You know that :-P
>
> > +impl<F: Send + Sync + DriverFenceAllowedData, C: Send + Sync> FenceCtx<F, C> {
>
> <snip>
>
> > +impl<F: Send + Sync, C: Send + Sync> PinnedDrop for FenceCtx<F, C> {
> > + fn drop(self: Pin<&mut Self>) {
> > + // SAFETY: `rcu_barrier()` is always safe to be called.
> > + unsafe { bindings::rcu_barrier() };
>
> We should probably add a safe function for this.
ACK.
>
> > +impl<T: FenceCb> FenceCbRegistration<T> {
> > + /// Register a callback on a fence.
> > + ///
> > + /// 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,
> > + {
> > + // Uses `pin_init_from_closure` instead of `try_pin_init!` so that on
> > + // `-ENOENT` (already signaled) the callback can be read back from the
> > + // partially-initialized slot and returned through the error.
>
> Seems a bit odd that this needs pin_init_from_closure(). You can still use
> try_pin_init!() with &this in Self an a _: initializer at the end in the worst
> case. But the fence and callback fields should be fine to initialize "normally"?
I'll investigate that.
>
> > + //
> > + // SAFETY: `pin_init_from_closure` requires:
> > + // - On `Ok(())`: the slot is fully initialized and valid for `Drop`.
> > + // - On `Err(_)`: the slot is clean, i.e.: no partially-initialized fields
> > + // remain, and the slot can be deallocated without dropping.
> > + //
> > + // We uphold this as follows:
> > + // - On success: all three fields are initialized. Ok(()) is returned.
> > + // - On ENOENT (already signaled): `callback` and `fence` are read back
> > + // from the slot via `ptr::read`, leaving the slot clean. `cb` was
> > + // initialized by `dma_fence_add_callback` (it calls
> > + // `INIT_LIST_HEAD(&cb->node)` even on error), but `cb` is
> > + // `Opaque<dma_fence_cb>` which has no `Drop`, so not dropping it is
> > + // fine. The callback is returned through `AlreadySignaled(T)`.
> > + // - On other errors: same cleanup as ENOENT, error returned as
> > + // `Other(e)`.
> > + unsafe {
> > + pin_init_from_closure(move |slot: *mut Self| {
> > + let slot_callback = &raw mut (*slot).callback;
> > + let slot_fence = &raw mut (*slot).fence;
> > + let slot_cb = &raw mut (*slot).cb;
> > +
> > + // Write callback and fence first — must be visible before
> > + // dma_fence_add_callback makes the registration live.
> > + core::ptr::write(slot_callback, callback);
> > + core::ptr::write(slot_fence, ARef::from(fence));
> > +
> > + let ret = to_result(bindings::dma_fence_add_callback(
> > + fence.inner.get(),
> > + Opaque::cast_into(slot_cb),
> > + Some(Self::dma_fence_callback),
> > + ));
> > +
> > + match ret {
> > + Ok(()) => Ok(()),
> > + Err(e) => {
> > + // Read back what we wrote to leave the slot clean.
> > + let cb_back = core::ptr::read(slot_callback);
> > + let _fence_back = core::ptr::read(slot_fence);
>
> What's the purpose of _fence_back?
Relic. Will rework.
>
> > +
> > + if e.to_errno() == ENOENT.to_errno() {
> > + Err(CallbackError::AlreadySignaled(cb_back))
> > + } else {
> > + Err(CallbackError::Other(e))
> > + }
> > + }
> > + }
> > + })
> > + }
> > + }
> > + /// 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: 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 the `DriverFence` has been dropped.
> > + unsafe {
> > + bindings::dma_fence_lock_irqsave(fence, flag_ptr);
> > + if !bindings::dma_fence_is_signaled_locked(fence) {
> > + if let Err(err) = res {
> > + bindings::dma_fence_set_error(fence, err.to_errno());
> > + }
> > + bindings::dma_fence_signal_locked(fence);
> > + }
> > + bindings::dma_fence_unlock_irqrestore(fence, flag_ptr);
> > + }
>
> Please use a single unsafe block per unsafe function call, here and in a few
> other places.
Is that an official rule? If so, the linters should inform about it.
At first glance, I don't see any advantage to it and the disadvantage
of greatly reducing readability.
>
> > + }
> > +}
> > +
> > +// SAFETY: Fences are literally designed to be shared between threads.
> > +unsafe impl<F: Send + Sync, C: Send + Sync> Send for DriverFence<F, C> {}
> > +
> > +impl<F: Send + Sync, C: Send + Sync> Deref for DriverFence<F, C> {
> > + type Target = F;
> > +
> > + fn deref(&self) -> &Self::Target {
> > + // SAFETY: Thanks to refcounting, `data` is always valid as long as `self` is.
> > + let data = unsafe { &*self.data.as_ptr() };
> > +
> > + &data.data
> > + }
> > +}
> > +
> > +/// A borrowed [`DriverFence`]. All you can do with it is access your user data
> > +/// and obtain a [`Fence`].
> > +pub struct DriverFenceBorrow<F: Send + Sync, C: Send + Sync> {
>
> This misses the lifetime bound, which is the purpose of this struct.
>
> > + /// 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<F, C>>,
>
> Why not use ManuallyDrop<DriverFence>? This way you would only need a Deref impl
> to &'a DriverFence.
>
> This way you basically reimplement the DriverFence type just without the
> destructor.
Good idea, will do.
P.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/4] rust: types: implement ForeignOwnable for ARef<T>
2026-05-30 14:35 ` [PATCH 1/4] rust: types: implement ForeignOwnable for ARef<T> Philipp Stanner
2026-05-30 14:45 ` sashiko-bot
@ 2026-06-01 9:46 ` Alice Ryhl
1 sibling, 0 replies; 35+ messages in thread
From: Alice Ryhl @ 2026-06-01 9:46 UTC (permalink / raw)
To: Philipp Stanner
Cc: Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Trevor Gross, Danilo Krummrich,
Sumit Semwal, Christian König, Paul E. McKenney,
Frederic Weisbecker, Neeraj Upadhyay, Joel Fernandes,
Josh Triplett, Uladzislau Rezki, Steven Rostedt,
Mathieu Desnoyers, Lai Jiangshan, Zqiang, Daniel Almeida,
Greg Kroah-Hartman, Igor Korotin, Lorenzo Stoakes,
Alexandre Courbot, FUJITA Tomonori, Krishna Ketan Rai,
Shankari Anand, manos, Boris Brezillon, linux-kernel,
rust-for-linux, linux-media, dri-devel, linaro-mm-sig, rcu
On Sat, May 30, 2026 at 04:35:09PM +0200, Philipp Stanner wrote:
> From: Danilo Krummrich <dakr@kernel.org>
>
> 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.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/4] rust: Add dma_fence abstractions
2026-06-01 8:46 ` Philipp Stanner
@ 2026-06-01 10:13 ` Danilo Krummrich
0 siblings, 0 replies; 35+ messages in thread
From: Danilo Krummrich @ 2026-06-01 10:13 UTC (permalink / raw)
To: Philipp Stanner
Cc: phasta, Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Sumit Semwal, Christian König, Paul E. McKenney,
Frederic Weisbecker, Neeraj Upadhyay, Joel Fernandes,
Josh Triplett, Uladzislau Rezki, Steven Rostedt,
Mathieu Desnoyers, Lai Jiangshan, Zqiang, Daniel Almeida,
Greg Kroah-Hartman, Igor Korotin, Lorenzo Stoakes,
Alexandre Courbot, FUJITA Tomonori, Krishna Ketan Rai,
Shankari Anand, manos, Boris Brezillon, linux-kernel,
rust-for-linux, linux-media, dri-devel, linaro-mm-sig, rcu
On Mon Jun 1, 2026 at 10:46 AM CEST, Philipp Stanner wrote:
> On Sat, 2026-05-30 at 17:16 +0200, Danilo Krummrich wrote:
>> (Not a full review, but a few drive-by comments.)
>>
>> On Sat May 30, 2026 at 4:35 PM CEST, Philipp Stanner wrote:
>> > +#[allow(unused_unsafe)]
>>
>> What is this needed for?
>
> You know that :-P
I don't, it's a serious question.
>> > + // 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 the `DriverFence` has been dropped.
>> > + unsafe {
>> > + bindings::dma_fence_lock_irqsave(fence, flag_ptr);
>> > + if !bindings::dma_fence_is_signaled_locked(fence) {
>> > + if let Err(err) = res {
>> > + bindings::dma_fence_set_error(fence, err.to_errno());
>> > + }
>> > + bindings::dma_fence_signal_locked(fence);
>> > + }
>> > + bindings::dma_fence_unlock_irqrestore(fence, flag_ptr);
>> > + }
>>
>> Please use a single unsafe block per unsafe function call, here and in a few
>> other places.
>
> Is that an official rule? If so, the linters should inform about it.
>
> At first glance, I don't see any advantage to it and the disadvantage
> of greatly reducing readability.
The advantage is that it separates the safety justifications per unsafe call,
which increases the chances of catching a bug, makes it easier for the reader to
match requirements against justifications and potentially allows tooling to
perform checks of the justification against the requirement.
For the specific case above, there's no documented requirements in the sense of
Rust safety requirements of course, as they are all FFI calls, but
dma_fence_signal_locked() for instance has the obvious requirement that it must
only be called with the fence lock held and dma_fence_set_error() must only be
called when the fence is actually signaled.
Besides that, since this pattern seems to occur at least twice, you could also
consider adding a lock guard.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/4] rust: Add dma_fence abstractions
2026-05-30 15:06 ` sashiko-bot
@ 2026-06-01 10:20 ` Alice Ryhl
2026-06-01 12:34 ` Philipp Stanner
0 siblings, 1 reply; 35+ messages in thread
From: Alice Ryhl @ 2026-06-01 10:20 UTC (permalink / raw)
To: sashiko-reviews; +Cc: Philipp Stanner, linux-media, ojeda
On Sat, May 30, 2026 at 03:06:21PM +0000, sashiko-bot@kernel.org wrote:
> > +impl<F: Send + Sync, C: Send + Sync> DriverFence<F, C> {
> > + fn as_raw(&self) -> *mut bindings::dma_fence {
> > + // SAFETY: Valid because `self` is valid.
> > + let fence_data = unsafe { &mut *self.data.as_ptr() };
>
> [Severity: High]
> Does this create an exclusive mutable reference to actively shared memory?
>
> DriverFenceData can be accessed concurrently by other threads holding Fence
> references (for instance, when checking if the fence is signaled). Creating
> a mutable reference (&mut) in Rust asserts exclusive access and violates
> aliasing rules, which allows the compiler to make invalid optimization
> assumptions.
>
> Could this use an immutable reference &*self.data.as_ptr() instead?
Yes, please use an immutable reference here.
> > +
> > + fence_data.inner.inner.get()
> > + }
>
> [ ... ]
>
> > +impl<F: Send + Sync, C: Send + Sync> DriverFenceBorrow<F, C> {
> > + fn as_raw(&self) -> *mut bindings::dma_fence {
> > + // SAFETY: Valid because `self` is valid.
> > + let fence_data = unsafe { &mut *self.data.as_ptr() };
>
> [Severity: High]
> Similar to DriverFence::as_raw(), does this also incorrectly create a
> mutable reference to shared data?
Here as well.
> > +
> > + fence_data.inner.inner.get()
> > + }
>
> [ ... ]
>
> > +impl<F: Send + Sync, C: Send + Sync> Drop for DriverFence<F, C> {
> > + fn drop(&mut self) {
> > + let fence = self.as_raw();
> > + let mut fence_flags: usize = 0;
> > + let flag_ptr = &raw mut fence_flags;
> > +
> > + // 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 the `DriverFence` has been dropped.
> > + unsafe {
> > + bindings::dma_fence_lock_irqsave(fence, flag_ptr);
> > + #[allow(unused_unsafe)]
> > + if warn_on!(!bindings::dma_fence_is_signaled_locked(fence)) {
> > + bindings::dma_fence_set_error(fence, ECANCELED as i32);
>
> [Severity: High]
> Is it intentional to pass a positive error code here?
>
> bindings::ECANCELED evaluates to a positive integer, but
> dma_fence_set_error() expects a negative error code (and triggers a WARN_ON
> for positive values). A positive value might also be interpreted as success
> by callers checking for values < 0.
>
> Should this be - (bindings::ECANCELED as i32)?
Please use ECANCELED.to_errno(). Do not import these from bindings.
> > + bindings::dma_fence_signal_locked(fence);
> > + }
> > + bindings::dma_fence_unlock_irqrestore(fence, flag_ptr);
> > + }
> > +
> > + // SAFETY: `self.data` is owned by the DriverFence, but could be accessed
> > + // through some dma_fence callbacks right now. Access is being revoked
> > + // above by signalling the fence. The DriverFenceAllowedData trait
> > + // ensures that the data either does not need drop, or if it does it
> > + // lives in a RcuBox which will delay dropping by one grace period, hence
> > + // ensuring that all readers have disappeared.
> > + unsafe { drop_in_place(self.data.as_ptr()) };
>
> [Severity: Critical]
> Could this eagerly drop FenceCtx and user data before the C dma_fence
> refcount reaches 0?
>
> The C dma_fence object can remain alive as long as consumers (like sync_file
> or the GPU scheduler) hold references to it. If the backend later invokes
> callbacks like ops->get_driver_name(), FenceCtx::get_driver_name() would
> attempt to read fctx.driver_name from the already-dropped FenceCtx:
>
> FenceCtx::get_driver_name() {
> ...
> fctx.driver_name.as_char_ptr()
> }
>
> Can this result in a use-after-free?
Hmm, I don't know about what sahisko said, but I don't think this is the
right way to do rcu freeing. I think the type's destructor should be
reserved for cases where the value becomes immediately unusable.
For example, let's say I'm using RcuBox<_> here. Yes, the data you get
from dereferencing the RcuBox will stay alive for a grace period, but
IMO once you run the destructor of the box itself, the *pointer* becomes
immediately unusable.
Alice
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/4] rust: Add dma_fence abstractions
2026-05-30 14:35 ` [PATCH 3/4] rust: Add dma_fence abstractions Philipp Stanner
2026-05-30 15:06 ` sashiko-bot
2026-05-30 15:16 ` Danilo Krummrich
@ 2026-06-01 10:36 ` Alice Ryhl
2026-06-01 10:59 ` Boris Brezillon
` (2 more replies)
2 siblings, 3 replies; 35+ messages in thread
From: Alice Ryhl @ 2026-06-01 10:36 UTC (permalink / raw)
To: Philipp Stanner
Cc: Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Trevor Gross, Danilo Krummrich,
Sumit Semwal, Christian König, Paul E. McKenney,
Frederic Weisbecker, Neeraj Upadhyay, Joel Fernandes,
Josh Triplett, Uladzislau Rezki, Steven Rostedt,
Mathieu Desnoyers, Lai Jiangshan, Zqiang, Daniel Almeida,
Greg Kroah-Hartman, Igor Korotin, Lorenzo Stoakes,
Alexandre Courbot, FUJITA Tomonori, Krishna Ketan Rai,
Shankari Anand, manos, Boris Brezillon, linux-kernel,
rust-for-linux, linux-media, dri-devel, linaro-mm-sig, rcu
On Sat, May 30, 2026 at 04:35:11PM +0200, Philipp Stanner wrote:
> C's dma_fence's are synchronisation primitives that will be needed by all
> Rust GPU drivers.
>
> The dma_fence framework sets a number of rules, notably:
> - fences must only be 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 through 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 <phasta@kernel.org>
> ---
> rust/bindings/bindings_helper.h | 1 +
> rust/helpers/dma_fence.c | 48 ++
> rust/helpers/helpers.c | 1 +
> rust/kernel/dma_buf/dma_fence.rs | 821 +++++++++++++++++++++++++++++++
> rust/kernel/dma_buf/mod.rs | 13 +
> rust/kernel/lib.rs | 1 +
> 6 files changed, 885 insertions(+)
> create mode 100644 rust/helpers/dma_fence.c
> create mode 100644 rust/kernel/dma_buf/dma_fence.rs
> create mode 100644 rust/kernel/dma_buf/mod.rs
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 2011645c7cfb..69daeb790f77 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -52,6 +52,7 @@
> #include <linux/debugfs.h>
> #include <linux/device/faux.h>
> #include <linux/dma-direction.h>
> +#include <linux/dma-fence.h>
> #include <linux/dma-mapping.h>
> #include <linux/dma-resv.h>
> #include <linux/errname.h>
> diff --git a/rust/helpers/dma_fence.c b/rust/helpers/dma_fence.c
> new file mode 100644
> index 000000000000..6244a5a61038
> --- /dev/null
> +++ b/rust/helpers/dma_fence.c
> @@ -0,0 +1,48 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/dma-fence.h>
> +
> +__rust_helper void rust_helper_dma_fence_get(struct dma_fence *f)
> +{
> + dma_fence_get(f);
> +}
> +
> +__rust_helper void rust_helper_dma_fence_put(struct dma_fence *f)
> +{
> + dma_fence_put(f);
> +}
> +
> +__rust_helper bool rust_helper_dma_fence_begin_signalling(void)
> +{
> + return dma_fence_begin_signalling();
> +}
> +
> +__rust_helper void rust_helper_dma_fence_end_signalling(bool cookie)
> +{
> + dma_fence_end_signalling(cookie);
> +}
> +
> +__rust_helper bool rust_helper_dma_fence_is_signaled(struct dma_fence *f)
> +{
> + return dma_fence_is_signaled(f);
> +}
> +
> +__rust_helper bool rust_helper_dma_fence_is_signaled_locked(struct dma_fence *f)
> +{
> + return dma_fence_is_signaled_locked(f);
> +}
> +
> +__rust_helper void rust_helper_dma_fence_lock_irqsave(struct dma_fence *f, unsigned long *flags)
> +{
> + dma_fence_lock_irqsave(f, *flags);
> +}
> +
> +__rust_helper void rust_helper_dma_fence_unlock_irqrestore(struct dma_fence *f, unsigned long *flags)
> +{
> + dma_fence_unlock_irqrestore(f, *flags);
> +}
> +
> +__rust_helper void rust_helper_dma_fence_set_error(struct dma_fence *f, int error)
> +{
> + dma_fence_set_error(f, error);
> +}
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 625921e27dfb..d9114d0b3c8f 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -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 "err.c"
> diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
> new file mode 100644
> index 000000000000..7dc1f5c16b02
> --- /dev/null
> +++ b/rust/kernel/dma_buf/dma_fence.rs
> @@ -0,0 +1,821 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (C) 2025, 2026 Red Hat Inc.:
> +// - Philipp Stanner <pstanner@redhat.com>
> +
> +//! DriverFence support.
> +//!
> +//! Reference: <https://docs.kernel.org/driver-api/dma-buf.html#c.dma_fence>
> +//!
> +//! C header: [`include/linux/dma-fence.h`](srctree/include/linux/dma-fence.h)
> +
> +use crate::{
> + alloc::AllocError,
> + bindings,
> + container_of,
> + error::to_result,
> + prelude::*,
> + sync::rcu::RcuBox,
> + types::ForeignOwnable,
> + types::Opaque,
> + warn_on, //
> +};
> +
> +use pin_init::pin_init_from_closure;
> +
> +use core::{
> + marker::PhantomData, //
> + ops::Deref,
> + ptr,
> + ptr::{
> + drop_in_place,
> + NonNull, //
> + },
> + sync::atomic::{
> + AtomicU64,
> + Ordering, //
> + },
Use atomics from the kernel crate instead.
> +};
> +
> +use bindings::ECANCELED;
> +
> +use kernel::str::CString;
> +use kernel::sync::{
> + aref::{
> + ARef,
> + AlwaysRefCounted, //
> + },
> + Arc,
> + ArcBorrow, //
> +};
> +
> +/// 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 {}
This empty trait is unused.
> +/// 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<F: Send + Sync, C: Send + Sync> {
No need to list any trait bounds here. You can list them on `impl`
blocks only.
> + /// The fence context number.
> + nr: u64,
> + /// The sequence number for the next fence created.
> + seqno: AtomicU64,
> + // The name parameters live in RcuBox because they can be accessed by the
> + // dma_fence backend_ops. Those accesses are guarded by the rcu_read_lock(),
> + // so dropping them must be delayed by a grace period.
> + /// 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,
> + #[pin]
> + data: C,
> + fence_type: PhantomData<F>,
> +}
> +
> +#[allow(unused_unsafe)]
> +impl<F: Send + Sync + DriverFenceAllowedData, C: Send + Sync> FenceCtx<F, C> {
> + // 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(
> + driver_name: CString,
> + timeline_name: CString,
> + data: impl PinInit<C>,
> + ) -> Result<Arc<Self>> {
> + let ctx = 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),
> + driver_name,
> + timeline_name,
> + data <- data,
> + fence_type: PhantomData,
> + });
> +
> + Arc::pin_init(ctx, GFP_KERNEL)
> + }
> +
> + 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: ArcBorrow<'_, Self>,
> + data: F,
> + ) -> Result<DriverFenceAllocation<F, C>> {
> + let fctx = Arc::<Self>::from(self);
> +
> + DriverFenceAllocation::new(fctx, data)
> + }
> +
> + /// 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<F, C>) -> DriverFence<F, C> {
> + 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<'a>(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<F, C>;
> +
> + // 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<F: Send + Sync, C: Send + Sync> PinnedDrop for FenceCtx<F, C> {
> + fn drop(self: Pin<&mut Self>) {
> + // SAFETY: `rcu_barrier()` is always safe to be called.
> + unsafe { bindings::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
> +/// }
> +///
> +/// impl FenceCb for MyCallback {
> +/// fn called(&mut self) {
> +/// pr_info!("Fence signaled!");
> +/// // Handle fence completion
> +/// }
> +/// }
> +/// ```
> +pub trait FenceCb: Send + 'static {
> + /// Called when the fence is signaled.
> + ///
> + /// This is called from the fence signaling path, which may be in interrupt
> + /// context or with locks held, which is why `self` is only borrowed, so that
> + /// it cannot drop. Implementations must not sleep or perform
> + /// long-running operations.
> + ///
> + /// An implementation likely wants to inform itself (e.g., through a work item)
> + /// within this callback that the associated [`FenceCbRegistration`] can now be
> + /// dropped.
> + fn called(&mut self);
> +}
> +
> +/// 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]
> + cb: Opaque<bindings::dma_fence_cb>,
> + callback: T,
> + fence: ARef<Fence>,
> +}
> +
> +impl<T: FenceCb> FenceCbRegistration<T> {
> + /// Register a callback on a fence.
> + ///
> + /// 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,
> + {
> + // Uses `pin_init_from_closure` instead of `try_pin_init!` so that on
> + // `-ENOENT` (already signaled) the callback can be read back from the
> + // partially-initialized slot and returned through the error.
> + //
> + // SAFETY: `pin_init_from_closure` requires:
> + // - On `Ok(())`: the slot is fully initialized and valid for `Drop`.
> + // - On `Err(_)`: the slot is clean, i.e.: no partially-initialized fields
> + // remain, and the slot can be deallocated without dropping.
> + //
> + // We uphold this as follows:
> + // - On success: all three fields are initialized. Ok(()) is returned.
> + // - On ENOENT (already signaled): `callback` and `fence` are read back
> + // from the slot via `ptr::read`, leaving the slot clean. `cb` was
> + // initialized by `dma_fence_add_callback` (it calls
> + // `INIT_LIST_HEAD(&cb->node)` even on error), but `cb` is
> + // `Opaque<dma_fence_cb>` which has no `Drop`, so not dropping it is
> + // fine. The callback is returned through `AlreadySignaled(T)`.
> + // - On other errors: same cleanup as ENOENT, error returned as
> + // `Other(e)`.
> + unsafe {
> + pin_init_from_closure(move |slot: *mut Self| {
> + let slot_callback = &raw mut (*slot).callback;
> + let slot_fence = &raw mut (*slot).fence;
> + let slot_cb = &raw mut (*slot).cb;
> +
> + // Write callback and fence first — must be visible before
> + // dma_fence_add_callback makes the registration live.
> + core::ptr::write(slot_callback, callback);
> + core::ptr::write(slot_fence, ARef::from(fence));
Here you are incrementing the fence refcount. It's better to change the
function argument to ARef<Fence> so that the user can avoid this
increment if they happen to own a refcount they're willing to give up.
> + let ret = to_result(bindings::dma_fence_add_callback(
> + fence.inner.get(),
> + Opaque::cast_into(slot_cb),
> + Some(Self::dma_fence_callback),
> + ));
> +
> + match ret {
> + Ok(()) => Ok(()),
> + Err(e) => {
> + // Read back what we wrote to leave the slot clean.
> + let cb_back = core::ptr::read(slot_callback);
> + let _fence_back = core::ptr::read(slot_fence);
This can be drop_in_place().
> + if e.to_errno() == ENOENT.to_errno() {
> + Err(CallbackError::AlreadySignaled(cb_back))
> + } else {
> + Err(CallbackError::Other(e))
> + }
> + }
> + }
> + })
> + }
> + }
> +
> + /// 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,
> + cb: *mut bindings::dma_fence_cb,
> + ) {
> + let ptr = Opaque::cast_from(cb).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, cb);
> +
> + (*reg).callback.called();
> + }
> + }
> +
> + /// Returns a reference to the fence this callback is registered on.
> + pub fn fence(self: Pin<&Self>) -> &Fence {
Can be simplified to `fn fence(&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.cb.get());
> + }
Formatting nit: Usually the ; goes outside the unsafe block.
> + }
> +}
> +
> +// SAFETY: FenceCbRegistration can be sent between threads
> +unsafe impl<T: FenceCb> Send for FenceCbRegistration<T> {}
> +
> +// SAFETY: &FenceCbRegistration can be shared between threads if &T can.
> +unsafe impl<T: FenceCb> Sync for FenceCbRegistration<T> where T: Sync {}
There's no &FenceCbRegistration<T> -> &T accessor, so I don't think this
bound is required.
unsafe impl<T: FenceCb> Sync for FenceCbRegistration<T> {}
There also can't be such an accessor in the future because the closure
takes a &mut T.
> +/// 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.
> +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.
> + pub fn is_signaled(&self) -> bool {
> + // SAFETY: self is by definition still valid. The backend ensures proper
> + // locking.
> + unsafe { bindings::dma_fence_is_signaled(self.as_raw()) }
> + }
> +
> + 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: Valid because `fence` was created validly above.
> + unsafe { bindings::dma_fence_put(fence) }
> + }
> +}
> +
> +#[repr(C)] // Necessary to guarantee that `inner` always comes first so that we can cast.
> +#[pin_data]
> +struct DriverFenceData<F: Send + Sync, C: Send + Sync> {
Ditto here about trait bounds. (And everywhere else.)
> + #[pin]
> + /// The inner fence.
> + inner: Fence,
> + /// Pointer to access the FenceCtx. Useful for obtaining name parameters.
> + // The FenceCtx lives as long as at least all its fences, hence this is safe.
> + fctx: Arc<FenceCtx<F, C>>,
> + /// The API user's data. As required by [`DriverFenceAllowedData`], this either
> + /// does not need drop, or must live in a [`rcu::RcuBox`]. It is essential
> + /// that the data only performs operations legal in atomic context in its
> + /// [`Drop`] implementation.
> + #[pin]
> + data: F,
> +}
> +
> +/// A trait to enforce that all data in a [`DriverFence`] either does not need
> +/// drop, or lives in a [`RcuBox`].
> +pub trait DriverFenceAllowedData: private::Sealed {}
> +
> +mod private {
> + pub trait Sealed {}
> +}
> +
> +impl<F: Copy> DriverFenceAllowedData for F {}
> +impl<F: Send> DriverFenceAllowedData for RcuBox<F> {}
> +
> +impl<F: Copy> private::Sealed for F {}
> +impl<F: Send> private::Sealed for RcuBox<F> {}
Why sealed? Just make the trait unsafe and require the things you
require from the user.
> +/// 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, FenceCb, FenceCbRegistration};
> +/// use kernel::str::CString;
> +/// use kernel::sync::{
> +/// aref::ARef,
> +/// rcu::RcuBox, //
> +/// };
> +/// use core::ops::Deref;
> +/// use core::fmt::Display;
Use fmt traits from kernel instead. (Actually, I don't think you use
Display at all here?)
> +/// struct CallbackData { }
> +///
> +/// impl FenceCb for CallbackData {
> +/// fn called(&mut self) {
> +/// pr_info!("DmaFence callback executed.\n");
> +/// }
> +/// }
> +///
> +/// let driver_name = CString::try_from_fmt(fmt!("dummy_driver"))?;
> +/// let timeline_name = CString::try_from_fmt(fmt!("dummy_timeline"))?;
> +///
> +/// let fctx = FenceCtx::new(driver_name, timeline_name, ())?;
> +///
> +/// let fence_data = CString::try_from_fmt(fmt!("dummy_data"))?;
> +/// // DriverFence::data must either not need drop, or live in an RcuBox.
> +/// let fence_data = RcuBox::new(fence_data, GFP_KERNEL)?;
> +///
> +/// let fence_alloc = fctx.as_arc_borrow().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)?;
> +///
> +/// // DriverFence implements Deref.
> +/// // FIXME: unit test claims that CString does not implement Display. Why?
> +/// // pr_info!("Fence's inner data is: {}", fence.deref().deref());
> +///
> +/// // TODO begin_signalling
> +/// fence.signal(Ok(()));
> +/// assert_eq!(waiting_fence.is_signaled(), true);
> +///
> +/// Ok::<(), Error>(())
> +/// ```
> +pub struct DriverFence<F: Send + Sync, C: Send + Sync> {
> + /// 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<F, C>>,
> +}
> +
> +/// 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<F: Send + Sync, C: Send + Sync> {
> + /// The memory for the actual content of the fence.
> + /// Handed over to a [`DriverFence`], or deallocated once the
> + /// [`DriverFenceAllocation`] drops.
> + data: KBox<DriverFenceData<F, C>>,
> +}
> +
> +impl<F: Send + Sync + DriverFenceAllowedData, C: Send + Sync> DriverFenceAllocation<F, C> {
> + /// Create a new allocation slot that can later be used to create a fully
> + /// initialized [`DriverFence`] without the need to allocate.
> + pub fn new(fctx: Arc<FenceCtx<F, C>>, data: F) -> Result<Self> {
> + let fence_data = DriverFenceData {
> + // `inner` remains uninitialized until a [`DriverFence`] takes over.
> + inner: Fence {
> + inner: Opaque::uninit(),
> + },
> + fctx,
> + data,
> + };
> +
> + // In order to support the C dma_fence callbacks, it is necessary for
> + // a `Fence` and a `DriverFence` to live in the same allocation,
> + // because the C backend passes a dma_fence, from which the driver most
> + // likely wants to be able to access its `data` in `DriverFence`.
> + //
> + // Hence, we need the manage the memory manually. It will be freed by the
> + // C backend automatically once the refcount within `Fence` drops to 0.
> + let data = KBox::new(fence_data, GFP_KERNEL | __GFP_ZERO)?;
> +
> + Ok(Self { data })
> + }
> +
> + fn as_raw(&self) -> *mut bindings::dma_fence {
> + self.data.inner.inner.get()
> + }
> +}
> +
> +impl<F: Send + Sync, C: Send + Sync> DriverFence<F, C> {
> + fn as_raw(&self) -> *mut bindings::dma_fence {
> + // SAFETY: Valid because `self` is valid.
> + let fence_data = unsafe { &mut *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
> + /// [`FenceCtx`].
> + unsafe fn from_raw(ptr: *mut bindings::dma_fence) -> Self {
> + let opaque_fence = Opaque::cast_from(ptr);
> +
> + // SAFETY: Safe due to the function's overall safety requirements.
> + let fence_ptr = unsafe { container_of!(opaque_fence, Fence, inner) };
> +
> + // DriverFenceData is repr(C) and a Fence is its first member.
> + let fence_data_ptr = fence_ptr as *mut DriverFenceData<F, C>;
> +
> + // 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: 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 the `DriverFence` has been dropped.
> + unsafe {
> + bindings::dma_fence_lock_irqsave(fence, flag_ptr);
> + if !bindings::dma_fence_is_signaled_locked(fence) {
> + if let Err(err) = res {
> + bindings::dma_fence_set_error(fence, err.to_errno());
> + }
> + bindings::dma_fence_signal_locked(fence);
> + }
> + bindings::dma_fence_unlock_irqrestore(fence, flag_ptr);
> + }
This single unsafe blocks spans five different unsafe operations.
> + }
> +}
> +
> +// SAFETY: Fences are literally designed to be shared between threads.
> +unsafe impl<F: Send + Sync, C: Send + Sync> Send for DriverFence<F, C> {}
> +
> +impl<F: Send + Sync, C: Send + Sync> Deref for DriverFence<F, C> {
> + type Target = F;
> +
> + fn deref(&self) -> &Self::Target {
> + // SAFETY: Thanks to refcounting, `data` is always valid as long as `self` is.
> + let data = unsafe { &*self.data.as_ptr() };
> +
> + &data.data
> + }
> +}
> +
> +/// A borrowed [`DriverFence`]. All you can do with it is access your user data
> +/// and obtain a [`Fence`].
> +pub struct DriverFenceBorrow<F: Send + Sync, C: Send + Sync> {
> + /// 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<F, C>>,
> +}
> +
> +impl<F: Send + Sync, C: Send + Sync> Deref for DriverFenceBorrow<F, C> {
> + type Target = F;
> +
> + fn deref(&self) -> &Self::Target {
> + // SAFETY: Thanks to refcounting, `data` is always valid as long as `self` is.
> + let data = unsafe { &*self.data.as_ptr() };
> +
> + &data.data
> + }
> +}
> +
> +impl<F: Send + Sync, C: Send + Sync> DriverFenceBorrow<F, C> {
> + fn as_raw(&self) -> *mut bindings::dma_fence {
> + // SAFETY: Valid because `self` is valid.
> + let fence_data = unsafe { &mut *self.data.as_ptr() };
> +
> + fence_data.inner.inner.get()
> + }
> +
> + /// 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()) }
> + }
> +
> + /// Get a [`DriverFenceBorrow`] from a raw pointer.
> + ///
> + /// # Safety
> + ///
> + /// `ptr` must point to a raw dma_fence within a [`Fence`] within a [`DriverFenceData`].
> + unsafe fn from_raw(ptr: *mut bindings::dma_fence) -> Self {
> + let opaque_fence = Opaque::cast_from(ptr);
> +
> + // SAFETY: Safe due to the function's overall safety requirements.
> + let fence_ptr = unsafe { container_of!(opaque_fence, Fence, inner) };
> +
> + // DriverFenceData is repr(C) and a Fence is its first member.
> + let fence_data_ptr = fence_ptr as *mut DriverFenceData<F, C>;
> +
> + // SAFETY: `fence_data_ptr` was created validly above.
> + let data = unsafe { NonNull::new_unchecked(fence_data_ptr) };
> +
> + Self { data }
> + }
> +}
> +
> +// 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<F: Send + Sync + 'static, C: Send + Sync + 'static> ForeignOwnable
> + for DriverFence<F, C>
> +{
> + // `DriverFence` is merely a wrapper around a raw pointer. Thus, we can just
> + // use it directly.
> + type Borrowed<'a> = DriverFenceBorrow<F, C>;
> + type BorrowedMut<'a> = DriverFenceBorrow<F, C>;
> +
> + 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.
> + core::mem::forget(fence);
Nit: Modern Rust uses ManuallyDrop instead of forget().
> + 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> {
> + // SAFETY: The trait implementation ensures that `ptr` always resides
> + // within a [`Fence`] within a [`DriverFenceData`].
> + unsafe { DriverFenceBorrow::from_raw(ptr.cast()) }
> + }
> +
> + unsafe fn borrow_mut<'a>(ptr: *mut c_void) -> Self::BorrowedMut<'a> {
> + // SAFETY: The trait implementation ensures that `ptr` always resides
> + // within a [`Fence`] within a [`DriverFenceData`].
> + unsafe { DriverFenceBorrow::from_raw(ptr.cast()) }
> + }
> +}
> +
> +impl<F: Send + Sync, C: Send + Sync> Drop for DriverFence<F, C> {
> + fn drop(&mut self) {
> + let fence = self.as_raw();
> + let mut fence_flags: usize = 0;
> + let flag_ptr = &raw mut fence_flags;
> +
> + // 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 the `DriverFence` has been dropped.
> + unsafe {
> + bindings::dma_fence_lock_irqsave(fence, flag_ptr);
> + #[allow(unused_unsafe)]
> + if warn_on!(!bindings::dma_fence_is_signaled_locked(fence)) {
> + bindings::dma_fence_set_error(fence, ECANCELED as i32);
> + bindings::dma_fence_signal_locked(fence);
> + }
> + bindings::dma_fence_unlock_irqrestore(fence, flag_ptr);
> + }
> +
> + // SAFETY: `self.data` is owned by the DriverFence, but could be accessed
> + // through some dma_fence callbacks right now. Access is being revoked
> + // above by signalling the fence. The DriverFenceAllowedData trait
> + // ensures that the data either does not need drop, or if it does it
> + // lives in a RcuBox which will delay dropping by one grace period, hence
> + // ensuring that all readers have disappeared.
> + unsafe { drop_in_place(self.data.as_ptr()) };
> +
> + // 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 the `DriverFence` has been dropped.
> + unsafe {
> + bindings::dma_fence_put(fence);
> + }
> +
> + // The actual memory the data associated with a `DriverFence` lives in
> + // gets freed by the C dma_fence backend once the fence's refcount reaches 0.
> + }
> +}
> diff --git a/rust/kernel/dma_buf/mod.rs b/rust/kernel/dma_buf/mod.rs
> new file mode 100644
> index 000000000000..d9da3dc57fce
> --- /dev/null
> +++ b/rust/kernel/dma_buf/mod.rs
> @@ -0,0 +1,13 @@
> +// 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, //
> +};
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index b72b2fbe046d..a05ccaa7598c 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -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;
> --
> 2.54.0
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/4] rust: Add dma_fence abstractions
2026-06-01 10:36 ` Alice Ryhl
@ 2026-06-01 10:59 ` Boris Brezillon
2026-06-01 11:17 ` Philipp Stanner
2026-06-01 12:26 ` Philipp Stanner
2026-06-01 12:37 ` Boris Brezillon
2 siblings, 1 reply; 35+ messages in thread
From: Boris Brezillon @ 2026-06-01 10:59 UTC (permalink / raw)
To: Alice Ryhl
Cc: Philipp Stanner, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Sumit Semwal,
Christian König, Paul E. McKenney, Frederic Weisbecker,
Neeraj Upadhyay, Joel Fernandes, Josh Triplett, Uladzislau Rezki,
Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, Zqiang,
Daniel Almeida, Greg Kroah-Hartman, Igor Korotin, Lorenzo Stoakes,
Alexandre Courbot, FUJITA Tomonori, Krishna Ketan Rai,
Shankari Anand, manos, linux-kernel, rust-for-linux, linux-media,
dri-devel, linaro-mm-sig, rcu
On Mon, 1 Jun 2026 10:36:06 +0000
Alice Ryhl <aliceryhl@google.com> wrote:
> > +};
> > +
> > +use bindings::ECANCELED;
> > +
> > +use kernel::str::CString;
> > +use kernel::sync::{
> > + aref::{
> > + ARef,
> > + AlwaysRefCounted, //
> > + },
> > + Arc,
> > + ArcBorrow, //
> > +};
> > +
> > +/// 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 {}
>
> This empty trait is unused.
I had initially suggested to add the F type (AKA FenceData) passed
around in multiple places type as an associated type
pub trait FenceCtxOps {
type FenceData: Send + Sync;
}
so we don't have to pass both F and C. The reasoning here is that:
1. We expect we'll have to define more methods to the FenceCtxOps trait
at some point, so adding it now kinda makes sense.
2. In the current design, we've assumed that a Fence can't live/be
created outside of a given context, so there's no world where the
FenceData wouldn't be known by the FenceCtx implementation, and forcing
users to pass F and C around seems needlessly verbose.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/4] rust: Add dma_fence abstractions
2026-06-01 10:59 ` Boris Brezillon
@ 2026-06-01 11:17 ` Philipp Stanner
2026-06-01 12:35 ` Boris Brezillon
0 siblings, 1 reply; 35+ messages in thread
From: Philipp Stanner @ 2026-06-01 11:17 UTC (permalink / raw)
To: Boris Brezillon, Alice Ryhl
Cc: Philipp Stanner, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Sumit Semwal,
Christian König, Paul E. McKenney, Frederic Weisbecker,
Neeraj Upadhyay, Joel Fernandes, Josh Triplett, Uladzislau Rezki,
Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, Zqiang,
Daniel Almeida, Greg Kroah-Hartman, Igor Korotin, Lorenzo Stoakes,
Alexandre Courbot, FUJITA Tomonori, Krishna Ketan Rai,
Shankari Anand, manos, linux-kernel, rust-for-linux, linux-media,
dri-devel, linaro-mm-sig, rcu
On Mon, 2026-06-01 at 12:59 +0200, Boris Brezillon wrote:
> On Mon, 1 Jun 2026 10:36:06 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
>
> > > +};
> > > +
> > > +use bindings::ECANCELED;
> > > +
> > > +use kernel::str::CString;
> > > +use kernel::sync::{
> > > + aref::{
> > > + ARef,
> > > + AlwaysRefCounted, //
> > > + },
> > > + Arc,
> > > + ArcBorrow, //
> > > +};
> > > +
> > > +/// 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 {}
> >
> > This empty trait is unused.
>
> I had initially suggested to add the F type (AKA FenceData) passed
> around in multiple places type as an associated type
>
> pub trait FenceCtxOps {
> type FenceData: Send + Sync;
> }
>
> so we don't have to pass both F and C. The reasoning here is that:
>
> 1. We expect we'll have to define more methods to the FenceCtxOps trait
> at some point, so adding it now kinda makes sense.
>
> 2. In the current design, we've assumed that a Fence can't live/be
> created outside of a given context, so there's no world where the
> FenceData wouldn't be known by the FenceCtx implementation, and forcing
> users to pass F and C around seems needlessly verbose.
I had investigated that, but found that this causes us to write things
like
DriverFence<T> (where T is the FencCtx generic)
and then in the actual implementation use
T::FenceData
which reads very weird IMO. Because now for reasons a fence's own data
are not referred to in its own implementation, but you derive it from
the context.
I do prefer it in a way where the DriverFence generic does appear in
said fence's actual code, on equal rank with the FenceCtx.
I suppose that is actually one use case for which PhantomData does
exist.
P.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/4] rust: Add dma_fence abstractions
2026-06-01 10:36 ` Alice Ryhl
2026-06-01 10:59 ` Boris Brezillon
@ 2026-06-01 12:26 ` Philipp Stanner
2026-06-01 12:39 ` Alice Ryhl
2026-06-01 12:37 ` Boris Brezillon
2 siblings, 1 reply; 35+ messages in thread
From: Philipp Stanner @ 2026-06-01 12:26 UTC (permalink / raw)
To: Alice Ryhl, Philipp Stanner
Cc: Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Trevor Gross, Danilo Krummrich,
Sumit Semwal, Christian König, Paul E. McKenney,
Frederic Weisbecker, Neeraj Upadhyay, Joel Fernandes,
Josh Triplett, Uladzislau Rezki, Steven Rostedt,
Mathieu Desnoyers, Lai Jiangshan, Zqiang, Daniel Almeida,
Greg Kroah-Hartman, Igor Korotin, Lorenzo Stoakes,
Alexandre Courbot, FUJITA Tomonori, Krishna Ketan Rai,
Shankari Anand, manos, Boris Brezillon, linux-kernel,
rust-for-linux, linux-media, dri-devel, linaro-mm-sig, rcu
On Mon, 2026-06-01 at 10:36 +0000, Alice Ryhl wrote:
> On Sat, May 30, 2026 at 04:35:11PM +0200, Philipp Stanner wrote:
> >
[…]
> > +use pin_init::pin_init_from_closure;
> > +
> > +use core::{
> > + marker::PhantomData, //
> > + ops::Deref,
> > + ptr,
> > + ptr::{
> > + drop_in_place,
> > + NonNull, //
> > + },
> > + sync::atomic::{
> > + AtomicU64,
> > + Ordering, //
> > + },
>
> Use atomics from the kernel crate instead.
OK.
>
> > +};
> > +
> > +use bindings::ECANCELED;
> > +
> > +use kernel::str::CString;
> > +use kernel::sync::{
> > + aref::{
> > + ARef,
> > + AlwaysRefCounted, //
> > + },
> > + Arc,
> > + ArcBorrow, //
> > +};
> > +
> > +/// 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 {}
>
> This empty trait is unused.
(discussed in the other thread with Boris)
>
> > +/// 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<F: Send + Sync, C: Send + Sync> {
>
> No need to list any trait bounds here. You can list them on `impl`
> blocks only.
ACK.
>
> >
[…]
> > + {
> > + // Uses `pin_init_from_closure` instead of `try_pin_init!` so that on
> > + // `-ENOENT` (already signaled) the callback can be read back from the
> > + // partially-initialized slot and returned through the error.
> > + //
> > + // SAFETY: `pin_init_from_closure` requires:
> > + // - On `Ok(())`: the slot is fully initialized and valid for `Drop`.
> > + // - On `Err(_)`: the slot is clean, i.e.: no partially-initialized fields
> > + // remain, and the slot can be deallocated without dropping.
> > + //
> > + // We uphold this as follows:
> > + // - On success: all three fields are initialized. Ok(()) is returned.
> > + // - On ENOENT (already signaled): `callback` and `fence` are read back
> > + // from the slot via `ptr::read`, leaving the slot clean. `cb` was
> > + // initialized by `dma_fence_add_callback` (it calls
> > + // `INIT_LIST_HEAD(&cb->node)` even on error), but `cb` is
> > + // `Opaque<dma_fence_cb>` which has no `Drop`, so not dropping it is
> > + // fine. The callback is returned through `AlreadySignaled(T)`.
> > + // - On other errors: same cleanup as ENOENT, error returned as
> > + // `Other(e)`.
> > + unsafe {
> > + pin_init_from_closure(move |slot: *mut Self| {
> > + let slot_callback = &raw mut (*slot).callback;
> > + let slot_fence = &raw mut (*slot).fence;
> > + let slot_cb = &raw mut (*slot).cb;
> > +
> > + // Write callback and fence first — must be visible before
> > + // dma_fence_add_callback makes the registration live.
> > + core::ptr::write(slot_callback, callback);
> > + core::ptr::write(slot_fence, ARef::from(fence));
>
> Here you are incrementing the fence refcount. It's better to change the
> function argument to ARef<Fence> so that the user can avoid this
> increment if they happen to own a refcount they're willing to give up.
Agreed, will do
>
> > + let ret = to_result(bindings::dma_fence_add_callback(
> > + fence.inner.get(),
> > + Opaque::cast_into(slot_cb),
> > + Some(Self::dma_fence_callback),
> > + ));
> > +
> > + match ret {
> > + Ok(()) => Ok(()),
> > + Err(e) => {
> > + // Read back what we wrote to leave the slot clean.
> > + let cb_back = core::ptr::read(slot_callback);
> > + let _fence_back = core::ptr::read(slot_fence);
>
> This can be drop_in_place().
>
> > + if e.to_errno() == ENOENT.to_errno() {
> > + Err(CallbackError::AlreadySignaled(cb_back))
> > + } else {
> > + Err(CallbackError::Other(e))
> > + }
> > + }
> > + }
> > + })
> > + }
> > + }
> > +
> > + /// 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,
> > + cb: *mut bindings::dma_fence_cb,
> > + ) {
> > + let ptr = Opaque::cast_from(cb).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, cb);
> > +
> > + (*reg).callback.called();
> > + }
> > + }
> > +
> > + /// Returns a reference to the fence this callback is registered on.
> > + pub fn fence(self: Pin<&Self>) -> &Fence {
>
> Can be simplified to `fn fence(&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.cb.get());
> > + }
>
> Formatting nit: Usually the ; goes outside the unsafe block.
I could have sworn that it was rustfmt who did that? Maybe because the
; was inside to begin with.
>
> > + }
> > +}
> > +
> > +// SAFETY: FenceCbRegistration can be sent between threads
> > +unsafe impl<T: FenceCb> Send for FenceCbRegistration<T> {}
> > +
> > +// SAFETY: &FenceCbRegistration can be shared between threads if &T can.
> > +unsafe impl<T: FenceCb> Sync for FenceCbRegistration<T> where T: Sync {}
>
> There's no &FenceCbRegistration<T> -> &T accessor, so I don't think this
> bound is required.
>
> unsafe impl<T: FenceCb> Sync for FenceCbRegistration<T> {}
>
> There also can't be such an accessor in the future because the closure
> takes a &mut T.
Hm, very correct. The entire design only allows serial access.
>
> > +/// 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.
> > +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.
> > + pub fn is_signaled(&self) -> bool {
> > + // SAFETY: self is by definition still valid. The backend ensures proper
> > + // locking.
> > + unsafe { bindings::dma_fence_is_signaled(self.as_raw()) }
> > + }
> > +
> > + 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: Valid because `fence` was created validly above.
> > + unsafe { bindings::dma_fence_put(fence) }
> > + }
> > +}
> > +
> > +#[repr(C)] // Necessary to guarantee that `inner` always comes first so that we can cast.
> > +#[pin_data]
> > +struct DriverFenceData<F: Send + Sync, C: Send + Sync> {
>
> Ditto here about trait bounds. (And everywhere else.)
>
> > + #[pin]
> > + /// The inner fence.
> > + inner: Fence,
> > + /// Pointer to access the FenceCtx. Useful for obtaining name parameters.
> > + // The FenceCtx lives as long as at least all its fences, hence this is safe.
> > + fctx: Arc<FenceCtx<F, C>>,
> > + /// The API user's data. As required by [`DriverFenceAllowedData`], this either
> > + /// does not need drop, or must live in a [`rcu::RcuBox`]. It is essential
> > + /// that the data only performs operations legal in atomic context in its
> > + /// [`Drop`] implementation.
> > + #[pin]
> > + data: F,
> > +}
> > +
> > +/// A trait to enforce that all data in a [`DriverFence`] either does not need
> > +/// drop, or lives in a [`RcuBox`].
> > +pub trait DriverFenceAllowedData: private::Sealed {}
> > +
> > +mod private {
> > + pub trait Sealed {}
> > +}
> > +
> > +impl<F: Copy> DriverFenceAllowedData for F {}
> > +impl<F: Send> DriverFenceAllowedData for RcuBox<F> {}
> > +
> > +impl<F: Copy> private::Sealed for F {}
> > +impl<F: Send> private::Sealed for RcuBox<F> {}
>
> Why sealed? Just make the trait unsafe and require the things you
> require from the user.
This is far better. We definitely only allow the user to pass A or B,
and only then it compiles.
The unsafe implementation could be messed up.
I thought that's what Sealed is for. Or isn't it?
>
> > +/// 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, FenceCb, FenceCbRegistration};
> > +/// use kernel::str::CString;
> > +/// use kernel::sync::{
> > +/// aref::ARef,
> > +/// rcu::RcuBox, //
> > +/// };
> > +/// use core::ops::Deref;
> > +/// use core::fmt::Display;
>
> Use fmt traits from kernel instead. (Actually, I don't think you use
> Display at all here?)
I tried, see a few lines below:
>
> > +/// struct CallbackData { }
> > +///
> > +/// impl FenceCb for CallbackData {
> > +/// fn called(&mut self) {
> > +/// pr_info!("DmaFence callback executed.\n");
> > +/// }
> > +/// }
> > +///
> > +/// let driver_name = CString::try_from_fmt(fmt!("dummy_driver"))?;
> > +/// let timeline_name = CString::try_from_fmt(fmt!("dummy_timeline"))?;
> > +///
> > +/// let fctx = FenceCtx::new(driver_name, timeline_name, ())?;
> > +///
> > +/// let fence_data = CString::try_from_fmt(fmt!("dummy_data"))?;
> > +/// // DriverFence::data must either not need drop, or live in an RcuBox.
> > +/// let fence_data = RcuBox::new(fence_data, GFP_KERNEL)?;
> > +///
> > +/// let fence_alloc = fctx.as_arc_borrow().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)?;
> > +///
> > +/// // DriverFence implements Deref.
> > +/// // FIXME: unit test claims that CString does not implement Display. Why?
> > +/// // pr_info!("Fence's inner data is: {}", fence.deref().deref());
Lazily, I was hoping that someone here will tell me how that is
supposed to be done correctly 8-)
> > +///
> > +/// // TODO begin_signalling
> > +/// fence.signal(Ok(()));
> > +/// assert_eq!(waiting_fence.is_signaled(), true);
> > +///
> > +/// Ok::<(), Error>(())
> > +/// ```
> > +pub struct DriverFence<F: Send + Sync, C: Send + Sync> {
> > + /// 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<F, C>>,
> > +}
> > +
> > +/// 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<F: Send + Sync, C: Send + Sync> {
> > + /// The memory for the actual content of the fence.
> > + /// Handed over to a [`DriverFence`], or deallocated once the
> > + /// [`DriverFenceAllocation`] drops.
> > + data: KBox<DriverFenceData<F, C>>,
> > +}
> > +
> > +impl<F: Send + Sync + DriverFenceAllowedData, C: Send + Sync> DriverFenceAllocation<F, C> {
> > + /// Create a new allocation slot that can later be used to create a fully
> > + /// initialized [`DriverFence`] without the need to allocate.
> > + pub fn new(fctx: Arc<FenceCtx<F, C>>, data: F) -> Result<Self> {
> > + let fence_data = DriverFenceData {
> > + // `inner` remains uninitialized until a [`DriverFence`] takes over.
> > + inner: Fence {
> > + inner: Opaque::uninit(),
> > + },
> > + fctx,
> > + data,
> > + };
> > +
> > + // In order to support the C dma_fence callbacks, it is necessary for
> > + // a `Fence` and a `DriverFence` to live in the same allocation,
> > + // because the C backend passes a dma_fence, from which the driver most
> > + // likely wants to be able to access its `data` in `DriverFence`.
> > + //
> > + // Hence, we need the manage the memory manually. It will be freed by the
> > + // C backend automatically once the refcount within `Fence` drops to 0.
> > + let data = KBox::new(fence_data, GFP_KERNEL | __GFP_ZERO)?;
> > +
> > + Ok(Self { data })
> > + }
> > +
> > + fn as_raw(&self) -> *mut bindings::dma_fence {
> > + self.data.inner.inner.get()
> > + }
> > +}
> > +
> > +impl<F: Send + Sync, C: Send + Sync> DriverFence<F, C> {
> > + fn as_raw(&self) -> *mut bindings::dma_fence {
> > + // SAFETY: Valid because `self` is valid.
> > + let fence_data = unsafe { &mut *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
> > + /// [`FenceCtx`].
> > + unsafe fn from_raw(ptr: *mut bindings::dma_fence) -> Self {
> > + let opaque_fence = Opaque::cast_from(ptr);
> > +
> > + // SAFETY: Safe due to the function's overall safety requirements.
> > + let fence_ptr = unsafe { container_of!(opaque_fence, Fence, inner) };
> > +
> > + // DriverFenceData is repr(C) and a Fence is its first member.
> > + let fence_data_ptr = fence_ptr as *mut DriverFenceData<F, C>;
> > +
> > + // 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: 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 the `DriverFence` has been dropped.
> > + unsafe {
> > + bindings::dma_fence_lock_irqsave(fence, flag_ptr);
> > + if !bindings::dma_fence_is_signaled_locked(fence) {
> > + if let Err(err) = res {
> > + bindings::dma_fence_set_error(fence, err.to_errno());
> > + }
> > + bindings::dma_fence_signal_locked(fence);
> > + }
> > + bindings::dma_fence_unlock_irqrestore(fence, flag_ptr);
> > + }
>
> This single unsafe blocks spans five different unsafe operations.
Same discussion with Danilo. I'd prefer it this way, but I guess
separate blocks also have some advantages.
>
> > + }
> > +}
> > +
> > +// SAFETY: Fences are literally designed to be shared between threads.
> > +unsafe impl<F: Send + Sync, C: Send + Sync> Send for DriverFence<F, C> {}
> > +
> > +impl<F: Send + Sync, C: Send + Sync> Deref for DriverFence<F, C> {
> > + type Target = F;
> > +
> > + fn deref(&self) -> &Self::Target {
> > + // SAFETY: Thanks to refcounting, `data` is always valid as long as `self` is.
> > + let data = unsafe { &*self.data.as_ptr() };
> > +
> > + &data.data
> > + }
> > +}
> > +
> > +/// A borrowed [`DriverFence`]. All you can do with it is access your user data
> > +/// and obtain a [`Fence`].
> > +pub struct DriverFenceBorrow<F: Send + Sync, C: Send + Sync> {
> > + /// 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<F, C>>,
> > +}
> > +
> > +impl<F: Send + Sync, C: Send + Sync> Deref for DriverFenceBorrow<F, C> {
> > + type Target = F;
> > +
> > + fn deref(&self) -> &Self::Target {
> > + // SAFETY: Thanks to refcounting, `data` is always valid as long as `self` is.
> > + let data = unsafe { &*self.data.as_ptr() };
> > +
> > + &data.data
> > + }
> > +}
> > +
> > +impl<F: Send + Sync, C: Send + Sync> DriverFenceBorrow<F, C> {
> > + fn as_raw(&self) -> *mut bindings::dma_fence {
> > + // SAFETY: Valid because `self` is valid.
> > + let fence_data = unsafe { &mut *self.data.as_ptr() };
> > +
> > + fence_data.inner.inner.get()
> > + }
> > +
> > + /// 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()) }
> > + }
> > +
> > + /// Get a [`DriverFenceBorrow`] from a raw pointer.
> > + ///
> > + /// # Safety
> > + ///
> > + /// `ptr` must point to a raw dma_fence within a [`Fence`] within a [`DriverFenceData`].
> > + unsafe fn from_raw(ptr: *mut bindings::dma_fence) -> Self {
> > + let opaque_fence = Opaque::cast_from(ptr);
> > +
> > + // SAFETY: Safe due to the function's overall safety requirements.
> > + let fence_ptr = unsafe { container_of!(opaque_fence, Fence, inner) };
> > +
> > + // DriverFenceData is repr(C) and a Fence is its first member.
> > + let fence_data_ptr = fence_ptr as *mut DriverFenceData<F, C>;
> > +
> > + // SAFETY: `fence_data_ptr` was created validly above.
> > + let data = unsafe { NonNull::new_unchecked(fence_data_ptr) };
> > +
> > + Self { data }
> > + }
> > +}
> > +
> > +// 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<F: Send + Sync + 'static, C: Send + Sync + 'static> ForeignOwnable
> > + for DriverFence<F, C>
> > +{
> > + // `DriverFence` is merely a wrapper around a raw pointer. Thus, we can just
> > + // use it directly.
> > + type Borrowed<'a> = DriverFenceBorrow<F, C>;
> > + type BorrowedMut<'a> = DriverFenceBorrow<F, C>;
> > +
> > + 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.
> > + core::mem::forget(fence);
>
> Nit: Modern Rust uses ManuallyDrop instead of forget().
You mean still take `self` here, then stuff it into ManuallyDrop and
let it go out of scope, aye?
Thx for the review,
P.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/4] rust: Add dma_fence abstractions
2026-06-01 10:20 ` Alice Ryhl
@ 2026-06-01 12:34 ` Philipp Stanner
2026-06-01 12:55 ` Alice Ryhl
0 siblings, 1 reply; 35+ messages in thread
From: Philipp Stanner @ 2026-06-01 12:34 UTC (permalink / raw)
To: Alice Ryhl, sashiko-reviews; +Cc: Philipp Stanner, linux-media, ojeda
On Mon, 2026-06-01 at 10:20 +0000, Alice Ryhl wrote:
> On Sat, May 30, 2026 at 03:06:21PM +0000, sashiko-bot@kernel.org wrote:
> > > +impl<F: Send + Sync, C: Send + Sync> DriverFence<F, C> {
> > > + fn as_raw(&self) -> *mut bindings::dma_fence {
> > > + // SAFETY: Valid because `self` is valid.
> > > + let fence_data = unsafe { &mut *self.data.as_ptr() };
> >
> > [Severity: High]
> > Does this create an exclusive mutable reference to actively shared memory?
> >
> > DriverFenceData can be accessed concurrently by other threads holding Fence
> > references (for instance, when checking if the fence is signaled). Creating
> > a mutable reference (&mut) in Rust asserts exclusive access and violates
> > aliasing rules, which allows the compiler to make invalid optimization
> > assumptions.
> >
> > Could this use an immutable reference &*self.data.as_ptr() instead?
>
> Yes, please use an immutable reference here.
>
> > > +
> > > + fence_data.inner.inner.get()
> > > + }
> >
> > [ ... ]
> >
> > > +impl<F: Send + Sync, C: Send + Sync> DriverFenceBorrow<F, C> {
> > > + fn as_raw(&self) -> *mut bindings::dma_fence {
> > > + // SAFETY: Valid because `self` is valid.
> > > + let fence_data = unsafe { &mut *self.data.as_ptr() };
> >
> > [Severity: High]
> > Similar to DriverFence::as_raw(), does this also incorrectly create a
> > mutable reference to shared data?
>
> Here as well.
`data` is not shared. By design there is only ever one DriverFence, and
the driver's data (`data.data`) is `Sync`.
But I guess an immutable one should do the trick, too.
>
> > > +
> > > + fence_data.inner.inner.get()
> > > + }
> >
> > [ ... ]
> >
> > > +impl<F: Send + Sync, C: Send + Sync> Drop for DriverFence<F, C> {
> > > + fn drop(&mut self) {
> > > + let fence = self.as_raw();
> > > + let mut fence_flags: usize = 0;
> > > + let flag_ptr = &raw mut fence_flags;
> > > +
> > > + // 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 the `DriverFence` has been dropped.
> > > + unsafe {
> > > + bindings::dma_fence_lock_irqsave(fence, flag_ptr);
> > > + #[allow(unused_unsafe)]
> > > + if warn_on!(!bindings::dma_fence_is_signaled_locked(fence)) {
> > > + bindings::dma_fence_set_error(fence, ECANCELED as i32);
> >
> > [Severity: High]
> > Is it intentional to pass a positive error code here?
> >
> > bindings::ECANCELED evaluates to a positive integer, but
> > dma_fence_set_error() expects a negative error code (and triggers a WARN_ON
> > for positive values). A positive value might also be interpreted as success
> > by callers checking for values < 0.
> >
> > Should this be - (bindings::ECANCELED as i32)?
>
> Please use ECANCELED.to_errno(). Do not import these from bindings.
>
> > > + bindings::dma_fence_signal_locked(fence);
> > > + }
> > > + bindings::dma_fence_unlock_irqrestore(fence, flag_ptr);
> > > + }
> > > +
> > > + // SAFETY: `self.data` is owned by the DriverFence, but could be accessed
> > > + // through some dma_fence callbacks right now. Access is being revoked
> > > + // above by signalling the fence. The DriverFenceAllowedData trait
> > > + // ensures that the data either does not need drop, or if it does it
> > > + // lives in a RcuBox which will delay dropping by one grace period, hence
> > > + // ensuring that all readers have disappeared.
> > > + unsafe { drop_in_place(self.data.as_ptr()) };
> >
> > [Severity: Critical]
> > Could this eagerly drop FenceCtx and user data before the C dma_fence
> > refcount reaches 0?
> >
> > The C dma_fence object can remain alive as long as consumers (like sync_file
> > or the GPU scheduler) hold references to it. If the backend later invokes
> > callbacks like ops->get_driver_name(), FenceCtx::get_driver_name() would
> > attempt to read fctx.driver_name from the already-dropped FenceCtx:
> >
> > FenceCtx::get_driver_name() {
> > ...
> > fctx.driver_name.as_char_ptr()
> > }
> >
> > Can this result in a use-after-free?
>
> Hmm, I don't know about what sahisko said, but I don't think this is the
> right way to do rcu freeing. I think the type's destructor should be
> reserved for cases where the value becomes immediately unusable.
We could guard the strings with RcuBox, but we could not then guard the
FenceCtx code against code UAF if we don't have the rcu_barrier().
Or could we?
If a rust module unloads, module::remove() should contain an
rcu_barrier() (right??). Would that be enough to guard against the
FenceCtx code being unloaded?
>
> For example, let's say I'm using RcuBox<_> here. Yes, the data you get
> from dereferencing the RcuBox will stay alive for a grace period, but
> IMO once you run the destructor of the box itself, the *pointer* becomes
> immediately unusable.
I don't know why you're stressing the pointer?
The trick above is simply that drop / dealloc *and* code unloading is
delayed by a grace period.
P.
>
> Alice
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/4] rust: Add dma_fence abstractions
2026-06-01 11:17 ` Philipp Stanner
@ 2026-06-01 12:35 ` Boris Brezillon
0 siblings, 0 replies; 35+ messages in thread
From: Boris Brezillon @ 2026-06-01 12:35 UTC (permalink / raw)
To: Philipp Stanner
Cc: phasta, Alice Ryhl, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Sumit Semwal,
Christian König, Paul E. McKenney, Frederic Weisbecker,
Neeraj Upadhyay, Joel Fernandes, Josh Triplett, Uladzislau Rezki,
Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, Zqiang,
Daniel Almeida, Greg Kroah-Hartman, Igor Korotin, Lorenzo Stoakes,
Alexandre Courbot, FUJITA Tomonori, Krishna Ketan Rai,
Shankari Anand, manos, linux-kernel, rust-for-linux, linux-media,
dri-devel, linaro-mm-sig, rcu
On Mon, 01 Jun 2026 13:17:27 +0200
Philipp Stanner <phasta@mailbox.org> wrote:
> On Mon, 2026-06-01 at 12:59 +0200, Boris Brezillon wrote:
> > On Mon, 1 Jun 2026 10:36:06 +0000
> > Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > > > +};
> > > > +
> > > > +use bindings::ECANCELED;
> > > > +
> > > > +use kernel::str::CString;
> > > > +use kernel::sync::{
> > > > + aref::{
> > > > + ARef,
> > > > + AlwaysRefCounted, //
> > > > + },
> > > > + Arc,
> > > > + ArcBorrow, //
> > > > +};
> > > > +
> > > > +/// 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 {}
> > >
> > > This empty trait is unused.
> >
> > I had initially suggested to add the F type (AKA FenceData) passed
> > around in multiple places type as an associated type
> >
> > pub trait FenceCtxOps {
> > type FenceData: Send + Sync;
> > }
> >
> > so we don't have to pass both F and C. The reasoning here is that:
> >
> > 1. We expect we'll have to define more methods to the FenceCtxOps trait
> > at some point, so adding it now kinda makes sense.
> >
> > 2. In the current design, we've assumed that a Fence can't live/be
> > created outside of a given context, so there's no world where the
> > FenceData wouldn't be known by the FenceCtx implementation, and forcing
> > users to pass F and C around seems needlessly verbose.
>
> I had investigated that, but found that this causes us to write things
> like
>
> DriverFence<T> (where T is the FencCtx generic)
>
> and then in the actual implementation use
>
> T::FenceData
>
> which reads very weird IMO. Because now for reasons a fence's own data
> are not referred to in its own implementation, but you derive it from
> the context.
Well, I actually think that's a good thing, because DriverFence and
FenceCtx are tightly related: FenceCtx<F, C> instantiates and manages
DriverFence<F, C> fences, and DriverFenceData<F, C> has an Arc to a
FenceCtx<F, C>.
>
> I do prefer it in a way where the DriverFence generic does appear in
> said fence's actual code, on equal rank with the FenceCtx.
Question is, can you really have random <F, C> combination or is C
dictating which F you'll get attached to the DriverFence? If a given
context can't handle more than one type of fence, I don't really see the
point of passing both around when one could be directly derived from the
other, and since the trait we consider defining for the future is on the
FenceCtx (FenceCtxOps), it makes sense to have FenceData defined as an
associated type of FenceCtx.
>
> I suppose that is actually one use case for which PhantomData does
> exist.
Yeah, I don't, it just feels weird to pass both around, and it doesn't
seem to match what we've been doing in other places (drm::Driver,
drm::Object, ...).
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/4] rust: Add dma_fence abstractions
2026-06-01 10:36 ` Alice Ryhl
2026-06-01 10:59 ` Boris Brezillon
2026-06-01 12:26 ` Philipp Stanner
@ 2026-06-01 12:37 ` Boris Brezillon
2 siblings, 0 replies; 35+ messages in thread
From: Boris Brezillon @ 2026-06-01 12:37 UTC (permalink / raw)
To: Alice Ryhl
Cc: Philipp Stanner, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Sumit Semwal,
Christian König, Paul E. McKenney, Frederic Weisbecker,
Neeraj Upadhyay, Joel Fernandes, Josh Triplett, Uladzislau Rezki,
Steven Rostedt, Mathieu Desnoyers, Lai Jiangshan, Zqiang,
Daniel Almeida, Greg Kroah-Hartman, Igor Korotin, Lorenzo Stoakes,
Alexandre Courbot, FUJITA Tomonori, Krishna Ketan Rai,
Shankari Anand, manos, linux-kernel, rust-for-linux, linux-media,
dri-devel, linaro-mm-sig, rcu
On Mon, 1 Jun 2026 10:36:06 +0000
Alice Ryhl <aliceryhl@google.com> wrote:
> > +pub trait FenceCtxOps {}
>
> This empty trait is unused.
>
> > +/// 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<F: Send + Sync, C: Send + Sync> {
BTW, if you define a FenceCtxOps trait, this should be:
pub struct FenceCtx<F: Send + Sync, C: FenceCtxOps + Send + Sync> {
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/4] rust: Add dma_fence abstractions
2026-06-01 12:26 ` Philipp Stanner
@ 2026-06-01 12:39 ` Alice Ryhl
2026-06-01 12:47 ` Philipp Stanner
0 siblings, 1 reply; 35+ messages in thread
From: Alice Ryhl @ 2026-06-01 12:39 UTC (permalink / raw)
To: phasta
Cc: Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Trevor Gross, Danilo Krummrich,
Sumit Semwal, Christian König, Paul E. McKenney,
Frederic Weisbecker, Neeraj Upadhyay, Joel Fernandes,
Josh Triplett, Uladzislau Rezki, Steven Rostedt,
Mathieu Desnoyers, Lai Jiangshan, Zqiang, Daniel Almeida,
Greg Kroah-Hartman, Igor Korotin, Lorenzo Stoakes,
Alexandre Courbot, FUJITA Tomonori, Krishna Ketan Rai,
Shankari Anand, manos, Boris Brezillon, linux-kernel,
rust-for-linux, linux-media, dri-devel, linaro-mm-sig, rcu
On Mon, Jun 01, 2026 at 02:26:17PM +0200, Philipp Stanner wrote:
> On Mon, 2026-06-01 at 10:36 +0000, Alice Ryhl wrote:
> > On Sat, May 30, 2026 at 04:35:11PM +0200, Philipp Stanner wrote:
> > > + unsafe {
> > > + bindings::dma_fence_remove_callback(self.fence.as_raw(), self.cb.get());
> > > + }
> >
> > Formatting nit: Usually the ; goes outside the unsafe block.
>
> I could have sworn that it was rustfmt who did that? Maybe because the
> ; was inside to begin with.
Indeed, rustfmt will not change whether the ; is inside or outside the
unsafe block.
> > > +/// A trait to enforce that all data in a [`DriverFence`] either does not need
> > > +/// drop, or lives in a [`RcuBox`].
> > > +pub trait DriverFenceAllowedData: private::Sealed {}
> > > +
> > > +mod private {
> > > + pub trait Sealed {}
> > > +}
> > > +
> > > +impl<F: Copy> DriverFenceAllowedData for F {}
> > > +impl<F: Send> DriverFenceAllowedData for RcuBox<F> {}
> > > +
> > > +impl<F: Copy> private::Sealed for F {}
> > > +impl<F: Send> private::Sealed for RcuBox<F> {}
> >
> > Why sealed? Just make the trait unsafe and require the things you
> > require from the user.
>
> This is far better. We definitely only allow the user to pass A or B,
> and only then it compiles.
What if I have another type that I want to use here? For example, maybe
I have a struct containing a copy field and an RcuBox. Or maybe I have
an ARef<_> of some C type that uses rcu for cleanup. Then I must edit
this file to add support for it?
> The unsafe implementation could be messed up.
>
> I thought that's what Sealed is for. Or isn't it?
Sealed is for making 100% sure that downstream crates/drivers cannot
provide their own implementations. But I don't see why you need that.
All you require is that the value remains valid for one grace period
after cleanup begins. As long as the type satisfies that, you are happy.
An unsafe trait can require that sort of requirement from the user.
I think what you want is expressed well by `RcuFreeSafe` from this
thread:
https://rust-for-linux.zulipchat.com/#narrow/channel/291566-Library/topic/Consolidate.20.60PollCondVarBox.60.20into.20.60Rcu.2ABox.60/near/598726724
> > > +/// 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, FenceCb, FenceCbRegistration};
> > > +/// use kernel::str::CString;
> > > +/// use kernel::sync::{
> > > +/// aref::ARef,
> > > +/// rcu::RcuBox, //
> > > +/// };
> > > +/// use core::ops::Deref;
> > > +/// use core::fmt::Display;
> >
> > Use fmt traits from kernel instead. (Actually, I don't think you use
> > Display at all here?)
>
> I tried, see a few lines below:
>
> >
> > > +/// struct CallbackData { }
> > > +///
> > > +/// impl FenceCb for CallbackData {
> > > +/// fn called(&mut self) {
> > > +/// pr_info!("DmaFence callback executed.\n");
> > > +/// }
> > > +/// }
> > > +///
> > > +/// let driver_name = CString::try_from_fmt(fmt!("dummy_driver"))?;
> > > +/// let timeline_name = CString::try_from_fmt(fmt!("dummy_timeline"))?;
> > > +///
> > > +/// let fctx = FenceCtx::new(driver_name, timeline_name, ())?;
> > > +///
> > > +/// let fence_data = CString::try_from_fmt(fmt!("dummy_data"))?;
> > > +/// // DriverFence::data must either not need drop, or live in an RcuBox.
> > > +/// let fence_data = RcuBox::new(fence_data, GFP_KERNEL)?;
> > > +///
> > > +/// let fence_alloc = fctx.as_arc_borrow().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)?;
> > > +///
> > > +/// // DriverFence implements Deref.
> > > +/// // FIXME: unit test claims that CString does not implement Display. Why?
> > > +/// // pr_info!("Fence's inner data is: {}", fence.deref().deref());
>
> Lazily, I was hoping that someone here will tell me how that is
> supposed to be done correctly 8-)
This specific code could be written cleaner as &**fence, but it looks
like CString should implement fmt::Display like CStr does.
> > > +// 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<F: Send + Sync + 'static, C: Send + Sync + 'static> ForeignOwnable
> > > + for DriverFence<F, C>
> > > +{
> > > + // `DriverFence` is merely a wrapper around a raw pointer. Thus, we can just
> > > + // use it directly.
> > > + type Borrowed<'a> = DriverFenceBorrow<F, C>;
> > > + type BorrowedMut<'a> = DriverFenceBorrow<F, C>;
> > > +
> > > + 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.
> > > + core::mem::forget(fence);
> >
> > Nit: Modern Rust uses ManuallyDrop instead of forget().
>
> You mean still take `self` here, then stuff it into ManuallyDrop and
> let it go out of scope, aye?
I mean this:
let fence = ManuallyDrop::new(self);
let ptr = fence.as_raw();
This avoids moving `fence` after calling `as_raw()`.
Alice
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/4] rust: Add dma_fence abstractions
2026-06-01 12:39 ` Alice Ryhl
@ 2026-06-01 12:47 ` Philipp Stanner
2026-06-01 13:22 ` Alice Ryhl
0 siblings, 1 reply; 35+ messages in thread
From: Philipp Stanner @ 2026-06-01 12:47 UTC (permalink / raw)
To: Alice Ryhl, phasta
Cc: Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Trevor Gross, Danilo Krummrich,
Sumit Semwal, Christian König, Paul E. McKenney,
Frederic Weisbecker, Neeraj Upadhyay, Joel Fernandes,
Josh Triplett, Uladzislau Rezki, Steven Rostedt,
Mathieu Desnoyers, Lai Jiangshan, Zqiang, Daniel Almeida,
Greg Kroah-Hartman, Igor Korotin, Lorenzo Stoakes,
Alexandre Courbot, FUJITA Tomonori, Krishna Ketan Rai,
Shankari Anand, manos, Boris Brezillon, linux-kernel,
rust-for-linux, linux-media, dri-devel, linaro-mm-sig, rcu
On Mon, 2026-06-01 at 12:39 +0000, Alice Ryhl wrote:
> On Mon, Jun 01, 2026 at 02:26:17PM +0200, Philipp Stanner wrote:
> > On Mon, 2026-06-01 at 10:36 +0000, Alice Ryhl wrote:
> > > On Sat, May 30, 2026 at 04:35:11PM +0200, Philipp Stanner wrote:
> > > > + unsafe {
> > > > + bindings::dma_fence_remove_callback(self.fence.as_raw(), self.cb.get());
> > > > + }
> > >
> > > Formatting nit: Usually the ; goes outside the unsafe block.
> >
> > I could have sworn that it was rustfmt who did that? Maybe because the
> > ; was inside to begin with.
>
> Indeed, rustfmt will not change whether the ; is inside or outside the
> unsafe block.
>
> > > > +/// A trait to enforce that all data in a [`DriverFence`] either does not need
> > > > +/// drop, or lives in a [`RcuBox`].
> > > > +pub trait DriverFenceAllowedData: private::Sealed {}
> > > > +
> > > > +mod private {
> > > > + pub trait Sealed {}
> > > > +}
> > > > +
> > > > +impl<F: Copy> DriverFenceAllowedData for F {}
> > > > +impl<F: Send> DriverFenceAllowedData for RcuBox<F> {}
> > > > +
> > > > +impl<F: Copy> private::Sealed for F {}
> > > > +impl<F: Send> private::Sealed for RcuBox<F> {}
> > >
> > > Why sealed? Just make the trait unsafe and require the things you
> > > require from the user.
> >
> > This is far better. We definitely only allow the user to pass A or B,
> > and only then it compiles.
>
> What if I have another type that I want to use here? For example, maybe
> I have a struct containing a copy field and an RcuBox. Or maybe I have
> an ARef<_> of some C type that uses rcu for cleanup. Then I must edit
> this file to add support for it?
>
> > The unsafe implementation could be messed up.
> >
> > I thought that's what Sealed is for. Or isn't it?
>
> Sealed is for making 100% sure that downstream crates/drivers cannot
> provide their own implementations. But I don't see why you need that.
> All you require is that the value remains valid for one grace period
> after cleanup begins. As long as the type satisfies that, you are happy.
> An unsafe trait can require that sort of requirement from the user.
>
> I think what you want is expressed well by `RcuFreeSafe` from this
> thread:
> https://rust-for-linux.zulipchat.com/#narrow/channel/291566-Library/topic/Consolidate.20.60PollCondVarBox.60.20into.20.60Rcu.2ABox.60/near/598726724
>
I guess this is a question of design principles. If you demand an
RcuBox, you have a guarantee that it's safe.
If you demand an unsafe trait, you open the possibility for people
messing up.
Due to the unsafe-contract you'd have moved the responsibility for the
soundness to the driver.
I would not want to block your suggestion, but I am not sure whether
that's really the better design idea.
> > >
P.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/4] rust: Add dma_fence abstractions
2026-06-01 12:34 ` Philipp Stanner
@ 2026-06-01 12:55 ` Alice Ryhl
2026-06-01 13:14 ` Philipp Stanner
0 siblings, 1 reply; 35+ messages in thread
From: Alice Ryhl @ 2026-06-01 12:55 UTC (permalink / raw)
To: phasta; +Cc: sashiko-reviews, linux-media, ojeda
On Mon, Jun 1, 2026 at 2:34 PM Philipp Stanner <phasta@mailbox.org> wrote:
>
> On Mon, 2026-06-01 at 10:20 +0000, Alice Ryhl wrote:
> > On Sat, May 30, 2026 at 03:06:21PM +0000, sashiko-bot@kernel.org wrote:
> > > > +impl<F: Send + Sync, C: Send + Sync> DriverFence<F, C> {
> > > > + fn as_raw(&self) -> *mut bindings::dma_fence {
> > > > + // SAFETY: Valid because `self` is valid.
> > > > + let fence_data = unsafe { &mut *self.data.as_ptr() };
> > >
> > > [Severity: High]
> > > Does this create an exclusive mutable reference to actively shared memory?
> > >
> > > DriverFenceData can be accessed concurrently by other threads holding Fence
> > > references (for instance, when checking if the fence is signaled). Creating
> > > a mutable reference (&mut) in Rust asserts exclusive access and violates
> > > aliasing rules, which allows the compiler to make invalid optimization
> > > assumptions.
> > >
> > > Could this use an immutable reference &*self.data.as_ptr() instead?
> >
> > Yes, please use an immutable reference here.
> >
> > > > +
> > > > + fence_data.inner.inner.get()
> > > > + }
> > >
> > > [ ... ]
> > >
> > > > +impl<F: Send + Sync, C: Send + Sync> DriverFenceBorrow<F, C> {
> > > > + fn as_raw(&self) -> *mut bindings::dma_fence {
> > > > + // SAFETY: Valid because `self` is valid.
> > > > + let fence_data = unsafe { &mut *self.data.as_ptr() };
> > >
> > > [Severity: High]
> > > Similar to DriverFence::as_raw(), does this also incorrectly create a
> > > mutable reference to shared data?
> >
> > Here as well.
>
> `data` is not shared. By design there is only ever one DriverFence, and
> the driver's data (`data.data`) is `Sync`.
>
> But I guess an immutable one should do the trick, too.
There's only one DriverFence, but I can perform shared access to that
one DriverFence from two threads in parallel. You made the type Sync,
and this is what you are allowing when you do so.
> > > > + bindings::dma_fence_signal_locked(fence);
> > > > + }
> > > > + bindings::dma_fence_unlock_irqrestore(fence, flag_ptr);
> > > > + }
> > > > +
> > > > + // SAFETY: `self.data` is owned by the DriverFence, but could be accessed
> > > > + // through some dma_fence callbacks right now. Access is being revoked
> > > > + // above by signalling the fence. The DriverFenceAllowedData trait
> > > > + // ensures that the data either does not need drop, or if it does it
> > > > + // lives in a RcuBox which will delay dropping by one grace period, hence
> > > > + // ensuring that all readers have disappeared.
> > > > + unsafe { drop_in_place(self.data.as_ptr()) };
> > >
> > > [Severity: Critical]
> > > Could this eagerly drop FenceCtx and user data before the C dma_fence
> > > refcount reaches 0?
> > >
> > > The C dma_fence object can remain alive as long as consumers (like sync_file
> > > or the GPU scheduler) hold references to it. If the backend later invokes
> > > callbacks like ops->get_driver_name(), FenceCtx::get_driver_name() would
> > > attempt to read fctx.driver_name from the already-dropped FenceCtx:
> > >
> > > FenceCtx::get_driver_name() {
> > > ...
> > > fctx.driver_name.as_char_ptr()
> > > }
> > >
> > > Can this result in a use-after-free?
> >
> > Hmm, I don't know about what sahisko said, but I don't think this is the
> > right way to do rcu freeing. I think the type's destructor should be
> > reserved for cases where the value becomes immediately unusable.
>
> We could guard the strings with RcuBox, but we could not then guard the
> FenceCtx code against code UAF if we don't have the rcu_barrier().
>
> Or could we?
>
> If a rust module unloads, module::remove() should contain an
> rcu_barrier() (right??). Would that be enough to guard against the
> FenceCtx code being unloaded?
>
> >
> > For example, let's say I'm using RcuBox<_> here. Yes, the data you get
> > from dereferencing the RcuBox will stay alive for a grace period, but
> > IMO once you run the destructor of the box itself, the *pointer* becomes
> > immediately unusable.
>
> I don't know why you're stressing the pointer?
>
> The trick above is simply that drop / dealloc *and* code unloading is
> delayed by a grace period.
Sorry let me try to rephrase. I'm not worried about the stuff behind
the pointer. After all, you're using RcuBox to protect that stuff.
What I'm worried about is the pointer itself. You invoked
drop_in_place() on the pointer to the fence context, so even though
the fence context may be valid for another grace period, the *pointer*
to the fence context is not. The pointer could have been zeroed by the
destructor.
Actually maybe I am worried about the strings too. Let's say that one
thread does:
rcu_read_lock()
get_driver_name()
// .. read the driver name
rcu_read_unlock()
and another thread signals the fence. If there are no queued
call_rcu() callbacks right now, then the rcu_barrier() could be a
no-op and the rcu reader will UAF on the driver name. It looks like
you need synchronize_rcu() rather than rcu_barrier() here?
Alice
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/4] rust: Add dma_fence abstractions
2026-06-01 12:55 ` Alice Ryhl
@ 2026-06-01 13:14 ` Philipp Stanner
2026-06-01 13:30 ` Philipp Stanner
2026-06-01 13:44 ` Alice Ryhl
0 siblings, 2 replies; 35+ messages in thread
From: Philipp Stanner @ 2026-06-01 13:14 UTC (permalink / raw)
To: Alice Ryhl, phasta; +Cc: sashiko-reviews, linux-media, ojeda, Boris Brezillon
+Cc Boris
On Mon, 2026-06-01 at 14:55 +0200, Alice Ryhl wrote:
> On Mon, Jun 1, 2026 at 2:34 PM Philipp Stanner <phasta@mailbox.org> wrote:
> >
> > On Mon, 2026-06-01 at 10:20 +0000, Alice Ryhl wrote:
> > > On Sat, May 30, 2026 at 03:06:21PM +0000, sashiko-bot@kernel.org wrote:
> > > > > +impl<F: Send + Sync, C: Send + Sync> DriverFence<F, C> {
> > > > > + fn as_raw(&self) -> *mut bindings::dma_fence {
> > > > > + // SAFETY: Valid because `self` is valid.
> > > > > + let fence_data = unsafe { &mut *self.data.as_ptr() };
> > > >
> > > > [Severity: High]
> > > > Does this create an exclusive mutable reference to actively shared memory?
> > > >
> > > > DriverFenceData can be accessed concurrently by other threads holding Fence
> > > > references (for instance, when checking if the fence is signaled). Creating
> > > > a mutable reference (&mut) in Rust asserts exclusive access and violates
> > > > aliasing rules, which allows the compiler to make invalid optimization
> > > > assumptions.
> > > >
> > > > Could this use an immutable reference &*self.data.as_ptr() instead?
> > >
> > > Yes, please use an immutable reference here.
> > >
> > > > > +
> > > > > + fence_data.inner.inner.get()
> > > > > + }
> > > >
> > > > [ ... ]
> > > >
> > > > > +impl<F: Send + Sync, C: Send + Sync> DriverFenceBorrow<F, C> {
> > > > > + fn as_raw(&self) -> *mut bindings::dma_fence {
> > > > > + // SAFETY: Valid because `self` is valid.
> > > > > + let fence_data = unsafe { &mut *self.data.as_ptr() };
> > > >
> > > > [Severity: High]
> > > > Similar to DriverFence::as_raw(), does this also incorrectly create a
> > > > mutable reference to shared data?
> > >
> > > Here as well.
> >
> > `data` is not shared. By design there is only ever one DriverFence, and
> > the driver's data (`data.data`) is `Sync`.
> >
> > But I guess an immutable one should do the trick, too.
>
> There's only one DriverFence, but I can perform shared access to that
> one DriverFence from two threads in parallel. You made the type Sync,
> and this is what you are allowing when you do so.
Nope, DriverFence is just Send, not Sync.
data.data is Sync, but `data` in the code above is not the actual user
data, but the memory backing it up.
>
> > > > > + bindings::dma_fence_signal_locked(fence);
> > > > > + }
> > > > > + bindings::dma_fence_unlock_irqrestore(fence, flag_ptr);
> > > > > + }
> > > > > +
> > > > > + // SAFETY: `self.data` is owned by the DriverFence, but could be accessed
> > > > > + // through some dma_fence callbacks right now. Access is being revoked
> > > > > + // above by signalling the fence. The DriverFenceAllowedData trait
> > > > > + // ensures that the data either does not need drop, or if it does it
> > > > > + // lives in a RcuBox which will delay dropping by one grace period, hence
> > > > > + // ensuring that all readers have disappeared.
> > > > > + unsafe { drop_in_place(self.data.as_ptr()) };
> > > >
> > > > [Severity: Critical]
> > > > Could this eagerly drop FenceCtx and user data before the C dma_fence
> > > > refcount reaches 0?
> > > >
> > > > The C dma_fence object can remain alive as long as consumers (like sync_file
> > > > or the GPU scheduler) hold references to it. If the backend later invokes
> > > > callbacks like ops->get_driver_name(), FenceCtx::get_driver_name() would
> > > > attempt to read fctx.driver_name from the already-dropped FenceCtx:
> > > >
> > > > FenceCtx::get_driver_name() {
> > > > ...
> > > > fctx.driver_name.as_char_ptr()
> > > > }
> > > >
> > > > Can this result in a use-after-free?
> > >
> > > Hmm, I don't know about what sahisko said, but I don't think this is the
> > > right way to do rcu freeing. I think the type's destructor should be
> > > reserved for cases where the value becomes immediately unusable.
> >
> > We could guard the strings with RcuBox, but we could not then guard the
> > FenceCtx code against code UAF if we don't have the rcu_barrier().
> >
> > Or could we?
> >
> > If a rust module unloads, module::remove() should contain an
> > rcu_barrier() (right??). Would that be enough to guard against the
> > FenceCtx code being unloaded?
> >
> > >
> > > For example, let's say I'm using RcuBox<_> here. Yes, the data you get
> > > from dereferencing the RcuBox will stay alive for a grace period, but
> > > IMO once you run the destructor of the box itself, the *pointer* becomes
> > > immediately unusable.
> >
> > I don't know why you're stressing the pointer?
> >
> > The trick above is simply that drop / dealloc *and* code unloading is
> > delayed by a grace period.
>
> Sorry let me try to rephrase. I'm not worried about the stuff behind
> the pointer. After all, you're using RcuBox to protect that stuff.
> What I'm worried about is the pointer itself. You invoked
> drop_in_place() on the pointer to the fence context,
>
on the pointer to DriverFenceData, which contains a refcount to the
FenceCtx, which might then want to drop.
> so even though
> the fence context may be valid for another grace period, the *pointer*
> to the fence context is not. The pointer could have been zeroed by the
> destructor.
That particular pointer to the DriverFenceData could have been zeroed.
But potential other accessors have already crafted themselves a new
pointer to the, by the power of RCU, still valid data. That new pointer
is container-of-ed from struct dma_fence *f.
>
> Actually maybe I am worried about the strings too. Let's say that one
> thread does:
>
> rcu_read_lock()
> get_driver_name()
> // .. read the driver name
> rcu_read_unlock()
>
> and another thread signals the fence. If there are no queued
> call_rcu() callbacks right now, then the rcu_barrier() could be a
> no-op and the rcu reader will UAF on the driver name. It looks like
> you need synchronize_rcu() rather than rcu_barrier() here?
It would seem to me that you are right.
Boris, do you have an opinion on that?
P.
>
> Alice
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/4] rust: Add dma_fence abstractions
2026-06-01 12:47 ` Philipp Stanner
@ 2026-06-01 13:22 ` Alice Ryhl
2026-06-01 13:23 ` Philipp Stanner
0 siblings, 1 reply; 35+ messages in thread
From: Alice Ryhl @ 2026-06-01 13:22 UTC (permalink / raw)
To: phasta
Cc: Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Trevor Gross, Danilo Krummrich,
Sumit Semwal, Christian König, Paul E. McKenney,
Frederic Weisbecker, Neeraj Upadhyay, Joel Fernandes,
Josh Triplett, Uladzislau Rezki, Steven Rostedt,
Mathieu Desnoyers, Lai Jiangshan, Zqiang, Daniel Almeida,
Greg Kroah-Hartman, Igor Korotin, Lorenzo Stoakes,
Alexandre Courbot, FUJITA Tomonori, Krishna Ketan Rai,
Shankari Anand, manos, Boris Brezillon, linux-kernel,
rust-for-linux, linux-media, dri-devel, linaro-mm-sig, rcu
On Mon, Jun 1, 2026 at 2:47 PM Philipp Stanner <phasta@mailbox.org> wrote:
>
> On Mon, 2026-06-01 at 12:39 +0000, Alice Ryhl wrote:
> > On Mon, Jun 01, 2026 at 02:26:17PM +0200, Philipp Stanner wrote:
> > > On Mon, 2026-06-01 at 10:36 +0000, Alice Ryhl wrote:
> > > > On Sat, May 30, 2026 at 04:35:11PM +0200, Philipp Stanner wrote:
> > > > > +/// A trait to enforce that all data in a [`DriverFence`] either does not need
> > > > > +/// drop, or lives in a [`RcuBox`].
> > > > > +pub trait DriverFenceAllowedData: private::Sealed {}
> > > > > +
> > > > > +mod private {
> > > > > + pub trait Sealed {}
> > > > > +}
> > > > > +
> > > > > +impl<F: Copy> DriverFenceAllowedData for F {}
> > > > > +impl<F: Send> DriverFenceAllowedData for RcuBox<F> {}
> > > > > +
> > > > > +impl<F: Copy> private::Sealed for F {}
> > > > > +impl<F: Send> private::Sealed for RcuBox<F> {}
> > > >
> > > > Why sealed? Just make the trait unsafe and require the things you
> > > > require from the user.
> > >
> > > This is far better. We definitely only allow the user to pass A or B,
> > > and only then it compiles.
> >
> > What if I have another type that I want to use here? For example, maybe
> > I have a struct containing a copy field and an RcuBox. Or maybe I have
> > an ARef<_> of some C type that uses rcu for cleanup. Then I must edit
> > this file to add support for it?
> >
> > > The unsafe implementation could be messed up.
> > >
> > > I thought that's what Sealed is for. Or isn't it?
> >
> > Sealed is for making 100% sure that downstream crates/drivers cannot
> > provide their own implementations. But I don't see why you need that.
> > All you require is that the value remains valid for one grace period
> > after cleanup begins. As long as the type satisfies that, you are happy.
> > An unsafe trait can require that sort of requirement from the user.
> >
> > I think what you want is expressed well by `RcuFreeSafe` from this
> > thread:
> > https://rust-for-linux.zulipchat.com/#narrow/channel/291566-Library/topic/Consolidate.20.60PollCondVarBox.60.20into.20.60Rcu.2ABox.60/near/598726724
> >
>
> I guess this is a question of design principles. If you demand an
> RcuBox, you have a guarantee that it's safe.
>
> If you demand an unsafe trait, you open the possibility for people
> messing up.
>
> Due to the unsafe-contract you'd have moved the responsibility for the
> soundness to the driver.
>
> I would not want to block your suggestion, but I am not sure whether
> that's really the better design idea.
Yes, it's a design principle. You are saying that if someone needs to
do X but might get it wrong, we should take away the ability to do X?
I fundamentally disagree with that principle. Unsafe traits is the
tool Rust created for the exact problem you have; marking places where
you should be careful is the entire point of 'unsafe'.
Alice
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/4] rust: Add dma_fence abstractions
2026-06-01 13:22 ` Alice Ryhl
@ 2026-06-01 13:23 ` Philipp Stanner
2026-06-01 13:27 ` Alice Ryhl
0 siblings, 1 reply; 35+ messages in thread
From: Philipp Stanner @ 2026-06-01 13:23 UTC (permalink / raw)
To: Alice Ryhl, phasta
Cc: Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Trevor Gross, Danilo Krummrich,
Sumit Semwal, Christian König, Paul E. McKenney,
Frederic Weisbecker, Neeraj Upadhyay, Joel Fernandes,
Josh Triplett, Uladzislau Rezki, Steven Rostedt,
Mathieu Desnoyers, Lai Jiangshan, Zqiang, Daniel Almeida,
Greg Kroah-Hartman, Igor Korotin, Lorenzo Stoakes,
Alexandre Courbot, FUJITA Tomonori, Krishna Ketan Rai,
Shankari Anand, manos, Boris Brezillon, linux-kernel,
rust-for-linux, linux-media, dri-devel, linaro-mm-sig, rcu
On Mon, 2026-06-01 at 15:22 +0200, Alice Ryhl wrote:
> On Mon, Jun 1, 2026 at 2:47 PM Philipp Stanner <phasta@mailbox.org> wrote:
> >
> > On Mon, 2026-06-01 at 12:39 +0000, Alice Ryhl wrote:
> > > On Mon, Jun 01, 2026 at 02:26:17PM +0200, Philipp Stanner wrote:
> > > > On Mon, 2026-06-01 at 10:36 +0000, Alice Ryhl wrote:
> > > > > On Sat, May 30, 2026 at 04:35:11PM +0200, Philipp Stanner wrote:
> > > > > > +/// A trait to enforce that all data in a [`DriverFence`] either does not need
> > > > > > +/// drop, or lives in a [`RcuBox`].
> > > > > > +pub trait DriverFenceAllowedData: private::Sealed {}
> > > > > > +
> > > > > > +mod private {
> > > > > > + pub trait Sealed {}
> > > > > > +}
> > > > > > +
> > > > > > +impl<F: Copy> DriverFenceAllowedData for F {}
> > > > > > +impl<F: Send> DriverFenceAllowedData for RcuBox<F> {}
> > > > > > +
> > > > > > +impl<F: Copy> private::Sealed for F {}
> > > > > > +impl<F: Send> private::Sealed for RcuBox<F> {}
> > > > >
> > > > > Why sealed? Just make the trait unsafe and require the things you
> > > > > require from the user.
> > > >
> > > > This is far better. We definitely only allow the user to pass A or B,
> > > > and only then it compiles.
> > >
> > > What if I have another type that I want to use here? For example, maybe
> > > I have a struct containing a copy field and an RcuBox. Or maybe I have
> > > an ARef<_> of some C type that uses rcu for cleanup. Then I must edit
> > > this file to add support for it?
> > >
> > > > The unsafe implementation could be messed up.
> > > >
> > > > I thought that's what Sealed is for. Or isn't it?
> > >
> > > Sealed is for making 100% sure that downstream crates/drivers cannot
> > > provide their own implementations. But I don't see why you need that.
> > > All you require is that the value remains valid for one grace period
> > > after cleanup begins. As long as the type satisfies that, you are happy.
> > > An unsafe trait can require that sort of requirement from the user.
> > >
> > > I think what you want is expressed well by `RcuFreeSafe` from this
> > > thread:
> > > https://rust-for-linux.zulipchat.com/#narrow/channel/291566-Library/topic/Consolidate.20.60PollCondVarBox.60.20into.20.60Rcu.2ABox.60/near/598726724
> > >
> >
> > I guess this is a question of design principles. If you demand an
> > RcuBox, you have a guarantee that it's safe.
> >
> > If you demand an unsafe trait, you open the possibility for people
> > messing up.
> >
> > Due to the unsafe-contract you'd have moved the responsibility for the
> > soundness to the driver.
> >
> > I would not want to block your suggestion, but I am not sure whether
> > that's really the better design idea.
>
> Yes, it's a design principle. You are saying that if someone needs to
> do X but might get it wrong, we should take away the ability to do X?
> I fundamentally disagree with that principle. Unsafe traits is the
> tool Rust created for the exact problem you have; marking places where
> you should be careful is the entire point of 'unsafe'.
I mean, fine by me if the others don't disagree.
But when then do you ever really want a Sealed trait?
P.
>
> Alice
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/4] rust: Add dma_fence abstractions
2026-06-01 13:23 ` Philipp Stanner
@ 2026-06-01 13:27 ` Alice Ryhl
0 siblings, 0 replies; 35+ messages in thread
From: Alice Ryhl @ 2026-06-01 13:27 UTC (permalink / raw)
To: phasta
Cc: Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Trevor Gross, Danilo Krummrich,
Sumit Semwal, Christian König, Paul E. McKenney,
Frederic Weisbecker, Neeraj Upadhyay, Joel Fernandes,
Josh Triplett, Uladzislau Rezki, Steven Rostedt,
Mathieu Desnoyers, Lai Jiangshan, Zqiang, Daniel Almeida,
Greg Kroah-Hartman, Igor Korotin, Lorenzo Stoakes,
Alexandre Courbot, FUJITA Tomonori, Krishna Ketan Rai,
Shankari Anand, manos, Boris Brezillon, linux-kernel,
rust-for-linux, linux-media, dri-devel, linaro-mm-sig, rcu
On Mon, Jun 1, 2026 at 3:24 PM Philipp Stanner <phasta@mailbox.org> wrote:
>
> On Mon, 2026-06-01 at 15:22 +0200, Alice Ryhl wrote:
> > On Mon, Jun 1, 2026 at 2:47 PM Philipp Stanner <phasta@mailbox.org> wrote:
> > >
> > > On Mon, 2026-06-01 at 12:39 +0000, Alice Ryhl wrote:
> > > > On Mon, Jun 01, 2026 at 02:26:17PM +0200, Philipp Stanner wrote:
> > > > > On Mon, 2026-06-01 at 10:36 +0000, Alice Ryhl wrote:
> > > > > > On Sat, May 30, 2026 at 04:35:11PM +0200, Philipp Stanner wrote:
> > > > > > > +/// A trait to enforce that all data in a [`DriverFence`] either does not need
> > > > > > > +/// drop, or lives in a [`RcuBox`].
> > > > > > > +pub trait DriverFenceAllowedData: private::Sealed {}
> > > > > > > +
> > > > > > > +mod private {
> > > > > > > + pub trait Sealed {}
> > > > > > > +}
> > > > > > > +
> > > > > > > +impl<F: Copy> DriverFenceAllowedData for F {}
> > > > > > > +impl<F: Send> DriverFenceAllowedData for RcuBox<F> {}
> > > > > > > +
> > > > > > > +impl<F: Copy> private::Sealed for F {}
> > > > > > > +impl<F: Send> private::Sealed for RcuBox<F> {}
> > > > > >
> > > > > > Why sealed? Just make the trait unsafe and require the things you
> > > > > > require from the user.
> > > > >
> > > > > This is far better. We definitely only allow the user to pass A or B,
> > > > > and only then it compiles.
> > > >
> > > > What if I have another type that I want to use here? For example, maybe
> > > > I have a struct containing a copy field and an RcuBox. Or maybe I have
> > > > an ARef<_> of some C type that uses rcu for cleanup. Then I must edit
> > > > this file to add support for it?
> > > >
> > > > > The unsafe implementation could be messed up.
> > > > >
> > > > > I thought that's what Sealed is for. Or isn't it?
> > > >
> > > > Sealed is for making 100% sure that downstream crates/drivers cannot
> > > > provide their own implementations. But I don't see why you need that.
> > > > All you require is that the value remains valid for one grace period
> > > > after cleanup begins. As long as the type satisfies that, you are happy.
> > > > An unsafe trait can require that sort of requirement from the user.
> > > >
> > > > I think what you want is expressed well by `RcuFreeSafe` from this
> > > > thread:
> > > > https://rust-for-linux.zulipchat.com/#narrow/channel/291566-Library/topic/Consolidate.20.60PollCondVarBox.60.20into.20.60Rcu.2ABox.60/near/598726724
> > > >
> > >
> > > I guess this is a question of design principles. If you demand an
> > > RcuBox, you have a guarantee that it's safe.
> > >
> > > If you demand an unsafe trait, you open the possibility for people
> > > messing up.
> > >
> > > Due to the unsafe-contract you'd have moved the responsibility for the
> > > soundness to the driver.
> > >
> > > I would not want to block your suggestion, but I am not sure whether
> > > that's really the better design idea.
> >
> > Yes, it's a design principle. You are saying that if someone needs to
> > do X but might get it wrong, we should take away the ability to do X?
> > I fundamentally disagree with that principle. Unsafe traits is the
> > tool Rust created for the exact problem you have; marking places where
> > you should be careful is the entire point of 'unsafe'.
>
> I mean, fine by me if the others don't disagree.
>
> But when then do you ever really want a Sealed trait?
Sealed traits are a very niche feature. The main use-case is
forwards-compatibility. It's not a breaking change to add a new method
to a sealed trait because no downstream crates could implement the
trait.
Alice
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/4] rust: Add dma_fence abstractions
2026-06-01 13:14 ` Philipp Stanner
@ 2026-06-01 13:30 ` Philipp Stanner
2026-06-01 13:54 ` Alice Ryhl
2026-06-01 13:44 ` Alice Ryhl
1 sibling, 1 reply; 35+ messages in thread
From: Philipp Stanner @ 2026-06-01 13:30 UTC (permalink / raw)
To: phasta, Alice Ryhl; +Cc: sashiko-reviews, linux-media, ojeda, Boris Brezillon
On Mon, 2026-06-01 at 15:14 +0200, Philipp Stanner wrote:
> +Cc Boris
>
> On Mon, 2026-06-01 at 14:55 +0200, Alice Ryhl wrote:
> > On Mon, Jun 1, 2026 at 2:34 PM Philipp Stanner <phasta@mailbox.org> wrote:
> > >
> > > On Mon, 2026-06-01 at 10:20 +0000, Alice Ryhl wrote:
> > > > On Sat, May 30, 2026 at 03:06:21PM +0000, sashiko-bot@kernel.org wrote:
> > > > > > +impl<F: Send + Sync, C: Send + Sync> DriverFence<F, C> {
> > > > > > + fn as_raw(&self) -> *mut bindings::dma_fence {
> > > > > > + // SAFETY: Valid because `self` is valid.
> > > > > > + let fence_data = unsafe { &mut *self.data.as_ptr() };
> > > > >
> > > > > [Severity: High]
> > > > > Does this create an exclusive mutable reference to actively shared memory?
> > > > >
> > > > > DriverFenceData can be accessed concurrently by other threads holding Fence
> > > > > references (for instance, when checking if the fence is signaled). Creating
> > > > > a mutable reference (&mut) in Rust asserts exclusive access and violates
> > > > > aliasing rules, which allows the compiler to make invalid optimization
> > > > > assumptions.
> > > > >
> > > > > Could this use an immutable reference &*self.data.as_ptr() instead?
> > > >
> > > > Yes, please use an immutable reference here.
> > > >
> > > > > > +
> > > > > > + fence_data.inner.inner.get()
> > > > > > + }
> > > > >
> > > > > [ ... ]
> > > > >
> > > > > > +impl<F: Send + Sync, C: Send + Sync> DriverFenceBorrow<F, C> {
> > > > > > + fn as_raw(&self) -> *mut bindings::dma_fence {
> > > > > > + // SAFETY: Valid because `self` is valid.
> > > > > > + let fence_data = unsafe { &mut *self.data.as_ptr() };
> > > > >
> > > > > [Severity: High]
> > > > > Similar to DriverFence::as_raw(), does this also incorrectly create a
> > > > > mutable reference to shared data?
> > > >
> > > > Here as well.
> > >
> > > `data` is not shared. By design there is only ever one DriverFence, and
> > > the driver's data (`data.data`) is `Sync`.
> > >
> > > But I guess an immutable one should do the trick, too.
> >
> > There's only one DriverFence, but I can perform shared access to that
> > one DriverFence from two threads in parallel. You made the type Sync,
> > and this is what you are allowing when you do so.
>
> Nope, DriverFence is just Send, not Sync.
>
> data.data is Sync, but `data` in the code above is not the actual user
> data, but the memory backing it up.
>
> >
> > > > > > + bindings::dma_fence_signal_locked(fence);
> > > > > > + }
> > > > > > + bindings::dma_fence_unlock_irqrestore(fence, flag_ptr);
> > > > > > + }
> > > > > > +
> > > > > > + // SAFETY: `self.data` is owned by the DriverFence, but could be accessed
> > > > > > + // through some dma_fence callbacks right now. Access is being revoked
> > > > > > + // above by signalling the fence. The DriverFenceAllowedData trait
> > > > > > + // ensures that the data either does not need drop, or if it does it
> > > > > > + // lives in a RcuBox which will delay dropping by one grace period, hence
> > > > > > + // ensuring that all readers have disappeared.
> > > > > > + unsafe { drop_in_place(self.data.as_ptr()) };
> > > > >
> > > > > [Severity: Critical]
> > > > > Could this eagerly drop FenceCtx and user data before the C dma_fence
> > > > > refcount reaches 0?
> > > > >
> > > > > The C dma_fence object can remain alive as long as consumers (like sync_file
> > > > > or the GPU scheduler) hold references to it. If the backend later invokes
> > > > > callbacks like ops->get_driver_name(), FenceCtx::get_driver_name() would
> > > > > attempt to read fctx.driver_name from the already-dropped FenceCtx:
> > > > >
> > > > > FenceCtx::get_driver_name() {
> > > > > ...
> > > > > fctx.driver_name.as_char_ptr()
> > > > > }
> > > > >
> > > > > Can this result in a use-after-free?
> > > >
> > > > Hmm, I don't know about what sahisko said, but I don't think this is the
> > > > right way to do rcu freeing. I think the type's destructor should be
> > > > reserved for cases where the value becomes immediately unusable.
> > >
> > > We could guard the strings with RcuBox, but we could not then guard the
> > > FenceCtx code against code UAF if we don't have the rcu_barrier().
> > >
> > > Or could we?
> > >
> > > If a rust module unloads, module::remove() should contain an
> > > rcu_barrier() (right??). Would that be enough to guard against the
> > > FenceCtx code being unloaded?
> > >
> > > >
> > > > For example, let's say I'm using RcuBox<_> here. Yes, the data you get
> > > > from dereferencing the RcuBox will stay alive for a grace period, but
> > > > IMO once you run the destructor of the box itself, the *pointer* becomes
> > > > immediately unusable.
> > >
> > > I don't know why you're stressing the pointer?
> > >
> > > The trick above is simply that drop / dealloc *and* code unloading is
> > > delayed by a grace period.
> >
> > Sorry let me try to rephrase. I'm not worried about the stuff behind
> > the pointer. After all, you're using RcuBox to protect that stuff.
> > What I'm worried about is the pointer itself. You invoked
> > drop_in_place() on the pointer to the fence context,
> >
>
> on the pointer to DriverFenceData, which contains a refcount to the
> FenceCtx, which might then want to drop.
>
> > so even though
> > the fence context may be valid for another grace period, the *pointer*
> > to the fence context is not. The pointer could have been zeroed by the
> > destructor.
>
> That particular pointer to the DriverFenceData could have been zeroed.
> But potential other accessors have already crafted themselves a new
> pointer to the, by the power of RCU, still valid data.
>
correction:
it is refcounting that ensures the memory is still valid.
dma_fence_put() in DriverFence::drop() could earliest free the memory.
P.
> That new pointer
> is container-of-ed from struct dma_fence *f.
>
> >
> > Actually maybe I am worried about the strings too. Let's say that one
> > thread does:
> >
> > rcu_read_lock()
> > get_driver_name()
> > // .. read the driver name
> > rcu_read_unlock()
> >
> > and another thread signals the fence. If there are no queued
> > call_rcu() callbacks right now, then the rcu_barrier() could be a
> > no-op and the rcu reader will UAF on the driver name. It looks like
> > you need synchronize_rcu() rather than rcu_barrier() here?
>
> It would seem to me that you are right.
>
> Boris, do you have an opinion on that?
>
>
> P.
>
> >
> > Alice
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/4] rust: rcu: add RcuBox type
2026-06-01 7:56 ` Philipp Stanner
@ 2026-06-01 13:41 ` Boqun Feng
0 siblings, 0 replies; 35+ messages in thread
From: Boqun Feng @ 2026-06-01 13:41 UTC (permalink / raw)
To: phasta
Cc: Miguel Ojeda, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Danilo Krummrich,
Sumit Semwal, Christian König, Paul E. McKenney,
Frederic Weisbecker, Neeraj Upadhyay, Joel Fernandes,
Josh Triplett, Uladzislau Rezki, Steven Rostedt,
Mathieu Desnoyers, Lai Jiangshan, Zqiang, Daniel Almeida,
Greg Kroah-Hartman, Igor Korotin, Lorenzo Stoakes,
Alexandre Courbot, FUJITA Tomonori, Krishna Ketan Rai,
Shankari Anand, manos, Boris Brezillon, linux-kernel,
rust-for-linux, linux-media, dri-devel, linaro-mm-sig, rcu
On Mon, Jun 01, 2026 at 09:56:23AM +0200, Philipp Stanner wrote:
> On Sat, 2026-05-30 at 08:08 -0700, Boqun Feng wrote:
> > On Sat, May 30, 2026 at 04:35:10PM +0200, Philipp Stanner wrote:
> > > From: Alice Ryhl <aliceryhl@google.com>
> > >
> > > This adds an RcuBox container, which is like KBox except that the value
> > > is freed with kfree_rcu.
> > >
> > > To allow containers to rely on the rcu properties of RcuBox, an
> > > extension of ForeignOwnable is added.
> > >
> > > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> > > ---
> >
> > I have the following on top of Alice's patch. @Alice, @Danilo, thoughts?
> >
> > Then we can have:
> >
> > type RcuKBox<T> = RcuBox<T, Kmalloc>;
> > type RcuVBox<T> = RcuBox<T, Vmalloc>;
>
> No objections by me.
>
> I just think we have to decide how the treat the namespaces, though.
> Probably Alice wrote it like that so that it's very apparent that this
> is not a normal box. It still breaks the naming convention in my
> opinion.
>
> rcu::Box vs rcu::RcuBox
>
> With all other subsystems, naming like that seems not allowed.
>
> dma::Fence vs dma::DmaFence
>
>
> I probably would allow the user to decide whether he wants to just use
> it as `rcu::Box` in all his code.
>
> But no hard feelings.
>
For this I think that rcu::RcuBox is a bit different than dma::Fence,
because Box has its widely-accepted meaning through all Rust code,
while `Fence` doesn't. Hence my current thought is rcu::RcuBox and
dma::Fence. My personal preference is using namespace as much as we
could until there might be some misleading.
>
>
> >
> > and Philipp can use the `RcuKBox` in this patchset. We also need to impl
> > InPlaceInit for RcuBox, but that can be added later.
>
> So shall we merge my series with Alice's patch, and later we add your
> patch and other features, or would you prefer to have the additional
> boxes from your patch from the get-go?
>
I would like to have it from the get-go mainly because of RcuBox vs
RcuKBox naming. Thank you!
Regards,
Boqun
>
> P.
>
> >
> > Regards,
> > Boqun
> >
> > ------------->8
> > Subject: [PATCH] rust: rcu: Make RcuBox generic over Allocator
> >
> > To support RCU-protected vmalloc allocation, we need to make `RcuBox`
> > generic over `Allocator`. Currently this works since all `Allocator`s
> > are either kmalloc() or vmalloc(), and kvfree_call_rcu() works with both
> > allocations.
> >
> > While we are at it, add some basic test cases.
> >
> > Signed-off-by: Boqun Feng <boqun@kernel.org>
> > ---
> > rust/kernel/sync/rcu/rcu_box.rs | 96 +++++++++++++++++++++++----------
> > 1 file changed, 67 insertions(+), 29 deletions(-)
> >
> > diff --git a/rust/kernel/sync/rcu/rcu_box.rs b/rust/kernel/sync/rcu/rcu_box.rs
> > index 2508fdb609ec..5c344d82c0d9 100644
> > --- a/rust/kernel/sync/rcu/rcu_box.rs
> > +++ b/rust/kernel/sync/rcu/rcu_box.rs
> > @@ -4,47 +4,59 @@
> >
> > //! Provides the `RcuBox` type for Rust allocations that live for a grace period.
> >
> > -use core::{ops::Deref, ptr::NonNull};
> > +use core::{
> > + marker::PhantomData,
> > + ops::Deref,
> > + ptr::NonNull, //
> > +};
> >
> > use kernel::{
> > - alloc::{self, AllocError},
> > + alloc::{
> > + self,
> > + AllocError,
> > + Allocator, //
> > + },
> > bindings,
> > ffi::c_void,
> > prelude::*,
> > - sync::rcu::{ForeignOwnableRcu, Guard},
> > types::ForeignOwnable,
> > };
> >
> > +use super::{
> > + ForeignOwnableRcu,
> > + Guard, //
> > +};
> > +
> > /// A box that is freed with rcu.
> > ///
> > /// The value must be `Send`, as rcu may drop it on another thread.
> > ///
> > /// # Invariants
> > ///
> > -/// * The pointer is valid and references a pinned `RcuBoxInner<T>` allocated with `kmalloc`.
> > +/// * The pointer is valid and references a pinned `RcuBoxInner<T>` allocated with `A`.
> > /// * This `RcuBox` holds exclusive permissions to rcu free the allocation.
> > -pub struct RcuBox<T: Send>(NonNull<RcuBoxInner<T>>);
> > +pub struct RcuBox<T: Send, A: Allocator>(NonNull<RcuBoxInner<T>>, PhantomData<A>);
> >
> > struct RcuBoxInner<T> {
> > value: T,
> > rcu_head: bindings::callback_head,
> > }
> >
> > -// Note that `T: Sync` is required since when moving an `RcuBox<T>`, the previous owner may still
> > -// access `&T` for one grace period.
> > +// Note that `T: Sync` is required since when moving an `RcuBox<T, A>`, the previous owner may
> > +// still access `&T` for one grace period.
> > //
> > -// SAFETY: Ownership of the `RcuBox<T>` allows for `&T` and dropping the `T`, so `T: Send + Sync`
> > -// implies `RcuBox<T>: Send`.
> > -unsafe impl<T: Send + Sync> Send for RcuBox<T> {}
> > +// SAFETY: Ownership of the `RcuBox<T, A>` allows for `&T` and dropping the `T`, so `T: Send +
> > +// Sync` implies `RcuBox<T, A>: Send`.
> > +unsafe impl<T: Send + Sync, A: Allocator> Send for RcuBox<T, A> {}
> >
> > -// SAFETY: `&RcuBox<T>` allows for no operations other than those permitted by `&T`, so `T: Sync`
> > -// implies `RcuBox<T>: Sync`.
> > -unsafe impl<T: Send + Sync> Sync for RcuBox<T> {}
> > +// SAFETY: `&RcuBox<T, A>` allows for no operations other than those permitted by `&T`, so `T:
> > +// Sync` implies `RcuBox<T, A>: Sync`.
> > +unsafe impl<T: Send + Sync, A: Allocator> Sync for RcuBox<T, A> {}
> >
> > -impl<T: Send> RcuBox<T> {
> > +impl<T: Send, A: Allocator> RcuBox<T, A> {
> > /// Create a new `RcuBox`.
> > pub fn new(x: T, flags: alloc::Flags) -> Result<Self, AllocError> {
> > - let b = KBox::new(
> > + let b = Box::<_, A>::new(
> > RcuBoxInner {
> > value: x,
> > rcu_head: Default::default(),
> > @@ -53,9 +65,9 @@ pub fn new(x: T, flags: alloc::Flags) -> Result<Self, AllocError> {
> > )?;
> >
> > // INVARIANT:
> > - // * The pointer contains a valid `RcuBoxInner` allocated with `kmalloc`.
> > + // * The pointer contains a valid `RcuBoxInner` allocated with `A`.
> > // * We just allocated it, so we own free permissions.
> > - Ok(RcuBox(NonNull::from(KBox::leak(b))))
> > + Ok(RcuBox(NonNull::from(Box::leak(b)), PhantomData))
> > }
> >
> > /// Access the value for a grace period.
> > @@ -66,7 +78,7 @@ pub fn with_rcu<'rcu>(&self, _read_guard: &'rcu Guard) -> &'rcu T {
> > }
> > }
> >
> > -impl<T: Send> Deref for RcuBox<T> {
> > +impl<T: Send, A: Allocator> Deref for RcuBox<T, A> {
> > type Target = T;
> > fn deref(&self) -> &T {
> > // SAFETY: While the `RcuBox<T>` exists, the value remains valid.
> > @@ -75,10 +87,10 @@ fn deref(&self) -> &T {
> > }
> >
> > // SAFETY:
> > -// * The `RcuBoxInner<T>` was allocated with `kmalloc`.
> > +// * The `RcuBoxInner<T>` was allocated with `A`.
> > // * `NonNull::as_ptr` returns a non-null pointer.
> > -unsafe impl<T: Send + 'static> ForeignOwnable for RcuBox<T> {
> > - const FOREIGN_ALIGN: usize = <KBox<RcuBoxInner<T>> as ForeignOwnable>::FOREIGN_ALIGN;
> > +unsafe impl<T: Send + 'static, A: Allocator> ForeignOwnable for RcuBox<T, A> {
> > + const FOREIGN_ALIGN: usize = <Box<RcuBoxInner<T>, A> as ForeignOwnable>::FOREIGN_ALIGN;
> >
> > type Borrowed<'a> = &'a T;
> > type BorrowedMut<'a> = &'a T;
> > @@ -88,9 +100,9 @@ fn into_foreign(self) -> *mut c_void {
> > }
> >
> > unsafe fn from_foreign(ptr: *mut c_void) -> Self {
> > - // INVARIANT: Pointer returned by `into_foreign` carries same invariants as `RcuBox<T>`.
> > + // INVARIANT: Pointer returned by `into_foreign, A` carries same invariants as `RcuBox<T>`.
> > // SAFETY: `into_foreign` never returns a null pointer.
> > - Self(unsafe { NonNull::new_unchecked(ptr.cast()) })
> > + Self(unsafe { NonNull::new_unchecked(ptr.cast()) }, PhantomData)
> > }
> >
> > unsafe fn borrow<'a>(ptr: *mut c_void) -> &'a T {
> > @@ -104,7 +116,7 @@ unsafe fn borrow_mut<'a>(ptr: *mut c_void) -> &'a T {
> > }
> > }
> >
> > -impl<T: Send + 'static> ForeignOwnableRcu for RcuBox<T> {
> > +impl<T: Send + 'static, A: Allocator> ForeignOwnableRcu for RcuBox<T, A> {
> > type RcuBorrowed<'a> = &'a T;
> >
> > unsafe fn rcu_borrow<'a>(ptr: *mut c_void) -> &'a T {
> > @@ -114,7 +126,7 @@ unsafe fn rcu_borrow<'a>(ptr: *mut c_void) -> &'a T {
> > }
> > }
> >
> > -impl<T: Send> Drop for RcuBox<T> {
> > +impl<T: Send, A: Allocator> Drop for RcuBox<T, A> {
> > fn drop(&mut self) {
> > // SAFETY: The `rcu_head` field is in-bounds of a valid allocation.
> > let rcu_head = unsafe { &raw mut (*self.0.as_ptr()).rcu_head };
> > @@ -122,9 +134,11 @@ fn drop(&mut self) {
> > // SAFETY: `rcu_head` is the `rcu_head` field of `RcuBoxInner<T>`. All users will be
> > // gone in an rcu grace period. This is the destructor, so we may pass ownership of the
> > // allocation.
> > - unsafe { bindings::call_rcu(rcu_head, Some(drop_rcu_box::<T>)) };
> > + unsafe { bindings::call_rcu(rcu_head, Some(drop_rcu_box::<T, A>)) };
> > } else {
> > // SAFETY: All users will be gone in an rcu grace period.
> > + // TODO: We are luckily since `kvfree_call_rcu()` works on both kmalloc and vmalloc,
> > + // maybe a new `Allocator` method is needed.
> > unsafe { bindings::kvfree_call_rcu(rcu_head, self.0.as_ptr().cast()) };
> > }
> > }
> > @@ -135,11 +149,35 @@ fn drop(&mut self) {
> > /// # Safety
> > ///
> > /// `head` references the `rcu_head` field of an `RcuBoxInner<T>` that has no references to it.
> > -/// Ownership of the `KBox<RcuBoxInner<T>>` must be passed.
> > -unsafe extern "C" fn drop_rcu_box<T>(head: *mut bindings::callback_head) {
> > +/// Ownership of the `Box<RcuBoxInner<T>, A>` must be passed.
> > +unsafe extern "C" fn drop_rcu_box<T, A: Allocator>(head: *mut bindings::callback_head) {
> > // SAFETY: Caller provides a pointer to the `rcu_head` field of a `RcuBoxInner<T>`.
> > let box_inner = unsafe { crate::container_of!(head, RcuBoxInner<T>, rcu_head) };
> >
> > // SAFETY: Caller ensures exclusive access and passed ownership.
> > - drop(unsafe { KBox::from_raw(box_inner) });
> > + drop(unsafe { Box::<_, A>::from_raw(box_inner) });
> > +}
> > +
> > +#[kunit_tests(rust_rcu_box)]
> > +mod tests {
> > + use super::*;
> > +
> > + #[test]
> > + fn rcu_box_basic() -> Result {
> > + let rb = RcuBox::<_, alloc::allocator::Kmalloc>::new(42i32, alloc::flags::GFP_KERNEL)?;
> > +
> > + assert_eq!(*rb, 42);
> > + assert_eq!(*rb.with_rcu(&Guard::new()), 42);
> > +
> > + drop(rb);
> > +
> > + let rb = RcuBox::<_, alloc::allocator::Vmalloc>::new(42i32, alloc::flags::GFP_KERNEL)?;
> > +
> > + assert_eq!(*rb, 42);
> > + assert_eq!(*rb.with_rcu(&Guard::new()), 42);
> > +
> > + drop(rb);
> > +
> > + Ok(())
> > + }
> > }
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/4] rust: Add dma_fence abstractions
2026-06-01 13:14 ` Philipp Stanner
2026-06-01 13:30 ` Philipp Stanner
@ 2026-06-01 13:44 ` Alice Ryhl
1 sibling, 0 replies; 35+ messages in thread
From: Alice Ryhl @ 2026-06-01 13:44 UTC (permalink / raw)
To: phasta; +Cc: sashiko-reviews, linux-media, ojeda, Boris Brezillon
On Mon, Jun 01, 2026 at 03:14:05PM +0200, Philipp Stanner wrote:
> +Cc Boris
>
> On Mon, 2026-06-01 at 14:55 +0200, Alice Ryhl wrote:
> > On Mon, Jun 1, 2026 at 2:34 PM Philipp Stanner <phasta@mailbox.org> wrote:
> > >
> > > On Mon, 2026-06-01 at 10:20 +0000, Alice Ryhl wrote:
> > > > On Sat, May 30, 2026 at 03:06:21PM +0000, sashiko-bot@kernel.org wrote:
> > > > > > +impl<F: Send + Sync, C: Send + Sync> DriverFence<F, C> {
> > > > > > + fn as_raw(&self) -> *mut bindings::dma_fence {
> > > > > > + // SAFETY: Valid because `self` is valid.
> > > > > > + let fence_data = unsafe { &mut *self.data.as_ptr() };
> > > > >
> > > > > [Severity: High]
> > > > > Does this create an exclusive mutable reference to actively shared memory?
> > > > >
> > > > > DriverFenceData can be accessed concurrently by other threads holding Fence
> > > > > references (for instance, when checking if the fence is signaled). Creating
> > > > > a mutable reference (&mut) in Rust asserts exclusive access and violates
> > > > > aliasing rules, which allows the compiler to make invalid optimization
> > > > > assumptions.
> > > > >
> > > > > Could this use an immutable reference &*self.data.as_ptr() instead?
> > > >
> > > > Yes, please use an immutable reference here.
> > > >
> > > > > > +
> > > > > > + fence_data.inner.inner.get()
> > > > > > + }
> > > > >
> > > > > [ ... ]
> > > > >
> > > > > > +impl<F: Send + Sync, C: Send + Sync> DriverFenceBorrow<F, C> {
> > > > > > + fn as_raw(&self) -> *mut bindings::dma_fence {
> > > > > > + // SAFETY: Valid because `self` is valid.
> > > > > > + let fence_data = unsafe { &mut *self.data.as_ptr() };
> > > > >
> > > > > [Severity: High]
> > > > > Similar to DriverFence::as_raw(), does this also incorrectly create a
> > > > > mutable reference to shared data?
> > > >
> > > > Here as well.
> > >
> > > `data` is not shared. By design there is only ever one DriverFence, and
> > > the driver's data (`data.data`) is `Sync`.
> > >
> > > But I guess an immutable one should do the trick, too.
> >
> > There's only one DriverFence, but I can perform shared access to that
> > one DriverFence from two threads in parallel. You made the type Sync,
> > and this is what you are allowing when you do so.
>
> Nope, DriverFence is just Send, not Sync.
>
> data.data is Sync, but `data` in the code above is not the actual user
> data, but the memory backing it up.
Ok, well, it probably should be Sync. I don't see any &self methods that
can't be called from multiple threads in parallel.
> > > > > > + bindings::dma_fence_signal_locked(fence);
> > > > > > + }
> > > > > > + bindings::dma_fence_unlock_irqrestore(fence, flag_ptr);
> > > > > > + }
> > > > > > +
> > > > > > + // SAFETY: `self.data` is owned by the DriverFence, but could be accessed
> > > > > > + // through some dma_fence callbacks right now. Access is being revoked
> > > > > > + // above by signalling the fence. The DriverFenceAllowedData trait
> > > > > > + // ensures that the data either does not need drop, or if it does it
> > > > > > + // lives in a RcuBox which will delay dropping by one grace period, hence
> > > > > > + // ensuring that all readers have disappeared.
> > > > > > + unsafe { drop_in_place(self.data.as_ptr()) };
> > > > >
> > > > > [Severity: Critical]
> > > > > Could this eagerly drop FenceCtx and user data before the C dma_fence
> > > > > refcount reaches 0?
> > > > >
> > > > > The C dma_fence object can remain alive as long as consumers (like sync_file
> > > > > or the GPU scheduler) hold references to it. If the backend later invokes
> > > > > callbacks like ops->get_driver_name(), FenceCtx::get_driver_name() would
> > > > > attempt to read fctx.driver_name from the already-dropped FenceCtx:
> > > > >
> > > > > FenceCtx::get_driver_name() {
> > > > > ...
> > > > > fctx.driver_name.as_char_ptr()
> > > > > }
> > > > >
> > > > > Can this result in a use-after-free?
> > > >
> > > > Hmm, I don't know about what sahisko said, but I don't think this is the
> > > > right way to do rcu freeing. I think the type's destructor should be
> > > > reserved for cases where the value becomes immediately unusable.
> > >
> > > We could guard the strings with RcuBox, but we could not then guard the
> > > FenceCtx code against code UAF if we don't have the rcu_barrier().
> > >
> > > Or could we?
> > >
> > > If a rust module unloads, module::remove() should contain an
> > > rcu_barrier() (right??). Would that be enough to guard against the
> > > FenceCtx code being unloaded?
> > >
> > > >
> > > > For example, let's say I'm using RcuBox<_> here. Yes, the data you get
> > > > from dereferencing the RcuBox will stay alive for a grace period, but
> > > > IMO once you run the destructor of the box itself, the *pointer* becomes
> > > > immediately unusable.
> > >
> > > I don't know why you're stressing the pointer?
> > >
> > > The trick above is simply that drop / dealloc *and* code unloading is
> > > delayed by a grace period.
> >
> > Sorry let me try to rephrase. I'm not worried about the stuff behind
> > the pointer. After all, you're using RcuBox to protect that stuff.
> > What I'm worried about is the pointer itself. You invoked
> > drop_in_place() on the pointer to the fence context,
> >
>
> on the pointer to DriverFenceData, which contains a refcount to the
> FenceCtx, which might then want to drop.
Let me clarify.
You invoked drop_in_place on DriverFenceData which is effectively this
method:
fn drop_in_place::<DriverFenceData<F,C>>(self: *mut DriverFenceData) {
drop_in_place::<Fence>(&raw mut (*self).fence);
drop_in_place::<Arc<FenceCtx<F,C>>>(&raw mut (*self).fctx);
drop_in_place::<F>(&raw mut (*self).data);
}
So yes you did indirectly invoke `drop_in_place` on Arc<FenceCtx<F,C>>.
> > so even though
> > the fence context may be valid for another grace period, the *pointer*
> > to the fence context is not. The pointer could have been zeroed by the
> > destructor.
>
> That particular pointer to the DriverFenceData could have been zeroed.
> But potential other accessors have already crafted themselves a new
> pointer to the, by the power of RCU, still valid data. That new pointer
> is container-of-ed from struct dma_fence *f.
I'm not talking about the pointer to DriverFenceData, I'm talking about
the pointer to the FenceCtx, or the pointer to the data (if F is
RcuBox).
The Arc type is not a type that opts-out of &mut == exclusive, so the
second drop_in_place() above is assumed exclusive access to the
Arc<FenceCtx<F,C>> field. If another thread obtains a pointer to the
FenceCtx via reading the fctx field of the DriverFence in parallel with
this, then that's not allowed because the drop_in_place() call has
exclusive access to that field.
This is why Boqun's RcuFreeSafe has a drop_in_place_before_gp() method
that can be used instead of drop_in_place() for this kind of use-case.
You must leave the fctx and data fields in a state where they contain
data that's safe to use for at least one grace period, and drop_in_place()
fundamentally does not leave the value in a usable state.
Alice
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 3/4] rust: Add dma_fence abstractions
2026-06-01 13:30 ` Philipp Stanner
@ 2026-06-01 13:54 ` Alice Ryhl
0 siblings, 0 replies; 35+ messages in thread
From: Alice Ryhl @ 2026-06-01 13:54 UTC (permalink / raw)
To: phasta; +Cc: sashiko-reviews, linux-media, ojeda, Boris Brezillon
On Mon, Jun 01, 2026 at 03:30:38PM +0200, Philipp Stanner wrote:
> On Mon, 2026-06-01 at 15:14 +0200, Philipp Stanner wrote:
> > +Cc Boris
> >
> > On Mon, 2026-06-01 at 14:55 +0200, Alice Ryhl wrote:
> > > On Mon, Jun 1, 2026 at 2:34 PM Philipp Stanner <phasta@mailbox.org> wrote:
> > > >
> > > > On Mon, 2026-06-01 at 10:20 +0000, Alice Ryhl wrote:
> > > > > For example, let's say I'm using RcuBox<_> here. Yes, the data you get
> > > > > from dereferencing the RcuBox will stay alive for a grace period, but
> > > > > IMO once you run the destructor of the box itself, the *pointer* becomes
> > > > > immediately unusable.
> > > >
> > > > I don't know why you're stressing the pointer?
> > > >
> > > > The trick above is simply that drop / dealloc *and* code unloading is
> > > > delayed by a grace period.
> > >
> > > Sorry let me try to rephrase. I'm not worried about the stuff behind
> > > the pointer. After all, you're using RcuBox to protect that stuff.
> > > What I'm worried about is the pointer itself. You invoked
> > > drop_in_place() on the pointer to the fence context,
> > >
> >
> > on the pointer to DriverFenceData, which contains a refcount to the
> > FenceCtx, which might then want to drop.
> >
> > > so even though
> > > the fence context may be valid for another grace period, the *pointer*
> > > to the fence context is not. The pointer could have been zeroed by the
> > > destructor.
> >
> > That particular pointer to the DriverFenceData could have been zeroed.
> > But potential other accessors have already crafted themselves a new
> > pointer to the, by the power of RCU, still valid data.
> >
>
> correction:
> it is refcounting that ensures the memory is still valid.
>
> dma_fence_put() in DriverFence::drop() could earliest free the memory.
Hrm, actually I realized that this is not quite Boqun's RcuFreeSafe.
That trait would require the fence allocation itself to remain valid for
one grace period, but you don't require that.
Alice
^ permalink raw reply [flat|nested] 35+ messages in thread
end of thread, other threads:[~2026-06-01 13:54 UTC | newest]
Thread overview: 35+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-30 14:35 [PATCH 0/4] rust / dma_buf: Add abstractions for dma_fence Philipp Stanner
2026-05-30 14:35 ` [PATCH 1/4] rust: types: implement ForeignOwnable for ARef<T> Philipp Stanner
2026-05-30 14:45 ` sashiko-bot
2026-06-01 9:46 ` Alice Ryhl
2026-05-30 14:35 ` [PATCH 2/4] rust: rcu: add RcuBox type Philipp Stanner
2026-05-30 14:54 ` sashiko-bot
2026-05-30 15:08 ` Boqun Feng
2026-05-30 15:27 ` Danilo Krummrich
2026-06-01 7:56 ` Philipp Stanner
2026-06-01 13:41 ` Boqun Feng
2026-05-30 14:35 ` [PATCH 3/4] rust: Add dma_fence abstractions Philipp Stanner
2026-05-30 15:06 ` sashiko-bot
2026-06-01 10:20 ` Alice Ryhl
2026-06-01 12:34 ` Philipp Stanner
2026-06-01 12:55 ` Alice Ryhl
2026-06-01 13:14 ` Philipp Stanner
2026-06-01 13:30 ` Philipp Stanner
2026-06-01 13:54 ` Alice Ryhl
2026-06-01 13:44 ` Alice Ryhl
2026-05-30 15:16 ` Danilo Krummrich
2026-06-01 8:46 ` Philipp Stanner
2026-06-01 10:13 ` Danilo Krummrich
2026-06-01 10:36 ` Alice Ryhl
2026-06-01 10:59 ` Boris Brezillon
2026-06-01 11:17 ` Philipp Stanner
2026-06-01 12:35 ` Boris Brezillon
2026-06-01 12:26 ` Philipp Stanner
2026-06-01 12:39 ` Alice Ryhl
2026-06-01 12:47 ` Philipp Stanner
2026-06-01 13:22 ` Alice Ryhl
2026-06-01 13:23 ` Philipp Stanner
2026-06-01 13:27 ` Alice Ryhl
2026-06-01 12:37 ` Boris Brezillon
2026-05-30 14:35 ` [PATCH 4/4] MAINTAINERS: Add entry for Rust dma-buf Philipp Stanner
2026-05-30 15:20 ` Danilo Krummrich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox