Re: [PATCH 2/2] drm/msm: release scanout framebuffers only after a vblank

[sashiko-bot@kernel.org]

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 <dmitry.baryshkov@oss.qualcomm.com>
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/20260903-fd-kms-fix-smmu-v1-0-608d02491666@oss.qualcomm.com?part=2