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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] dma-buf: dma_fence_get() uses kref_get_unless_zero() breaking API contract
- [High] dma-buf/sw_sync: sync_timeline_signal() leaves zero-refcount pt linked
--

commit 6ff0958843bbbad778cdc3d38a038557ac00bd49
Author: Edward Adam Davis <eadavis@sina.com>

dma-buf/sw_sync: Optimize the retrieval of fence reference counts

This commit changes dma_fence_get() to use kref_get_unless_zero() and updates
sync_timeline_signal() to skip fences when dma_fence_get() returns NULL. This
is intended to avoid a refcount addition-on-zero when racing with fence release
in sw_sync.

> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> index 8df20b0218a9e..ad60ad5e5cd9e 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;

[Severity: High]
When acquiring a reference to pt->base fails because its refcount has already
dropped to zero on another CPU, is it safe to continue without unlinking pt
from obj->pt_list and obj->pt_tree?

Continuing here leaves a fence with pt->base.seqno <= obj->value linked at the
head of obj->pt_list and in obj->pt_tree until timeline_fence_release()
acquires obj->lock:

drivers/dma-buf/sw_sync.c:timeline_fence_release() {
	...
	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);
	...
}

Before timeline_fence_release() takes obj->lock, leaving this stale entry
linked can cause several regressions:

1. In sw_sync_ioctl_inc(), increments larger than INT_MAX call
sync_timeline_signal(obj, INT_MAX) in a loop (or consecutive SW_SYNC_IOC_INC
calls may run):

	while (value > INT_MAX)  {
		sync_timeline_signal(obj, INT_MAX);
		value -= INT_MAX;
	}

	sync_timeline_signal(obj, value);

If obj->value advances by more than INT_MAX past the stale pt->base.seqno at
the head of obj->pt_list, the signed difference in
timeline_fence_signaled() -> __dma_fence_is_later() wraps positive.
timeline_fence_signaled(&pt->base) then returns false at the stale head entry,
causing sync_timeline_signal() to break out of the loop early without
signaling active fences later in obj->pt_list.

2. In sync_pt_create(), rbtree insertion assumes all nodes in obj->pt_tree
have seqno in (obj->value, obj->value + INT_MAX]:

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

If a stale pt with seqno <= obj->value remains in obj->pt_tree and a new
unsignaled fence is created with value in (pt->base.seqno + INT_MAX,
obj->value + INT_MAX], cmp overflows a signed 32-bit int to a negative value.
That inserts the new fence to the left of the stale pt and ahead of earlier
active fences in obj->pt_list, corrupting the list and tree ordering even
after timeline_fence_release() later removes the stale pt.

3. If sync_info_debugfs_show() -> sync_print_obj() -> sync_print_fence() runs
while the skipped pt is still on obj->pt_list, dma_fence_get_status_locked()
calls dma_fence_is_signaled_locked(&pt->base), which sees
timeline_fence_signaled(&pt->base) is true and sets
DMA_FENCE_FLAG_SIGNALED_BIT on pt->base. If sw_sync_debugfs_release() then
runs before timeline_fence_release() acquires obj->lock:

	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() triggers WARN_ON(test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
&fence->flags)).

Since timeline_fence_release() checks !list_empty(&pt->link) under obj->lock,
should sync_timeline_signal() unlink pt with list_del_init(&pt->link) and
rb_erase(&pt->node, &obj->pt_tree) before continuing when acquiring the fence
reference fails?

>  
>  		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 dd07d128adc3b..4d885c8e1d4f5 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;
>  }

[Severity: High]
Does changing dma_fence_get() to use kref_get_unless_zero() break the
dma_fence_get() API contract across the kernel?

dma_fence_get() is meant for callers that already hold a valid reference to a
fence, and its kernel-doc comment right above states that it returns the same
fence with its refcount increased by 1.

Replacing kref_get() with kref_get_unless_zero() here suppresses refcount_t
addition-on-zero warnings and saturation when a caller has a genuine
use-after-free bug. Because dma_fence_get() is not annotated with
__must_check, callers that do not check its return value (such as
dma_resv_add_fence(), dma_buf_poll(), dma_fence_array_enable_signaling(),
dma_fence_chain_enable_signaling(), drm_syncobj_add_point(), and
drm_syncobj_replace_fence()) will silently fail to take a reference and later
underflow the refcount in dma_fence_put(). Other callers that pass a non-NULL
fence and dereference or pass the return value to container_of() (such as
i915_request_get(), sync_file_create(), dma_resv_replace_fences(), and
__dma_fence_unwrap_merge()) can unexpectedly receive NULL.

include/linux/dma-fence.h already provides dma_fence_get_rcu() right below
dma_fence_get() for acquiring a reference when the refcount might be zero:

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;
}

And drivers/dma-buf/sw_sync.c already uses dma_fence_get_rcu() in
sync_pt_create() for this exact situation:

			} else {
				if (dma_fence_get_rcu(&other->base)) {
					sync_timeline_put(obj);
					kfree(pt);
					pt = other;
					goto unlock;
				}
				p = &parent->rb_left;
			}

Should dma_fence_get() be left unchanged and sync_timeline_signal() call
dma_fence_get_rcu(&pt->base) instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105041446.51979-1-eadavis@sina.com?part=1