Re: [PATCH v2] dma-buf/sw_sync: Avoid taking references to released fences

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] dma-buf/sw_sync: leaving zero-ref sync_pt on pt_list in sync_timeline_signal()
--

commit 0caa32d111a52826b2d396e063857e0fec8d7dcf
Author: Kirtikumar Anandrao Ramchandani <kirtiar15502@gmail.com>

dma-buf/sw_sync: Avoid taking references to released fences

This patch replaces dma_fence_get() with dma_fence_get_rcu() in
sync_timeline_signal() so that sync points whose reference count has
already dropped to zero are skipped rather than incremented from zero.

> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> index 8df20b0218a9e..7f1eadee4cdff 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_rcu(&pt->base))
> +			continue;

[Severity: High]
Can skipping a zero-refcount sync_pt here without removing it from
obj->pt_list and obj->pt_tree break the timeline ordering invariants?

When the last dma_fence_put() drops pt->base.refcount to zero, and
sync_timeline_signal() acquires obj->lock before timeline_fence_release()
does, obj->value is advanced past pt->base.seqno. Continuing without
unlinking pt leaves a point with seqno <= obj->value in obj->pt_list and
obj->pt_tree after obj->lock is dropped.

Before timeline_fence_release() acquires obj->lock, several problems can
occur:

1. In sync_pt_create(), tree insertion compares sequence numbers using
signed 32-bit difference:

    other = rb_entry(parent, typeof(*pt), node);
    cmp = value - other->base.seqno;

If a new fence is created with value in (pt->base.seqno + INT_MAX,
obj->value + INT_MAX], cmp overflows to a negative value against the stale
pt, inserting the later fence to the left of pt and placing it into
obj->pt_list ahead of earlier unsignaled fences. Even after
timeline_fence_release() removes pt, obj->pt_list remains misordered, so
subsequent sync_timeline_signal() calls will hit the later unsignaled fence
first and break out of the loop without signaling earlier fences behind it.

2. Similarly, if another sync_timeline_signal() call advances obj->value
past pt->base.seqno + INT_MAX before timeline_fence_release() runs,
timeline_fence_signaled(&pt->base) will evaluate __dma_fence_is_later() as
true at the head of obj->pt_list and break early without signaling live
fences queued behind pt.

3. If debugfs reads the timeline via sync_info_debugfs_show() ->
sync_print_obj() -> sync_print_fence() while holding obj->lock,
dma_fence_get_status_locked() calls dma_fence_is_signaled_locked(), which
signals pt->base and sets DMA_FENCE_FLAG_SIGNALED_BIT while leaving pt on
obj->pt_list. If sw_sync_debugfs_release() then iterates over obj->pt_list
before timeline_fence_release() runs:

    list_for_each_entry_safe(pt, next, &obj->pt_list, link) {
        dma_fence_set_error(&pt->base, -ENOENT);
        dma_fence_signal_locked(&pt->base);
    }

dma_fence_set_error() will trigger WARN_ON(test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
&fence->flags)).

Since timeline_fence_release() checks !list_empty(&pt->link) under
obj->lock:

    dma_fence_lock_irqsave(fence, flags);
    if (!list_empty(&pt->link)) {
        list_del(&pt->link);
        rb_erase(&pt->node, &parent->pt_tree);
    }
    dma_fence_unlock_irqrestore(fence, flags);

should sync_timeline_signal() still unlink pt via list_del_init(&pt->link)
and rb_erase(&pt->node, &obj->pt_tree) when dma_fence_get_rcu(&pt->base)
returns NULL?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/CADZg-m0hnuZ+KpqkUgwZvkhjUdN24syvc8qxuR+oZC5rkJHSNw@mail.gmail.com?part=1