Yes, this is right. Draining gpu->worker can still run a recover_work that captures a crash state, and a devcoredump that is still registered queues the free when it is released, so the flush has to come last. I'll move it after kthread_destroy_worker(), add dev_coredump_put() before it (which also closes the pre-existing lifetime gap mentioned in the cover letter), and send that in v2 once the series has had human review.
Thanks, Nico On Mon, Nov 2, 2026 at 2:20 AM <[email protected]> wrote: > 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 >
