Re: [PATCH v2 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] drm_user_fence_cancel() causes a Use-After-Free if the fence has signaled, because it requires the caller to pass a fence pointer without requiring the caller to hold a separate reference.
- [High] Contradictory documentation in drm_user_fence_cancel() instructs the caller to put references that the function already puts internally, leading to double-free bugs.
--

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

drm: Add common drm_user_fence helper

This commit introduces a common DRM user fence helper providing a pattern
to access userspace memory from a kthread context when a GPU fence signals.
It implements a kref-managed, dma-fence-callback-to-workqueue pattern that
borrows the process MM.

> diff --git a/drivers/gpu/drm/drm_user_fence.c b/drivers/gpu/drm/drm_user_fence.c
> new file mode 100644
> index 0000000000000..a3b14245163ae
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_user_fence.c

[ ... ]

> +/**
> + * drm_user_fence_cancel - Cancel a pending user fence callback
> + * @ufence: user fence
> + * @fence: the same dma-fence passed to drm_user_fence_add_callback()
> + *
> + * Attempts to remove the pending callback before driver context teardown.
> + * Must be called before the driver tears down its workqueue or ops.
> + *
> + * If the callback has already fired this is a no-op and returns false.
> + * If removal succeeds the caller must call drm_user_fence_put() to
> + * release the reference taken by drm_user_fence_add_callback(), and
> + * must also call dma_fence_put() to release the fence reference that
> + * was transferred at add_callback time.

[Severity: High]
The kernel-doc instructions here state that the caller must call
drm_user_fence_put() and dma_fence_put() upon success.

However, looking at the implementation of drm_user_fence_cancel() below,
it internally executes dma_fence_put() and drm_user_fence_put() before
returning true.

If a driver strictly follows this documentation, will it lead to double-free
bugs when they try to drop the references again?

> + *
> + * Return: true if callback was removed, false if it had already fired.
> + */
> +bool drm_user_fence_cancel(struct drm_user_fence *ufence,
> +			   struct dma_fence *fence)
> +{
> +	if (dma_fence_remove_callback(fence, &ufence->cb)) {

[Severity: High]
Can this result in a use-after-free if the fence has already signaled?

According to the drm_user_fence_add_callback() design, the caller transfers
its fence reference to the callback. If the fence signals asynchronously, the
callback fires and internally drops that reference via dma_fence_put(). If
that was the last reference, the fence object is freed.

When the driver later initiates teardown and calls drm_user_fence_cancel(),
it passes this potentially dangling fence pointer to
dma_fence_remove_callback(), which dereferences it to acquire a spinlock.

Is there a way for the caller to safely cancel the callback without taking
and holding an additional reference to the fence?

> +		dma_fence_put(fence);
> +		drm_user_fence_put(ufence);
> +		return true;
> +	}
> +	return false;
> +}
> +EXPORT_SYMBOL_GPL(drm_user_fence_cancel);

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