Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Critical] Moving the signaled bit update creates a window where `fence->ops` is cleared to NULL but the fence is not yet marked as signaled, causing a NULL pointer dereference in lockless accessors. - [High] Missing release memory barrier before setting the signaled bit, defeating the purpose of the patch on weakly-ordered architectures and risking use-after-free. - [High] Delaying the signaled bit breaks the fast-path lockless check in `dma_fence_is_signaled()`, leading to lock contention, recursive deadlocks, and API contract violations. --
commit 0ba6de5d00b831f763affe10965d5c591e6f0b5f Author: Philipp Stanner <[email protected]> dma-buf/dma_fence: Make races for dma_fence_is_signaled() less likely This commit addresses a race condition where dma_fence_is_signaled() can return true while fence callbacks are still executing. It delays setting the DMA_FENCE_FLAG_SIGNALED_BIT until after the callbacks have completed, instead of setting it at the beginning of dma_fence_signal_timestamp_locked(). > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -359,8 +359,19 @@ void dma_fence_signal_timestamp_locked(struct dma_fence > *fence, > > dma_fence_assert_held(fence); > > - if (unlikely(test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, > - &fence->flags))) > + /* > + * First test the bit, so we don't signal an already signaled fence > again. > + * The lock protects against multiple parties setting the bit. The bit > + * is then set at the end of the function. > + * > + * The background is that there is a fast path check in > + * dma_fence_is_signaled() which does not use lock protection and can > + * return true *while* the fence callbacks are still executing. > + * > + * This fast path check supposedly cannot be guarded by the lock because > + * of significant performance regressions. > + */ > + if (unlikely(test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))) > return; > > trace_dma_fence_signaled(fence); [Severity: Critical] Because the signaled bit is no longer set here, is there a risk of a NULL pointer dereference for concurrent lockless readers? Just below this in dma_fence_signal_timestamp_locked(), fence->ops can be cleared: ops = rcu_dereference_protected(fence->ops, true); if (!ops->release && !ops->wait) RCU_INIT_POINTER(fence->ops, NULL); With the signaled bit still false, could lockless accessors like dma_fence_driver_name() see the unsignaled state and evaluate ops->get_driver_name(fence) on the NULL ops pointer? [ ... ] > @@ -384,6 +395,9 @@ void dma_fence_signal_timestamp_locked(struct dma_fence > *fence, > INIT_LIST_HEAD(&cur->node); > cur->func(fence, cur); > } [Severity: High] If a callback executes while the signaled bit is still false, won't this break the lockless fast-path check in dma_fence_is_signaled()? If dma_fence_is_signaled() or dma_fence_timestamp() is called from within a callback, the unsignaled state would cause dma_fence_timestamp() to hit a WARN_ON(!test_bit(...)). Additionally, dma_fence_is_signaled() would fall back to dma_fence_signal(), which attempts to acquire the fence->lock. Since the lock is already held by the current thread executing the callbacks, would this result in a recursive deadlock? > + > + // TODO: we need some barrier here, don't we? > + set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags); [Severity: High] Does this missing memory barrier reintroduce the use-after-free risk on weakly-ordered architectures? Since set_bit() does not imply a memory barrier, could the CPU reorder this assignment to become globally visible before the memory writes performed by the callbacks? If a concurrent lockless reader using dma_fence_test_signaled_flag() observes the bit as set but reads stale data, this might defeat the purpose of the patch. Is an explicit smp_mb__before_atomic() needed here? > } > EXPORT_SYMBOL(dma_fence_signal_timestamp_locked); -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
