Re: [PATCH v6 5/6] rust: Add dma_fence abstractions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [Critical] Use-After-Free of `FenceContext` in `dma_fence_ops` callbacks because `Fence` outlives the context.
- [Critical] Passing a NULL pointer for the spinlock to `dma_fence_init` violates the C API contract and causes an immediate kernel panic.
- [High] Concurrent allocation failures in `fence_alloc` corrupt the shared sequence number atomic, causing duplicate sequence numbers.
- [High] Using `GFP_KERNEL` while holding a spinlock guard sleeps in atomic context.
- [High] `FenceContext::drop` holds the XArray spinlock while invoking `dma_fence_signal_locked`, risking deadlocks and lock inversions.
- [High] Iterating from 0 to `highest_seqno` under a spinlock in `FenceContext::drop` causes unbounded latency and stalls.
- [High] Casting the 64-bit `seqno` to `usize` for XArray indexing truncates on 32-bit platforms, causing index collisions.
- [High] Implementing `ForeignOwnable` for `DriverFence<'a, T>` erases its lifetime, allowing safe Rust code to trigger UAF.
- [Medium] Dropping a `DriverFence` without explicitly signaling it leaks `DriverFenceData` and the `Fence` in the XArray.
- [Medium] `FenceCallbackRegistration::new` leaks the callback state if `dma_fence_add_callback` returns an error other than `ENOENT`.
--

--- Patch [5]: [PATCH v6 5/6] rust: Add dma_fence abstractions ---
commit df80ab90d41cfd4c4088816a593de340df6f198e
Author: Philipp Stanner <phasta@kernel.org>

rust: Add dma_fence abstractions

This commit adds Rust abstractions for the dma_fence framework. It provides
DriverFence for signaling and Fence for consumption, tying them to a
DmaFenceContext to manage sequence numbering and timelines.

> diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
> index 0000000000000..a7d9e22e7e81e 100644
> --- /dev/null
> +++ b/rust/kernel/dma_buf/dma_fence.rs
> @@ -147,15 +147,15 @@ impl<'a, T: Send + Sync + FenceContextOps> FenceContext<T> {
>      fn get_next_seqno(&self) -> u64 {
>          self.seqno.fetch_add(1, Relaxed)
>      }
>  
>      fn decrement_seqno(&self) {
>          let _ = self.seqno.fetch_sub(1, Relaxed);
>      }
>  

[ ... ]

> @@ -178,14 +178,14 @@ impl<'a, T: Send + Sync + FenceContextOps> FenceContext<T> {
>                  // Take the next seqno right now, so that we can use it for pre-allocating
>                  // an entry in the xarray.
>                  let seqno: u64 = self.get_next_seqno();
>  
>                  match self.fences.reserve(seqno as usize, GFP_KERNEL) {

[Severity: High]
Does casting the 64-bit seqno to usize for XArray indexing cause truncation
on 32-bit platforms?

DMA fence sequence numbers are u64 to prevent wrap-around. When the sequence
number exceeds u32::MAX, the cast to usize on 32-bit platforms truncates the
value. Will this overwrite existing unsignaled fences in the XArray and cause
fatal corruption of the fence tracking mechanism?

>                      Ok(reservation) => Ok(DriverFenceAllocation {
>                          data,
>                          seqno,
>                          reservation,
>                          ops: &Self::OPS,
>                      }),
>                      Err(e) => {
>                          self.decrement_seqno();

[Severity: High]
Does rolling back the sequence number corrupt the timeline?

If the reservation fails, the code calls self.decrement_seqno(), which executes
fetch_sub(1, Relaxed).

If Thread B acquires seqno 2, and Thread A (which acquired seqno 1) fails and
decrements the counter back to 1, won't a subsequent Thread C be assigned seqno
2, creating duplicate sequence numbers and breaking the DMA fence timeline
guarantees?

>                          Err(e)
>                      }
>                  }
>              }

[ ... ]

> @@ -189,14 +189,14 @@ impl<'a, T: Send + Sync + FenceContextOps> FenceContext<T> {
>      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: Critical]
Can this code introduce a use-after-free risk?

If a Fence is exported to userspace (for example via a sync_file), userspace
could call SYNC_IOC_FILE_INFO on the descriptor after the driver's job is
complete and the FenceContext has been dropped.

This triggers the C backend to call fence->ops->get_driver_name. The Rust
callback retrieves the FenceContext reference from the DriverFenceData and
dereferences fctx.driver_name.

Since FenceContext::drop does not wait for the Fence refcount to drop to 0,
does this result in a direct read from freed memory?

[ ... ]

> @@ -248,15 +248,15 @@ impl<T: FenceContextOps + Send + Sync> PinnedDrop for FenceContext<T> {
>      fn drop(self: Pin<&mut Self>) {
>          let highest_seqno = self.get_current_seqno();
>  
>          let fences_guard = self.fences.lock();
>  
>          // TODO: Implement a public iterator for XArray and use it here.
>          for seqno in 0..highest_seqno {

[Severity: High]
Could iterating from 0 to highest_seqno under a spinlock cause unbounded
latency and stalls?

FenceContext::drop linearly loops from 0 to highest_seqno, performing an XArray
lookup on every iteration while holding the fences_guard spinlock.

Since drivers often initialize FenceContext with a very large initial_seqno,
could this loop run billions of times with preemption disabled?

>              if let Some(fence) = fences_guard.get(seqno as usize) {
>                  let fence = fence.lock();
>  
>                  // SAFETY: We still hold a reference, thus the pointer is valid.
>                  let signaled: bool =
>                      unsafe { bindings::dma_fence_test_signaled_flag(*fence.deref()) };
>                  if !signaled {
>                      // SAFETY: We still hold a reference, thus the pointer is
>                      // valid. The fence must not have been signaled yet, which we
>                      // check directly above. The lock must be held, which we
>                      // acquired above.
>                      unsafe { bindings::dma_fence_set_error(*fence.deref(), ECANCELED.to_errno()) };
>                      // SAFETY: We still hold a reference, thus the pointer is
>                      // valid. The lock must be held, which we acquired above.
>                      unsafe { bindings::dma_fence_signal_locked(*fence.deref()) };

[Severity: High]
Is it safe to invoke dma_fence_signal_locked while holding the subsystem
internal XArray lock?

While holding the fences_guard spinlock, the code calls
bindings::dma_fence_signal_locked. This synchronously invokes any registered
dma_fence_ops callbacks.

If any callback attempts to interact with the same FenceContext, or attempts
to take a driver lock that is held elsewhere while a fence is signaled, won't
this guarantee an ABBA deadlock?

>                  } else {
>                      pr_warn!("Signaled Fences in FenceContext.");
>                  }
>              }

[ ... ]

> @@ -357,11 +357,11 @@ impl<T: FenceCallback> FenceCallbackRegistration<T> {
>                              // and pass it to the user again.
>                              let cb_back = unsafe { ManuallyDrop::take(callback) };
>                              Err(CallbackError::AlreadySignaled(cb_back))
>                          } else {
>                              Err(CallbackError::Other(e))
>                          }

[Severity: Medium]
Does this code leak the callback state if dma_fence_add_callback returns
an error other than ENOENT?

The user's callback is moved into a ManuallyDrop<T>. If the C backend fails
with an error other than ENOENT, the error is returned directly.

Because the callback is never extracted from ManuallyDrop on this error path,
is it permanently leaked when the partially initialized struct is cleaned up?

[ ... ]

> @@ -668,14 +668,14 @@ impl<'a, T: Send + Sync + FenceContextOps> DriverFenceAllocation<'a, T> {
>      pub fn new_fence(self) -> DriverFence<'a, T> {
>          // We feed the C dma_fence backend a NULL for the spinlock so that it
>          // uses per-fence locks automatically.
>          let null_ptr: *mut bindings::spinlock = ptr::null_mut();
>          let fence_ptr = self.as_raw();
>          // SAFETY: fence_ptr has been created directly above. It will live
>          // at least as long as Self. The same applies to &Self::OPS.
>          unsafe {
>              bindings::dma_fence_init(fence_ptr, self.ops, null_ptr, self.data.fctx.nr, self.seqno)
>          };

[Severity: Critical]
Will this hardcoded NULL pointer passed directly to the C initialization
function cause an immediate kernel panic?

The C implementation in drivers/dma-buf/dma-fence.c starts with BUG_ON(!lock).
Does passing ptr::null_mut() here violate the C API contract and crash the
system upon fence creation?

>  
>          let fence = ARef::from(&self.data.inner);
>          let mut fences_guard = self.data.fctx.fences.lock();
>          // Can never be an error because we already allocated an entry for
>          // exactly this sequence number.
>          // TODO: write some sort of XArray::replace() function or mechanism for
>          // reserving entries so that this can obviously never fail.
>          if fences_guard
>              .store(self.seqno as usize, fence, GFP_KERNEL)
>              .is_err()
>          {

[Severity: High]
Is it safe to use GFP_KERNEL while holding a spinlock guard?

The code acquires the XArray spinlock via self.data.fctx.fences.lock(), and
then calls fences_guard.store() with GFP_KERNEL.

Since GFP_KERNEL implies __GFP_DIRECT_RECLAIM, which allows the allocation to
sleep, doesn't this strictly violate atomic context constraints and risk system
deadlocks?

[ ... ]

> @@ -768,7 +768,7 @@ impl<'a, T: Send + Sync + FenceContextOps> Deref for DriverFenceBorrow<'a, T> {
>      }
>  }
>  
>  // SAFETY: The Rust dma_fence abstractions are already designed around the inner
>  // C dma_fence, which can serve safely as the identification point when being
>  // owned by C. Moreover, safety is ensured by not dropping DriverFence and by
>  // only allowing operations without side effects on the Borrowed type.
>  unsafe impl<T: Send + Sync + FenceContextOps + 'static> ForeignOwnable for DriverFence<'_, T> {

[Severity: High]
Does implementing ForeignOwnable for DriverFence erase its lifetime?

The DriverFence type relies on its lifetime to ensure it cannot outlive its
associated FenceContext. Implementing ForeignOwnable for DriverFence allows
safe Rust code to convert the object into a raw pointer.

When it is later reconstructed from the raw pointer, the compiler cannot enforce
the lifetime dependency. Could this allow it to outlive the FenceContext and
trigger a use-after-free when accessing the context to signal the fence?

[ ... ]

> @@ -833,14 +833,14 @@ 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 };

[Severity: Medium]
Should this drop implementation clean up the fctx.fences XArray entry?

If a DriverFence drops without being explicitly signaled first, this code
signals it with -ECANCELED and schedules RCU destruction, but fails to remove
the fence from the XArray.

Will the XArray continue to hold an ARef<Fence>, keeping the dma_fence
reference count above zero indefinitely and causing a permanent memory leak?

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