Re: [RFC PATCH v5 5/6] rust: DmaFence: Replace call_rcu() with synchronize_rcu()

[sashiko-bot@kernel.org]

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

    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/20261009191124.1022902-2-phasta@kernel.org?part=5