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

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

[Kirtikumar Anandrao Ramchandani]

[-- Attachment #1: Type: text/plain, Size: 1854 bytes --]

The last dma_fence_put() can drop a sync point's reference count to
zero before timeline_fence_release() acquires the timeline lock to
remove it from pt_list. If sync_timeline_signal() acquires the lock in
that window, dma_fence_get() attempts to increment a zero reference
count and triggers a refcount warning.

Use dma_fence_get_rcu() to acquire the temporary signaling reference
only if the fence is still referenced, as sync_pt_create() already
does when looking up an existing point. Skip a point whose reference
count has reached zero: its release callback will remove it from the
list and tree once the timeline lock is released.

The timeline lock prevents the release callback from removing or
freeing a listed point during this check. For a live point, retain the
temporary reference until after dropping the lock, preserving the
protection against recursive locking from signaling callbacks.

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: Kirtikumar Anandrao Ramchandani <kirtiar15502@gmail.com>
---

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

diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index 8df20b0218a9..7f1eadee4cdf 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -221,7 +221,8 @@
                if (!timeline_fence_signaled(&pt->base))
                        break;

-               dma_fence_get(&pt->base);
+               if (!dma_fence_get_rcu(&pt->base))
+                       continue;

                list_move_tail(&pt->link, &signalled);
                rb_erase(&pt->node, &obj->pt_tree);

base-commit: 05441d697f7f9a927264f4058cbbc6a03dfcac9d

[-- Attachment #2: Type: text/html, Size: 2470 bytes --]

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

[Kirtikumar Anandrao Ramchandani]

The last dma_fence_put() can drop a sync point's reference count to
zero before timeline_fence_release() acquires the timeline lock to
remove it from pt_list. If sync_timeline_signal() acquires the lock in
that window, dma_fence_get() attempts to increment a zero reference
count and triggers a refcount warning.

Use dma_fence_get_rcu() to acquire the temporary signaling reference
only if the fence is still referenced, as sync_pt_create() already
does when looking up an existing point. Skip a point whose reference
count has reached zero: its release callback will remove it from the
list and tree once the timeline lock is released.

The timeline lock prevents the release callback from removing or
freeing a listed point during this check. For a live point, retain the
temporary reference until after dropping the lock, preserving the
protection against recursive locking from signaling callbacks.

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: Kirtikumar Anandrao Ramchandani <kirtiar15502@gmail.com>
---
Changes in v2:
- Resend as plain text after the previous mail client reformatted the diff.

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

diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index 8df20b0218a9..7f1eadee4cdf 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -221,7 +221,8 @@
 		if (!timeline_fence_signaled(&pt->base))
 			break;

-		dma_fence_get(&pt->base);
+		if (!dma_fence_get_rcu(&pt->base))
+			continue;

 		list_move_tail(&pt->link, &signalled);
 		rb_erase(&pt->node, &obj->pt_tree);

base-commit: 05441d697f7f9a927264f4058cbbc6a03dfcac9d

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

[sashiko-bot]

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

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

[Kirtikumar Anandrao Ramchandani]

The last dma_fence_put() can drop a sync point's reference count to zero
before timeline_fence_release() acquires the timeline lock. If
sync_timeline_signal() takes the lock in that window, dma_fence_get()
attempts to increment a zero reference count.

Use dma_fence_get_rcu() for the temporary signaling reference. When the
reference count is already zero, remove the point from both the ordered
list and the rb-tree under the timeline lock. The release callback will
see the empty list link and leave the indexes alone. Removing the point
here also prevents a retired entry from blocking or misordering later
timeline operations while its release callback waits for the lock.

The timeline lock prevents the release callback from freeing a listed
point during this check. For a live point, keep the temporary reference
until after dropping the lock, avoiding recursive locking through
signaling callbacks.

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: Kirtikumar Anandrao Ramchandani <kirtiar15502@gmail.com>
---
Changes in v3:
- Unlink zero-reference points from both indexes while holding the
  timeline lock, addressing the ordering and debugfs concerns raised
  during review of v2.

 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 8df20b021..261bc98b1 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);

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

[Christian König]

On 11/4/26 23:18, Kirtikumar Anandrao Ramchandani wrote:
> The last dma_fence_put() can drop a sync point's reference count to
> zero before timeline_fence_release() acquires the timeline lock to
> remove it from pt_list. If sync_timeline_signal() acquires the lock in
> that window, dma_fence_get() attempts to increment a zero reference
> count and triggers a refcount warning.
> 
> Use dma_fence_get_rcu() to acquire the temporary signaling reference
> only if the fence is still referenced, as sync_pt_create() already
> does when looking up an existing point. Skip a point whose reference
> count has reached zero: its release callback will remove it from the
> list and tree once the timeline lock is released.
> 

> The timeline lock prevents the release callback from removing or
> freeing a listed point during this check. For a live point, retain the
> temporary reference until after dropping the lock, preserving the
> protection against recursive locking from signaling callbacks.

Please add this explanation as code comment to the patch.

> 
> Fixes: e531fdb5cd5e ("dma-buf/sw_sync: Avoid recursive lock during fence signal")

Drop that here. SW_SYNC is a unit testing functionality which can intentionally crash the kernel.

You get literally tons of warnings and a tainted kernel when it is enabled/used. So bugs like those are generally not backported.

Apart from that the patch looks good to me,
Christian.

> Reported-by: syzbot+cba1a1f459230395a076@syzkaller.appspotmail.com <mailto:syzbot%2Bcba1a1f459230395a076@syzkaller.appspotmail.com>
> Closes: https://syzkaller.appspot.com/bug?extid=cba1a1f459230395a076 <https://syzkaller.appspot.com/bug?extid=cba1a1f459230395a076>
> Signed-off-by: Kirtikumar Anandrao Ramchandani <kirtiar15502@gmail.com <mailto:kirtiar15502@gmail.com>>
> ---
> 
>  drivers/dma-buf/sw_sync.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> index 8df20b0218a9..7f1eadee4cdf 100644
> --- a/drivers/dma-buf/sw_sync.c
> +++ b/drivers/dma-buf/sw_sync.c
> @@ -221,7 +221,8 @@
>                 if (!timeline_fence_signaled(&pt->base))
>                         break;
> 
> -               dma_fence_get(&pt->base);
> +               if (!dma_fence_get_rcu(&pt->base))
> +                       continue;
> 
>                 list_move_tail(&pt->link, &signalled);
>                 rb_erase(&pt->node, &obj->pt_tree);
> 
> base-commit: 05441d697f7f9a927264f4058cbbc6a03dfcac9d

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

[Kirtikumar Anandrao Ramchandani]

The last dma_fence_put() can drop a sync point's reference count to zero
before timeline_fence_release() acquires the timeline lock. If
sync_timeline_signal() takes the lock in that window, dma_fence_get()
attempts to increment a zero reference count.

Use dma_fence_get_rcu() for the temporary signaling reference and skip a
point whose reference count has reached zero. The timeline lock serializes
with timeline_fence_release(), keeping the point allocated while its
reference count is examined. The release callback removes the point from
the list and tree after acquiring the lock.

For a live point, retain the temporary reference until after dropping the
lock, avoiding recursive locking through signaling callbacks.

Reported-by: syzbot+cba1a1f459230395a076@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=cba1a1f459230395a076
Signed-off-by: Kirtikumar Anandrao Ramchandani <kirtiar15502@gmail.com>
---
Changes in v4:
- Document why the timeline lock makes the conditional reference
  acquisition safe without an RCU read-side critical section.
- Leave list and rb-tree removal to timeline_fence_release().
- Drop the Fixes tag because SW_SYNC fixes are not intended for stable.

Changes in v3:
- Unlink zero-reference points from both indexes while holding the
  timeline lock.

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

diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index 8df20b021..8aa3ed777 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -221,7 +221,12 @@ static void sync_timeline_signal(struct
sync_timeline *obj, unsigned int inc)
 		if (!timeline_fence_signaled(&pt->base))
 			break;

-		dma_fence_get(&pt->base);
+		/*
+		 * The timeline lock prevents timeline_fence_release() from
+		 * removing and freeing pt while its reference count is examined.
+		 */
+		if (!dma_fence_get_rcu(&pt->base))
+			continue;

 		list_move_tail(&pt->link, &signalled);
 		rb_erase(&pt->node, &obj->pt_tree);

base-commit: 05441d697f7f9a927264f4058cbbc6a03dfcac9d

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

[Kirtikumar Anandrao Ramchandani]

Thanks for the review.

I have sent v4 with a code comment explaining that the timeline lock
serializes with timeline_fence_release(), keeping the sync point allocated
while dma_fence_get_rcu() examines its reference count. The zero-reference
case now leaves list and rb-tree removal to timeline_fence_release(), and
the Fixes tag has been dropped.

Regards,
Kirtikumar

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

[Christian König]

On 11/5/26 15:26, Kirtikumar Anandrao Ramchandani wrote:
> The last dma_fence_put() can drop a sync point's reference count to zero
> before timeline_fence_release() acquires the timeline lock. If
> sync_timeline_signal() takes the lock in that window, dma_fence_get()
> attempts to increment a zero reference count.
> 
> Use dma_fence_get_rcu() for the temporary signaling reference and skip a
> point whose reference count has reached zero. The timeline lock serializes
> with timeline_fence_release(), keeping the point allocated while its
> reference count is examined. The release callback removes the point from
> the list and tree after acquiring the lock.
> 
> For a live point, retain the temporary reference until after dropping the
> lock, avoiding recursive locking through signaling callbacks.
> 
> Reported-by: syzbot+cba1a1f459230395a076@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=cba1a1f459230395a076
> Signed-off-by: Kirtikumar Anandrao Ramchandani <kirtiar15502@gmail.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

I will push this to drm-misc-next soon.

Thanks,
Christian.

> ---
> Changes in v4:
> - Document why the timeline lock makes the conditional reference
>   acquisition safe without an RCU read-side critical section.
> - Leave list and rb-tree removal to timeline_fence_release().
> - Drop the Fixes tag because SW_SYNC fixes are not intended for stable.
> 
> Changes in v3:
> - Unlink zero-reference points from both indexes while holding the
>   timeline lock.
> 
>  drivers/dma-buf/sw_sync.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> index 8df20b021..8aa3ed777 100644
> --- a/drivers/dma-buf/sw_sync.c
> +++ b/drivers/dma-buf/sw_sync.c
> @@ -221,7 +221,12 @@ static void sync_timeline_signal(struct
> sync_timeline *obj, unsigned int inc)
>                 if (!timeline_fence_signaled(&pt->base))
>                         break;
> 
> -               dma_fence_get(&pt->base);
> +               /*
> +                * The timeline lock prevents timeline_fence_release() from
> +                * removing and freeing pt while its reference count is examined.
> +                */
> +               if (!dma_fence_get_rcu(&pt->base))
> +                       continue;
> 
>                 list_move_tail(&pt->link, &signalled);
>                 rb_erase(&pt->node, &obj->pt_tree);
> 
> base-commit: 05441d697f7f9a927264f4058cbbc6a03dfcac9d