Re: [PATCH v3 3/8] drm/msm: unwind msm_drm_kms_init() on failure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] msm_disp_snapshot_destroy() unconditionally passes an ERR_PTR to kthread_destroy_worker() if kthread creation failed.
- [High] destroy_workqueue() is called on a potentially NULL kms->wq when msm_kms_init() fails.
- [High] Double destruction of DRM private object state if hw_init fails.
--

commit 972acdabb4a5813d8316cd0796e9b9df36ce8dff
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Subject: drm/msm: unwind msm_drm_kms_init() on failure

This commit updates the error unwinding in msm_drm_kms_init() and
msm_drm_init() to avoid calling the full teardown process when early
initialization steps fail. It undoes only the steps which have successfully
completed.

> diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
> --- a/drivers/gpu/drm/msm/msm_kms.c
> +++ b/drivers/gpu/drm/msm/msm_kms.c
[ ... ]
> @@ -328,14 +334,20 @@ int msm_drm_kms_init(struct device *dev, const struct drm_driver *drv)
>  	pm_runtime_put_sync(dev);
>  	if (ret < 0) {
>  		DRM_DEV_ERROR(dev, "failed to install IRQ handler\n");
> -		goto err_msm_uninit;
> +		goto err_destroy_event_threads;
>  	}
>  
>  	drm_mode_config_reset(ddev);
>  
>  	return 0;
>  
> -err_msm_uninit:
> +err_destroy_event_threads:
> +	msm_drm_kms_destroy_event_threads(kms);
> +err_destroy_kms:
> +	msm_disp_snapshot_destroy(ddev);

[Severity: High]
This is a pre-existing issue, but does this error path lead to a kernel 
panic? 

If kthread_run_worker fails in msm_disp_snapshot_init, it stores an ERR_PTR 
in kms->dump_worker but returns success. When we hit this error path and call 
msm_disp_snapshot_destroy, it evaluates if (kms->dump_worker) which is true 
for an ERR_PTR, and passes the error pointer directly to 
kthread_destroy_worker, leading to an invalid memory dereference.

> +	if (kms->funcs)
> +		kms->funcs->destroy(kms);

[Severity: High]
This is also a pre-existing issue, but could this cause a NULL pointer 
dereference if the workqueue failed to allocate?

If alloc_ordered_workqueue fails earlier in msm_kms_init, kms->wq is NULL 
but kms->funcs is already assigned. This error propagates up, jumping to 
err_destroy_kms, and we call kms->funcs->destroy here. That delegates to 
msm_kms_destroy, which unconditionally calls destroy_workqueue(kms->wq) 
without checking if the workqueue was actually allocated.

[Severity: High]
Additionally, this is a pre-existing issue, but can this lead to a double 
free and list corruption in the DPU driver?

If dpu_kms_hw_init fails, it internally cleans up by calling 
_dpu_kms_hw_destroy. This invokes drm_atomic_private_obj_fini to remove 
the private object from its list via list_del. The error propagates to 
msm_drm_kms_init, we jump to err_destroy_kms, and execute this destroy 
callback. This resolves to dpu_kms_destroy, which calls 
_dpu_kms_hw_destroy a second time and executes list_del on a poisoned 
list head.

> +
>  	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912-fd-kms-fix-smmu-v3-0-a7ddc6fe2032@oss.qualcomm.com?part=3