Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] rust: dma_fence: Memory leak of T::FenceDataType in DriverFence::drop() - [High] rust: dma_fence: Sleep-in-atomic hazard due to synchronize_rcu() in Drop --
commit fd18c02da88f26cc4c8c5dc518b90c8f1e16560b Author: Philipp Stanner <[email protected]> rust: DmaFence: Replace call_rcu() with synchronize_rcu() The C dma_fence contract demands that a fence's data disappears no earlier than 1 RCU grace period after the DriverFence was signaled. So far, we achieved this with dropping the DriverFence's data in a deferred manner with a manual, raw binding call to call_rcu(). > diff --git a/rust/kernel/dma_buf/dma_fence.rs > b/rust/kernel/dma_buf/dma_fence.rs > index 4e9b4a6a9471b..cefcde80bf092 100644 > --- a/rust/kernel/dma_buf/dma_fence.rs > +++ b/rust/kernel/dma_buf/dma_fence.rs [ ... ] > @@ -1022,59 +1020,18 @@ fn drop(&mut self) { > return; > } > > + // Make sure none of the fence backend_ops can access data anymore. > + // > + // TODO: > + // This would not be necessary if the C dma_fence backend were using > a > + // spinlock to properly synchronize its signaled state. Fix it in C > and > + // then remove synchronize_rcu(). > + synchronize_rcu(); [Severity: High] Does this introduce a sleep-in-atomic hazard when a DriverFence is dropped? Since synchronize_rcu() blocks the current thread until an RCU grace period completes, and Drop is executed implicitly when a value goes out of scope in Rust, this could sleep if a DriverFence is dropped in an atomic context (such as an interrupt handler or a spinlock critical section). Because dma_fence is frequently manipulated in hardware IRQs, could this cause a 'scheduling while atomic' kernel panic or deadlock? > > // SAFETY: Valid because `self` is valid. > - let rcu_head_ptr = unsafe { &raw mut (*self.data.as_ptr()).rcu_head > }; > + unsafe { drop_in_place(&raw mut self.data) }; [Severity: High] Does this leak the T::FenceDataType payload? Since self.data appears to be a NonNull wrapper, the expression &raw mut self.data yields a pointer to the wrapper itself, not the actual payload it points to. Calling drop_in_place() on this pointer invokes the destructor for NonNull, which is a no-op since it only implements Copy. Should this be dropping the underlying payload, such as via &raw mut (*self.data.as_ptr()).data, as it did in the previous drop_driver_fence_data callback? > > - // SAFETY: `call_rcu()` is always safe to be called. `rcu_head_ptr` > was > - // created validly above. The module must perform a > `synchronize_rcu()` > - // or `rcu_barrier()` call to guard against module unload. > - unsafe { bindings::call_rcu(rcu_head_ptr, > Some(drop_driver_fence_data::<T>)) }; > + // SAFETY: The `synchronize_rcu()` above ensures all accessors are > gone. > + unsafe { bindings::dma_fence_put(self.as_raw()) }; > } > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
