Re: [PATCH v8 4/5] rust: Add dma_fence abstractions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Soundness bug in `ForeignOwnable` allows safe lifetime erasure, leading to UAF of `FenceContext`.
- [High] `Fence` objects safely outlive their `FenceContext`, leading to UAF in C callbacks.
- [High] Unconditional system-wide `rcu_barrier()` on `FenceContext` drop causes severe latency spikes.
- [High] Calling `Fence::is_signaled()` from a fence callback causes a hard deadlock.
- [Medium] `FenceContext::drop` does not signal leaked fences, causing permanent deadlocks for consumers.
- [Medium] `nr_of_unsignaled_fences` counter leaks if a fence is signaled via the C API.
--

--- Patch [4]: [PATCH v8 4/5] rust: Add dma_fence abstractions ---
commit baeb68fb999484f7d372bacbf53bbc08faf26f2e
Author: Philipp Stanner <phasta@kernel.org>
rust: Add dma_fence abstractions

This commit adds abstractions for dma_fence in Rust. It provides DriverFence 
and Fence types to cleanly decouple fence issuers and consumers, along with 
a FenceContext to handle sequencing.

> diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
> new file mode 100644
> index 0000000000000..d6322e062faff
> --- /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: High]
Can Fence objects safely outlive their FenceContext, leading to a 
use-after-free here?

It looks like Fence implements AlwaysRefCounted but lacks a lifetime 
parameter connecting it to its FenceContext. If a user drops the DriverFence 
and then drops the FenceContext while the ARef to the Fence remains alive, 
callbacks like get_driver_name could access a dangling fctx pointer.

Could this happen if a consumer accesses the exported fence, for example 
through debugfs reading the fence driver name or through SYNC_IOC_FILE_INFO, 
triggering the C backend to call this function on a signaled fence?

[ ... ]
> +#[pinned_drop]
> +impl<T: FenceContextOps + Send + Sync> PinnedDrop for FenceContext<T> {
> +    fn drop(self: Pin<&mut Self>) {
> +        if self.nr_of_unsignaled_fences.load(Relaxed) > 0 {
> +            pr_err!("Unsignaled DriverFence(s) left in FenceContext. UAF possible.\n");
> +        }
> +
> +        // TODO:
> +        // It would be even more robust if the fence context signals all
> +        // forgotten fences itself. To do so, it would keep a list of unsignaled
> +        // fences. That list members would have to be pre-allocated (see
> +        // `FenceCallback::fence_alloc()`).

[Severity: Medium]
Will this cause waiting consumers to hang indefinitely?

The commit message notes that as a safety measure, if a FenceContext drops 
with an unsignaled fence present, it will signal it to ensure full 
decoupling. However, this code leaves a TODO and only prints a warning 
instead of actually signaling the leaked fences.

Does this mean that if a fence is forgotten without being signaled, any 
kernel threads waiting on it will enter a permanent deadlock?

> +
> +        rcu_barrier();
> +    }
> +}

[Severity: High]
Could calling rcu_barrier() unconditionally here cause severe latency spikes?

Since rcu_barrier() blocks the current thread until all pending call_rcu() 
callbacks across the entire kernel have completed, using it for regular 
object destruction seems problematic.

If GPU drivers frequently create and destroy fence contexts dynamically 
(such as per-process or per-timeline), could destroying a context become a 
denial of service vector by repeatedly blocking kernel threads?

[ ... ]
> +    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: `fence` stems from `self`, which is valid by definition.
> +        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]
Does acquiring the lock here cause a hard deadlock if called from a callback?

The is_signaled method intentionally acquires the fence spinlock to guarantee 
signaling completion. However, the C backend executes callbacks registered via 
FenceCallbackRegistration with this exact same spinlock already held.

If a user calls fence.is_signaled() from within their callback implementation, 
would it attempt to recursively acquire the non-recursive Linux spinlock?

[ ... ]
> +unsafe impl<T: Send + Sync + FenceContextOps + 'static> ForeignOwnable for DriverFence<'_, T> {
> +    type Borrowed<'a>
> +        = DriverFenceBorrow<'a, T>
> +    where
> +        Self: 'a;
> +    type BorrowedMut<'a>
> +        = DriverFenceBorrow<'a, T>
> +    where
> +        Self: 'a;
> +
> +    const FOREIGN_ALIGN: usize = core::mem::align_of::<bindings::dma_fence>();
> +
> +    fn into_foreign(self) -> *mut c_void {
> +        let fence = self;
> +
> +        let ptr = fence.as_raw();
> +
> +        // DriverFence must not drop.
> +        let _ = ManuallyDrop::new(fence);
> +
> +        ptr.cast()
> +    }

[Severity: High]
Does this implementation allow safe lifetime erasure of DriverFence?

The ForeignOwnable trait is implemented for DriverFence regardless of its 
lifetime. Because into_foreign is a safe method that returns a raw pointer, 
it completely erases the lifetime parameter of DriverFence.

Could safe Rust code call fence.into_foreign(), successfully ending the 
borrow of FenceContext from the compiler's perspective without dropping the 
fence? This might allow the FenceContext to be safely dropped, invalidating 
the fctx pointer and leading to a use-after-free when the kernel later 
accesses the exported fence.

[ ... ]
> +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.deref()) };
> +        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.deref(), 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.deref()) };
> +
> +            // 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: Medium]
Can the nr_of_unsignaled_fences counter leak if the fence is signaled 
externally?

The logic here only decrements the counter if dma_fence_test_signaled_flag 
returns false. If another subsystem signals the fence through the C API, the 
signaled flag is set.

When this DriverFence is subsequently dropped, it appears it would skip the 
decrement, permanently leaking the counter. Would this cause false-positive 
memory corruption warnings later when the FenceContext is dropped?

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