Re: [RFC PATCH] dma-fence: Fix races of fence callbacks versus destructors by locking

[Tvrtko Ursulin <tvrtko.ursulin@igalia.com>]

On 09/06/2026 14:19, Philipp Stanner wrote:
> +Cc Dave
> 
> On Mon, 2026-06-08 at 20:47 +0200, Christian König wrote:
>> On 6/8/26 20:39, Danilo Krummrich wrote:
>>> How did you get to this conclusion considering that you run into what I
>>> mentioned above as well and the fact that we seem to agree that the performance
>>> concern is rather questionable?
>>
>> Quite simple, it's the cleaner approach.
>>
>> Calling callbacks with locks held is rather questionable even putting the performance issue aside.
> 
> 
> I'm right now going through all fence users to see whether we can
> implement my solution.
> 
> And look what I found:
> 
> static inline bool
> nouveau_cli_work_ready(struct dma_fence *fence)
> {
> 	unsigned long flags;
> 	bool ret = true;
> 
> 	dma_fence_lock_irqsave(fence, flags);
> 	if (!dma_fence_is_signaled_locked(fence))
> 		ret = false;
> 	dma_fence_unlock_irqrestore(fence, flags);
> 
> 	if (ret == true)
> 		dma_fence_put(fence);
> 	return ret;
> }
> 
> 
> That looks weird, doesn't it?
> 
> 
> We do some git-blame:
> 
> c8a5d5ea3ba6a18958f8d76430e4cd68eea33943
> 
> and we find that it's Dave who wrote that code, because
> 
> "
>      My analysis: two threads are running, one in the irq signalling the
>      fence, in dma_fence_signal_timestamp_locked, it has done the
>      DMA_FENCE_FLAG_SIGNALLED_BIT setting, but hasn't yet reached the
>      callbacks.
>      
>      The second thread in nouveau_cli_work_ready, where it sees the fence is
>      signalled, so then puts the fence, cleanups the object and frees the
>      work item, which contains the callback.
>      
>      Thread one goes again and tries to call the callback and causes the
>      use-after-free.
> "
> 
> 
> So this is a race, caused by lockless speed optimization.
> 
> And it would further seem that there is one invention in computer
> science that can prevent such races:
> 
> Locks.
> 
> There is no cleaner, safer synchronization strategy in computer science
> than locking.
> 
> This race became possible because the lock does not guard the entirety
> of dma_fence_is_signaled().

One could also argue the problem was caused by sharing the fence 
reference without holding it, anyway, I wanted to ask something else. 
What happened to the idea to remove opportunistic signalling from 
dma_fence_is_signaled?

Regards,

Tvrtko

> 
> Wouldn't you agree that this is a strong indicator for the great
> advantages that consequent and consistent lock-protection grants? IOW,
> by using locks more strictly in dma_fence, we can increase its
> robustness and reliability.
> 
> 
> P.