Re: [PATCH v2 3/6] drm/msm: release scanout framebuffers only after a vblank

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Fatal NULL pointer dereference in msm_kms_fini_fb_unpin() when handling initialization errors if msm_kms_init_fb_unpin() was not completed.
- [High] Deferred framebuffer unpin logic creates a TOCTOU race for shared framebuffers, leading to premature synchronous unpin and IOMMU faults.
--

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

drm/msm: release scanout framebuffers only after a vblank

This commit defers the release of scanout framebuffers until after a vblank
has occurred by queuing the unpin work via a vblank worker. It prevents
hardware from fetching unmapped memory by ensuring the new configuration is
fully latched before detaching the VMA.

> diff --git a/drivers/gpu/drm/msm/msm_fb.c b/drivers/gpu/drm/msm/msm_fb.c
> index dfe040d5260bc..2e317bec9fbbb 100644
> --- a/drivers/gpu/drm/msm/msm_fb.c
> +++ b/drivers/gpu/drm/msm/msm_fb.c
[ ... ]
> @@ -144,6 +145,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))
> +		goto out;
> +
>  	for (i = 0; i < n; i++) {
>  		msm_gem_unpin_iova(fb->obj[i], vm);
>  		msm_gem_vma_put(fb->obj[i]);

[Severity: High]
Does this deferred unpin logic handle shared framebuffers safely?

If a framebuffer is shared across multiple CRTCs and an atomic commit removes
it from all of them simultaneously, msm_framebuffer_cleanup() is called
sequentially for each plane. The last plane processed drops prepare_count to
zero and funnels the unpin to a single CRTC.

If that final CRTC happens to be disabled (where crtc->state->active is false
in msm_crtc_queue_fb_unpin()), it returns false, causing an immediate
synchronous unpin here. This could fault any other active CRTCs that haven't
reached their vblank and are still actively scanning out the old frame.

Even if active, could the last processed CRTC reach its vblank and unpin the
buffer before slower CRTCs finish their frames?

[ ... ]
> diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
> index e5d0ea6294484..6a857ad3464c4 100644
> --- a/drivers/gpu/drm/msm/msm_kms.c
> +++ b/drivers/gpu/drm/msm/msm_kms.c
[ ... ]
> +void msm_kms_fini_fb_unpin(struct drm_device *dev)
> +{
> +	struct msm_drm_private *priv = dev->dev_private;
> +	struct msm_kms *kms = priv->kms;
> +	struct drm_crtc *crtc;
> +
> +	drm_for_each_crtc(crtc, dev) {
> +		unsigned int idx = drm_crtc_index(crtc);
> +
> +		if (idx >= ARRAY_SIZE(kms->fb_unpin))
> +			continue;
> +
> +		drm_vblank_work_cancel_sync(&kms->fb_unpin[idx].vblank_work);

[Severity: High]
Can this cause a NULL pointer dereference on initialization error paths?

If msm_drm_kms_init() fails after CRTCs are created but before
msm_kms_init_fb_unpin() succeeds (for example, if drm_vblank_init() fails),
the error path calls msm_drm_kms_uninit(), which unconditionally calls
msm_kms_fini_fb_unpin().

Because the vblank_work structs were zero-allocated and never initialized,
drm_vblank_work_cancel_sync() will dereference a NULL work->vblank:

drivers/gpu/drm/drm_vblank_work.c:drm_vblank_work_cancel_sync() {
    struct drm_vblank_crtc *vblank = work->vblank;
    struct drm_device *dev = vblank->dev;
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-fd-kms-fix-smmu-v2-0-9391815742a8@oss.qualcomm.com?part=3