Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider: - [Critical] dma-fence: Unsignaled dma-fences and leaked callbacks on the error cleanup path - [Critical] qaic: Race condition in qaic_fence_sync_cb_func causes NULL pointer dereference - [Critical] dma-fence: Reusing embedded dma_fence_cb corrupts callback list pointers - [High] dma-mapping: Concurrent CPU and device SGT synchronization due to fence signaling race - [High] uapi: QAIC_WAIT_BO ioctl returns -ENOENT for idle buffers - [High] dma-fence: Reused sequence numbers violate monotonically increasing API contract - [Medium] dma-mapping: Cancelled BO executions skip CPU DMA synchronization - [Medium] uapi: Overriding -ERESTARTSYS with -EINTR breaks syscall restart ABI --
commit cb1752759fe062100cc452aaddf96c2432a9f99c Author: Carl Vanderlip <[email protected]> accel/qaic: Replace transfer completion with dma-fence This commit replaces completion mechanisms with dma-fences to signal finished IRQ work. It integrates buffer synchronization with dma_resv and prepares for drm_sched support. [Severity: Critical] In drivers/accel/qaic/qaic_data.c, within the send_bo_list_to_device function around line 1357, the error cleanup path removes previously queued buffer objects from the transfer list without calling dma_fence_signal or qaic_remove_bo_fence. Can this leak pending unsignaled fences into the dma_resv object? Because qaic_acquire_bo_fence already added the fence to the reservation object, it appears the unsignaled fence could remain there permanently. This might cause a deadlock for kernel or userspace components waiting on the buffer implicit sync barriers, and the embedded cb might remain registered to the leaked fence, resulting in a use-after-free when the buffer is destroyed. [Severity: High] Looking at qaic_fence_sync_cb_func in drivers/accel/qaic/qaic_fence.c around line 42, the CPU DMA sync is performed asynchronously when the fence signals. However, in send_bo_list_to_device in drivers/accel/qaic/qaic_data.c around line 1338, the device DMA sync runs upon resubmission. Could this race if the previous fence callback has not finished? A concurrent qaic_execute_bo_ioctl could see bo_queued as false, acquire the lock, and resubmit the transfer. This would allow the scheduler to run dma_sync_sgtable_for_device while the IRQ thread is still executing dma_sync_sgtable_for_cpu, potentially violating DMA API ownership rules and causing data corruption. [Severity: High] In the qaic_wait_bo_ioctl function in drivers/accel/qaic/qaic_data.c around line 1899, the code now returns -ENOENT if the fence is missing. Does this change break the userspace API behavior? Previously, a buffer with no pending operations returned 0 immediately because it waited on a pre-completed completion. Applications expecting success when waiting on an already-idle or newly-mapped buffer might now fail when receiving this error code instead. [Severity: Medium] When operations are aborted, such as in empty_xfer_list in drivers/accel/qaic/qaic_data.c around line 2115, the fences are signaled with -ECANCELED. Then, in qaic_fence_sync_cb_func in drivers/accel/qaic/qaic_fence.c around line 38, the callback explicitly checks the error status and returns early. Does this permanently skip the required CPU DMA sync for cancelled operations? If dma_sync_sgtable_for_cpu is skipped, the memory might be left indefinitely mapped in the device DMA domain, violating API ownership rules. [Severity: High] In drivers/accel/qaic/qaic_fence.c around line 77, qaic_create_fence initializes the dma_fence with a hardcoded sequence number of 1. The context is allocated once per buffer object in qaic_alloc_init_bo, located in drivers/accel/qaic/qaic_data.c around line 766. Is it correct to reuse the same sequence number on the same context for every submission? The dma-fence API requires sequence numbers on a given context to monotonically increase. Reusing sequence numbers could cause dma_fence_is_later to evaluate timelines incorrectly, potentially bypassing synchronization when buffers are shared. [Severity: Critical] There appears to be a race condition involving qaic_fence_sync_cb_func in drivers/accel/qaic/qaic_fence.c around line 41 and qaic_unprepare_bo in drivers/accel/qaic/qaic_data.c around line 994. The callback executes locklessly and accesses the device pointer and scatter-gather table. Concurrently, a thread calling qaic_detach_slice_bo_ioctl could see bo_queued return false, acquire the lock, and execute qaic_unprepare_bo. This tears down the DMA mappings and clears the channel pointer. Could the fence callback on another CPU dereference the newly cleared channel pointer and access the freed memory, leading to a null pointer dereference? [Severity: Critical] In qaic_set_bo_resv_fence in drivers/accel/qaic/qaic_fence.c around line 98, dma_fence_add_callback adds the embedded callback node to the new fence. Is it safe to reuse the embedded callback node for subsequent submissions? The bo_queued check evaluates to false immediately when the signaling thread sets the signaled bit, which could happen while it is still iterating over the old fence callback list. If a concurrent submission thread allocates a new fence and adds the callback, it could overwrite the node pointers, corrupting the kernel linked lists. [Severity: Medium] In qaic_fence_wait in drivers/accel/qaic/qaic_fence.c around line 54, the function checks if timeout is less than 0 and sets the return value to -EINTR. Since dma_fence_wait_timeout typically returns -ERESTARTSYS when interrupted by a signal, does mapping this internal restart instruction to an explicit failure break the syscall restart ABI? This change seems to disable the kernel automatic VFS syscall restart mechanism for the ioctl, forcing userspace to handle interrupted waits manually. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
