[PATCH 0/2] dma-fence: Order is-signaled-bit

[PATCH 0/2] dma-fence: Order is-signaled-bit

[Philipp Stanner]

Changes in v2:
  - Add a set_bit_release() function for proper ordering and use it.

Based on the DRM branch drm-misc-next

Philipp Stanner (2):
  asm-generic/bitops: Add set_bit_release()
  dma-buf/dma-fence: Order setting of the signaled-bit properly

 drivers/dma-buf/dma-fence.c                      | 11 +++++++++--
 include/asm-generic/bitops/instrumented-atomic.h | 14 ++++++++++++++
 include/linux/dma-fence.h                        |  6 +++++-
 3 files changed, 28 insertions(+), 3 deletions(-)


base-commit: 1ecab8b0bad393750c60cc3819ce9c256595463c
-- 
2.56.0

[PATCH 1/2] asm-generic/bitops: Add set_bit_release()

[Philipp Stanner]

The kernel already has test_bit_acquire(). A set_bit_release(), however,
was missing so far. It can be expected to be useful for parties that
need release ordering, but not the stricter full ordering of the other
bit-setting functions.

Add set_bit_release().

Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
 include/asm-generic/bitops/instrumented-atomic.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/include/asm-generic/bitops/instrumented-atomic.h b/include/asm-generic/bitops/instrumented-atomic.h
index c010d54275e4..fb2d1472fd4d 100644
--- a/include/asm-generic/bitops/instrumented-atomic.h
+++ b/include/asm-generic/bitops/instrumented-atomic.h
@@ -72,6 +72,20 @@ static __always_inline bool test_and_set_bit(long nr, volatile unsigned long *ad
 	return arch_test_and_set_bit(nr, addr);
 }
 
+/**
+ * set_bit_release - Set a bit with release ordering
+ * @nr: Bit to set
+ * @addr: Address to start counting from
+ *
+ * This is an atomic ordered operation (implied write memory barrier).
+ */
+static __always_inline void set_bit_release(long nr, volatile unsigned long *addr)
+{
+	kcsan_wmb();
+	instrument_atomic_write(addr + BIT_WORD(nr), sizeof(long));
+	arch_set_bit(nr, addr);
+}
+
 /**
  * test_and_clear_bit - Clear a bit and return its old value
  * @nr: Bit to clear
-- 
2.56.0

[PATCH 2/2] dma-buf/dma-fence: Order setting of the signaled-bit properly

[Philipp Stanner]

dma_fence_is_signaled() can return true *while* the fence is still
signaling. This is fragile and has been involved in at least two bugs:

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

The signaled-bit is set under lock-protection but read without it.
Adding the lock has been rejected on the grounds of the lock-acquisition
shooting down cache lines of parties that merely want to check whether
the fence is already signaled.

Make sure the bit can only become visible as 1 once the callbacks have
ran. To achieve this, read the bit with test_bit_acquire() and set it
with set_bit_release().

Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
 drivers/dma-buf/dma-fence.c | 11 +++++++++--
 include/linux/dma-fence.h   |  6 +++++-
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index bd58688b81a7..07d499fa35ac 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -359,8 +359,7 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
 
 	dma_fence_assert_held(fence);
 
-	if (unlikely(test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
-				      &fence->flags)))
+	if (unlikely(dma_fence_test_signaled_flag(fence)))
 		return;
 
 	trace_dma_fence_signaled(fence);
@@ -384,6 +383,14 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
 		INIT_LIST_HEAD(&cur->node);
 		cur->func(fence, cur);
 	}
+
+	/*
+	 * This orders the signaled-state with the test_bit_acquire() in
+	 * dma_fence_test_signaled_flag(). This ensures that functions like
+	 * dma_fence_is_signaled() can only ever return true once signaling is
+	 * completed (i.e., after the callbacks above ran).
+	 */
+	set_bit_release(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags);
 }
 EXPORT_SYMBOL(dma_fence_signal_timestamp_locked);
 
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index dd07d128adc3..61522027af41 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -496,7 +496,11 @@ const char __rcu *dma_fence_timeline_name(struct dma_fence *fence);
 static inline bool
 dma_fence_test_signaled_flag(struct dma_fence *fence)
 {
-	return test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags);
+	/*
+	 * The counterpart of this ordered operation can be found in
+	 * dma_fence_signal_timestamp_locked().
+	 */
+	return test_bit_acquire(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags);
 }
 
 /**
-- 
2.56.0

Re: [PATCH 1/2] asm-generic/bitops: Add set_bit_release()

[Arnd Bergmann]

On Wed, Oct 21, 2026, at 11:55, Philipp Stanner wrote:
> The kernel already has test_bit_acquire(). A set_bit_release(), however,
> was missing so far. It can be expected to be useful for parties that
> need release ordering, but not the stricter full ordering of the other
> bit-setting functions.
>
> Add set_bit_release().
>
> Signed-off-by: Philipp Stanner <phasta@kernel.org>

> +/**
> + * set_bit_release - Set a bit with release ordering
> + * @nr: Bit to set
> + * @addr: Address to start counting from
> + *
> + * This is an atomic ordered operation (implied write memory barrier).
> + */

Can we fix the mismatch between code comments and documentation for
the set_bit()/clear_bit()/change_bit() barrier semantics while adding
this? I'm always confused by this and the new function makes it harder
to understand in the current form:

- Your patch description mentions that set_bit() has strong rw
  barrier semantics and that set_bit_release() would be a weaker
  write-barrier version of it. This makes sense given the
  set_bit() implementation based on strong raw_atomic_long_or()
  in arch_set_bit() and the text in Documentation/memory-barriers.txt

- Contrary to that, the kerneldoc comment over set_bit() mentions
  "This is a relaxed atomic operation (no implied memory barriers)"
  and leaves out the kcsan_mb() that is in test_and_set_bit().
  This makes the new set_bit_release() sound like a /stronger/
  version of set_bit() the same way that test_bit_acquire()
  is a stronger version of the (relaxed) test_bit().

As I understand it, the kerneldoc comments here are incorrect
and trace back to the include/asm-i386/bitops.h file before the
git era. The patch below should make them consistent with your
patch and with the actual implementation.

       Arnd

diff --git a/include/asm-generic/bitops/instrumented-atomic.h b/include/asm-generic/bitops/instrumented-atomic.h
index c010d54275e4..df200802e929 100644
--- a/include/asm-generic/bitops/instrumented-atomic.h
+++ b/include/asm-generic/bitops/instrumented-atomic.h
@@ -18,13 +18,14 @@
  * @nr: the bit to set
  * @addr: the address to start counting from
  *
- * This is a relaxed atomic operation (no implied memory barriers).
+ * This is an atomic fully-ordered operation (implied full memory barrier).
  *
  * Note that @nr may be almost arbitrarily large; this function is not
  * restricted to acting on a single-word quantity.
  */
 static __always_inline void set_bit(long nr, volatile unsigned long *addr)
 {
+	kcsan_mb();
 	instrument_atomic_write(addr + BIT_WORD(nr), sizeof(long));
 	arch_set_bit(nr, addr);
 }
@@ -34,10 +35,11 @@ static __always_inline void set_bit(long nr, volatile unsigned long *addr)
  * @nr: Bit to clear
  * @addr: Address to start counting from
  *
- * This is a relaxed atomic operation (no implied memory barriers).
+ * This is an atomic fully-ordered operation (implied full memory barrier).
  */
 static __always_inline void clear_bit(long nr, volatile unsigned long *addr)
 {
+	kcsan_mb();
 	instrument_atomic_write(addr + BIT_WORD(nr), sizeof(long));
 	arch_clear_bit(nr, addr);
 }
@@ -47,13 +49,14 @@ static __always_inline void clear_bit(long nr, volatile unsigned long *addr)
  * @nr: Bit to change
  * @addr: Address to start counting from
  *
- * This is a relaxed atomic operation (no implied memory barriers).
+ * This is an atomic fully-ordered operation (implied full memory barrier).
  *
  * Note that @nr may be almost arbitrarily large; this function is not
  * restricted to acting on a single-word quantity.
  */
 static __always_inline void change_bit(long nr, volatile unsigned long *addr)
 {
+	kcsan_mb();
 	instrument_atomic_write(addr + BIT_WORD(nr), sizeof(long));
 	arch_change_bit(nr, addr);
 }

Re: [PATCH 1/2] asm-generic/bitops: Add set_bit_release()

[Gary Guo]

On Wed Oct 21, 2026 at 12:33 PM BST, Arnd Bergmann wrote:
> On Wed, Oct 21, 2026, at 11:55, Philipp Stanner wrote:
>> The kernel already has test_bit_acquire(). A set_bit_release(), however,
>> was missing so far. It can be expected to be useful for parties that
>> need release ordering, but not the stricter full ordering of the other
>> bit-setting functions.
>>
>> Add set_bit_release().
>>
>> Signed-off-by: Philipp Stanner <phasta@kernel.org>
>
>> +/**
>> + * set_bit_release - Set a bit with release ordering
>> + * @nr: Bit to set
>> + * @addr: Address to start counting from
>> + *
>> + * This is an atomic ordered operation (implied write memory barrier).
>> + */
>
> Can we fix the mismatch between code comments and documentation for
> the set_bit()/clear_bit()/change_bit() barrier semantics while adding
> this? I'm always confused by this and the new function makes it harder
> to understand in the current form:
>
> - Your patch description mentions that set_bit() has strong rw
>   barrier semantics and that set_bit_release() would be a weaker
>   write-barrier version of it. This makes sense given the
>   set_bit() implementation based on strong raw_atomic_long_or()
>   in arch_set_bit() and the text in Documentation/memory-barriers.txt
>
> - Contrary to that, the kerneldoc comment over set_bit() mentions
>   "This is a relaxed atomic operation (no implied memory barriers)"
>   and leaves out the kcsan_mb() that is in test_and_set_bit().
>   This makes the new set_bit_release() sound like a /stronger/
>   version of set_bit() the same way that test_bit_acquire()
>   is a stronger version of the (relaxed) test_bit().

set_bit has relaxed semantics while test_and_set_bit implies barrier. RISC-V for
exmaple will generate the amoor instruction for set_bit, not amoor.aqrl.

Best,
Gary

>
> As I understand it, the kerneldoc comments here are incorrect
> and trace back to the include/asm-i386/bitops.h file before the
> git era. The patch below should make them consistent with your
> patch and with the actual implementation.
>
>        Arnd

Re: [PATCH 1/2] asm-generic/bitops: Add set_bit_release()

[Gary Guo]

On Wed Oct 21, 2026 at 10:55 AM BST, Philipp Stanner wrote:
> The kernel already has test_bit_acquire(). A set_bit_release(), however,
> was missing so far. It can be expected to be useful for parties that
> need release ordering, but not the stricter full ordering of the other
> bit-setting functions.
>
> Add set_bit_release().
>
> Signed-off-by: Philipp Stanner <phasta@kernel.org>
> ---
>  include/asm-generic/bitops/instrumented-atomic.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/include/asm-generic/bitops/instrumented-atomic.h b/include/asm-generic/bitops/instrumented-atomic.h
> index c010d54275e4..fb2d1472fd4d 100644
> --- a/include/asm-generic/bitops/instrumented-atomic.h
> +++ b/include/asm-generic/bitops/instrumented-atomic.h
> @@ -72,6 +72,20 @@ static __always_inline bool test_and_set_bit(long nr, volatile unsigned long *ad
>  	return arch_test_and_set_bit(nr, addr);
>  }
>  
> +/**
> + * set_bit_release - Set a bit with release ordering
> + * @nr: Bit to set
> + * @addr: Address to start counting from
> + *
> + * This is an atomic ordered operation (implied write memory barrier).
> + */
> +static __always_inline void set_bit_release(long nr, volatile unsigned long *addr)
> +{
> +	kcsan_wmb();
> +	instrument_atomic_write(addr + BIT_WORD(nr), sizeof(long));
> +	arch_set_bit(nr, addr);

This is just wrong. There's no code here that gives you the release semantics.

kcsan_wmb() is just an annotation and does nothing. wmb is also not release
semantics as it does not order LOAD->STORE.

Given that set_bit is a relaxed RMW atomic, you can implement set_bit_release
with just

    smp_mb__before_atomic();
    set_bit(nr, addr);

Which should be correct, but is too strong on some architectures (like RISC-V),
becuase it will produce a full MB instead. So we can either add release barriers
(hint: I really like it), or we can add arch_set_bit_release and implement it
per arch; on RISC-V for example it can then be translated to amoor.rl (which is
not being produced by any existing bit ops).c

Best,
Gary

> +}
> +
>  /**
>   * test_and_clear_bit - Clear a bit and return its old value
>   * @nr: Bit to clear

Re: [PATCH 1/2] asm-generic/bitops: Add set_bit_release()

[Arnd Bergmann]

On Wed, Oct 21, 2026, at 13:54, Gary Guo wrote:
> On Wed Oct 21, 2026 at 12:33 PM BST, Arnd Bergmann wrote:

>> I'm always confused by this and the new function makes it harder
>> to understand in the current form:
...
>> - Contrary to that, the kerneldoc comment over set_bit() mentions
>>   "This is a relaxed atomic operation (no implied memory barriers)"
>>   and leaves out the kcsan_mb() that is in test_and_set_bit().
>>   This makes the new set_bit_release() sound like a /stronger/
>>   version of set_bit() the same way that test_bit_acquire()
>>   is a stronger version of the (relaxed) test_bit().
>
> set_bit has relaxed semantics while test_and_set_bit implies barrier. RISC-V for
> exmaple will generate the amoor instruction for set_bit, not amoor.aqrl.

Thanks for the clarification, I did misread the implementation
and see how I was confused once more.

     Arnd

Re: [PATCH 1/2] asm-generic/bitops: Add set_bit_release()

[Philipp Stanner]

On Wed, 2026-10-21 at 13:06 +0100, Gary Guo wrote:


[…]

> > +/**
> > + * set_bit_release - Set a bit with release ordering
> > + * @nr: Bit to set
> > + * @addr: Address to start counting from
> > + *
> > + * This is an atomic ordered operation (implied write memory barrier).
> > + */
> > +static __always_inline void set_bit_release(long nr, volatile unsigned long *addr)
> > +{
> > +	kcsan_wmb();
> > +	instrument_atomic_write(addr + BIT_WORD(nr), sizeof(long));
> > +	arch_set_bit(nr, addr);
> 
> This is just wrong. There's no code here that gives you the release semantics.
> 
> kcsan_wmb() is just an annotation and does nothing. wmb is also not release
> semantics as it does not order LOAD->STORE.

What are the other functions here then even about?

/**
 * test_and_set_bit - Set a bit and return its old value
 * @nr: Bit to set
 * @addr: Address to count from
 *
 * This is an atomic fully-ordered operation (implied full memory barrier).
 */
static __always_inline bool test_and_set_bit(long nr, volatile unsigned long *addr)
{
	kcsan_mb();
	instrument_atomic_read_write(addr + BIT_WORD(nr), sizeof(long));
	return arch_test_and_set_bit(nr, addr);
}

Where does the "full ordering" come from? I suppose implied barrier
then means it comes from the architecture's implementation. But what's
then the purpose of kcsan_XX ?

> Given that set_bit is a relaxed RMW atomic, you can implement set_bit_release
> with just
> 
>     smp_mb__before_atomic();
>     set_bit(nr, addr);
> 
> Which should be correct, but is too strong on some architectures (like RISC-V),
> becuase it will produce a full MB instead. So we can either add release barriers
> (hint: I really like it), or we can add arch_set_bit_release and implement it
> per arch; on RISC-V for example it can then be translated to amoor.rl (which is
> not being produced by any existing bit ops).c


All fine by me.


P.

Re: [PATCH 1/2] asm-generic/bitops: Add set_bit_release()

[Gary Guo]

On Wed Oct 21, 2026 at 1:17 PM BST, Philipp Stanner wrote:
> On Wed, 2026-10-21 at 13:06 +0100, Gary Guo wrote:
>
>
> […]
>
>> > +/**
>> > + * set_bit_release - Set a bit with release ordering
>> > + * @nr: Bit to set
>> > + * @addr: Address to start counting from
>> > + *
>> > + * This is an atomic ordered operation (implied write memory barrier).
>> > + */
>> > +static __always_inline void set_bit_release(long nr, volatile unsigned long *addr)
>> > +{
>> > +	kcsan_wmb();
>> > +	instrument_atomic_write(addr + BIT_WORD(nr), sizeof(long));
>> > +	arch_set_bit(nr, addr);
>> 
>> This is just wrong. There's no code here that gives you the release semantics.
>> 
>> kcsan_wmb() is just an annotation and does nothing. wmb is also not release
>> semantics as it does not order LOAD->STORE.
>
> What are the other functions here then even about?
>
> /**
>  * test_and_set_bit - Set a bit and return its old value
>  * @nr: Bit to set
>  * @addr: Address to count from
>  *
>  * This is an atomic fully-ordered operation (implied full memory barrier).
>  */
> static __always_inline bool test_and_set_bit(long nr, volatile unsigned long *addr)
> {
> 	kcsan_mb();
> 	instrument_atomic_read_write(addr + BIT_WORD(nr), sizeof(long));
> 	return arch_test_and_set_bit(nr, addr);
> }
>
> Where does the "full ordering" come from? I suppose implied barrier
> then means it comes from the architecture's implementation. But what's
> then the purpose of kcsan_XX ?

It's from arch_test_and_set_bit.

KCSAN doesn't see arch code, so the kcsan_mb() and in fact
instrumnent_atomic_read_write() both are annotations to indicate that this is
conceptually a mb() + STORE for instrumentation purpose.

Best,
Gary

Re: [PATCH 1/2] asm-generic/bitops: Add set_bit_release()

[Philipp Stanner]

On Wed, 2026-10-21 at 13:24 +0100, Gary Guo wrote:
> On Wed Oct 21, 2026 at 1:17 PM BST, Philipp Stanner wrote:

[…]

> > static __always_inline bool test_and_set_bit(long nr, volatile unsigned long *addr)
> > {
> > 	kcsan_mb();
> > 	instrument_atomic_read_write(addr + BIT_WORD(nr), sizeof(long));
> > 	return arch_test_and_set_bit(nr, addr);
> > }
> > 
> > Where does the "full ordering" come from? I suppose implied barrier
> > then means it comes from the architecture's implementation. But what's
> > then the purpose of kcsan_XX ?
> 
> It's from arch_test_and_set_bit.
> 
> KCSAN doesn't see arch code, so the kcsan_mb() and in fact
> instrumnent_atomic_read_write() both are annotations to indicate that this is
> conceptually a mb() + STORE for instrumentation purpose.


We (i.e., someone who's got the overview) could probably document where
and why our atomic instructions are a bit scattered.

Why is test_bit_acquire() defined in include/linux/bitops.h, and should
set_bit_release() be defined there – or here?


P.

Re: [PATCH 2/2] dma-buf/dma-fence: Order setting of the signaled-bit properly

[Christian König]

On 10/21/26 11:55, Philipp Stanner wrote:
> dma_fence_is_signaled() can return true *while* the fence is still
> signaling. This is fragile and has been involved in at least two bugs:
> 
> 	commit c8a5d5ea3ba6 ("nouveau: fix client work fence deletion race")
> 	commit 7c703a7d3f2b ("drm/amdgpu: Fix one use-after-free of VM")
> 
> The signaled-bit is set under lock-protection but read without it.
> Adding the lock has been rejected on the grounds of the lock-acquisition
> shooting down cache lines of parties that merely want to check whether
> the fence is already signaled.
> 
> Make sure the bit can only become visible as 1 once the callbacks have
> ran. To achieve this, read the bit with test_bit_acquire() and set it
> with set_bit_release().

That has been suggested and rejected before.

Background is that the signaling bit must be visible before the callbacks are run or things like dma_fence_default_wait() (as an example) just break.

Regards,
Christian.

> 
> Signed-off-by: Philipp Stanner <phasta@kernel.org>
> ---
>  drivers/dma-buf/dma-fence.c | 11 +++++++++--
>  include/linux/dma-fence.h   |  6 +++++-
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index bd58688b81a7..07d499fa35ac 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -359,8 +359,7 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>  
>  	dma_fence_assert_held(fence);
>  
> -	if (unlikely(test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> -				      &fence->flags)))
> +	if (unlikely(dma_fence_test_signaled_flag(fence)))
>  		return;
>  
>  	trace_dma_fence_signaled(fence);
> @@ -384,6 +383,14 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>  		INIT_LIST_HEAD(&cur->node);
>  		cur->func(fence, cur);
>  	}
> +
> +	/*
> +	 * This orders the signaled-state with the test_bit_acquire() in
> +	 * dma_fence_test_signaled_flag(). This ensures that functions like
> +	 * dma_fence_is_signaled() can only ever return true once signaling is
> +	 * completed (i.e., after the callbacks above ran).
> +	 */
> +	set_bit_release(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags);
>  }
>  EXPORT_SYMBOL(dma_fence_signal_timestamp_locked);
>  
> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index dd07d128adc3..61522027af41 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -496,7 +496,11 @@ const char __rcu *dma_fence_timeline_name(struct dma_fence *fence);
>  static inline bool
>  dma_fence_test_signaled_flag(struct dma_fence *fence)
>  {
> -	return test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags);
> +	/*
> +	 * The counterpart of this ordered operation can be found in
> +	 * dma_fence_signal_timestamp_locked().
> +	 */
> +	return test_bit_acquire(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags);
>  }
>  
>  /**

Re: [PATCH 2/2] dma-buf/dma-fence: Order setting of the signaled-bit properly

[Philipp Stanner]

On Wed, 2026-10-21 at 15:47 +0200, Christian König wrote:
> On 10/21/26 11:55, Philipp Stanner wrote:
> > dma_fence_is_signaled() can return true *while* the fence is still
> > signaling. This is fragile and has been involved in at least two bugs:
> > 
> > 	commit c8a5d5ea3ba6 ("nouveau: fix client work fence deletion race")
> > 	commit 7c703a7d3f2b ("drm/amdgpu: Fix one use-after-free of VM")
> > 
> > The signaled-bit is set under lock-protection but read without it.
> > Adding the lock has been rejected on the grounds of the lock-acquisition
> > shooting down cache lines of parties that merely want to check whether
> > the fence is already signaled.
> > 
> > Make sure the bit can only become visible as 1 once the callbacks have
> > ran. To achieve this, read the bit with test_bit_acquire() and set it
> > with set_bit_release().
> 
> That has been suggested and rejected before.
> 
> Background is that the signaling bit must be visible before the
> callbacks are run or things like dma_fence_default_wait() (as an
> example) just break.

dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
{
	struct default_wait_cb cb;
	unsigned long flags;
	signed long ret = timeout ? timeout : 1;

	dma_fence_lock_irqsave(fence, flags);

	if (dma_fence_test_signaled_flag(fence))
		goto out;

	if (intr && signal_pending(current)) {
		ret = -ERESTARTSYS;
		goto out;
	}

	if (!timeout) {
		ret = 0;
		goto out;
	}

	cb.base.func = dma_fence_default_wait_cb;
	cb.task = current;
	list_add(&cb.base.node, &fence->cb_list);

	while (!dma_fence_test_signaled_flag(fence) && ret > 0) {
		if (intr)
			__set_current_state(TASK_INTERRUPTIBLE);
		else
			__set_current_state(TASK_UNINTERRUPTIBLE);
		dma_fence_unlock_irqrestore(fence, flags);


That function evaluates the flag under lock-protection, with full
ordering. The bit is only ever set under lock-protection, with full
ordering.

So currently, the bit is 1 before the callbacks ran. It becomes 1
before the callbacks ran, but dma_fence_default_wait() can only see
that after it took the lock, and it can only do that after the lock is
released after the callback ran.

So behavior for this function is unchanged, unless I miss sth.

Actually, this barrier makes things more robust, since things like

if (dma_fence_is_signaled(f))
  ts = dma_fence_timestamp(f)

could misorder the accesses to the signaled-bit.


Do you have another example of what would break?


You see, the bit sometimes being locked and sometimes being lockless
causes confusion.


P.

Re: [PATCH 2/2] dma-buf/dma-fence: Order setting of the signaled-bit properly

[Christian König]

On 10/21/26 16:52, Philipp Stanner wrote:
> On Wed, 2026-10-21 at 15:47 +0200, Christian König wrote:
>> On 10/21/26 11:55, Philipp Stanner wrote:
>>> dma_fence_is_signaled() can return true *while* the fence is still
>>> signaling. This is fragile and has been involved in at least two bugs:
>>>
>>> 	commit c8a5d5ea3ba6 ("nouveau: fix client work fence deletion race")
>>> 	commit 7c703a7d3f2b ("drm/amdgpu: Fix one use-after-free of VM")
>>>
>>> The signaled-bit is set under lock-protection but read without it.
>>> Adding the lock has been rejected on the grounds of the lock-acquisition
>>> shooting down cache lines of parties that merely want to check whether
>>> the fence is already signaled.
>>>
>>> Make sure the bit can only become visible as 1 once the callbacks have
>>> ran. To achieve this, read the bit with test_bit_acquire() and set it
>>> with set_bit_release().
>>
>> That has been suggested and rejected before.
>>
>> Background is that the signaling bit must be visible before the
>> callbacks are run or things like dma_fence_default_wait() (as an
>> example) just break.
> 
> dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
> {
> 	struct default_wait_cb cb;
> 	unsigned long flags;
> 	signed long ret = timeout ? timeout : 1;
> 
> 	dma_fence_lock_irqsave(fence, flags);
> 
> 	if (dma_fence_test_signaled_flag(fence))
> 		goto out;
> 
> 	if (intr && signal_pending(current)) {
> 		ret = -ERESTARTSYS;
> 		goto out;
> 	}
> 
> 	if (!timeout) {
> 		ret = 0;
> 		goto out;
> 	}
> 
> 	cb.base.func = dma_fence_default_wait_cb;
> 	cb.task = current;
> 	list_add(&cb.base.node, &fence->cb_list);
> 
> 	while (!dma_fence_test_signaled_flag(fence) && ret > 0) {
> 		if (intr)
> 			__set_current_state(TASK_INTERRUPTIBLE);
> 		else
> 			__set_current_state(TASK_UNINTERRUPTIBLE);
> 		dma_fence_unlock_irqrestore(fence, flags);
> 
> 
> That function evaluates the flag under lock-protection, with full
> ordering. The bit is only ever set under lock-protection, with full
> ordering.
> 
> So currently, the bit is 1 before the callbacks ran. It becomes 1
> before the callbacks ran, but dma_fence_default_wait() can only see
> that after it took the lock, and it can only do that after the lock is
> released after the callback ran.
> 
> So behavior for this function is unchanged, unless I miss sth.

Yeah that argument came up before and no it is not correct.

> Actually, this barrier makes things more robust, since things like
> 
> if (dma_fence_is_signaled(f))
>   ts = dma_fence_timestamp(f)
> 
> could misorder the accesses to the signaled-bit.
> 
> 
> Do you have another example of what would break?

Please search the mailing list. We had the same patch like three or four times already.

It was even applied one IIRC, but yes it immediately breaks things.

Regards,
Christian.

> 
> 
> You see, the bit sometimes being locked and sometimes being lockless
> causes confusion.
> 
> 
> P.