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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Lifetime erasure via `ForeignOwnable` or `core::mem::forget` on `DriverFence` leads to a Use-After-Free of `FenceCtx`.
- [Medium] `DriverFence::signal` and `DriverFence::drop` omit the required `dma_fence_begin_signalling()` and `dma_fence_end_signalling()` lockdep annotations.
- [Low] `FenceCbRegistration`'s `# Invariants` documentation is outdated and lacks the mandatory `// INVARIANT:` comment upon construction.
--

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

rust: Add dma_fence abstractions

This commit adds Rust abstractions for the dma_fence subsystem, providing
DriverFence and Fence types to cleanly decouple fence issuers and consumers
while ensuring safe reference counting and signaling.

> diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
> --- /dev/null
> +++ b/rust/kernel/dma_buf/dma_fence.rs
[ ... ]
> +/// # Invariants
> +///
> +/// If callback is Some, then cb is registered with the fence and the
> +/// callback hasn't been invoked yet. If None, the callback has been invoked
> +/// or the fence was already signaled when we tried to register.
> +#[pin_data(PinnedDrop)]
> +pub struct FenceCbRegistration<T: FenceCb + 'static> {
> +    #[pin]
> +    callback_foreign: Opaque<bindings::dma_fence_cb>,
> +    callback: ManuallyDrop<T>,

[Severity: Low]
Is this documentation accurate?

The invariants description references Some and None for the callback
field, but the field is defined as ManuallyDrop<T> rather than an Option.

[ ... ]
> +    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),

[Severity: Low]
Are we missing the mandatory INVARIANT comment here?

The Rust subsystem guidelines require an explicit INVARIANT comment when
constructing a struct with an Invariants documentation section, but it
appears to be omitted here.

[ ... ]
> +    pub fn signal(self, res: Result) {
> +        let fence = self.as_raw();
> +        let mut fence_flags: usize = 0;
> +        let flag_ptr = &raw mut fence_flags;
> +
> +        // SAFETY: fence is valid because self is valid. flag_ptr is
> +        // merely a pointer to an integer, which lives as long as this function.
> +        unsafe { bindings::dma_fence_lock_irqsave(fence, flag_ptr) };
> +        // SAFETY: fence is valid because self is valid. The lock must be
> +        // held, which we acquired directly above.
> +        if !unsafe { bindings::dma_fence_test_signaled_flag(fence) } {
> +            if let Err(err) = res {
> +                // SAFETY: fence is valid because self is valid. The fence
> +                // must not have been signaled yet, which we check directly above.
> +                unsafe { bindings::dma_fence_set_error(fence, err.to_errno()) };
> +            }
> +            // SAFETY: fence is valid because self is valid. The lock must
> +            // be held, which we acquired above.
> +            unsafe { bindings::dma_fence_signal_locked(fence) };
> +        }
> +        // SAFETY: fence is valid because self is valid. flag_ptr is
> +        // merely a pointer to an integer, which lives as long as this function.
> +        // The lock must be held, which we acquired above.
> +        unsafe { bindings::dma_fence_unlock_irqrestore(fence, flag_ptr) };
> +    }

[Severity: Medium]
Should this manual signaling path include lockdep annotations?

By open-coding the fence signaling using dma_fence_lock_irqsave() and
dma_fence_signal_locked(), this function omits the
dma_fence_begin_signalling() and dma_fence_end_signalling() calls used in
the equivalent C dma_fence_signal() function.

Without these annotations, lockdep cannot track cross-release dependencies
during fence signaling, which might lead to false negatives when detecting
deadlocks between fence signaling and wait paths.

[ ... ]
> +unsafe impl<'b, T: Send + Sync + FenceCtxOps + 'static> ForeignOwnable for DriverFence<'b, T> {

[Severity: High]
Could this lead to a Use-After-Free if the DriverFence is leaked?

Since DriverFence implements ForeignOwnable without requiring a 'static
lifetime for the 'b parameter, a user can call into_foreign() on a
DriverFence<'a, T> or use core::mem::forget() to leak it.

If the DriverFence is leaked, DriverFence::drop() is never called.
The Rust compiler will then allow the borrowed FenceCtx (tied to
lifetime 'a in DriverFenceData) to be dropped. Once FenceCtx::drop()
completes, the driver_name is freed.

However, the C backend could later interact with the dma_fence, which
is kept alive by ARef<Fence>, and invoke get_driver_name(). This would
dereference the dangling fctx pointer, causing a Use-After-Free.

[ ... ]
> +    fn drop(&mut self) {
> +        let fence = self.as_raw();
> +        let mut fence_flags: usize = 0;
> +        let flag_ptr = &raw mut fence_flags;
> +
> +        // SAFETY: fence is valid until the call_rcu() below. flag_ptr is
> +        // merely a pointer to an integer, which lives as long as this function.
> +        unsafe { bindings::dma_fence_lock_irqsave(fence, flag_ptr) };
> +
> +        // 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: fence is valid until the call_rcu() below.
> +        let signaled: bool = unsafe { bindings::dma_fence_test_signaled_flag(fence) };
> +        if warn_on!(!signaled) {
> +            // SAFETY: fence 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(fence, ECANCELED.to_errno()) };
> +            // SAFETY: fence is valid until the call_rcu() below. The lock must
> +            // be held, which we acquired above.
> +            unsafe { bindings::dma_fence_signal_locked(fence) };
> +        }
> +        // SAFETY: fence is valid until the call_rcu() below. flag_ptr is
> +        // merely a pointer to an integer, which lives as long as this function.
> +        // The lock must be held, which we acquired above.
> +        unsafe { bindings::dma_fence_unlock_irqrestore(fence, flag_ptr) };

[Severity: Medium]
Does this cancellation path also need the lockdep annotations?

Similar to DriverFence::signal(), this open-coded signaling omits the
dma_fence_begin_signalling() and dma_fence_end_signalling() lockdep wrappers,
which prevents cross-release dependency tracking.

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