Re: [PATCH v7 3/4] 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] Race condition in user fence signaling causes spurious -EBUSY failures for valid VMA unmap operations.

Pre-existing issues:
- [High] Userspace can stall the global xe->ordered_wq indefinitely via userfaultfd on user fence signaling, causing a Denial of Service for all compute applications.
--

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

This commit replaces the open-coded user fence implementation in xe_sync.c
with the new common drm_user_fence helper. The xe_user_fence structure now
embeds drm_user_fence and the previous local functions are replaced by
operations provided to the drm helper.

> diff --git a/drivers/gpu/drm/xe/xe_sync.c b/drivers/gpu/drm/xe/xe_sync.c
> index 37866768d64c2..05f6794af1ee2 100644
> --- a/drivers/gpu/drm/xe/xe_sync.c
> +++ b/drivers/gpu/drm/xe/xe_sync.c
[ ... ]
> @@ -19,36 +18,58 @@
>  #include "xe_macros.h"
>  #include "xe_sched_job_types.h"
>  
> +/*
> + * xe_user_fence wraps drm_user_fence with XE-specific fields.
> + * The drm_user_fence base handles MM borrowing and work-item lifetime.
> + */
>  struct xe_user_fence {
> -	struct xe_device *xe;
> -	struct kref refcount;
> -	struct dma_fence_cb cb;
> -	struct work_struct worker;
> -	struct mm_struct *mm;
> -	u64 __user *addr;
> -	u64 value;
> -	int signalled;
> +	struct drm_user_fence base;
> +	struct xe_device     *xe;
> +	u64 __user           *addr;
> +	u64                   value;
> +	int                   signalled;
>  };
>  
> -static void user_fence_destroy(struct kref *kref)
> +static void xe_ufence_worker(struct drm_user_fence *base, bool mm_ok)
>  {
> -	struct xe_user_fence *ufence = container_of(kref, struct xe_user_fence,
> -						 refcount);
> +	struct xe_user_fence *ufence = container_of(base, struct xe_user_fence, base);
>  
> -	mmdrop(ufence->mm);
> -	kfree(ufence);
> -}
> +	if (mm_ok) {
> +		if (copy_to_user(ufence->addr, &ufence->value, sizeof(ufence->value)))

[Severity: High]
This is a pre-existing issue, but can this copy_to_user() call cause a
denial of service?

If a malicious unprivileged userspace process creates a user fence using
DRM_XE_SYNC_TYPE_USER_FENCE and provides a memory address backed by
userfaultfd, the page fault will block indefinitely here because userspace
can refuse to resolve it.

> +			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");
> +	}
>  
> -static void user_fence_get(struct xe_user_fence *ufence)
> -{
> -	kref_get(&ufence->refcount);
> +	/*
> +	 * Ensure the fence value write is visible before signalled=1.
> +	 * A UMD polling signalled must see the committed fence value.
> +	 */
> +	smp_wmb();
> +
> +	/*
> +	 * Mark signalled after the user memory write so UMD can safely
> +	 * reuse the same ufence without hitting -EBUSY.
> +	 */
> +	WRITE_ONCE(ufence->signalled, 1);

[Severity: High]
Does this new ordering introduce a race condition causing spurious -EBUSY
failures for valid VMA unmap operations?

The original code updated the ufence->signalled state before calling
copy_to_user(). By inverting the order, if userspace is polling the memory
address, it will observe the memory update and immediately issue a VM unmap
ioctl. 

If this unmap ioctl reaches the driver before the worker thread executes
WRITE_ONCE(ufence->signalled, 1), the kernel state will still read as 0 and
incorrectly fail the unmap with -EBUSY.

> +
> +	wake_up_all(&ufence->xe->ufence_wq);
>  }
[ ... ]
> @@ -63,51 +84,22 @@ static struct xe_user_fence *user_fence_create(struct xe_device *xe, u64 addr,
>  	if (!ufence)
>  		return ERR_PTR(-ENOMEM);
>  
> -	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 does using the global device-wide
xe->ordered_wq amplify the impact of the userfaultfd block mentioned above?

Since critical tasks like vm->preempt.rebind_work are queued on this same
ordered workqueue, stalling it prevents preempt rebinds globally for all
VMs, completely hanging the GPU driver.

>  
>  	return ufence;
>  }

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