[RFC PATCH] dma-buf/dma_fence: Make races for dma_fence_is_signaled() less likely

[RFC PATCH] dma-buf/dma_fence: Make races for dma_fence_is_signaled() less likely

[Philipp Stanner]

dma_fence_is_signaled() returns whether a fence has been signaled
already. That function contains a fast path opportunistic check which is
not guarded by the lock and, according to Christian, cannot be guarded
by the lock without causing a massive performance regression.

This now means that dma_fence_is_signaled() can return true WHILE the
fence callbacks are still being executed. This is razy and has lead to
at least one bug solved in:

commit c8a5d5ea3ba6 ("nouveau: fix client work fence deletion race")

Make this race impossible, by simply setting the bit only once the
callbacks are actually completed.

Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
 drivers/dma-buf/dma-fence.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index c7ea1e75d38a..2416cc86ce93 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -359,8 +359,19 @@ 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)))
+	/*
+	 * First test the bit, so we don't signal an already signaled fence again.
+	 * The lock protects against multiple parties setting the bit. The bit
+	 * is then set at the end of the function.
+	 *
+	 * The background is that there is a fast path check in
+	 * dma_fence_is_signaled() which does not use lock protection and can
+	 * return true *while* the fence callbacks are still executing.
+	 *
+	 * This fast path check supposedly cannot be guarded by the lock because
+	 * of significant performance regressions.
+	 */
+	if (unlikely(test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)))
 		return;
 
 	trace_dma_fence_signaled(fence);
@@ -384,6 +395,9 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
 		INIT_LIST_HEAD(&cur->node);
 		cur->func(fence, cur);
 	}
+
+	// TODO: we need some barrier here, don't we?
+	set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags);
 }
 EXPORT_SYMBOL(dma_fence_signal_timestamp_locked);
 
-- 
2.54.0

Re: [RFC PATCH] dma-buf/dma_fence: Make races for dma_fence_is_signaled() less likely

[Christian König]

On 6/12/26 12:42, Philipp Stanner wrote:
> dma_fence_is_signaled() returns whether a fence has been signaled
> already. That function contains a fast path opportunistic check which is
> not guarded by the lock and, according to Christian, cannot be guarded
> by the lock without causing a massive performance regression.
> 
> This now means that dma_fence_is_signaled() can return true WHILE the
> fence callbacks are still being executed. This is razy and has lead to
> at least one bug solved in:
> 
> commit c8a5d5ea3ba6 ("nouveau: fix client work fence deletion race")
> 
> Make this race impossible, by simply setting the bit only once the
> callbacks are actually completed.

Groundhog day, that has been suggested before and it simply doesn't work.

The flag is intentional set before calling the callbacks because the state needs to be visible.

Just see dma_fence_default_wait() for an example why that approach doesn't work.

Regards,
Christian.

> 
> Signed-off-by: Philipp Stanner <phasta@kernel.org>
> ---
>  drivers/dma-buf/dma-fence.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index c7ea1e75d38a..2416cc86ce93 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -359,8 +359,19 @@ 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)))
> +	/*
> +	 * First test the bit, so we don't signal an already signaled fence again.
> +	 * The lock protects against multiple parties setting the bit. The bit
> +	 * is then set at the end of the function.
> +	 *
> +	 * The background is that there is a fast path check in
> +	 * dma_fence_is_signaled() which does not use lock protection and can
> +	 * return true *while* the fence callbacks are still executing.
> +	 *
> +	 * This fast path check supposedly cannot be guarded by the lock because
> +	 * of significant performance regressions.
> +	 */
> +	if (unlikely(test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)))
>  		return;
>  
>  	trace_dma_fence_signaled(fence);
> @@ -384,6 +395,9 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>  		INIT_LIST_HEAD(&cur->node);
>  		cur->func(fence, cur);
>  	}
> +
> +	// TODO: we need some barrier here, don't we?
> +	set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags);
>  }
>  EXPORT_SYMBOL(dma_fence_signal_timestamp_locked);
>  

Re: [RFC PATCH] dma-buf/dma_fence: Make races for dma_fence_is_signaled() less likely

[Gary Guo]

On Fri Jun 12, 2026 at 11:42 AM BST, Philipp Stanner wrote:
> dma_fence_is_signaled() returns whether a fence has been signaled
> already. That function contains a fast path opportunistic check which is
> not guarded by the lock and, according to Christian, cannot be guarded
> by the lock without causing a massive performance regression.
>
> This now means that dma_fence_is_signaled() can return true WHILE the
> fence callbacks are still being executed. This is razy and has lead to
> at least one bug solved in:
>
> commit c8a5d5ea3ba6 ("nouveau: fix client work fence deletion race")
>
> Make this race impossible, by simply setting the bit only once the
> callbacks are actually completed.
>
> Signed-off-by: Philipp Stanner <phasta@kernel.org>
> ---
>  drivers/dma-buf/dma-fence.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index c7ea1e75d38a..2416cc86ce93 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -359,8 +359,19 @@ 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)))
> +	/*
> +	 * First test the bit, so we don't signal an already signaled fence again.
> +	 * The lock protects against multiple parties setting the bit. The bit
> +	 * is then set at the end of the function.
> +	 *
> +	 * The background is that there is a fast path check in
> +	 * dma_fence_is_signaled() which does not use lock protection and can
> +	 * return true *while* the fence callbacks are still executing.
> +	 *
> +	 * This fast path check supposedly cannot be guarded by the lock because
> +	 * of significant performance regressions.
> +	 */
> +	if (unlikely(test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)))
>  		return;
>  
>  	trace_dma_fence_signaled(fence);
> @@ -384,6 +395,9 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>  		INIT_LIST_HEAD(&cur->node);
>  		cur->func(fence, cur);
>  	}
> +
> +	// TODO: we need some barrier here, don't we?

You want a release barrier here, which we don't have currently, but you can use
`smp_mb__before_atomic`.

However note that barriers must be paired, so in order for the added barrier to
do anything, dma_fence_is_signaled lockless fast path would need to have an
acquire barrier, which we also don't have, so you need a `smp_mb()` there after
the flag test.

Best,
Gary

> +	set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags);
>  }
>  EXPORT_SYMBOL(dma_fence_signal_timestamp_locked);
>  

Re: [RFC PATCH] dma-buf/dma_fence: Make races for dma_fence_is_signaled() less likely

[Philipp Stanner]

On Mon, 2026-06-15 at 11:53 +0200, Christian König wrote:
> On 6/12/26 12:42, Philipp Stanner wrote:
> > dma_fence_is_signaled() returns whether a fence has been signaled
> > already. That function contains a fast path opportunistic check which is
> > not guarded by the lock and, according to Christian, cannot be guarded
> > by the lock without causing a massive performance regression.
> > 
> > This now means that dma_fence_is_signaled() can return true WHILE the
> > fence callbacks are still being executed. This is razy and has lead to
> > at least one bug solved in:
> > 
> > commit c8a5d5ea3ba6 ("nouveau: fix client work fence deletion race")
> > 
> > Make this race impossible, by simply setting the bit only once the
> > callbacks are actually completed.
> 
> Groundhog day, that has been suggested before and it simply doesn't work.
> 
> The flag is intentional set before calling the callbacks because the state needs to be visible.

It will be visible. Just later.

> 
> Just see dma_fence_default_wait() for an example why that approach doesn't work.

What's the issue? It will be set. Just later. Who is ordering with
whom?


I BTW suggest to write more code comments in the future to document all
these supposed pitfalls for those who will hack on that code base once
we have left.


P.

> 
> Regards,
> Christian.
> 
> > 
> > Signed-off-by: Philipp Stanner <phasta@kernel.org>
> > ---
> >  drivers/dma-buf/dma-fence.c | 18 ++++++++++++++++--
> >  1 file changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > index c7ea1e75d38a..2416cc86ce93 100644
> > --- a/drivers/dma-buf/dma-fence.c
> > +++ b/drivers/dma-buf/dma-fence.c
> > @@ -359,8 +359,19 @@ 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)))
> > +	/*
> > +	 * First test the bit, so we don't signal an already signaled fence again.
> > +	 * The lock protects against multiple parties setting the bit. The bit
> > +	 * is then set at the end of the function.
> > +	 *
> > +	 * The background is that there is a fast path check in
> > +	 * dma_fence_is_signaled() which does not use lock protection and can
> > +	 * return true *while* the fence callbacks are still executing.
> > +	 *
> > +	 * This fast path check supposedly cannot be guarded by the lock because
> > +	 * of significant performance regressions.
> > +	 */
> > +	if (unlikely(test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)))
> >  		return;
> >  
> >  	trace_dma_fence_signaled(fence);
> > @@ -384,6 +395,9 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
> >  		INIT_LIST_HEAD(&cur->node);
> >  		cur->func(fence, cur);
> >  	}
> > +
> > +	// TODO: we need some barrier here, don't we?
> > +	set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags);
> >  }
> >  EXPORT_SYMBOL(dma_fence_signal_timestamp_locked);
> >  

Re: [RFC PATCH] dma-buf/dma_fence: Make races for dma_fence_is_signaled() less likely

[Christian König]

On 6/15/26 12:04, Philipp Stanner wrote:
> On Mon, 2026-06-15 at 11:53 +0200, Christian König wrote:
>> On 6/12/26 12:42, Philipp Stanner wrote:
>>> dma_fence_is_signaled() returns whether a fence has been signaled
>>> already. That function contains a fast path opportunistic check which is
>>> not guarded by the lock and, according to Christian, cannot be guarded
>>> by the lock without causing a massive performance regression.
>>>
>>> This now means that dma_fence_is_signaled() can return true WHILE the
>>> fence callbacks are still being executed. This is razy and has lead to
>>> at least one bug solved in:
>>>
>>> commit c8a5d5ea3ba6 ("nouveau: fix client work fence deletion race")
>>>
>>> Make this race impossible, by simply setting the bit only once the
>>> callbacks are actually completed.
>>
>> Groundhog day, that has been suggested before and it simply doesn't work.
>>
>> The flag is intentional set before calling the callbacks because the state needs to be visible.
> 
> It will be visible. Just later.

It must be visible *before* the callbacks are called. The whole idea with the callbacks is that you can install a notification of state change.

>> Just see dma_fence_default_wait() for an example why that approach doesn't work.
> 
> What's the issue? It will be set. Just later. Who is ordering with
> whom?

See the functions dma_fence_default_wait() and dma_fence_default_wait_cb().

It wakes up the sleeping thread which in turn needs to observes the new state.

Regards,
Christian.

> I BTW suggest to write more code comments in the future to document all
> these supposed pitfalls for those who will hack on that code base once
> we have left.
> 
> 
> P.
> 
>>
>> Regards,
>> Christian.
>>
>>>
>>> Signed-off-by: Philipp Stanner <phasta@kernel.org>
>>> ---
>>>  drivers/dma-buf/dma-fence.c | 18 ++++++++++++++++--
>>>  1 file changed, 16 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
>>> index c7ea1e75d38a..2416cc86ce93 100644
>>> --- a/drivers/dma-buf/dma-fence.c
>>> +++ b/drivers/dma-buf/dma-fence.c
>>> @@ -359,8 +359,19 @@ 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)))
>>> +	/*
>>> +	 * First test the bit, so we don't signal an already signaled fence again.
>>> +	 * The lock protects against multiple parties setting the bit. The bit
>>> +	 * is then set at the end of the function.
>>> +	 *
>>> +	 * The background is that there is a fast path check in
>>> +	 * dma_fence_is_signaled() which does not use lock protection and can
>>> +	 * return true *while* the fence callbacks are still executing.
>>> +	 *
>>> +	 * This fast path check supposedly cannot be guarded by the lock because
>>> +	 * of significant performance regressions.
>>> +	 */
>>> +	if (unlikely(test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)))
>>>  		return;
>>>  
>>>  	trace_dma_fence_signaled(fence);
>>> @@ -384,6 +395,9 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>>>  		INIT_LIST_HEAD(&cur->node);
>>>  		cur->func(fence, cur);
>>>  	}
>>> +
>>> +	// TODO: we need some barrier here, don't we?
>>> +	set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags);
>>>  }
>>>  EXPORT_SYMBOL(dma_fence_signal_timestamp_locked);
>>>  

Re: [RFC PATCH] dma-buf/dma_fence: Make races for dma_fence_is_signaled() less likely

[Philipp Stanner]

On Mon, 2026-06-15 at 12:09 +0200, Christian König wrote:
> On 6/15/26 12:04, Philipp Stanner wrote:
> > On Mon, 2026-06-15 at 11:53 +0200, Christian König wrote:
> > > On 6/12/26 12:42, Philipp Stanner wrote:
> > > > dma_fence_is_signaled() returns whether a fence has been signaled
> > > > already. That function contains a fast path opportunistic check which is
> > > > not guarded by the lock and, according to Christian, cannot be guarded
> > > > by the lock without causing a massive performance regression.
> > > > 
> > > > This now means that dma_fence_is_signaled() can return true WHILE the
> > > > fence callbacks are still being executed. This is razy and has lead to
> > > > at least one bug solved in:
> > > > 
> > > > commit c8a5d5ea3ba6 ("nouveau: fix client work fence deletion race")
> > > > 
> > > > Make this race impossible, by simply setting the bit only once the
> > > > callbacks are actually completed.
> > > 
> > > Groundhog day, that has been suggested before and it simply doesn't work.
> > > 
> > > The flag is intentional set before calling the callbacks because the state needs to be visible.
> > 
> > It will be visible. Just later.
> 
> It must be visible *before* the callbacks are called. The whole idea with the callbacks is that you can install a notification of state change.
> 
> > > Just see dma_fence_default_wait() for an example why that approach doesn't work.
> > 
> > What's the issue? It will be set. Just later. Who is ordering with
> > whom?
> 
> See the functions dma_fence_default_wait() and dma_fence_default_wait_cb().
> 
> It wakes up the sleeping thread which in turn needs to observes the new state.

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);   // <------- cool, a lock! ^_^

	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);    // <--------------- guarded by lock

	while (!dma_fence_test_signaled_flag(fence) && ret > 0) { // <-------- flag-check is guarded by lock. Fully ordered. Doesn't matter where
		if (intr)                                                      dma_fence_signal_timeout_locked() sets the bit.
			__set_current_state(TASK_INTERRUPTIBLE);
		else
			__set_current_state(TASK_UNINTERRUPTIBLE);
		dma_fence_unlock_irqrestore(fence, flags); // <------------ dma_fence_signal_timeout_locked() can change the flag only after here

		ret = schedule_timeout(ret);

		dma_fence_lock_irqsave(fence, flags);
		if (ret > 0 && intr && signal_pending(current))
			ret = -ERESTARTSYS;
	}


P.

> 
> Regards,
> Christian.
> 
> > I BTW suggest to write more code comments in the future to document all
> > these supposed pitfalls for those who will hack on that code base once
> > we have left.
> > 
> > 
> > P.
> > 
> > > 
> > > Regards,
> > > Christian.
> > > 
> > > > 
> > > > Signed-off-by: Philipp Stanner <phasta@kernel.org>
> > > > ---
> > > >  drivers/dma-buf/dma-fence.c | 18 ++++++++++++++++--
> > > >  1 file changed, 16 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > > > index c7ea1e75d38a..2416cc86ce93 100644
> > > > --- a/drivers/dma-buf/dma-fence.c
> > > > +++ b/drivers/dma-buf/dma-fence.c
> > > > @@ -359,8 +359,19 @@ 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)))
> > > > +	/*
> > > > +	 * First test the bit, so we don't signal an already signaled fence again.
> > > > +	 * The lock protects against multiple parties setting the bit. The bit
> > > > +	 * is then set at the end of the function.
> > > > +	 *
> > > > +	 * The background is that there is a fast path check in
> > > > +	 * dma_fence_is_signaled() which does not use lock protection and can
> > > > +	 * return true *while* the fence callbacks are still executing.
> > > > +	 *
> > > > +	 * This fast path check supposedly cannot be guarded by the lock because
> > > > +	 * of significant performance regressions.
> > > > +	 */
> > > > +	if (unlikely(test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)))
> > > >  		return;
> > > >  
> > > >  	trace_dma_fence_signaled(fence);
> > > > @@ -384,6 +395,9 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
> > > >  		INIT_LIST_HEAD(&cur->node);
> > > >  		cur->func(fence, cur);
> > > >  	}
> > > > +
> > > > +	// TODO: we need some barrier here, don't we?
> > > > +	set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags);
> > > >  }
> > > >  EXPORT_SYMBOL(dma_fence_signal_timestamp_locked);
> > > >  

Re: [RFC PATCH] dma-buf/dma_fence: Make races for dma_fence_is_signaled() less likely

[Tvrtko Ursulin]

On 12/06/2026 11:42, Philipp Stanner wrote:
> dma_fence_is_signaled() returns whether a fence has been signaled
> already. That function contains a fast path opportunistic check which is
> not guarded by the lock and, according to Christian, cannot be guarded
> by the lock without causing a massive performance regression.
> 
> This now means that dma_fence_is_signaled() can return true WHILE the
> fence callbacks are still being executed. This is razy and has lead to
> at least one bug solved in:
> 
> commit c8a5d5ea3ba6 ("nouveau: fix client work fence deletion race")
> 
> Make this race impossible, by simply setting the bit only once the
> callbacks are actually completed.

My 2c is that I would be wary of trying to turn around the semantics of 
the callbacks. They are not "run these to make the fence signaled" but 
instead "let me know please when fence is signaled". I do not see a good 
justification for the change.

The bugs such as above quoted c8a5d5ea3ba6 are in fact more about 
nouveau itself pulling data underneath fences with callbacks and not a 
bug in dma-fence. With the opportunistic cleanup it is doing in 
nouveau_cli_work() perhaps it would want to use 
dma_fence_remove_callback() to appear somewhat sensible, although that 
too may be racy, I did not think about it. Anyway, I've sent a patch to 
make that one clearer and more documented, even faster for the 
unsignaled case.

Regards,

Tvrtko

> 
> Signed-off-by: Philipp Stanner <phasta@kernel.org>
> ---
>   drivers/dma-buf/dma-fence.c | 18 ++++++++++++++++--
>   1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index c7ea1e75d38a..2416cc86ce93 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -359,8 +359,19 @@ 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)))
> +	/*
> +	 * First test the bit, so we don't signal an already signaled fence again.
> +	 * The lock protects against multiple parties setting the bit. The bit
> +	 * is then set at the end of the function.
> +	 *
> +	 * The background is that there is a fast path check in
> +	 * dma_fence_is_signaled() which does not use lock protection and can
> +	 * return true *while* the fence callbacks are still executing.
> +	 *
> +	 * This fast path check supposedly cannot be guarded by the lock because
> +	 * of significant performance regressions.
> +	 */
> +	if (unlikely(test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)))
>   		return;
>   
>   	trace_dma_fence_signaled(fence);
> @@ -384,6 +395,9 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>   		INIT_LIST_HEAD(&cur->node);
>   		cur->func(fence, cur);
>   	}
> +
> +	// TODO: we need some barrier here, don't we?
> +	set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags);
>   }
>   EXPORT_SYMBOL(dma_fence_signal_timestamp_locked);
>   

Re: [RFC PATCH] dma-buf/dma_fence: Make races for dma_fence_is_signaled() less likely

[Philipp Stanner]

On Mon, 2026-06-15 at 10:56 +0100, Gary Guo wrote:
> On Fri Jun 12, 2026 at 11:42 AM BST, Philipp Stanner wrote:
> > dma_fence_is_signaled() returns whether a fence has been signaled
> > already. That function contains a fast path opportunistic check which is
> > not guarded by the lock and, according to Christian, cannot be guarded
> > by the lock without causing a massive performance regression.
> > 
> > This now means that dma_fence_is_signaled() can return true WHILE the
> > fence callbacks are still being executed. This is razy and has lead to
> > at least one bug solved in:
> > 
> > commit c8a5d5ea3ba6 ("nouveau: fix client work fence deletion race")
> > 
> > Make this race impossible, by simply setting the bit only once the
> > callbacks are actually completed.
> > 
> > Signed-off-by: Philipp Stanner <phasta@kernel.org>
> > ---
> >  drivers/dma-buf/dma-fence.c | 18 ++++++++++++++++--
> >  1 file changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > index c7ea1e75d38a..2416cc86ce93 100644
> > --- a/drivers/dma-buf/dma-fence.c
> > +++ b/drivers/dma-buf/dma-fence.c
> > @@ -359,8 +359,19 @@ 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)))
> > +	/*
> > +	 * First test the bit, so we don't signal an already signaled fence again.
> > +	 * The lock protects against multiple parties setting the bit. The bit
> > +	 * is then set at the end of the function.
> > +	 *
> > +	 * The background is that there is a fast path check in
> > +	 * dma_fence_is_signaled() which does not use lock protection and can
> > +	 * return true *while* the fence callbacks are still executing.
> > +	 *
> > +	 * This fast path check supposedly cannot be guarded by the lock because
> > +	 * of significant performance regressions.
> > +	 */
> > +	if (unlikely(test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)))
> >  		return;
> >  
> >  	trace_dma_fence_signaled(fence);
> > @@ -384,6 +395,9 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
> >  		INIT_LIST_HEAD(&cur->node);
> >  		cur->func(fence, cur);
> >  	}
> > +
> > +	// TODO: we need some barrier here, don't we?
> 
> You want a release barrier here, which we don't have currently, but you can use
> `smp_mb__before_atomic`.
> 
> However note that barriers must be paired, so in order for the added barrier to
> do anything, dma_fence_is_signaled lockless fast path would need to have an
> acquire barrier, which we also don't have, so you need a `smp_mb()` there after
> the flag test.

Thx for the explanations.

I tend to conclude that this solution is far too fragile and without
even further synchronization we couldn't solve that, because the fence-
>signaled is at other places evaluated locklessly

const char __rcu *dma_fence_timeline_name(struct dma_fence *fence)
{
	const struct dma_fence_ops *ops;

	/* RCU protection is required for safe access to returned string */
	ops = rcu_dereference(fence->ops);
	if (!dma_fence_test_signaled_flag(fence))
		return (const char __rcu *)ops->get_driver_name(fence);
	else
		return (const char __rcu *)"signaled-timeline";
}
EXPORT_SYMBOL(dma_fence_timeline_name);

The signaled flag here is ordered in the reverse order.
dma_fence_signal_timestamp_locked() sets the ops pointer to NULL, so
you might dereference a NULL pointer there.


P.

> 
> Best,
> Gary
> 
> > +	set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags);
> >  }
> >  EXPORT_SYMBOL(dma_fence_signal_timestamp_locked);
> >  
>