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

["Zhu, Lingshan" <lingshan.zhu@amd.com>]

[-- Attachment #1: Type: text/plain, Size: 7580 bytes --]

On 8/29/2026 12:26 AM, Christian König wrote:

> On 8/28/26 17:59, Zhu, Lingshan wrote:
>> On 8/28/2026 9:09 PM, Christian König wrote:
>>
>>> On 8/28/26 11:53, Zhu Lingshan wrote:
>>>> The life cycle of a user queue is managed by its
>>>> kref. However when destroy a userq manager,
>>>> the kref_put of its queues in amdgpu_userq_mgr_fini
>>>> may not be the last put, therefore the queues
>>>> could be still alive after the userq manager
>>>> has been destroyed, resulting 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
>>> Clear NAK to that one as well, this is just nonsense.
>> It could be better to have some explanations.
>>
>> I am not sure how to guarantee the put_kref in amdgpu_userq_mgr_fini
>> is the last put and result in kref == 0, if not the last one,
>> there can be userq->userq_mgr UAF bugs.
> The rules are actually pretty simple:
>
> The reference is for keeping the userq alive while IOCTLs happen. And IOCTL can only happen while the file and therefor the fpriv, userq_mgr etc... are still alive.
>
> What can potentially be is that we also need to grab a reference from a work item, but in this case the fpriv/userq_mgr cleanup functions just need to cancel and wait for the work to finish.
>
> There should *never* be a reference grabbed from interrupt context, explicitely because releasing that reference is also not possible from interrupt context. Instead xa_lock_irqsave() needs to be used to make sure that the userq stays alive while the interrupt processing happens.

To protect a user queue from unexpected free and use-after-free, we either use xa_lock or the kref. 
Please note, currently the xa lock can work only because the user queue destruction routine also acquires the xa lock, and this is not always true.

However in some code paths, none of kref or xa_lock are employed, for example, in mes_userq_reset_queue, its just xa_for_each, no locking, no kref.
That is the problem this series intends to fix (UAF issues, racing with the destroyer).

And I believe we should choose kref rather than xa lock, the reasons:
1) in some code paths, like  mes_userq_reset_queue, amdgpu_userq_stop_sched_for_enforce_isolation and amdgpu_userq_start_sched_for_enforce_isolation,
they all acquire a mutex, may sleep, so can not use xa lock in such routines
2) xa lock is for the XArray, it serializes the access to the queues, means when lock the xarray, only one thread can process one queue, it is a performance degradation.
3) xa lock is a spin lock, so don't hold it for long time while processing a queue.
4) since the struct kref is embedded in user queue, it introduces a new destruction process of the queue, which is invoked when kref == 0,
so we should take kref in every routine that process the queue to avoid UAF.

So this patch is a prepare for the following patches, after fixing the user queue UAF issues, there would be more code paths get and put the kref to protect the queues,
therefore, the put in amdgpu_userq_mgr_fini may not be the last put, and amdgpu_userq_mgr_fini destroys the userq manager, resulting in userq->userq_mgr use-after-free,
that is why we need this improvement.

Thanks
Lingshan

>
> Regards,
> Christian.
>
>> Thanks
>> Lingshan
>>
>>> Christian.
>>>
>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
>>>> ---
>>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 30 +++++++++++++++++++++++
>>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h |  9 +++++++
>>>>  2 files changed, 39 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>>> index e0639f844a8e..f398986a61a5 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>>>> @@ -27,6 +27,7 @@
>>>>  #include <linux/pm_runtime.h>
>>>>  #include <linux/overflow.h>
>>>>  #include <drm/drm_drv.h>
>>>> +#include <linux/wait_bit.h>
>>>>  
>>>>  #include "amdgpu.h"
>>>>  #include "amdgpu_reset.h"
>>>> @@ -533,6 +534,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_var(&uq_mgr->refs);
>>>> +}
>>>> +
>>>>  static int
>>>>  amdgpu_userq_destroy(struct amdgpu_userq_mgr *uq_mgr, struct amdgpu_usermode_queue *queue)
>>>>  {
>>>> @@ -594,6 +606,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)
>>>> @@ -707,6 +721,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);
>>>>  
>>>> @@ -819,6 +834,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;
>>>> @@ -1331,6 +1347,7 @@ 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);
>>>>  	userq_mgr->adev = adev;
>>>>  	userq_mgr->file = file_priv;
>>>>  	userq_mgr->proc_ctx_allocated = false;
>>>> @@ -1380,6 +1397,19 @@ void amdgpu_userq_mgr_fini(struct amdgpu_userq_mgr *userq_mgr)
>>>>  		amdgpu_userq_put(queue);
>>>>  	}
>>>>  
>>>> +	/*
>>>> +	 * The above amdgpu_userq_put() may not be the last put
>>>> +	 * of the kref of a user queue, therefore there could
>>>> +	 * be some queues still alive even when the userq manager
>>>> +	 * has been destroyed. This wait_evet() blocks
>>>> +	 * amdgpu_userq_mgr_fini(), so keep userq_mgr alive
>>>> +	 * while any queues holding it.
>>>> +	 *
>>>> +	 * This prevents queue->userq_mgr use-after-free issues.
>>>> +	 */
>>>> +	wait_var_event(&userq_mgr->refs,
>>>> +		       !atomic_read_acquire(&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 8fc73862f64e..a13d8d4dd5c7 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
>>>> @@ -126,6 +126,15 @@ struct amdgpu_userq_mgr {
>>>>  	 */
>>>>  	struct xarray			userq_xa;
>>>>  	struct mutex			userq_mutex;
>>>> +
>>>> +	/**
>>>> +	 * @refs:
>>>> +	 *
>>>> +	 * Each queue increases this counter when join this manager,
>>>> +	 * and decreases it when leave this manager.
>>>> +	 */
>>>> +	atomic_t			refs;
>>>> +
>>>>  	struct amdgpu_device		*adev;
>>>>  	struct delayed_work		resume_work;
>>>>  	struct drm_file			*file;

[-- Attachment #2: Type: text/html, Size: 8469 bytes --]