Re: [PATCH v3 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:
- [High] Canceling a queued drm_user_fence work item leaks a reference to the user fence, its mm_struct, and the stored dma_fence.
- [High] Unconditional cancellation of user fences in xe_sync_entry_cleanup() completely breaks VM_BIND user fences.
--

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

drm/xe: Convert xe_user_fence to drm_user_fence

This patch replaces the open-coded user fence implementation in the xe driver
with the new common drm_user_fence helper. It moves the local destruction and
worker logic into xe_ufence_ops callbacks which are executed by the core helper.

> diff --git a/drivers/gpu/drm/xe/xe_sync.c b/drivers/gpu/drm/xe/xe_sync.c
> --- a/drivers/gpu/drm/xe/xe_sync.c
> +++ b/drivers/gpu/drm/xe/xe_sync.c

[ ... ]

> @@ -310,8 +295,10 @@ void xe_sync_entry_cleanup(struct xe_sync_entry *sync)
>  	dma_fence_put(sync->fence);
>  	dma_fence_chain_free(sync->chain_fence);
>  	dma_fence_chain_free(sync->ufence_chain_fence);
> -	if (!IS_ERR_OR_NULL(sync->ufence))
> +	if (!IS_ERR_OR_NULL(sync->ufence)) {
> +		drm_user_fence_cancel_sync(&sync->ufence->base);

[Severity: High]
Does this unconditionally cancel the user fence worker during the ioctl's
teardown path?

During the normal, successful execution of xe_vm_bind_ioctl() in
drivers/gpu/drm/xe/xe_vm.c, the function falls through to the free_syncs
label and calls xe_sync_entry_cleanup() for all parsed sync entries:

    free_syncs:
        ...
        while (num_syncs--)
            xe_sync_entry_cleanup(&syncs[num_syncs]);

If the worker is canceled immediately after the ioctl submits the GPU work,
it appears the user fence is disarmed before the GPU finishes. Could this
cause userspace applications using VM_BIND with user fences to hang
indefinitely, as the completion signal is never delivered?

[Severity: High]
Additionally, when drm_user_fence_cancel_sync() is called here, can it leak
a reference to the user fence?

Looking at the implementation of drm_user_fence_cancel_sync() in
drivers/gpu/drm/drm_user_fence.c:

    void drm_user_fence_cancel_sync(struct drm_user_fence *ufence)
    {
        drm_user_fence_cancel(ufence);
        cancel_work_sync(&ufence->work);
    }

The return value of cancel_work_sync() is ignored. If it returns true
(meaning the work was queued but hadn't started yet), the 
drm_user_fence_work() worker function is bypassed. 

Since the work item owns a reference to the user fence (acquired during
drm_user_fence_add_callback()), skipping the worker means
drm_user_fence_put() is never called. Could this result in a memory leak of
the drm_user_fence structure, its mmgrab() reference, and its stored 
dma_fence?

>  		user_fence_put(sync->ufence);
> +	}
>  }

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