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 <[email protected]> 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/[email protected]?part=2
