Re: [PATCH] drm/amdgpu: keep the userq manager alive as long as its queues

On 8/24/2026 10:38 PM, Christian König wrote:

>
> On 8/18/26 13:57, Zhu Lingshan wrote:
>> The userq manager is embedded in fpriv, so any code
>> paths trigger kfree(fpriv) would destroy the userq_mgr,
>> even some of its queues are still alive,
>> resuting in userq->userq_mgr use-after-free issues.
>>
>> This commit fixes this problem by introduce a new
>> counter refs representing for the number of its queues,
>> and only free the userq_manager when refs == 0
> Well the userqueues are kept alive as long as there are IOCTLs using it.
>
> And and IOCTL in turn keeps a reference on the file and so the fpriv which contains the userq manager.
>
> So that here doesn't make any sense at all.
>
> If there is still an userq alive when the userq manager is destroyed we should probably call BUG(), cause that mean a major reference count issue in a core kernel component.

This can be more complex, what if the amdgpu_userq_put in amdgpu_userq_mgr_fini is not the last put of a queue, that means the queue is still alive even when
the userq manager has been destroyed. I will put this patch in my next series where the problem is better demonstrated and more reachable.

Thanks
Lingshan

>
> Regards,
> Christian.
>
>> Signed-off-by: Zhu Lingshan <[email protected]>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 27 +++++++++++++++++++++++
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h | 19 ++++++++++++++++
>>  2 files changed, 46 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>> index 24adad7be251..434384b53b85 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>> @@ -533,6 +533,17 @@ amdgpu_userq_get_doorbell_index(struct amdgpu_userq_mgr *uq_mgr,
>>  	return r;
>>  }
>>  
>> +static void amdgpu_userq_mgr_inc_refs(struct amdgpu_userq_mgr *uq_mgr)
>> +{
>> +	atomic_inc(&uq_mgr->refs);
>> +}
>> +
>> +static void amdgpu_userq_mgr_dec_refs(struct amdgpu_userq_mgr *uq_mgr)
>> +{
>> +	if (atomic_dec_and_test(&uq_mgr->refs))
>> +		wake_up(&uq_mgr->refs_wq);
>> +}
>> +
>>  static int
>>  amdgpu_userq_destroy(struct amdgpu_userq_mgr *uq_mgr, struct amdgpu_usermode_queue *queue)
>>  {
>> @@ -594,6 +605,8 @@ static void amdgpu_userq_kref_destroy(struct kref *kref)
>>  	r = amdgpu_userq_destroy(uq_mgr, queue);
>>  	if (r)
>>  		drm_file_err(uq_mgr->file, "Failed to destroy usermode queue %d\n", r);
>> +
>> +	amdgpu_userq_mgr_dec_refs(uq_mgr);
>>  }
>>  
>>  struct amdgpu_usermode_queue *amdgpu_userq_get(struct amdgpu_userq_mgr *uq_mgr, u32 qid)
>> @@ -677,6 +690,7 @@ amdgpu_userq_create(struct drm_file *filp, union drm_amdgpu_userq *args)
>>  	queue->xcp_id = (fpriv->xcp_id != AMDGPU_XCP_NO_PARTITION) ?
>>  				fpriv->xcp_id : 0;
>>  	queue->userq_mgr = uq_mgr;
>> +	amdgpu_userq_mgr_inc_refs(uq_mgr);
>>  	INIT_DELAYED_WORK(&queue->hang_detect_work,
>>  			  amdgpu_userq_hang_detect_work);
>>  
>> @@ -789,6 +803,7 @@ amdgpu_userq_create(struct drm_file *filp, union drm_amdgpu_userq *args)
>>  free_queue:
>>  	trace_amdgpu_userq_create_end(queue, r);
>>  	kfree(queue);
>> +	amdgpu_userq_mgr_dec_refs(uq_mgr);
>>  err_pm_runtime:
>>  	pm_runtime_put_autosuspend(adev_to_drm(adev)->dev);
>>  	return r;
>> @@ -1277,6 +1292,8 @@ int amdgpu_userq_mgr_init(struct amdgpu_userq_mgr *userq_mgr, struct drm_file *f
>>  {
>>  	mutex_init(&userq_mgr->userq_mutex);
>>  	xa_init_flags(&userq_mgr->userq_xa, XA_FLAGS_ALLOC);
>> +	atomic_set(&userq_mgr->refs, 0);
>> +	init_waitqueue_head(&userq_mgr->refs_wq);
>>  	userq_mgr->adev = adev;
>>  	userq_mgr->file = file_priv;
>>  	userq_mgr->proc_ctx_allocated = false;
>> @@ -1326,6 +1343,16 @@ void amdgpu_userq_mgr_fini(struct amdgpu_userq_mgr *userq_mgr)
>>  		amdgpu_userq_put(queue);
>>  	}
>>  
>> +	/*
>> +	 * userq_mgr is embedded in struct amdgpu_fpriv and get
>> +	 * freed when kfree(fpriv). This wait_evet() blocks
>> +	 * amdgpu_userq_mgr_fini() which is drm_release path,
>> +	 * so keep userq_mgr alive while any queues holding it.
>> +	 *
>> +	 * This prevents queue->userq_mgr use-after-free issues.
>> +	 */
>> +	wait_event(userq_mgr->refs_wq, !atomic_read(&userq_mgr->refs));
>> +
>>  	xa_destroy(&userq_mgr->userq_xa);
>>  
>>  	/*
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
>> index 6412a7f7b6ef..7141383ea2ae 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
>> @@ -24,6 +24,8 @@
>>  
>>  #ifndef AMDGPU_USERQ_H_
>>  #define AMDGPU_USERQ_H_
>> +#include <linux/wait.h>
>> +
>>  #include "amdgpu_eviction_fence.h"
>>  
>>  #define AMDGPU_MAX_USERQ_COUNT 512
>> @@ -126,6 +128,23 @@ struct amdgpu_userq_mgr {
>>  	 */
>>  	struct xarray			userq_xa;
>>  	struct mutex			userq_mutex;
>> +
>> +	/**
>> +	 * @refs:
>> +	 *
>> +	 * Each queue increase this counter when join this manager,
>> +	 * and decreash this counter when leave this manager.
>> +	 */
>> +	atomic_t			refs;
>> +
>> +	/**
>> +	 * @refs_wq:
>> +	 *
>> +	 * Waited on by amdgpu_userq_mgr_fini() until @refs drops to zero.
>> +	 * This wait prevents userq->userq_mgr UAF
>> +	 */
>> +	wait_queue_head_t		refs_wq;
>> +
>>  	struct amdgpu_device		*adev;
>>  	struct delayed_work		resume_work;
>>  	struct drm_file			*file;