Re: [BUG] WARNING: refcount bug in sync_timeline_signal

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The commit improperly merges fuzzer reproducer code that intentionally stalls the kernel, introducing a severe 50ms delay inside a spinlock with local interrupts disabled.

Pre-existing issues:
- [Critical] Calling `dma_fence_get()` in `sync_timeline_signal()` on a fence whose refcount may have already dropped to 0 leads to a use-after-free, double-free, and rb-tree corruption.
--

--- Patch [1]: [BUG] WARNING: refcount bug in sync_timeline_signal ---
commit 6c2b9513def2f661abe36d3f3a4c129d8212ea60
Author: Jaeyoung Chung <[email protected]>

WARNING: refcount bug in sync_timeline_signal

This commit merges a fuzzer's reproducer delay patch directly into the
sw_sync subsystem. It does not provide a fix for the underlying race
condition, but instead introduces an intentional delay.

> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> index 8df20b0218a9e..d30ce9d915ba7 100644
> --- a/drivers/dma-buf/sw_sync.c
> +++ b/drivers/dma-buf/sw_sync.c

[ ... ]

> @@ -215,6 +216,10 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
>  
>  	spin_lock_irq(&obj->lock);
>  
> +	if (strncmp(current->comm, "syzrepro1", 9) == 0) {
> +		mdelay(50);
> +	}
> +

[Severity: High]
Is it intentional to merge reproducer delay code into the tree?

Adding a 50-millisecond busy loop (mdelay(50)) inside a critical section
protected by spin_lock_irq(&obj->lock) in sync_timeline_signal() can cause
severe latency spikes and missed hardware interrupts.

An unprivileged user could trigger this by naming their task 'syzrepro1' and
interacting with the sw_sync subsystem to create a local Denial of Service.

>  	obj->value += inc;
>  
>  	list_for_each_entry_safe(pt, next, &obj->pt_list, link) {

[Severity: Critical]
This is a pre-existing issue, but does sync_timeline_signal() unsafely
call dma_fence_get() on a fence whose refcount may have already dropped to 0?

When a userspace program closes a sync_file descriptor, it drops the
underlying sync_pt fence refcount to 0, which triggers
timeline_fence_release().

timeline_fence_release() blocks waiting for obj->lock. Concurrently, if
sync_timeline_signal() holds obj->lock and iterates over obj->pt_list, it
encounters the 0-refcount fence and unconditionally calls
dma_fence_get(&pt->base):

drivers/dma-buf/sw_sync.c:sync_timeline_signal() {
    ...
    dma_fence_get(&pt->base);
    list_move_tail(&pt->link, &signalled);
    rb_erase(&pt->node, &obj->pt_tree);
    ...
}

It then moves the fence to a local list, erases it from the tree, and
unlocks.

timeline_fence_release() then acquires the lock, illegally modifies the other
thread's local list, performs a double rb_erase() on the tree, and frees the
fence:

drivers/dma-buf/sw_sync.c:timeline_fence_release() {
    ...
    if (!list_empty(&pt->link)) {
        list_del(&pt->link);
        rb_erase(&pt->node, &parent->pt_tree);
    }
    ...
}

Finally, sync_timeline_signal() iterates its local list, accessing and
putting the freed fence, leading to a use-after-free and double free.

Would using dma_fence_get_rcu() in sync_timeline_signal() safely increment
the refcount only if it is non-zero?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1