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.