[PATCH 1/3] drm/amdgpu/userq: don't publish the queue before create is done with it

[Jesse Zhang]

amdgpu_userq_create() inserts the queue into userq_xa and then sets up
its debugfs entry without holding a reference. A concurrent
AMDGPU_USERQ_OP_FREE on the (easily guessed) queue ID can free the queue
in that window, so create writes to freed memory and leaves a queue_N
debugfs directory whose mqd_info points to the freed queue:

  debugfs: 'queue_1' already exists in 'client-137'

Only reserve the ID in xa_alloc() and store the queue once create is
done with it. Don't let FREE release a reserved ID.

Fixes: 28cacaace5cd ("drm/amdgpu/userq: defer queue publication until create 
completes")
Signed-off-by: Jesse Zhang <jesse.zh...@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
index 63875761dfac..89e03d3a930c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
@@ -775,7 +775,12 @@ amdgpu_userq_create(struct drm_file *filp, union 
drm_amdgpu_userq *args)
        atomic_inc(&uq_mgr->userq_count[queue->queue_type]);
        mutex_unlock(&uq_mgr->userq_mutex);
 
-       r = xa_alloc(&uq_mgr->userq_xa, &qid, queue,
+       /*
+        * Only reserve the ID here. Once the queue is visible in userq_xa,
+        * a concurrent AMDGPU_USERQ_OP_FREE can drop the last reference, so
+        * publish it only after we are done with it.
+        */
+       r = xa_alloc(&uq_mgr->userq_xa, &qid, NULL,
                     XA_LIMIT(1, AMDGPU_MAX_USERQ_COUNT),
                     GFP_KERNEL);
        if (r) {
@@ -790,6 +795,8 @@ amdgpu_userq_create(struct drm_file *filp, union 
drm_amdgpu_userq *args)
 
        amdgpu_debugfs_userq_init(filp, queue, qid);
        trace_amdgpu_userq_create_end(queue, 0);
+       /* Storing to a reserved entry doesn't allocate memory and can't fail */
+       xa_store(&uq_mgr->userq_xa, qid, queue, GFP_KERNEL);
        args->out.queue_id = qid;
        return 0;
 
@@ -924,7 +931,10 @@ int amdgpu_userq_ioctl(struct drm_device *dev, void *data,
 
        case AMDGPU_USERQ_OP_FREE: {
                xa_lock(&fpriv->userq_mgr.userq_xa);
-               queue = __xa_erase(&fpriv->userq_mgr.userq_xa, 
args->in.queue_id);
+               /* Don't release an ID reserved by an in-flight CREATE */
+               queue = xa_load(&fpriv->userq_mgr.userq_xa, args->in.queue_id);
+               if (queue)
+                       __xa_erase(&fpriv->userq_mgr.userq_xa, 
args->in.queue_id);
                xa_unlock(&fpriv->userq_mgr.userq_xa);
                if (!queue)
                        return -ENOENT;
-- 
2.50.1