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

[sashiko-bot@kernel.org]

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

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