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

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.

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;