Re: [RFC PATCH v5 3/6] rust: DmaFence: Don't drop on signal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] dma_fence: counter underflow in DriverFence::signal on multiple calls
- [High] dma_fence: sleep-in-atomic bug when dropping DriverFence under spinlock
--

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

rust: DmaFence: Don't drop on signal

This commit decouples the dropping of a DriverFence from its signaling,
changing the signal() method to take &self rather than consuming the fence.
This allows the fence to be signaled multiple times and defers its
destruction to when the object goes out of scope. The intent is to avoid
dropping fences in an atomic context where deferred deletion is problematic.

> diff --git a/rust/kernel/dma_buf/dma_fence.rs b/rust/kernel/dma_buf/dma_fence.rs
> index 0028124e6aa64..b67121c7abd41 100644
> --- a/rust/kernel/dma_buf/dma_fence.rs
> +++ b/rust/kernel/dma_buf/dma_fence.rs
> @@ -806,7 +806,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) {

[Severity: High]
Could the unconditional decrement at the end of this function corrupt the
nr_of_unsignaled_fences counter since signal() can now be called multiple
times?

rust/kernel/dma_buf/dma_fence.rs:DriverFence::signal() {
...
        let fctx = unsafe { self.data.as_ref().fctx };
        let _ = fctx.nr_of_unsignaled_fences.fetch_sub(1, Relaxed);
}

If signal() is called twice on the same fence, it looks like it will
underflow the hazard counter, causing a delayed panic when the FenceContext
is dropped.

[Severity: High]
Will this decoupling of signal() from drop() lead to a sleep-in-atomic bug
later in the series?

Since dropping is now deferred to when the DriverFence goes out of scope,
it appears to conflict with other patches in this series.

In patch 5, call_rcu() is replaced with synchronize_rcu() in
DriverFence::drop():

rust/kernel/dma_buf/dma_fence.rs:DriverFence::drop() {
...
        synchronize_rcu();
...
}

In patch 6, DriverFence is dropped while a SpinLockIrq is held:

rust/kernel/drm/job_queue.rs:JobQueue::complete_jobs_up_to_seqno() {
        let mut jq_locked = jq.lock();
        let mut jq = jq_locked.as_mut().project();

        let mut cursor = jq.running_jobs.cursor_front();

        while let Some(job) = cursor.peek_next() {
            let mut job = job.remove();
            ...
        } // `job` goes out of scope here, triggering DriverFence::drop
}

Dropping the DriverFence will call synchronize_rcu(), which sleeps, leading
to a sleep-in-atomic panic when invoked from the JobQueue lock guard.

>          let fence = self.as_fence().lock();
>  
>          // SAFETY: `fence` is valid because `self` is valid. The lock must be

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