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

[Philipp Stanner <phasta@mailbox.org>]

On Mon, 2026-07-13 at 10:58 +0200, Jiri Slaby wrote:
> Hi,
> 
> On 20. 01. 26, 11:54, Christian König wrote:
> > Some driver use fence->ops to test if a fence was initialized or not.
> > The problem is that this utilizes internal behavior of the dma_fence
> > implementation.
> > 
> > So better abstract that into a function.
> > 
> > v2: use a flag instead of testing fence->ops, rename the function, move
> >      to the beginning of the patch set.
> ...
> > --- a/drivers/gpu/drm/qxl/qxl_release.c
> > +++ b/drivers/gpu/drm/qxl/qxl_release.c
> > @@ -146,7 +146,7 @@ qxl_release_free(struct qxl_device *qdev,
> >   	idr_remove(&qdev->release_idr, release->id);
> >   	spin_unlock(&qdev->release_idr_lock);
> >   
> > -	if (release->base.ops) {
> > +	if (dma_fence_was_initialized(&release->base)) {

Could you verify the cause with sth like

if (release->base.ops && dma_fence_was_initialized(…)) {

?

> >   		WARN_ON(list_empty(&release->bos));
> >   		qxl_release_free_list(release);
> >   
> 
> This likely breaks qxl:
> https://bugzilla.suse.com/show_bug.cgi?id=1271081
> 
>  > refcount_t: underflow; use-after-free.
>  > WARNING: lib/refcount.c:28 at refcount_warn_saturate+0x59/0x90, 
> CPU#0: kworker/0:0/1534
>  > Modules linked in: af_packet nft_fib_inet ...
>  > CPU: 0 UID: 0 PID: 1534 Comm: kworker/0:0 Not tainted 7.1.3-1-default 
> #1 PREEMPT(full) openSUSE Tumbleweed 
> b041a6527f6e58424f4cd3de0fade8d408b378fd
>  > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 
> rel-1.16.3-2-gc13ff2cd-prebuilt.qemu.org 04/01/2014
>  > Workqueue: events qxl_gc_work [qxl]
>  > RIP: 0010:refcount_warn_saturate+0x59/0x90
>  > Code: 44 48 8d 3d a9 72 b7 01 67 48 0f b9 3a c3 cc cc cc cc 48 8d 3d 
> a8 72 b7 01 67 48 0f b9 3a c3 cc cc cc cc 48 8d 3d a7 72 b7 01 <67> 48 
> 0f b9 3a c3 cc cc cc cc 48 8d 3d a6 72 b7 01 67 48 0f b9 3a
>  > RSP: 0018:ffffce09019efe00 EFLAGS: 00010246
>  > RAX: 0000000000000000 RBX: ffff8b5a56f819c0 RCX: 0000000000000017
>  > RDX: 0000000000011696 RSI: 0000000000000003 RDI: ffffffffbc953b70
>  > RBP: ffff8b5a4a314000 R08: 0000000000000001 R09: ffffffffc07d488a
>  > R10: fffffa6fc4787d80 R11: ffff8b5a40044000 R12: 0000000000000004
>  > R13: 0000000000000003 R14: ffffce09003f1434 R15: ffff8b5a410c4640
>  > FS:  0000000000000000(0000) GS:ffff8b5afe954000(0000) 
> knlGS:0000000000000000
>  > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>  > CR2: 00007ff463d65000 CR3: 000000012b476004 CR4: 0000000000772ef0
>  > PKRU: 55555554
>  > Call Trace:
>  >  <TASK>
>  >  qxl_release_free+0xee/0xf0 [qxl 
> d93e9381353e619799d56790f5f8dda6cce491f6]
>  >  qxl_garbage_collect+0xd1/0x1b0 [qxl 
> d93e9381353e619799d56790f5f8dda6cce491f6]
>  >  process_one_work+0x19e/0x3a0
>  > ...
> 
> The function does now:
> 
>  > void
>  > qxl_release_free(struct qxl_device *qdev,
>  >                  struct qxl_release *release)
>  > {
>  > ...
>  >         if (dma_fence_was_initialized(&release->base)) {
>  >                 WARN_ON(list_empty(&release->bos)); 
> 
>  >                 qxl_release_free_list(release); 
> 
>  > 
> 
>  >                 dma_fence_signal(&release->base); 
> 
>  >                 dma_fence_put(&release->base); 
> 
>   
> 
> refcount_dec_and_test() in kref_put() in this ^^ crashes. Apparently,
> the reference count was not increased anywhere. QXL does not call 
> dma_fence_get() -- who is supposed to increase the refcount.
> 

dma_fence_init() initializes the refcount to 1. So since there is only
one place with dma_fence_put(), it probably aims at countering that.


>  And why it 
> did not fail before?

My guess would be that base->ops was used as a boolean to see whether
qxl_release contains a valid fence. It seems qxl_release_free() is
intended as some sort of universal function; it is called at many
places. So I would suppose it is sometimes being called without the
fence being initialized.

Should most certainly have been documented. Anyways, verifying the
issue would probably be cool

Cheers,
P.

>   
> 
>  >         } else { 
> 
>  >                 qxl_release_free_list(release); 
> 
>  >                 kfree(release); 
> 
>  >         } 
> 
>  >         atomic_dec(&qdev->release_count); 
> 
>  > }
> 
> thanks,