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.