Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Lifetime erasure via `ForeignOwnable` or `core::mem::forget` on `DriverFence` leads to a Use-After-Free of `FenceCtx`. - [Medium] `DriverFence::signal` and `DriverFence::drop` omit the required `dma_fence_begin_signalling()` and `dma_fence_end_signalling()` lockdep annotations. - [Low] `FenceCbRegistration`'s `# Invariants` documentation is outdated and lacks the mandatory `// INVARIANT:` comment upon construction. --
commit d72bac0aa1194434de7bc7000b2d5a50bb882d68 Author: Philipp Stanner <[email protected]> rust: Add dma_fence abstractions This commit adds Rust abstractions for the dma_fence subsystem, providing DriverFence and Fence types to cleanly decouple fence issuers and consumers while ensuring safe reference counting and signaling. > diff --git a/rust/kernel/dma_buf/dma_fence.rs > b/rust/kernel/dma_buf/dma_fence.rs > --- /dev/null > +++ b/rust/kernel/dma_buf/dma_fence.rs [ ... ] > +/// # Invariants > +/// > +/// If callback is Some, then cb is registered with the fence and the > +/// callback hasn't been invoked yet. If None, the callback has been invoked > +/// or the fence was already signaled when we tried to register. > +#[pin_data(PinnedDrop)] > +pub struct FenceCbRegistration<T: FenceCb + 'static> { > + #[pin] > + callback_foreign: Opaque<bindings::dma_fence_cb>, > + callback: ManuallyDrop<T>, [Severity: Low] Is this documentation accurate? The invariants description references Some and None for the callback field, but the field is defined as ManuallyDrop<T> rather than an Option. [ ... ] > + 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), [Severity: Low] Are we missing the mandatory INVARIANT comment here? The Rust subsystem guidelines require an explicit INVARIANT comment when constructing a struct with an Invariants documentation section, but it appears to be omitted here. [ ... ] > + pub fn signal(self, res: Result) { > + let fence = self.as_raw(); > + let mut fence_flags: usize = 0; > + let flag_ptr = &raw mut fence_flags; > + > + // SAFETY: fence is valid because self is valid. flag_ptr is > + // merely a pointer to an integer, which lives as long as this > function. > + unsafe { bindings::dma_fence_lock_irqsave(fence, flag_ptr) }; > + // SAFETY: fence is valid because self is valid. The lock must be > + // held, which we acquired directly above. > + if !unsafe { bindings::dma_fence_test_signaled_flag(fence) } { > + if let Err(err) = res { > + // SAFETY: fence is valid because self is valid. The fence > + // must not have been signaled yet, which we check directly > above. > + unsafe { bindings::dma_fence_set_error(fence, > err.to_errno()) }; > + } > + // SAFETY: fence is valid because self is valid. The lock must > + // be held, which we acquired above. > + unsafe { bindings::dma_fence_signal_locked(fence) }; > + } > + // SAFETY: fence is valid because self is valid. flag_ptr is > + // merely a pointer to an integer, which lives as long as this > function. > + // The lock must be held, which we acquired above. > + unsafe { bindings::dma_fence_unlock_irqrestore(fence, flag_ptr) }; > + } [Severity: Medium] Should this manual signaling path include lockdep annotations? By open-coding the fence signaling using dma_fence_lock_irqsave() and dma_fence_signal_locked(), this function omits the dma_fence_begin_signalling() and dma_fence_end_signalling() calls used in the equivalent C dma_fence_signal() function. Without these annotations, lockdep cannot track cross-release dependencies during fence signaling, which might lead to false negatives when detecting deadlocks between fence signaling and wait paths. [ ... ] > +unsafe impl<'b, T: Send + Sync + FenceCtxOps + 'static> ForeignOwnable for > DriverFence<'b, T> { [Severity: High] Could this lead to a Use-After-Free if the DriverFence is leaked? Since DriverFence implements ForeignOwnable without requiring a 'static lifetime for the 'b parameter, a user can call into_foreign() on a DriverFence<'a, T> or use core::mem::forget() to leak it. If the DriverFence is leaked, DriverFence::drop() is never called. The Rust compiler will then allow the borrowed FenceCtx (tied to lifetime 'a in DriverFenceData) to be dropped. Once FenceCtx::drop() completes, the driver_name is freed. However, the C backend could later interact with the dma_fence, which is kept alive by ARef<Fence>, and invoke get_driver_name(). This would dereference the dangling fctx pointer, causing a Use-After-Free. [ ... ] > + fn drop(&mut self) { > + let fence = self.as_raw(); > + let mut fence_flags: usize = 0; > + let flag_ptr = &raw mut fence_flags; > + > + // SAFETY: fence is valid until the call_rcu() below. flag_ptr is > + // merely a pointer to an integer, which lives as long as this > function. > + unsafe { bindings::dma_fence_lock_irqsave(fence, flag_ptr) }; > + > + // Use dma_fence_test_signaled_flag() instead of > dma_fence_is_signaled_locked() > + // because the C backend wants to get rid of the latter. > + > + // SAFETY: fence is valid until the call_rcu() below. > + let signaled: bool = unsafe { > bindings::dma_fence_test_signaled_flag(fence) }; > + if warn_on!(!signaled) { > + // SAFETY: fence is valid until the call_rcu() below. The fence > + // must not have been signaled yet, which we check directly > above. > + unsafe { bindings::dma_fence_set_error(fence, > ECANCELED.to_errno()) }; > + // SAFETY: fence is valid until the call_rcu() below. The lock > must > + // be held, which we acquired above. > + unsafe { bindings::dma_fence_signal_locked(fence) }; > + } > + // SAFETY: fence is valid until the call_rcu() below. flag_ptr is > + // merely a pointer to an integer, which lives as long as this > function. > + // The lock must be held, which we acquired above. > + unsafe { bindings::dma_fence_unlock_irqrestore(fence, flag_ptr) }; [Severity: Medium] Does this cancellation path also need the lockdep annotations? Similar to DriverFence::signal(), this open-coded signaling omits the dma_fence_begin_signalling() and dma_fence_end_signalling() lockdep wrappers, which prevents cross-release dependency tracking. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
