Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues: - [Critical] Unpin worker accesses freed `kms->vm` during teardown because `destroy_workqueue(kms->wq)` is called after `kms->vm` is destroyed. - [Critical] Unconditional cleanup of uninitialized `drm_vblank_work` structs causes a NULL pointer dereference in `msm_kms_destroy` on systems with fewer than MAX_CRTCS (8). - [High] Teardown race in `msm_kms_destroy` causes a spurious WARN_ON because flip works are cleaned up before the workqueue is flushed. - [High] Framebuffers and their pinned GPU memory leak permanently when a CRTC is disabled because `drm_vblank_work_schedule()` returns 0 when the CRTC is off, bypassing the manual flip work commit. Pre-existing issues: - [High] Concurrent prepare_fb and cleanup_fb of a shared framebuffer causes `msm_fb->iova` to be erroneously zeroed out, leading to IOMMU faults. -- commit e00895b91abad99d555ba6e2dfe2858e778599be Author: Dmitry Baryshkov <[email protected]> Subject: drm/msm: release scanout framebuffers only after a vblank This commit delays the release of scanout framebuffers until after a vblank event completes to prevent the display hardware from fetching unmapped memory during an atomic commit. It defers the unpin operations to a workqueue running from the vblank irq context. > diff --git a/drivers/gpu/drm/msm/msm_fb.c b/drivers/gpu/drm/msm/msm_fb.c > index 934337202afd4..0865ecce77de2 100644 > --- a/drivers/gpu/drm/msm/msm_fb.c > +++ b/drivers/gpu/drm/msm/msm_fb.c [ ... ] > @@ -127,6 +128,9 @@ void msm_framebuffer_cleanup(struct drm_framebuffer *fb, > bool needed_dirtyfb) > > memset(msm_fb->iova, 0, sizeof(msm_fb->iova)); > > + if (crtc && msm_crtc_queue_fb_unpin(crtc, fb)) > + return; > + [Severity: High] This is a pre-existing issue, but can a concurrent atomic commit cause IOMMU faults here? The lockless gap between atomic_dec_return(&msm_fb->prepare_count) (which occurs earlier in this function) and the memset(msm_fb->iova, 0, ...) creates a time-of-check to time-of-use race. If msm_framebuffer_prepare() runs concurrently on the same framebuffer, it can increment prepare_count and pin the valid IOVAs before this unprotected memset executes, overwriting the newly pinned, valid IOVAs with zeros. The display hardware is then programmed with IOVA 0, causing it to read unmapped memory. > diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c > index e5d0ea6294484..bf56fbe99a346 100644 > --- a/drivers/gpu/drm/msm/msm_kms.c > +++ b/drivers/gpu/drm/msm/msm_kms.c [ ... ] > +void msm_kms_fb_unpin_worker(struct drm_flip_work *work, void *val) > +{ > + struct drm_framebuffer *fb = val; > + struct msm_drm_private *priv = fb->dev->dev_private; > + struct drm_gpuvm *vm = priv->kms->vm; > + int i, n = fb->format->num_planes; > + > + for (i = 0; i < n; i++) { > + msm_gem_unpin_iova(fb->obj[i], vm); [Severity: Critical] Can this unpin worker dereference a freed vm pointer during module unload? During driver teardown, mdp4_kms_destroy() and mdp5_kms_destroy() drop the final reference and free the VM. Following that, msm_kms_destroy() is called which invokes destroy_workqueue(kms->wq). Because the workqueue is drained after the VM is already destroyed, any pending unpin tasks will execute, read the freed priv->kms->vm pointer, and cause a fatal use-after-free crash when attempting to unpin the memory. [ ... ] > +bool msm_crtc_queue_fb_unpin(struct drm_crtc *crtc, struct drm_framebuffer > *fb) > +{ > + struct msm_drm_private *priv = crtc->dev->dev_private; > + struct msm_kms *kms = priv->kms; > + unsigned int idx = drm_crtc_index(crtc); > + > + if (!kms || idx >= ARRAY_SIZE(kms->fb_unpin)) > + return false; > + > + drm_framebuffer_get(fb); > + drm_flip_work_queue(&kms->fb_unpin[idx].work, fb); > + > + /* no vblank to wait for: the crtc is off, so it is not fetching */ > + if (drm_vblank_work_schedule(&kms->fb_unpin[idx].vblank_work, > + drm_crtc_vblank_count(crtc) + 1, true) < 0) > + drm_flip_work_commit(&kms->fb_unpin[idx].work, kms->wq); > + > + return true; > +} [Severity: High] Does this code permanently leak framebuffers when a CRTC is disabled? The fallback check assumes drm_vblank_work_schedule() returns a negative error code when the CRTC is off. However, looking at the drm core: drm_vblank_work_schedule() { ... spin_lock(&dev->vbl_lock); inmodeset = vblank->inmodeset; spin_unlock(&dev->vbl_lock); if (inmodeset) goto out; // returns 0 ... } Because drm_vblank_work_schedule() returns 0 instead of < 0 when vblank->inmodeset is true, the manual drm_flip_work_commit() fallback is bypassed. This leaves the unpin work permanently queued, leaking the framebuffer and its pinned GPU memory since no worker will be scheduled. > diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h > index f25b31e502d21..4b73132b5f6ea 100644 > --- a/drivers/gpu/drm/msm/msm_kms.h > +++ b/drivers/gpu/drm/msm/msm_kms.h [ ... ] > @@ -203,6 +227,11 @@ static inline void msm_kms_destroy(struct msm_kms *kms) > for (i = 0; i < ARRAY_SIZE(kms->pending_timers); i++) > msm_atomic_destroy_pending_timer(&kms->pending_timers[i]); > > + for (i = 0; i < ARRAY_SIZE(kms->fb_unpin); i++) { > + drm_vblank_work_cancel_sync(&kms->fb_unpin[i].vblank_work); > + drm_flip_work_cleanup(&kms->fb_unpin[i].work); > + } > + [Severity: Critical] Does this unconditional teardown loop cause a NULL pointer dereference on systems with fewer than MAX_CRTCS? msm_kms_init_fb_unpin() initializes vblank_work structures only for the actually registered CRTCs. If the hardware has fewer CRTCs than MAX_CRTCS (8), the remaining array entries are left zero-initialized. drm_vblank_work_cancel_sync() immediately dereferences work->vblank to get the device pointer, which will cause a fatal crash for those uninitialized entries. [Severity: High] Can this teardown order cause a spurious kernel warning? drm_flip_work_cleanup() expects the work->commited and work->queued lists to be empty and will trigger a WARN_ON if they are not. Because it is called here before destroy_workqueue(kms->wq) has flushed the pending tasks, the work items may still be actively queued. > destroy_workqueue(kms->wq); > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
