Re: [PATCH 04/12] drm/vmwgfx: take fman->lock around fence list mutation in fifo_down

[Matthew Brost <matthew.brost@intel.com>]

On Tue, May 05, 2026 at 06:22:25PM -0400, Zack Rusin wrote:
> vmw_fence_fifo_down() drops fman->lock to wait on a fence and, on
> timeout, mutates fman->fence_list via list_del_init() and signals
> the fence without re-acquiring the lock.  __vmw_fences_update() walks
> and removes entries from the same list under fman->lock from any
> other waiter, the fence-IRQ thread, or vmw_fences_update(), so the
> unlocked list_del_init() can corrupt the list head.
> 
> Re-take fman->lock before manipulating fence->head and use
> dma_fence_signal_locked().  Wrap the locked signalling in
> dma_fence_begin_signalling() / dma_fence_end_signalling() so the
> lockdep annotation that dma_fence_signal() previously provided is
> preserved (the same pattern as __vmw_fences_update()).
> 
> dma_fence_put() is moved outside the lock to avoid a recursive
> acquire from vmw_fence_obj_destroy(), which also takes fman->lock.
> 

Just looking as someone who is curious about AI - not my driver but this
almost certainly looks like a good fix from quick look at vmwgfx code.

> Fixes: ae2a104058e2 ("vmwgfx: Implement fence objects")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-4.7
> Signed-off-by: Zack Rusin <zack.rusin@broadcom.com>
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_fence.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c b/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c
> index 4ef84ff9b638..384c6736cf6b 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c
> @@ -367,13 +367,24 @@ void vmw_fence_fifo_down(struct vmw_fence_manager *fman)
>  		ret = vmw_fence_obj_wait(fence, false, false,
>  					 VMW_FENCE_WAIT_TIMEOUT);
>  
> +		spin_lock(&fman->lock);
>  		if (unlikely(ret != 0)) {
> +			bool cookie = dma_fence_begin_signalling();
> +
>  			list_del_init(&fence->head);
> -			dma_fence_signal(&fence->base);
> +			if (fence->waiter_added) {
> +				vmw_seqno_waiter_remove(fman->dev_priv);
> +				fence->waiter_added = false;
> +			}
> +			dma_fence_signal_locked(&fence->base);
> +			dma_fence_end_signalling(cookie);
>  		}
>  
>  		BUG_ON(!list_empty(&fence->head));
> +		spin_unlock(&fman->lock);
> +

You likely can drop spin_unlock/spin_lock dance around the put here as a
put is just ref count move or vmw_fence_obj_destroy on final which seems
to resolve to kfree_rcu in any case. ofc, if you want to be parnoid it
is perfectly fine to drop the reacquire the lock.

Matt

>  		dma_fence_put(&fence->base);
> +
>  		spin_lock(&fman->lock);
>  	}
>  	spin_unlock(&fman->lock);
> -- 
> 2.51.0
>