Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [High] Soundness bug in `ForeignOwnable` allows safe lifetime erasure, leading to UAF of `FenceContext`. - [High] `Fence` objects safely outlive their `FenceContext`, leading to UAF in C callbacks. - [High] Unconditional system-wide `rcu_barrier()` on `FenceContext` drop causes severe latency spikes. - [High] Calling `Fence::is_signaled()` from a fence callback causes a hard deadlock. - [Medium] `FenceContext::drop` does not signal leaked fences, causing permanent deadlocks for consumers. - [Medium] `nr_of_unsignaled_fences` counter leaks if a fence is signaled via the C API. --
--- Patch [4]: [PATCH v8 4/5] rust: Add dma_fence abstractions --- commit baeb68fb999484f7d372bacbf53bbc08faf26f2e Author: Philipp Stanner <[email protected]> rust: Add dma_fence abstractions This commit adds abstractions for dma_fence in Rust. It provides DriverFence and Fence types to cleanly decouple fence issuers and consumers, along with a FenceContext to handle sequencing. > diff --git a/rust/kernel/dma_buf/dma_fence.rs > b/rust/kernel/dma_buf/dma_fence.rs > new file mode 100644 > index 0000000000000..d6322e062faff > --- /dev/null > +++ b/rust/kernel/dma_buf/dma_fence.rs [ ... ] > + 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] Can Fence objects safely outlive their FenceContext, leading to a use-after-free here? It looks like Fence implements AlwaysRefCounted but lacks a lifetime parameter connecting it to its FenceContext. If a user drops the DriverFence and then drops the FenceContext while the ARef to the Fence remains alive, callbacks like get_driver_name could access a dangling fctx pointer. Could this happen if a consumer accesses the exported fence, for example through debugfs reading the fence driver name or through SYNC_IOC_FILE_INFO, triggering the C backend to call this function on a signaled fence? [ ... ] > +#[pinned_drop] > +impl<T: FenceContextOps + Send + Sync> PinnedDrop for FenceContext<T> { > + fn drop(self: Pin<&mut Self>) { > + if self.nr_of_unsignaled_fences.load(Relaxed) > 0 { > + pr_err!("Unsignaled DriverFence(s) left in FenceContext. UAF > possible.\n"); > + } > + > + // TODO: > + // It would be even more robust 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::fence_alloc()`). [Severity: Medium] Will this cause waiting consumers to hang indefinitely? The commit message notes that as a safety measure, if a FenceContext drops with an unsignaled fence present, it will signal it to ensure full decoupling. However, this code leaves a TODO and only prints a warning instead of actually signaling the leaked fences. Does this mean that if a fence is forgotten without being signaled, any kernel threads waiting on it will enter a permanent deadlock? > + > + rcu_barrier(); > + } > +} [Severity: High] Could calling rcu_barrier() unconditionally here cause severe latency spikes? Since rcu_barrier() blocks the current thread until all pending call_rcu() callbacks across the entire kernel have completed, using it for regular object destruction seems problematic. If GPU drivers frequently create and destroy fence contexts dynamically (such as per-process or per-timeline), could destroying a context become a denial of service vector by repeatedly blocking kernel threads? [ ... ] > + 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: `fence` stems from `self`, which is valid by definition. > + 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 > + // 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. > + > + let _ = self.lock(); > + > + ret > + } [Severity: High] Does acquiring the lock here cause a hard deadlock if called from a callback? The is_signaled method intentionally acquires the fence spinlock to guarantee signaling completion. However, the C backend executes callbacks registered via FenceCallbackRegistration with this exact same spinlock already held. If a user calls fence.is_signaled() from within their callback implementation, would it attempt to recursively acquire the non-recursive Linux spinlock? [ ... ] > +unsafe impl<T: Send + Sync + FenceContextOps + 'static> ForeignOwnable for > DriverFence<'_, T> { > + 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() > + } [Severity: High] Does this implementation allow safe lifetime erasure of DriverFence? The ForeignOwnable trait is implemented for DriverFence regardless of its lifetime. Because into_foreign is a safe method that returns a raw pointer, it completely erases the lifetime parameter of DriverFence. Could safe Rust code call fence.into_foreign(), successfully ending the borrow of FenceContext from the compiler's perspective without dropping the fence? This might allow the FenceContext to be safely dropped, invalidating the fctx pointer and leading to a use-after-free when the kernel later accesses the exported fence. [ ... ] > +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); [Severity: Medium] Can the nr_of_unsignaled_fences counter leak if the fence is signaled externally? The logic here only decrements the counter if dma_fence_test_signaled_flag returns false. If another subsystem signals the fence through the C API, the signaled flag is set. When this DriverFence is subsequently dropped, it appears it would skip the decrement, permanently leaking the counter. Would this cause false-positive memory corruption warnings later when the FenceContext is dropped? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
