Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] Missing NULL check for `ops` after `rcu_dereference()` allows a potential NULL pointer dereference on weakly-ordered CPUs if the fence is concurrently signaled. -- commit 0888eeb31d9224a94ad809f7df8b6fc25b78b27a Author: André Draszik <[email protected]> dma-fence: use correct callback in dma_fence_timeline_name() dma_fence_timeline_name() is a wrapper around dma_fence_ops::get_timeline_name(). Since the blamed commit below, it calls an incorrect callback. Update it to restore functionality by calling the intended callback. > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > index b3bfa6943a8e1..5292d714419b1 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -1202,7 +1202,7 @@ const char __rcu *dma_fence_timeline_name(struct > dma_fence *fence) > /* RCU protection is required for safe access to returned string */ > ops = rcu_dereference(fence->ops); > if (!dma_fence_test_signaled_flag(fence)) > - return (const char __rcu *)ops->get_driver_name(fence); > + return (const char __rcu *)ops->get_timeline_name(fence); [Severity: High] This isn't a bug introduced by this patch, but is there a missing NULL check for ops here? Because rcu_dereference() does not provide a read memory barrier against independent loads, can weakly-ordered CPUs (like ARM64) execute the load of fence->flags in dma_fence_test_signaled_flag() before the load of fence->ops? If a concurrent writer in dma_fence_signal_timestamp_locked() sets the signaled bit and then sets fence->ops to NULL, it seems a reader could observe fence->flags as unsignaled (0) and then subsequently load fence->ops as NULL. This could cause the reader to take the true path of the if statement and dereference a NULL ops pointer: Reader thread in dma_fence_timeline_name(): ops = rcu_dereference(fence->ops); // delayed load, gets NULL if (!dma_fence_test_signaled_flag(fence)) // loaded early, gets 0 ops->get_timeline_name(fence); // oops: ops is NULL Concurrent writer thread: dma_fence_signal_timestamp_locked() test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags); RCU_INIT_POINTER(fence->ops, NULL); Since this code path is reachable from userspace via the SYNC_IOC_FILE_INFO ioctl in sync_file.c, could this allow a local denial of service? Note that the adjacent dma_fence_driver_name() function seems to suffer from the exact same pre-existing issue. > else > return (const char __rcu *)"signaled-timeline"; > } -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
