On 10/24/26 1:19 AM, Dmitry Baryshkov wrote:
> msm_disp_snapshot_init() only logs a failure of kthread_run_worker() and
> returns success, leaving the error pointer in kms->dump_worker. Both of
> its users then dereference it: msm_disp_snapshot_state() queues the dump
> work on it on the first display error, and msm_disp_snapshot_destroy()
> only checks the pointer for NULL before passing it to
> kthread_destroy_worker().
>
> Clear the pointer and return the error instead, so that the KMS init
> fails rather than leaving a snapshot facility which crashes when used.
>
> Fixes: 98659487b845 ("drm/msm: add support to take dpu snapshot")
> Assisted-by: LLM
> Signed-off-by: Dmitry Baryshkov <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/msm_disp_snapshot.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> index d99771684728..d1b10656e41a 100644
> --- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> @@ -98,6 +98,7 @@ int msm_disp_snapshot_init(struct drm_device *drm_dev)
> {
> struct msm_drm_private *priv;
> struct msm_kms *kms;
> + int ret;
>
> if (!drm_dev) {
> DRM_ERROR("invalid params\n");
> @@ -110,12 +111,21 @@ int msm_disp_snapshot_init(struct drm_device *drm_dev)
> mutex_init(&kms->dump_mutex);
>
> kms->dump_worker = kthread_run_worker(0, "%s", "disp_snapshot");
> - if (IS_ERR(kms->dump_worker))
> + if (IS_ERR(kms->dump_worker)) {
> + ret = PTR_ERR(kms->dump_worker);
> DRM_ERROR("failed to create disp state task\n");
> + goto err_destroy_mutex;
> + }
>
> kthread_init_work(&kms->dump_work, _msm_disp_snapshot_work);
>
> return 0;
> +
> +err_destroy_mutex:
> + kms->dump_worker = NULL;
> + mutex_destroy(&kms->dump_mutex);
LLM:
The error path calls mutex_destroy(&kms->dump_mutex), but
msm_disp_snapshot_destroy() runs later on the same failure path and
destroys the mutex again. That's harmless in practice (mutex_destroy()
on an unlocked mutex is benign, and the debug checks don't fire), but
it's a redundant double destroy. A simpler version would drop the mutex
handling from the error path and let the destroy function own it
Konrad
