Re: [RFC PATCH v2 2/3] dma-fence: use lock to protect against module-unload

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] dma-fence: deadlock in dma_fence_set_deadline() holding fence lock
- [High] dma-fence: recursive lock deadlock in dma_fence_is_signaled()
- [High] dma-fence: module .rodata UAF on ops in dma_fence_set_deadline()
--

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

dma-fence: use lock to protect against module-unload

This commit acquires the dma_fence spinlock in dma_fence_set_deadline()
and dma_fence_is_signaled() to serialize fence->ops callback invocations
against signaling and module unload.

> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index 5dab647e3f3f9..e4350bce9cf9b 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -997,9 +997,14 @@ EXPORT_SYMBOL(dma_fence_wait_any_timeout);
>  void dma_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
>  {
>  	const struct dma_fence_ops *ops = fence->ops;
> +	unsigned long flags;
> +
> +	dma_fence_lock_irqsave(fence, flags);
>  
> -	if (ops->set_deadline && !dma_fence_is_signaled(fence))
> +	if (ops->set_deadline && !dma_fence_is_signaled_locked(fence))
>  		ops->set_deadline(fence, deadline);
> +
> +	dma_fence_unlock_irqrestore(fence, flags);
>  }
>  EXPORT_SYMBOL(dma_fence_set_deadline);

[Severity: High]
Will holding the fence spinlock across ops->set_deadline() here cause
deadlocks in the existing set_deadline implementations?

The comment on struct dma_fence_ops in include/linux/dma-fence.h states:

	 * This is called without &dma_fence.lock held, it can be called
	 * multiple times and from any context.  Locking is up to the callee
	 * if it has some state to manage.

All three non-container implementations of set_deadline acquire the
fence's own spinlock inside the callback, which will self-deadlock on the
same CPU when called on an unsignaled fence (for example via the
SYNC_IOC_SET_DEADLINE ioctl or DRM atomic commit paths):

1. drivers/gpu/drm/scheduler/sched_fence.c:
   drm_sched_fence_set_deadline_finished() calls
   dma_fence_lock_irqsave(f, flags) on the same fence:

	dma_fence_lock_irqsave(f, flags);

	/* If we already have an earlier deadline, keep it: */
	if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) &&
	    ktime_before(fence->deadline, deadline)) {
		dma_fence_unlock_irqrestore(f, flags);
		return;
	}

   In addition, drm_sched_fence_set_deadline_finished() then propagates
   the deadline to the hardware parent fence:

	parent = smp_load_acquire(&fence->parent);
	if (parent)
		dma_fence_set_deadline(parent, deadline);

   If the outer dma_fence_set_deadline() holds s_fence->finished's lock
   across ops->set_deadline(), calling dma_fence_set_deadline(parent, ...)
   acquires parent's lock under s_fence->finished's lock. However, job
   completion in drivers/gpu/drm/scheduler/sched_main.c runs
   dma_fence_signal(parent) -> drm_sched_job_done_cb() ->
   drm_sched_job_done() -> drm_sched_fence_finished() ->
   dma_fence_signal(&s_fence->finished), which acquires
   s_fence->finished's lock while holding parent's lock, creating an
   AB-BA lock-order inversion.

2. drivers/gpu/drm/msm/msm_fence.c:
   msm_fence_set_deadline() acquires &fctx->spinlock:

	spin_lock_irqsave(&fctx->spinlock, flags);

   which is the fence's extern_lock passed to dma_fence_init() in
   msm_fence_init().

3. drivers/dma-buf/sw_sync.c:
   timeline_fence_set_deadline() acquires the fence lock directly:

	dma_fence_lock_irqsave(fence, flags);

[Severity: High]
Also in dma_fence_set_deadline() above, can this still access freed
driver memory or unmapped module .rodata when called on an already
signaled fence after module unload?

First, because there is no unlocked dma_fence_test_signaled_flag(fence)
check before dma_fence_lock_irqsave(fence, flags), calling
dma_fence_set_deadline() on a signaled fence unconditionally touches
dma_fence_spinlock(fence), which may be an extern_lock freed by the
driver after signaling.

Second, inside the lock:

	if (ops->set_deadline && !dma_fence_is_signaled_locked(fence))

evaluates ops->set_deadline before checking
!dma_fence_is_signaled_locked(fence). Since fence->ops is no longer set to
NULL on signaling (after parent commit 4607587506fc ("dma-fence: Remove
RCU protection for fence->ops")), calling dma_fence_set_deadline() on a
signaled fence after the producer module has unloaded will dereference
ops->set_deadline in the unmapped module's .rodata section.

Similarly, the commit message notes that release() and wait() are
deprecated, but even when a modular driver leaves .release and .wait as
NULL in its struct dma_fence_ops table, dma_fence_release():

	if (ops->release)
		ops->release(fence);
	else
		dma_fence_free(fence);

and dma_fence_wait_timeout():

	if (ops->wait) {
		ret = ops->wait(fence, intr, timeout);
	} else {
		ret = dma_fence_default_wait(fence, intr, timeout);
	}

still dereference the ops table in the module's .rodata to check whether
those pointers are NULL when a consumer waits on or drops the last
reference to a signaled fence after the module has unloaded.

> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index 874848a4aec46..2223d75c5a496 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -549,16 +549,29 @@ static inline bool
>  dma_fence_is_signaled(struct dma_fence *fence)
>  {
>  	const struct dma_fence_ops *ops = fence->ops;
> +	unsigned long flags;
> +	bool ret = false;
>  
> +	/* Fast-path optimization. */
>  	if (dma_fence_test_signaled_flag(fence))
>  		return true;
>  
> -	if (ops && ops->signaled && ops->signaled(fence)) {
> -		dma_fence_signal(fence);
> -		return true;
> +	dma_fence_lock_irqsave(fence, flags);
> +
> +	if (dma_fence_test_signaled_flag(fence)) {
> +		ret = true;
> +		goto out;
>  	}
>  
> -	return false;
> +	if (ops->signaled && ops->signaled(fence)) {
> +		ret = true;
> +		dma_fence_signal_locked(fence);
> +	}
> +
> +out:
> +	dma_fence_unlock_irqrestore(fence, flags);
> +
> +	return ret;
>  }

[Severity: High]
Can acquiring dma_fence_lock_irqsave() unconditionally in
dma_fence_is_signaled() cause a recursive spinlock deadlock in
amdkfd_fence_enable_signaling()?

In drivers/dma-buf/dma-fence.c, __dma_fence_enable_signaling() holds the
fence spinlock and calls ops->enable_signaling(fence) when the fence is
not yet signaled:

	dma_fence_assert_held(fence);

	was_set = test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
				   &fence->flags);

	if (dma_fence_test_signaled_flag(fence))
		return false;

	if (!was_set && ops->enable_signaling) {
		trace_dma_fence_enable_signal(fence);

		if (!ops->enable_signaling(fence)) {

In drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c,
amdkfd_fence_enable_signaling() (where amdkfd_fence_ops.signaled is NULL)
calls dma_fence_is_signaled(f) and
kgd2kfd_schedule_evict_and_restore_process():

static bool amdkfd_fence_enable_signaling(struct dma_fence *f)
{
	struct amdgpu_amdkfd_fence *fence = to_amdgpu_amdkfd_fence(f);

	if (!fence)
		return false;

	if (dma_fence_is_signaled(f))
		return true;

	if (!kgd2kfd_schedule_evict_and_restore_process(fence->mm, fence->context_id, f))
		return true;

and kgd2kfd_schedule_evict_and_restore_process() in
drivers/gpu/drm/amd/amdkfd/kfd_device.c also checks:

	if (dma_fence_is_signaled(fence))
		return 0;

Previously, dma_fence_is_signaled(f) saw that ops->signaled was NULL and
returned false without taking the lock. With this change, when
DMA_FENCE_FLAG_SIGNALED_BIT is not set, dma_fence_is_signaled(f) attempts
to acquire f's spinlock while __dma_fence_enable_signaling(f) already
holds it on the same CPU.

!]

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