Re: [PATCH 3/4] rust: Add dma_fence abstractions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] DriverFence::drop eagerly drops FenceCtx and user data before the C dma_fence refcount reaches 0, leading to a Use-After-Free.
- [High] FenceCtx::drop unconditionally calls rcu_barrier(), causing a 'scheduling while atomic' panic when a DriverFence is dropped from an interrupt handler or under a spinlock.
- [High] DriverFence::drop() passes a positive error code (ECANCELED) to dma_fence_set_error(), triggering a WARN_ON and misconfiguring the error state.
- [High] DriverFence::as_raw and DriverFenceBorrow::as_raw create exclusive mutable references (&mut) to actively shared data, causing Rust aliasing Undefined Behavior.
--

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

This commit introduces Rust abstractions for the dma_fence framework. It
provides DriverFence, Fence, and FenceCtx types to decouple fence issuers
from consumers and safely manage synchronization primitives required by
GPU drivers.

> diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
> new file mode 100644
> --- /dev/null
> +++ b/rust/kernel/dma_buf/dma_fence.rs

[ ... ]

> +#[pinned_drop]
> +impl<F: Send + Sync, C: Send + Sync> PinnedDrop for FenceCtx<F, C> {
> +    fn drop(self: Pin<&mut Self>) {
> +        // SAFETY: `rcu_barrier()` is always safe to be called.
> +        unsafe { bindings::rcu_barrier() };

[Severity: High]
Does calling rcu_barrier() here risk a scheduling-while-atomic panic?

Since DriverFence::signal() consumes self, the DriverFence will be dropped
at the end of the method. If this happens in an atomic context (such as an
interrupt handler or under a spinlock), and it holds the last reference to
the FenceCtx, dropping it triggers FenceCtx::drop().

Because rcu_barrier() blocks and sleeps until all pending RCU callbacks
complete, this could attempt to sleep in atomic context.

> +    }
> +}

[ ... ]

> +impl<F: Send + Sync, C: Send + Sync> DriverFence<F, C> {
> +    fn as_raw(&self) -> *mut bindings::dma_fence {
> +        // SAFETY: Valid because `self` is valid.
> +        let fence_data = unsafe { &mut *self.data.as_ptr() };

[Severity: High]
Does this create an exclusive mutable reference to actively shared memory?

DriverFenceData can be accessed concurrently by other threads holding Fence
references (for instance, when checking if the fence is signaled). Creating
a mutable reference (&mut) in Rust asserts exclusive access and violates
aliasing rules, which allows the compiler to make invalid optimization
assumptions.

Could this use an immutable reference &*self.data.as_ptr() instead?

> +
> +        fence_data.inner.inner.get()
> +    }

[ ... ]

> +impl<F: Send + Sync, C: Send + Sync> DriverFenceBorrow<F, C> {
> +    fn as_raw(&self) -> *mut bindings::dma_fence {
> +        // SAFETY: Valid because `self` is valid.
> +        let fence_data = unsafe { &mut *self.data.as_ptr() };

[Severity: High]
Similar to DriverFence::as_raw(), does this also incorrectly create a
mutable reference to shared data?

> +
> +        fence_data.inner.inner.get()
> +    }

[ ... ]

> +impl<F: Send + Sync, C: Send + Sync> Drop for DriverFence<F, C> {
> +    fn drop(&mut self) {
> +        let fence = self.as_raw();
> +        let mut fence_flags: usize = 0;
> +        let flag_ptr = &raw mut fence_flags;
> +
> +        // 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 the `DriverFence` has been dropped.
> +        unsafe {
> +            bindings::dma_fence_lock_irqsave(fence, flag_ptr);
> +            #[allow(unused_unsafe)]
> +            if warn_on!(!bindings::dma_fence_is_signaled_locked(fence)) {
> +                bindings::dma_fence_set_error(fence, ECANCELED as i32);

[Severity: High]
Is it intentional to pass a positive error code here?

bindings::ECANCELED evaluates to a positive integer, but
dma_fence_set_error() expects a negative error code (and triggers a WARN_ON
for positive values). A positive value might also be interpreted as success
by callers checking for values < 0.

Should this be - (bindings::ECANCELED as i32)?

> +                bindings::dma_fence_signal_locked(fence);
> +            }
> +            bindings::dma_fence_unlock_irqrestore(fence, flag_ptr);
> +        }
> +
> +        // SAFETY: `self.data` is owned by the DriverFence, but could be accessed
> +        // through some dma_fence callbacks right now. Access is being revoked
> +        // above by signalling the fence. The DriverFenceAllowedData trait
> +        // ensures that the data either does not need drop, or if it does it
> +        // lives in a RcuBox which will delay dropping by one grace period, hence
> +        // ensuring that all readers have disappeared.
> +        unsafe { drop_in_place(self.data.as_ptr()) };

[Severity: Critical]
Could this eagerly drop FenceCtx and user data before the C dma_fence
refcount reaches 0?

The C dma_fence object can remain alive as long as consumers (like sync_file
or the GPU scheduler) hold references to it. If the backend later invokes
callbacks like ops->get_driver_name(), FenceCtx::get_driver_name() would
attempt to read fctx.driver_name from the already-dropped FenceCtx:

FenceCtx::get_driver_name() {
    ...
    fctx.driver_name.as_char_ptr()
}

Can this result in a use-after-free?

> +
> +        // 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 the `DriverFence` has been dropped.
> +        unsafe {
> +            bindings::dma_fence_put(fence);
> +        }

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