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;
}
/**
