Re: [PATCH 1/2] drm: Add common drm_user_fence helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Lack of a teardown/cancellation API allows Use-After-Free of driver resources and module memory if a fence signals after context teardown.
- [Medium] Deferred dynamic initialization of `work_struct` leads to uninitialized memory access on teardown, data races, and breaks lockdep tracking.
--

commit 4ecbf619a439e885246ba05ae0272714c1c83eb8
Author: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>

drm: Add common drm_user_fence helper

Introduce a common DRM user fence helper providing the kref-managed,
MM-borrowing dma-fence-callback-to-workqueue pattern used by drivers
that must access userspace memory from a kthread context when a GPU
fence signals.

> diff --git a/drivers/gpu/drm/drm_user_fence.c b/drivers/gpu/drm/drm_user_fence.c
> index 0000000000000..bd76e3d03120e 100644
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_user_fence.c

[ ... ]

> +static void drm_user_fence_work(struct work_struct *w)
> +{
> +	struct drm_user_fence *ufence =
> +		container_of(w, struct drm_user_fence, work);
> +	bool mm_ok = false;
> +
> +	if (mmget_not_zero(ufence->mm)) {
> +		kthread_use_mm(ufence->mm);
> +		mm_ok = true;
> +	}
> +
> +	ufence->ops->worker(ufence, mm_ok);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
If the driver module has been unloaded after tearing down its context, will
this jump into unmapped driver module text and cause a kernel panic?

This can happen if a foreign dma-fence signals after driver teardown, as
there is no way to safely detach or cancel the user fence.

> +
> +	if (mm_ok) {
> +		kthread_unuse_mm(ufence->mm);
> +		mmput(ufence->mm);
> +	}
> +
> +	drm_user_fence_put(ufence);
> +}
> +
> +static void drm_user_fence_cb(struct dma_fence *fence, struct dma_fence_cb *cb)
> +{
> +	struct drm_user_fence *ufence =
> +		container_of(cb, struct drm_user_fence, cb);
> +
> +	INIT_WORK(&ufence->work, drm_user_fence_work);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Medium]
Will calling INIT_WORK() dynamically here instead of during object
initialization hide the work_struct state from teardown paths and break
lockdep tracking?

The work_struct remains completely uninitialized between the call to
drm_user_fence_init() and the fence signaling, preventing the use of standard
workqueue teardown functions prior to signaling.

> +	queue_work(ufence->wq, &ufence->work);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Could this dereference a freed workqueue if a foreign dma-fence signals
after the driver has torn down its context?

Since drm_user_fence_add_callback() takes a kref on the ufence and there is
no cancellation API, the ufence remains alive on the foreign dma-fence's
callback list even after the driver device is unbound or the module is
unloaded.

> +}
> +
> +/**
> + * drm_user_fence_init - Initialize a user fence
> + * @ufence: user fence to initialize
> + * @wq: workqueue to run the worker on (must be ordered if sequencing matters)
> + * @ops: driver operations
> + *
> + * Must be called from process context. Grabs a reference to current->mm.
> + */
> +void drm_user_fence_init(struct drm_user_fence *ufence,
> +			 struct workqueue_struct *wq,
> +			 const struct drm_user_fence_ops *ops)
> +{
> +	kref_init(&ufence->refcount);
> +	ufence->mm = current->mm;
> +	mmgrab(ufence->mm);
> +	ufence->wq = wq;
> +	ufence->ops = ops;
> +}

[Severity: Medium]
Should INIT_WORK() be called here instead?

Initializing the work_struct here would prevent lockdep class divergence and
ensure the work_struct is in a valid state for teardown functions.

> +EXPORT_SYMBOL_GPL(drm_user_fence_init);
> +
> +/**
> + * drm_user_fence_add_callback - Attach a user fence to a dma-fence
> + * @ufence: user fence
> + * @fence: dma-fence to watch; caller retains ownership of this reference
> + *
> + * When @fence signals, a work item is queued that calls ops->worker() with
> + * the process MM active. If @fence has already signaled the work item is
> + * queued immediately.
> + *
> + * Return: 0 on success, negative errno on error.
> + */
> +int drm_user_fence_add_callback(struct drm_user_fence *ufence,
> +				struct dma_fence *fence)
> +{
> +	int err;
> +
> +	drm_user_fence_get(ufence);
> +	err = dma_fence_add_callback(fence, &ufence->cb, drm_user_fence_cb);
> +	if (err == -ENOENT) {
> +		/* fence already signaled — queue work immediately */
> +		INIT_WORK(&ufence->work, drm_user_fence_work);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Medium]
Does calling INIT_WORK() dynamically in two different locations cause lockdep
class divergence?

Since INIT_WORK() allocates a static lock class key at the call site, the
work_struct will receive different lockdep keys depending on whether the
fence was already signaled or not.

> +		queue_work(ufence->wq, &ufence->work);
> +		err = 0;
> +	} else if (err) {
> +		drm_user_fence_put(ufence);
> +	}
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(drm_user_fence_add_callback);

> diff --git a/include/drm/drm_user_fence.h b/include/drm/drm_user_fence.h
> index 0000000000000..de8e3f47be186 100644
> --- /dev/null
> +++ b/include/drm/drm_user_fence.h

[ ... ]

> +/**
> + * struct drm_user_fence - embeddable DRM user fence
> + *
> + * Drivers embed this in their own structure and implement
> + * &drm_user_fence_ops. Call drm_user_fence_init() at creation and
> + * drm_user_fence_add_callback() to arm on a dma-fence.
> + */
> +struct drm_user_fence {
> +	/** @refcount: Reference count. */
> +	struct kref refcount;
> +	/** @mm: Process MM grabbed at init time. */
> +	struct mm_struct *mm;
> +	/** @work: Work item queued when the dma-fence signals. */
> +	struct work_struct work;
> +	/** @cb: dma-fence callback. */
> +	struct dma_fence_cb cb;
> +	/** @wq: Workqueue to run @work on. */
> +	struct workqueue_struct *wq;
> +	/** @ops: Driver operations. */
> +	const struct drm_user_fence_ops *ops;
> +};

[Severity: High]
Does this API need a cancellation mechanism (e.g., drm_user_fence_cancel())?

Without a way to safely detach the ufence from a foreign dma-fence's
callback list, the ufence remains alive and bound to driver resources even
after the driver tears down its context, which can lead to a use-after-free.

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