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

["Christian König" <christian.koenig@amd.com>]

On 11/5/26 05:14, 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 kref_get_unless_zero() to optimize dma_get_fence(), in order to avoid
> acquiring an object with a reference count of 0.
> 
> 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>
> ---
>  drivers/dma-buf/sw_sync.c | 3 ++-
>  include/linux/dma-fence.h | 5 +++--
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> index 8df20b0218a9..ad60ad5e5cd9 100644
> --- a/drivers/dma-buf/sw_sync.c
> +++ b/drivers/dma-buf/sw_sync.c
> @@ -221,7 +221,8 @@ 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(&pt->base))
> +                       continue;
> 
>                 list_move_tail(&pt->link, &signalled);
>                 rb_erase(&pt->node, &obj->pt_tree);
> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index dd07d128adc3..4d885c8e1d4f 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -320,8 +320,9 @@ static inline void dma_fence_put(struct dma_fence *fence)
>  static inline struct dma_fence *dma_fence_get(struct dma_fence *fence)
>  {
>         if (fence)
> -               kref_get(&fence->refcount);
> -       return fence;
> +               if (kref_get_unless_zero(&fence->refcount))
> +                       return fence;
> +       return NULL;

Well we should have an automated filter function for nonsense like that.

First of all we have an dma_fence_get_rcu() function which uses kref_get_unless_zero() for this use case, so modifying the dma_fence_get() function is a clear no-go.

Then SW_SYNC is a testing functionality which can literally be used to intentionally crash the kernel with it! There is a massive warning and tainting the kernel when you try to use it.

So a race condition like that one here is completely negligible to this and so there is no justification for any stable backport whatsoever. We could of course fix it to improve the documentation purpose of the code, but that's about it.

And all of this is either obvious by reading the three lines of code below the ones you modified and/or the documentation of the component.

So please before you send out anything like that take a look around and thing for a few seconds if that really makes sense or is just a waste of time.

Regards,
Christian.

>  }
> 
>  /**
> --
> 2.43.0
>