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

[sashiko-bot@kernel.org]

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