On 19. 08. 26, 14:51, Philipp Stanner wrote:
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)
It makes the system dead during early boot :P.
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,
This missing piece was here:
- fence->flags |= BIT(DMA_FENCE_FLAG_INLINE_LOCK_BIT);
+ flags |= BIT(DMA_FENCE_FLAG_INLINE_LOCK_BIT);
}
fence->error = 0;
+ smp_mb();
+ fence->flags = flags | BIT(DMA_FENCE_FLAG_INITIALIZED_BIT);
trace_dma_fence_init(fence);
}
But it does not help either...
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;
}
/**
--
js
suse labs
