Re: [PATCH v7 4/5] 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] Infinite loop under spinlock in `remove_from_fctx_list`.
- [Critical] IRQ-context Deadlock due to using non-IRQ-safe `SpinLock` for `fctx.fences`.
- [High] Use-After-Free in dma_fence_ops callbacks (get_driver_name / get_timeline_name).
- [High] Memory leak of `DriverFenceData` and list nodes when implicitly dropped.
- [Low] Compilation Error (use-after-move) in `FenceCallbackRegistration::new`.
--

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

rust: Add dma_fence abstractions

This commit introduces Rust abstractions for the C dma_fence framework.
It provides DriverFence and Fence types to decouple fence issuers and
consumers, along with a FenceContext for synchronization.

> 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
[ ... ]
> +pub struct FenceContext<T: FenceContextOps + Send + Sync> {
> +    /// The fence context number.
> +    nr: u64,
[ ... ]
> +    #[pin]
> +    fences: SpinLock<List<FenceLastResortSignaler>>,

[Severity: Critical]
Can this lead to an IRQ-context deadlock?

If a process-context thread acquires the lock during DriverFence creation,
and a hardware interrupt fires on the same CPU before the lock is released:

Process context:
fctx.fence_alloc()
    new_fence()
        fctx.fences.lock()

IRQ context on same CPU:
GPU job completes
    DriverFence::signal()
        remove_from_fctx_list()
            fctx.fences.lock()

It seems the interrupt handler will attempt to acquire the already-held
lock. Since SpinLock resolves to spin_lock() which leaves interrupts
enabled, would this cause the CPU to deadlock?

[ ... ]
> +    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]
Could this result in a use-after-free?

If the device is removed and the FenceContext is dropped, userspace could
still hold a reference to the fence via a sync_file file descriptor. If
userspace then calls the SYNC_IOC_FILE_INFO ioctl, the C backend would
unconditionally invoke get_driver_name.

It appears fctx would point to freed memory, causing a use-after-free when
accessing driver_name.

[ ... ]
> +    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]
Does this have the same use-after-free issue as get_driver_name above?

[ ... ]
> +    pub fn new<'a>(fence: &'a Fence, callback: T) -> impl PinInit<Self, CallbackError<T>> + 'a
> +    where
> +        T: 'a,
> +    {
> +        try_pin_init!(Self {
> +            // We need to fully initialize the fence because after
> +            // `dma_fence_add_callback()` ran, the callback might immediately
> +            // get invoked.
> +            callback: ManuallyDrop::new(callback),
> +            fence: ARef::from(fence),
> +            callback_foreign <- Opaque::try_ffi_init(|ptr| {
[ ... ]
> +                match ret {
> +                    Ok(()) => Ok(()),
> +                    Err(e) => {
> +                        // SAFETY: We could not register the callback. Thus,
> +                        // C will not use it. So we can just take it back
> +                        // and pass it to the user again.
> +                        let cb_back = unsafe { ManuallyDrop::take(callback) };

[Severity: Low]
Does this compile successfully?

It looks like the callback parameter is moved into ManuallyDrop::new(), and
then later accessed inside the try_ffi_init closure. This appears to
violate ownership rules with a use-after-move. Additionally, wouldn't
ManuallyDrop::take() expect a mutable reference rather than the underlying
type?

[ ... ]
> +    fn remove_from_fctx_list(&self) {
> +        // SAFETY: `self.data` is valid because `self` was validly initialized.
> +        let mut guard = unsafe { self.data.as_ref().fctx.fences.lock() };
> +        let mut cursor = guard.cursor_front();
> +
> +        while let Some(pos) = cursor.peek_next() {
> +            // Fences in this list are always initialized, so `unwrap` is OK.
> +            if self.as_fence().get_seqno() == pos.fence.as_ref().unwrap().get_seqno() {
> +                let _ = pos.remove();
> +            }
> +        }
> +    }

[Severity: Critical]
Can this cause an infinite loop?

If the current element's seqno does not match the signaled fence, the code
takes no action and fails to advance the cursor. The next iteration would
peek at the exact same element indefinitely, leading to a lockup under the
spinlock.

Should there be a cursor.move_next() call when the sequence number does not
match?

[ ... ]
> +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 warn_on!(!signaled) {
> +            // 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()) };
> +        }
> +        drop(guard);
> +
> +        // SAFETY: Valid because `self` is valid.
> +        let rcu_head_ptr = unsafe { &raw mut (*self.data.as_ptr()).rcu_head };
[ ... ]
> +        unsafe { bindings::call_rcu(rcu_head_ptr, Some(drop_driver_fence_data::<T>)) };
> +    }
> +}

[Severity: High]
Does implicitly dropping a DriverFence leak the tracking nodes?

The drop implementation orchestrates the destruction and signals the fence,
but it seems it does not call self.remove_from_fctx_list().

If the driver implicitly drops the DriverFence on an error path without
explicitly calling signal(), the tracking node would remain in the
fctx.fences list indefinitely. This holds an ARef<Fence> that keeps the C
dma_fence refcount elevated, preventing the memory from being freed.

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