Re: [PATCH v11 1/2] rust: Add dma_fence abstractions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The consumer `Fence` can outlive its `FenceContext`, leading to a use-after-free when C callbacks access the context.
- [High] Unconditional spinlock acquisition in `Fence::is_signaled()` causes a deadlock if invoked from within a fence callback.
- [High] `FenceContext`'s `PinnedDrop` implementation calls `rcu_barrier()`, which blocks and sleeps, making it unsafe to drop in atomic context.
- [Low] Missing `#[inline]` annotation on small abstraction functions forwarding to C bindings.
--

commit fb115447c7bec2def2c1691d01dc640dec6f4092
Author: Philipp Stanner <phasta@kernel.org>

    rust: Add dma_fence abstractions
    
    C's dma_fence's are synchronisation primitives that will be needed by all
    Rust GPU drivers.

> diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
> index 0000000000000..32d9999e75d44
> --- /dev/null
> +++ b/rust/kernel/dma_buf/dma_fence.rs

[ ... ]

> +    extern "C" fn get_driver_name(ptr: *mut bindings::dma_fence) -> *const c_char {
> +        // SAFETY: The C backend only invokes this callback with `ptr` pointing
> +        // to a valid, unsignaled `bindings::dma_fence`. All fences created in
> +        // this module always reside within `Fence` which always resides in a
> +        // `DriverFenceData`, thus satisfying the function's safety
> +        // requirements.
> +        let fctx = unsafe { Self::from_raw_fence(ptr) };
> +
> +        fctx.driver_name.as_char_ptr()
> +    }
> +
> +    extern "C" fn get_timeline_name(ptr: *mut bindings::dma_fence) -> *const c_char {
> +        // SAFETY: The C backend only invokes this callback with `ptr` pointing
> +        // to a valid, unsignaled `bindings::dma_fence`. All fences created in
> +        // this module always reside within `Fence` which always resides in a
> +        // `DriverFenceData`, thus satisfying the function's safety
> +        // requirements.
> +        let fctx = unsafe { Self::from_raw_fence(ptr) };
> +
> +        fctx.timeline_name.as_char_ptr()
> +    }

[Severity: High]
Could this lead to a use-after-free if the consumer Fence outlives its
FenceContext? The Fence object (representing the consumer side of dma_fence) is
refcounted and has no Rust lifetime constraint tying it to FenceContext. It can
be shared with C subsystems or exported to userspace. If the driver drops
DriverFence and subsequently drops FenceContext, the memory for FenceContext is
freed. If an external consumer then queries the fence, the callback casts the
fence pointer and dereferences the now-dangling fctx reference to read the name.

[ ... ]

> +#[pinned_drop]
> +impl<T: FenceContextOps + Send + Sync> PinnedDrop for FenceContext<T> {
> +    fn drop(self: Pin<&mut Self>) {
> +        // Fence ops callbacks can be called on unsignaled fences. Since these
> +        // callbacks can access the fence context and its data, it needs to be
> +        // guaranteed that a context only drops after all associated
> +        // `DriverFence`s have been dropped. This is unlikely to occur, but
> +        // would result in silent UAF. Throw a panic to prevent that.
> +        //
> +        // TODO:
> +        // It would be better if the fence context signals all forgotten fences
> +        // itself. To do so, it would keep a list of unsignaled fences. That
> +        // list's members would have to be pre-allocated (see
> +        // `FenceCallback::new_fence_allocation()`).
> +        if self.nr_of_unsignaled_fences.load(Relaxed) != 0 {
> +            panic!("Forgotten fences in FenceContext.");
> +        }
> +
> +        // Ensure that the driver cannot unload while there are still dma_fence
> +        // callbacks running. At the same time, the RCU barrier addresses the
> +        // problem inherited by the C backend, in which backend ops callbacks
> +        // might be accessing the fence while it is being signaled (or shortly
> +        // after). This could cause UAF access on the fence context's
> +        // `fctx.driver_name` and `fctx.timeline_name`.
> +        //
> +        // Wait for the RCU callbacks in `DriverFence::drop`.
> +        rcu_barrier();
> +    }
> +}

[Severity: High]
Is it safe to call rcu_barrier() during destruction? rcu_barrier() waits for
all pending RCU callbacks, which involves blocking and sleeping. If a driver
stores the FenceContext in a reference-counted container and the last reference
is dropped from an atomic context (such as an interrupt handler or while holding
a spinlock), wouldn't this sleep in atomic context cause a kernel panic?

[ ... ]

> +    unsafe extern "C" fn dma_fence_callback(
> +        _fence: *mut bindings::dma_fence,
> +        callback_foreign: *mut bindings::dma_fence_cb,
> +    ) {
> +        let ptr = Opaque::cast_from(callback_foreign).cast_mut();
> +
> +        // SAFETY: All `cb` we can receive here have been created in such a way
> +        // that they are embedded into a `FenceCallbackRegistration`. The
> +        // backend ensures synchronisation so whoever holds the registration
> +        // object cannot drop it while this code is running. See
> +        // `FenceCallbackRegistration::drop`.
> +        unsafe {
> +            let reg: *mut Self = container_of!(ptr, Self, callback_foreign);
> +
> +            (*reg).callback.on_signal();
> +        }
> +    }

[ ... ]

> +impl Fence {
> +    /// Check whether the fence was signaled at the moment of the function call.
> +    ///
> +    /// Note that this can return `true` for a [`Fence`] whose [`DriverFence`]
> +    /// has not yet been dropped. The reason is that the fence ops callbacks can
> +    /// cause the fence to get signaled by the C backend.
> +    #[inline]
> +    pub fn is_signaled(&self) -> bool {
> +        // We should not use `dma_fence_is_signaled_locked()` here, because
> +        // according to the C backend's recommendations, that function is
> +        // problematic and we should avoid calling that function with a lock
> +        // held.
> +
> +        // SAFETY: Inner `fence` is valid because `self` is valid.
> +        let ret = unsafe { bindings::dma_fence_is_signaled(self.as_raw()) };
> +
> +        // To be as robust as possible for the future we guarantee that an API
> +        // caller can 100% rely on the signaling being completed (i.e., all
> +        // fence callbacks ran), so we have to take the lock.
> +        //
> +        // The reason is that the C dma_fence backend currently does not
> +        // carefully synchronize the `dma_fence_is_signaled()` function with the
> +        // proper spinlock. This can lead to the function returning `true` while
> +        // fence callbacks are still being executed. This can be mitigated by
> +        // guarding the entire function with the spinlock.
> +        //
> +        // The fundamental reason is that the C backend currently does guard
> +        // setting of the fence's signaled-bit with the fence's spinlock, but
> +        // reading is done locklessly.
> +        //
> +        // See commit c8a5d5ea3ba6a.
> +        let _ = self.lock();
> +
> +        ret
> +    }

[Severity: High]
Does this unconditionally acquire the spinlock even though it might already be
held? When a fence is signaled, the C function dma_fence_signal_locked()
iterates over and executes all registered callbacks while actively holding the
fence's spinlock. If the user callback attempts to check the fence's status by
calling Fence::is_signaled() from within on_signal(), it attempts to acquire
the exact same non-recursive spinlock, which would cause a deadlock.

[ ... ]

> +    fn as_raw(&self) -> *mut bindings::dma_fence {
> +        self.inner.get()
> +    }

[Severity: Low]
Should this small getter function be annotated with #[inline] according to the
Rust kernel coding guidelines?

[ ... ]

> +// SAFETY: These implement the C backends refcounting methods which are proven
> +// to work correctly.
> +unsafe impl AlwaysRefCounted for Fence {
> +    fn inc_ref(&self) {
> +        // SAFETY: `self.as_raw()` is a pointer to a valid `struct dma_fence`.
> +        unsafe { bindings::dma_fence_get(self.as_raw()) }
> +    }

[Severity: Low]
Should this small function forwarding to C bindings be annotated with #[inline]
according to the Rust kernel coding guidelines?

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