Re: [PATCH 2/6] accel/qaic: Replace transfer completion with dma-fence

[sashiko-bot@kernel.org]

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 <carl.vanderlip@oss.qualcomm.com>
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/20261003023818.1161666-1-carl.vanderlip@oss.qualcomm.com?part=2