Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] rust: dma_fence: payload leak in DriverFence::drop() dropping NonNull wrapper --
commit 5c06afa6bb1de1d597e5eea16a29dd0b6c6d9220 Author: Philipp Stanner <[email protected]> rust: DmaFence: Replace call_rcu() with synchronize_rcu() This commit replaces the deferred RCU callback in DriverFence::drop() with synchronize_rcu() so that driver fence data is not dropped from atomic context, and updates FenceContext::drop() to use synchronize_rcu() instead of rcu_barrier(). > diff --git a/rust/kernel/dma_buf/dma_fence.rs > b/rust/kernel/dma_buf/dma_fence.rs > index 735c4f090a2c4..fbbc6ab202eca 100644 > --- a/rust/kernel/dma_buf/dma_fence.rs > +++ b/rust/kernel/dma_buf/dma_fence.rs [ ... ] > @@ -980,59 +968,18 @@ fn drop(&mut self) { > return; > } > > + // Make sure none of the fence backend_ops can access fence-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(); > + > // 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) }; > > - // 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()) }; > } > } [Severity: High] As reported by Sashiko on RFC v6 [1], does DriverFence::drop() leak the T::FenceDataType payload when a DriverFence is dropped? In DriverFence, self.data has type NonNull<DriverFenceData<'a, T>>, while the user payload of type T::FenceDataType lives in the inner data field of DriverFenceData: rust/kernel/dma_buf/dma_fence.rs: struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> { #[pin] inner: Fence, fctx: &'a FenceContext<T>, #[pin] data: T::FenceDataType, } ... pub struct DriverFence<'a, T: Send + Sync + FenceContextOps> { data: NonNull<DriverFenceData<'a, T>>, } Because self is &mut DriverFence<'a, T>, passing &raw mut self.data to drop_in_place() passes *mut NonNull<DriverFenceData<'a, T>> rather than *mut T::FenceDataType. Since NonNull is a Copy pointer wrapper with no Drop implementation, drop_in_place(&raw mut self.data) is a no-op, and the subsequent dma_fence_put() releases the underlying allocation without ever running the destructor for T::FenceDataType. Should this obtain the raw dma_fence pointer before dropping the inner payload, and then pass &raw mut (*self.data.as_ptr()).data to drop_in_place() so that self.as_raw() does not create a shared reference to DriverFenceData after its data field has been dropped in place? [1] https://lore.kernel.org/r/[email protected] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
