Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] rust: dma_fence: premature unsignaled fence count decrement in signal() --
commit d89d19611f93fbf6fde4c15619608d55fb454178 Author: Philipp Stanner <[email protected]> rust: DmaFence: Don't drop on signal This commit changes DriverFence::signal() to take &self instead of consuming self by value so that signaling a fence in atomic context does not immediately drop it, and moves the nr_of_unsignaled_fences decrement inside the unsignaled check. > diff --git a/rust/kernel/dma_buf/dma_fence.rs > b/rust/kernel/dma_buf/dma_fence.rs > index a1d851656562d..3022bdb5f7c3e 100644 > --- a/rust/kernel/dma_buf/dma_fence.rs > +++ b/rust/kernel/dma_buf/dma_fence.rs > @@ -805,7 +805,7 @@ pub fn as_fence(&self) -> &Fence { > } > > /// Signal the fence. This will invoke all registered callbacks. > - pub fn signal(self, res: Result) { > + pub fn signal(&self, res: Result) { > let fence = self.as_fence().lock(); > > // SAFETY: `fence` is valid because `self` is valid. The lock must be > @@ -816,14 +816,15 @@ pub fn signal(self, res: Result) { > // must not have been signaled yet, which we check directly > above. > unsafe { bindings::dma_fence_set_error(fence.as_raw(), > err.to_errno()) }; > } > + > + // 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); > + > // SAFETY: `fence` is valid because `self` is valid. The lock > must > // be held, which we acquired above. > unsafe { bindings::dma_fence_signal_locked(fence.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); > } > } [Severity: High] Can decrementing fctx.nr_of_unsignaled_fences in DriverFence::signal() before DriverFence::drop() runs lead to a use-after-free of FenceContext? Previously, DriverFence::signal(self, res) consumed self by value, so decrementing nr_of_unsignaled_fences was immediately followed by DriverFence::drop(), which queues the drop_driver_fence_data() callback via call_rcu(): rust/kernel/dma_buf/dma_fence.rs:DriverFence::drop() { ... unsafe { bindings::call_rcu(rcu_head_ptr, Some(drop_driver_fence_data::<T>)) }; } In FenceContext::drop(), the check on nr_of_unsignaled_fences and the subsequent rcu_barrier() rely on all associated DriverFence instances having already been dropped and having queued their call_rcu() callbacks: rust/kernel/dma_buf/dma_fence.rs:FenceContext::drop() { if self.nr_of_unsignaled_fences.load(Relaxed) != 0 { panic!("Forgotten fences in FenceContext."); } ... rcu_barrier(); } With DriverFence::signal(&self, res) no longer consuming self, fctx.nr_of_unsignaled_fences is decremented before DriverFence::drop() runs and before bindings::dma_fence_signal_locked() is called. If a DriverFence is signaled via DriverFence::signal() and then leaked with core::mem::forget(), or held as a foreign pointer via ForeignOwnable::into_foreign() and signaled via DriverFenceBorrow::signal(), nr_of_unsignaled_fences can reach 0 before DriverFence::drop() has queued call_rcu(). When FenceContext::drop() then runs: 1. The nr_of_unsignaled_fences check passes even though DriverFence::drop() has not run. 2. rcu_barrier() only waits for already-queued call_rcu() callbacks, so it can return without waiting for a grace period after dma_fence_signal_locked(), or before a foreign-owned DriverFence is later reclaimed via DriverFence::from_foreign() and dropped. 3. For a foreign-owned DriverFence, decrementing nr_of_unsignaled_fences with Relaxed ordering before dma_fence_signal_locked() also allows FenceContext::drop() to free fctx.driver_name and fctx.timeline_name while dma_fence_signal_locked() (via trace_dma_fence_signaled()) or concurrent readers in dma_fence_driver_name() and dma_fence_timeline_name() are still accessing the FenceContext through FenceContext::get_driver_name() and FenceContext::get_timeline_name(). Should nr_of_unsignaled_fences only be decremented in DriverFence::drop() (or tracked until drop) so that FenceContext::drop() cannot complete before all DriverFence instances have queued their call_rcu() callbacks? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
