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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Use-After-Free of FenceContext when a Fence outlives its DriverFence or when `!needs_drop` bypasses RCU synchronization.
- [High] FenceContext will reliably trigger a kernel panic on drop if a DriverFence is signaled by external C code.
- [High] Fence::is_signaled() unconditionally acquires the fence spinlock, leading to spinlock recursion deadlocks if called from a callback.
--

--- Patch [1]: [PATCH v10 1/2] rust: Add dma_fence abstractions ---
commit 4a2030e75f6de53abe7df28e7314d4e3c4ae44a8
Author: Philipp Stanner <[email protected]>

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
> new file mode 100644
> --- /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()
> +    }

[Severity: Critical]
Is it possible for the FenceContext to be accessed after it is freed here?

The C dma_fence API allows callbacks like get_driver_name() to be invoked
at any time as long as the fence is alive (refcount > 0), even on signaled
fences. Because the Rust abstraction explicitly allows a Fence to outlive
its FenceContext, fctx can become a dangling pointer.

A userspace application calling SYNC_IOC_FILE_INFO or accessing debugfs
could trigger this after the driver has unloaded and the FenceContext is
dropped.

[ ... ]
> +    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 signalling 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]
Can this spinlock acquisition cause a recursion deadlock?

The C dma_fence_is_signaled() function is explicitly lockless. By acquiring
the fence spinlock here, this wrapper introduces a blocking operation into
what is supposed to be a lockless API.

If a user registers a FenceCallback that invokes is_signaled() on the same
fence from within on_signal(), it will attempt to acquire the lock. Since
dma_fence_signal executes these callbacks with the spinlock already held,
this will result in an immediate deadlock.

[ ... ]
> +impl<'a, T: Send + Sync + FenceContextOps> Drop for DriverFence<'a, T> {
> +    fn drop(&mut self) {
> +        let guard = self.as_fence().lock();
> +
> +        // 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: `guard` is valid until the `call_rcu()` below.
> +        let signaled: bool = unsafe { bindings::dma_fence_test_signaled_flag(guard.as_raw()) };
> +        if !signaled {
> +            pr_err!("DriverFence drops unsignaled. Danger of memory corruption!\n");
> +            // SAFETY: `guard` 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(guard.as_raw(), ECANCELED.to_errno()) };
> +            // SAFETY: `guard` is valid until the `call_rcu()` below. The lock
> +            // must be held, which we acquired above.
> +            unsafe { bindings::dma_fence_signal_locked(guard.as_raw()) };
> +
> +            // SAFETY: `self.data` is valid because `self` is valid.
> +            let fctx = unsafe { self.data.as_ref().fctx };
> +            let _ = fctx.nr_of_unsignaled_fences.fetch_sub(1, Relaxed);
> +        }
> +        drop(guard);

[Severity: High]
Will this skip the counter decrement if the fence is signaled by external
C code?

If a C subsystem (like the DRM scheduler or a software signaling debugfs
hook) signals the DriverFence, the signaled variable will be true. If the
driver subsequently drops the DriverFence without calling signal(), this
block is skipped and the fetch_sub(1) decrement is permanently lost.

When the FenceContext is later dropped, it will unconditionally panic
because the counter is imbalanced:

rust/kernel/dma_buf/dma_fence.rs:FenceContext::drop() {
    ...
    if self.nr_of_unsignaled_fences.load(Relaxed) != 0 {
        panic!("Forgotten fences in FenceContext.");
    }
    ...
}

> +
> +        // `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.
> +
> +        if !core::mem::needs_drop::<T::FenceDataType>() {
> +            // 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(self.as_raw()) };
> +            return;
> +        }

[Severity: Critical]
Does this optimization bypass the RCU synchronization required for safe
teardown?

If this block returns early, no RCU callback is queued. Consequently, when
the FenceContext drops, its rcu_barrier() call will not wait for a grace
period because there are no callbacks pending.

This allows the FenceContext to be freed immediately while concurrent C
readers holding RCU read locks might still be executing, leading to a
use-after-free.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1