Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] dma_fence: counter underflow in DriverFence::signal on multiple calls - [High] dma_fence: sleep-in-atomic bug when dropping DriverFence under spinlock --
commit 79012d18b924ead15254b36baf7e1933525374af Author: Philipp Stanner <[email protected]> rust: DmaFence: Don't drop on signal This commit decouples the dropping of a DriverFence from its signaling, changing the signal() method to take &self rather than consuming the fence. This allows the fence to be signaled multiple times and defers its destruction to when the object goes out of scope. The intent is to avoid dropping fences in an atomic context where deferred deletion is problematic. > diff --git a/rust/kernel/dma_buf/dma_fence.rs > b/rust/kernel/dma_buf/dma_fence.rs > index 0028124e6aa64..b67121c7abd41 100644 > --- a/rust/kernel/dma_buf/dma_fence.rs > +++ b/rust/kernel/dma_buf/dma_fence.rs > @@ -806,7 +806,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) { [Severity: High] Could the unconditional decrement at the end of this function corrupt the nr_of_unsignaled_fences counter since signal() can now be called multiple times? rust/kernel/dma_buf/dma_fence.rs:DriverFence::signal() { ... let fctx = unsafe { self.data.as_ref().fctx }; let _ = fctx.nr_of_unsignaled_fences.fetch_sub(1, Relaxed); } If signal() is called twice on the same fence, it looks like it will underflow the hazard counter, causing a delayed panic when the FenceContext is dropped. [Severity: High] Will this decoupling of signal() from drop() lead to a sleep-in-atomic bug later in the series? Since dropping is now deferred to when the DriverFence goes out of scope, it appears to conflict with other patches in this series. In patch 5, call_rcu() is replaced with synchronize_rcu() in DriverFence::drop(): rust/kernel/dma_buf/dma_fence.rs:DriverFence::drop() { ... synchronize_rcu(); ... } In patch 6, DriverFence is dropped while a SpinLockIrq is held: rust/kernel/drm/job_queue.rs:JobQueue::complete_jobs_up_to_seqno() { let mut jq_locked = jq.lock(); let mut jq = jq_locked.as_mut().project(); let mut cursor = jq.running_jobs.cursor_front(); while let Some(job) = cursor.peek_next() { let mut job = job.remove(); ... } // `job` goes out of scope here, triggering DriverFence::drop } Dropping the DriverFence will call synchronize_rcu(), which sleeps, leading to a sleep-in-atomic panic when invoked from the JobQueue lock guard. > let fence = self.as_fence().lock(); > > // SAFETY: `fence` is valid because `self` is valid. The lock must be -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
