Re: [PATCH v9 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 3 potential issue(s) to consider:

New issues:
- [Low] Unused function xe_sync_ufence_cancel() introduced.

Pre-existing issues:
- [High] User fence signaling can block the device-wide ordered workqueue, causing a cross-process denial of service.
- [High] Userspace can trigger kernel WARNINGs and potential DoS by concurrently resetting the syncobj fence.
--

commit c30f53c58eadaed9972496eb0819631ccfe8878e
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. It moves the local user fence
logic into xe_ufence_ops.writeback and xe_ufence_ops.destroy callbacks.

> diff --git a/drivers/gpu/drm/xe/xe_sync.c b/drivers/gpu/drm/xe/xe_sync.c
> index 37866768d64c2..a335dd5f4478b 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(kref, struct xe_user_fence,
> -						 refcount);
> +	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]
This isn't a bug introduced by this patch, but can this copy_to_user() block
the device-wide ordered workqueue?

As noted in the comment above it, if userspace backs the fence address with
userfaultfd or FUSE, the copy_to_user() call can be intentionally stalled
indefinitely. Since this worker runs on xe->ordered_wq which has max_active=1,
a stalled copy could block all other work items on this queue, resulting in a
denial of service for the device. 

Should this use a dedicated non-blocking workqueue instead?

[ ... ]
> @@ -282,24 +281,15 @@ void xe_sync_entry_signal(struct xe_sync_entry *sync, struct dma_fence *fence)
>  	} else if (sync->syncobj) {
>  		drm_syncobj_replace_fence(sync->syncobj, fence);
>  	} else if (sync->ufence) {
> -		int err;
> -
>  		drm_syncobj_add_point(sync->ufence_syncobj,
>  				      sync->ufence_chain_fence,
>  				      fence, sync->ufence_timeline_value);
>  		sync->ufence_chain_fence = NULL;
>  
>  		fence = drm_syncobj_fence_get(sync->ufence_syncobj);
> -		user_fence_get(sync->ufence);
> -		err = dma_fence_add_callback(fence, &sync->ufence->cb,
> -					     user_fence_cb);
> -		if (err == -ENOENT) {
> -			kick_ufence(sync->ufence, fence);
> -		} else if (err) {
> +		if (drm_user_fence_add_callback(&sync->ufence->base, fence))

[Severity: High]
This isn't a bug introduced by this patch, but can this sequence trigger a
kernel WARNING?

In xe_sync_entry_signal(), drm_syncobj_fence_get() retrieves a fence from the
syncobj. If userspace concurrently resets the syncobj using
DRM_IOCTL_SYNCOBJ_RESET, the retrieved fence could be NULL. Passing this
NULL fence to drm_user_fence_add_callback() would trigger a WARN_ON inside
the underlying dma_fence_add_callback() function.

Is there a way to safely handle the NULL fence case here?

>  			XE_WARN_ON("failed to add user fence");
> -			user_fence_put(sync->ufence);
> -			dma_fence_put(fence);
> -		}
> +		/* fence ref consumed by drm_user_fence_add_callback */
>  	}
>  }
[ ... ]
> @@ -434,6 +424,21 @@ void xe_sync_ufence_put(struct xe_user_fence *ufence)
>  	user_fence_put(ufence);
>  }
>  
> +/**
> + * xe_sync_ufence_cancel() - Non-blocking cancel of user fence callback
> + * @ufence: user fence reference
> + *
> + * Attempts to cancel the pending callback without waiting for the worker.
> + * Safe to call while holding dma_resv_lock or vm->lock. If the callback
> + * has already fired, the worker runs independently — xe_ufence_worker
> + * only accesses device-level and userspace resources, both safe after
> + * VMA teardown.
> + */
> +void xe_sync_ufence_cancel(struct xe_user_fence *ufence)
> +{
> +	drm_user_fence_cancel(&ufence->base);
> +}
> +

[Severity: Low]
Is this function still needed?

The commit message mentions that callers for xe_sync_ufence_cancel() were
removed in v9, but the function definition itself remains here, resulting
in unused code.

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