On Tue, Sep 08, 2026 at 08:58:39AM +0000, [email protected] wrote: All of this looks right. So I believe the solution is:
- s/cancel_work_/disable_work_ - In drm_work_fence_queue if queue_work returns false, call drm_work_fence_put() Matt > 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
