Re: [PATCH 1/9] dma-buf: add dma_fence_was_initialized function v2

[Philipp Stanner <phasta@mailbox.org>]

On Wed, 2026-08-19 at 14:33 +0200, Philipp Stanner wrote:


[…]

> 
> Regardless, looking at the code again, I would say that this might be a
> race, but I don't know enough about QXL to say for sure.
> 
> dma_fence_init() is (of course) not ordered:
> 
> 
> static void
> __dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
> 	         spinlock_t *lock, u64 context, u64 seqno, unsigned long flags)
> {
> 	BUG_ON(!ops || !ops->get_driver_name || !ops->get_timeline_name);
> 
> 	kref_init(&fence->refcount);
> 	/*
> 	 * While it is counter intuitive to protect a constant function pointer
> 	 * table by RCU it allows modules to wait for an RCU grace period
> 	 * before they unload, to make sure that nobody is executing their
> 	 * functions any more.
> 	 */
> 	RCU_INIT_POINTER(fence->ops, ops);
> 	INIT_LIST_HEAD(&fence->cb_list);
> 	fence->context = context;
> 	fence->seqno = seqno;
> 	fence->flags = flags | BIT(DMA_FENCE_FLAG_INITIALIZED_BIT);
> 
> (Should this maybe be set_bit() btw?)
> 
> 
> The fact that QXL could run into qxl_release_free() with an
> uninitialized fence hints at the fact that this might race, so
> DMA_FENCE_FLAG_INITIALIZED_BIT could be set / read before kref_init()
> ran.
> 
> 
> Maybe one way to verify / debug that would be to move
> spin_unlock(&qdev->release_idr_lock) downwards so it also guards
> dma_fence_was_initialized(), and also lock the initialization of the
> fence (in qxl_release_fence_buffer_objects() ?) with said lock.
> 
> If that's possible. Just brainstorming a bit for ways how to debug.
> 
> QXL does a few tricky things with the release->base.ops pointer.
> qxl_release_alloc() sets it to NULL, and only
> qxl_release_fence_buffer_objects() then actually sets it. So this could
> be the race? Setting of the ops pointer got replaced by setting of the
> fence-flag.
> 
> 
> P.

Could you test something like this? (not even compile-tested, just an idea)


diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 87797bea91cb..df1aa48b2809 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -1075,7 +1075,6 @@ __dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
        INIT_LIST_HEAD(&fence->cb_list);
        fence->context = context;
        fence->seqno = seqno;
-       fence->flags = flags | BIT(DMA_FENCE_FLAG_INITIALIZED_BIT);
        if (lock) {
                fence->extern_lock = lock;
        } else {
@@ -1084,6 +1083,8 @@ __dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
        }
        fence->error = 0;
 
+       smp_mb();
+       fence->flags = flags | BIT(DMA_FENCE_FLAG_INITIALIZED_BIT);
        trace_dma_fence_init(fence);
 }
 
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index b52ab692b22e..40ffdcafaac1 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -292,7 +292,12 @@ void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq);
  */
 static inline bool dma_fence_was_initialized(struct dma_fence *fence)
 {
-       return fence && test_bit(DMA_FENCE_FLAG_INITIALIZED_BIT, &fence->flags);
+       bool init;
+
+       init = fence && test_bit(DMA_FENCE_FLAG_INITIALIZED_BIT, &fence->flags);
+       smp_mb();
+
+       return init;
 }
 
 /**