[PATCH v5] drm/imagination: Fix double call to drm_sched_entity_fini()

[PATCH v5] drm/imagination: Fix double call to drm_sched_entity_fini()

[Brajesh Gupta]

Call sequence of double call:
pvr_context_destroy
  pvr_context_kill_queues
    pvr_queue_kill
      drm_sched_entity_destroy
        drm_sched_entity_fini // here
  pvr_context_put
    kref_put(..., pvr_context_release)
      pvr_context_destroy_queues
        pvr_queue_destroy
          drm_sched_entity_fini // here

Call to drm_sched_entity_destroy() from pvr_context_kill_queues() calls
drm_sched_entity_flush() + drm_sched_entity_fini().
drm_sched_entity_flush() ensures all pending jobs are completed and
drm_sched_entity_fini() ensures no further submission is allowed as
per expectation from pvr_context_kill_queues(). Double call to
drm_sched_entity_fini() is misuse of the API so keep call only in
pvr_context_create() failure path.

Stack trace for issue with addition of refcounting for DRM entity
stats in commit fd177135f0e6 ("drm/sched: Account entity GPU time"):
[  789.490527] ------------[ cut here ]------------
[  789.490559] refcount_t: underflow; use-after-free.
[  789.490657] WARNING: lib/refcount.c:28 at refcount_warn_saturate+0xf4/0x144, CPU#0: kworker/u16:1/440
[  789.490695] Modules linked in: powervr drm_gpuvm drm_exec gpu_sched drm_shmem_helper xhci_plat_hcd xhci_hcd dwc3 usbcore usb_common snd_soc_simple_card snd_soc_simple_card_utils sa2ul sha512 sha256 dwc3_am62 sha1 authenc rti_wdt libsha512 at24 sch_fq_codel fuse dm_mod ipv6
[  789.490798] CPU: 0 UID: 0 PID: 440 Comm: kworker/u16:1 Not tainted 7.0.0-rc7-02049-g5e2c0700091b #22 PREEMPT
[  789.490809] Hardware name: Texas Instruments AM625 SK (DT)
[  789.490815] Workqueue: powervr-sched pvr_queue_fence_release_work [powervr]
[  789.490868] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[  789.490876] pc : refcount_warn_saturate+0xf4/0x144
[  789.490884] lr : refcount_warn_saturate+0xf4/0x144
[  789.490892] sp : ffff8000822cbcc0
[  789.490895] x29: ffff8000822cbcc0 x28: 0000000000000000 x27: 0000000000000000
[  789.490909] x26: 0000000000000000 x25: ffff800081b1e338 x24: ffff000004541405
[  789.490922] x23: ffff000004bea950 x22: ffff00000042e400 x21: ffff000007123e30
[  789.490935] x20: ffff000007123000 x19: ffff000007a80d50 x18: fffffffffffe7768
[  789.490948] x17: 74736574202c6e6f x16: 697461746e656d65 x15: ffff800081b269f0
[  789.490962] x14: 0000000000000030 x13: ffff800081b26a70 x12: 0000000000000211
[  789.490975] x11: 00000000000000c0 x10: 0000000000000b50 x9 : ffff8000822cbb30
[  789.490988] x8 : ffff0000014e7bb0 x7 : ffff00007725e780 x6 : 0000000372a05f49
[  789.491001] x5 : 0000000000000000 x4 : 0000000000000001 x3 : 0000000000000010
[  789.491013] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0000014e7000
[  789.491027] Call trace:
[  789.491032]  refcount_warn_saturate+0xf4/0x144 (P)
[  789.491043]  drm_sched_entity_fini+0x164/0x18c [gpu_sched]
[  789.491081]  pvr_queue_destroy+0x64/0x134 [powervr]
[  789.491110]  pvr_context_destroy_queues+0x34/0x64 [powervr]
[  789.491138]  pvr_context_release+0x70/0xac [powervr]
[  789.491166]  pvr_context_put.part.0+0x5c/0x7c [powervr]
[  789.491193]  pvr_context_put+0x14/0x24 [powervr]
[  789.491221]  pvr_queue_fence_release_work+0x20/0x38 [powervr]
[  789.491249]  process_one_work+0x160/0x4c4
[  789.491264]  worker_thread+0x188/0x310
[  789.491276]  kthread+0x130/0x13c
[  789.491287]  ret_from_fork+0x10/0x20
[  789.491300] ---[ end trace 0000000000000000 ]---

Fixes: eaf01ee5ba28 ("drm/imagination: Implement job submission and scheduling")
Cc: stable@vger.kernel.org
Signed-off-by: Brajesh Gupta <brajesh.gupta@imgtec.com>
---
Changes in v5:
- Update description of the issue and added stable tag.
- Modified variable name to align with behaviour.
- Link to v4: https://lore.kernel.org/r/20260619-b4-sched_fix-v4-1-65de5b2fd71d@imgtec.com

Changes in v4:
- Simplify logic in v3 by pushing new flag to pvr_queue_destroy().
- Link to v3: https://lore.kernel.org/r/20260611-b4-sched_fix-v3-1-693beb50ea01@imgtec.com

Changes in v3:
- Fixed a typo.
- Handled missing memory leak for RENDER_CONTEXT.
- Link to v2: https://lore.kernel.org/r/20260611-b4-sched_fix-v2-1-17a93be86fcd@imgtec.com

Changes in v2:
- Fixed memory leak identified in following error path handling of pvr_context_create():
- pvr_context_create()
-   ...
-   err_destroy_queues:
-     pvr_context_destroy_queues()
-       pvr_queue_destroy()
- Link to v1: https://lore.kernel.org/r/20260610-b4-sched_fix-v1-1-c5977a6e0b4c@imgtec.com
---
 drivers/gpu/drm/imagination/pvr_context.c | 18 ++++++++++--------
 drivers/gpu/drm/imagination/pvr_queue.c   |  6 ++++--
 drivers/gpu/drm/imagination/pvr_queue.h   |  2 +-
 3 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/imagination/pvr_context.c b/drivers/gpu/drm/imagination/pvr_context.c
index eba4694400b5..b6f9e078315d 100644
--- a/drivers/gpu/drm/imagination/pvr_context.c
+++ b/drivers/gpu/drm/imagination/pvr_context.c
@@ -161,22 +161,24 @@ ctx_fw_data_init(void *cpu_ptr, void *priv)
 /**
  * pvr_context_destroy_queues() - Destroy all queues attached to a context.
  * @ctx: Context to destroy queues on.
+ * @cleanup_queue_entity: Whether to cleanup the queue entity e.g. context
+ *                      creation failure path.
  *
  * Should be called when the last reference to a context object is dropped.
  * It releases all resources attached to the queues bound to this context.
  */
-static void pvr_context_destroy_queues(struct pvr_context *ctx)
+static void pvr_context_destroy_queues(struct pvr_context *ctx, bool cleanup_queue_entity)
 {
 	switch (ctx->type) {
 	case DRM_PVR_CTX_TYPE_RENDER:
-		pvr_queue_destroy(ctx->queues.fragment);
-		pvr_queue_destroy(ctx->queues.geometry);
+		pvr_queue_destroy(ctx->queues.fragment, cleanup_queue_entity);
+		pvr_queue_destroy(ctx->queues.geometry, cleanup_queue_entity);
 		break;
 	case DRM_PVR_CTX_TYPE_COMPUTE:
-		pvr_queue_destroy(ctx->queues.compute);
+		pvr_queue_destroy(ctx->queues.compute, cleanup_queue_entity);
 		break;
 	case DRM_PVR_CTX_TYPE_TRANSFER_FRAG:
-		pvr_queue_destroy(ctx->queues.transfer);
+		pvr_queue_destroy(ctx->queues.transfer, cleanup_queue_entity);
 		break;
 	}
 }
@@ -240,7 +242,7 @@ static int pvr_context_create_queues(struct pvr_context *ctx,
 	return -EINVAL;
 
 err_destroy_queues:
-	pvr_context_destroy_queues(ctx);
+	pvr_context_destroy_queues(ctx, true);
 	return err;
 }
 
@@ -349,7 +351,7 @@ int pvr_context_create(struct pvr_file *pvr_file, struct drm_pvr_ioctl_create_co
 	pvr_fw_object_destroy(ctx->fw_obj);
 
 err_destroy_queues:
-	pvr_context_destroy_queues(ctx);
+	pvr_context_destroy_queues(ctx, true);
 
 err_free_ctx_id:
 	/*
@@ -384,7 +386,7 @@ pvr_context_release(struct kref *ref_count)
 	spin_unlock(&pvr_dev->ctx_list_lock);
 
 	xa_erase(&pvr_dev->ctx_ids, ctx->ctx_id);
-	pvr_context_destroy_queues(ctx);
+	pvr_context_destroy_queues(ctx, false);
 	pvr_fw_object_destroy(ctx->fw_obj);
 	kfree(ctx->data);
 	pvr_vm_context_put(ctx->vm_ctx);
diff --git a/drivers/gpu/drm/imagination/pvr_queue.c b/drivers/gpu/drm/imagination/pvr_queue.c
index 7ed60e1c1a86..941c017399fc 100644
--- a/drivers/gpu/drm/imagination/pvr_queue.c
+++ b/drivers/gpu/drm/imagination/pvr_queue.c
@@ -1439,11 +1439,12 @@ void pvr_queue_kill(struct pvr_queue *queue)
 /**
  * pvr_queue_destroy() - Destroy a queue.
  * @queue: The queue to destroy.
+ * @cleanup_queue_entity: Whether to cleanup the queue entity.
  *
  * Cleanup the queue and free the resources attached to it. Should be
  * called from the context release function.
  */
-void pvr_queue_destroy(struct pvr_queue *queue)
+void pvr_queue_destroy(struct pvr_queue *queue, bool cleanup_queue_entity)
 {
 	if (!queue)
 		return;
@@ -1453,7 +1454,8 @@ void pvr_queue_destroy(struct pvr_queue *queue)
 	mutex_unlock(&queue->ctx->pvr_dev->queues.lock);
 
 	drm_sched_fini(&queue->scheduler);
-	drm_sched_entity_fini(&queue->entity);
+	if (cleanup_queue_entity)
+		drm_sched_entity_fini(&queue->entity);
 
 	if (WARN_ON(queue->last_queued_job_scheduled_fence))
 		dma_fence_put(queue->last_queued_job_scheduled_fence);
diff --git a/drivers/gpu/drm/imagination/pvr_queue.h b/drivers/gpu/drm/imagination/pvr_queue.h
index 4aa72665ce25..5b07d7edf1c1 100644
--- a/drivers/gpu/drm/imagination/pvr_queue.h
+++ b/drivers/gpu/drm/imagination/pvr_queue.h
@@ -158,7 +158,7 @@ struct pvr_queue *pvr_queue_create(struct pvr_context *ctx,
 
 void pvr_queue_kill(struct pvr_queue *queue);
 
-void pvr_queue_destroy(struct pvr_queue *queue);
+void pvr_queue_destroy(struct pvr_queue *queue, bool queue_entity_fini);
 
 void pvr_queue_process(struct pvr_queue *queue);
 

---
base-commit: 61de054a772a1feda6364931ab1baf9038abf1c8
change-id: 20260610-b4-sched_fix-ac3b920f475b

Best regards,
-- 
Brajesh Gupta <brajesh.gupta@imgtec.com>

Re: [PATCH v5] drm/imagination: Fix double call to drm_sched_entity_fini()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Exposing a partially initialized context via `xa_alloc` allows concurrent destruction, leading to a NULL pointer dereference or Use-After-Free.
- [Critical] Concurrent execution of `pvr_submit_jobs` and `pvr_context_destroy` leads to leaked jobs, contexts, and Use-After-Free vulnerabilities.
- [High] Use-After-Free of `ctx->fw_obj` in the `pvr_context_create` error path when `xa_alloc` fails.
- [High] ABBA deadlock between pvr_queue_device_pre_reset() and the DRM scheduler run job worker.
--

commit 98944fa2f1c44497ea7e4317dca4599e33e19c35
Author: Brajesh Gupta <brajesh.gupta@imgtec.com>

drm/imagination: Fix double call to drm_sched_entity_fini()

This commit prevents a double call to drm_sched_entity_fini() during context 
destruction by only calling it in the pvr_context_create() failure path. The 
prior logic incorrectly triggered entity destruction from both pvr_queue_kill() 
and pvr_queue_destroy().

> diff --git a/drivers/gpu/drm/imagination/pvr_context.c b/drivers/gpu/drm/imagination/pvr_context.c
> index eba4694400b5a..b6f9e078315dc 100644
> --- a/drivers/gpu/drm/imagination/pvr_context.c
> +++ b/drivers/gpu/drm/imagination/pvr_context.c
[ ... ]
> @@ -349,7 +351,7 @@ int pvr_context_create(struct pvr_file *pvr_file, struct drm_pvr_ioctl_create_co
>  	pvr_fw_object_destroy(ctx->fw_obj);
>  
>  err_destroy_queues:
> -	pvr_context_destroy_queues(ctx);
> +	pvr_context_destroy_queues(ctx, true);

[Severity: High]
This is a pre-existing issue, but does this error path lead to a use-after-free
if context handles or memory are exhausted?

If xa_alloc() fails earlier in pvr_context_create(), the error path jumps to
err_destroy_fw_obj, which calls pvr_fw_object_destroy(ctx->fw_obj). This frees
the firmware object but leaves the pointer in ctx->fw_obj intact.

Execution then falls through to err_destroy_queues, which calls
pvr_context_destroy_queues() -> pvr_queue_destroy() -> 
pvr_queue_cleanup_fw_context().

Since queue->ctx->fw_obj is non-NULL, pvr_queue_cleanup_fw_context() will call
pvr_fw_structure_cleanup() on the already-freed pointer, causing a 
use-after-free:

drivers/gpu/drm/imagination/pvr_queue.c:pvr_queue_cleanup_fw_context() {
	if (!queue->ctx->fw_obj)
		return 0;

	return pvr_fw_structure_cleanup(queue->ctx->pvr_dev, ...);
}

Could ctx->fw_obj be set to NULL after destruction, or could this cleanup logic
be restructured to avoid accessing the freed pointer?

[Severity: Critical]
This is also a pre-existing issue, but does the successful creation path
expose a partially initialized context to userspace?

In pvr_context_create(), after successfully allocating a handle via xa_alloc(),
the context handle is immediately visible to userspace:

drivers/gpu/drm/imagination/pvr_context.c:pvr_context_create() {
	err = xa_alloc(&pvr_file->ctx_handles, &args->handle, ctx,
                       xa_limit_32b, GFP_KERNEL);
	if (err)
		goto err_destroy_fw_obj;

	spin_lock(&pvr_dev->ctx_list_lock);
	list_add_tail(&ctx->file_link, &pvr_file->contexts);
    ...
}

If a concurrent thread guesses the newly allocated handle and calls
pvr_ioctl_destroy_context() before list_add_tail() completes, it will
find the context, remove it, and drop its reference. This triggers
pvr_context_release(), which calls list_del(&ctx->file_link). Since
ctx->file_link is still NULL, this dereferences a NULL pointer.

If list debugging catches it, the original thread might still access
the freed context to add it to the list, leading to a use-after-free.

Should the context be fully initialized and added to the list before it
is exposed via xa_alloc()?

>  
>  err_free_ctx_id:
>  	/*
[ ... ]
> @@ -384,7 +386,7 @@ pvr_context_release(struct kref *ref_count)
>  	spin_unlock(&pvr_dev->ctx_list_lock);
>  
>  	xa_erase(&pvr_dev->ctx_ids, ctx->ctx_id);
> -	pvr_context_destroy_queues(ctx);
> +	pvr_context_destroy_queues(ctx, false);
>  	pvr_fw_object_destroy(ctx->fw_obj);
>  	kfree(ctx->data);
>  	pvr_vm_context_put(ctx->vm_ctx);
> diff --git a/drivers/gpu/drm/imagination/pvr_queue.c b/drivers/gpu/drm/imagination/pvr_queue.c
> index 7ed60e1c1a866..941c017399fc3 100644
> --- a/drivers/gpu/drm/imagination/pvr_queue.c
> +++ b/drivers/gpu/drm/imagination/pvr_queue.c
[ ... ]
> @@ -1439,11 +1439,12 @@ void pvr_queue_kill(struct pvr_queue *queue)
>  /**
>   * pvr_queue_destroy() - Destroy a queue.
>   * @queue: The queue to destroy.
> + * @cleanup_queue_entity: Whether to cleanup the queue entity.
>   *
>   * Cleanup the queue and free the resources attached to it. Should be
>   * called from the context release function.
>   */
> -void pvr_queue_destroy(struct pvr_queue *queue)
> +void pvr_queue_destroy(struct pvr_queue *queue, bool cleanup_queue_entity)
>  {
>  	if (!queue)
>  		return;
> @@ -1453,7 +1454,8 @@ void pvr_queue_destroy(struct pvr_queue *queue)
>  	mutex_unlock(&queue->ctx->pvr_dev->queues.lock);

[Severity: High]
This is a pre-existing issue, but does taking pvr_dev->queues.lock here or
during GPU reset risk a deadlock with the scheduler's run job worker?

During a GPU reset, pvr_queue_device_pre_reset() takes queues.lock and
then calls pvr_queue_stop() -> drm_sched_stop() -> drm_sched_wqueue_stop(),
which waits for the currently executing scheduler job to finish:

drivers/gpu/drm/imagination/pvr_queue.c:pvr_queue_device_pre_reset() {
	mutex_lock(&pvr_dev->queues.lock);
	list_for_each_entry(queue, &pvr_dev->queues.idle, node)
		pvr_queue_stop(queue, NULL);
    ...
}

Concurrently, if the scheduler is executing drm_sched_run_job_work() ->
pvr_queue_run_job() -> pvr_queue_submit_job_to_cccb() ->
pvr_queue_update_active_state(), it will try to acquire the same queues.lock:

drivers/gpu/drm/imagination/pvr_queue.c:pvr_queue_update_active_state() {
	mutex_lock(&pvr_dev->queues.lock);
	pvr_queue_update_active_state_locked(queue);
	mutex_unlock(&pvr_dev->queues.lock);
}

This creates a scenario where the reset path holds queues.lock and waits for
the job worker to finish, while the job worker is blocked trying to acquire
queues.lock. Can this deadlock be avoided?

>  
>  	drm_sched_fini(&queue->scheduler);
> -	drm_sched_entity_fini(&queue->entity);
> +	if (cleanup_queue_entity)
> +		drm_sched_entity_fini(&queue->entity);

[Severity: Critical]
This isn't a bug introduced by this patch, but does killing the queue entity
early lead to leaked jobs and a use-after-free if jobs are still being pushed?

In pvr_context_destroy(), if another thread is simultaneously executing
pvr_submit_jobs(), it might hold a valid reference to the context. However,
pvr_context_destroy() calls pvr_context_kill_queues(), which forcibly destroys
the scheduler entity before the context is actually released:

drivers/gpu/drm/imagination/pvr_context.c:pvr_context_destroy() {
	pvr_context_kill_queues(ctx);

	/* Release the reference held by the handle set. */
	pvr_context_put(ctx);
}

This destroys the DRM scheduler entity via drm_sched_entity_destroy(), which
also frees entity->stats.

Meanwhile, the concurrent thread in pvr_submit_jobs() can proceed to push jobs
to this destroyed entity:

drivers/gpu/drm/imagination/pvr_job.c:pvr_submit_jobs() {
    ...
	for (i = 0; i < args->jobs_count; i++) {
		pvr_queue_job_arm(jobs[i]);
	}
    ...
}

pvr_queue_job_arm() -> drm_sched_job_arm() will access the freed entity->stats
via drm_sched_entity_stats_get(), causing a use-after-free. Additionally, the
pushed jobs are permanently leaked, which in turn leaks the context they hold a
reference to.

Should the entity destruction be deferred until the context reference count
reaches zero?

>  
>  	if (WARN_ON(queue->last_queued_job_scheduled_fence))
>  		dma_fence_put(queue->last_queued_job_scheduled_fence);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630-b4-sched_fix-v5-1-2a84cbf18bfe@imgtec.com?part=1