[PATCH v2] dma-buf/sw_sync: Optimize the retrieval of fence reference counts

[PATCH v2] dma-buf/sw_sync: Optimize the retrieval of fence reference counts

[Edward Adam Davis]

There is a race condition between closing a sync file and the SW_SYNC_IOC_INC
operation on a sync obj. When a fence is created via SW_SYNC_IOC_CREATE_FENCE,
its reference count is initialized to 1. When the sync file is closed, the
fence's reference count drops to 0. A race occurs if SW_SYNC_IOC_INC accesses
the fence whose ref count has already reached 0 before dma_fence_release()
calls timeline_fence_release() to erase the corresponding pt_tree node from
the sync object, potentially hitting:

refcount_t: addition on 0; use-after-free.
------------[ cut here ]------------
kernel BUG at lib/refcount.c:34!
Oops: invalid opcode: 0000 [#1] SMP KASAN NOPTI
Call Trace:
 kref_get include/linux/kref.h:45 [inline]
 dma_fence_get include/linux/dma-fence.h:323 [inline]
 sync_timeline_signal+0x6d9/0x8a0 drivers/dma-buf/sw_sync.c:224
 sw_sync_ioctl_inc drivers/dma-buf/sw_sync.c:405 [inline]
 sw_sync_ioctl+0x7b4/0x13f0 drivers/dma-buf/sw_sync.c:470

Use dma_fence_get_rcu() to replace dma_get_fence(), if fence reference
count is 0 then do like timeline fence release things: remove pt link
and erase pt node.

Fixes: e531fdb5cd5e ("dma-buf/sw_sync: Avoid recursive lock during fence signal")
Reported-by: syzbot+cba1a1f459230395a076@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=cba1a1f459230395a076
Signed-off-by: Edward Adam Davis <eadavis@sina.com>
---
v1 -> v2: use dma_fence_get_rcu() and remove link & erase pt node

 drivers/dma-buf/sw_sync.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index 8df20b0218a9..261bc98b1ed6 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -221,7 +221,11 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
 		if (!timeline_fence_signaled(&pt->base))
 			break;
 
-		dma_fence_get(&pt->base);
+		if (!dma_fence_get_rcu(&pt->base)) {
+			list_del_init(&pt->link);
+			rb_erase(&pt->node, &obj->pt_tree);
+			continue;
+		}
 
 		list_move_tail(&pt->link, &signalled);
 		rb_erase(&pt->node, &obj->pt_tree);
-- 
2.43.0

Re: [PATCH v2] dma-buf/sw_sync: Optimize the retrieval of fence reference counts

[Philipp Stanner]

On Thu, 2026-11-05 at 13:27 +0800, Edward Adam Davis wrote:
> ---
> v1 -> v2: use dma_fence_get_rcu() and remove link & erase pt node

Ideally, one avoids sending new versions within 24 hours :)

> 
>  drivers/dma-buf/sw_sync.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> index 8df20b0218a9..261bc98b1ed6 100644
> --- a/drivers/dma-buf/sw_sync.c
> +++ b/drivers/dma-buf/sw_sync.c
> @@ -221,7 +221,11 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
>  		if (!timeline_fence_signaled(&pt->base))
>  			break;
>  
> -		dma_fence_get(&pt->base);
> +		if (!dma_fence_get_rcu(&pt->base)) {

You must hold the rcu_read_lock() while doing that, don't you?

This probably works "by coincidence" since the surrounding spinlock
also disables preemption, like the rcu lock does.

That spinlock, why can't it guard the dma_fence reference? Doesn't obj-
>pt_list hold its own reference?



P.

[PATCH v2] dma-buf/sw_sync: Optimize the retrieval of fence reference counts

[Edward Adam Davis]

On Thu, 05 Nov 2026 08:49:49 +0100, Philipp Stanner wrote:
> > 
> >  drivers/dma-buf/sw_sync.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> > index 8df20b0218a9..261bc98b1ed6 100644
> > --- a/drivers/dma-buf/sw_sync.c
> > +++ b/drivers/dma-buf/sw_sync.c
> > @@ -221,7 +221,11 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
> >  		if (!timeline_fence_signaled(&pt->base))
> >  			break;
> >  
> > -		dma_fence_get(&pt->base);
> > +		if (!dma_fence_get_rcu(&pt->base)) {
> 
> You must hold the rcu_read_lock() while doing that, don't you?
dma_fence_get_rcu() is simply a wrapper around kref_get_unless_zero(),
it has nothing to do with RCU. For instance, the use of dma_fence_get_rcu()
within sync_pt_create(), as seen in the SW_SYNC_IOC_CREATE_FENCE code path
does not require an RCU lock.
> 
> This probably works "by coincidence" since the surrounding spinlock
> also disables preemption, like the rcu lock does.
> 
> That spinlock, why can't it guard the dma_fence reference? Doesn't obj-
> >pt_list hold its own reference?
This is neither a coincidence nor related to any outer spinlock.
The comments I included in my patch provide a complete explanation of
the fence's reference counting, from the moment of its creation to its
final destruction.

BR,
Edward

Re: [PATCH v2] dma-buf/sw_sync: Optimize the retrieval of fence reference counts

[Christian König]

On 11/5/26 06:27, Edward Adam Davis wrote:
> There is a race condition between closing a sync file and the SW_SYNC_IOC_INC
> operation on a sync obj. When a fence is created via SW_SYNC_IOC_CREATE_FENCE,
> its reference count is initialized to 1. When the sync file is closed, the
> fence's reference count drops to 0. A race occurs if SW_SYNC_IOC_INC accesses
> the fence whose ref count has already reached 0 before dma_fence_release()
> calls timeline_fence_release() to erase the corresponding pt_tree node from
> the sync object, potentially hitting:
> 
> refcount_t: addition on 0; use-after-free.
> ------------[ cut here ]------------
> kernel BUG at lib/refcount.c:34!
> Oops: invalid opcode: 0000 [#1] SMP KASAN NOPTI
> Call Trace:
>  kref_get include/linux/kref.h:45 [inline]
>  dma_fence_get include/linux/dma-fence.h:323 [inline]
>  sync_timeline_signal+0x6d9/0x8a0 drivers/dma-buf/sw_sync.c:224
>  sw_sync_ioctl_inc drivers/dma-buf/sw_sync.c:405 [inline]
>  sw_sync_ioctl+0x7b4/0x13f0 drivers/dma-buf/sw_sync.c:470
> 
> Use dma_fence_get_rcu() to replace dma_get_fence(), if fence reference
> count is 0 then do like timeline fence release things: remove pt link
> and erase pt node.
> 
> Fixes: e531fdb5cd5e ("dma-buf/sw_sync: Avoid recursive lock during fence signal")

Please completely drop that. This is not something which has any justification to be backported.

> Reported-by: syzbot+cba1a1f459230395a076@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=cba1a1f459230395a076
> Signed-off-by: Edward Adam Davis <eadavis@sina.com>
> ---
> v1 -> v2: use dma_fence_get_rcu() and remove link & erase pt node
> 
>  drivers/dma-buf/sw_sync.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> index 8df20b0218a9..261bc98b1ed6 100644
> --- a/drivers/dma-buf/sw_sync.c
> +++ b/drivers/dma-buf/sw_sync.c
> @@ -221,7 +221,11 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
>                 if (!timeline_fence_signaled(&pt->base))
>                         break;
> 
> -               dma_fence_get(&pt->base);
> +               if (!dma_fence_get_rcu(&pt->base)) {

Well that is at least better than before but still needs a code comment explaining why this is possible without holding the RCU read side lock.

Something like /* It's possible to race with timeline_fence_release() here */.

> +                       list_del_init(&pt->link);
> +                       rb_erase(&pt->node, &obj->pt_tree);

What happens is that you race with timeline_fence_release(), so that here is superfluous and potentially even harmful.

Regards,
Christian.

> +                       continue;
> +               }
> 
>                 list_move_tail(&pt->link, &signalled);
>                 rb_erase(&pt->node, &obj->pt_tree);
> --
> 2.43.0
> 

Re: [PATCH v2] dma-buf/sw_sync: Optimize the retrieval of fence reference counts

[Philipp Stanner]

On Thu, 2026-11-05 at 17:35 +0800, Edward Adam Davis wrote:
> On Thu, 05 Nov 2026 08:49:49 +0100, Philipp Stanner wrote:
> > > 
> > >  drivers/dma-buf/sw_sync.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> > > index 8df20b0218a9..261bc98b1ed6 100644
> > > --- a/drivers/dma-buf/sw_sync.c
> > > +++ b/drivers/dma-buf/sw_sync.c
> > > @@ -221,7 +221,11 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
> > >  		if (!timeline_fence_signaled(&pt->base))
> > >  			break;
> > >  
> > > -		dma_fence_get(&pt->base);
> > > +		if (!dma_fence_get_rcu(&pt->base)) {
> > 
> > You must hold the rcu_read_lock() while doing that, don't you?
> dma_fence_get_rcu() is simply a wrapper around kref_get_unless_zero(),
> it has nothing to do with RCU. For instance, the use of dma_fence_get_rcu()
> within sync_pt_create(), as seen in the SW_SYNC_IOC_CREATE_FENCE code path
> does not require an RCU lock.

How come the function you're using here literally says in its docu that
you need RCU?

/**
 * dma_fence_get_rcu - get a fence from a dma_resv_list with
 *                     rcu read lock
 * @fence: fence to increase refcount of
 *
 * Function returns NULL if no refcount could be obtained, or the fence.
 */
static inline struct dma_fence *dma_fence_get_rcu(struct dma_fence *fence)
{
	if (kref_get_unless_zero(&fence->refcount))
		return fence;
	else
		return NULL;
}


The rcu_read_lock() protects your kref_get_unless_zero() from running
into UAF if refcount == 0.

That's why dma_fence_free() uses kfree_rcu(). Your read-side-lock
prevents kfree() from running so that you can safely read the refcount
== 0 from memory.

If we think about it, having kref_get_unless_zero() without RCU doesn't
even make sense, because refcount == 0 must mean that the object is now
supposed to be freed. That's the point of refcounting.

Or am I missing something?

> 

P.

[PATCH v2] dma-buf/sw_sync: Optimize the retrieval of fence reference counts

[Edward Adam Davis]

On Thu, 5 Nov 2026 11:03:10 +0100, Christian König wrote:
> > There is a race condition between closing a sync file and the SW_SYNC_IOC_INC
> > operation on a sync obj. When a fence is created via SW_SYNC_IOC_CREATE_FENCE,
> > its reference count is initialized to 1. When the sync file is closed, the
> > fence's reference count drops to 0. A race occurs if SW_SYNC_IOC_INC accesses
> > the fence whose ref count has already reached 0 before dma_fence_release()
> > calls timeline_fence_release() to erase the corresponding pt_tree node from
> > the sync object, potentially hitting:
> >
> > refcount_t: addition on 0; use-after-free.
> > ------------[ cut here ]------------
> > kernel BUG at lib/refcount.c:34!
> > Oops: invalid opcode: 0000 [#1] SMP KASAN NOPTI
> > Call Trace:
> >  kref_get include/linux/kref.h:45 [inline]
> >  dma_fence_get include/linux/dma-fence.h:323 [inline]
> >  sync_timeline_signal+0x6d9/0x8a0 drivers/dma-buf/sw_sync.c:224
> >  sw_sync_ioctl_inc drivers/dma-buf/sw_sync.c:405 [inline]
> >  sw_sync_ioctl+0x7b4/0x13f0 drivers/dma-buf/sw_sync.c:470
> >
> > Use dma_fence_get_rcu() to replace dma_get_fence(), if fence reference
> > count is 0 then do like timeline fence release things: remove pt link
> > and erase pt node.
> >
> > Fixes: e531fdb5cd5e ("dma-buf/sw_sync: Avoid recursive lock during fence signal")
> 
> Please completely drop that. This is not something which has any justification to be backported.
OK.
> 
> > Reported-by: syzbot+cba1a1f459230395a076@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=cba1a1f459230395a076
> > Signed-off-by: Edward Adam Davis <eadavis@sina.com>
> > ---
> > v1 -> v2: use dma_fence_get_rcu() and remove link & erase pt node
> >
> >  drivers/dma-buf/sw_sync.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> > index 8df20b0218a9..261bc98b1ed6 100644
> > --- a/drivers/dma-buf/sw_sync.c
> > +++ b/drivers/dma-buf/sw_sync.c
> > @@ -221,7 +221,11 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
> >                 if (!timeline_fence_signaled(&pt->base))
> >                         break;
> >
> > -               dma_fence_get(&pt->base);
> > +               if (!dma_fence_get_rcu(&pt->base)) {
> 
> Well that is at least better than before but still needs a code comment explaining why this is possible without holding the RCU read side lock.
> 
> Something like /* It's possible to race with timeline_fence_release() here */.
> 
> > +                       list_del_init(&pt->link);
> > +                       rb_erase(&pt->node, &obj->pt_tree);
> 
> What happens is that you race with timeline_fence_release(), so that here is superfluous and potentially even harmful.
timeline_fence_release() and sync_timeline_signal() are synchronized using
the same spinlock(timeline obj->lock is same as fence->extern_lock), so
there is no race condition between them. Instead, a race condition exists
between sync_timeline_signal() and sync_file_release(), which causes the
fence reference count to drop to zero prematurely.

By removing the link and re-initializing it here, list_empty(&pt->link) will
evaluate to true when the spinlock is acquired within timeline_fence_release(),
preventing the redundant execution of the link-removal and node-erase operation.

BR,
Edward

Re: [PATCH v2] dma-buf/sw_sync: Optimize the retrieval of fence reference counts

[Danilo Krummrich]

On Thu Nov 5, 2026 at 11:03 AM CET, Christian König wrote:
> On 11/5/26 06:27, Edward Adam Davis wrote:
>> Fixes: e531fdb5cd5e ("dma-buf/sw_sync: Avoid recursive lock during fence signal")
>
> Please completely drop that. This is not something which has any justification to be backported.

Fixes: identifies the commit that introduced a bug for better reference,
independently of whether the fix should be backported.

If something shouldn't be selected by autosel one can add

	Cc: <stable+noautosel@kernel.org> # reason to exclude from stable

In this case I assume you said that because SW_SYNC is test infrastructure? If
so, AFAIK the stable team explicitly does backport test infrastructure as people
also run selftests on stable kernels.

Thanks,
Danilo

Re: [PATCH v2] dma-buf/sw_sync: Optimize the retrieval of fence reference counts

[Christian König]

On 11/5/26 14:55, Danilo Krummrich wrote:
> On Thu Nov 5, 2026 at 11:03 AM CET, Christian König wrote:
>> On 11/5/26 06:27, Edward Adam Davis wrote:
>>> Fixes: e531fdb5cd5e ("dma-buf/sw_sync: Avoid recursive lock during fence signal")
>>
>> Please completely drop that. This is not something which has any justification to be backported.
> 
> Fixes: identifies the commit that introduced a bug for better reference,
> independently of whether the fix should be backported.
> 
> If something shouldn't be selected by autosel one can add
> 
> 	Cc: <stable+noautosel@kernel.org> # reason to exclude from stable

Oh, thanks for that! I wasn't aware that this possibility exists as well.

> In this case I assume you said that because SW_SYNC is test infrastructure? If
> so, AFAIK the stable team explicitly does backport test infrastructure as people
> also run selftests on stable kernels.

Well yes and no. The situation is a bit more complicated, the point is that SW_SYNC is largely deprecated test infrastructure.

The provided functionality from SW_SYNC can intentionally hard crash the kernel without any chance to recover.

The patch is now fixing a bug which could potentially crash the kernel while using SW_SYNC, but for a component which does exactly that it is kind of pointless.

I mean it is nice to have for documentation purposes, but SW_SYNC is only still around because we don't want to break the uAPI and keep CI systems running.

And *YES*, SW_SYNC shouldn't have been merged in the first place.

Regards,
Christian.

> 
> Thanks,
> Danilo