Re: [RFC PATCH v2 1/3] dma-fence: Remove RCU protection for fence->ops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dma-fence: broken KUnit test_signaling() after keeping fence->ops on signal
- [Low] dma-fence: sparse warnings from remaining rcu_access_pointer() on fence->ops
--

commit 4607587506fc5e0428c9d4ca148fc5a0c94b0003
Author: Philipp Stanner <phasta@kernel.org>

dma-fence: Remove RCU protection for fence->ops

This commit removes the RCU protection and __rcu annotation for
struct dma_fence::ops, and stops clearing fence->ops to NULL when
signaling fences without release or wait callbacks.

> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index bd58688b81a72..5dab647e3f3f9 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -353,7 +353,6 @@ void __dma_fence_might_wait(void)
>  void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>  				      ktime_t timestamp)
>  {
> -	const struct dma_fence_ops *ops;
>  	struct dma_fence_cb *cur, *tmp;
>  	struct list_head cb_list;
>  
> @@ -365,15 +364,6 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>  
>  	trace_dma_fence_signaled(fence);
>  
> -	/*
> -	 * When neither a release nor a wait operation is specified set the ops
> -	 * pointer to NULL to allow the fence structure to become independent
> -	 * from who originally issued it.
> -	 */
> -	ops = rcu_dereference_protected(fence->ops, true);
> -	if (!ops->release && !ops->wait)
> -		RCU_INIT_POINTER(fence->ops, NULL);
> -

[Severity: Medium]
Does keeping fence->ops set here break test_signaling() in
drivers/dma-buf/st-dma-fence.c?

In drivers/dma-buf/st-dma-fence.c, test_signaling() (added in commit
3695d754ffbb ("dma-buf/selftests: test RCU ops and inline lock v2"))
creates a mock fence using mock_ops (which defines neither .release nor
.wait), signals it via dma_fence_check_and_signal(), and then checks:

	if (rcu_dereference_protected(f->ops, true)) {
		KUNIT_FAIL(test, "Fence ops not cleared on signal");
		goto err_free;
	}

Since dma_fence_signal_timestamp_locked() no longer clears fence->ops to
NULL, won't this KUnit check unconditionally fail?  Additionally, passing
the non-__rcu f->ops pointer to rcu_dereference_protected() there will
trigger a Sparse address-space warning.

>  	/* Stash the cb_list before replacing it with the timestamp */
>  	list_replace(&fence->cb_list, &cb_list);
>  

[ ... ]

> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index dd07d128adc3b..874848a4aec46 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -72,7 +72,7 @@ struct dma_fence {
>  		spinlock_t *extern_lock;
>  		spinlock_t inline_lock;
>  	};
> -	const struct dma_fence_ops __rcu *ops;
> +	const struct dma_fence_ops *ops;

[Severity: Low]
Should the remaining rcu_access_pointer(fence->ops) callers in the tree
also be updated now that the __rcu annotation is removed from
struct dma_fence::ops?

While dma_fence_is_array() and dma_fence_is_chain() are updated below,
several other functions converted in commits f4cc3ab824d6, 764f241ad227,
and 7519ba791b00 still wrap fence->ops in rcu_access_pointer(), which
causes Sparse to emit address-space mismatch warnings when building with
C=1:

drivers/gpu/drm/drm_crtc.c:fence_to_crtc():
	BUG_ON(rcu_access_pointer(fence->ops) != &drm_crtc_fence_ops);

drivers/gpu/drm/scheduler/sched_fence.c:to_drm_sched_fence():
	if (rcu_access_pointer(f->ops) == &drm_sched_fence_ops_scheduled)
		return container_of(f, struct drm_sched_fence, scheduled);

	if (rcu_access_pointer(f->ops) == &drm_sched_fence_ops_finished)
		return container_of(f, struct drm_sched_fence, finished);

drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c:to_amdgpu_amdkfd_fence():
	if (rcu_access_pointer(f->ops) == &amdkfd_fence_ops)
		return fence;

drivers/gpu/host1x/fence.c:host1x_fence_extract():
	if (rcu_access_pointer(fence->ops) != &host1x_syncpt_fence_ops)
		return -EINVAL;

>  	/*
>  	 * We clear the callback list on kref_put so that by the time we
>  	 * release the fence it is unused. No one should be adding to the

[ ... ]

> @@ -769,7 +761,7 @@ extern const struct dma_fence_ops dma_fence_chain_ops;
>   */
>  static inline bool dma_fence_is_array(struct dma_fence *fence)
>  {
> -	return rcu_access_pointer(fence->ops) == &dma_fence_array_ops;
> +	return fence->ops == &dma_fence_array_ops;
>  }
>  
>  /**
> @@ -780,7 +772,7 @@ static inline bool dma_fence_is_array(struct dma_fence *fence)
>   */
>  static inline bool dma_fence_is_chain(struct dma_fence *fence)
>  {
> -	return rcu_access_pointer(fence->ops) == &dma_fence_chain_ops;
> +	return fence->ops == &dma_fence_chain_ops;
>  }
>  
>  /**

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020100034.759183-2-phasta@kernel.org?part=1