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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Incorrect struct layout in `DriverFenceData` places `rcu_head` before `inner: Fence`, causing misaligned pointer casts and SLUB corruption when the C backend frees the interior pointer.
- [Critical] `DriverFence::drop` prematurely destroys `FenceCtx` and user data before the `dma_fence` reference count reaches 0, causing Use-After-Free.
- [High] `FenceCtx<T>` and `DriverFence<T>` lack a `'static` bound on `T`, allowing safe Rust code to erase lifetimes and cause Use-After-Free.
- [High] `FenceCbRegistration::new` leaks an `ARef<Fence>` reference count on the error path.
- [High] Dropping the last reference to `FenceCtx` from the RCU callback will execute the user's `T` (context data) `Drop` implementation in softirq context.
--

commit 95eb8ea421a40fbf4b3b019ea0a7b2c3928e1e11
Author: Philipp Stanner <phasta@kernel.org>
rust: Add dma_fence abstractions

This commit introduces Rust abstractions for the C dma_fence framework.
It provides types such as DriverFence, Fence, and FenceCtx to cleanly 
decouple fence issuers and consumers while interacting with the C backend.

> diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
> new file mode 100644
> index 0000000000000..3af2dee58e13f
> --- /dev/null
> +++ b/rust/kernel/dma_buf/dma_fence.rs
> @@ -0,0 +1,884 @@
[ ... ]
> +/// A dma-fence context. A fence context takes care of associating related fences with each other,
> +/// providing each with raising sequence numbers and a common identifier.
> +#[pin_data]
> +pub struct FenceCtx<T: Send + Sync> {

[Severity: High]
Does T need a static bound here?

Fence contains an Opaque<bindings::dma_fence> without lifetime parameters,
making it implicitly static. DriverFence::as_fence allows extracting an
owned ARef<Fence>. Because it is static, safe Rust code could send this
ARef to threads that outlive T. 

If T contains non-static references, the ARef<Fence> will keep
DriverFenceData alive beyond T's lifetime. Could this allow lifetime
erasure and a subsequent use-after-free?

> +    /// The fence context number.
> +    nr: u64,
> +    /// The sequence number for the next fence created.
> +    seqno: AtomicU64,
[ ... ]
> +#[allow(unused_unsafe)]
> +impl<T: Send + Sync + FenceCtxOps> FenceCtx<T> {
> +    // This can later be extended as a vtable in case other parties need support
> +    // for the more "exotic" callbacks.
> +    const OPS: bindings::dma_fence_ops = bindings::dma_fence_ops {
> +        get_driver_name: Some(Self::get_driver_name),
> +        get_timeline_name: Some(Self::get_timeline_name),
> +        enable_signaling: None,
> +        signaled: None,
> +        wait: None,
> +        release: None,
> +        set_deadline: None,
> +    };

[Severity: Critical]
Will the C backend crash when trying to free this fence? 

By leaving release as None, the dma_fence backend defaults to using
dma_fence_free(). As defined later in DriverFenceData, the dma_fence is
embedded at a non-zero offset. dma_fence_free() will call kfree_rcu() on
this interior pointer. Does this trigger a bad pointer panic in the SLUB
allocator?

[ ... ]
> +    /// Create a [`FenceCtx`] from an associated [`bindings::dma_fence`].
> +    ///
> +    /// # Safety
> +    ///
> +    /// `ptr` must be a valid pointer to a dma_fence which resides within a [`Fence`],
> +    /// which in turn resides in a [`DriverFenceData`].
> +    unsafe fn from_raw_fence<'a>(ptr: *mut bindings::dma_fence) -> &'a Self {
> +        let opaque_fence = Opaque::cast_from(ptr);
> +
> +        // SAFETY: Safe due to the function's overall safety requirements.
> +        let fence_ptr = unsafe { container_of!(opaque_fence, Fence, inner) };
> +
> +        // DriverFenceData is repr(C) and a Fence is its first member.
> +        let fence_data_ptr = fence_ptr as *mut DriverFenceData<T>;

[Severity: Critical]
Can this direct cast lead to misaligned structure access? 

The comment states Fence is the first member, but rcu_head is actually
defined before it in DriverFenceData. Casting the pointer directly to
DriverFenceData without accounting for the 16-byte offset of the inner
field could cause memory corruption when fctx is accessed later.

[ ... ]
> +    pub fn new<'a>(fence: &'a Fence, callback: T) -> impl PinInit<Self, CallbackError<T>> + 'a
> +    where
> +        T: 'a,
> +    {
[ ... ]
> +        unsafe {
> +            pin_init_from_closure(move |slot: *mut Self| {
> +                let slot_callback = &raw mut (*slot).callback;
> +                let slot_fence = &raw mut (*slot).fence;
> +                let slot_cb = &raw mut (*slot).cb;
> +
> +                // Write callback and fence first — must be visible before
> +                // dma_fence_add_callback makes the registration live.
> +                core::ptr::write(slot_callback, callback);
> +                core::ptr::write(slot_fence, ARef::from(fence));
> +
> +                let ret = to_result(bindings::dma_fence_add_callback(
> +                    fence.inner.get(),
> +                    Opaque::cast_into(slot_cb),
> +                    Some(Self::dma_fence_callback),
> +                ));
> +
> +                match ret {
> +                    Ok(()) => Ok(()),
> +                    Err(e) => {
> +                        // Read back what we wrote to leave the slot clean.
> +                        let cb_back = core::ptr::read(slot_callback);
> +
> +                        if e.to_errno() == ENOENT.to_errno() {
> +                            Err(CallbackError::AlreadySignaled(cb_back))
> +                        } else {
> +                            Err(CallbackError::Other(e))
> +                        }
> +                    }
> +                }
> +            })
> +        }
> +    }

[Severity: High]
Does this error path leak the ARef reference count?

Earlier in this closure, ARef::from(fence) is written into slot_fence,
which increments the reference count. If dma_fence_add_callback() fails,
the error path reads back slot_callback to return it, but slot_fence is
neither read back nor manually dropped. 

Since pin_init_from_closure assumes the slot is uninitialized on error
and won't run drop on it, will this permanently leak the ARef<Fence> and
its memory allocation?

[ ... ]
> +#[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,

[Severity: Critical]
Is the inner Fence guaranteed to be at offset 0 here? 

The struct is marked repr(C), but rcu_head precedes inner. This shifts
the embedded Fence by the size of the callback_head, directly conflicting
with the assumption in from_raw_fence that Fence is the first member.

[ ... ]
> +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>>,
> +}

[Severity: High]
Similar to FenceCtx, does DriverFence also require a static bound on T to
prevent safe code from erasing lifetimes and causing a use-after-free?

[ ... ]
> +/// 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) };

[Severity: High]
Could this drop the user's data T in softirq context?

Since drop_driver_fence_data runs as an RCU callback, it executes in atomic
(softirq) context. If this drop_in_place() puts the last reference to the
Arc<FenceCtx<T>>, it will trigger the deallocation of the context and drop
its inner data T.

There appear to be no trait bounds or documentation enforcing that T must
only perform operations legal in atomic context. If a user provides context
data whose Drop implementation sleeps (like acquiring a mutex or freeing
large structures), would this trigger a sleeping in atomic context kernel
panic?

[Severity: Critical]
Does this prematurely destroy the user data and FenceCtx before the
dma_fence reference count actually reaches zero?

The dma_fence refcount is shared with ARef<Fence>. If an ARef<Fence> is
still alive when DriverFence drops, this RCU callback will execute
drop_in_place(fence_data). This destroys the Arc<FenceCtx> and the
user data. However, the C dma_fence is still alive with a non-zero
refcount.

When the refcount eventually hits zero later on, the C backend could
trigger callbacks like get_driver_name() via trace_dma_fence_destroy().
Could those callbacks access the already-freed fence_data.fctx, leading
to a use-after-free? 

Should the Rust data instead be dropped in a custom release callback in
dma_fence_ops when the refcount actually reaches 0?

> +
> +    // The `drop_in_place()` above might drop the `FenceCtx` by putting the last
> +    // reference.
> +
> +    // SAFETY: Once a `DriverFence` is initialized, the inner `fence` is
> +    // valid and initialized. It is valid until the refcount drops
> +    // to 0, which can earliest happen once we drop the `DriverFence`'s reference
> +    // here.
> +    unsafe { bindings::dma_fence_put(fence) };
> +
> +    // The actual memory the data associated with a `DriverFence` lives in
> +    // gets freed by the C dma_fence backend once the fence's refcount reaches 0.
> +}

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