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. > diff --git a/rust/kernel/dma_buf/dma_fence.rs > b/rust/kernel/dma_buf/dma_fence.rs > new file mode 100644 > --- /dev/null > +++ b/rust/kernel/dma_buf/dma_fence.rs [ ... ] > +pub struct FenceContext<T: FenceContextOps + Send + Sync> { > + /// The fence context number. > + nr: u64, [ ... ] > + #[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? [ ... ] > + 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. [ ... ] > + 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? [ ... ] > + 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| { [ ... ] > + 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? [ ... ] > + 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? [ ... ] > +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 > }; [ ... ] > + 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. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
