rust / dma_buf: Add abstractions for dma_fence
Subsystems (4)
Philipp Stanner
maintainerv11 cover rust / dma_buf: Add abstractions for dma_fence
Based on drm-rust-next.
Changes since v10:
- Rebase on drm-rust-next, solve small conflict.
- Improve some comments.
- More spellchecking. (Danilo)
- Use reference instead of rawpointer for FenceOps. (Danilo)
- Take a &CStr as argument for FenceContext. (Danilo)
- Remove unnecessary (?) unsafe comment. (Danilo)
- Improve formatting / style in doctest examples. (Danilo)
Changes since v9:
- Rebase on rust-next, since Miguel has merged our prerequisites.
- Add R-b from Daniel.
All from Gary:
- Use an Atomic<usize> for nr_of_unsignaled_fences.
- Comment improvement / formatting.
- *const instead of *mut raw pointer.
- Make small public functions inline.
- Rename "called" callback to "on_signal".
- Improve the internal helper lock FenceGuard.
- Add needs_drop fastpath check in DriverFence::drop().
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]/
Philipp Stanner (2):
rust: Add dma_fence abstractions
MAINTAINERS: Add entry for Rust dma-buf
MAINTAINERS | 5 + rust/bindings/bindings_helper.h | 1 + rust/helpers/dma_fence.c | 49 ++ rust/helpers/helpers.c | 1 + rust/kernel/dma_buf/dma_fence.rs | 1023 ++++++++++++++++++++++++++++++ rust/kernel/dma_buf/mod.rs | 14 + rust/kernel/lib.rs | 1 + 7 files changed, 1094 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: 6cb331644c441ff4101a4f8726283a6ed0d5947e
Signature · 1 line
2.55.0
Philipp Stanner
maintainerPATCH v11 1/2 rust: Add dma_fence abstractions
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 | 49 ++ rust/helpers/helpers.c | 1 + rust/kernel/dma_buf/dma_fence.rs | 1023 ++++++++++++++++++++++++++++++ rust/kernel/dma_buf/mod.rs | 14 + rust/kernel/lib.rs | 1 + 6 files changed, 1089 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 +49 -0
49 lines not on this page · raw message
rust/helpers/helpers.c +1 -0
@@ -57,6 +57,7 @@#include "cred.c" #include "device.c" #include "dma.c" +#include "dma_fence.c" #ifdef CONFIG_DMA_SHARED_BUFFER #include "dma-resv.c" #endif
rust/kernel/dma_buf/dma_fence.rs new file +1023 -0
1,023 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 v11 1/2] rust: Add dma_fence abstractions
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] The consumer `Fence` can outlive its `FenceContext`, leading to a use-after-free when C callbacks…
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The consumer Fence can outlive its FenceContext, leading to a use-after-free when C callbacks access the context.
- [High] Unconditional spinlock acquisition in Fence::is_signaled() causes a deadlock if invoked from within a fence callback.
- [High] FenceContext's PinnedDrop implementation calls rcu_barrier(), which blocks and sleeps, making it unsafe to drop in atomic context.
- [Low] Missing #[inline] annotation on small abstraction functions forwarding to C bindings.
--
commit fb115447c7bec2def2c1691d01dc640dec6f4092
Author: Philipp Stanner <[email protected]>
rust: Add dma_fence abstractions
C's dma_fence's are synchronisation primitives that will be needed by all Rust GPU drivers.
Quoted 4 lines
rust/kernel/dma_buf/dma_fence.rs new file +0
-0
[ ... ]
Quoted 11 lines
rust/kernel/dma_buf/dma_fence.rs new file +11
-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 () + } +
rust/kernel/dma_buf/dma_fence.rs line 189
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]
Could this lead to a use-after-free if the consumer Fence outlives its FenceContext? The Fence object (representing the consumer side of dma_fence) is refcounted and has no Rust lifetime constraint tying it to FenceContext. It can be shared with C subsystems or exported to userspace. If the driver drops DriverFence and subsequently drops FenceContext, the memory for FenceContext is freed. If an external consumer then queries the fence, the callback casts the fence pointer and dereferences the now-dangling fctx reference to read the name.
[ ... ]
Quoted 19 lines
rust/kernel/dma_buf/dma_fence.rs new file +19
-0+#[pinned_drop] +impl <T :FenceContextOps +Send +Sync >PinnedDrop for FenceContext <T > { +fn drop (self:Pin <&mut Self >) { +// Fence ops callbacks can be called on unsignaled fences. Since these +// callbacks can access the fence context and its data, it needs to be +// guaranteed that a context only drops after all associated +// `DriverFence`s have been dropped. This is unlikely to occur, but +// would result in silent UAF. Throw a panic to prevent that. +// +// 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's 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." ); + } + +// Ensure that the driver cannot unload while there are still dma_fence
rust/kernel/dma_buf/dma_fence.rs line 241
Quoted 10 lines
rust/kernel/dma_buf/dma_fence.rs new file +10
-0+// callbacks running. At the same time, the RCU barrier addresses the +// problem inherited by the C backend, in which backend ops callbacks +// might be accessing the fence while it is being signaled (or shortly +// after). This could cause UAF access on the fence context's +// `fctx.driver_name` and `fctx.timeline_name`. +// +// Wait for the RCU callbacks in `DriverFence::drop`. +rcu_barrier (); + } +}
[Severity: High]
Is it safe to call rcu_barrier() during destruction? rcu_barrier() waits for all pending RCU callbacks, which involves blocking and sleeping. If a driver stores the FenceContext in a reference-counted container and the last reference is dropped from an atomic context (such as an interrupt handler or while holding a spinlock), wouldn't this sleep in atomic context cause a kernel panic?
[ ... ]
Quoted 17 lines
rust/kernel/dma_buf/dma_fence.rs new file +17
-0+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 { +let reg: *mut Self =container_of! (ptr,Self , callback_foreign); + + (*reg).callback.on_signal (); + } + }
[ ... ]
Quoted 25 lines
rust/kernel/dma_buf/dma_fence.rs new file +25
-0+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. +#[inline] +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 signaling 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 +// 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.
rust/kernel/dma_buf/dma_fence.rs line 527
Quoted 10 lines
rust/kernel/dma_buf/dma_fence.rs new file +10
-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 + }
[Severity: High]
Does this unconditionally acquire the spinlock even though it might already be held? When a fence is signaled, the C function dma_fence_signal_locked() iterates over and executes all registered callbacks while actively holding the fence's spinlock. If the user callback attempts to check the fence's status by calling Fence::is_signaled() from within on_signal(), it attempts to acquire the exact same non-recursive spinlock, which would cause a deadlock.
[ ... ]
rust/kernel/dma_buf/dma_fence.rs line 553
Quoted 3 lines
rust/kernel/dma_buf/dma_fence.rs new file +3
-0+fn as_raw (&self) -> *mut bindings::dma_fence { + self.inner.get () + }
[Severity: Low]
Should this small getter function be annotated with #[inline] according to the Rust kernel coding guidelines?
[ ... ]
rust/kernel/dma_buf/dma_fence.rs line 573
Quoted 7 lines
rust/kernel/dma_buf/dma_fence.rs new file +7
-0+// 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 ()) } + }
[Severity: Low]
Should this small function forwarding to C bindings be annotated with #[inline] according to the Rust kernel coding guidelines?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Danilo Krummrich
maintainerRe: [PATCH v11 1/2] rust: Add dma_fence abstractions
In reply to Philipp Stannerrust/kernel/dma_buf/dma_fence.rs line 938
Quoted 12 lines · Philipp Stanner
rust/kernel/dma_buf/dma_fence.rs new file +12
-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.as_raw ()) }; +if !signaled { +pr_err! ("DriverFence drops unsignaled. Danger of memory corruption!\n" );
I'm not sure we want to keep this as pr_err!().
If we really want to keep warning about this I'd either make this a WARN_ON() or dev_warn() (we can easily store a device reference in the fence context), such that it is at least clear who's the offender.
My preference would be dev_warn(), as I don't think it is that bad of an error condition to begin with. It would be pretty odd to have a driver where a DriverFence drops while the corresponding GPU job is not dropped. And further it'd be pretty odd if dropping the GPU job would not imply that the GPU actually stopped processing the work associated with the job.
For the same reason I also think it is a bit misleading to say "Danger of memory corruption!". It's not the signaling of the fence that does prevent memory corruption; it's the driver implementing a proper teardown sequence. And if this sequence is structurally detached from the lifetime of the DriverFence (and Job) structure, something is structurally wrong with the driver anyway.
Furthermore, it would be very natural to just require the generic Job type to own a DriverFence. In this case it becomes natural to either signal the fence on Job completion, or just drop the Jobqueue, which does the ring teardown and subsequently drops all the Jobs, which would also imply signaling the DriverFence with ECANCELED. I.e. I think the fact that the DriverFence is signaled with ECANCELED if it is still unsignaled should just be an API contract and not an error condition.
Honestly, given all that, I'd expect drivers to otherwise just wrap a DriverFence in a new type, which just signales the inner DriverFence with an error code in its own destructor.
Re: [PATCH v11 1/2] rust: Add dma_fence abstractions
Quoted 20 lines · Danilo Krummrich
rust/kernel/dma_buf/dma_fence.rs line 938
Quoted 12 lines · Philipp Stanner
rust/kernel/dma_buf/dma_fence.rs new file +12
-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.as_raw()) }; + if !signaled { + pr_err!("DriverFence drops unsignaled. Danger of memory corruption!\n");I'm not sure we want to keep this as pr_err!().
If we really want to keep warning about this I'd either make this a
WARN_ON()ordev_warn()(we can easily store a device reference in the fence context), such that it is at least clear who's the offender.
Quoted 5 lines
My preference would be
dev_warn(), as I don't think it is that bad of an error condition to begin with. It would be pretty odd to have a driver where a DriverFence drops while the corresponding GPU job is not dropped. And further it'd be pretty odd if dropping the GPU job would not imply that the GPU actually stopped processing the work associated with the job.
Yeah, it would be odd. I would not expect it to happen. But many things have happened which I did not expect, so … :)
Quoted 6 lines
For the same reason I also think it is a bit misleading to say "Danger of memory corruption!". It's not the signaling of the fence that does prevent memory corruption; it's the driver implementing a proper teardown sequence. And if this sequence is structurally detached from the lifetime of the DriverFence (and Job) structure, something is structurally wrong with the driver anyway.
Hypothetically, it could mean that there is a forgotten job still running on the GPU, which might then access freed resources through DMA. Sure, that would mean that the driver is fundamentally broken, but that's what warnings like that are about.
Just to ellaborate on my thinking. I wouldn't insist on any wording. From my POV we can just say "… drops unsignaled.". That should be enough.
Quoted 8 lines
Furthermore, it would be very natural to just require the generic Job type to own a DriverFence. In this case it becomes natural to either signal the fence on Job completion, or just drop the Jobqueue, which does the ring teardown and subsequently drops all the Jobs, which would also imply signaling the DriverFence with ECANCELED. I.e. I think the fact that the DriverFence is signaled with ECANCELED if it is still unsignaled should just be an API contract and not an error condition.
Yup, with the JobQueue design I have in mind this error is largely impossible, just as the forgetting of DriverFences is. Which is why I think it's a good design idea to have the JQ own the jobs, isolating the driver from direct fence interaction.
P.
Philipp Stanner
maintainerPATCH v11 2/2 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
@@ -7680,6 +7680,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)
@@ -7693,6 +7694,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
@@ -8697,7 +8700,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
Quoted 1 line · Philipp Stanner
[PATCH v11 0/2] rust /
dma_buf: Add abstractions fordma_fence
Applied, thanks!
Branch: drm-rust-next
Tree: https://gitlab.freedesktop.org/drm/rust/kernel.git
[1/2] rust: Add dma_fence abstractions
commit: 3282d5916019
[ In dma_fence_callback(), split combined unsafe block into separate
blocks for container_of!() and pointer dereference. - Danilo ]
[2/2] MAINTAINERS: Add entry for Rust dma-buf
commit: 7aa432f5121b
The patches will appear in the next linux-next integration (typically within 24 hours on weekdays).
The patches are queued up for the upcoming merge window for the next major kernel release.