[PATCH] drm/amdgpu: fix slab-use-after-free in amdgpu_userq_mgr_fini+0x70c

[Christian König]

On 23.06.25 17:04, vitaly prosyak wrote:
> 
> On 2025-06-23 10:34, Christian König wrote:
>> On 19.06.25 04:26, Alex Deucher wrote:
>>>> ---
>>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 12 +++---------
>>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |  3 +++
>>>>  2 files changed, 6 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>> index 7fd233f160bf..204178d949e1 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>> @@ -2914,16 +2914,10 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
>>>>
>>>>  static int amdgpu_drm_release(struct inode *inode, struct file *filp)
>>>>  {
>>>> -       struct drm_file *file_priv = filp->private_data;
>>>> -       struct amdgpu_fpriv *fpriv = file_priv->driver_priv;
>>>> -
>>>> -       if (fpriv) {
>>>> -               fpriv->evf_mgr.fd_closing = true;
>>>> -               amdgpu_eviction_fence_destroy(&fpriv->evf_mgr);
>>>> -               amdgpu_userq_mgr_fini(&fpriv->userq_mgr);
>>>> -       }
>>>> +       int r;
>>>>
>>>> -       return drm_release(inode, filp);
>>>> +       r = drm_release(inode, filp);
>>>> +       return r;
>>>>  }
>>> You can just drop amdgpu_drm_release() altogether and just assign
>>> drm_release() as the callback directly.
>> Stop, that change here is just broken.
>>
>> We need to call amdgpu_userq_mgr_fini() before drm_release() since the later will release all BOs and so eventually also the ring buffer of the user queues.
> Yes, we observe that amdgpu_driver_postclose_kms is invoked before drm_release. Within amdgpu_driver_postclose_kms, we perform all the necessary cleanup steps, including:
> 
>     amdgpu_eviction_fence_destroy()
> 
>     amdgpu_userq_mgr_fini()
> 
>     amdgpu_ctx_mgr_fini()
> 
>     amdgpu_vm_fini()
> 
> These functions handle eviction fences, user queue management, context management, and VM cleanup respectively

That won't work. See drm_file_free():
...
        if (drm_core_check_feature(dev, DRIVER_GEM))
                drm_gem_release(dev, file);

        if (drm_is_primary_client(file))
                drm_master_release(file);

        if (dev->driver->postclose)
                dev->driver->postclose(dev, file);
...

The GEM handles are released first and then postclose is called.

So postclose is to late to stop the queues from processing.

Regards,
Christian.

> 
> Thanks, Vitaly
> 
>>
>> Regards,
>> Christian.
>>
>>> Alex
>>>
>>>>  long amdgpu_drm_ioctl(struct file *filp,
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>>> index d2ce7d86dbc8..195ed81d39ff 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>>> @@ -1501,6 +1501,9 @@ void amdgpu_driver_postclose_kms(struct drm_device *dev,
>>>>                 amdgpu_vm_bo_del(adev, fpriv->prt_va);
>>>>                 amdgpu_bo_unreserve(pd);
>>>>         }
>>>> +       fpriv->evf_mgr.fd_closing = true;
>>>> +       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.34.1
>>>>