All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 00/10] drm/amdgpu: secure userq lifecycle by its kref
@ 2026-08-28  9:53 Zhu Lingshan
  2026-08-28  9:53 ` [PATCH 01/10] drm/amdgpu: introduce amdgpu_lookup_queue_by_doorbell Zhu Lingshan
                   ` (9 more replies)
  0 siblings, 10 replies; 18+ messages in thread
From: Zhu Lingshan @ 2026-08-28  9:53 UTC (permalink / raw)
  To: Alexander.Deucher, Christian.Koenig, felix.kuehling
  Cc: Ray.Huang, amd-gfx, Zhu Lingshan

A struct kref is embedded in user queue, which manages the
lifecycle of a user queue. However, several code paths
access user queues without hoding the kref of a
user queue, especially from the doorbell XArray.

These accesses can race with the queue destruction
process and result in use-after-free bugs.

To fix this issue, this commit:
1) Introduces a new helper amdgpu_lookup_queue_by_doorbell,
which looks up a user queue with locking and hold
its kref during access.

2) Implement asynchronous userq destruction routine,
because the last put of a queue kref may be placed in
a code path where can not sleep or conflict locking
with the destruction process.

3) Hold kref during access the user queues

4) Keep the userq manager alive as long as its queues,
to avoid UAF issues.

This seires passed amd_basic tests in igt tests

Zhu Lingshan (10):
  drm/amdgpu: introduce amdgpu_lookup_queue_by_doorbell
  drm/amdgpu: keep the userq manager alive as long as its queues
  drm/amdgpu/gfx11: hold userq refs in private fault worker
  drm/amdgpu/gfx12: hold userq refs in private fault worker
  drm/amdgpu: implement asynchronous userq destruction routine
  drm/amdgpu: hold userq kref in MES reset
  drm/amdgpu: hold userq kref during isolation scheduling
  drm/amdgpu: hold userq kref during suspend and resume
  drm/amdgpu: free userq by kref_put when fails to create
  drm/amdgpu: take queue kref in userq_create to avoid UAF

 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c  | 214 +++++++++++++++++++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h  |  18 ++
 drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c     |   7 +-
 drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c     |   7 +-
 drivers/gpu/drm/amd/amdgpu/mes_userqueue.c |  46 ++---
 5 files changed, 246 insertions(+), 46 deletions(-)

-- 
2.53.0


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 01/10] drm/amdgpu: introduce amdgpu_lookup_queue_by_doorbell
  2026-08-28  9:53 [PATCH 00/10] drm/amdgpu: secure userq lifecycle by its kref Zhu Lingshan
@ 2026-08-28  9:53 ` Zhu Lingshan
  2026-08-28 13:08   ` Christian König
  2026-08-28  9:53 ` [PATCH 02/10] drm/amdgpu: keep the userq manager alive as long as its queues Zhu Lingshan
                   ` (8 subsequent siblings)
  9 siblings, 1 reply; 18+ messages in thread
From: Zhu Lingshan @ 2026-08-28  9:53 UTC (permalink / raw)
  To: Alexander.Deucher, Christian.Koenig, felix.kuehling
  Cc: Ray.Huang, amd-gfx, Zhu Lingshan

This commit introduces a new helper
amdgpu_lookup_queue_by_doorbell which helps
look up a user queue with the given doorbell id
in a xarray.

This function takes a kref of the user space queue.

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 |  2 ++
 2 files changed, 32 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
index 0a816b3c5ff9..e0639f844a8e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
@@ -609,6 +609,36 @@ struct amdgpu_usermode_queue *amdgpu_userq_get(struct amdgpu_userq_mgr *uq_mgr,
 	return queue;
 }
 
+/**
+ * amdgpu_lookup_queue_by_doorbell - look up a user queue by doorbell
+ * @xa: user queue XArray indexed by doorbell
+ * @doorbell: doorbell index
+ *
+ * Return: A queue with the doorbell indexed, or NULL if no such a queue found.
+ *
+ * This function increases kref of the queue, the caller
+ * must release the reference with amdgpu_userq_put().
+ */
+struct amdgpu_usermode_queue *
+amdgpu_lookup_queue_by_doorbell(struct xarray *xa, u32 doorbell)
+{
+	struct amdgpu_usermode_queue *queue;
+	unsigned long flags;
+
+	xa_lock_irqsave(xa, flags);
+	queue = xa_load(xa, doorbell);
+	if (!queue)
+		goto out_unlock;
+
+	if (!kref_get_unless_zero(&queue->refcount))
+		queue = NULL;
+
+out_unlock:
+	xa_unlock_irqrestore(xa, flags);
+
+	return queue;
+}
+
 void amdgpu_userq_put(struct amdgpu_usermode_queue *queue)
 {
 	if (queue)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
index 6412a7f7b6ef..8fc73862f64e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
@@ -151,6 +151,8 @@ struct amdgpu_db_info {
 };
 
 struct amdgpu_usermode_queue *amdgpu_userq_get(struct amdgpu_userq_mgr *uq_mgr, u32 qid);
+struct amdgpu_usermode_queue *
+amdgpu_lookup_queue_by_doorbell(struct xarray *xa, u32 doorbell);
 void amdgpu_userq_put(struct amdgpu_usermode_queue *queue);
 
 int amdgpu_userq_ioctl(struct drm_device *dev, void *data, struct drm_file *filp);
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 02/10] drm/amdgpu: keep the userq manager alive as long as its queues
  2026-08-28  9:53 [PATCH 00/10] drm/amdgpu: secure userq lifecycle by its kref Zhu Lingshan
  2026-08-28  9:53 ` [PATCH 01/10] drm/amdgpu: introduce amdgpu_lookup_queue_by_doorbell Zhu Lingshan
@ 2026-08-28  9:53 ` Zhu Lingshan
  2026-08-28 13:09   ` Christian König
  2026-08-28  9:53 ` [PATCH 03/10] drm/amdgpu/gfx11: hold userq refs in private fault worker Zhu Lingshan
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 18+ messages in thread
From: Zhu Lingshan @ 2026-08-28  9:53 UTC (permalink / raw)
  To: Alexander.Deucher, Christian.Koenig, felix.kuehling
  Cc: Ray.Huang, amd-gfx, Zhu Lingshan

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

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;
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 03/10] drm/amdgpu/gfx11: hold userq refs in private fault worker
  2026-08-28  9:53 [PATCH 00/10] drm/amdgpu: secure userq lifecycle by its kref Zhu Lingshan
  2026-08-28  9:53 ` [PATCH 01/10] drm/amdgpu: introduce amdgpu_lookup_queue_by_doorbell Zhu Lingshan
  2026-08-28  9:53 ` [PATCH 02/10] drm/amdgpu: keep the userq manager alive as long as its queues Zhu Lingshan
@ 2026-08-28  9:53 ` Zhu Lingshan
  2026-08-28 13:11   ` Christian König
  2026-08-28  9:53 ` [PATCH 04/10] drm/amdgpu/gfx12: " Zhu Lingshan
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 18+ messages in thread
From: Zhu Lingshan @ 2026-08-28  9:53 UTC (permalink / raw)
  To: Alexander.Deucher, Christian.Koenig, felix.kuehling
  Cc: Ray.Huang, amd-gfx, Zhu Lingshan

The GFX11 user queue private fault woker loads
the relevnt user queue from the userq doorbell xarray.
However it does not hold the spin_lock of the xarray
when walking the xarray, and does not increase the
kref of the user queue, so it races with queue
destruction path and may run into an use-after-free
userq problem.

This commit fixes this UAF problem by utilizing
amdgpu_lookup_queue_by_doorbell helper, which
properly hoding the xarray spin lock and the kref
of the user queue.

Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c
index a447562977ab..a063f86a8847 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c
@@ -6733,9 +6733,12 @@ static void gfx_v11_0_userq_priv_fault_work(struct work_struct *work)
 
 		doorbell = (db_ctrl & CP_RB_DOORBELL_CONTROL__DOORBELL_OFFSET_MASK) >>
 			   CP_RB_DOORBELL_CONTROL__DOORBELL_OFFSET__SHIFT;
-		q = xa_load(&adev->userq_doorbell_xa, doorbell);
-		if (q)
+		q = amdgpu_lookup_queue_by_doorbell(&adev->userq_doorbell_xa,
+						    doorbell);
+		if (q) {
 			amdgpu_userq_start_hang_detect_work(q);
+			amdgpu_userq_put(q);
+		}
 	}
 }
 
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 04/10] drm/amdgpu/gfx12: hold userq refs in private fault worker
  2026-08-28  9:53 [PATCH 00/10] drm/amdgpu: secure userq lifecycle by its kref Zhu Lingshan
                   ` (2 preceding siblings ...)
  2026-08-28  9:53 ` [PATCH 03/10] drm/amdgpu/gfx11: hold userq refs in private fault worker Zhu Lingshan
@ 2026-08-28  9:53 ` Zhu Lingshan
  2026-08-28  9:53 ` [PATCH 05/10] drm/amdgpu: implement asynchronous userq destruction routine Zhu Lingshan
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: Zhu Lingshan @ 2026-08-28  9:53 UTC (permalink / raw)
  To: Alexander.Deucher, Christian.Koenig, felix.kuehling
  Cc: Ray.Huang, amd-gfx, Zhu Lingshan

The GFX12 user queue private fault woker loads
the relevnt user queue from the userq doorbell xarray.
However it does not hold the spin_lock of the xarray
when walking the xarray, and does not increase the
kref of the user queue, so it races with queue
destruction path and may run into an use-after-free
userq problem.

This commit fixes this UAF problem by utilizing
amdgpu_lookup_queue_by_doorbell helper, which
properly hoding the xarray spin lock and the kref
of the user queue.

Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c
index fdfee88e41e3..b6ae15066205 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c
@@ -5070,9 +5070,12 @@ static void gfx_v12_0_userq_priv_fault_work(struct work_struct *work)
 
 		doorbell = (db_ctrl & CP_RB_DOORBELL_CONTROL__DOORBELL_OFFSET_MASK) >>
 			   CP_RB_DOORBELL_CONTROL__DOORBELL_OFFSET__SHIFT;
-		q = xa_load(&adev->userq_doorbell_xa, doorbell);
-		if (q)
+		q = amdgpu_lookup_queue_by_doorbell(&adev->userq_doorbell_xa,
+						    doorbell);
+		if (q) {
 			amdgpu_userq_start_hang_detect_work(q);
+			amdgpu_userq_put(q);
+		}
 	}
 }
 
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 05/10] drm/amdgpu: implement asynchronous userq destruction routine
  2026-08-28  9:53 [PATCH 00/10] drm/amdgpu: secure userq lifecycle by its kref Zhu Lingshan
                   ` (3 preceding siblings ...)
  2026-08-28  9:53 ` [PATCH 04/10] drm/amdgpu/gfx12: " Zhu Lingshan
@ 2026-08-28  9:53 ` Zhu Lingshan
  2026-08-28  9:53 ` [PATCH 06/10] drm/amdgpu: hold userq kref in MES reset Zhu Lingshan
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: Zhu Lingshan @ 2026-08-28  9:53 UTC (permalink / raw)
  To: Alexander.Deucher, Christian.Koenig, felix.kuehling
  Cc: Ray.Huang, amd-gfx, Zhu Lingshan

The last kref of a user queue may be released
from a context where can not sleep or
require an alrady hold lock(reset_domain->sem, reset_mutext, etc)
in the same context.

This commit helps resovle these issues by refactoring
the user queue destruction function to a deferred
work item, so it does not conflict with the caller context.

Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 15 ++++++++++++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h |  7 +++++++
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
index f398986a61a5..9fe20cb9af58 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
@@ -596,11 +596,11 @@ amdgpu_userq_destroy(struct amdgpu_userq_mgr *uq_mgr, struct amdgpu_usermode_que
 	return r;
 }
 
-static void amdgpu_userq_kref_destroy(struct kref *kref)
+static void amdgpu_userq_destroy_worker(struct work_struct *work)
 {
 	int r;
 	struct amdgpu_usermode_queue *queue =
-		container_of(kref, struct amdgpu_usermode_queue, refcount);
+		container_of(work, struct amdgpu_usermode_queue, destroy_work);
 	struct amdgpu_userq_mgr *uq_mgr = queue->userq_mgr;
 
 	r = amdgpu_userq_destroy(uq_mgr, queue);
@@ -610,6 +610,14 @@ static void amdgpu_userq_kref_destroy(struct kref *kref)
 	amdgpu_userq_mgr_dec_refs(uq_mgr);
 }
 
+static void amdgpu_userq_kref_release(struct kref *kref)
+{
+	struct amdgpu_usermode_queue *queue =
+		container_of(kref, struct amdgpu_usermode_queue, refcount);
+
+	WARN_ON_ONCE(!queue_work(system_unbound_wq, &queue->destroy_work));
+}
+
 struct amdgpu_usermode_queue *amdgpu_userq_get(struct amdgpu_userq_mgr *uq_mgr, u32 qid)
 {
 	struct amdgpu_usermode_queue *queue;
@@ -656,7 +664,7 @@ amdgpu_lookup_queue_by_doorbell(struct xarray *xa, u32 doorbell)
 void amdgpu_userq_put(struct amdgpu_usermode_queue *queue)
 {
 	if (queue)
-		kref_put(&queue->refcount, amdgpu_userq_kref_destroy);
+		kref_put(&queue->refcount, amdgpu_userq_kref_release);
 }
 
 static int amdgpu_userq_priority_permit(struct drm_file *filp,
@@ -724,6 +732,7 @@ amdgpu_userq_create(struct drm_file *filp, union drm_amdgpu_userq *args)
 	amdgpu_userq_mgr_inc_refs(uq_mgr);
 	INIT_DELAYED_WORK(&queue->hang_detect_work,
 			  amdgpu_userq_hang_detect_work);
+	INIT_WORK(&queue->destroy_work, amdgpu_userq_destroy_worker);
 
 	r = amdgpu_userq_fence_driver_alloc(adev, &queue->fence_drv);
 	if (r)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
index a13d8d4dd5c7..ecc450be7eb2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
@@ -87,6 +87,13 @@ struct amdgpu_usermode_queue {
 	 * Delayed work which runs when userq_fences time out.
 	 */
 	struct delayed_work	hang_detect_work;
+
+	/**
+	 * @destroy_work:
+	 *
+	 * Work struct used to destroy the queue when kref == 0
+	 */
+	struct work_struct	destroy_work;
 	struct kref		refcount;
 
 	union {
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 06/10] drm/amdgpu: hold userq kref in MES reset
  2026-08-28  9:53 [PATCH 00/10] drm/amdgpu: secure userq lifecycle by its kref Zhu Lingshan
                   ` (4 preceding siblings ...)
  2026-08-28  9:53 ` [PATCH 05/10] drm/amdgpu: implement asynchronous userq destruction routine Zhu Lingshan
@ 2026-08-28  9:53 ` Zhu Lingshan
  2026-08-28  9:53 ` [PATCH 07/10] drm/amdgpu: hold userq kref during isolation scheduling Zhu Lingshan
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: Zhu Lingshan @ 2026-08-28  9:53 UTC (permalink / raw)
  To: Alexander.Deucher, Christian.Koenig, felix.kuehling
  Cc: Ray.Huang, amd-gfx, Zhu Lingshan

Current mes_userq_reset_queue() walks the
user queue doorbell xarray without holding its
spin lock and does not take a queue->kref when
if finds a queue. So the queue could be
unexpectedly deconstructed and casuing an
use-after-free problem.

This commit fixes this problem by using
the helper amdgpu_lookup_queue_by_doorbell()
to properly acquire the spin lock of the xarray,
and hold its kref during processing the queue.

Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/mes_userqueue.c | 46 +++++++++++-----------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
index 7f334f718cd8..3de71615cde0 100644
--- a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
+++ b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
@@ -244,30 +244,30 @@ int mes_userq_reset_queue(struct amdgpu_device *adev,
 {
 	struct amdgpu_usermode_queue *uq;
 	bool use_mmio = adev->gfx.mec.use_mmio_for_reset;
-	unsigned long uq_id;
-	int r;
+	int r = 0;
 
-	xa_for_each(&adev->userq_doorbell_xa, uq_id, uq) {
-		if (uq->queue_type == queue_type) {
-			if (uq == guilty_uq)
-				continue;
-			if (uq->doorbell_index == db) {
-				uq->state = AMDGPU_USERQ_STATE_HUNG;
-				if (use_mmio)
-					r = amdgpu_mes_reset_queue_mmio(adev, queue_type, 0, 1, pipe, queue, 0);
-				else
-					r = amdgpu_mes_reset_user_queue(adev, queue_type, db, 0);
-				if (r)
-					return r;
-				r = mes_userq_unmap(uq);
-				if (r)
-					return r;
-				amdgpu_userq_fence_driver_force_completion(uq);
-				break;
-			}
-		}
-	}
-	return 0;
+	uq = amdgpu_lookup_queue_by_doorbell(&adev->userq_doorbell_xa, db);
+	if (!uq)
+		return 0;
+
+	if (uq == guilty_uq || uq->queue_type != queue_type)
+		goto put_queue;
+
+	uq->state = AMDGPU_USERQ_STATE_HUNG;
+	if (use_mmio)
+		r = amdgpu_mes_reset_queue_mmio(adev, queue_type, 0, 1, pipe, queue, 0);
+	else
+		r = amdgpu_mes_reset_user_queue(adev, queue_type, db, 0);
+	if (r)
+		goto put_queue;
+
+	r = mes_userq_unmap(uq);
+	if (!r)
+		amdgpu_userq_fence_driver_force_completion(uq);
+
+put_queue:
+	amdgpu_userq_put(uq);
+	return r;
 }
 
 static int mes_userq_create_ctx_space(struct amdgpu_userq_mgr *uq_mgr,
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 07/10] drm/amdgpu: hold userq kref during isolation scheduling
  2026-08-28  9:53 [PATCH 00/10] drm/amdgpu: secure userq lifecycle by its kref Zhu Lingshan
                   ` (5 preceding siblings ...)
  2026-08-28  9:53 ` [PATCH 06/10] drm/amdgpu: hold userq kref in MES reset Zhu Lingshan
@ 2026-08-28  9:53 ` Zhu Lingshan
  2026-08-28  9:53 ` [PATCH 08/10] drm/amdgpu: hold userq kref during suspend and resume Zhu Lingshan
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: Zhu Lingshan @ 2026-08-28  9:53 UTC (permalink / raw)
  To: Alexander.Deucher, Christian.Koenig, felix.kuehling
  Cc: Ray.Huang, amd-gfx, Zhu Lingshan

The enforce-isolation stop and start handlers
iterate the doorbell XArray, but process a user queue
without holding its kref. Therefore, a concurrent queue
destruction process can free a queue before these paths
finish their work, causing use-after-free problems.

This commit fixes this issue by introducing a new helper
amdgpu_userq_xa_find() which finds a queue from a XArray
and hold its kref, and employ this helper in the
enforce-isolation stop and start handlers.

Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 72 +++++++++++++++++++++--
 1 file changed, 68 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
index 9fe20cb9af58..1427ff175dab 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
@@ -661,6 +661,54 @@ amdgpu_lookup_queue_by_doorbell(struct xarray *xa, u32 doorbell)
 	return queue;
 }
 
+/**
+ * amdgpu_userq_xa_find - search the XArray for a queue
+ * @xa: user queue XArray
+ * @index: first index to search, updated by every iteration of the search
+ *
+ * Return: a queue which has the lowest index that is at least @index,
+ * or NULL when no queue was found.
+ *
+ * The caller must release the kref of a queue with amdgpu_userq_put() after use.
+ */
+static struct amdgpu_usermode_queue *
+amdgpu_userq_xa_find(struct xarray *xa, unsigned long *index)
+{
+	struct amdgpu_usermode_queue *queue;
+	unsigned long flags;
+
+	xa_lock_irqsave(xa, flags);
+	queue = xa_find(xa, index, ULONG_MAX, XA_PRESENT);
+	while (queue) {
+		/*
+		 * If found a queue but failed to get a kref,
+		 * it means the queue is in destruction process,
+		 * so skip it by continuing the loop.
+		 *
+		 * If get a kref of the queue, break the loop and return it.
+		 */
+		if (kref_get_unless_zero(&queue->refcount))
+			break;
+
+		/*
+		 * If the index is ULONG_MAX, we have reached the end of the XArray.
+		 * Break the loop and return NULL because ULONG_MAX + 1 = 0,
+		 * which is the start of the XArray.
+		 */
+		if (*index == ULONG_MAX) {
+			queue = NULL;
+			break;
+		}
+
+		(*index)++;
+		queue = xa_find(xa, index, ULONG_MAX, XA_PRESENT);
+	}
+
+	xa_unlock_irqrestore(xa, flags);
+
+	return queue;
+}
+
 void amdgpu_userq_put(struct amdgpu_usermode_queue *queue)
 {
 	if (queue)
@@ -1496,7 +1544,7 @@ int amdgpu_userq_stop_sched_for_enforce_isolation(struct amdgpu_device *adev,
 	u32 ip_mask = amdgpu_userq_get_supported_ip_mask(adev);
 	struct amdgpu_usermode_queue *queue;
 	struct amdgpu_userq_mgr *uqm;
-	unsigned long queue_id;
+	unsigned long queue_id = 0;
 	int ret = 0, r;
 
 	/* only need to stop gfx/compute */
@@ -1506,7 +1554,8 @@ int amdgpu_userq_stop_sched_for_enforce_isolation(struct amdgpu_device *adev,
 	if (adev->userq_halt_for_enforce_isolation)
 		dev_warn(adev->dev, "userq scheduling already stopped!\n");
 	adev->userq_halt_for_enforce_isolation = true;
-	xa_for_each(&adev->userq_doorbell_xa, queue_id, queue) {
+	queue = amdgpu_userq_xa_find(&adev->userq_doorbell_xa, &queue_id);
+	while (queue) {
 		uqm = queue->userq_mgr;
 		cancel_delayed_work_sync(&uqm->resume_work);
 		mutex_lock(&uqm->userq_mutex);
@@ -1518,6 +1567,13 @@ int amdgpu_userq_stop_sched_for_enforce_isolation(struct amdgpu_device *adev,
 				ret = r;
 		}
 		mutex_unlock(&uqm->userq_mutex);
+		amdgpu_userq_put(queue);
+
+		if (queue_id == ULONG_MAX)
+			break;
+		queue_id++;
+		queue = amdgpu_userq_xa_find(&adev->userq_doorbell_xa,
+					     &queue_id);
 	}
 
 	return ret;
@@ -1529,7 +1585,7 @@ int amdgpu_userq_start_sched_for_enforce_isolation(struct amdgpu_device *adev,
 	u32 ip_mask = amdgpu_userq_get_supported_ip_mask(adev);
 	struct amdgpu_usermode_queue *queue;
 	struct amdgpu_userq_mgr *uqm;
-	unsigned long queue_id;
+	unsigned long queue_id = 0;
 	int ret = 0, r;
 
 	/* only need to stop gfx/compute */
@@ -1541,7 +1597,8 @@ int amdgpu_userq_start_sched_for_enforce_isolation(struct amdgpu_device *adev,
 
 	adev->userq_halt_for_enforce_isolation = false;
 
-	xa_for_each(&adev->userq_doorbell_xa, queue_id, queue) {
+	queue = amdgpu_userq_xa_find(&adev->userq_doorbell_xa, &queue_id);
+	while (queue) {
 		uqm = queue->userq_mgr;
 		mutex_lock(&uqm->userq_mutex);
 		if (((queue->queue_type == AMDGPU_HW_IP_GFX) ||
@@ -1552,6 +1609,13 @@ int amdgpu_userq_start_sched_for_enforce_isolation(struct amdgpu_device *adev,
 				ret = r;
 		}
 		mutex_unlock(&uqm->userq_mutex);
+		amdgpu_userq_put(queue);
+
+		if (queue_id == ULONG_MAX)
+			break;
+		queue_id++;
+		queue = amdgpu_userq_xa_find(&adev->userq_doorbell_xa,
+					     &queue_id);
 	}
 
 	return ret;
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 08/10] drm/amdgpu: hold userq kref during suspend and resume
  2026-08-28  9:53 [PATCH 00/10] drm/amdgpu: secure userq lifecycle by its kref Zhu Lingshan
                   ` (6 preceding siblings ...)
  2026-08-28  9:53 ` [PATCH 07/10] drm/amdgpu: hold userq kref during isolation scheduling Zhu Lingshan
@ 2026-08-28  9:53 ` Zhu Lingshan
  2026-08-28  9:53 ` [PATCH 09/10] drm/amdgpu: free userq by kref_put when fails to create Zhu Lingshan
  2026-08-28  9:53 ` [PATCH 10/10] drm/amdgpu: take queue kref in userq_create to avoid UAF Zhu Lingshan
  9 siblings, 0 replies; 18+ messages in thread
From: Zhu Lingshan @ 2026-08-28  9:53 UTC (permalink / raw)
  To: Alexander.Deucher, Christian.Koenig, felix.kuehling
  Cc: Ray.Huang, amd-gfx, Zhu Lingshan

The userq suspend and resume handlers walks the doorbell XArray
but process a user queuewithout holding its kref.

Therefore, a concurrent queue destruction process can free
a queue before these handlers finish their work, causing
an use-after-free problem.

This commit fixes this problem by using
amdgpu_userq_xa_find helper which properly holds the
kref of a queue in a loop of searching for queues.

Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 35 +++++++++++++++++++----
 1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
index 1427ff175dab..0dab395ef0f4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
@@ -1493,23 +1493,35 @@ int amdgpu_userq_suspend(struct amdgpu_device *adev)
 	u32 ip_mask = amdgpu_userq_get_supported_ip_mask(adev);
 	struct amdgpu_usermode_queue *queue;
 	struct amdgpu_userq_mgr *uqm;
-	unsigned long queue_id;
+	unsigned long queue_id = 0;
 	int r;
 
 	if (!ip_mask)
 		return 0;
 
-	xa_for_each(&adev->userq_doorbell_xa, queue_id, queue) {
+	queue = amdgpu_userq_xa_find(&adev->userq_doorbell_xa, &queue_id);
+	while (queue) {
 		uqm = queue->userq_mgr;
 		cancel_delayed_work_sync(&uqm->resume_work);
-		guard(mutex)(&uqm->userq_mutex);
+		mutex_lock(&uqm->userq_mutex);
 		if (adev->in_s0ix)
 			r = amdgpu_userq_preempt_helper(queue);
 		else
 			r = amdgpu_userq_unmap_helper(queue);
+		mutex_unlock(&uqm->userq_mutex);
+		amdgpu_userq_put(queue);
+
 		if (r)
 			return r;
+
+		if (queue_id == ULONG_MAX)
+			break;
+
+		queue_id++;
+		queue = amdgpu_userq_xa_find(&adev->userq_doorbell_xa,
+					     &queue_id);
 	}
+
 	return 0;
 }
 
@@ -1518,21 +1530,32 @@ int amdgpu_userq_resume(struct amdgpu_device *adev)
 	u32 ip_mask = amdgpu_userq_get_supported_ip_mask(adev);
 	struct amdgpu_usermode_queue *queue;
 	struct amdgpu_userq_mgr *uqm;
-	unsigned long queue_id;
+	unsigned long queue_id = 0;
 	int r;
 
 	if (!ip_mask)
 		return 0;
 
-	xa_for_each(&adev->userq_doorbell_xa, queue_id, queue) {
+	queue = amdgpu_userq_xa_find(&adev->userq_doorbell_xa, &queue_id);
+	while (queue) {
 		uqm = queue->userq_mgr;
-		guard(mutex)(&uqm->userq_mutex);
+		mutex_lock(&uqm->userq_mutex);
 		if (adev->in_s0ix)
 			r = amdgpu_userq_restore_helper(queue);
 		else
 			r = amdgpu_userq_map_helper(queue);
+		mutex_unlock(&uqm->userq_mutex);
+		amdgpu_userq_put(queue);
+
 		if (r)
 			return r;
+
+		if (queue_id == ULONG_MAX)
+			break;
+
+		queue_id++;
+		queue = amdgpu_userq_xa_find(&adev->userq_doorbell_xa,
+					     &queue_id);
 	}
 
 	return 0;
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 09/10] drm/amdgpu: free userq by kref_put when fails to create
  2026-08-28  9:53 [PATCH 00/10] drm/amdgpu: secure userq lifecycle by its kref Zhu Lingshan
                   ` (7 preceding siblings ...)
  2026-08-28  9:53 ` [PATCH 08/10] drm/amdgpu: hold userq kref during suspend and resume Zhu Lingshan
@ 2026-08-28  9:53 ` Zhu Lingshan
  2026-08-28  9:53 ` [PATCH 10/10] drm/amdgpu: take queue kref in userq_create to avoid UAF Zhu Lingshan
  9 siblings, 0 replies; 18+ messages in thread
From: Zhu Lingshan @ 2026-08-28  9:53 UTC (permalink / raw)
  To: Alexander.Deucher, Christian.Koenig, felix.kuehling
  Cc: Ray.Huang, amd-gfx, Zhu Lingshan

In amdgpu_userq_create(), once the user queue is
published to userq_doorbell_xa, other threads could
access the queue from the XArray. But the queue
may fails to map the queue and kfree(queue),
this causes the accessor use-after-free issue.

This commit fixes this issue by properly
get and put the kref of a queue to maintain
the lifecycle of a user queue.

There are some minor improvements in this commit:

1) Early detach the doorbell of a queue when fails to map
a queue, because the map helper set the queue state to HUNG,
and the asynchronous post reset helper amdgpu_userq_post_reset
can find the queue from the XArray and remaps the queue again,
which causes a queue leaking because the user space
already receive an error code for the queue
from amdgpu_userq_create()

2) Use xa_cmpxchg_irq in amdgpu_userq_detach_doorbell().
Once fails to map a queue, the queue destruction process
is asynchronous (a delayed worker), and a new created queue
could re-use the doorbell, xa_cmpxchg_irq compares the doorbell
which to be deleted with the doorbell which @index points to,
so it does not detach the doorbell of the new created queue.

Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
index 0dab395ef0f4..21a9a2138fc8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
@@ -424,7 +424,9 @@ static void amdgpu_userq_detach_doorbell(struct amdgpu_usermode_queue *queue)
 	struct amdgpu_device *adev = queue->userq_mgr->adev;
 
 	down_read(&adev->reset_domain->sem);
-	xa_erase_irq(&adev->userq_doorbell_xa, queue->doorbell_index);
+	/* It doesn't alloc any memory here, so pass 0 to gfp */
+	xa_cmpxchg_irq(&adev->userq_doorbell_xa, queue->doorbell_index,
+		       queue, NULL, 0);
 	up_read(&adev->reset_domain->sem);
 }
 
@@ -836,6 +838,14 @@ amdgpu_userq_create(struct drm_file *filp, union drm_amdgpu_userq *args)
 	if (r)
 		goto clean_mqd;
 
+	/*
+	 * Once the queue has been published to doorbell_xa,
+	 * it could be accessed by other threads, so it has to be
+	 * destroyed through kref put. The destroy work decreases
+	 * userq_count, so we have to increase it here.
+	 */
+	atomic_inc(&uq_mgr->userq_count[queue->queue_type]);
+
 	amdgpu_userq_ensure_ev_fence(&fpriv->userq_mgr, &fpriv->evf_mgr);
 
 	/* don't map the queue if scheduling is halted */
@@ -851,12 +861,14 @@ amdgpu_userq_create(struct drm_file *filp, union drm_amdgpu_userq *args)
 		if (r) {
 			drm_file_err(uq_mgr->file, "Failed to map Queue\n");
 			trace_amdgpu_userq_create_end(queue, r);
+			amdgpu_userq_detach_doorbell(queue);
 			mutex_unlock(&uq_mgr->userq_mutex);
-			goto erase_doorbell;
+			amdgpu_userq_put(queue);
+
+			return r;
 		}
 	}
 
-	atomic_inc(&uq_mgr->userq_count[queue->queue_type]);
 	mutex_unlock(&uq_mgr->userq_mutex);
 
 	r = xa_alloc(&uq_mgr->userq_xa, &qid, queue,
@@ -877,8 +889,6 @@ amdgpu_userq_create(struct drm_file *filp, union drm_amdgpu_userq *args)
 	args->out.queue_id = qid;
 	return 0;
 
-erase_doorbell:
-	xa_erase_irq(&adev->userq_doorbell_xa, index);
 clean_mqd:
 	uq_funcs->mqd_destroy(queue);
 clean_doorbell_bo:
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 10/10] drm/amdgpu: take queue kref in userq_create to avoid UAF
  2026-08-28  9:53 [PATCH 00/10] drm/amdgpu: secure userq lifecycle by its kref Zhu Lingshan
                   ` (8 preceding siblings ...)
  2026-08-28  9:53 ` [PATCH 09/10] drm/amdgpu: free userq by kref_put when fails to create Zhu Lingshan
@ 2026-08-28  9:53 ` Zhu Lingshan
  9 siblings, 0 replies; 18+ messages in thread
From: Zhu Lingshan @ 2026-08-28  9:53 UTC (permalink / raw)
  To: Alexander.Deucher, Christian.Koenig, felix.kuehling
  Cc: Ray.Huang, amd-gfx, Zhu Lingshan

In amdgpu_userq_create(), once a newly created
user queue has been assigned a qid and published
to userq_xa, a concurrent AMDGPU_USERQ_OP_FREE ioctl
can free the queue, resulting in use-after-free
issues in amdgpu_userq_create.

This is surely a user space bug, but kernel should
not crash. This commit fixes this issue by
taking an additional kref before xa_alloc()

Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
index 21a9a2138fc8..83e6c87a5940 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
@@ -871,22 +871,32 @@ amdgpu_userq_create(struct drm_file *filp, union drm_amdgpu_userq *args)
 
 	mutex_unlock(&uq_mgr->userq_mutex);
 
+	/*
+	 * A concurrent AMDGPU_USERQ_OP_FREE ioctl can free the queue once
+	 * xa_alloc() publishes it and assigne a qid to it.
+	 * Take a kref to avoid use-after-free issues.
+	 */
+	kref_get(&queue->refcount);
 	r = xa_alloc(&uq_mgr->userq_xa, &qid, queue,
 		     XA_LIMIT(1, AMDGPU_MAX_USERQ_COUNT),
 		     GFP_KERNEL);
 	if (r) {
 		/*
-		 * This drops the last reference which should take care of
+		 * This drops the last two references which should take care of
 		 * all cleanup.
 		 */
 		trace_amdgpu_userq_create_end(queue, r);
 		amdgpu_userq_put(queue);
+		amdgpu_userq_put(queue);
+
 		return r;
 	}
 
 	amdgpu_debugfs_userq_init(filp, queue, qid);
 	trace_amdgpu_userq_create_end(queue, 0);
 	args->out.queue_id = qid;
+	amdgpu_userq_put(queue);
+
 	return 0;
 
 clean_mqd:
-- 
2.53.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH 01/10] drm/amdgpu: introduce amdgpu_lookup_queue_by_doorbell
  2026-08-28  9:53 ` [PATCH 01/10] drm/amdgpu: introduce amdgpu_lookup_queue_by_doorbell Zhu Lingshan
@ 2026-08-28 13:08   ` Christian König
  2026-08-28 15:59     ` Zhu, Lingshan
  0 siblings, 1 reply; 18+ messages in thread
From: Christian König @ 2026-08-28 13:08 UTC (permalink / raw)
  To: Zhu Lingshan, Alexander.Deucher, felix.kuehling; +Cc: Ray.Huang, amd-gfx

On 8/28/26 11:53, Zhu Lingshan wrote:
> This commit introduces a new helper
> amdgpu_lookup_queue_by_doorbell which helps
> look up a user queue with the given doorbell id
> in a xarray.
> 
> This function takes a kref of the user space queue.

Well absolutely clear NAK to the whole approach.

This is the nonsense Sunil and I have worked quite hard to remove and we certainly shouldn't repeat such mistakes.

When the userq needs to be used from interrupt context we need to hold the xa_lock_irqsave() or otherwise we don't have any guarantee that the userq, userq_mgr or associated fpriv went out of scope.

Grabbing references from this side would obviously result in circle dependencies.

Regards,
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 |  2 ++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> index 0a816b3c5ff9..e0639f844a8e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> @@ -609,6 +609,36 @@ struct amdgpu_usermode_queue *amdgpu_userq_get(struct amdgpu_userq_mgr *uq_mgr,
>  	return queue;
>  }
>  
> +/**
> + * amdgpu_lookup_queue_by_doorbell - look up a user queue by doorbell
> + * @xa: user queue XArray indexed by doorbell
> + * @doorbell: doorbell index
> + *
> + * Return: A queue with the doorbell indexed, or NULL if no such a queue found.
> + *
> + * This function increases kref of the queue, the caller
> + * must release the reference with amdgpu_userq_put().
> + */
> +struct amdgpu_usermode_queue *
> +amdgpu_lookup_queue_by_doorbell(struct xarray *xa, u32 doorbell)
> +{
> +	struct amdgpu_usermode_queue *queue;
> +	unsigned long flags;
> +
> +	xa_lock_irqsave(xa, flags);
> +	queue = xa_load(xa, doorbell);
> +	if (!queue)
> +		goto out_unlock;
> +
> +	if (!kref_get_unless_zero(&queue->refcount))
> +		queue = NULL;
> +
> +out_unlock:
> +	xa_unlock_irqrestore(xa, flags);
> +
> +	return queue;
> +}
> +
>  void amdgpu_userq_put(struct amdgpu_usermode_queue *queue)
>  {
>  	if (queue)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
> index 6412a7f7b6ef..8fc73862f64e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
> @@ -151,6 +151,8 @@ struct amdgpu_db_info {
>  };
>  
>  struct amdgpu_usermode_queue *amdgpu_userq_get(struct amdgpu_userq_mgr *uq_mgr, u32 qid);
> +struct amdgpu_usermode_queue *
> +amdgpu_lookup_queue_by_doorbell(struct xarray *xa, u32 doorbell);
>  void amdgpu_userq_put(struct amdgpu_usermode_queue *queue);
>  
>  int amdgpu_userq_ioctl(struct drm_device *dev, void *data, struct drm_file *filp);


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 02/10] drm/amdgpu: keep the userq manager alive as long as its queues
  2026-08-28  9:53 ` [PATCH 02/10] drm/amdgpu: keep the userq manager alive as long as its queues Zhu Lingshan
@ 2026-08-28 13:09   ` Christian König
  2026-08-28 15:59     ` Zhu, Lingshan
  0 siblings, 1 reply; 18+ messages in thread
From: Christian König @ 2026-08-28 13:09 UTC (permalink / raw)
  To: Zhu Lingshan, Alexander.Deucher, felix.kuehling; +Cc: Ray.Huang, amd-gfx

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.

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;


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 03/10] drm/amdgpu/gfx11: hold userq refs in private fault worker
  2026-08-28  9:53 ` [PATCH 03/10] drm/amdgpu/gfx11: hold userq refs in private fault worker Zhu Lingshan
@ 2026-08-28 13:11   ` Christian König
  2026-08-28 15:59     ` Zhu, Lingshan
  0 siblings, 1 reply; 18+ messages in thread
From: Christian König @ 2026-08-28 13:11 UTC (permalink / raw)
  To: Zhu Lingshan, Alexander.Deucher, felix.kuehling; +Cc: Ray.Huang, amd-gfx

On 8/28/26 11:53, Zhu Lingshan wrote:
> The GFX11 user queue private fault woker loads
> the relevnt user queue from the userq doorbell xarray.
> However it does not hold the spin_lock of the xarray
> when walking the xarray, and does not increase the
> kref of the user queue, so it races with queue
> destruction path and may run into an use-after-free
> userq problem.
> 
> This commit fixes this UAF problem by utilizing
> amdgpu_lookup_queue_by_doorbell helper, which
> properly hoding the xarray spin lock and the kref
> of the user queue.
> 
> Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c
> index a447562977ab..a063f86a8847 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c
> @@ -6733,9 +6733,12 @@ static void gfx_v11_0_userq_priv_fault_work(struct work_struct *work)
>  
>  		doorbell = (db_ctrl & CP_RB_DOORBELL_CONTROL__DOORBELL_OFFSET_MASK) >>
>  			   CP_RB_DOORBELL_CONTROL__DOORBELL_OFFSET__SHIFT;
> -		q = xa_load(&adev->userq_doorbell_xa, doorbell);
> -		if (q)
> +		q = amdgpu_lookup_queue_by_doorbell(&adev->userq_doorbell_xa,
> +						    doorbell);
> +		if (q) {
>  			amdgpu_userq_start_hang_detect_work(q);
> +			amdgpu_userq_put(q);
> +		}

This must just lock the XA irqsave, this code has been suggested like that before and been removed because it doesn't work correctly.

Please check the git history before suggesting bringing back a buggy approach.

Christian.

>  	}
>  }
>  


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 02/10] drm/amdgpu: keep the userq manager alive as long as its queues
  2026-08-28 13:09   ` Christian König
@ 2026-08-28 15:59     ` Zhu, Lingshan
  2026-08-28 16:26       ` Christian König
  0 siblings, 1 reply; 18+ messages in thread
From: Zhu, Lingshan @ 2026-08-28 15:59 UTC (permalink / raw)
  To: Christian König, Alexander.Deucher, felix.kuehling
  Cc: Ray.Huang, amd-gfx

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

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.

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: 5489 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 01/10] drm/amdgpu: introduce amdgpu_lookup_queue_by_doorbell
  2026-08-28 13:08   ` Christian König
@ 2026-08-28 15:59     ` Zhu, Lingshan
  0 siblings, 0 replies; 18+ messages in thread
From: Zhu, Lingshan @ 2026-08-28 15:59 UTC (permalink / raw)
  To: Christian König, Alexander.Deucher, felix.kuehling
  Cc: Ray.Huang, amd-gfx

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

On 8/28/2026 9:08 PM, Christian König wrote:

> On 8/28/26 11:53, Zhu Lingshan wrote:
>> This commit introduces a new helper
>> amdgpu_lookup_queue_by_doorbell which helps
>> look up a user queue with the given doorbell id
>> in a xarray.
>>
>> This function takes a kref of the user space queue.

Hello Christian

Thanks for your comments.

> Well absolutely clear NAK to the whole approach.
>
> This is the nonsense Sunil and I have worked quite hard to remove and we certainly shouldn't repeat such mistakes.
>
> When the userq needs to be used from interrupt context we need to hold the xa_lock_irqsave() or otherwise we don't have any guarantee that the userq, userq_mgr or associated fpriv went out of scope.

Holding the spin lock by xa_lock_irqsave() can surely avoid racing with the destruction process, however, it does not apply to all scenarios, for example, you can not hold spin lock in mes_userq_reset_queue(),
because it calls either amdgpu_mes_reset_queue_mmio or amdgpu_mes_reset_queue_mmio, both of them acquire the MES mutex through amdgpu_mes_lock.

Another thing, out of the topic is, holding xa_lock does not guarantee fpriv/userq_mgr alive, for example, when drm_device->unplugged is true, all amdgpu teardown paths in amdgpu_drm_release are skipped,
and the fpriv/userq_mgr is freed, no matter whether holding the xa spin lock.

So IMHO since we have userq->kref, lets use it to maintain the lifecycle of the queues. 

>
> Grabbing references from this side would obviously result in circle dependencies.

I am not sure, we should use the lock/unlock and kref_put/get in pairs in sequence, can you name some circle dependencies or AB-BA lockings as examples?

Thanks
Lingshan

>
> Regards,
> 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 |  2 ++
>>  2 files changed, 32 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>> index 0a816b3c5ff9..e0639f844a8e 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
>> @@ -609,6 +609,36 @@ struct amdgpu_usermode_queue *amdgpu_userq_get(struct amdgpu_userq_mgr *uq_mgr,
>>  	return queue;
>>  }
>>  
>> +/**
>> + * amdgpu_lookup_queue_by_doorbell - look up a user queue by doorbell
>> + * @xa: user queue XArray indexed by doorbell
>> + * @doorbell: doorbell index
>> + *
>> + * Return: A queue with the doorbell indexed, or NULL if no such a queue found.
>> + *
>> + * This function increases kref of the queue, the caller
>> + * must release the reference with amdgpu_userq_put().
>> + */
>> +struct amdgpu_usermode_queue *
>> +amdgpu_lookup_queue_by_doorbell(struct xarray *xa, u32 doorbell)
>> +{
>> +	struct amdgpu_usermode_queue *queue;
>> +	unsigned long flags;
>> +
>> +	xa_lock_irqsave(xa, flags);
>> +	queue = xa_load(xa, doorbell);
>> +	if (!queue)
>> +		goto out_unlock;
>> +
>> +	if (!kref_get_unless_zero(&queue->refcount))
>> +		queue = NULL;
>> +
>> +out_unlock:
>> +	xa_unlock_irqrestore(xa, flags);
>> +
>> +	return queue;
>> +}
>> +
>>  void amdgpu_userq_put(struct amdgpu_usermode_queue *queue)
>>  {
>>  	if (queue)
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
>> index 6412a7f7b6ef..8fc73862f64e 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.h
>> @@ -151,6 +151,8 @@ struct amdgpu_db_info {
>>  };
>>  
>>  struct amdgpu_usermode_queue *amdgpu_userq_get(struct amdgpu_userq_mgr *uq_mgr, u32 qid);
>> +struct amdgpu_usermode_queue *
>> +amdgpu_lookup_queue_by_doorbell(struct xarray *xa, u32 doorbell);
>>  void amdgpu_userq_put(struct amdgpu_usermode_queue *queue);
>>  
>>  int amdgpu_userq_ioctl(struct drm_device *dev, void *data, struct drm_file *filp);

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 03/10] drm/amdgpu/gfx11: hold userq refs in private fault worker
  2026-08-28 13:11   ` Christian König
@ 2026-08-28 15:59     ` Zhu, Lingshan
  0 siblings, 0 replies; 18+ messages in thread
From: Zhu, Lingshan @ 2026-08-28 15:59 UTC (permalink / raw)
  To: Christian König, Alexander.Deucher, felix.kuehling
  Cc: Ray.Huang, amd-gfx

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

On 8/28/2026 9:11 PM, Christian König wrote:

> On 8/28/26 11:53, Zhu Lingshan wrote:
>> The GFX11 user queue private fault woker loads
>> the relevnt user queue from the userq doorbell xarray.
>> However it does not hold the spin_lock of the xarray
>> when walking the xarray, and does not increase the
>> kref of the user queue, so it races with queue
>> destruction path and may run into an use-after-free
>> userq problem.
>>
>> This commit fixes this UAF problem by utilizing
>> amdgpu_lookup_queue_by_doorbell helper, which
>> properly hoding the xarray spin lock and the kref
>> of the user queue.
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c
>> index a447562977ab..a063f86a8847 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c
>> @@ -6733,9 +6733,12 @@ static void gfx_v11_0_userq_priv_fault_work(struct work_struct *work)
>>  
>>  		doorbell = (db_ctrl & CP_RB_DOORBELL_CONTROL__DOORBELL_OFFSET_MASK) >>
>>  			   CP_RB_DOORBELL_CONTROL__DOORBELL_OFFSET__SHIFT;
>> -		q = xa_load(&adev->userq_doorbell_xa, doorbell);
>> -		if (q)
>> +		q = amdgpu_lookup_queue_by_doorbell(&adev->userq_doorbell_xa,
>> +						    doorbell);
>> +		if (q) {
>>  			amdgpu_userq_start_hang_detect_work(q);
>> +			amdgpu_userq_put(q);
>> +		}
> This must just lock the XA irqsave, this code has been suggested like that before and been removed because it doesn't work correctly.\

The xa spin lock is held in the helper amdgpu_lookup_queue_by_doorbell().

Please see patch 1:

38 +amdgpu_lookup_queue_by_doorbell(struct xarray *xa, u32 doorbell)
 39 +{
 40 +       struct amdgpu_usermode_queue *queue;
 41 +       unsigned long flags;
 42 +
 43 +       xa_lock_irqsave(xa, flags);



Thanks
Lingshan

>
> Please check the git history before suggesting bringing back a buggy approach.
>
> Christian.
>
>>  	}
>>  }
>>  

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 02/10] drm/amdgpu: keep the userq manager alive as long as its queues
  2026-08-28 15:59     ` Zhu, Lingshan
@ 2026-08-28 16:26       ` Christian König
  0 siblings, 0 replies; 18+ messages in thread
From: Christian König @ 2026-08-28 16:26 UTC (permalink / raw)
  To: Zhu, Lingshan, Alexander.Deucher, felix.kuehling; +Cc: Ray.Huang, amd-gfx

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 <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;


^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2026-08-28 16:26 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-28  9:53 [PATCH 00/10] drm/amdgpu: secure userq lifecycle by its kref Zhu Lingshan
2026-08-28  9:53 ` [PATCH 01/10] drm/amdgpu: introduce amdgpu_lookup_queue_by_doorbell Zhu Lingshan
2026-08-28 13:08   ` Christian König
2026-08-28 15:59     ` Zhu, Lingshan
2026-08-28  9:53 ` [PATCH 02/10] drm/amdgpu: keep the userq manager alive as long as its queues Zhu Lingshan
2026-08-28 13:09   ` Christian König
2026-08-28 15:59     ` Zhu, Lingshan
2026-08-28 16:26       ` Christian König
2026-08-28  9:53 ` [PATCH 03/10] drm/amdgpu/gfx11: hold userq refs in private fault worker Zhu Lingshan
2026-08-28 13:11   ` Christian König
2026-08-28 15:59     ` Zhu, Lingshan
2026-08-28  9:53 ` [PATCH 04/10] drm/amdgpu/gfx12: " Zhu Lingshan
2026-08-28  9:53 ` [PATCH 05/10] drm/amdgpu: implement asynchronous userq destruction routine Zhu Lingshan
2026-08-28  9:53 ` [PATCH 06/10] drm/amdgpu: hold userq kref in MES reset Zhu Lingshan
2026-08-28  9:53 ` [PATCH 07/10] drm/amdgpu: hold userq kref during isolation scheduling Zhu Lingshan
2026-08-28  9:53 ` [PATCH 08/10] drm/amdgpu: hold userq kref during suspend and resume Zhu Lingshan
2026-08-28  9:53 ` [PATCH 09/10] drm/amdgpu: free userq by kref_put when fails to create Zhu Lingshan
2026-08-28  9:53 ` [PATCH 10/10] drm/amdgpu: take queue kref in userq_create to avoid UAF Zhu Lingshan

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.