Re: [PATCH v3 4/8] drm/msm: release scanout framebuffers only after a vblank

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Concurrent execution of msm_crtc_queue_fb_unpin() and msm_crtc_vblank_off() causes a use-after-free of the unpin work item.
--

commit d546c6b2b93655071c146a3c50f9a3cb4baa8f1b
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Subject: drm/msm: release scanout framebuffers only after a vblank

This commit modifies the framebuffer cleanup process to defer unpinning
until after a vblank by scheduling a drm_vblank_work item. It also
provides a mechanism to release these works directly if the CRTC is
switched off and no further vblanks will arrive.

> diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
> index f3e39c3907a93..4582ba040f6ff 100644
> --- a/drivers/gpu/drm/msm/msm_kms.c
> +++ b/drivers/gpu/drm/msm/msm_kms.c
[ ... ]
> @@ -165,6 +167,119 @@ void msm_crtc_disable_vblank(struct drm_crtc *crtc)
>  	vblank_ctrl_queue_work(priv, crtc, false);
>  }
>  
> +struct msm_fb_unpin_work {
> +	struct drm_vblank_work base;
> +	struct list_head node;
> +	struct msm_kms_fb_unpin *pending;
> +	struct drm_framebuffer *fb;
> +};
> +
> +static void msm_kms_fb_unpin_release(struct msm_fb_unpin_work *unpin)
> +{
> +	msm_framebuffer_unpin(unpin->fb);
> +	drm_framebuffer_put(unpin->fb);
> +	kfree(unpin);
> +}
[ ... ]
> +void msm_crtc_vblank_off(struct drm_crtc *crtc)
> +{
[ ... ]
> +	for (;;) {
> +		spin_lock(&pending->lock);
> +		unpin = list_first_entry_or_null(&pending->fbs, typeof(*unpin),
> +						 node);
> +		if (unpin)
> +			list_del_init(&unpin->node);
> +		spin_unlock(&pending->lock);
> +
> +		if (!unpin)
> +			break;
> +
> +		drm_vblank_work_cancel_sync(&unpin->base);
> +		msm_kms_fb_unpin_release(unpin);

[Severity: High]
Is it possible for a concurrent msm_crtc_queue_fb_unpin() call to still be
holding the unpin pointer when this releases it?

If an atomic commit calls msm_crtc_queue_fb_unpin(), and a concurrent CRTC
disable commit concurrently calls msm_crtc_vblank_off() here, could the newly
added unpin object be popped and freed before it has been scheduled?

> +	}
> +
> +out:
> +	drm_crtc_vblank_off(crtc);
> +}
> +
> +bool msm_crtc_queue_fb_unpin(struct drm_crtc *crtc, struct drm_framebuffer *fb)
> +{
[ ... ]
> +	drm_vblank_work_init(&unpin->base, crtc, msm_kms_fb_unpin_work);
> +
> +	spin_lock(&pending->lock);
> +	list_add_tail(&unpin->node, &pending->fbs);
> +	spin_unlock(&pending->lock);
> +
> +	if (drm_vblank_work_schedule(&unpin->base,
> +				     drm_crtc_vblank_count(crtc) + 1, true) != 1) {

[Severity: High]
Can this sequence trigger a use-after-free or double-free?

Since the unpin object is published to pending->fbs before calling
drm_vblank_work_schedule(), and the lock is dropped, a concurrent
msm_crtc_vblank_off() could potentially grab the object from the list and
free it.

If that happens, does this code then attempt to access the freed unpin->base,
and potentially double-free it with the kfree(unpin) below if scheduling
fails?

> +		spin_lock(&pending->lock);
> +		list_del_init(&unpin->node);
> +		spin_unlock(&pending->lock);
> +
> +		drm_framebuffer_put(fb);
> +		kfree(unpin);
> +
> +		return false;
> +	}
> +
> +	return true;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912-fd-kms-fix-smmu-v3-0-a7ddc6fe2032@oss.qualcomm.com?part=4