Re: [PATCH 2/4] drm/msm: free the crash state asynchronously

[sashiko-bot@kernel.org]

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 <nicoangelodelacruz@gmail.com>

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/20261101175553.53746-1-nicoangelodelacruz@gmail.com?part=2