Re: [PATCH 2/2] 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] rust: dma_fence: leak of FenceDataType in DriverFence::drop()
- [High] rust: dma_fence: missing RCU grace period before FenceContext drop
- [Low] rust: dma_fence: stale atomic-context Drop docs on FenceDataType
- [Low] rust: dma_fence: broken intra-doc link to rcu::synchronize_rcu
--

commit 5b8baa0550a0166f96a31017e673de763e929c7c
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()
with a synchronous synchronize_rcu() call so that driver fence payload 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 3022bdb5f7c3e..ad9cebf8428c0 100644
> --- a/rust/kernel/dma_buf/dma_fence.rs
> +++ b/rust/kernel/dma_buf/dma_fence.rs
> @@ -42,7 +42,10 @@
>              Atomic,
>              Relaxed, //
>          },
> -        rcu::rcu_barrier, //
> +        rcu::{
> +            rcu_barrier,
> +            synchronize_rcu, //
> +        }, //
>      }, //
>  };
>  
> @@ -65,7 +68,7 @@ pub struct FenceContext<T: FenceContextOps + Send + Sync> {
>      /// The sequence number for the next fence created.
>      seqno: Atomic<u64>,
>      // The name parameters can be accessed by the dma_fence backend_ops. UAF
> -    // errors are prevented by the `call_rcu()` in `drop_driver_fence_data()`.
> +    // errors are prevented by the `synchronize_rcu()` in `drop_driver_fence_data()`.
>      /// The name of the driver this FenceContext's fences belong to.
>      driver_name: CString,
>      /// The name of the timeline this FenceContext's fences belong to.

[ ... ]

> @@ -600,8 +602,6 @@ struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> {
>      // necessary so that the C backend can free the allocation (coming from our
>      // Rust code) with kfree_rcu().
>      inner: Fence,
> -    /// Callback head for dropping this in a deferred manner through RCU.
> -    rcu_head: bindings::callback_head,
>      /// Reference to access the FenceContext.
>      fctx: &'a FenceContext<T>,
>      /// The API user's data. It is essential that the data only performs

[Severity: Low]
Since DriverFence::drop() now drops the payload synchronously after
synchronize_rcu() instead of inside a call_rcu() callback, should the doc
comments on DriverFenceData::data and FenceContext::new_fence_allocation() be
updated to remove the requirement that T::FenceDataType::drop() only perform
operations legal in atomic context?

> @@ -644,6 +644,10 @@ struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> {
>  ///
>  /// In the unlikely case of such violations, a panic is thrown.
>  ///
> +/// For requirements imposed by the C backend, a [`DriverFence`] might perform
> +/// a call to [`rcu::synchronize_rcu`] when it drops. Thus, avoid dropping a
> +/// [`DriverFence`] in atomic context.

[Severity: Low]
Because synchronize_rcu is imported directly from kernel::sync::rcu rather
than importing the rcu module, will the intra-doc link to rcu::synchronize_rcu
fail to resolve in rustdoc unless written as synchronize_rcu?

[ ... ]

> @@ -973,59 +977,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]
Can this cause a use-after-free on FenceContext (and its driver_name and
timeline_name fields) if a concurrent RCU reader is in dma_fence_driver_name()
or dma_fence_timeline_name() when the last fence is signaled and FenceContext
is dropped?

In DriverFence::drop(), if !core::mem::needs_drop::<T::FenceDataType>() is
true, the function returns early before calling synchronize_rcu():

    if !core::mem::needs_drop::<T::FenceDataType>() {
        unsafe { bindings::dma_fence_put(self.as_raw()) };
        return;
    }

Similarly, if a signaled DriverFence is forgotten or converted via
ForeignOwnable::into_foreign() after DriverFence::signal() has already
decremented nr_of_unsignaled_fences, DriverFence::drop() does not run before
FenceContext can be dropped.

In FenceContext::drop(), the code calls rcu_barrier() to wait for callbacks
from DriverFence::drop():

    // Wait for the RCU callbacks in `DriverFence::drop`.
    rcu_barrier();

However, DriverFence::drop() no longer queues any call_rcu() callbacks, and
rcu_barrier() only waits for pending call_rcu() callbacks rather than waiting
for an RCU grace period. Should FenceContext::drop() call synchronize_rcu()
instead of rcu_barrier() (and should the comment in FenceContext be updated to
no longer refer to the removed drop_driver_fence_data() function)?

>  
>          // 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 when DriverFence is dropped?

On DriverFence, self.data has type NonNull<DriverFenceData<'a, T>>:

    pub struct DriverFence<'a, T: Send + Sync + FenceContextOps> {
        data: NonNull<DriverFenceData<'a, T>>,
    }

While the user payload is the data field inside DriverFenceData:

    struct DriverFenceData<'a, T: Send + Sync + FenceContextOps> {
        inner: Fence,
        fctx: &'a FenceContext<T>,
        #[pin]
        data: T::FenceDataType,
    }

Passing &raw mut self.data to drop_in_place() drops the NonNull pointer itself
(which is a no-op) rather than (*self.data.as_ptr()).data, and the subsequent
dma_fence_put() lets C free the allocation via kfree_rcu() without ever
running the destructor of T::FenceDataType. Should this be:

    unsafe { drop_in_place(&raw mut (*self.data.as_ptr()).data) };

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