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 <[email protected]> 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-66a30120a...@oss.qualcomm.com?part=4
