Re: [PATCH v2 5/6] rust: Add dma_fence abstractions

[Philipp Stanner <phasta@mailbox.org>]

On Tue, 2026-06-16 at 10:28 +0200, Philipp Stanner wrote:
> 
> 

[…]

> Add abstractions for dma_fence in Rust.
> 
> Signed-off-by: Philipp Stanner <phasta@kernel.org>

RFC for one thing I'm worried about below

> ---
>  rust/bindings/bindings_helper.h  |   1 +
>  rust/helpers/dma_fence.c         |  48 ++
>  rust/helpers/helpers.c           |   1 +
>  rust/kernel/dma_buf/dma_fence.rs | 884 +++++++++++++++++++++++++++++++
>  rust/kernel/dma_buf/mod.rs       |  14 +
>  rust/kernel/lib.rs               |   1 +
>  6 files changed, 949 insertions(+)
>  create mode 100644 rust/helpers/dma_fence.c
>  create mode 100644 rust/kernel/dma_buf/dma_fence.rs
>  create mode 100644 rust/kernel/dma_buf/mod.rs
> 
> 
> 

[…]

> +
> +/// The receiving counterpart of a [`DriverFence`], designed to register callbacks
> +/// on, check the signalled state etc. A [`Fence`] cannot be signalled.
> +/// A [`Fence`] is always refcounted.
> +pub struct Fence {
> +    /// The actual dma_fence passed to C.
> +    inner: Opaque<bindings::dma_fence>,
> +}
> +
> 
> 

[…]

> +
> +#[repr(C)] // Necessary to guarantee that `inner` always comes first so that we can cast.
> +#[pin_data]
> +struct DriverFenceData<T: Send + Sync + FenceCtxOps> {
> +    /// Callback head for dropping this in a deferred manner through RCU.
> +    rcu_head: bindings::callback_head,
> +    #[pin]
> +    /// The inner fence.
> +    inner: Fence,
> +    /// Pointer to access the FenceCtx. Useful for obtaining name parameters.
> +    fctx: Arc<FenceCtx<T>>,
> +    /// The API user's data. This must either not need drop, or must delay its
> +    /// drop by a grace period. It is essential that the data only performs
> +    /// operations legal in atomic context in its [`Drop`] implementation.
> +    #[pin]
> +    data: T::FenceDataType,
> +}
> +
> 
> 
> 

[…]

> +pub struct DriverFence<T: Send + Sync + FenceCtxOps> {
> +    /// The actual content of the fence. Lives in a raw pointer so that its
> +    /// memory can be managed independently. Valid until both the [`DriverFence`]
> +    /// and all associated [`Fence`]s have disappeared.
> +    data: NonNull<DriverFenceData<T>>,
> +}
> +
> +
> 

[…]

> }
> +
> +impl<T: Send + Sync + FenceCtxOps> Drop for DriverFence<T> {
> +    fn drop(&mut self) {
> +        let fence = self.as_raw();
> +        let mut fence_flags: usize = 0;
> +        let flag_ptr = &raw mut fence_flags;
> +
> +        // SAFETY: `fence` is valid until the `call_rcu()` below. `flag_ptr` is
> +        // merely a pointer to an integer, which lives as long as this function.
> +        unsafe { bindings::dma_fence_lock_irqsave(fence, flag_ptr) };
> +
> +        // Use dma_fence_test_signaled_flag() instead of dma_fence_is_signaled_locked()
> +        // because the C backend wants to get rid of the latter.
> +
> +        // SAFETY: `fence` is valid until the `call_rcu()` below.
> +        let signaled: bool = unsafe { bindings::dma_fence_test_signaled_flag(fence) };
> +        if warn_on!(!signaled) {
> +            // SAFETY: `fence` is valid until the `call_rcu()` below. The fence
> +            // must not have been signaled yet, which we check directly above.
> +            unsafe { bindings::dma_fence_set_error(fence, ECANCELED.to_errno()) };
> +            // SAFETY: `fence` is valid until the `call_rcu()` below. The lock must
> +            // be held, which we acquired above.
> +            unsafe { bindings::dma_fence_signal_locked(fence) };
> +        }
> +        // SAFETY: `fence` is valid until the `call_rcu()` below. `flag_ptr` is
> +        // merely a pointer to an integer, which lives as long as this function.
> +        // The lock must be held, which we acquired above.
> +        unsafe { bindings::dma_fence_unlock_irqrestore(fence, flag_ptr) };
> +
> +        // SAFETY: Valid because `self` is valid.
> +        let rcu_head_ptr = unsafe { &raw mut (*self.data.as_ptr()).rcu_head };
> +
> +        // `DriverFenceData` but could be accessed through some dma_fence callbacks
> +        // right now. Access is being revoked in principle above by signalling
> +        // the fence, but since the C backend does not guarantee perfect full
> +        // synchronization, we have to wait for one grace period to ensure that
> +        // all accessors of `DriverFenceData` (through the dma_fence_ops accessible
> +        // through a `Fence`) are gone.
> +
> +        // 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>)) };
> +    }
> +}
> +
> +// TODO:
> +// The entire call_rcu() mechanism in the drop above and the code below would be
> +// unnecessary if C's dma_fence_signal() could be reworked in a way that after it
> +// ran, the caller knows that no fence_ops callbacks can be running anymore.
> +// In other words, if the dma_fence backend would use its spinlock for full
> +// synchronization.
> +//
> +// Then we could move the drop_in_place() and dma_fence_put() upwards into the
> +// drop() implementation and call it a day.
> +
> +/// Finally really drop this `DriverFence<T>`
> +///
> +/// # Safety
> +///
> +/// `head` references the `rcu_head` field of an `DriverFenceData<T>`. All
> +/// accessors to that `DriverFenceData<T>` must be gone by now. This must be
> +/// ensured by signalling the associated `DriverFence<T>` and then waiting
> +/// for a grace period until calling this function here.
> +unsafe extern "C" fn drop_driver_fence_data<T: Send + Sync + FenceCtxOps>(
> +    head: *mut bindings::callback_head,
> +) {
> +    // SAFETY: Caller provides a pointer to the `rcu_head` field of a `DriverFenceData<C>`.
> +    let fence_data = unsafe { container_of!(head, DriverFenceData<T>, rcu_head) };
> +
> +    // SAFETY: `fence_data` was created validly above. All the fence's data will
> +    // only drop below, but the raw pointer to the raw C `dma_fence` remains
> +    // valid because the reference count is only decremented at the end of the
> +    // function.
> +    let fence = unsafe { (*fence_data).inner.inner.get() };
> +
> +    // SAFETY: A grace period has passed. All accessors to that `DriverFenceData`
> +    // are gone now. We have exclusive access and can now drop it.
> +    unsafe { drop_in_place(fence_data) };


Here we would drop DriverFenceData. We do it like that (RCU delay) now
because of two-fold reasoning:

a) dropping too soon could deallocate the FenceCtx, causing UAF.

b) drop_in_place() invalidates the fctx pointer, so the Rust compiler
could hypothetically write some random data for CPU pipelining etc.
reasons into the pointer, despite it still being used from the backend
functions.


So what I'm wondering about is whether we also solved that problem for
DriverFenceData.inner.inner, i.e., the C dma_fence which resides in
that memory, which can be in use by an arbitrary number of refcounting
Fence's

Does Opaque guarantee that the backing memory within DriverFenceData
will never be interfered with in an undefined behavior way? The
documentation does not explicitly seem to state that.


P.