Skip to contentSkip to conversation

rust / dma_buf: Add abstractions for dma_fence

PATCH Superseded Philipp Stanner 7 participants 17 messages Aug 5 – Aug 11

Subsystems (6)

v9 of 11 Superseded Latest is v11 →

lore
Share & tools
lore

Philipp Stanner

author maintainer
#0 lore raw

v9 cover rust / dma_buf: Add abstractions for dma_fence

(Timur's patch is just included so that the series builds)

Changes since v8:
- Improve function naming. (Daniel)
- Panic in case of forgotten fences. (Alice)
(reason being that this cannot happen with the intended JobQueue design, it would be a fatal bug anyways and we can easily fix this up)
- Improve spelling and grammar. (Daniel)
- Add Daniel's Tested-by
- Improve Copyright notice to SPDX. (Daniel)

Changes since v7:
- Remove the forgotten-fences-tracking from FenceContext. Merely warn in this unlikely case, using an atomic integer.
- Be more verbose in warnings about what exactly the problem is.

Changes since v6:
- Add Timur's error code patch (just so that the series builds. Merge however you prefer). (Timur)
- Replace XArray with list, remove XArray patch.
- Detail the issue with core::mem::forget() in the commit message.
- Some code formatting etc.

Changes since v5:
- Because Rust can "forget" DriverFences, it could bypass the lifetime ensuring that DriverFences don't outlive a FenceContext. Mitigate this by having the FenceContext keep track of unsignaled fences. (Danilo)
- To support the point above, add support for memory reservations to XArray.
- To support the point above, have FenceContext::alloc_fence() increment the sequence number. This is necessary for the xarray index. It might become critical with fence seqno holes.. please review.
- Remove acronyms in declarations. (Daniel)
- Allow for providing initial seqno in FenceContext::new(). (Daniel)
- Flesh out documentation about DriverFence <-> Fence. (Daniel)
- Flesh out documentation about weirdness in C backend. (Daniel)
- Add new, purely internal locking helper / guard for dma_fence.
- Use the kernel atomics (Gary).
- Curse angrily at my computer.

Changes since v4:
- Fix an uninitialized memory bug for FenceCbRegistration with ManuallyDrop.
- Return FenceCtx as impl PinInit
- Make FenceCtx return an impl PinInit<T, Error> (Danilo)
- Reformat some comments

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

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

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

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

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

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

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

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

---

Old cover letter for RFC:

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

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

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

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

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

Many regards,
Philipp

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

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

Philipp Stanner (3):
rust: sync: Add abstraction for rcu_barrier()
rust: Add dma_fence abstractions
MAINTAINERS: Add entry for Rust dma-buf

Timur Tabi (1):
rust: error: add remaining error codes

 MAINTAINERS                      |    5 +
 rust/bindings/bindings_helper.h  |    1 +
 rust/helpers/dma_fence.c         |   48 ++
 rust/helpers/helpers.c           |    1 +
 rust/kernel/dma_buf/dma_fence.rs | 1002 ++++++++++++++++++++++++++++++
 rust/kernel/dma_buf/mod.rs       |   14 +
 rust/kernel/error.rs             |  105 ++++
 rust/kernel/lib.rs               |    1 +
 rust/kernel/sync/aref.rs         |   40 ++
 rust/kernel/sync/rcu.rs          |   20 +
 10 files changed, 1237 insertions(+)
 create mode 100644 rust/helpers/dma_fence.c
 create mode 100644 rust/kernel/dma_buf/dma_fence.rs
 create mode 100644 rust/kernel/dma_buf/mod.rs

base-commit: 44e7e7f7cffb10a93bb88e7cb59b7b8b3e2deb1c

Signature · 1 line
2.55.0
author maintainer
#1 lore raw

PATCH v9 1/5 rust: error: add remaining error codes

From: Timur Tabi <[email protected]>

Add all of the remaining error codes from include/uapi/asm-generic/errno.h.

Previous updates to error.rs have been piecemeal -- adding single error codes as needed. Instead, we can avoid future problems by adding all the remaining error code in one swoop.

EDEADLOCK and EWOULDBLOCK are intentionally left out: they are just deprecated compatibility aliases of EDEADLK and EAGAIN, kept around for non-Linux/POSIX code, and have no use in new kernel code.

---
 rust/kernel/error.rs | 105 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 105 insertions(+)
rust/kernel/error.rs +105 -0
index a56ba6309594..f3096d7c73ac 100644
@@ -30,6 +30,8 @@ macro_rules! declare_err {
         };
     }
 
+    // From include/uapi/asm-generic/errno-base.h
+
     declare_err!(EPERM, "Operation not permitted.");
     declare_err!(ENOENT, "No such file or directory.");
     declare_err!(ESRCH, "No such process.");
@@ -64,9 +66,112 @@ macro_rules! declare_err {
     declare_err!(EPIPE, "Broken pipe.");
     declare_err!(EDOM, "Math argument out of domain of func.");
     declare_err!(ERANGE, "Math result not representable.");
+
+    // From include/uapi/asm-generic/errno.h
+
+    declare_err!(EDEADLK, "Resource deadlock would occur.");
+    declare_err!(ENAMETOOLONG, "File name too long.");
+    declare_err!(ENOLCK, "No record locks available.");
+    declare_err!(ENOSYS, "Invalid system call number.");
+    declare_err!(ENOTEMPTY, "Directory not empty.");
+    declare_err!(ELOOP, "Too many symbolic links encountered.");
+    declare_err!(ENOMSG, "No message of desired type.");
+    declare_err!(EIDRM, "Identifier removed.");
+    declare_err!(ECHRNG, "Channel number out of range.");
+    declare_err!(EL2NSYNC, "Level 2 not synchronized.");
+    declare_err!(EL3HLT, "Level 3 halted.");
+    declare_err!(EL3RST, "Level 3 reset.");
+    declare_err!(ELNRNG, "Link number out of range.");
+    declare_err!(EUNATCH, "Protocol driver not attached.");
+    declare_err!(ENOCSI, "No CSI structure available.");
+    declare_err!(EL2HLT, "Level 2 halted.");
+    declare_err!(EBADE, "Invalid exchange.");
+    declare_err!(EBADR, "Invalid request descriptor.");
+    declare_err!(EXFULL, "Exchange full.");
+    declare_err!(ENOANO, "No anode.");
+    declare_err!(EBADRQC, "Invalid request code.");
+    declare_err!(EBADSLT, "Invalid slot.");
+    declare_err!(EBFONT, "Bad font file format.");
+    declare_err!(ENOSTR, "Device not a stream.");
+    declare_err!(ENODATA, "No data available.");
+    declare_err!(ETIME, "Timer expired.");
+    declare_err!(ENOSR, "Out of streams resources.");
+    declare_err!(ENONET, "Machine is not on the network.");
+    declare_err!(ENOPKG, "Package not installed.");
+    declare_err!(EREMOTE, "Object is remote.");
+    declare_err!(ENOLINK, "Link has been severed.");
+    declare_err!(EADV, "Advertise error.");
+    declare_err!(ESRMNT, "Srmount error.");
+    declare_err!(ECOMM, "Communication error on send.");
+    declare_err!(EPROTO, "Protocol error.");
+    declare_err!(EMULTIHOP, "Multihop attempted.");
+    declare_err!(EDOTDOT, "RFS specific error.");
+    declare_err!(EBADMSG, "Not a data message.");
+    declare_err!(EFSBADCRC, "Bad CRC detected.");
     declare_err!(EOVERFLOW, "Value too large for defined data type.");
+    declare_err!(ENOTUNIQ, "Name not unique on network.");
+    declare_err!(EBADFD, "File descriptor in bad state.");
+    declare_err!(EREMCHG, "Remote address changed.");
+    declare_err!(ELIBACC, "Can not access a needed shared library.");
+    declare_err!(ELIBBAD, "Accessing a corrupted shared library.");
+    declare_err!(ELIBSCN, ".lib section in a.out corrupted.");
+    declare_err!(ELIBMAX, "Attempting to link in too many shared libraries.");
+    declare_err!(ELIBEXEC, "Cannot exec a shared library directly.");
+    declare_err!(EILSEQ, "Illegal byte sequence.");
+    declare_err!(ERESTART, "Interrupted system call should be restarted.");
+    declare_err!(ESTRPIPE, "Streams pipe error.");
+    declare_err!(EUSERS, "Too many users.");
+    declare_err!(ENOTSOCK, "Socket operation on non-socket.");
+    declare_err!(EDESTADDRREQ, "Destination address required.");
     declare_err!(EMSGSIZE, "Message too long.");
+    declare_err!(EPROTOTYPE, "Protocol wrong type for socket.");
+    declare_err!(ENOPROTOOPT, "Protocol not available.");
+    declare_err!(EPROTONOSUPPORT, "Protocol not supported.");
+    declare_err!(ESOCKTNOSUPPORT, "Socket type not supported.");
+    declare_err!(EOPNOTSUPP, "Operation not supported on transport endpoint.");
+    declare_err!(EPFNOSUPPORT, "Protocol family not supported.");
+    declare_err!(EAFNOSUPPORT, "Address family not supported by protocol.");
+    declare_err!(EADDRINUSE, "Address already in use.");
+    declare_err!(EADDRNOTAVAIL, "Cannot assign requested address.");
+    declare_err!(ENETDOWN, "Network is down.");
+    declare_err!(ENETUNREACH, "Network is unreachable.");
+    declare_err!(ENETRESET, "Network dropped connection because of reset.");
+    declare_err!(ECONNABORTED, "Software caused connection abort.");
+    declare_err!(ECONNRESET, "Connection reset by peer.");
+    declare_err!(ENOBUFS, "No buffer space available.");
+    declare_err!(EISCONN, "Transport endpoint is already connected.");
+    declare_err!(ENOTCONN, "Transport endpoint is not connected.");
+    declare_err!(ESHUTDOWN, "Cannot send after transport endpoint shutdown.");
+    declare_err!(ETOOMANYREFS, "Too many references: cannot splice.");
     declare_err!(ETIMEDOUT, "Connection timed out.");
+    declare_err!(ECONNREFUSED, "Connection refused.");
+    declare_err!(EHOSTDOWN, "Host is down.");
+    declare_err!(EHOSTUNREACH, "No route to host.");
+    declare_err!(EALREADY, "Operation already in progress.");
+    declare_err!(EINPROGRESS, "Operation now in progress.");
+    declare_err!(ESTALE, "Stale file handle.");
+    declare_err!(EUCLEAN, "Structure needs cleaning.");
+    declare_err!(EFSCORRUPTED, "Filesystem is corrupted.");
+    declare_err!(ENOTNAM, "Not a XENIX named type file.");
+    declare_err!(ENAVAIL, "No XENIX semaphores available.");
+    declare_err!(EISNAM, "Is a named type file.");
+    declare_err!(EREMOTEIO, "Remote I/O error.");
+    declare_err!(EDQUOT, "Quota exceeded.");
+    declare_err!(ENOMEDIUM, "No medium found.");
+    declare_err!(EMEDIUMTYPE, "Wrong medium type.");
+    declare_err!(ECANCELED, "Operation Canceled.");
+    declare_err!(ENOKEY, "Required key not available.");
+    declare_err!(EKEYEXPIRED, "Key has expired.");
+    declare_err!(EKEYREVOKED, "Key has been revoked.");
+    declare_err!(EKEYREJECTED, "Key was rejected by service.");
+    declare_err!(EOWNERDEAD, "Owner died.");
+    declare_err!(ENOTRECOVERABLE, "State not recoverable.");
+    declare_err!(ERFKILL, "Operation not possible due to RF-kill.");
+    declare_err!(EHWPOISON, "Memory page has hardware error.");
+    declare_err!(EFTYPE, "Wrong file type for the intended operation.");
+
+    // From include/linux/errno.h
+
     declare_err!(ERESTARTSYS, "Restart the system call.");
     declare_err!(ERESTARTNOINTR, "System call was interrupted by a signal and will be restarted.");
     declare_err!(ERESTARTNOHAND, "Restart if no handler.");
Signature · 1 line
2.55.0
author maintainer
#2 lore raw

PATCH v9 2/5 rust: types: implement ForeignOwnable for ARef<T>

In reply to Philipp Stanner

From: Danilo Krummrich <[email protected]>

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

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

---
 rust/kernel/sync/aref.rs | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)
rust/kernel/sync/aref.rs +40 -0
index b721b2e00b98..540766613659 100644
@@ -24,6 +24,11 @@
     ptr::NonNull, //
 };
 
+use crate::{
+    prelude::*,
+    types::ForeignOwnable, //
+};
+
 /// Types that are _always_ reference counted.
 ///
 /// It allows such types to define their own custom ref increment and decrement functions.
@@ -188,6 +193,41 @@ fn eq(&self, other: &ARef<U>) -> bool {
 }
 impl<T: AlwaysRefCounted + Eq> Eq for ARef<T> {}
 
+// SAFETY: `into_foreign` returns a pointer from `NonNull::as_ptr`, so it's non-null. The
+// `ARef` invariant guarantees that `ptr` points to a valid `T`, so it's aligned to `T`.
+unsafe impl<T: AlwaysRefCounted + 'static> ForeignOwnable for ARef<T> {
+    const FOREIGN_ALIGN: usize = core::mem::align_of::<T>();
+
+    type Borrowed<'a> = &'a T;
+    type BorrowedMut<'a> = &'a T;
+
+    fn into_foreign(self) -> *mut c_void {
+        ARef::into_raw(self).as_ptr().cast()
+    }
+
+    unsafe fn from_foreign(ptr: *mut c_void) -> Self {
+        // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous
+        // call to `Self::into_foreign`.
+        let ptr = unsafe { NonNull::new_unchecked(ptr.cast()) };
+
+        // SAFETY: `ptr` came from `into_foreign`, which consumed an `ARef` without decrementing
+        // the refcount, so we can transfer the ownership to the new `ARef`.
+        unsafe { ARef::from_raw(ptr) }
+    }
+
+    unsafe fn borrow<'a>(ptr: *mut c_void) -> &'a T {
+        // SAFETY: The safety requirements of this method ensure that the object remains alive and
+        // immutable for the duration of 'a.
+        unsafe { &*ptr.cast() }
+    }
+
+    unsafe fn borrow_mut<'a>(ptr: *mut c_void) -> &'a T {
+        // SAFETY: The safety requirements for `borrow_mut` are a superset of the safety
+        // requirements for `borrow`.
+        unsafe { <Self as ForeignOwnable>::borrow(ptr) }
+    }
+}
+
 impl<T, U> PartialEq<&'_ U> for ARef<T>
 where
     T: AlwaysRefCounted + PartialEq<U>,
Signature · 1 line
2.55.0
#3 lore raw

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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] Missing `#[inline]` annotations on small forwarding methods within an abstraction. --

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Missing #[inline] annotations on small forwarding methods within an abstraction. --

--- Patch [2]: [PATCH v9 2/5] rust: types: implement ForeignOwnable for ARef<T> --- commit 46a32182bbc01f0e36763ebdf80c949f544fec26
Author: Danilo Krummrich <[email protected]>

rust: types: implement ForeignOwnable for ARef<T>

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 type.

Quoted 4 lines
rust/kernel/sync/aref.rs +0 -0
index b721b2e00b986..5407666136595 100644

[ ... ]

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

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

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

[Severity: Low]
This isn't a bug, but according to the Rust subsystem guidelines, small or forwarding functions within abstractions should be annotated with #[inline]. Would it be appropriate to add the #[inline] attribute to into_foreign() here?

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

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

[Severity: Low]
Could from_foreign() also benefit from an #[inline] annotation, given it is a small forwarding function?

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

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

[Severity: Low]
Similarly, should borrow() be annotated with #[inline] to match the subsystem guidelines?

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

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

[Severity: Low]
This function also appears to be a short forwarding wrapper. Is it missing the #[inline] attribute as well?

Quoted 2 lines
rust/kernel/sync/aref.rs +2 -0
+}
+

[ ... ]

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

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

In reply to Philipp Stanner
Quoted 32 lines · Philipp Stanner

From: Danilo Krummrich <[email protected]>

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

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

---
 rust/kernel/sync/aref.rs | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)
rust/kernel/sync/aref.rs +5 -0
index b721b2e00b98..540766613659 100644
@@ -24,6 +24,11 @@
     ptr::NonNull, //
 };
 
+use crate::{
+    prelude::*,
+    types::ForeignOwnable, //
+};
+
 /// Types that are _always_ reference counted.
 ///
 /// It allows such types to define their own custom ref increment and decrement functions.

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

Quoted 6 lines
rust/kernel/sync/aref.rs +3 -0
@@ -188,6 +193,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> {

This doesn't need to be static, if you can add where Self: 'a on Borrowed and BorrowedMut instead.

Best,
Gary

Quoted 35 lines
rust/kernel/sync/aref.rs +32 -0
+    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>,
maintainer
#5 lore raw

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

In reply to Philipp Stanner
Quoted 33 lines · Philipp Stanner

From: Danilo Krummrich <[email protected]>

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

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

---
 rust/kernel/sync/aref.rs | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)
rust/kernel/sync/aref.rs +5 -0
index b721b2e00b98..540766613659 100644
@@ -24,6 +24,11 @@
     ptr::NonNull, //
 };
 
+use crate::{
+    prelude::*,
+    types::ForeignOwnable, //
+};
+
 /// Types that are _always_ reference counted.
 ///
 /// It allows such types to define their own custom ref increment and decrement functions.
@@ -188,6 +193,41 @@ fn eq(&self, other: &ARef<U>) -> bool {
 }

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

Quoted 10 lines
rust/kernel/sync/aref.rs +8 -0
 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;
+

#[inline] here and all others.

Best,
Gary

Quoted 30 lines
rust/kernel/sync/aref.rs +27 -0
+    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>,
author maintainer
#6 lore raw

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

In reply to Philipp Stanner

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

Add a safe abstraction for rcu_barrier().

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

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

Quoted 7 lines · Philipp Stanner

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

Add a safe abstraction for rcu_barrier().

Quoted 9 lines
---
 rust/kernel/sync/rcu.rs | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
rust/kernel/sync/rcu.rs +0 -0
index a32bef6e490b..7031ca5d2473 100644

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

Quoted 5 lines
rust/kernel/sync/rcu.rs +2 -0
@@ -50,3 +50,23 @@ fn drop(&mut self) {
 pub fn read_lock() -> Guard {
     Guard::new()
 }
+
+/// Wait until all in-flight call_rcu() callbacks complete.

This misses some `` quoting but these can be applied on fixup.

Best,
Gary

Quoted 18 lines
rust/kernel/sync/rcu.rs +18 -0
+///
+/// Note that this primitive does not necessarily wait for an RCU grace period
+/// to complete.  For example, if there are no RCU callbacks queued anywhere
+/// in the system, then rcu_barrier() is within its rights to return
+/// immediately, without waiting for anything, much less an RCU grace period.
+/// In fact, rcu_barrier() will normally not result in any RCU grace periods
+/// beyond those that were already destined to be executed.
+///
+/// In kernels built with CONFIG_RCU_LAZY=y, this function also hurries all
+/// pending lazy RCU callbacks.
+///
+/// Note that this is one of the RCU primitives which must not be called in
+/// atomic context.
+#[inline]
+pub fn rcu_barrier() {
+    // SAFETY: `rcu_barrier()` is always safe to be called. It just might wait for a grace period.
+    unsafe { bindings::rcu_barrier() };
+}
author maintainer
#8 lore raw

PATCH v9 4/5 rust: Add dma_fence abstractions

In reply to Philipp Stanner

C's dma_fence's are synchronisation primitives that will be needed by all Rust GPU drivers.

The dma_fence framework sets a number of rules, notably:
- fences must only be signaled once
- all fences must be signaled at some point
- fence error codes must only be set before signaling
- every pointer to a fence must be backed by a reference

All those rules are being addressed by these abstractions.

To cleanly decouple fence issuers and consumers, two types are provided:
- DriverFence: the only fence type that can be signaled and that carries driver-specific data.
- Fence: the fence type to be shared with other drivers and / or userspace. The only type callbacks can be registered on. Cannot be signaled.

Hereby, a Fence lives in the same chunk of memory as a DriverFence. Both share the refcount of the underlying C dma_fence. Since this implementation does not provide a custom dma_fence_backend_ops.release() function, the memory is freed by the dma_fence backend once the refcount drops to 0.

To create a DriverFence, the user must first allocate a DriverFenceAllocation, so that the creation of the DriverFence later on can always succeed. Otherwise, deadlocks could occur if fences need to be created in a GPU job submission path.

Synchronization is ensured by the dma_fence backend.

All DriverFence's created through this abstraction must be signaled by the creator with an error code. In case a DriverFence drops without being signaled beforehand, it is signaled with -ECANCELLED as its error and a warning is printed. This allows the Rust abstraction to very cleanly decouple fence issuer and consumer by relying on the decoupling mechanisms in the C backend, which ensures through RCU and the 'signaled' fence-flag that dma_fence_backend_ops functions cannot access the potentially unloaded driver code anymore.

Signalling fences on drop thus grants many advantages. Not signaling fences on drop would risk deadlock and does not grant real advantages:
By definition only the drivers can ensure that a fence always represents the hardware's state correctly.

This implementation models a DmaFenceContext object on which fences are to be created, thereby ensuring correct sequence numbering according to the timeline.

dma_fence supports a variety of callbacks. The mandatory callbacks (get_timeline_name() and get_driver_name()) are implemented in this patch. For convenience, they store those name parameters in the fence context, saving the driver from implementing these two callbacks.

Support for other callbacks (like for hardware signaling) is prepared for through the fact that both DriverFence and Fence live in the same allocation, allowing for usage of container_of from the callback to access the driver-specific data.

It is expected that other callbacks, added in the future, also mostly operate on the generic data in the FenceContext. To make this safe, the implementation ensures through a lifetime that a DriverFence cannot outlive its FenceContext.

Synchronization for dma_fence_ops callbacks is ensured by only running the Rust deconstructor delayed with call_rcu(), which prevents UAF-bugs should a DriverFence drop while a Fence callback is currently operating on the associated driver data. Since they can also operate on the FenceContext's data, its drop implementation also performs the necessary delay with rcu_barrier().

An additional issue discovered during the review process of this code is that there is (currently) no mechanism in Rust to prevent someone from circumventing the DriverFence's FenceContext-reference's lifetime by "forgetting" the fence, e.g. with core::mem::forget(). This would enable UAF bugs on the FenceContext. Throw a panic if this happens and document a path towards a more robust solution.

Add abstractions for dma_fence in Rust.

---
 rust/bindings/bindings_helper.h  |    1 +
 rust/helpers/dma_fence.c         |   48 ++
 rust/helpers/helpers.c           |    1 +
 rust/kernel/dma_buf/dma_fence.rs | 1002 ++++++++++++++++++++++++++++++
 rust/kernel/dma_buf/mod.rs       |   14 +
 rust/kernel/lib.rs               |    1 +
 6 files changed, 1067 insertions(+)
 create mode 100644 rust/helpers/dma_fence.c
 create mode 100644 rust/kernel/dma_buf/dma_fence.rs
 create mode 100644 rust/kernel/dma_buf/mod.rs
rust/bindings/bindings_helper.h +1 -0
index 1124785e210b..54b62d952e01 100644
@@ -53,6 +53,7 @@
 #include <linux/debugfs.h>
 #include <linux/device/faux.h>
 #include <linux/dma-direction.h>
+#include <linux/dma-fence.h>
 #include <linux/dma-mapping.h>
 #include <linux/dma-resv.h>
 #include <linux/errname.h>
rust/helpers/dma_fence.c new file +48 -0

48 lines not on this page · raw message

rust/helpers/helpers.c +1 -0
index 998e31052e66..4ab8aa9da7e7 100644
@@ -58,6 +58,7 @@
 #include "cred.c"
 #include "device.c"
 #include "dma.c"
+#include "dma_fence.c"
 #include "dma-resv.c"
 #include "drm.c"
 #include "drm_gpuvm.c"
rust/kernel/dma_buf/dma_fence.rs new file +1002 -0

1,002 lines not on this page · raw message

rust/kernel/dma_buf/mod.rs new file +14 -0
new file mode 100644
index 000000000000..4764a828642e
@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+
+//! DMA-buf subsystem abstractions.
+
+pub mod dma_fence;
+
+pub use self::dma_fence::{
+    DriverFence,
+    Fence,
+    FenceCallback,
+    FenceCallbackRegistration,
+    FenceContext,
+    FenceContextOps, //
+};
rust/kernel/lib.rs +1 -0
index 68f4d9a3425d..6221ebfe71df 100644
@@ -67,6 +67,7 @@
 pub mod device_id;
 pub mod devres;
 pub mod dma;
+pub mod dma_buf;
 pub mod driver;
 #[cfg(CONFIG_DRM = "y")]
 pub mod drm;
Signature · 1 line
2.55.0
reviewer Reviewed-by on v9 4/5
#9 lore raw

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

Quoted 82 lines · Philipp Stanner

C's dma_fence's are synchronisation primitives that will be needed by all Rust GPU drivers.

The dma_fence framework sets a number of rules, notably:
- fences must only be signaled once
- all fences must be signaled at some point
- fence error codes must only be set before signaling
- every pointer to a fence must be backed by a reference

All those rules are being addressed by these abstractions.

To cleanly decouple fence issuers and consumers, two types are provided:
- DriverFence: the only fence type that can be signaled and that carries driver-specific data.
- Fence: the fence type to be shared with other drivers and / or userspace. The only type callbacks can be registered on. Cannot be signaled.

Hereby, a Fence lives in the same chunk of memory as a DriverFence. Both share the refcount of the underlying C dma_fence. Since this implementation does not provide a custom dma_fence_backend_ops.release() function, the memory is freed by the dma_fence backend once the refcount drops to 0.

To create a DriverFence, the user must first allocate a DriverFenceAllocation, so that the creation of the DriverFence later on can always succeed. Otherwise, deadlocks could occur if fences need to be created in a GPU job submission path.

Synchronization is ensured by the dma_fence backend.

All DriverFence's created through this abstraction must be signaled by the creator with an error code. In case a DriverFence drops without being signaled beforehand, it is signaled with -ECANCELLED as its error and a warning is printed. This allows the Rust abstraction to very cleanly decouple fence issuer and consumer by relying on the decoupling mechanisms in the C backend, which ensures through RCU and the 'signaled' fence-flag that dma_fence_backend_ops functions cannot access the potentially unloaded driver code anymore.

Signalling fences on drop thus grants many advantages. Not signaling fences on drop would risk deadlock and does not grant real advantages:
By definition only the drivers can ensure that a fence always represents the hardware's state correctly.

This implementation models a DmaFenceContext object on which fences are to be created, thereby ensuring correct sequence numbering according to the timeline.

dma_fence supports a variety of callbacks. The mandatory callbacks (get_timeline_name() and get_driver_name()) are implemented in this patch. For convenience, they store those name parameters in the fence context, saving the driver from implementing these two callbacks.

Support for other callbacks (like for hardware signaling) is prepared for through the fact that both DriverFence and Fence live in the same allocation, allowing for usage of container_of from the callback to access the driver-specific data.

It is expected that other callbacks, added in the future, also mostly operate on the generic data in the FenceContext. To make this safe, the implementation ensures through a lifetime that a DriverFence cannot outlive its FenceContext.

Synchronization for dma_fence_ops callbacks is ensured by only running the Rust deconstructor delayed with call_rcu(), which prevents UAF-bugs should a DriverFence drop while a Fence callback is currently operating on the associated driver data. Since they can also operate on the FenceContext's data, its drop implementation also performs the necessary delay with rcu_barrier().

An additional issue discovered during the review process of this code is that there is (currently) no mechanism in Rust to prevent someone from circumventing the DriverFence's FenceContext-reference's lifetime by "forgetting" the fence, e.g. with core::mem::forget(). This would enable UAF bugs on the FenceContext. Throw a panic if this happens and document a path towards a more robust solution.

Quoted 5 lines

Add abstractions for dma_fence in Rust.

Owing to the ongoing discussion in v8, I still think we could propose some refinements in the future, like most source code out there :) But overall this is a solid base to build upon and it doesn’t make sense to delay seeking the “perfect” solution, if such a thing even exists..

Thanks for your hard work here!

#10 lore raw

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

In reply to Philipp Stanner
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Calling Fence::is_signaled() from within a fence callback for the same fence will cause a…

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Calling Fence::is_signaled() from within a fence callback for the same fence will cause a deadlock.
- [High] FenceContext::drop unconditionally calls rcu_barrier(), which can lead to sleeping in atomic context panics and severe performance issues. --

--- Patch [4]: [PATCH v9 4/5] rust: Add dma_fence abstractions --- commit 38c478417a422309c2a228acac8f860a0f96dd34
Author: Philipp Stanner <[email protected]>

rust: Add dma_fence abstractions

This commit introduces abstractions for C's dma_fence to be used by Rust GPU drivers. It implements DriverFence and Fence types to handle signaling, sequence numbering, and callback registration.

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

[ ... ]

rust/kernel/dma_buf/dma_fence.rs line 227

Quoted 13 lines
rust/kernel/dma_buf/dma_fence.rs new file +13 -0
+#[pinned_drop]
+impl<T: FenceContextOps + Send + Sync> PinnedDrop for FenceContext<T> {
+    fn drop(self: Pin<&mut Self>) {
+        // TODO:
+        // It would be better if the fence context signals all forgotten fences
+        // itself. To do so, it would keep a list of unsignaled fences. That
+        // list members would have to be pre-allocated (see
+        // FenceCallback::new_fence_allocation()).
+        if self.nr_of_unsignaled_fences.load(Relaxed) != 0 {
+            panic!("Forgotten fences in FenceContext.");
+        }
+
+        rcu_barrier();

[Severity: High]
Can dropping a FenceContext in atomic context cause a panic here?

Because Rust drops objects implicitly when they go out of scope, a FenceContext dropped in an atomic context (such as inside an interrupt handler or under a spinlock) will trigger a 'sleeping in atomic context' panic since rcu_barrier() blocks system-wide.

Even if dropped safely in process context, could waiting on a system-wide RCU barrier during the destruction of every FenceContext cause severe performance bottlenecks?

[ ... ]

Quoted 1 line

+ pub fn is_signaled(&self) -> bool {

[ ... ]

Quoted 11 lines
rust/kernel/dma_buf/dma_fence.rs new file +11 -0
+        // The fundamental reason is that the C backend currently does guard
+        // setting of the fence's signaled-bit with the fence's spinlock, but
+        // reading is done locklessly.
+        //
+        // See commit c8a5d5ea3ba6a.
+
+        let _ = self.lock();
+
+        ret
+    }
+

rust/kernel/dma_buf/dma_fence.rs line 524

Quoted 10 lines
rust/kernel/dma_buf/dma_fence.rs new file +10 -0
+    fn lock(&self) -> FenceGuard {
+        let mut guard = FenceGuard {
+            inner: self.as_raw(),
+            flags: 0,
+        };
+
+        // SAFETY: fence is valid because self is valid. flag_ptr is
+        // merely a pointer to an integer, whose lifetime is tied to the guard
+        // object.
+        unsafe { bindings::dma_fence_lock_irqsave(guard.inner, &raw mut guard.flags) };

[Severity: High]
Could this result in a deadlock if Fence::is_signaled() is called from within a fence callback?

When a fence is signaled, the C backend (dma_fence_signal_locked) acquires the fence's spinlock and iterates over the registered callbacks, executing them synchronously while holding the lock.

If a Rust FenceCallback implementation calls is_signaled() on that same fence, it invokes self.lock() which will attempt to acquire the exact same spinlock via dma_fence_lock_irqsave(), leading to a non-recursive spinlock deadlock.

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

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

In reply to Philipp Stanner
Quoted 251 lines · Philipp Stanner

C's dma_fence's are synchronisation primitives that will be needed by all Rust GPU drivers.

The dma_fence framework sets a number of rules, notably:
- fences must only be signaled once
- all fences must be signaled at some point
- fence error codes must only be set before signaling
- every pointer to a fence must be backed by a reference

All those rules are being addressed by these abstractions.

To cleanly decouple fence issuers and consumers, two types are provided:
- DriverFence: the only fence type that can be signaled and that carries driver-specific data.
- Fence: the fence type to be shared with other drivers and / or userspace. The only type callbacks can be registered on. Cannot be signaled.

Hereby, a Fence lives in the same chunk of memory as a DriverFence. Both share the refcount of the underlying C dma_fence. Since this implementation does not provide a custom dma_fence_backend_ops.release() function, the memory is freed by the dma_fence backend once the refcount drops to 0.

To create a DriverFence, the user must first allocate a DriverFenceAllocation, so that the creation of the DriverFence later on can always succeed. Otherwise, deadlocks could occur if fences need to be created in a GPU job submission path.

Synchronization is ensured by the dma_fence backend.

All DriverFence's created through this abstraction must be signaled by the creator with an error code. In case a DriverFence drops without being signaled beforehand, it is signaled with -ECANCELLED as its error and a warning is printed. This allows the Rust abstraction to very cleanly decouple fence issuer and consumer by relying on the decoupling mechanisms in the C backend, which ensures through RCU and the 'signaled' fence-flag that dma_fence_backend_ops functions cannot access the potentially unloaded driver code anymore.

Signalling fences on drop thus grants many advantages. Not signaling fences on drop would risk deadlock and does not grant real advantages:
By definition only the drivers can ensure that a fence always represents the hardware's state correctly.

This implementation models a DmaFenceContext object on which fences are to be created, thereby ensuring correct sequence numbering according to the timeline.

dma_fence supports a variety of callbacks. The mandatory callbacks (get_timeline_name() and get_driver_name()) are implemented in this patch. For convenience, they store those name parameters in the fence context, saving the driver from implementing these two callbacks.

Support for other callbacks (like for hardware signaling) is prepared for through the fact that both DriverFence and Fence live in the same allocation, allowing for usage of container_of from the callback to access the driver-specific data.

It is expected that other callbacks, added in the future, also mostly operate on the generic data in the FenceContext. To make this safe, the implementation ensures through a lifetime that a DriverFence cannot outlive its FenceContext.

Synchronization for dma_fence_ops callbacks is ensured by only running the Rust deconstructor delayed with call_rcu(), which prevents UAF-bugs should a DriverFence drop while a Fence callback is currently operating on the associated driver data. Since they can also operate on the FenceContext's data, its drop implementation also performs the necessary delay with rcu_barrier().

An additional issue discovered during the review process of this code is that there is (currently) no mechanism in Rust to prevent someone from circumventing the DriverFence's FenceContext-reference's lifetime by "forgetting" the fence, e.g. with core::mem::forget(). This would enable UAF bugs on the FenceContext. Throw a panic if this happens and document a path towards a more robust solution.

Add abstractions for dma_fence in Rust.

---
 rust/bindings/bindings_helper.h  |    1 +
 rust/helpers/dma_fence.c         |   48 ++
 rust/helpers/helpers.c           |    1 +
 rust/kernel/dma_buf/dma_fence.rs | 1002 ++++++++++++++++++++++++++++++
 rust/kernel/dma_buf/mod.rs       |   14 +
 rust/kernel/lib.rs               |    1 +
 6 files changed, 1067 insertions(+)
 create mode 100644 rust/helpers/dma_fence.c
 create mode 100644 rust/kernel/dma_buf/dma_fence.rs
 create mode 100644 rust/kernel/dma_buf/mod.rs
rust/bindings/bindings_helper.h +1 -0
index 1124785e210b..54b62d952e01 100644
@@ -53,6 +53,7 @@
 #include <linux/debugfs.h>
 #include <linux/device/faux.h>
 #include <linux/dma-direction.h>
+#include <linux/dma-fence.h>
 #include <linux/dma-mapping.h>
 #include <linux/dma-resv.h>
 #include <linux/errname.h>
rust/helpers/dma_fence.c new file +48 -0
new file mode 100644
index 000000000000..0e08411098fa
@@ -0,0 +1,48 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/dma-fence.h>
+
+__rust_helper void rust_helper_dma_fence_get(struct dma_fence *f)
+{
+	dma_fence_get(f);
+}
+
+__rust_helper void rust_helper_dma_fence_put(struct dma_fence *f)
+{
+	dma_fence_put(f);
+}
+
+__rust_helper bool rust_helper_dma_fence_begin_signalling(void)
+{
+	return dma_fence_begin_signalling();
+}
+
+__rust_helper void rust_helper_dma_fence_end_signalling(bool cookie)
+{
+	dma_fence_end_signalling(cookie);
+}
+
+__rust_helper bool rust_helper_dma_fence_is_signaled(struct dma_fence *f)
+{
+	return dma_fence_is_signaled(f);
+}
+
+__rust_helper bool rust_helper_dma_fence_test_signaled_flag(struct dma_fence *f)
+{
+	return dma_fence_test_signaled_flag(f);
+}
+
+__rust_helper void rust_helper_dma_fence_lock_irqsave(struct dma_fence *f, unsigned long *flags)
+{
+	dma_fence_lock_irqsave(f, *flags);
+}
+
+__rust_helper void rust_helper_dma_fence_unlock_irqrestore(struct dma_fence *f, unsigned long *flags)
+{
+	dma_fence_unlock_irqrestore(f, *flags);
+}
+
+__rust_helper void rust_helper_dma_fence_set_error(struct dma_fence *f, int error)
+{
+	dma_fence_set_error(f, error);
+}
rust/helpers/helpers.c +1 -0
index 998e31052e66..4ab8aa9da7e7 100644
@@ -58,6 +58,7 @@
 #include "cred.c"
 #include "device.c"
 #include "dma.c"
+#include "dma_fence.c"
 #include "dma-resv.c"
 #include "drm.c"
 #include "drm_gpuvm.c"
rust/kernel/dma_buf/dma_fence.rs new file +73 -0
new file mode 100644
index 000000000000..e61b4b2d8b8c
@@ -0,0 +1,1002 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2025-2026 Red Hat Inc.
+ * Author: Philipp Stanner <[email protected]>
+ */
+
+//! DriverFence support.
+//!
+//! Reference: <https://docs.kernel.org/driver-api/dma-buf.html#c.dma_fence>
+//!
+//! header: [`include/linux/dma-fence.h`](srctree/include/linux/dma-fence.h)
+
+use crate::{
+    alloc::AllocError,
+    bindings,
+    container_of,
+    error::to_result,
+    prelude::*,
+    types::ForeignOwnable,
+    types::Opaque, //
+};
+
+use core::{
+    marker::PhantomData,
+    mem::ManuallyDrop,
+    ops::Deref,
+    ptr,
+    ptr::{
+        drop_in_place,
+        NonNull, //
+    }, //
+};
+
+use kernel::{
+    str::CString,
+    sync::{
+        aref::{
+            ARef,
+            AlwaysRefCounted, //
+        },
+        atomic::{
+            Atomic,
+            Relaxed, //
+        },
+        rcu::rcu_barrier, //
+    }, //
+};
+
+/// VTable for dma_fence backend_ops callbacks.
+//
+// Mandatory dma_fence backend_ops are implemented implicitly through
+// [`FenceContext`]. Additional ones shall get implemented on this trait.
+pub trait FenceContextOps {
+    /// The generic payload data for [`DriverFence`]s created on this fctx.
+    type FenceDataType: Send + Sync;
+}
+
+/// A dma-fence context. A fence context takes care of associating related fences
+/// with each other, providing each with raising sequence numbers and a common
+/// identifier.
+#[pin_data(PinnedDrop)]
+pub struct FenceContext<T: FenceContextOps + Send + Sync> {
+    /// The fence context number.
+    nr: u64,
+    /// The sequence number for the next fence created.
+    seqno: Atomic<u64>,
+    // The name parameters can be accessed by the dma_fence backend_ops. UAF
+    // errors are prevented by the `call_rcu()` in `drop_driver_fence_data()`.
+    /// The name of the driver this FenceContext's fences belong to.
+    driver_name: CString,
+    /// The name of the timeline this FenceContext's fences belong to.
+    timeline_name: CString,
+    /// The number of all unsignaled fences on this context.

rust/kernel/dma_buf/dma_fence.rs line 83

Quoted 10 lines
rust/kernel/dma_buf/dma_fence.rs new file +10 -0
+    // Used to prevent bugs due to forgotten fences.
+    //
+    // The lifetime on `DriverFence`s should typically prevent this from
+    // happening.
+    //
+    // However, we cannot fully guarantee in Rust that `DriverFence`s will not
+    // be forgotten, e.g., through `core::mem::forget()`. This could circumvent
+    // the lifetime which intends to enforce that all fences disappear before
+    // their context.
+    nr_of_unsignaled_fences: Atomic<u64>,

This can be Atomic<usize> so it doesn't need to go through the generic 64-bit atomic mechanism on 32-bit systems.

Quoted 106 lines
rust/kernel/dma_buf/dma_fence.rs new file +106 -0
+    /// The user's data.
+    #[pin]
+    data: T,
+}
+
+impl<'a, T: Send + Sync + FenceContextOps> FenceContext<T> {
+    // This can later be extended as a vtable in case other parties need support
+    // for the more "exotic" callbacks.
+    const OPS: bindings::dma_fence_ops = bindings::dma_fence_ops {
+        get_driver_name: Some(Self::get_driver_name),
+        get_timeline_name: Some(Self::get_timeline_name),
+        enable_signaling: None,
+        signaled: None,
+        wait: None,
+        release: None,
+        set_deadline: None,
+    };
+
+    /// Create a new `FenceContext`.
+    pub fn new<E>(
+        initial_seqno: u64,
+        driver_name: CString,
+        timeline_name: CString,
+        data: impl PinInit<T, E>,
+    ) -> impl PinInit<Self, Error>
+    where
+        Error: From<E>,
+    {
+        try_pin_init!(Self {
+            // SAFETY: `dma_fence_context_alloc()` merely works on a global
+            // atomic. Parameter `1` is the number of contexts we want to
+            // allocate.
+            nr: unsafe { bindings::dma_fence_context_alloc(1) },
+            seqno: Atomic::new(initial_seqno),
+            driver_name,
+            timeline_name,
+            nr_of_unsignaled_fences: Atomic::new(0),
+            data <- data,
+        })
+    }
+
+    fn next_seqno(&self) -> u64 {
+        self.seqno.fetch_add(1, Relaxed)
+    }
+
+    /// Allocate the memory for a [`DriverFence`] and already store `data` inside.
+    ///
+    /// This is needed because many times, creation of a [`DriverFence`] must not
+    /// fail, and allocating might deadlock in some situations.
+    ///
+    /// The `data` you pass here must not perform any operations that are illegal
+    /// in atomic context in its [`Drop`] implementation.
+    pub fn new_fence_allocation(
+        &self,
+        data: T::FenceDataType,
+    ) -> Result<DriverFenceAllocation<'_, T>> {
+        let fence_data = DriverFenceData {
+            rcu_head: Default::default(),
+            // `inner` remains uninitialized until a `DriverFence` takes over.
+            inner: Fence {
+                inner: Opaque::uninit(),
+            },
+            fctx: self,
+            data,
+        };
+
+        // In order to support the C dma_fence callbacks, it is necessary for
+        // a `Fence` and a `DriverFence` to live in the same allocation,
+        // because the C backend passes a dma_fence, from which the driver most
+        // likely wants to be able to access its `data` in `DriverFence`.
+        //
+        // Hence, we need the manage the memory manually. It will be freed by the
+        // C backend automatically once the refcount within `Fence` drops to 0.
+        let data = KBox::new(fence_data, GFP_KERNEL | __GFP_ZERO)?;
+
+        Ok(DriverFenceAllocation {
+            data,
+            ops: &Self::OPS,
+        })
+    }
+
+    extern "C" fn get_driver_name(ptr: *mut bindings::dma_fence) -> *const c_char {
+        // SAFETY: The C backend only invokes this callback with `ptr` pointing
+        // to a valid, unsignaled `bindings::dma_fence`. All fences created in
+        // this module always reside within `Fence` which always resides in a
+        // `DriverFenceData`, thus satisfying the function's safety
+        // requirements.
+        let fctx = unsafe { Self::from_raw_fence(ptr) };
+
+        fctx.driver_name.as_char_ptr()
+    }
+
+    extern "C" fn get_timeline_name(ptr: *mut bindings::dma_fence) -> *const c_char {
+        // SAFETY: The C backend only invokes this callback with `ptr` pointing
+        // to a valid, unsignaled `bindings::dma_fence`. All fences created in
+        // this module always reside within `Fence` which always resides in a
+        // `DriverFenceData`, thus satisfying the function's safety
+        // requirements.
+        let fctx = unsafe { Self::from_raw_fence(ptr) };
+
+        fctx.timeline_name.as_char_ptr()
+    }
+
+    /// Create a [`FenceContext`] from an associated [`bindings::dma_fence`].
+    ///
+    /// # Safety

rust/kernel/dma_buf/dma_fence.rs line 199

Quoted 10 lines
rust/kernel/dma_buf/dma_fence.rs new file +10 -0
+    ///
+    /// `ptr` must be a valid pointer to a [`bindings::dma_fence`] which resides
+    /// within a [`Fence`], which in turn resides in a [`DriverFenceData`].
+    unsafe fn from_raw_fence(ptr: *mut bindings::dma_fence) -> &'a Self {
+        let opaque_fence = Opaque::cast_from(ptr);
+
+        // SAFETY: Safe due to the function's overall safety requirements.
+        let fence_ptr = unsafe { container_of!(opaque_fence, Fence, inner) };
+
+        // CAST: `DriverFenceData` is repr(C) and a `Fence` is its first member.

repr(C)

rust/kernel/dma_buf/dma_fence.rs line 200

Quoted 1 line
rust/kernel/dma_buf/dma_fence.rs new file +1 -0
+        let fence_data_ptr = fence_ptr as *mut DriverFenceData<'a, T>;

*const should work here too?

Quoted 4 lines
rust/kernel/dma_buf/dma_fence.rs new file +4 -0
+
+        // SAFETY: Safe because of the comments directly above.
+        let fence_data = unsafe { &*fence_data_ptr };
+

rust/kernel/dma_buf/dma_fence.rs line 214

Quoted 10 lines
rust/kernel/dma_buf/dma_fence.rs new file +10 -0
+        fence_data.fctx
+    }
+}
+
+// FenceContext's drop() ensures that the driver cannot unload while there are
+// still dma_fence callbacks running. This also prevents UAF problems with
+// `fctx.driver_name` and `fctx.timeline_name`.
+//
+// DriverFence data gets dropped through `call_rcu()` in `DriverFence::drop`.
+// This `rcu_barrier()` also serves to wait for their completion.

These should be comment on the drop code itself, not as comment of PinnedDrop impl.

This can be more detailed about why a UAF problem exists (and this should be commented on the panic part, as something like:

    // Fence ops callbacks may be called on unsignaled fences, so we may not
    // leak any driver fences, otherwise `fctx.driver_name` and
    // `fctx.timeline_name` can be accessed after drop (UAF).
    if self.nr_of_unsignaled_fences.load(Relaxed) != 0 {
    
    // Fence ops callbacks use RCU to sychronize callbacks and thus we use `call_rcu`
    // to destroy `DriverFence` data. `rcu_barrier` here synchronize with driver
    // fence's destruction.
Quoted 13 lines
rust/kernel/dma_buf/dma_fence.rs new file +13 -0
+#[pinned_drop]
+impl<T: FenceContextOps + Send + Sync> PinnedDrop for FenceContext<T> {
+    fn drop(self: Pin<&mut Self>) {
+        // TODO:
+        // It would be better if the fence context signals all forgotten fences
+        // itself. To do so, it would keep a list of unsignaled fences. That
+        // list members would have to be pre-allocated (see
+        // `FenceCallback::new_fence_allocation()`).
+        if self.nr_of_unsignaled_fences.load(Relaxed) != 0 {
+            panic!("Forgotten fences in FenceContext.");
+        }
+
+        rcu_barrier();

rust/kernel/dma_buf/dma_fence.rs line 237

Quoted 10 lines
rust/kernel/dma_buf/dma_fence.rs new file +10 -0
+    }
+}
+
+/// Error type for fence callback registration.
+///
+/// Generic over `T` so that `AlreadySignaled` can return the callback to the
+/// caller, allowing it to reclaim any resources owned by the callback (e.g.,
+/// a fence handle that needs to be signaled).
+#[derive(Debug)]
+pub enum CallbackError<T = ()> {

What is this = () used for?

rust/kernel/dma_buf/dma_fence.rs line 245

Quoted 8 lines
rust/kernel/dma_buf/dma_fence.rs new file +8 -0
+    /// The fence was already signaled. The callback is returned so the caller
+    /// can extract owned resources without losing them.
+    AlreadySignaled(T),
+    /// Some other error occurred during registration.
+    Other(Error),
+}
+
+impl<T> From<CallbackError<T>> for Error {

#[inline]

rust/kernel/dma_buf/dma_fence.rs line 254

Quoted 9 lines
rust/kernel/dma_buf/dma_fence.rs new file +9 -0
+    fn from(err: CallbackError<T>) -> Self {
+        match err {
+            CallbackError::AlreadySignaled(_) => ENOENT,
+            CallbackError::Other(e) => e,
+        }
+    }
+}
+
+impl<T> From<AllocError> for CallbackError<T> {

#[inline]

Quoted 27 lines
rust/kernel/dma_buf/dma_fence.rs new file +27 -0
+    fn from(e: AllocError) -> Self {
+        CallbackError::Other(Error::from(e))
+    }
+}
+
+/// Trait for callbacks that can be registered on fences.
+///
+/// When the fence signals, the callback will be invoked.
+///
+/// # Example
+///
+/// ```rust
+/// use kernel::dma_buf::FenceCallback;
+///
+/// struct MyCallback {
+///     // Your callback state here
+/// }
+///
+/// impl FenceCallback for MyCallback {
+///     fn called(&mut self) {
+///         pr_info!("Fence signaled!");
+///         // Handle fence completion
+///     }
+/// }
+/// ```
+pub trait FenceCallback: Send + 'static {
+    /// Called when the fence is signaled.

rust/kernel/dma_buf/dma_fence.rs line 291

Quoted 10 lines
rust/kernel/dma_buf/dma_fence.rs new file +10 -0
+    ///
+    /// This is called from the fence signaling path, which may be in interrupt
+    /// context or with locks held, which is why `self` is only borrowed, so that
+    /// it cannot drop. Implementations must not sleep or perform
+    /// long-running operations.
+    ///
+    /// An implementation likely wants to inform itself (e.g., through a work item)
+    /// within this callback that the associated [`FenceCallbackRegistration`]
+    /// can now be dropped.
+    fn called(&mut self);

The name feels a bit awkward to me. I think this should either look like an action on the callback, in which case "call" or describe an event on the fence, i.e. "on_signal" or "signaled". Naming it "called" is very weird because it's not a event that is triggered when something is "called".

Quoted 81 lines
rust/kernel/dma_buf/dma_fence.rs new file +81 -0
+}
+
+/// A callback registration on a fence.
+///
+/// When this object is dropped, the callback is automatically removed if it
+/// hasn't been called yet.
+#[pin_data(PinnedDrop)]
+pub struct FenceCallbackRegistration<T: FenceCallback + 'static> {
+    #[pin]
+    callback_foreign: Opaque<bindings::dma_fence_cb>,
+    callback: ManuallyDrop<T>,
+    fence: ARef<Fence>,
+}
+
+impl<T: FenceCallback> FenceCallbackRegistration<T> {
+    /// Create a [`PinInit`] closure for registering a callback on a fence.
+    ///
+    /// The actual attempt at registering the callback will take place once you
+    /// call an allocator's `pin_init()` function.
+    ///
+    /// On success the callback is pinned in place and will fire when the fence
+    /// signals. On `AlreadySignaled` the callback is returned to the caller so
+    /// that owned resources can be reclaimed.
+    pub fn new<'a>(fence: &'a Fence, callback: T) -> impl PinInit<Self, CallbackError<T>> + 'a
+    where
+        T: 'a,
+    {
+        try_pin_init!(Self {
+            // We need to fully initialize the fence because after
+            // `dma_fence_add_callback()` ran, the callback might immediately
+            // get invoked.
+            callback: ManuallyDrop::new(callback),
+            fence: ARef::from(fence),
+            callback_foreign <- Opaque::try_ffi_init(|ptr| {
+                // SAFETY: `fence.inner.get()` is a valid, initialized `struct
+                // dma_fence`. `ptr` points to the `struct dma_fence_cb` field
+                // within the pinned allocation, so it remains valid until
+                // `dma_fence_remove_callback()` in `PinnedDrop` or until the
+                // callback fires.
+                let ret = unsafe {
+                    to_result(bindings::dma_fence_add_callback(
+                        fence.inner.get(),
+                        ptr,
+                        Some(Self::dma_fence_callback),
+                    ))
+                };
+                match ret {
+                    Ok(()) => Ok(()),
+                    Err(e) => {
+                        // SAFETY: We could not register the callback. Thus,
+                        // C will not use it. So we can just take it back
+                        // and pass it to the user again.
+                        let cb_back = unsafe { ManuallyDrop::take(callback) };
+                        if e == ENOENT {
+                            Err(CallbackError::AlreadySignaled(cb_back))
+                        } else {
+                            Err(CallbackError::Other(e))
+                        }
+                    },
+                }
+            }),
+        }? CallbackError<T>)
+    }
+
+    /// Raw dma fence callback that is called by the C code.
+    ///
+    /// # Safety
+    ///
+    /// This is only called by the dma_fence subsystem with valid pointers.
+    unsafe extern "C" fn dma_fence_callback(
+        _fence: *mut bindings::dma_fence,
+        callback_foreign: *mut bindings::dma_fence_cb,
+    ) {
+        let ptr = Opaque::cast_from(callback_foreign).cast_mut();
+
+        // SAFETY: All `cb` we can receive here have been created in such a way
+        // that they are embedded into a `FenceCallbackRegistration`. The
+        // backend ensures synchronisation so whoever holds the registration
+        // object cannot drop it while this code is running. See
+        // `FenceCallbackRegistration::drop`.
+        unsafe {

rust/kernel/dma_buf/dma_fence.rs line 382

Quoted 10 lines
rust/kernel/dma_buf/dma_fence.rs new file +10 -0
+            let reg: *mut Self = container_of!(ptr, Self, callback_foreign);
+
+            (*reg).callback.called();
+        }
+    }
+
+    /// Returns a reference to the fence this callback is registered on.
+    pub fn fence(self: Pin<&Self>) -> &Fence {
+        &self.get_ref().fence
+    }

This can just be fence(&self) -> &Fence.

rust/kernel/dma_buf/dma_fence.rs line 389

Quoted 7 lines
rust/kernel/dma_buf/dma_fence.rs new file +7 -0
+}
+
+#[pinned_drop]
+impl<T: FenceCallback> PinnedDrop for FenceCallbackRegistration<T> {
+    fn drop(self: Pin<&mut Self>) {
+        // Always call dma_fence_remove_callback, even if `callback` has already
+        // been taken by `dma_fence_callback`.  This is necessary for

Is this still up-to-date? You're not taking callback anymore in dma_fence_callback.

Quoted 59 lines
rust/kernel/dma_buf/dma_fence.rs new file +59 -0
+        // synchronization: `dma_fence_remove_callback` acquires `fence->lock`,
+        // which ensures that any in-flight `dma_fence_signal` (which calls our
+        // callback while holding the same lock) has completed before we free
+        // the struct.
+        //
+        // Without this, Drop can race with a concurrent signal:
+        //   CPU0 (signal, lock held): take() -> signaled(fence_ref) (in progress)
+        //   CPU1 (drop): sees is_some()==false -> skips lock -> frees struct
+        //   CPU0: accesses fence_ref -> use-after-free
+        //
+        // When the callback has already fired, the signal path detached the
+        // list node via INIT_LIST_HEAD, so dma_fence_remove_callback just sees
+        // an empty node and returns false — the lock acquisition is the only
+        // thing that matters.
+        //
+        // SAFETY: The fence pointer is valid and the cb was initialized by
+        // dma_fence_add_callback during construction.
+        unsafe {
+            bindings::dma_fence_remove_callback(self.fence.as_raw(), self.callback_foreign.get());
+        }
+
+        // SAFETY: This is literally the drop implementation, so no one has
+        // dropped this so far; so we can do it now.
+        unsafe { ManuallyDrop::<T>::drop(self.project().callback) };
+    }
+}
+
+// SAFETY: FenceCallbackRegistration can be sent between threads.
+unsafe impl<T: FenceCallback> Send for FenceCallbackRegistration<T> {}
+
+// SAFETY: &FenceCallbackRegistration can be shared between threads if &T can.
+unsafe impl<T: FenceCallback> Sync for FenceCallbackRegistration<T> where T: Sync {}
+
+/// The receiving counterpart of a [`DriverFence`].
+///
+/// The Rust DMA fence implementation has a dualistic design: [`DriverFence`]s
+/// are the producer-side, intended to be always owned by only one party. That
+/// party has the monopoly on signalling the fence.
+///
+/// A [`Fence`] is the counterpart for consumers. Thus, [`Fence`]s are always
+/// refcounted and can shared with an arbitrary number of parties, including
+/// userspace. A [`Fence`] can only be used for actions such as checking the
+/// fence's status or for registering callbacks on it.
+///
+/// Once the associated [`DriverFence`] signals, all
+/// [`FenceCallbackRegistration`]s registered on the [`Fence`] will be executed.
+///
+/// A [`Fence`] can arbitrarily outlive its [`DriverFence`] and the
+/// [`FenceContext`]. Signalling a [`DriverFence`] decouples it from its
+/// [`Fence`]s.
+#[repr(transparent)]
+pub struct Fence {
+    /// The actual dma_fence passed to C.
+    inner: Opaque<bindings::dma_fence>,
+}
+
+/// Guard helper for locking within this module.
+///
+/// Its only purpose for now is to avoid a number of unsafe lock-unlock cycles.

rust/kernel/dma_buf/dma_fence.rs line 458

Quoted 10 lines
rust/kernel/dma_buf/dma_fence.rs new file +10 -0
+/// It is never used outside of this module.
+// TODO: This should be made more canonical, probably by basing it on a
+// SpinLockIrqGuard once available.
+struct FenceGuard {
+    inner: *mut bindings::dma_fence,
+    flags: usize,
+}
+
+impl Deref for FenceGuard {
+    type Target = *mut bindings::dma_fence;

Why not store and return &Fence?

Quoted 41 lines
rust/kernel/dma_buf/dma_fence.rs new file +41 -0
+
+    fn deref(&self) -> &Self::Target {
+        &self.inner
+    }
+}
+
+impl Drop for FenceGuard {
+    fn drop(&mut self) {
+        // SAFETY: `fence` is valid because `self` is valid. `flag_ptr` is
+        // merely a pointer to an integer, which lives as long as this function.
+        // When a `FenceGuard` exists, the lock has been taken by definition.
+        unsafe { bindings::dma_fence_unlock_irqrestore(self.inner, &raw mut self.flags) };
+    }
+}
+
+// SAFETY: Fences are literally designed to be shared between threads.
+unsafe impl Send for Fence {}
+// SAFETY: Fences are literally designed to be shared between threads.
+unsafe impl Sync for Fence {}
+
+impl Fence {
+    /// Check whether the fence was signaled at the moment of the function call.
+    ///
+    /// Note that this can return `true` for a [`Fence`] whose [`DriverFence`]
+    /// has not yet been dropped. The reason is that the fence ops callbacks can
+    /// cause the fence to get signaled by the C backend.
+    pub fn is_signaled(&self) -> bool {
+        // We should not use `dma_fence_is_signaled_locked()` here, because
+        // according to the C backend's recommendations, that function is
+        // problematic and we should avoid calling that function with a lock
+        // held.
+
+        // SAFETY: Inner `fence` is valid because `self` is valid.
+        let ret = unsafe { bindings::dma_fence_is_signaled(self.as_raw()) };
+
+        // To be as robust as possible for the future we guarantee that an API
+        // caller can 100% rely on the signalling being completed (i.e., all
+        // fence callbacks ran), so we have to take the lock.
+        //
+        // The reason is that the C dma_fence backend currently does not
+        // carefully synchronize the `dma_fence_is_signaled()` function with the

rust/kernel/dma_buf/dma_fence.rs line 509

Quoted 10 lines
rust/kernel/dma_buf/dma_fence.rs new file +10 -0
+        // proper spinlock. This can lead to the function returning `true` while
+        // fence callbacks are still being executed. This can be mitigated by
+        // guarding the entire function with the spinlock.
+        //
+        // The fundamental reason is that the C backend currently does guard
+        // setting of the fence's signaled-bit with the fence's spinlock, but
+        // reading is done locklessly.
+        //
+        // See commit c8a5d5ea3ba6a.
+

Extra newline here.

rust/kernel/dma_buf/dma_fence.rs line 514

Quoted 5 lines
rust/kernel/dma_buf/dma_fence.rs new file +5 -0
+        let _ = self.lock();
+
+        ret
+    }
+

#[inline] here and many more below.

Quoted 188 lines
rust/kernel/dma_buf/dma_fence.rs new file +188 -0
+    fn lock(&self) -> FenceGuard {
+        let mut guard = FenceGuard {
+            inner: self.as_raw(),
+            flags: 0,
+        };
+
+        // SAFETY: `fence` is valid because `self` is valid. `flag_ptr` is
+        // merely a pointer to an integer, whose lifetime is tied to the guard
+        // object.
+        unsafe { bindings::dma_fence_lock_irqsave(guard.inner, &raw mut guard.flags) };
+
+        guard
+    }
+
+    /// Get the fence's sequence number.
+    pub fn seqno(&self) -> u64 {
+        // SAFETY: Valid because `self` is valid.
+        unsafe { (*self.as_raw()).seqno }
+    }
+
+    fn as_raw(&self) -> *mut bindings::dma_fence {
+        self.inner.get()
+    }
+
+    /// Create a [`Fence`] from a raw C [`bindings::dma_fence`].
+    ///
+    /// # Safety
+    ///
+    /// `ptr` must point to an initialized fence that is embedded into a [`Fence`].
+    pub unsafe fn from_raw<'a>(ptr: *mut bindings::dma_fence) -> &'a Self {
+        // SAFETY: Safe as per the function's overall safety requirements.
+        unsafe { &*ptr.cast() }
+    }
+}
+
+// SAFETY: These implement the C backends refcounting methods which are proven
+// to work correctly.
+unsafe impl AlwaysRefCounted for Fence {
+    fn inc_ref(&self) {
+        // SAFETY: `self.as_raw()` is a pointer to a valid `struct dma_fence`.
+        unsafe { bindings::dma_fence_get(self.as_raw()) }
+    }
+
+    /// # Safety
+    ///
+    /// `ptr`must be a valid pointer to a [`DriverFence`].
+    unsafe fn dec_ref(ptr: NonNull<Self>) {
+        // SAFETY: `ptr` is never a NULL pointer; and when `dec_ref()` is called
+        // the fence is by definition still valid.
+        let fence = unsafe { (*ptr.as_ptr()).inner.get() };
+
+        // SAFETY: `fence` was created validly above. When `dec_ref()` is called,
+        // there is by definition still a reference alive that can be put.
+        unsafe { bindings::dma_fence_put(fence) }
+    }
+}
+
+// Necessary to guarantee that `inner` always comes first and can be freed by C.
+// Also useful for using casts instead of container_of().
+#[repr(C)]
+#[pin_data]
+struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> {
+    #[pin]
+    /// The inner fence.
+    // Must always be the first member so that unsafe casting works; but also
+    // necessary so that the C backend can free the allocation (coming from our
+    // Rust code) with kfree_rcu().
+    inner: Fence,
+    /// Callback head for dropping this in a deferred manner through RCU.
+    rcu_head: bindings::callback_head,
+    /// Reference to access the FenceContext. Useful for obtaining name parameters.
+    fctx: &'a FenceContext<T>,
+    /// The API user's data. It is essential that the data only performs
+    /// operations legal in atomic context in its [`Drop`] implementation.
+    #[pin]
+    data: T::FenceDataType,
+}
+
+/// A synchronization primitive mainly for GPU drivers.
+///
+/// The Rust DMA fence implementation has a dualistic design: [`DriverFence`]s
+/// are the producer-side, intended to be always owned by only one party. That
+/// party has the monopoly on signalling the fence.
+///
+/// A [`Fence`] is the counterpart for consumers. Thus, [`Fence`]s are always
+/// refcounted and can shared with an arbitrary number of parties, including
+/// userspace. A [`Fence`] can only be used for actions such as checking the
+/// fence's status or for registering callbacks on it.
+///
+/// Once the associated [`DriverFence`] signals, all
+/// [`FenceCallbackRegistration`]s registered on a [`Fence`] will be executed.
+///
+/// A [`Fence`] can arbitrarily outlive its [`DriverFence`] and the
+/// [`FenceContext`]. Signalling a [`DriverFence`] decouples it from its
+/// [`Fence`]s.
+///
+/// It is crucial that a [`DriverFence`] always correctly represents the state
+/// of the associated job on the hardware. Especially, it is strictly necessary
+/// that the owner ensures that all [`DriverFence`]s eventually get signaled.
+/// As a last resort, a [`DriverFence`] will signal itself if it drops
+/// unsignaled and print a warning.
+///
+/// This design intends to implement the [`bindings::dma_fence_ops`] in such a
+/// way that the driver-data necessary to implement the callback's functionality
+/// resides in the [`FenceContext`]. Thus, a [`DriverFence`] contains a
+/// reference to the context, which can be accessed in the callbacks. The
+/// implementation, therefore, ensures that a [`DriverFence`] cannot outlive its
+/// [`FenceContext`]. Unfortunately, this can be circumvented under certain
+/// circumstances in Rust (e.g., usage of [`core::mem::forget`]).
+///
+/// In the unlikely case of such violations, error warnings are printed.
+///
+/// # Examples
+///
+/// ```
+/// use kernel::dma_buf::{
+///     DriverFence,
+///     FenceContext,
+///     FenceContextOps,
+///     FenceCallback,
+///     FenceCallbackRegistration, //
+/// };
+/// use kernel::str::CString;
+/// use kernel::sync::aref::ARef;
+/// use core::fmt::Display;
+///
+/// struct CallbackData { }
+///
+/// impl FenceCallback for CallbackData {
+///     fn called(&mut self) {
+///         pr_info!("DmaFence callback executed.\n");
+///     }
+/// }
+///
+/// #[pin_data]
+/// struct FenceContextData {}
+///
+/// impl FenceContextData {
+///     fn new() -> impl PinInit<Self> {
+///         pin_init!(Self {})
+///     }
+/// }
+///
+/// impl FenceContextOps for FenceContextData {
+///     type FenceDataType = FenceData;
+/// }
+///
+/// let fctx_data = FenceContextData::new();
+///
+/// let driver_name = CString::try_from_fmt(fmt!("dummy_driver"))?;
+/// let timeline_name = CString::try_from_fmt(fmt!("dummy_timeline"))?;
+///
+/// let mut fctx = KBox::pin_init(
+///     FenceContext::new(0, driver_name, timeline_name, fctx_data), GFP_KERNEL)?;
+///
+/// struct FenceData {
+///     data: CString,
+/// }
+///
+/// let data = CString::try_from_fmt(fmt!("dummy_data"))?;
+/// let fence_data = FenceData { data };
+///
+/// let fence_alloc = fctx.new_fence_allocation(fence_data)?;
+/// let mut fence = fence_alloc.new_fence();
+///
+/// let cb_data = CallbackData { };
+/// let waiting_fence = ARef::from(fence.as_fence());
+/// let cb_reg = FenceCallbackRegistration::new(&waiting_fence, cb_data);
+/// let cb_reg = KBox::pin_init(cb_reg, GFP_KERNEL)?;
+///
+/// // TODO signalling guards
+/// fence.signal(Ok(()));
+/// assert_eq!(waiting_fence.is_signaled(), true);
+///
+/// Ok::<(), Error>(())
+/// ```
+pub struct DriverFence<'a, T: Send + Sync + FenceContextOps> {
+    /// The actual content of the fence. Lives in a [`NonNull`] so that its
+    /// memory can be managed independently. Valid until both the [`DriverFence`]
+    /// and all associated [`Fence`]s have disappeared.
+    data: NonNull<DriverFenceData<'a, T>>,
+}
+
+/// A pre-prepared DMA fence, carrying the user's data and the memory it and the
+/// fence reside in. Only useful for creating a [`DriverFence`]. Splitting
+/// allocation and full initialization is necessary because fences cannot be
+/// allocated dynamically in some circumstances (deadlock).
+pub struct DriverFenceAllocation<'a, T: Send + Sync + FenceContextOps> {

rust/kernel/dma_buf/dma_fence.rs line 712

Quoted 10 lines
rust/kernel/dma_buf/dma_fence.rs new file +10 -0
+    /// The memory for the actual content of the fence.
+    /// Handed over to a [`DriverFence`], or deallocated once the
+    /// [`DriverFenceAllocation`] drops.
+    data: KBox<DriverFenceData<'a, T>>,
+    /// Pointer for the ops for the associated [`FenceContext`]
+    ops: *const bindings::dma_fence_ops,
+}
+
+impl<'a, T: Send + Sync + FenceContextOps> DriverFenceAllocation<'a, T> {
+    /// Create a new fence, consuming `data`.

There's no data.

Quoted 67 lines
rust/kernel/dma_buf/dma_fence.rs new file +67 -0
+    ///
+    /// This increments the sequence number in the associated [`FenceContext`].
+    pub fn new_fence(self) -> DriverFence<'a, T> {
+        // We feed the C dma_fence backend a NULL for the spinlock so that it
+        // uses per-fence locks automatically.
+        let null_ptr: *mut bindings::spinlock = ptr::null_mut();
+        let seqno = self.data.fctx.next_seqno();
+        let fence_ptr = self.as_raw();
+        // SAFETY: `fence_ptr` has been created directly above. It will live
+        // at least as long as `Self`. The same applies to `&Self::OPS`.
+        unsafe {
+            bindings::dma_fence_init(fence_ptr, self.ops, null_ptr, self.data.fctx.nr, seqno)
+        };
+
+        self.data.fctx.nr_of_unsignaled_fences.fetch_add(1, Relaxed);
+
+        // A `DriverFenceAllocation`'s purpose is to carry allocated memory, so that
+        // `DriverFence`s can always be created without allocating. In this
+        // method, ownership over that memory is transferred to the new
+        // `DriverFence` and managed through refcounting. The C dma_fence
+        // backend will ultimately free the memory once the refcount reaches 0.
+        let ptr = KBox::into_raw(self.data);
+        // SAFETY: `ptr` was just created validly directly above.
+        let ptr = unsafe { NonNull::new_unchecked(ptr) };
+
+        DriverFence { data: ptr }
+    }
+
+    fn as_raw(&self) -> *mut bindings::dma_fence {
+        self.data.inner.inner.get()
+    }
+}
+
+impl<'a, T: Send + Sync + FenceContextOps> DriverFence<'a, T> {
+    fn as_raw(&self) -> *mut bindings::dma_fence {
+        // SAFETY: Valid because `self` is valid.
+        let fence_data = unsafe { &*self.data.as_ptr() };
+
+        fence_data.inner.inner.get()
+    }
+
+    /// Create a [`DriverFence`] from a raw pointer to a [`bindings::dma_fence`].
+    ///
+    /// # Safety
+    ///
+    /// `ptr` must be a valid pointer to a `dma_fence` that was obtained through
+    /// a [`DriverFence`] with matching generic data for both fence and associated
+    /// [`FenceContext`].
+    unsafe fn from_raw(ptr: *mut bindings::dma_fence) -> Self {
+        let opaque_fence = Opaque::cast_from(ptr);
+
+        // SAFETY: Safe due to the function's overall safety requirements.
+        let fence_ptr = unsafe { container_of!(opaque_fence, Fence, inner) };
+
+        // DriverFenceData is repr(C) and a Fence is its first member.
+        let fence_data_ptr = fence_ptr as *mut DriverFenceData<'a, T>;
+
+        // SAFETY: `fence_data_ptr` was created validly above.
+        let data = unsafe { NonNull::new_unchecked(fence_data_ptr) };
+
+        Self { data }
+    }
+
+    /// Return the underlying [`Fence`].
+    pub fn as_fence(&self) -> &Fence {
+        // SAFETY: `self` is by definition still valid, and it cannot drop until
+        // this new reference is gone.

rust/kernel/dma_buf/dma_fence.rs line 789

Quoted 10 lines
rust/kernel/dma_buf/dma_fence.rs new file +10 -0
+        unsafe { Fence::from_raw(self.as_raw()) }
+    }
+
+    /// Signal the fence. This will invoke all registered callbacks.
+    pub fn signal(self, res: Result) {
+        let fence = self.as_fence().lock();
+
+        // SAFETY: `fence` is valid because `self` is valid. The lock must be
+        // held, which we acquired directly above.
+        if !unsafe { bindings::dma_fence_test_signaled_flag(*fence.deref()) } {

These *fence.deref() are quite weird as consequence of FenceGuard design. If FenceGuard just derefs to &Fence then this can be fence.as_raw().

rust/kernel/dma_buf/dma_fence.rs line 802

Quoted 13 lines
rust/kernel/dma_buf/dma_fence.rs new file +13 -0
+            if let Err(err) = res {
+                // SAFETY: `fence` is valid because `self` is valid. The fence
+                // must not have been signaled yet, which we check directly above.
+                unsafe { bindings::dma_fence_set_error(*fence.deref(), err.to_errno()) };
+            }
+            // SAFETY: `fence` is valid because `self` is valid. The lock must
+            // be held, which we acquired above.
+            unsafe { bindings::dma_fence_signal_locked(*fence.deref()) };
+        }
+
+        // SAFETY: `self.data` is valid because `self` is valid.
+        let fctx = unsafe { self.data.as_ref().fctx };
+        let _ = fctx.nr_of_unsignaled_fences.fetch_sub(1, Relaxed);

Drop impl of self here will neededlessly take lock again before checking it's signaled already and unlock.

Quoted 28 lines
rust/kernel/dma_buf/dma_fence.rs new file +28 -0
+    }
+}
+
+// SAFETY: Fences are literally designed to be shared between threads.
+unsafe impl<'a, T: Send + Sync + FenceContextOps> Send for DriverFence<'a, T> {}
+// SAFETY: Fences are literally designed to be shared between threads.
+unsafe impl<'a, T: Send + Sync + FenceContextOps> Sync for DriverFence<'a, T> {}
+
+impl<'a, T: Send + Sync + FenceContextOps> Deref for DriverFence<'a, T> {
+    type Target = T::FenceDataType;
+
+    fn deref(&self) -> &Self::Target {
+        // SAFETY: Thanks to refcounting, `data` is always valid as long as `self` is.
+        let data = unsafe { &*self.data.as_ptr() };
+
+        &data.data
+    }
+}
+
+/// A borrow wrapper for [`DriverFence`]. Implements [`Deref`].
+pub struct DriverFenceBorrow<'a, T: Send + Sync + FenceContextOps> {
+    driver_fence: ManuallyDrop<DriverFence<'a, T>>,
+    _lifetime: PhantomData<&'a T>,
+}
+
+impl<'a, T: Send + Sync + FenceContextOps> Deref for DriverFenceBorrow<'a, T> {
+    type Target = DriverFence<'a, T>;
+

rust/kernel/dma_buf/dma_fence.rs line 840

Quoted 10 lines
rust/kernel/dma_buf/dma_fence.rs new file +10 -0
+    fn deref(&self) -> &Self::Target {
+        self.driver_fence.deref()
+    }
+}
+
+// SAFETY: The Rust dma_fence abstractions are already designed around the inner
+// C `dma_fence`, which can serve safely as the identification point when being
+// owned by C. Moreover, safety is ensured by not dropping `DriverFence` and by
+// only allowing operations without side effects on the Borrowed type.
+unsafe impl<T: Send + Sync + FenceContextOps + 'static> ForeignOwnable for DriverFence<'_, T> {

The 'static shouldn't be needed here.

Quoted 98 lines
rust/kernel/dma_buf/dma_fence.rs new file +98 -0
+    type Borrowed<'a>
+        = DriverFenceBorrow<'a, T>
+    where
+        Self: 'a;
+    type BorrowedMut<'a>
+        = DriverFenceBorrow<'a, T>
+    where
+        Self: 'a;
+
+    const FOREIGN_ALIGN: usize = core::mem::align_of::<bindings::dma_fence>();
+
+    fn into_foreign(self) -> *mut c_void {
+        let fence = self;
+
+        let ptr = fence.as_raw();
+
+        // DriverFence must not drop.
+        let _ = ManuallyDrop::new(fence);
+
+        ptr.cast()
+    }
+
+    unsafe fn from_foreign(ptr: *mut c_void) -> Self {
+        // SAFETY: Safe because the trait implementation only invokes this with
+        // a valid `ptr`, associated to a `DriverFence` with matching generic data.
+        unsafe { Self::from_raw(ptr.cast()) }
+    }
+
+    unsafe fn borrow<'a>(ptr: *mut c_void) -> Self::Borrowed<'a>
+    where
+        Self: 'a,
+    {
+        // SAFETY: The trait implementation ensures that `ptr` always resides
+        // within a [`Fence`] within a [`DriverFenceData`].
+        let driver_fence = unsafe { Self::from_raw(ptr.cast()) };
+
+        let driver_fence = ManuallyDrop::new(driver_fence);
+
+        DriverFenceBorrow {
+            driver_fence,
+            _lifetime: PhantomData,
+        }
+    }
+
+    unsafe fn borrow_mut<'a>(ptr: *mut c_void) -> Self::BorrowedMut<'a>
+    // FIXME: The bound below and the one above in `borrow` should actually be
+    // unnecessary since the compiler should be able to completely derive all
+    // necessary information automatically. There is currently a compiler bug
+    // preventing that, though:
+    //
+    // https://github.com/rust-lang/rust/issues/155430.
+    //
+    // (Help to) fix the compiler bug and remove the bounds afterwards.
+    where
+        Self: 'a,
+    {
+        // SAFETY: The trait implementation ensures that `ptr` always resides
+        // within a [`Fence`] within a [`DriverFenceData`].
+        let driver_fence = unsafe { Self::from_raw(ptr.cast()) };
+
+        let driver_fence = ManuallyDrop::new(driver_fence);
+
+        DriverFenceBorrow {
+            driver_fence,
+            _lifetime: PhantomData,
+        }
+    }
+}
+
+impl<'a, T: Send + Sync + FenceContextOps> Drop for DriverFence<'a, T> {
+    fn drop(&mut self) {
+        let guard = self.as_fence().lock();
+
+        // Use dma_fence_test_signaled_flag() instead of
+        // dma_fence_is_signaled_locked() because the C backend wants to get rid
+        // of the latter.
+
+        // SAFETY: `guard` is valid until the `call_rcu()` below.
+        let signaled: bool = unsafe { bindings::dma_fence_test_signaled_flag(*guard.deref()) };
+        if !signaled {
+            pr_err!("DriverFence drops unsignaled. Danger of memory corruption!\n");
+            // SAFETY: `guard` is valid until the `call_rcu()` below. The fence
+            // must not have been signaled yet, which we check directly above.
+            unsafe { bindings::dma_fence_set_error(*guard.deref(), ECANCELED.to_errno()) };
+            // SAFETY: `guard` is valid until the `call_rcu()` below. The lock
+            // must be held, which we acquired above.
+            unsafe { bindings::dma_fence_signal_locked(*guard.deref()) };
+
+            // SAFETY: `self.data` is valid because `self` is valid.
+            let fctx = unsafe { self.data.as_ref().fctx };
+            let _ = fctx.nr_of_unsignaled_fences.fetch_sub(1, Relaxed);
+        }
+        drop(guard);
+
+        // SAFETY: Valid because `self` is valid.
+        let rcu_head_ptr = unsafe { &raw mut (*self.data.as_ptr()).rcu_head };
+
+        // `DriverFenceData` but could be accessed through some dma_fence

rust/kernel/dma_buf/dma_fence.rs line 948

Quoted 10 lines
rust/kernel/dma_buf/dma_fence.rs new file +10 -0
+        // callbacks right now. Access is being revoked in principle above by
+        // signalling the fence, but since the C backend does not guarantee
+        // perfect full synchronization, we have to wait for one grace period to
+        // ensure that all accessors of `DriverFenceData` (through the
+        // dma_fence_ops accessible through a `Fence`) are gone.
+
+        // SAFETY: `call_rcu()` is always safe to be called. `rcu_head_ptr` was
+        // created validly above. The module must perform a `synchronize_rcu()`
+        // or `rcu_barrier()` call to guard against module unload.
+        unsafe { bindings::call_rcu(rcu_head_ptr, Some(drop_driver_fence_data::<T>)) };

I thought at some point it was mentioned that we want a fast path

    if !mem::needs_drop::<...>() {
    }

?

Quoted 26 lines
rust/kernel/dma_buf/dma_fence.rs new file +26 -0
+    }
+}
+
+// TODO:
+// The entire call_rcu() mechanism in the drop above and the code below would be
+// unnecessary if C's dma_fence_signal() could be reworked in a way that after it
+// ran, the caller knows that no fence_ops callbacks can be running anymore.
+// In other words, if the dma_fence backend would use its spinlock for full
+// synchronization.
+//
+// Then we could move the drop_in_place() and dma_fence_put() upwards into the
+// drop() implementation and call it a day.
+
+/// Finally really drop this `DriverFence<T>`
+///
+/// # Safety
+///
+/// `head` references the `rcu_head` field of an `DriverFenceData<T>`. All
+/// accessors to that `DriverFenceData<T>` must be gone by now. This must be
+/// ensured by signalling the associated `DriverFence<T>` and then waiting
+/// for a grace period until calling this function here.
+unsafe extern "C" fn drop_driver_fence_data<T: Send + Sync + FenceContextOps>(
+    head: *mut bindings::callback_head,
+) {
+    // SAFETY: Caller provides a pointer to the `rcu_head` field of a `DriverFenceData<C>`.
+    let fence_data = unsafe { container_of!(head, DriverFenceData<'_, T>, rcu_head) };

rust/kernel/dma_buf/dma_fence.rs line 984

Quoted 10 lines
rust/kernel/dma_buf/dma_fence.rs new file +10 -0
+
+    // SAFETY: `fence_data` was created validly above. All the fence's data will
+    // only drop below, but the raw pointer to the raw C `dma_fence` remains
+    // valid because the reference count is only decremented at the end of the
+    // function.
+    let fence = unsafe { (*fence_data).inner.inner.get() };
+
+    // SAFETY: `fence_data` was created validly above. A grace period has passed.
+    // All callbacks which might have had access to the `fctx` are gone now.
+    unsafe { drop_in_place(&raw mut (*fence_data).fctx) };

fctx is just a reference, so this is a no-op.

Best,
Gary

Quoted 50 lines
rust/kernel/dma_buf/dma_fence.rs new file +18 -0
+
+    // SAFETY: `fence_data` was created validly above. The user has already
+    // dropped the only conventional accessor to the user data, the `DriverFence`,
+    // one grace period ago. All accessors are gone now.
+    unsafe { drop_in_place(&raw mut (*fence_data).data) };
+
+    // The inner `Fence` explicitly does not get dropped because there may be
+    // many more users / consumers, each holding their own reference.
+
+    // SAFETY: Once a `DriverFence` is initialized, the inner `fence` is
+    // valid and initialized. It is valid until the refcount drops
+    // to 0, which can earliest happen once we drop the `DriverFence`'s reference
+    // here.
+    unsafe { bindings::dma_fence_put(fence) };
+
+    // The actual memory the data associated with a `DriverFence` lives in
+    // gets freed by the C dma_fence backend once the fence's refcount reaches 0.
+}
rust/kernel/dma_buf/mod.rs new file +14 -0
new file mode 100644
index 000000000000..4764a828642e
@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+
+//! DMA-buf subsystem abstractions.
+
+pub mod dma_fence;
+
+pub use self::dma_fence::{
+    DriverFence,
+    Fence,
+    FenceCallback,
+    FenceCallbackRegistration,
+    FenceContext,
+    FenceContextOps, //
+};
rust/kernel/lib.rs +1 -0
index 68f4d9a3425d..6221ebfe71df 100644
@@ -67,6 +67,7 @@
 pub mod device_id;
 pub mod devres;
 pub mod dma;
+pub mod dma_buf;
 pub mod driver;
 #[cfg(CONFIG_DRM = "y")]
 pub mod drm;
#12 lore raw

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

Quoted 2 lines · Gary Guo
Quoted 1 line · Philipp Stanner

[…]

(feel free to crop huge mails like this)

Quoted 9 lines

rust/kernel/dma_buf/dma_fence.rs line 237

Quoted 7 lines
rust/kernel/dma_buf/dma_fence.rs new file +7 -0
+/// Error type for fence callback registration.
+///
+/// Generic over `T` so that `AlreadySignaled` can return the callback to the
+/// caller, allowing it to reclaim any resources owned by the callback (e.g.,
+/// a fence handle that needs to be signaled).
+#[derive(Debug)]
+pub enum CallbackError<T = ()> {

What is this = () used for?

A relic, can remove.

Quoted 30 lines

rust/kernel/dma_buf/dma_fence.rs line 245

Quoted 9 lines

+    /// 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),

rust/kernel/dma_buf/dma_fence.rs new file +3 -0
+}
+
+impl<T> From<CallbackError<T>> for Error {

#[inline]

rust/kernel/dma_buf/dma_fence.rs line 254

Quoted 9 lines

+    fn from(err: CallbackError<T>) -> Self {
+        match err {
+            CallbackError::AlreadySignaled(_) => ENOENT, +            CallbackError::Other(e) => e,
+        }
+    }

rust/kernel/dma_buf/dma_fence.rs new file +3 -0
+}
+
+impl<T> From<AllocError> for CallbackError<T> {

#[inline]

Quoted 6 lines

+    fn from(e: AllocError) -> Self {
+        CallbackError::Other(Error::from(e))
+    }

(quoted diff) +2 -0
+}
+

[…]

Quoted 13 lines
Quoted 4 lines

+pub trait FenceCallback: Send + 'static {
+    /// Called when the fence is signaled.
+    ///
+    /// This is called from the fence signaling path, which may be in interrupt

Quoted 8 lines

+    /// context or with locks held, which is why self is only borrowed, so that +    /// it cannot drop. Implementations must not sleep or perform
+    /// long-running operations.
+    ///
+    /// An implementation likely wants to inform itself (e.g., through a work item) +    /// within this callback that the associated [FenceCallbackRegistration] +    /// can now be dropped.
+    fn called(&mut self);

Quoted 4 lines

The name feels a bit awkward to me. I think this should either look like an action on the callback, in which case "call" or describe an event on the fence, i.e. "on_signal" or "signaled". Naming it "called" is very weird because it's not a event that is triggered when something is "called".

on_signal() sounds good.

Quoted 4 lines
Quoted 4 lines
(quoted diff) +2 -0
+}
+

[…]

Quoted 8 lines

rust/kernel/dma_buf/dma_fence.rs line 389

Quoted 5 lines
rust/kernel/dma_buf/dma_fence.rs new file +5 -0
+#[pinned_drop]
+impl<T: FenceCallback> PinnedDrop for FenceCallbackRegistration<T> {
+    fn drop(self: Pin<&mut Self>) {
+        // Always call dma_fence_remove_callback, even if `callback` has already
+        // been taken by `dma_fence_callback`.  This is necessary for

Is this still up-to-date? You're not taking callback anymore in dma_fence_callback.

The comment is outdated, but the race prevention is still necessary. I'll update the text.

Quoted 2 lines
Quoted 2 lines

[…]

Quoted 4 lines
Quoted 2 lines

+impl Deref for FenceGuard {
+    type Target = *mut bindings::dma_fence;

Why not store and return &Fence?

Sounds good, although it would be cool if the FenceGuard directly derefs to the raw pointer, since the need for accessing the latter with the lock held is the reason the guard exists.

Quoted 3 lines
Quoted 3 lines

+

[…]

Quoted 2 lines

#[inline] here and many more below.

I do not understand the Rust policy regarding inline at all. When do you need it and when not?

Quoted 17 lines
Quoted 7 lines

+    fn lock(&self) -> FenceGuard {
+        let mut guard = FenceGuard {
+            inner: self.as_raw(),
+            flags: 0,
+        };

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

rust/kernel/dma_buf/dma_fence.rs line 530

Quoted 10 lines
rust/kernel/dma_buf/dma_fence.rs new file +10 -0
+        // SAFETY: `fence` is valid because `self` is valid. `flag_ptr` is
+        // merely a pointer to an integer, whose lifetime is tied to the guard
+        // object.
+        unsafe { bindings::dma_fence_lock_irqsave(guard.inner, &raw mut guard.flags) };
+
+        guard
+    }
+
+    /// Get the fence's sequence number.
+    pub fn seqno(&self) -> u64 {

inline?

Quoted 6 lines
Quoted 6 lines

+        // SAFETY: Valid because self is valid. +        unsafe { (*self.as_raw()).seqno } +    }
+
+    fn as_raw(&self) -> *mut bindings::dma_fence { +        self.inner.get()

Quoted 8 lines
Quoted 8 lines

+    }
+
+    /// 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 {

inline?

Quoted 4 lines
Quoted 4 lines

+        // SAFETY: Safe as per the function's overall safety requirements. +        unsafe { &*ptr.cast() }
+    }
+}

In C, we spent literally decades to get it out of people's heads to use the inline keyword, because "the compiler knows better than you". It should only be used for code in headers (and I even think that there should not be code in headers).

What is the Rust equivalent?

Quoted 2 lines
Quoted 2 lines

+

[…]

Quoted 6 lines
Quoted 3 lines

+        // SAFETY: fence is valid because self is valid. The lock must be +        // held, which we acquired directly above.
+        if !unsafe { bindings::dma_fence_test_signaled_flag(*fence.deref()) } {

These *fence.deref() are quite weird as consequence of FenceGuard design. If FenceGuard just derefs to &Fence then this can be fence.as_raw().

True; it's good to fix this.

Quoted 15 lines
Quoted 14 lines

+            if let Err(err) = res {
+                // SAFETY: fence is valid because self is valid. The fence +                // must not have been signaled yet, which we check directly above. +                unsafe { bindings::dma_fence_set_error(*fence.deref(), err.to_errno()) }; +            }
+            // SAFETY: fence is valid because self is valid. The lock must +            // be held, which we acquired above.
+            unsafe { bindings::dma_fence_signal_locked(*fence.deref()) }; +        }
+
+        // SAFETY: self.data is valid because self is valid.
+        let fctx = unsafe { self.data.as_ref().fctx };
+        let _ = fctx.nr_of_unsignaled_fences.fetch_sub(1, Relaxed);

Quoted 2 lines

Drop impl of self here will neededlessly take lock again before checking it's signaled already and unlock.

Yes, I know.
So? What could we do about it?

We need to take the lock when signaling, and we need to take it when dropping to check if everyone was kind and did indeed signal before.

What you describe is a lock-relock on the same CPU, and it is unlikely that there are contenders (only contender right now could be Fence::is_signaled(), which is nothing upcoming users will poll intensively).

tl;dr I don't see a problem with that

Quoted 4 lines
Quoted 4 lines

+    }
+}

[…]

Quoted 9 lines
Quoted 4 lines

+        // SAFETY: call_rcu() is always safe to be called. rcu_head_ptr was +        // created validly above. The module must perform a synchronize_rcu() +        // or rcu_barrier() call to guard against module unload.
+        unsafe { bindings::call_rcu(rcu_head_ptr, Some(drop_driver_fence_data::<T>)) };

I thought at some point it was mentioned that we want a fast path

    if !mem::needs_drop::<...>() {
    }

True. Added it.

Quoted 6 lines

?

Review

Quoted 3 lines

+    }

(quoted diff) +2 -0
+}
+

Regarding all your nits and comments I skipped above, I agree with them for the most part and will address them in v10.

P.

author maintainer
#13 lore raw

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

In reply to Philipp Stanner

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

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

---
 MAINTAINERS | 5 +++++
 1 file changed, 5 insertions(+)
MAINTAINERS +5 -0
index 721ecafe4b2a..e00e8ea312dc 100644
@@ -7622,6 +7622,7 @@ F:	fs/dlm/
 DMA BUFFER SHARING FRAMEWORK
 M:	Sumit Semwal <[email protected]>
 M:	Christian König <[email protected]>
+M:	Philipp Stanner <[email protected]>
 L:	[email protected]
 L:	[email protected]
 L:	[email protected] (moderated for non-subscribers)
@@ -7635,6 +7636,8 @@ F:	include/linux/dma-buf.h
 F:	include/linux/dma-buf/
 F:	include/linux/dma-resv.h
 F:	rust/helpers/dma-resv.c
+F:	rust/helpers/dma_fence.c
+F:	rust/kernel/dma_buf/
 K:	\bdma_(?:buf|fence|resv)\b
 
 DMA GENERIC OFFLOAD ENGINE SUBSYSTEM
@@ -8627,7 +8630,9 @@ T:	git https://gitlab.freedesktop.org/drm/rust/kernel.git
 F:	drivers/gpu/drm/nova/
 F:	drivers/gpu/drm/tyr/
 F:	drivers/gpu/nova-core/
+F:	rust/helpers/dma_fence.c
 F:	rust/helpers/gpu.c
+F:	rust/kernel/dma_buf/
 F:	rust/kernel/drm/
 F:	rust/kernel/gpu.rs
 F:	rust/kernel/gpu/
Signature · 1 line
2.55.0
maintainer Acked-by on the v9 series
#14 lore raw
In reply to Philipp Stanner
Quoted 2 lines · Philipp Stanner

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

Nit: You also need to add your SoB to this patch; you can just reply with your tag.

Quoted 4 lines

Philipp Stanner (3):
rust: sync: Add abstraction for rcu_barrier()
rust: Add dma_fence abstractions
MAINTAINERS: Add entry for Rust dma-buf

Good stuff! :)

Quoted 2 lines

Timur Tabi (1):
rust: error: add remaining error codes

Also needs your SoB.

For the series,

#15 lore raw
In reply to Philipp Stanner
Quoted 4 lines · Philipp Stanner

rust: types: implement ForeignOwnable for ARef<T>
rust: sync: Add abstraction for rcu_barrier()
rust: error: add remaining error codes

Applied (these three) to rust-next -- thanks everyone!

Please note that your Signed-off-by is required when sending a patch from someone else:

https://docs.kernel.org/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

I added it here on apply for you -- please let me know if that is not what you intended!

[ Formatted comments. Added the submitter's Signed-off-by tag. - Miguel ]

[ Relaxed 'static bound and added #[inline] as discussed. Added
the submitter's Signed-off-by tag. - Miguel ]

[ Formatted documentation. Sorted tags. - Miguel ]

Thanks!

Cheers,
Miguel

#16 lore raw
Quoted 8 lines · Miguel Ojeda
Quoted 4 lines · Philipp Stanner

  rust: types: implement ForeignOwnable for ARef<T>
  rust: sync: Add abstraction for rcu_barrier()
  rust: error: add remaining error codes

Applied (these three) to rust-next -- thanks everyone!

Quoted 7 lines

Please note that your Signed-off-by is required when sending a patch from someone else:

  https://docs.kernel.org/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

I added it here on apply for you -- please let me know if that is not what you intended!

No, fine by me, thank you for fixing it

P.

Quoted 12 lines

    [ Formatted comments. Added the submitter's Signed-off-by tag. - Miguel ]

    [ Relaxed 'static bound and added #[inline] as discussed. Added the submitter's Signed-off-by tag. - Miguel ]

    [ Formatted documentation. Sorted tags. - Miguel ]

Thanks!

Cheers,
Miguel