Re: [PATCH v8 3/3] drm/xe: Convert xe_user_fence to drm_user_fence

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Missing memory barrier allows visibility inversion between the user fence value and the signalled flag.

Pre-existing issues:
- [High] Denial of Service (DoS) due to indefinitely blocking `copy_to_user` stalling the device-wide ordered workqueue.
--

commit a3a566db4faca9c6c8375b3c0711ddb1c2742bca
Author: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
Subject: drm/xe: Convert xe_user_fence to drm_user_fence

This commit replaces the open-coded user fence implementation in the xe driver
with the new common drm_user_fence helper. It wraps the new helper in
xe_user_fence and moves the worker and callback logic to the provided
writeback and destroy ops.

> diff --git a/drivers/gpu/drm/xe/xe_sync.c b/drivers/gpu/drm/xe/xe_sync.c
> index 37866768d64c2..c0e720cf579ac 100644
> --- a/drivers/gpu/drm/xe/xe_sync.c
> +++ b/drivers/gpu/drm/xe/xe_sync.c
[ ... ]
> +static void xe_ufence_worker(struct drm_user_fence *base, bool mm_ok)
>  {
> +	struct xe_user_fence *ufence = container_of(base, struct xe_user_fence, base);
>  
> -	mmdrop(ufence->mm);
> -	kfree(ufence);
> -}
> +	/*
> +	 * Mark signalled before the userspace write so that if userspace
> +	 * observes the written value and immediately issues a VM unmap,
> +	 * check_ufence() will see signalled=1 and not return -EBUSY.
> +	 *
> +	 * NOTE: copy_to_user() may block if userspace backs the address with
> +	 * userfaultfd or FUSE. Drivers requiring non-blocking semantics should
> +	 * use a dedicated workqueue instead of xe->ordered_wq.
> +	 */
> +	WRITE_ONCE(ufence->signalled, 1);
>  
> -static void user_fence_get(struct xe_user_fence *ufence)
> -{
> -	kref_get(&ufence->refcount);
> +	if (mm_ok) {
> +		if (copy_to_user(ufence->addr, &ufence->value,
> +				 sizeof(ufence->value)))

[Severity: High]
Does this missing memory barrier allow visibility inversion?

In xe_ufence_worker(), WRITE_ONCE(ufence->signalled, 1) is called before
copy_to_user(). If a weakly ordered CPU (like ARM64) reorders these stores,
could userspace observe the new value in memory and issue a VM unmap ioctl
before the signalled flag is globally visible?

The previous implementation relied on the implicit full memory barrier of
mmget_not_zero() between these two operations, but this refactoring moved
that check before the worker executes. Should there be an smp_wmb() between
the WRITE_ONCE() and copy_to_user() calls to prevent spurious -EBUSY
failures in the VM unmap path?

> +			drm_dbg(&ufence->xe->drm,
> +				"copy_to_user failed, user fence wasn't signaled\n");
> +	} else {
> +		drm_dbg(&ufence->xe->drm,
> +			"mmget_not_zero() failed, ufence wasn't signaled\n");
> +	}
[ ... ]
> -	ufence->xe = xe;
> -	kref_init(&ufence->refcount);
> -	ufence->addr = ptr;
> +	ufence->xe    = xe;
> +	ufence->addr  = ptr;
>  	ufence->value = value;
> -	ufence->mm = current->mm;
> -	mmgrab(ufence->mm);
> +	drm_user_fence_init(&ufence->base, xe->ordered_wq, &xe_ufence_ops);

[Severity: High]
This is a pre-existing issue, but can this indefinitely blocking userspace
memory access stall the device-wide ordered workqueue?

If an unprivileged userspace application maps a memory region with
userfaultfd, submits a user fence operation pointing to this region, and
refuses to resolve the fault, copy_to_user() will block indefinitely.

Since the worker is initialized with xe->ordered_wq, this could permanently
stall the single-threaded workqueue used for critical device-wide operations
like GT resets, TDRs, and compute mode preempt rebinds, leading to a denial
of service.

>  
>  	return ufence;
>  }
>  
[ ... ]

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