[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