[PATCH] drm/amdgpu: Fix eviction fence worker race during fd close

[PATCH] drm/amdgpu: Fix eviction fence worker race during fd close

[Jesse.Zhang]

The current cleanup order during file descriptor close can lead to
a race condition where the eviction fence worker attempts to access
a destroyed mutex from the user queue manager:

[  517.294055] DEBUG_LOCKS_WARN_ON(lock->magic != lock)
[  517.294060] WARNING: CPU: 8 PID: 2030 at kernel/locking/mutex.c:564
[  517.294094] Workqueue: events amdgpu_eviction_fence_suspend_worker [amdgpu]

The issue occurs because:
1. We destroy the user queue manager (including its mutex) first
2. Then try to destroy eviction fences which may have pending work
3. The eviction fence worker may try to access the already-destroyed mutex

Fix this by reordering the cleanup to:
1. First mark the fd as closing and destroy eviction fences,
   which flushes any pending work
2. Then safely destroy the user queue manager after we're certain
   no more fence work will be executed

Signed-off-by: Jesse Zhang <Jesse.Zhang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 4ddd08ce8885..4db92e0a60da 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -2913,8 +2913,8 @@ static int amdgpu_drm_release(struct inode *inode, struct file *filp)
 
 	if (fpriv) {
 		fpriv->evf_mgr.fd_closing = true;
-		amdgpu_userq_mgr_fini(&fpriv->userq_mgr);
 		amdgpu_eviction_fence_destroy(&fpriv->evf_mgr);
+		amdgpu_userq_mgr_fini(&fpriv->userq_mgr);
 	}
 
 	return drm_release(inode, filp);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 9fbb04aee97b..1fec3713fbf2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -1504,8 +1504,8 @@ void amdgpu_driver_postclose_kms(struct drm_device *dev,
 
 	if (!fpriv->evf_mgr.fd_closing) {
 		fpriv->evf_mgr.fd_closing = true;
-		amdgpu_userq_mgr_fini(&fpriv->userq_mgr);
 		amdgpu_eviction_fence_destroy(&fpriv->evf_mgr);
+		amdgpu_userq_mgr_fini(&fpriv->userq_mgr);
 	}
 	amdgpu_ctx_mgr_fini(&fpriv->ctx_mgr);
 	amdgpu_vm_fini(adev, &fpriv->vm);
-- 
2.49.0

Re: [PATCH] drm/amdgpu: Fix eviction fence worker race during fd close

[Christian König]

On 5/15/25 09:06, Jesse.Zhang wrote:
> The current cleanup order during file descriptor close can lead to
> a race condition where the eviction fence worker attempts to access
> a destroyed mutex from the user queue manager:
> 
> [  517.294055] DEBUG_LOCKS_WARN_ON(lock->magic != lock)
> [  517.294060] WARNING: CPU: 8 PID: 2030 at kernel/locking/mutex.c:564
> [  517.294094] Workqueue: events amdgpu_eviction_fence_suspend_worker [amdgpu]
> 
> The issue occurs because:
> 1. We destroy the user queue manager (including its mutex) first
> 2. Then try to destroy eviction fences which may have pending work
> 3. The eviction fence worker may try to access the already-destroyed mutex
> 
> Fix this by reordering the cleanup to:
> 1. First mark the fd as closing and destroy eviction fences,
>    which flushes any pending work
> 2. Then safely destroy the user queue manager after we're certain
>    no more fence work will be executed
> 
> Signed-off-by: Jesse Zhang <Jesse.Zhang@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 4ddd08ce8885..4db92e0a60da 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -2913,8 +2913,8 @@ static int amdgpu_drm_release(struct inode *inode, struct file *filp)
>  
>  	if (fpriv) {
>  		fpriv->evf_mgr.fd_closing = true;
> -		amdgpu_userq_mgr_fini(&fpriv->userq_mgr);
>  		amdgpu_eviction_fence_destroy(&fpriv->evf_mgr);
> +		amdgpu_userq_mgr_fini(&fpriv->userq_mgr);
>  	}
>  
>  	return drm_release(inode, filp);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 9fbb04aee97b..1fec3713fbf2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -1504,8 +1504,8 @@ void amdgpu_driver_postclose_kms(struct drm_device *dev,
>  

>  	if (!fpriv->evf_mgr.fd_closing) {
>  		fpriv->evf_mgr.fd_closing = true;
> -		amdgpu_userq_mgr_fini(&fpriv->userq_mgr);
>  		amdgpu_eviction_fence_destroy(&fpriv->evf_mgr);
> +		amdgpu_userq_mgr_fini(&fpriv->userq_mgr);
>  	}

That we have duplicated the code in both amdgpu_drm_release() and amdgpu_driver_postclose_kms() is a bug.

The copy in amdgpu_driver_postclose_kms() needs to be removed.

Apart from that looks good to me,
Christian.

>  	amdgpu_ctx_mgr_fini(&fpriv->ctx_mgr);
>  	amdgpu_vm_fini(adev, &fpriv->vm);

RE: [PATCH] drm/amdgpu: Fix eviction fence worker race during fd close

[Liang, Prike]

[Public]

I haven't cleaned up the userq resource destroy at postclose callback in my last patch, so here please remove the duplicated useq destroy. With that, the change in the patch is
Reviewed-by: Prike Liang <Prike.Liang@amd.com>

Regards,
      Prike

> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Jesse.Zhang
> Sent: Thursday, May 15, 2025 3:07 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Koenig, Christian
> <Christian.Koenig@amd.com>; Zhang, Jesse(Jie) <Jesse.Zhang@amd.com>
> Subject: [PATCH] drm/amdgpu: Fix eviction fence worker race during fd close
>
> The current cleanup order during file descriptor close can lead to a race condition
> where the eviction fence worker attempts to access a destroyed mutex from the
> user queue manager:
>
> [  517.294055] DEBUG_LOCKS_WARN_ON(lock->magic != lock) [  517.294060]
> WARNING: CPU: 8 PID: 2030 at kernel/locking/mutex.c:564 [  517.294094]
> Workqueue: events amdgpu_eviction_fence_suspend_worker [amdgpu]
>
> The issue occurs because:
> 1. We destroy the user queue manager (including its mutex) first 2. Then try to
> destroy eviction fences which may have pending work 3. The eviction fence worker
> may try to access the already-destroyed mutex
>
> Fix this by reordering the cleanup to:
> 1. First mark the fd as closing and destroy eviction fences,
>    which flushes any pending work
> 2. Then safely destroy the user queue manager after we're certain
>    no more fence work will be executed
>
> Signed-off-by: Jesse Zhang <Jesse.Zhang@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 4ddd08ce8885..4db92e0a60da 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -2913,8 +2913,8 @@ static int amdgpu_drm_release(struct inode *inode,
> struct file *filp)
>
>       if (fpriv) {
>               fpriv->evf_mgr.fd_closing = true;
> -             amdgpu_userq_mgr_fini(&fpriv->userq_mgr);
>               amdgpu_eviction_fence_destroy(&fpriv->evf_mgr);
> +             amdgpu_userq_mgr_fini(&fpriv->userq_mgr);
>       }
>
>       return drm_release(inode, filp);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 9fbb04aee97b..1fec3713fbf2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -1504,8 +1504,8 @@ void amdgpu_driver_postclose_kms(struct drm_device
> *dev,
>
>       if (!fpriv->evf_mgr.fd_closing) {
>               fpriv->evf_mgr.fd_closing = true;
> -             amdgpu_userq_mgr_fini(&fpriv->userq_mgr);
>               amdgpu_eviction_fence_destroy(&fpriv->evf_mgr);
> +             amdgpu_userq_mgr_fini(&fpriv->userq_mgr);
>       }
>       amdgpu_ctx_mgr_fini(&fpriv->ctx_mgr);
>       amdgpu_vm_fini(adev, &fpriv->vm);
> --
> 2.49.0

Re: [PATCH] drm/amdgpu: Fix eviction fence worker race during fd close

[Yadav, Arvind]

Reviewed-by: Arvind Yadav <Arvind.Yadav@amd.com>

On 5/15/2025 6:49 PM, Liang, Prike wrote:
> [Public]
>
> [Public]
>
> I haven't cleaned up the userq resource destroy at postclose callback in my last patch, so here please remove the duplicated useq destroy. With that, the change in the patch is
> Reviewed-by: Prike Liang <Prike.Liang@amd.com>
>
> Regards,
>        Prike
>
>> -----Original Message-----
>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
>> Jesse.Zhang
>> Sent: Thursday, May 15, 2025 3:07 PM
>> To: amd-gfx@lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Koenig, Christian
>> <Christian.Koenig@amd.com>; Zhang, Jesse(Jie) <Jesse.Zhang@amd.com>
>> Subject: [PATCH] drm/amdgpu: Fix eviction fence worker race during fd close
>>
>> The current cleanup order during file descriptor close can lead to a race condition
>> where the eviction fence worker attempts to access a destroyed mutex from the
>> user queue manager:
>>
>> [  517.294055] DEBUG_LOCKS_WARN_ON(lock->magic != lock) [  517.294060]
>> WARNING: CPU: 8 PID: 2030 at kernel/locking/mutex.c:564 [  517.294094]
>> Workqueue: events amdgpu_eviction_fence_suspend_worker [amdgpu]
>>
>> The issue occurs because:
>> 1. We destroy the user queue manager (including its mutex) first 2. Then try to
>> destroy eviction fences which may have pending work 3. The eviction fence worker
>> may try to access the already-destroyed mutex
>>
>> Fix this by reordering the cleanup to:
>> 1. First mark the fd as closing and destroy eviction fences,
>>     which flushes any pending work
>> 2. Then safely destroy the user queue manager after we're certain
>>     no more fence work will be executed
>>
>> Signed-off-by: Jesse Zhang <Jesse.Zhang@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +-
>> drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> index 4ddd08ce8885..4db92e0a60da 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> @@ -2913,8 +2913,8 @@ static int amdgpu_drm_release(struct inode *inode,
>> struct file *filp)
>>
>>        if (fpriv) {
>>                fpriv->evf_mgr.fd_closing = true;
>> -             amdgpu_userq_mgr_fini(&fpriv->userq_mgr);
>>                amdgpu_eviction_fence_destroy(&fpriv->evf_mgr);
>> +             amdgpu_userq_mgr_fini(&fpriv->userq_mgr);
>>        }
>>
>>        return drm_release(inode, filp);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> index 9fbb04aee97b..1fec3713fbf2 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> @@ -1504,8 +1504,8 @@ void amdgpu_driver_postclose_kms(struct drm_device
>> *dev,
>>
>>        if (!fpriv->evf_mgr.fd_closing) {
>>                fpriv->evf_mgr.fd_closing = true;
>> -             amdgpu_userq_mgr_fini(&fpriv->userq_mgr);
>>                amdgpu_eviction_fence_destroy(&fpriv->evf_mgr);
>> +             amdgpu_userq_mgr_fini(&fpriv->userq_mgr);
>>        }
>>        amdgpu_ctx_mgr_fini(&fpriv->ctx_mgr);
>>        amdgpu_vm_fini(adev, &fpriv->vm);
>> --
>> 2.49.0