Re: [PATCH v7 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 2 potential issue(s) to consider:
- [High] copy_from_user_nofault() disables page faults, causing legitimate completion events to be permanently dropped if the CPU PTE is not populated.
- [Medium] drm_user_fence_set_compare() returns void and silently ignores invalid configuration arguments.
--

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

drm: Add per-signal compare functionality to drm_user_fence
    
This patch adds optional compare fields to drm_user_fence to support per-signal
filtering, as used in the EOP eventfd path. It introduces
drm_user_fence_set_compare to configure the comparison and modifies the worker
to read the userspace value using copy_from_user_nofault before signaling.

> diff --git a/drivers/gpu/drm/drm_user_fence.c b/drivers/gpu/drm/drm_user_fence.c
> index 0f229b7210a9b..06a5683db10d9 100644
> --- a/drivers/gpu/drm/drm_user_fence.c
> +++ b/drivers/gpu/drm/drm_user_fence.c

[ ... ]

> @@ -30,13 +39,46 @@ static void drm_user_fence_do_work(struct drm_work_fence *wfence)

[ ... ]

> +	if (ufence->cmp_op != DRM_USER_FENCE_CMP_NONE &&
> +	    !(wfence->fence && wfence->fence->error)) {
> +		if (!mm) {
> +			call_worker = false;
> +		} else {
> +			__le64 raw;
> +
> +			/*
> +			 * Use copy_from_user_nofault() to prevent a
> +			 * userfaultfd-registered page from blocking this
> +			 * workqueue thread indefinitely (DoS).
> +			 */
> +			if (copy_from_user_nofault(&raw, ufence->cmp_addr,
> +						   sizeof(raw))) {
> +				call_worker = false;

[Severity: High]
Because copy_from_user_nofault disables page faults in drm_user_fence_do_work,
will this return -EFAULT and permanently drop legitimate completion events if
the CPU PTE is not populated?

This can happen if the page is not resident, such as when NUMA balancing
sets PROT_NONE, or if the memory uses lazy mapping and the CPU hasn't
touched it since the GPU wrote to it.

> +			} else {
> +				/* GPU writes LE; convert before comparing. */
> +				u64 cur_val = le64_to_cpu(raw);

[ ... ]

> @@ -63,8 +105,47 @@ void drm_user_fence_init(struct drm_user_fence *ufence,

[ ... ]

> +void drm_user_fence_set_compare(struct drm_user_fence *ufence,
> +				u64 __user *addr, u64 value,
> +				enum drm_user_fence_cmp op)
> +{
> +	/*
> +	 * get_user() of u64 is not atomic on 32-bit — caller should not
> +	 * reach here on non-64-bit kernels.
> +	 */
> +	if (WARN_ON_ONCE(!IS_ENABLED(CONFIG_64BIT)))
> +		return;

[Severity: Medium]
Because this function returns void, does this early return on unsupported
architectures silently fail without the caller knowing?

If a driver attempts to configure a comparison here, it appears it would
silently leave cmp_op disabled as DRM_USER_FENCE_CMP_NONE, treating the
fence as an unconditional signal.

Could this function return an error code instead so the driver can
propagate the failure to userspace?

> +
> +	if (op != DRM_USER_FENCE_CMP_NONE) {
> +		if (!addr || !IS_ALIGNED((unsigned long)addr, sizeof(u64)))
> +			return;

[Severity: Medium]
Similarly, if a driver passes an unaligned userspace address, does this
return silently bypass the intended logic and leave the fence as an
unconditional signal?

> +	}
> +
> +	ufence->cmp_addr  = addr;
> +	ufence->cmp_value = value;
> +	ufence->cmp_op    = op;
> +}

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