Re: [PATCH v6 1/4] drm: Add drm_work_fence helper

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

On Mon, Aug 31, 2026 at 07:15:36PM +0530, Srinivasan Shanmugam wrote:
> 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 pattern is currently
> open-coded in multiple drivers.
> 
> Introduce drm_work_fence — an embeddable base structure that handles
> the dma-fence-callback-to-workqueue pattern in one place. Drivers
> embed this in their own structure and implement ops->work() for the
> deferred work and ops->destroy() for cleanup.
> 
> The helper manages:
>  - kref lifetime
>  - dma-fence callback registration
>  - workqueue dispatch on fence signal
>  - safe cancellation before driver teardown
> 
> For work that additionally requires borrowing the process MM via
> kthread_use_mm(), see drm_user_fence which builds on top of this.
> 
> Suggested-by: Matthew Brost <matthew.brost@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: intel-xe@lists.freedesktop.org
> Cc: amd-gfx@lists.freedesktop.org
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
> ---
>  drivers/gpu/drm/Makefile         |   1 +
>  drivers/gpu/drm/drm_work_fence.c | 195 +++++++++++++++++++++++++++++++
>  include/drm/drm_work_fence.h     |  76 ++++++++++++
>  3 files changed, 272 insertions(+)
>  create mode 100644 drivers/gpu/drm/drm_work_fence.c
>  create mode 100644 include/drm/drm_work_fence.h
> 
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index e97faabcd783..c5be8e80d0c8 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -72,6 +72,7 @@ drm-y := \
>  	drm_vblank.o \
>  	drm_vblank_work.o \
>  	drm_vma_manager.o \
> +	drm_work_fence.o \
>  	drm_writeback.o
>  drm-$(CONFIG_DRM_CLIENT) += \
>  	drm_client.o \
> diff --git a/drivers/gpu/drm/drm_work_fence.c b/drivers/gpu/drm/drm_work_fence.c
> new file mode 100644
> index 000000000000..9f6b779d0fe9
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_work_fence.c
> @@ -0,0 +1,195 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2024 The Linux Foundation
> + *
> + * Common DRM work fence helper.
> + *
> + * When a GPU dma-fence signals, drivers often need to perform work that
> + * cannot run in IRQ context (e.g., memory allocation, copy_to_user,
> + * eventfd_signal). This helper queues a work item when a dma-fence
> + * signals, allowing that work to run safely in a workqueue context.
> + *
> + * NOTE: This helper consumes dma_fences but CANNOT implement
> + * dma_fence_ops. Work items queued here may sleep; dma_fence_ops
> + * callbacks are called under the fence spinlock and must not sleep.
> + *
> + * For work that additionally requires accessing userspace memory via
> + * kthread_use_mm(), see drm_user_fence which builds on top of this.
> + */
> +
> +#include <linux/workqueue.h>
> +
> +#include <drm/drm_work_fence.h>
> +
> +static void drm_work_fence_destroy(struct kref *kref)
> +{
> +	struct drm_work_fence *wfence =
> +		container_of(kref, struct drm_work_fence, refcount);
> +
> +	if (wfence->fence)
> +		dma_fence_put(wfence->fence);
> +
> +	wfence->ops->destroy(wfence);

I'd invert these for safety in case destroy wants to looks at the fence,
admittedly that would be an odd use case.

So...

	struct drm_work_fence *wfence =
		container_of(kref, struct drm_work_fence, refcount);
	struct dma_fence *fence = wfence->fence;

	wfence->ops->destroy(wfence);
	dma_fence_put(fence);	/* this has a NULL check */


> +}
> +
> +/**
> + * drm_work_fence_get - Acquire a reference to a work fence
> + * @wfence: work fence
> + */
> +void drm_work_fence_get(struct drm_work_fence *wfence)
> +{
> +	kref_get(&wfence->refcount);
> +}
> +EXPORT_SYMBOL_GPL(drm_work_fence_get);
> +
> +/**
> + * drm_work_fence_put - Release a reference to a work fence
> + * @wfence: work fence
> + */
> +void drm_work_fence_put(struct drm_work_fence *wfence)
> +{
> +	kref_put(&wfence->refcount, drm_work_fence_destroy);
> +}
> +EXPORT_SYMBOL_GPL(drm_work_fence_put);
> +
> +static void drm_work_fence_work(struct work_struct *w)
> +{
> +	struct drm_work_fence *wfence =
> +		container_of(w, struct drm_work_fence, work);
> +
> +	wfence->ops->work(wfence);
> +	drm_work_fence_put(wfence);
> +}
> +
> +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);
> +
> +	queue_work(wfence->wq, &wfence->work);
> +	/*
> +	 * Put the transferred reference from add_callback. The stored
> +	 * reference in wfence->fence is released in drm_work_fence_destroy().
> +	 */
> +	dma_fence_put(fence);
> +}
> +
> +/**
> + * drm_work_fence_init - Initialize a work fence
> + * @wfence: work fence to initialize
> + * @wq: workqueue to run the worker on (must be ordered if sequencing matters)
> + * @ops: driver operations
> + */
> +void drm_work_fence_init(struct drm_work_fence *wfence,
> +			 struct workqueue_struct *wq,
> +			 const struct drm_work_fence_ops *ops)
> +{
> +	kref_init(&wfence->refcount);
> +	wfence->wq = wq;
> +	wfence->ops = ops;
> +	wfence->fence = NULL;
> +	INIT_WORK(&wfence->work, drm_work_fence_work);
> +}
> +EXPORT_SYMBOL_GPL(drm_work_fence_init);
> +
> +/**
> + * drm_work_fence_add_callback - Attach a work fence to a dma-fence
> + * @wfence: work fence
> + * @fence: dma-fence to watch; ownership of this reference is transferred
> + *         to the callback — caller must NOT put it afterward.

This isn't right. It is perfectly reasonable for caller to hold more
than 1 reference to @fence, thus put it again. It consumes a single
reference @fence on success or failure - that is it.

> + *
> + * When @fence signals, a work item is queued that calls ops->work().
> + * If @fence has already signaled, the work item is queued immediately.
> + *
> + * An additional reference to @fence is stored internally in @wfence to
> + * allow drm_work_fence_cancel() to be called safely without the caller
> + * needing to hold a separate fence reference.
> + *

Ideally get rid of double ref count on @fence. I don't think above
reasoning justifies the needed for a double ref on the fence. I'd tie
exactly one refernece @fence which is attached to lifetime of @wfence
(i.e., drop the dma_fence_put in drm_work_fence_cb).

> + * On any return value the caller's fence reference is consumed.
> + *

I'd mention regardless of success or fail, a reference to drm_work_fence
is consumed too.

> + * Return: 0 on success, negative errno on error.
> + */
> +int drm_work_fence_add_callback(struct drm_work_fence *wfence,
> +				struct dma_fence *fence)
> +{
> +	int err;
> +
> +	drm_work_fence_get(wfence);
> +	wfence->fence = dma_fence_get(fence);
> +
> +	err = dma_fence_add_callback(fence, &wfence->cb, drm_work_fence_cb);
> +	if (err == -ENOENT) {
> +		queue_work(wfence->wq, &wfence->work);
> +		dma_fence_put(fence);

Keep the implementation in one place?

drm_work_fence_work(&wfence->work);

> +		err = 0;
> +	} else if (err) {
> +		dma_fence_put(wfence->fence);
> +		wfence->fence = NULL;
> +		drm_work_fence_put(wfence);

Won't drm_work_fence_put just drop the 'wfence->fence' reference if
'wfence->fence' isn't set to NULL. i.e., drm_work_fence_put(wfence) can
replace the above 3 lines.

> +		dma_fence_put(fence);
> +	}
> +	/* on success: transferred ref goes to drm_work_fence_cb */
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(drm_work_fence_add_callback);
> +
> +/**
> + * drm_work_fence_cancel - Cancel a pending work fence callback
> + * @wfence: work fence
> + *
> + * Attempts to remove the pending callback before driver context teardown.
> + * The caller must hold a reference to @wfence across this call.
> + *
> + * If the callback has already fired this returns false and all cleanup
> + * has been handled internally.
> + *
> + * If removal succeeds the callback reference is released internally.
> + * The caller must still release its own reference via drm_work_fence_put().
> + *
> + * This function is safe to call from atomic context as it only acquires
> + * the dma-fence spinlock internally. If the caller also needs to wait
> + * for the worker to finish, use drm_work_fence_cancel_sync() instead,
> + * which may sleep.
> + *
> + * Return: true if callback was removed, false if it had already fired.
> + */
> +bool drm_work_fence_cancel(struct drm_work_fence *wfence)
> +{
> +	struct dma_fence *fence = wfence->fence;
> +
> +	if (!fence)
> +		return false;
> +
> +	if (dma_fence_remove_callback(fence, &wfence->cb)) {
> +		wfence->fence = NULL;
> +		dma_fence_put(fence);  /* callback ref */
> +		dma_fence_put(fence);  /* stored ref */
> +		drm_work_fence_put(wfence);

Same comments as above: No need for 'wfence->fence = NULL' and
drm_work_fence_put, drm_work_fence_put is work by itself. Also see my
comment about dropped the double ref, that isn't need either.

> +		return true;
> +	}
> +
> +	return false;
> +}
> +EXPORT_SYMBOL_GPL(drm_work_fence_cancel);
> +
> +/**
> + * drm_work_fence_cancel_sync - Cancel callback and wait for worker to finish
> + * @wfence: work fence
> + *
> + * Calls drm_work_fence_cancel() then cancel_work_sync() to guarantee
> + * the worker has fully completed before returning.
> + *
> + * This function may sleep. Must not be called from atomic or interrupt
> + * context. Use drm_work_fence_cancel() instead when sleeping is not allowed.
> + *
> + * Drivers must call this during teardown before freeing any resources
> + * accessed by ops->work().
> + */
> +void drm_work_fence_cancel_sync(struct drm_work_fence *wfence)
> +{
> +	drm_work_fence_cancel(wfence);
> +	if (cancel_work_sync(&wfence->work))
> +		drm_work_fence_put(wfence);

This will UAF if drm_work_fence_cancel removed the callback.

I actually don't think drm_work_fence_cancel, drm_work_fence_cancel_sync
is safe unless the caller has reference to drm_work_fence.

Consider the following case:

- A driver calls drm_work_fence_add_callback
- Sometime later if calls drm_work_fence_cancel or drm_work_fence_cancel_sync
- drm_work_fence_work completes before either drm_work_fence_cancel,
  drm_work_fence_cancel_sync completes, we UAF

So with additional reference at the caller assumed...

I'd write this like:

if (drm_work_fence_cancel(wfence))
	return;	/* Worker not running, all internal refs dropped */

if (cancel_work_sync(&wfence->work))
	drm_work_fence_put(wfence);	/* Worker cancelled, drop it ref */

> +}
> +EXPORT_SYMBOL_GPL(drm_work_fence_cancel_sync);
> diff --git a/include/drm/drm_work_fence.h b/include/drm/drm_work_fence.h
> new file mode 100644
> index 000000000000..4fa369f937d7
> --- /dev/null
> +++ b/include/drm/drm_work_fence.h
> @@ -0,0 +1,76 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2024 The Linux Foundation
> + */
> +
> +#ifndef __DRM_WORK_FENCE_H__
> +#define __DRM_WORK_FENCE_H__
> +
> +#include <linux/dma-fence.h>
> +#include <linux/kref.h>
> +#include <linux/workqueue.h>
> +
> +struct drm_work_fence;
> +
> +/**
> + * struct drm_work_fence_ops - driver callbacks for a DRM work fence
> + */
> +struct drm_work_fence_ops {
> +	/**
> +	 * @work: Called from workqueue context when the dma-fence signals.
> +	 * Perform any work that cannot run in IRQ context here.
> +	 */
> +	void (*work)(struct drm_work_fence *wfence);
> +
> +	/**
> +	 * @destroy: Called when the last reference is dropped.
> +	 * Free the containing structure here.
> +	 */
> +	void (*destroy)(struct drm_work_fence *wfence);
> +};
> +
> +/**
> + * struct drm_work_fence - embeddable DRM work fence
> + *
> + * Provides a dma-fence callback that queues a work item when the fence
> + * signals, allowing work that cannot run in IRQ context to be deferred
> + * to a workqueue. Drivers embed this in their own structure.
> + *
> + * NOTE: This helper is a *consumer* of dma_fences only. It CANNOT be
> + * used to implement dma_fence_ops. dma_fence callbacks are invoked
> + * while holding the fence spinlock; work queued here may sleep
> + * (copy_to_user, kthread_use_mm, eventfd_signal) and must not be
> + * called under that spinlock.
> + *
> + * Call drm_work_fence_init() at creation and drm_work_fence_add_callback()
> + * to arm. Call drm_work_fence_cancel_sync() before driver teardown.
> + */
> +struct drm_work_fence {
> +	/** @refcount: Reference count. */
> +	struct kref refcount;
> +	/** @work: Work item queued when the dma-fence signals. */
> +	struct work_struct work;
> +	/** @cb: dma-fence callback. */
> +	struct dma_fence_cb cb;

You could likely use union trick here on work_struct, dma_fence_cb and
only defer the INIT_WORK to drm_work_fence_cb.

> +	/**
> +	 * @fence: Extra reference held for safe cancel(). Set during
> +	 * add_callback, released in destroy().
> +	 */

See my comments this ref count. Ideally: "A single reference held for
the lifetime of drm_work_fence after drm_work_fence_init is called"

Matt

> +	struct dma_fence *fence;
> +	/** @wq: Workqueue to run @work on. */
> +	struct workqueue_struct *wq;
> +	/** @ops: Driver operations. */
> +	const struct drm_work_fence_ops *ops;
> +};
> +
> +void drm_work_fence_init(struct drm_work_fence *wfence,
> +			 struct workqueue_struct *wq,
> +			 const struct drm_work_fence_ops *ops);
> +void drm_work_fence_get(struct drm_work_fence *wfence);
> +void drm_work_fence_put(struct drm_work_fence *wfence);
> +int drm_work_fence_add_callback(struct drm_work_fence *wfence,
> +				struct dma_fence *fence);
> +bool drm_work_fence_cancel(struct drm_work_fence *wfence);
> +void drm_work_fence_cancel_sync(struct drm_work_fence *wfence);
> +
> +#endif /* __DRM_WORK_FENCE_H__ */
> -- 
> 2.34.1
>