Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Critical] Use-After-Free of FenceContext when a Fence outlives its DriverFence or when `!needs_drop` bypasses RCU synchronization. - [High] FenceContext will reliably trigger a kernel panic on drop if a DriverFence is signaled by external C code. - [High] Fence::is_signaled() unconditionally acquires the fence spinlock, leading to spinlock recursion deadlocks if called from a callback. --
--- Patch [1]: [PATCH v10 1/2] rust: Add dma_fence abstractions --- commit 4a2030e75f6de53abe7df28e7314d4e3c4ae44a8 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. > 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 [ ... ] > + 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: Critical] Is it possible for the FenceContext to be accessed after it is freed here? The C dma_fence API allows callbacks like get_driver_name() to be invoked at any time as long as the fence is alive (refcount > 0), even on signaled fences. Because the Rust abstraction explicitly allows a Fence to outlive its FenceContext, fctx can become a dangling pointer. A userspace application calling SYNC_IOC_FILE_INFO or accessing debugfs could trigger this after the driver has unloaded and the FenceContext is dropped. [ ... ] > + 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 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] Can this spinlock acquisition cause a recursion deadlock? The C dma_fence_is_signaled() function is explicitly lockless. By acquiring the fence spinlock here, this wrapper introduces a blocking operation into what is supposed to be a lockless API. If a user registers a FenceCallback that invokes is_signaled() on the same fence from within on_signal(), it will attempt to acquire the lock. Since dma_fence_signal executes these callbacks with the spinlock already held, this will result in an immediate deadlock. [ ... ] > +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"); > + // 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.as_raw(), > 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.as_raw()) }; > + > + // 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: High] Will this skip the counter decrement if the fence is signaled by external C code? If a C subsystem (like the DRM scheduler or a software signaling debugfs hook) signals the DriverFence, the signaled variable will be true. If the driver subsequently drops the DriverFence without calling signal(), this block is skipped and the fetch_sub(1) decrement is permanently lost. When the FenceContext is later dropped, it will unconditionally panic because the counter is imbalanced: rust/kernel/dma_buf/dma_fence.rs:FenceContext::drop() { ... if self.nr_of_unsignaled_fences.load(Relaxed) != 0 { panic!("Forgotten fences in FenceContext."); } ... } > + > + // `DriverFenceData` but could be accessed through some dma_fence > + // callbacks right now. Access is being revoked in principle above by > + // signalling the fence, but since the C backend does not guarantee > + // perfect full synchronization, we have to wait for one grace > period to > + // ensure that all accessors of `DriverFenceData` (through the > + // dma_fence_ops accessible through a `Fence`) are gone. > + > + if !core::mem::needs_drop::<T::FenceDataType>() { > + // SAFETY: Once a `DriverFence` is initialized, the inner > `fence` is > + // valid and initialized. It is valid until the refcount drops > + // to 0, which can earliest happen once we drop the > `DriverFence`'s reference > + // here. > + unsafe { bindings::dma_fence_put(self.as_raw()) }; > + return; > + } [Severity: Critical] Does this optimization bypass the RCU synchronization required for safe teardown? If this block returns early, no RCU callback is queued. Consequently, when the FenceContext drops, its rcu_barrier() call will not wait for a grace period because there are no callbacks pending. This allows the FenceContext to be freed immediately while concurrent C readers holding RCU read locks might still be executing, leading to a use-after-free. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
