Re: [PATCH 1/2] rust: DmaFence: Don't drop on signal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] rust: dma_fence: premature unsignaled count decrement in signal()
--

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

rust: DmaFence: Don't drop on signal

This commit changes DriverFence::signal() in rust/kernel/dma_buf/dma_fence.rs
to take a shared reference instead of consuming self, allowing fences to be
signaled without immediately dropping the DriverFence.

> diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
> index a1d851656562d..3022bdb5f7c3e 100644
> --- a/rust/kernel/dma_buf/dma_fence.rs
> +++ b/rust/kernel/dma_buf/dma_fence.rs
> @@ -805,7 +805,7 @@ pub fn as_fence(&self) -> &Fence {
>      }
>  
>      /// Signal the fence. This will invoke all registered callbacks.
> -    pub fn signal(self, res: Result) {
> +    pub fn signal(&self, res: Result) {
>          let fence = self.as_fence().lock();
>  
>          // SAFETY: `fence` is valid because `self` is valid. The lock must be
> @@ -816,14 +816,15 @@ pub fn signal(self, res: Result) {
>                  // must not have been signaled yet, which we check directly above.
>                  unsafe { bindings::dma_fence_set_error(fence.as_raw(), err.to_errno()) };
>              }
> +
> +            // 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);
> +
>              // SAFETY: `fence` is valid because `self` is valid. The lock must
>              // be held, which we acquired above.
>              unsafe { bindings::dma_fence_signal_locked(fence.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);
>      }
>  }

[Severity: High]
Since DriverFence::signal() no longer consumes self and no longer runs
DriverFence::drop(), can decrementing fctx.nr_of_unsignaled_fences here allow
FenceContext::drop() to free FenceContext too early?

In FenceContext::drop(), nr_of_unsignaled_fences is checked before calling
rcu_barrier() to wait for the RCU callbacks queued by DriverFence::drop():

rust/kernel/dma_buf/dma_fence.rs:FenceContext::drop() {
    if self.nr_of_unsignaled_fences.load(Relaxed) != 0 {
        panic!("Forgotten fences in FenceContext.");
    }
    ...
    // Wait for the RCU callbacks in `DriverFence::drop`.
    rcu_barrier();
}

Because DriverFence::signal() now takes &self and does not drop the fence,
DriverFence::drop() has not queued call_rcu(rcu_head_ptr,
Some(drop_driver_fence_data::<T>)) when signal() returns.

If a DriverFence is signaled via DriverFence::signal() and then forgotten in
safe Rust via core::mem::forget() (or held in foreign storage via
ForeignOwnable::into_foreign()) when FenceContext is dropped,
FenceContext::drop() will observe nr_of_unsignaled_fences == 0 without
panicking. It then runs rcu_barrier(), which returns without waiting for an
RCU grace period after signaling because no RCU callback was ever queued by
DriverFence::drop(). Concurrent RCU readers in dma_fence_driver_name() or
dma_fence_timeline_name() that raced with signaling could then read freed
fctx.driver_name and fctx.timeline_name memory, or a later
DriverFence::drop() on a foreign-owned fence could access a dangling fctx
reference and queue call_rcu() after FenceContext::drop() has already
completed.

Additionally, fctx.nr_of_unsignaled_fences.fetch_sub(1, Relaxed) now runs
before bindings::dma_fence_signal_locked(fence.as_raw()). During
dma_fence_signal_locked(), trace_dma_fence_signaled() invokes
FenceContext::get_driver_name() and FenceContext::get_timeline_name(), which
dereference fctx. If a foreign-owned DriverFence is signaled via
DriverFence::borrow().signal() concurrently with FenceContext::drop(), could
FenceContext::drop() see nr_of_unsignaled_fences == 0 and free FenceContext
before dma_fence_signal_locked() even finishes?

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