Re: [PATCH v3 04/14] drm/msm: order the display snapshot sources around its worker

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/msm: unsynchronized fault handler clear in msm_drm_kms_uninit()
--

commit a95d27af7e4b3b82df2fddca01a3fd982e834f6d
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

drm/msm: order the display snapshot sources around its worker

This commit reorders the registration and teardown of the display IOMMU fault
handler and the display snapshot worker in msm_drm_kms_init() and
msm_drm_kms_uninit() so that snapshot sources are started after KMS setup and
stopped before the snapshot worker is destroyed.

> diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
> index f3e39c3907a93..a94d2888a21e5 100644
> --- a/drivers/gpu/drm/msm/msm_kms.c
> +++ b/drivers/gpu/drm/msm/msm_kms.c

[ ... ]

> @@ -245,6 +241,8 @@ void msm_drm_kms_uninit(struct device *dev)
>  
>  	BUG_ON(!kms);
>  
> +	msm_mmu_set_fault_handler(to_msm_vm(kms->vm)->mmu, kms, NULL);
> +
>  	/* We must cancel and cleanup any pending vblank enable/disable
>  	 * work before msm_irq_uninstall() to avoid work re-enabling an
>  	 * irq after uninstall has disabled it.
> @@ -256,12 +254,12 @@ void msm_drm_kms_uninit(struct device *dev)
>  
>  	drm_kms_helper_poll_fini(ddev);
>  
> -	msm_disp_snapshot_destroy(ddev);
> -
>  	pm_runtime_get_sync(dev);
>  	msm_irq_uninstall(ddev);
>  	pm_runtime_put_sync(dev);
>  
> +	msm_disp_snapshot_destroy(ddev);

[Severity: High]
Can clearing the fault handler in msm_drm_kms_uninit() still race with an
in-flight display IOMMU fault and cause a NULL function pointer call or a
use-after-free on kms->dump_worker?

In drivers/gpu/drm/msm/msm_mmu.h, msm_mmu_set_fault_handler() performs plain
unlocked stores:

static inline void msm_mmu_set_fault_handler(struct msm_mmu *mmu, void *arg,
		int (*handler)(void *arg, unsigned long iova, int flags, void *data))
{
	mmu->arg = arg;
	mmu->handler = handler;
}

And in drivers/gpu/drm/msm/msm_iommu.c, msm_disp_fault_handler() checks and
calls iommu->base.handler without locking or READ_ONCE():

static int msm_disp_fault_handler(struct iommu_domain *domain, struct device *dev,
				  unsigned long iova, int flags, void *arg)
{
	struct msm_iommu *iommu = arg;

	if (iommu->base.handler)
		return iommu->base.handler(iommu->base.arg, iova, flags, NULL);

	return -ENOSYS;
}

First, without READ_ONCE() / WRITE_ONCE() or a lock, can the compiler reload
iommu->base.handler after the NULL check in msm_disp_fault_handler() while
msm_drm_kms_uninit() clears it, calling NULL?

Second, if a display SMMU context fault (for example, via the threaded IRQ
handler qcom_smmu_context_fault() -> report_iommu_fault() ->
msm_disp_fault_handler() -> msm_kms_fault_handler()) is already running when
msm_drm_kms_uninit() sets mmu->handler to NULL, nothing between
msm_mmu_set_fault_handler() and msm_disp_snapshot_destroy() waits for that
in-flight fault handler to complete (msm_irq_uninstall() only frees kms->irq,
while the SMMU domain and its fault IRQ are only torn down later in
kms->funcs->destroy(kms)).

If msm_kms_fault_handler() is preempted or running on another CPU, couldn't it
call msm_disp_snapshot_state() -> kthread_queue_work(kms->dump_worker,
&kms->dump_work) concurrently with or after msm_disp_snapshot_destroy() calls
kthread_destroy_worker(kms->dump_worker)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261024-msm-kms-destroy-fixes-v3-0-66a30120a5ea@oss.qualcomm.com?part=4