Re: [PATCH v4 2/2] 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:
- [Critical] AB-BA deadlock between `dma_resv_lock` and page fault handling (`mmap_lock`) due to synchronous work cancellation in `xe_vma_destroy_late`
- [Low] Missing `#endif` include guard in `xe_sync_types.h` causes compilation error
--

commit b5c1e8acd90810b219cbad1a85281b1b796fc3ea
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 lifecycle functions are moved to the new ops
structure.

> diff --git a/drivers/gpu/drm/xe/xe_sync.c b/drivers/gpu/drm/xe/xe_sync.c
> index 37866768d64c2..f2eb66b8fe54d 100644
> --- a/drivers/gpu/drm/xe/xe_sync.c
> +++ b/drivers/gpu/drm/xe/xe_sync.c
[ ... ]
> @@ -19,36 +18,60 @@
[ ... ]
> +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);
[ ... ]
> +	if (mm_ok) {
> +		if (copy_to_user(ufence->addr, &ufence->value, sizeof(ufence->value)))

[Severity: Critical]
Could this copy_to_user() trigger a page fault that attempts to re-acquire
the dma_resv_lock? If a userspace application mmaps a BO, creates a user
fence pointing into that mapped BO address, and binds it via VM_BIND, this
worker will target the mapped BO.

When this occurs concurrently with VMA teardown, it appears to lead to a
deadlock. See below.

[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_sync_types.h b/drivers/gpu/drm/xe/xe_sync_types.h
> index b88f1833e28cc..2a75f998558e6 100644
> --- a/drivers/gpu/drm/xe/xe_sync_types.h
> +++ b/drivers/gpu/drm/xe/xe_sync_types.h
> @@ -28,4 +27,3 @@ struct xe_sync_entry {
>  	u32 flags;
>  };
>  
> -#endif

[Severity: Low]
Does this missing include guard cause a build failure? It looks like the
#endif for #ifndef _XE_SYNC_TYPES_H_ was accidentally deleted, which will
result in an unterminated #ifndef compiler error for any C file including
this header.

> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 19b3d0be79282..ef0784601f226 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -1214,6 +1214,7 @@ static void xe_vma_destroy_late(struct xe_vma *vma)
>  	struct xe_bo *bo = xe_vma_bo(vma);
>  
>  	if (vma->ufence) {
> +		xe_sync_ufence_cancel_sync(vma->ufence);

[Severity: Critical]
Does this synchronous cancellation create an AB-BA deadlock?

During VMA teardown, xe_vma_destroy_unlocked() acquires the BO's
dma_resv_lock (and vm->lock) and then synchronously waits for the user
fence worker to finish via cancel_work_sync() inside
xe_sync_ufence_cancel_sync().

If the worker is concurrently executing copy_to_user() (as noted above)
and triggers a page fault, the page fault handler (xe_gem_fault) will
attempt to acquire the same dma_resv_lock to resolve the fault. Since
the teardown thread already holds this lock while waiting for the
worker, the two threads will deadlock.

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