Re: [RFC PATCH] dma-buf/dma-fence: Close race window against error checks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Critical] dma-buf/dma-fence: recursive lock deadlock in
  dma_fence_is_signaled()
- [Critical] dma-buf/dma-fence: extern_lock UAF in
  dma_fence_is_signaled() after signal
- [Medium] dma-buf/dma-fence: redundant IO rmb() in
  dma_fence_is_signaled_locked()
--

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

dma-buf/dma-fence: Close race window against error checks

This commit adds an rmb() to dma_fence_is_signaled_locked() and acquires the
fence spinlock in dma_fence_is_signaled() to order signaled-flag checks
against subsequent reads of fence->error.

Link: https://lore.kernel.org/dri-devel/20261020100034.759183-2-phasta@kernel.org/

> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index dd07d128adc3b..fe6fbc5a5a35d 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -517,8 +517,18 @@ static inline bool
>  dma_fence_is_signaled_locked(struct dma_fence *fence)
>  {
>  	const struct dma_fence_ops *ops;
> +	bool signaled;
>  
> -	if (dma_fence_test_signaled_flag(fence))
> +	signaled = dma_fence_test_signaled_flag(fence);
> +	/*
> +	 * Whoever holds the lock might next try to obtain the fence's error
> +	 * code - but there is no ordering between reading the signaled flag
> +	 * and the reading of the error code.
> +	 *
> +	 * Order properly with rmb() since fences might be involved in IO.
> +	 */
> +	rmb();
> +	if (signaled)
>  		return true;
>  
>  	rcu_read_lock();

[Severity: Medium]
Why is a mandatory I/O rmb() executed unconditionally here, even when
signaled is false and the caller already holds the fence spinlock?

Because dma_fence_signal_timestamp_locked() sets DMA_FENCE_FLAG_SIGNALED_BIT
with test_and_set_bit() under the same spinlock, a reader holding that lock
is already ordered against any prior dma_fence_set_error() once the bit is
observed set, and running rmb() before checking if (signaled) adds a heavy
I/O barrier to every unsignaled check in dma_fence_get_status().  Meanwhile,
because dma_fence_test_signaled_flag() still uses relaxed test_bit() instead
of test_bit_acquire(), lockless callers such as the -ENOENT fast path in
dma_fence_add_callback() remain unordered when callers like
dma_fence_array_enable_signaling() immediately read fence->error.

> @@ -553,9 +563,17 @@ static inline bool
>  dma_fence_is_signaled(struct dma_fence *fence)
>  {
>  	const struct dma_fence_ops *ops;
> +	unsigned long flags;
>  
> -	if (dma_fence_test_signaled_flag(fence))
> +	/*
> +	 * Take the lock to order against people reading the fence's error code.
> +	 */
> +	dma_fence_lock_irqsave(fence, flags);
> +	if (dma_fence_test_signaled_flag(fence)) {
> +		dma_fence_unlock_irqrestore(fence, flags);
>  		return true;
> +	}
> +	dma_fence_unlock_irqrestore(fence, flags);
>  
>  	rcu_read_lock();
>  	ops = rcu_dereference(fence->ops);

[Severity: Critical]
Does taking the fence spinlock in dma_fence_is_signaled() cause a recursive
spinlock deadlock when signaling is enabled on an AMDKFD eviction fence?

dma_fence_enable_signaling() and dma_fence_add_callback() acquire the fence
spinlock before calling __dma_fence_enable_signaling(), which invokes
ops->enable_signaling() with the lock held and interrupts disabled:

  dma_fence_enable_signaling()
    dma_fence_lock_irqsave(fence, flags)
    __dma_fence_enable_signaling(fence)
      amdkfd_fence_enable_signaling(fence)
        dma_fence_is_signaled(fence)
          dma_fence_lock_irqsave(fence, flags)  <-- spins forever

drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c:amdkfd_fence_enable_signaling() {
	struct amdgpu_amdkfd_fence *fence = to_amdgpu_amdkfd_fence(f);

	if (!fence)
		return false;

	if (dma_fence_is_signaled(f))
		return true;
    ...
}

Any wait, callback registration, or TTM eviction on an unsignaled AMDKFD
fence will lock up the CPU.

[Severity: Critical]
Can taking the fence spinlock before testing DMA_FENCE_FLAG_SIGNALED_BIT
cause a use-after-free on fence->extern_lock when querying an already
signaled fence?

As documented in "Safe external access to driver provided object members" in
include/linux/dma-fence.h, data not embedded in struct dma_fence, including
the external spinlock passed to dma_fence_init(), must not be accessed after
the fence has signaled because drivers may free the lock container once the
fence signals and an RCU grace period elapses:

  CPU0                              CPU1
  ffs_dmabuf_signal_done()
    dma_fence_signal(&fence->base)
  ffs_dmabuf_detach()
    ...
      ffs_dmabuf_release()
        kfree(priv)
                                    dma_resv_add_fence()
                                      dma_fence_is_signaled(fence)
                                        dma_fence_lock_irqsave(fence, flags)
                                          <-- use-after-free on priv->lock

Drivers such as f_fs, panthor, xe, and msm allocate the spinlock in a driver
object that can be freed after signaling while references to the signaled
struct dma_fence remain in dma_resv or sync_file.

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