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;
