Re: [PATCH v6 4/4] drm: Add per-signal compare functionality to drm_user_fence

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Per-signal comparison silently drops events and hangs userspace if the underlying dma_fence signals with an error (e.g., GPU hang).
- [High] `get_user()` is called in a workqueue thread without disabling page faults, allowing indefinite stalls via userfaultfd.
- [Medium] The comparison API exposes user-triggerable kernel WARNINGs and performs non-atomic 64-bit reads on 32-bit systems, leading to torn reads and lost events.
--

commit 9b6e7653adf98ad34fe45f552b39016755ccc0e5
Author: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
Subject: drm: Add per-signal compare functionality to drm_user_fence

This commit adds optional comparison fields to drm_user_fence to support
per-signal filtering, which is used in AMDGPU's EOP eventfd path. It
introduces drm_user_fence_set_compare() to configure these fields,
allowing the worker to be called conditionally based on a value read
from a userspace address.

> diff --git a/drivers/gpu/drm/drm_user_fence.c b/drivers/gpu/drm/drm_user_fence.c
> index 664178e2d74cf..e55d03ebf75f5 100644
> --- a/drivers/gpu/drm/drm_user_fence.c
> +++ b/drivers/gpu/drm/drm_user_fence.c

[ ... ]

> @@ -34,7 +49,32 @@ static void drm_user_fence_do_work(struct drm_work_fence *wfence)
>  		mm_ok = true;
>  	}
>  
> -	ufence->ops->worker(ufence, mm_ok);
> +	/*
> +	 * Per-signal comparison: read a value from userspace and compare
> +	 * with the expected value. Skip ops->worker if the condition is
> +	 * not met. Drivers that do not need filtering leave cmp_addr NULL.
> +	 *
> +	 * If the MM is gone and cmp_addr is set we cannot perform the
> +	 * comparison, so skip the worker rather than calling it without
> +	 * having verified the condition.
> +	 */
> +	if (ufence->cmp_op != DRM_USER_FENCE_CMP_NONE) {
> +		if (!mm_ok) {
> +			call_worker = false;
> +		} else {
> +			u64 cur_val;
> +
> +			if (get_user(cur_val, ufence->cmp_addr) ||
> +			    !drm_user_fence_cmp_match(cur_val,
> +						      ufence->cmp_value,
> +						      ufence->cmp_op))
> +				call_worker = false;

[Severity: High]
Is it possible for a page fault here to indefinitely stall the workqueue?

If a malicious user maps cmp_addr to a region backed by userfaultfd, they
could intentionally avoid resolving the fault. Because this executes in a
workqueue thread context without page faults disabled, this would block the
thread forever and stall all other tasks queued on the same workqueue.
Should pagefault_disable() be used around get_user()?

[Severity: High]
What happens if the underlying dma_fence signals with an error, such as
during a GPU hang?

If a GPU hang occurs, the hardware will fail to write the expected sequence
value to userspace. Instead, the GPU reset mechanism will signal all pending
fences with an error code (e.g., -ECANCELED). When this executes, get_user()
reads stale memory, causing drm_user_fence_cmp_match() to fail and
call_worker to be set to false.

Since the worker is completely skipped and the fence only signals once, the
event is permanently dropped, causing waiting userspace processes to hang.
Could the logic check the fence's error state (e.g., wfence->fence->error)
and bypass the comparison when an error is present?

[Severity: Medium]
Can this lead to torn reads on 32-bit architectures?

The get_user() macro on a 64-bit sequence number is not natively atomic on
32-bit systems and is implemented as two separate 32-bit loads. If the GPU
concurrently updates this value, the CPU may read a torn value. A torn read
can cause the comparison to falsely fail, resulting in a permanently lost
event.

> +		}
> +	}
> +
> +	if (call_worker)
> +		ufence->ops->worker(ufence, mm_ok);
>  
>  	if (mm_ok) {
>  		kthread_unuse_mm(ufence->mm);

[ ... ]

> +/**
> + * drm_user_fence_set_compare - Configure per-signal value comparison
> + * @ufence: user fence
> + * @addr: userspace VA to read when the fence signals
> + * @value: expected value to compare against
> + * @op: comparison operator (see &enum drm_user_fence_cmp)
> + *
> + * When set, drm_user_fence reads @addr via get_user() each time the
> + * fence signals and calls ops->worker() only if the comparison passes.
> + * This enables per-signal filtering without open-coding the read+compare
> + * pattern in each driver.
> + *
> + * Must be called after drm_user_fence_init() and before
> + * drm_user_fence_add_callback().
> + */
> +void drm_user_fence_set_compare(struct drm_user_fence *ufence,
> +				u64 __user *addr, u64 value,
> +				enum drm_user_fence_cmp op)
> +{
> +	WARN_ON_ONCE(!IS_ENABLED(CONFIG_64BIT));
> +
> +	if (WARN_ON(op != DRM_USER_FENCE_CMP_NONE && !addr))
> +		return;

[Severity: Medium]
Are these warnings triggerable by unprivileged userspace?

Because this function is used to configure user fences based on ioctl inputs,
this allows unprivileged userspace to trivially trigger kernel WARNINGs.
This creates a denial of service vector on systems with panic_on_warn.
Additionally, the 64-bit warning triggers unconditionally on 32-bit kernels.

> +
> +	ufence->cmp_addr = addr;
> +	ufence->cmp_value = value;
> +	ufence->cmp_op = op;
> +}
> +EXPORT_SYMBOL_GPL(drm_user_fence_set_compare);

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