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

[Jiri Slaby <jirislaby@kernel.org>]

On 28. 08. 26, 9:29, Jiri Slaby wrote:
> 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...

What helps is indeed the revert back to:

--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -147,7 +147,7 @@ qxl_release_free(struct qxl_device *qdev,
         idr_remove(&qdev->release_idr, release->id);
         spin_unlock(&qdev->release_idr_lock);

-       if (dma_fence_was_initialized(&release->base)) {
+       if (release->base.ops) {
                 WARN_ON(list_empty(&release->bos));
                 qxl_release_free_list(release);



Or the bool flag appears to help too:

--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -144,6 +144,7 @@ enum {
  #define QXL_MAX_RES 96
  struct qxl_release {
         struct dma_fence base;
+       bool uses_fence;

         int id;
         int type;
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -97,6 +97,7 @@ qxl_release_alloc(struct qxl_device *qdev, int type,
                 return -ENOMEM;
         }
         release->base.ops = NULL;
+       release->uses_fence = false;
         release->type = type;
         release->release_offset = 0;
         release->surface_release_id = 0;
@@ -147,7 +148,7 @@ qxl_release_free(struct qxl_device *qdev,
         idr_remove(&qdev->release_idr, release->id);
         spin_unlock(&qdev->release_idr_lock);

-       if (dma_fence_was_initialized(&release->base)) {
+       if (release->uses_fence) {
                 WARN_ON(list_empty(&release->bos));
                 qxl_release_free_list(release);

@@ -431,6 +432,7 @@ void qxl_release_fence_buffer_objects(struct 
qxl_release *release)
          */
         dma_fence_init(&release->base, &qxl_fence_ops, &qdev->release_lock,
                        release->id | 0xf0000000, release->base.seqno);
+       release->uses_fence = true;
         trace_dma_fence_emit(&release->base);

         list_for_each_entry(entry, &release->bos, list) {


thanks,
-- 
js
suse labs