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

[sashiko-bot@kernel.org]

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