Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] mm: memory leak of T::FenceDataType in DriverFence::drop due to dropping NonNull wrapper - [High] locking: synchronize_rcu() in DriverFence::drop causes sleep-in-atomic - [High] dma_fence: UAF of FenceContext strings via outliving Fence consumers - [Low] doc: outdated safety comments referencing removed call_rcu() --
commit 900eae064aeab4bf810f0f2081db8a2981b08546 Author: Philipp Stanner <[email protected]> rust: DmaFence: Replace call_rcu() with synchronize_rcu() This commit replaces the use of call_rcu() with synchronize_rcu() for dropping DriverFence data. It removes the raw C callback and drops the data in-place using drop_in_place(). > diff --git a/rust/kernel/dma_buf/dma_fence.rs > b/rust/kernel/dma_buf/dma_fence.rs > index b67121c7abd41..9efb84c00b6a2 100644 > --- a/rust/kernel/dma_buf/dma_fence.rs > +++ b/rust/kernel/dma_buf/dma_fence.rs [ ... ] > @@ -973,59 +971,18 @@ fn drop(&mut self) { > return; > } [Severity: Low] Does this patch leave outdated safety comments in DriverFence::drop()? The deferred call_rcu() has been removed from DriverFence::drop() and replaced with synchronize_rcu(), but multiple SAFETY comments still reference it: // SAFETY: `guard` is valid until the `call_rcu()` below. let signaled: bool = unsafe { bindings::dma_fence_test_signaled_flag(guard.as_raw()) }; Could these comments be updated to prevent confusion during future maintenance? > > + // 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] DriverFence implements Send + Sync, and Drop is called automatically when variables go out of scope. Can this synchronize_rcu() be called from atomic contexts like spinlock critical sections, interrupt handlers, or RCU read-side critical sections? Because synchronize_rcu() blocks and waits for a grace period, wouldn't dropping a DriverFence in these contexts cause a might_sleep() lockdep warning or a system deadlock on PREEMPT_RT? [Severity: High] This problem wasn't introduced by this patch, but does FenceContext have a use-after-free issue with its strings? The safety comment in get_driver_name() assumes it is only called on unsignaled fences: rust/kernel/dma_buf/dma_fence.rs:get_driver_name() { // SAFETY: The C backend only invokes this callback with `ptr` pointing // to a valid, unsignaled `bindings::dma_fence`. // ... let fctx = unsafe { Self::from_raw_fence(ptr) }; fctx.driver_name.as_char_ptr() } However, sync_fill_fence_info() accesses the driver name when userspace queries the sync_file info, and it calls get_driver_name() regardless of the signaled state. A consumer holding a sync_file fd can retain a reference to the Fence long after the driver signals it and drops the FenceContext. Furthermore, by replacing call_rcu() with synchronize_rcu() in DriverFence::drop(), doesn't this completely defeat the rcu_barrier() in FenceContext::drop() which was meant to wait for these tear-downs? rust/kernel/dma_buf/dma_fence.rs:FenceContext::drop() { // Wait for the RCU callbacks in `DriverFence::drop`. rcu_barrier(); } > > // 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 actually drop the underlying data, or does it just drop the NonNull pointer itself? Since self.data is of type NonNull<DriverFenceData<'a, T>>, &raw mut self.data produces a pointer to the NonNull wrapper (*mut NonNull<...>). Because NonNull is Copy and has no Drop implementation, calling drop_in_place() on this pointer is a no-op. Will this permanently leak the actual user payload T::FenceDataType located at (*self.data.as_ptr()).data? > - // 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=4
