Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] drm/msm: premature flush_work() of crashstate_free_work in msm_gpu_cleanup() --
commit 9e216db338c387162280bcdd65d14c7f062f6e18 Author: Nico Angelo Dela Cruz <[email protected]> drm/msm: free the crash state asynchronously This commit defers msm_gpu_crashstate_put() in msm_gpu_devcoredump_free() to a work item on system_dfl_wq so that synchronous free callbacks from dev_coredumpm() do not recursively acquire gpu->lock. > diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c > index caa86279f561d..fded78fdbf8db 100644 > --- a/drivers/gpu/drm/msm/msm_gpu.c > +++ b/drivers/gpu/drm/msm/msm_gpu.c [ ... ] > @@ -1047,6 +1062,10 @@ void msm_gpu_cleanup(struct msm_gpu *gpu) > > DBG("%s", gpu->name); > > + /* May be unset if probe failed before msm_gpu_init() */ > + if (gpu->crashstate_free_work.func) > + flush_work(&gpu->crashstate_free_work); [Severity: High] Could flushing gpu->crashstate_free_work at the beginning of msm_gpu_cleanup() lead to a use-after-free of struct msm_gpu if msm_gpu_devcoredump_free() is invoked later during teardown? In msm_gpu_cleanup(), the MMU fault handler and gpu->worker are only stopped further down: drivers/gpu/drm/msm/msm_gpu.c:msm_gpu_cleanup() { if (gpu->crashstate_free_work.func) flush_work(&gpu->crashstate_free_work); ... if (!IS_ERR_OR_NULL(gpu->vm)) { struct msm_mmu *mmu = to_msm_vm(gpu->vm)->mmu; mmu->funcs->detach(mmu); drm_gpuvm_put(gpu->vm); } if (gpu->worker) { kthread_destroy_worker(gpu->worker); } } Both recover_worker() (running on gpu->worker) and adreno_fault_handler() call msm_gpu_crashstate_capture() -> dev_coredumpm(..., msm_gpu_devcoredump_free). During unbind in adreno_unbind(), if adreno_system_suspend() times out waiting for a hung GPU or returns once retire_submits() drops active_submits to zero before recover_worker() finishes, adreno_unbind() proceeds to gpu->funcs->destroy(gpu) -> adreno_gpu_cleanup() -> msm_gpu_cleanup(). If kthread_destroy_worker(gpu->worker) drains a pending recover_work (or a fault occurs before mmu->funcs->detach(mmu)) and dev_coredumpm() fails or devcoredump is disabled, msm_gpu_devcoredump_free() will queue gpu->crashstate_free_work on system_dfl_wq after the flush_work() at the top of msm_gpu_cleanup() has already completed. Similarly, if an earlier dev_coredumpm() succeeded and left an active devcoredump device, msm_gpu_cleanup() does not call dev_coredump_put(&gpu->pdev->dev) before flushing crashstate_free_work. When that devcoredump later expires or is cleared via sysfs, devcd_dev_release() calls msm_gpu_devcoredump_free(gpu) and queues gpu->crashstate_free_work. Once msm_gpu_cleanup() and adreno_gpu_cleanup() return, the destroy callback (such as a6xx_destroy()) frees the enclosing GPU structure with kfree(), so msm_gpu_crashstate_free_worker() can subsequently run on system_dfl_wq and dereference freed memory in msm_gpu_crashstate_put(gpu). Should msm_gpu_cleanup() call dev_coredump_put(&gpu->pdev->dev) and flush_work(&gpu->crashstate_free_work) after mmu->funcs->detach(mmu) and kthread_destroy_worker(gpu->worker) have completed? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
