rust / dma_buf: Add abstractions for dma_fence
Subsystems (6)
Philipp Stanner
maintainerv7 cover rust / dma_buf: Add abstractions for dma_fence
I had chats with the XArray maintainers and concluded that it's not the right data structure for us. So the most notable change is that I for now abstain from using XArray for the fctx's fence list (the one we need to guard against UAF against core::mem::forget). Reasons are detailed in the code comments, especially at DriverFence::remove_from_fctx_list().
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 | 1111 ++++++++++++++++++++++++++++++ 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, 1346 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: 71d4e7233f235871b13553e504e591ace6b54373
Signature · 1 line
2.55.0
Philipp Stanner
maintainerPATCH v7 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
@@ -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
Philipp Stanner
maintainerPATCH v7 2/5 rust: types: implement ForeignOwnable for ARef<T>
In reply to Philipp StannerFrom: 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
@@ -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> = &'aT ; +type BorrowedMut <'a> = &'aT ; + +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 ) -> &'aT { +// 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 ) -> &'aT { +// 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
[email protected]
AI reviewRe: [PATCH v7 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 functions in an abstraction. The `into_foreign`…
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Missing #[inline] annotations on small functions in an abstraction. The into_foreign, from_foreign, borrow, and borrow_mut methods in the ForeignOwnable trait implementation for ARef<T> are small, forwarding-style wrappers but lack the required #[inline] attribute. --
--- Patch [2]: [PATCH v7 2/5] rust: types: implement ForeignOwnable for ARef<T> --- commit 8f27d3e39cb5663e64788b9670c36ec543c46826
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 yields a shared reference.
Quoted 4 lines
rust/kernel/sync/aref.rs +0
-0
[ ... ]
rust/kernel/sync/aref.rs fn eq(&self, other: &ARef<U>) -> bool { · line 204
Quoted 9 lines
rust/kernel/sync/aref.rs +9
-0+// 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> = &'aT ; +type BorrowedMut <'a> = &'aT ; + +fn into_foreign (self) -> *mut c_void {
[Severity: Low]
This isn't a bug, but should the small methods implemented here (into_foreign, from_foreign, borrow, and borrow_mut) include #[inline] annotations?
The Rust subsystem guidelines note that small functions or forwarding methods in abstractions need to be annotated with #[inline] to allow optimal inlining across crate boundaries.
Quoted 25 lines
rust/kernel/sync/aref.rs +25
-0+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 ) -> &'aT { +// 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 ) -> &'aT { +// SAFETY: The safety requirements for `borrow_mut` are a superset of the safety +// requirements for `borrow`. +unsafe { <Self as ForeignOwnable >::borrow (ptr) } + } +}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
Philipp Stanner
maintainerPATCH v7 3/5 rust: sync: Add abstraction for rcu_barrier()
In reply to Philipp Stannerrcu_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
@@ -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
Paul E. McKenney
maintainerRe: [PATCH v7 3/5] rust: sync: Add abstraction for rcu_barrier()
Quoted 2 lines · Philipp Stanner
rcu_barrier()is a frequently used C function which is always safe to be called.
Just checking... Here "always safe" means only from task level, with BH, preemption, and interrupts all enabled, correct?
In contrast, if you do this:
preempt_disable(); rcu_barrier(); preempt_enable();
the results won't be safe.
Thanx, Paul
Quoted 39 lines
Add a safe abstraction for
rcu_barrier().Signed-off-by Philipp Stanner <[email protected]>Tested-by Daniel Almeida <[email protected]>--- rust/kernel/sync/rcu.rs | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)rust/kernel/sync/rcu.rs +20
-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. +/// +/// 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
Gary Guo
maintainerRe: [PATCH v7 3/5] rust: sync: Add abstraction for rcu_barrier()
Quoted 7 lines · Paul E. McKenney
Quoted 2 lines · Philipp Stanner
rcu_barrier()is a frequently used C function which is always safe to be called.Just checking... Here "always safe" means only from task level, with BH, preemption, and interrupts all enabled, correct?
Quoted 7 lines
In contrast, if you do this:
preempt_disable(); rcu_barrier(); preempt_enable();the results won't be safe.
This is same for all sleepable functions. In order to avoid having to mark all sleeping function unsafe, we've decided that sleeping from non-preemptable context is "safe", but is a bug regardless.
Best,
Gary
Paul E. McKenney
maintainerRe: [PATCH v7 3/5] rust: sync: Add abstraction for rcu_barrier()
Quoted 14 lines · Gary Guo
Quoted 13 lines · Paul E. McKenney
Quoted 2 lines · Philipp Stanner
rcu_barrier()is a frequently used C function which is always safe to be called.Just checking... Here "always safe" means only from task level, with BH, preemption, and interrupts all enabled, correct?
In contrast, if you do this:
preempt_disable(); rcu_barrier(); preempt_enable();
Quoted 5 lines
Quoted 1 line
the results won't be safe.
This is same for all sleepable functions. In order to avoid having to mark all sleeping function unsafe, we've decided that sleeping from non-preemptable context is "safe", but is a bug regardless.
Got it, thank you!
I tried to resist parameterizing "safe" over things like execution contexts, but obviously failed to do so. Besides, there is a name for that sort of thing, and that name is "precondition". ;-)
Adding Julia Lawall on CC based on her recent work with preconditions, which might be an act of kindness or of vandalism. You guys get to decide. ;-)
Thanx, Paul
Philipp Stanner
maintainerPATCH v7 4/5 rust: Add dma_fence abstractions
In reply to Philipp StannerC'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. Since this can, currently, not be fully guaranteed (core::mem::forget() could make the compiler forget about the memory) in Rust, an additional safety measure is implemented: the FenceContext keeps track of all unsignaled fences, and should it ever drop with such a fence present, it will signal it, ensuring full decoupling.
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 from circumventing the DriverFence's lifetime on the FenceContext by "forgetting" the fence, e.g. with core::mem::forget(). Since this apparently can also happen with recfounting cycles, it's quite likely that it would enable UAF bugs on the FenceContext. To make this absolutely water proof, the FenceContext keeps track of unsignaled fences and signals those should the context drop before some fences.
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 | 1111 ++++++++++++++++++++++++++++++ rust/kernel/dma_buf/mod.rs | 14 + rust/kernel/lib.rs | 1 + 6 files changed, 1176 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
@@ -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
@@ -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 +1111 -0
1,111 lines not on this page · raw message
rust/kernel/dma_buf/mod.rs new file +14 -0
@@ -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
@@ -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
[email protected]
AI reviewRe: [PATCH v7 4/5] rust: Add dma_fence abstractions
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [Critical] Infinite loop under spinlock in `remove_from_fctx_list`. - [Critical] IRQ-context Deadlock due…
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Infinite loop under spinlock in remove_from_fctx_list.
- [Critical] IRQ-context Deadlock due to using non-IRQ-safe SpinLock for fctx.fences.
- [High] Use-After-Free in dma_fence_ops callbacks (get_driver_name / get_timeline_name).
- [High] Memory leak of DriverFenceData and list nodes when implicitly dropped.
- [Low] Compilation Error (use-after-move) in FenceCallbackRegistration::new. --
--- Patch [4]: [PATCH v7 4/5] rust: Add dma_fence abstractions --- commit 6943f462ac2321e63a53d88290790522a943cbc4
Author: Philipp Stanner <[email protected]>
rust: Add dma_fence abstractions
This commit introduces Rust abstractions for the C dma_fence framework. It provides DriverFence and Fence types to decouple fence issuers and consumers, along with a FenceContext for synchronization.
Quoted 4 lines
rust/kernel/dma_buf/dma_fence.rs new file +0
-0
[ ... ]
Quoted 3 lines
rust/kernel/dma_buf/dma_fence.rs new file +3
-0+pub struct FenceContext <T :FenceContextOps +Send +Sync > { +/// The fence context number. + nr:u64 ,
[ ... ]
rust/kernel/dma_buf/dma_fence.rs
Quoted 2 lines
rust/kernel/dma_buf/dma_fence.rs new file +2
-0+#[pin] + fences: SpinLock<List<FenceLastResortSignaler>>,
[Severity: Critical]
Can this lead to an IRQ-context deadlock?
If a process-context thread acquires the lock during DriverFence creation, and a hardware interrupt fires on the same CPU before the lock is released:
Process context:fctx.fence_alloc()
new_fence()
fctx.fences.lock()IRQ context on same CPU:
GPU job completes
DriverFence::signal()
remove_from_fctx_list()
fctx.fences.lock()
It seems the interrupt handler will attempt to acquire the already-held lock. Since SpinLock resolves to spin_lock() which leaves interrupts enabled, would this cause the CPU to deadlock?
[ ... ]
rust/kernel/dma_buf/dma_fence.rs line 215
Quoted 10 lines
rust/kernel/dma_buf/dma_fence.rs new file +10
-0+extern "C" fn get_driver_name (ptr: *mut bindings::dma_fence ) -> *const c_char { +// SAFETY: The C backend only invokes this callback with `ptr` pointing +// to a valid, unsignaled `bindings::dma_fence`. All fences created in +// this module always reside within `Fence` which always resides in a +// `DriverFenceData`, thus satisfying the function's safety +// requirements. +let fctx =unsafe {Self ::from_raw_fence (ptr) }; + + fctx.driver_name.as_char_ptr () + }
[Severity: High]
Could this result in a use-after-free?
If the device is removed and the FenceContext is dropped, userspace could still hold a reference to the fence via a sync_file file descriptor. If userspace then calls the SYNC_IOC_FILE_INFO ioctl, the C backend would unconditionally invoke get_driver_name.
It appears fctx would point to freed memory, causing a use-after-free when accessing driver_name.
[ ... ]
rust/kernel/dma_buf/dma_fence.rs line 226
Quoted 10 lines
rust/kernel/dma_buf/dma_fence.rs new file +10
-0+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 () + }
[Severity: High]
Does this have the same use-after-free issue as get_driver_name above?
[ ... ]
Quoted 11 lines
rust/kernel/dma_buf/dma_fence.rs new file +11
-0+ 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| {
[ ... ]
rust/kernel/dma_buf/dma_fence.rs line 405
Quoted 7 lines
rust/kernel/dma_buf/dma_fence.rs new file +7
-0+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) };
[Severity: Low]
Does this compile successfully?
It looks like the callback parameter is moved into ManuallyDrop::new(), and then later accessed inside the try_ffi_init closure. This appears to violate ownership rules with a use-after-move. Additionally, wouldn't ManuallyDrop::take() expect a mutable reference rather than the underlying type?
[ ... ]
rust/kernel/dma_buf/dma_fence.rs line 896
Quoted 12 lines
rust/kernel/dma_buf/dma_fence.rs new file +12
-0+fn remove_from_fctx_list (&self) { +// SAFETY: `self.data` is valid because `self` was validly initialized. +let mut guard =unsafe { self.data.as_ref ().fctx.fences.lock () }; +let mut cursor = guard.cursor_front (); + +while let Some (pos) = cursor.peek_next () { +// Fences in this list are always initialized, so `unwrap` is OK. +if self.as_fence ().get_seqno () == pos.fence.as_ref ().unwrap ().get_seqno () { +let _ = pos.remove (); + } + } + }
[Severity: Critical]
Can this cause an infinite loop?
If the current element's seqno does not match the signaled fence, the code takes no action and fails to advance the cursor. The next iteration would peek at the exact same element indefinitely, leading to a lockup under the spinlock.
Should there be a cursor.move_next() call when the sequence number does not match?
[ ... ]
Quoted 22 lines
rust/kernel/dma_buf/dma_fence.rs new file +22
-0+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 warn_on! (!signaled) { +// 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 ()) }; + } +drop (guard); + +// SAFETY: Valid because `self` is valid. +let rcu_head_ptr =unsafe { &raw mut (*self.data.as_ptr ()).rcu_head };
[ ... ]
rust/kernel/dma_buf/dma_fence.rs line 1059
Quoted 3 lines
rust/kernel/dma_buf/dma_fence.rs new file +3
-0+unsafe { bindings::call_rcu (rcu_head_ptr,Some (drop_driver_fence_data ::<T >)) }; + } +}
[Severity: High]
Does implicitly dropping a DriverFence leak the tracking nodes?
The drop implementation orchestrates the destruction and signals the fence, but it seems it does not call self.remove_from_fctx_list().
If the driver implicitly drops the DriverFence on an error path without explicitly calling signal(), the tracking node would remain in the fctx.fences list indefinitely. This holds an ARef<Fence> that keeps the C dma_fence refcount elevated, preventing the memory from being freed.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
Philipp Stanner
maintainerPATCH v7 5/5 MAINTAINERS: Add entry for Rust dma-buf
In reply to Philipp StannerRust 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
@@ -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
@@ -8621,7 +8624,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