Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [Critical] ABI breakage in UAPI `sync_fence_info` and severe string truncation. Pre-existing issues: - [Critical] Potential NULL pointer dereference due to unordered reads of `fence->ops` and `fence->flags`. -- commit 2d4e0eac87d76c56e73514e9ac37b9db142b0e4f Author: Philipp Stanner <[email protected]> dma-buf/dma-fence: Set better string for dma_fence_driver_name() Rename the "detached-driver" string returned by dma_fence_driver_name() to something more descriptive for signaled fences. > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > index b3bfa6943a8e1..4a326d2ba7d19 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -1171,7 +1171,7 @@ const char __rcu *dma_fence_driver_name(struct > dma_fence *fence) > if (!dma_fence_test_signaled_flag(fence)) > return (const char __rcu *)ops->get_driver_name(fence); [Severity: Critical] This is a pre-existing issue, but could this cause a NULL pointer dereference? In dma_fence_driver_name(), fence->ops and fence->flags are read independently without an smp_rmb() memory barrier. If the CPU reads fence->flags as unsignaled first, and concurrently another CPU signals the fence, dma_fence_signal_timestamp_locked() can set fence->flags to signaled and then set fence->ops to NULL (for fences lacking .release and .wait ops, such as virtgpu_fence or v3d_fence). The reader CPU might then read fence->ops as NULL, take the if branch (using the cached flags=0), and crash on ops->get_driver_name(fence). > else > - return (const char __rcu *)"detached-driver"; > + return (const char __rcu *)"driver-whose-fence-is-signaled"; [Severity: Critical] Does this break the userspace ABI and cause string truncation? The driver_name field in struct sync_fence_info is exposed to userspace via the SYNC_IOC_FILE_INFO ioctl. Changing this string from "detached-driver" to "driver-whose-fence-is-signaled" breaks userspace tools that rely on the established string to identify decoupled fences. Also, this formats the UAPI sync_file name into a 32-byte buffer in sync_file_get_name(): drivers/dma-buf/sync_file.c:sync_file_get_name() { ... snprintf(buf, len, "%s-%s%llu-%lld", rcu_dereference(driver), rcu_dereference(timeline), fence->context, fence->seqno); ... } The new 30-character driver name consumes almost the entire 32-byte buffer, which truncates the timeline name and all subsequent fence metadata. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
