Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Critical] Race conditions in dma-fence signaling and callback addition allow deferred work to be queued after cancellation completes, leading to Use-After-Free. --
commit 43b8d6b8cd3ff463894478af19fe890e4879d181 Author: Srinivasan Shanmugam <[email protected]> Subject: drm: Add drm_work_fence helper GPU drivers often need to queue work when a dma-fence signals because certain operations (copy_to_user, eventfd_signal, memory allocation) cannot run in IRQ context. This helper handles the dma-fence-callback-to-workqueue pattern. > diff --git a/drivers/gpu/drm/drm_work_fence.c > b/drivers/gpu/drm/drm_work_fence.c > index 0000000000000..ac66ba129ffff > --- /dev/null > +++ b/drivers/gpu/drm/drm_work_fence.c [ ... ] > +static void drm_work_fence_cb(struct dma_fence *fence, struct dma_fence_cb > *cb) > +{ > + struct drm_work_fence *wfence = > + container_of(cb, struct drm_work_fence, cb); > + > + drm_work_fence_queue(wfence); > + /* Single ref: wfence->fence released in drm_work_fence_destroy(). */ > +} [Severity: Critical] Does this callback execution leave a race window? The callback in drm_work_fence_cb() executes asynchronously. Between its execution and the call to queue_work(), there appears to be no state indicating to cancel_work_sync() that the work is active. [ ... ] > +int drm_work_fence_add_callback(struct drm_work_fence *wfence, > + struct dma_fence *fence) > +{ > + int err; > + > + drm_work_fence_get(wfence); > + wfence->fence = fence; /* transfer caller's ref — single ref, no get */ > + > + err = dma_fence_add_callback(fence, &wfence->cb, drm_work_fence_cb); > + if (err == -ENOENT) { > + drm_work_fence_queue(wfence); > + err = 0; [Severity: Critical] Does this -ENOENT fast path also contain the same race window? If drm_work_fence_cancel_sync() is called by a teardown thread just before drm_work_fence_queue() is executed here, it seems cancel_work_sync() could fail to wait for the work to finish. [ ... ] > +void drm_work_fence_cancel_sync(struct drm_work_fence *wfence) > +{ > + if (drm_work_fence_cancel(wfence)) > + return; > + if (cancel_work_sync(&wfence->work)) > + drm_work_fence_put(wfence); > +} [Severity: Critical] Could this logic lead to a use-after-free regression during teardown? This function seems to assume that if the callback is not in the list, either the work is already pending/running, or the work has finished. It appears to ignore the window where the work is about to be queued but is not yet pending. Can this sequence occur when an unprivileged userspace application concurrently signals a fence and destroys the context? CPU1 (signaling thread or -ENOENT path) Removes callback from the list, then executes drm_work_fence_cb() but is preempted before calling queue_work(). CPU2 (teardown thread) sees this in drm_work_fence_cancel_sync(): if (drm_work_fence_cancel(wfence)) // Returns false because callback is not in the list if (cancel_work_sync(&wfence->work)) // Returns false immediately because work hasn't been queued yet Function returns, allowing the driver to tear down resources. CPU1 resumes: Calls queue_work(), and the worker executes, accessing the freed driver resources. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
