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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm_work_fence_cancel_sync() fails to synchronize with the dma-fence callback, allowing queue_work() to be called concurrently with driver teardown.
- [High] The driver's ops->destroy() callback can unexpectedly be executed in hard IRQ context during the cancellation race.
--

commit 781c5ea671786cbee47b9bed0159aa892cc5e2b7
Author: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>

drm: Add drm_work_fence helper

This commit introduces drm_work_fence, a helper that queues a work item
when a dma-fence signals. It manages dma-fence callbacks, kref lifetime,
and workqueue dispatch.

> diff --git a/drivers/gpu/drm/drm_work_fence.c b/drivers/gpu/drm/drm_work_fence.c
> new file mode 100644
> index 0000000000000..5e0cbc53edb80
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_work_fence.c
[ ... ]
> +static void drm_work_fence_destroy(struct kref *kref)
> +{
> +	struct drm_work_fence *wfence =
> +		container_of(kref, struct drm_work_fence, refcount);
> +	struct dma_fence *fence = wfence->fence;
> +
> +	wfence->ops->destroy(wfence);

[Severity: High]
Can this ops->destroy() callback be executed in hard IRQ context?

If a dma-fence callback races with driver teardown and queue_work() fails,
the final reference might be dropped directly from the IRQ callback:

drm_work_fence_cb() -> drm_work_fence_queue()

If queue_work() returns false, drm_work_fence_put() drops the last reference
here, triggering this destroy callback under the dma-fence spinlock with
IRQs disabled.

> +	dma_fence_put(fence);	/* NULL-safe */
> +}
[ ... ]
> +static void drm_work_fence_queue(struct drm_work_fence *wfence)
> +{
> +	if (!queue_work(wfence->wq, &wfence->work))
> +		drm_work_fence_put(wfence);
> +}

[Severity: High]
Is there a use-after-free risk if wfence->wq is accessed here during a
concurrent driver teardown?

This relies on drm_work_fence_cancel_sync() successfully synchronizing
against the callback. However, if the synchronization fails, wfence->wq
could be accessed after the workqueue has been destroyed by the caller.

[ ... ]
> +void drm_work_fence_cancel_sync(struct drm_work_fence *wfence)
> +{
> +	if (drm_work_fence_cancel(wfence))
> +		return;
> +	if (disable_work_sync(&wfence->work))
> +		drm_work_fence_put(wfence);
> +}

[Severity: High]
Does this provide the expected synchronous cancellation guarantee, or does
it leave a window for concurrent execution during teardown?

If the dma-fence signals, the callback is dequeued but hasn't called
queue_work() yet. If drm_work_fence_cancel_sync() is called concurrently:

1. drm_work_fence_cancel() calls dma_fence_remove_callback(), which
   returns false because the callback is already dequeued.
2. disable_work_sync() returns false immediately because the work is
   not yet queued.
3. The function returns, falsely indicating the work is fully canceled.

This allows the driver teardown to proceed while the in-flight callback
might then call queue_work() on a potentially destroyed workqueue.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909044454.399340-1-srinivasan.shanmugam@amd.com?part=1