Re: [PATCH v8 1/3] drm: Add drm_work_fence helper

[Matthew Brost <matthew.brost@intel.com>]

On Tue, Sep 08, 2026 at 08:58:39AM +0000, sashiko-bot@kernel.org 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 <srinivasan.shanmugam@amd.com>
> 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/20260908084627.352535-1-srinivasan.shanmugam@amd.com?part=1