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

[Jiri Slaby <jirislaby@kernel.org>]

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