[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);
>  }
>  
>  /**