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. Regards, Christian. > > Thanks > Lingshan > >> Christian. >> >>> Signed-off-by: Zhu Lingshan <[email protected]> >>> --- >>> 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;
