Re: [RFC PATCH v7 4/8] rust: DmaFence: Replace call_rcu() with synchronize_rcu()

[sashiko-bot@kernel.org]

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 <phasta@kernel.org>

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/20261016085724.1031766-6-phasta@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103153138.1087330-2-phasta@kernel.org?part=4