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

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

[Philipp Stanner]

Many users of dma_fence do things like:

// Thread A
dma_fence_set_error(f, -EINVAL);
dma_fence_signal(f); // <- Takes lock, which orders against the above.

// Thread B
if (dma_fence_is_signaled(f)) // <- Does test_bit(), no ordering.
	pr_info("err: %i\n", f->error); // <- Can be loaded first.

There is no memory ordering between reading of the signaled-bit and
reading of the error code.

Though dma_fence_get_status() does take the fence lock, it is equally
broken because the misordering can simply happen within the critical
section I think?

(dma_fence_is_signaled() shouldn't even exist for users btw, a user
should always only call a working, ordered dma_fence_get_status() so he
cannot that easily ignore the error code

Likewise, dma_fence_set_error() should not exist since it massively
incentivizes messing up. I suppose the _locked functions came to be to
fix that up in hindsight?).

Since I want to work towards a dma_fence implementation where users do
not take the fence lock manually anymore, the proposed fix is: Order
dma_fence_is_signaled() with the lock, and
dma_fence_is_signaled_locked() with a barrier.

In the mid-term, we would deprecate all XXX_locked functions. The
self-locking fence API can satisfy all user requirements with the
inline-lock.

Ordering dma_fence_is_signaled() with the spinlock instead of a barrier
also has the advantage of protecting against frequent bugs where people
run into UAF because they got their refcounting wrong and rely on the
fence being freeable once the signaled-bit lights up:

	commit c8a5d5ea3ba6 ("nouveau: fix client work fence deletion race")
	commit 7c703a7d3f2b ("drm/amdgpu: Fix one use-after-free of VM")

This related pattern works:

	dma_fence_lock_irqsave(f, flags);
	dma_fence_set_error(f, -EINVAL);
	dma_fence_signal_locked(f); // uses test_and_set_bit(), which is strongly ordered
	dma_fence_unlock_irqrestore(f, flags);

This is all very subtle and implicit.

Regardless, I think we really need to establish what the signaled-bit
really means and who is responsible where for its evaluation semantics.

Lucas recently (rightfully) pointed out that the signaled-bit originally
was supposed to *only* mean that the hardware is done. But this is just
not the API reality anymore, where we have to use the signaled-state for
driver-unload and for correct evaluation of error codes.

Note at this point my proposal to transition fence to a stable state
where these subtleties disappear. The time window to add this change
right now is unusually good because the inline_lock is not used by
anyone yet except for our Rust abstractions, so we can get things right
without breaking old drivers.

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

Fixes: d6c99f4bf093 ("dma-fence: Wrap querying the fence->status")
Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
Regarding ordering between

dma_fence_get_status()

and

dma_fence_set_error() followed by
dma_fence_signal()

Would probably be good if those highly experienced with ordering (Paul,
Gary) can share their opinion whether there's actually a problem that
can occurr in reality.
---
 include/linux/dma-fence.h | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index dd07d128adc3..fe6fbc5a5a35 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();
@@ -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);
-- 
2.56.0

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

[sashiko-bot]

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