Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [Critical] Concurrent `pvr_submit_jobs` and `pvr_context_destroy` causes a Use-After-Free of scheduler entity stats. - [High] Use-After-Free of `ctx->fw_obj` in the error path of `pvr_context_create()`. -- commit aae839872c492d8ca3e68e0c05f57b751671962e Author: Brajesh Gupta <[email protected]> Subject: drm/imagination: Fix double call to drm_sched_entity_fini() This commit addresses a double call to drm_sched_entity_fini() during the destruction of a PVR context. It prevents drm_sched_entity_fini() from being called in pvr_context_put() if it has already been called during pvr_context_kill_queues(). References: fd177135f0e6 ("drm/sched: Account entity GPU time") [ ... ] > diff --git a/drivers/gpu/drm/imagination/pvr_context.c > b/drivers/gpu/drm/imagination/pvr_context.c > index eba4694400b5a..74c8aad7fe64e 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 isn't a bug introduced by this patch, but does the error path here trigger a use-after-free? When xa_alloc() fails in pvr_context_create(), it jumps to err_destroy_fw_obj which calls pvr_fw_object_destroy(ctx->fw_obj). The ctx->fw_obj pointer is not cleared, and execution falls through to err_destroy_queues. pvr_context_destroy_queues() then triggers pvr_queue_destroy(), which calls pvr_queue_cleanup_fw_context(). Because the dangling fw_obj pointer is not NULL, it bypasses the NULL check and is passed to pvr_fw_structure_cleanup(), where it is dereferenced. [ ... ] > diff --git a/drivers/gpu/drm/imagination/pvr_queue.c > b/drivers/gpu/drm/imagination/pvr_queue.c > index 7ed60e1c1a866..324bdadc3d718 100644 > --- a/drivers/gpu/drm/imagination/pvr_queue.c > +++ b/drivers/gpu/drm/imagination/pvr_queue.c [ ... ] > @@ -1453,7 +1455,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 (queue_entity_fini) > + drm_sched_entity_fini(&queue->entity); [Severity: Critical] This is a pre-existing issue, but is there a race condition between pvr_submit_jobs() and pvr_context_destroy() that can cause a use-after-free of the scheduler entity stats? pvr_submit_jobs() looks up the context using pvr_context_lookup(), which acquires a kref on the context without holding any locks blocking destruction. Concurrently, a thread calling pvr_context_destroy() removes the context handle via xa_erase() and immediately calls pvr_context_kill_queues() without waiting for the context's kref to reach 0. pvr_context_kill_queues() invokes pvr_queue_kill(), which calls drm_sched_entity_destroy(). This flushes the entity and calls drm_sched_entity_fini(), freeing entity->stats. pvr_submit_jobs() then proceeds to arm and push jobs via pvr_queue_job_push() -> drm_sched_job_arm(), which dereferences the freed entity->stats. Could this be reliably triggered by a userspace program running PVR_SUBMIT_JOBS and PVR_DESTROY_CONTEXT ioctls simultaneously on different threads? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
