Re: [PATCH 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 to consider:
- [High] rust: dma_fence: payload leak in DriverFence::drop() dropping
  NonNull wrapper
--

commit afa4bc5452ca231722f66b50ee2edd5a29c21433
Author: Philipp Stanner <phasta@kernel.org>

rust: DmaFence: Replace call_rcu() with synchronize_rcu()

This commit replaces the deferred call_rcu() callback in DriverFence::drop()
and the rcu_barrier() in FenceContext::drop() with synchronous
synchronize_rcu() calls so that driver fence data is not dropped in atomic
context.

> diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
> index c04b68fe67fe3..f74119b4d7f87 100644
> --- a/rust/kernel/dma_buf/dma_fence.rs
> +++ b/rust/kernel/dma_buf/dma_fence.rs

[ ... ]

> @@ -988,59 +980,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]
Does drop_in_place(&raw mut self.data) leak the user payload
T::FenceDataType on every DriverFence drop?

On DriverFence, self.data is NonNull<DriverFenceData<'a, T>>, which is a
pointer wrapper with no Drop implementation, whereas the user payload lives
in (*self.data.as_ptr()).data.  As a result, drop_in_place(&raw mut
self.data) is a no-op and T::FenceDataType's destructor never runs before the
C backend frees the allocation.

Also, when dropping (*self.data.as_ptr()).data in place, the raw dma_fence
pointer should be obtained before drop_in_place() because self.as_raw()
creates a shared reference &DriverFenceData<'a, T>, which would otherwise
happen after its data field has already been dropped.

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