[PATCH 0/2] drm/msm: fix SMMU fault dumps

[PATCH 0/2] drm/msm: fix SMMU fault dumps

[Dmitry Baryshkov]

In several cases the drm/msm can cause an SMMU fault on modesetting (due
to the display controller still scanning the BO which is being
unmapped). Fix two cases which I stumbled upon.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
Dmitry Baryshkov (2):
      drm/msm: fix framebuffer pin refcount leak on prepare failure
      drm/msm: release scanout framebuffers only after a vblank

 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c           |  2 +-
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c    |  2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c          |  3 +-
 drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c          |  2 +-
 drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c         |  2 +-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c          |  2 +-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c         |  2 +-
 drivers/gpu/drm/msm/msm_drv.h                      |  3 +-
 drivers/gpu/drm/msm/msm_fb.c                       | 26 ++++++-
 drivers/gpu/drm/msm/msm_kms.c                      | 88 ++++++++++++++++++++++
 drivers/gpu/drm/msm/msm_kms.h                      | 29 +++++++
 11 files changed, 150 insertions(+), 11 deletions(-)
---
base-commit: 140b13475302601368c0cf4e193e66126a49feb3
change-id: 20260902-fd-kms-fix-smmu-2d4baaf460b0
prerequisite-change-id: 20260202-msm-ci-sm8650-bd3933b73635:v1

Best regards,
--  
With best wishes
Dmitry

[PATCH 1/2] drm/msm: fix framebuffer pin refcount leak on prepare failure

[Dmitry Baryshkov]

msm_framebuffer_prepare() bumps prepare_count before pinning, but returns
straight out of the pin loop on error, leaking the count, the
msm_gem_vma_get() reference and any planes already pinned.
drm_atomic_helper_prepare_planes() does not call cleanup_fb() for the
plane whose prepare_fb() failed, so nothing ever drops it.

Since commit 8ac37c88f991 ("drm/msm: Refcount framebuffer pins") a
prepare which finds the count already non-zero returns early, assuming
iova[] is populated.  With the count stuck, every later prepare of that
framebuffer reports success while iova[] is still zero, and DPU scans out
from a NULL base address:

  arm-smmu 15000000.iommu: Unhandled context fault: fsr=0x402,
      iova=0x00000100, fsynr=0x3e0023, cbfrsynra=0x1c00, cb=11

Unwind properly on failure instead.

Fixes: 8ac37c88f991 ("drm/msm: Refcount framebuffer pins")
Assisted-by: LLM
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/msm_fb.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_fb.c b/drivers/gpu/drm/msm/msm_fb.c
index 60c108d35d2a..934337202afd 100644
--- a/drivers/gpu/drm/msm/msm_fb.c
+++ b/drivers/gpu/drm/msm/msm_fb.c
@@ -89,11 +89,27 @@ int msm_framebuffer_prepare(struct drm_framebuffer *fb, bool needs_dirtyfb)
 		ret = msm_gem_get_and_pin_iova(fb->obj[i], vm, &msm_fb->iova[i]);
 		drm_dbg_state(fb->dev, "FB[%u]: iova[%d]: %08llx (%d)\n",
 			      fb->base.id, i, msm_fb->iova[i], ret);
-		if (ret)
-			return ret;
+		if (ret) {
+			msm_gem_vma_put(fb->obj[i]);
+			goto unwind;
+		}
 	}
 
 	return 0;
+
+unwind:
+	while (i--) {
+		msm_gem_unpin_iova(fb->obj[i], vm);
+		msm_gem_vma_put(fb->obj[i]);
+	}
+
+	memset(msm_fb->iova, 0, sizeof(msm_fb->iova));
+
+	atomic_dec(&msm_fb->prepare_count);
+	if (needs_dirtyfb)
+		refcount_dec(&msm_fb->dirtyfb);
+
+	return ret;
 }
 
 void msm_framebuffer_cleanup(struct drm_framebuffer *fb, bool needed_dirtyfb)

-- 
2.47.3

Re: [PATCH 1/2] drm/msm: fix framebuffer pin refcount leak on prepare failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Unwinding a failed framebuffer prepare while a concurrent prepare exists causes a double-unpin and refcount underflow.

Pre-existing issues:
- [High] Concurrent `msm_framebuffer_prepare` calls for the same framebuffer can race due to lockless `prepare_count`, leading to uninitialized `iova` access and IOMMU faults.
--

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

drm/msm: fix framebuffer pin refcount leak on prepare failure

This commit addresses a resource leak in msm_framebuffer_prepare() by
unwinding successfully pinned planes and releasing the vma reference when
a subsequent plane fails to pin.

[Severity: High]
This is a pre-existing issue, but can concurrent msm_framebuffer_prepare()
calls for the same framebuffer race due to the lockless prepare_count check?

Looking at msm_framebuffer_prepare() in drivers/gpu/drm/msm/msm_fb.c:

	if (atomic_inc_return(&msm_fb->prepare_count) > 1)
		return 0;

If userspace issues concurrent atomic commits for different CRTCs sharing the
same framebuffer, Thread A could increment prepare_count to 1 and start
pinning planes. Thread B could concurrently increment prepare_count to 2, hit
this lockless fast-path, and return success immediately before Thread A has
finished populating the iova array.

Thread B's commit might then proceed to scanout using uninitialized memory
addresses (zeros) for DMA, which could cause an IOMMU unhandled context fault.

Could msm_framebuffer_cleanup() also race in a similar way?

	if (atomic_dec_return(&msm_fb->prepare_count))
		return;

	memset(msm_fb->iova, 0, sizeof(msm_fb->iova));

This lockless memset could race with a concurrent prepare_fb from Thread A,
overwriting a newly pinned IOVA with zero.

> diff --git a/drivers/gpu/drm/msm/msm_fb.c b/drivers/gpu/drm/msm/msm_fb.c
> index 60c108d35d2a1..934337202afd4 100644
> --- a/drivers/gpu/drm/msm/msm_fb.c
> +++ b/drivers/gpu/drm/msm/msm_fb.c
> @@ -89,11 +89,27 @@ int msm_framebuffer_prepare(struct drm_framebuffer *fb, bool needs_dirtyfb)
[ ... ]
>  	return 0;
> +
> +unwind:
> +	while (i--) {
> +		msm_gem_unpin_iova(fb->obj[i], vm);
> +		msm_gem_vma_put(fb->obj[i]);
> +	}
> +
> +	memset(msm_fb->iova, 0, sizeof(msm_fb->iova));
> +
> +	atomic_dec(&msm_fb->prepare_count);

[Severity: High]
Does this unwind path create a double-unpin if there is a concurrent
prepare that succeeded via the fast-path?

If Thread A and B enter msm_framebuffer_prepare() concurrently, Thread A
increments prepare_count to 1 and begins pinning. Thread B increments it
to 2 and returns success.

If Thread A encounters an error, it enters this newly added unwind block,
unpins the planes it successfully pinned, and decrements prepare_count from
2 to 1.

When Thread B's commit succeeds and later calls msm_framebuffer_cleanup(),
it will decrement prepare_count from 1 to 0. Since it reaches 0, Thread B
will unconditionally unpin all planes:

drivers/gpu/drm/msm/msm_fb.c:msm_framebuffer_cleanup() {
    ...
	if (atomic_dec_return(&msm_fb->prepare_count))
		return;

	memset(msm_fb->iova, 0, sizeof(msm_fb->iova));

	if (crtc && msm_crtc_queue_fb_unpin(crtc, fb))
		return;

	for (i = 0; i < n; i++) {
		msm_gem_unpin_iova(fb->obj[i], vm);
		msm_gem_vma_put(fb->obj[i]);
	}
}

Because Thread A already unpinned the planes it originally pinned, Thread
B's cleanup would result in a double-unpin, causing vma_ref and pin_count
underflows, and potentially a use-after-free of GEM memory.

> +	if (needs_dirtyfb)
> +		refcount_dec(&msm_fb->dirtyfb);
> +
> +	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903-fd-kms-fix-smmu-v1-0-608d02491666@oss.qualcomm.com?part=1

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

[Dmitry Baryshkov]

msm_framebuffer_cleanup() releases a framebuffer as soon as
drm_atomic_helper_cleanup_planes() runs.  msm_atomic_commit_tail() waits
only for ->wait_flush() before that, which for video mode waits for
CTL_FLUSH to read back zero, ie. for the new configuration to be latched;
the frame in flight with the old one is still being fetched.  Since
commit 111fdd2198e6 ("drm/msm: drm_gpuvm conversion") the unpin also
detaches the vma, so the display is left reading unmapped memory:

  arm-smmu 15000000.iommu: Unhandled context fault: fsr=0x402,
      iova=0x007eb100, fsynr=0x3f0023, cbfrsynra=0xc20, cb=27

Defer the release: hand the retired framebuffer to the crtc and drop the
pin and the vma reference from a drm_flip_work committed from the vblank
irq, as mdp4 and mdp5 already do for their LM cursor buffers.  A vblank
reference is held while work is outstanding; a crtc with no vblank is not
fetching, so it releases immediately.

Fixes: 111fdd2198e6 ("drm/msm: drm_gpuvm conversion")
Assisted-by: LLM
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c           |  2 +-
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c    |  2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c          |  3 +-
 drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c          |  2 +-
 drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c         |  2 +-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c          |  2 +-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c         |  2 +-
 drivers/gpu/drm/msm/msm_drv.h                      |  3 +-
 drivers/gpu/drm/msm/msm_fb.c                       |  6 +-
 drivers/gpu/drm/msm/msm_kms.c                      | 88 ++++++++++++++++++++++
 drivers/gpu/drm/msm/msm_kms.h                      | 29 +++++++
 11 files changed, 132 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 42d0a529b4d5..bf593020e8e4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1213,7 +1213,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
 	}
 
 	/* Disable/save vblank irq handling */
-	drm_crtc_vblank_off(crtc);
+	msm_crtc_vblank_off(crtc);
 
 	drm_for_each_encoder_mask(encoder, crtc->dev,
 				  old_crtc_state->encoder_mask) {
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
index 22433bfbea1e..5db33e49c345 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
@@ -615,7 +615,7 @@ static void dpu_encoder_phys_wb_cleanup_wb_job(struct dpu_encoder_phys *phys_enc
 	if (!job->fb)
 		return;
 
-	msm_framebuffer_cleanup(job->fb, false);
+	msm_framebuffer_cleanup(job->fb, NULL, false);
 	wb_enc->wb_job = NULL;
 	wb_enc->wb_conn = NULL;
 }
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 7b92082d35a6..0e986b533bf0 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -684,7 +684,8 @@ static void dpu_plane_cleanup_fb(struct drm_plane *plane,
 
 	DPU_DEBUG_PLANE(pdpu, "FB[%u]\n", old_state->fb->base.id);
 
-	msm_framebuffer_cleanup(old_state->fb, old_pstate->needs_dirtyfb);
+	msm_framebuffer_cleanup(old_state->fb, old_state->crtc,
+				old_pstate->needs_dirtyfb);
 }
 
 static int dpu_plane_check_inline_rotation(struct dpu_plane *pdpu,
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c
index 57dfce58450b..195ee6b4a0c6 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c
@@ -267,7 +267,7 @@ static void mdp4_crtc_atomic_disable(struct drm_crtc *crtc,
 		return;
 
 	/* Disable/save vblank irq handling before power is disabled */
-	drm_crtc_vblank_off(crtc);
+	msm_crtc_vblank_off(crtc);
 
 	mdp_irq_unregister(&mdp4_kms->base, &mdp4_crtc->err);
 	mdp4_disable(mdp4_kms);
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c
index 9459f70ce0ba..5f669a02d798 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c
@@ -97,7 +97,7 @@ static void mdp4_plane_cleanup_fb(struct drm_plane *plane,
 		return;
 
 	DBG("%s: cleanup: FB[%u]", mdp4_plane->name, fb->base.id);
-	msm_framebuffer_cleanup(fb, false);
+	msm_framebuffer_cleanup(fb, old_state->crtc, false);
 }
 
 
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
index 4c4a897fc1ee..547f6fdb83d5 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
@@ -499,7 +499,7 @@ static void mdp5_crtc_atomic_disable(struct drm_crtc *crtc,
 		return;
 
 	/* Disable/save vblank irq handling before power is disabled */
-	drm_crtc_vblank_off(crtc);
+	msm_crtc_vblank_off(crtc);
 
 	if (mdp5_cstate->cmd_mode)
 		mdp_irq_unregister(&mdp5_kms->base, &mdp5_crtc->pp_done);
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
index 841f444a8d68..dacb387d9bb6 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
@@ -155,7 +155,7 @@ static void mdp5_plane_cleanup_fb(struct drm_plane *plane,
 		return;
 
 	DBG("%s: cleanup: FB[%u]", plane->name, fb->base.id);
-	msm_framebuffer_cleanup(fb, needed_dirtyfb);
+	msm_framebuffer_cleanup(fb, old_state->crtc, needed_dirtyfb);
 }
 
 static int mdp5_plane_atomic_check_with_state(struct drm_crtc_state *crtc_state,
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index eb4bbae8557b..18a9728838fd 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -254,7 +254,8 @@ int msm_gem_prime_pin(struct drm_gem_object *obj);
 void msm_gem_prime_unpin(struct drm_gem_object *obj);
 
 int msm_framebuffer_prepare(struct drm_framebuffer *fb, bool needs_dirtyfb);
-void msm_framebuffer_cleanup(struct drm_framebuffer *fb, bool needed_dirtyfb);
+void msm_framebuffer_cleanup(struct drm_framebuffer *fb, struct drm_crtc *crtc,
+			     bool needed_dirtyfb);
 uint32_t msm_framebuffer_iova(struct drm_framebuffer *fb, int plane);
 struct drm_gem_object *msm_framebuffer_bo(struct drm_framebuffer *fb, int plane);
 const struct msm_format *msm_framebuffer_format(struct drm_framebuffer *fb);
diff --git a/drivers/gpu/drm/msm/msm_fb.c b/drivers/gpu/drm/msm/msm_fb.c
index 934337202afd..0865ecce77de 100644
--- a/drivers/gpu/drm/msm/msm_fb.c
+++ b/drivers/gpu/drm/msm/msm_fb.c
@@ -112,7 +112,8 @@ int msm_framebuffer_prepare(struct drm_framebuffer *fb, bool needs_dirtyfb)
 	return ret;
 }
 
-void msm_framebuffer_cleanup(struct drm_framebuffer *fb, bool needed_dirtyfb)
+void msm_framebuffer_cleanup(struct drm_framebuffer *fb, struct drm_crtc *crtc,
+			     bool needed_dirtyfb)
 {
 	struct msm_drm_private *priv = fb->dev->dev_private;
 	struct drm_gpuvm *vm = priv->kms->vm;
@@ -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;
+
 	for (i = 0; i < n; i++) {
 		msm_gem_unpin_iova(fb->obj[i], vm);
 		msm_gem_vma_put(fb->obj[i]);
diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
index e5d0ea629448..bf56fbe99a34 100644
--- a/drivers/gpu/drm/msm/msm_kms.c
+++ b/drivers/gpu/drm/msm/msm_kms.c
@@ -11,6 +11,7 @@
 #include <uapi/linux/sched/types.h>
 
 #include <drm/drm_drv.h>
+#include <drm/drm_framebuffer.h>
 #include <drm/drm_mode_config.h>
 #include <drm/drm_vblank.h>
 #include <drm/clients/drm_client_setup.h>
@@ -165,6 +166,89 @@ void msm_crtc_disable_vblank(struct drm_crtc *crtc)
 	vblank_ctrl_queue_work(priv, crtc, false);
 }
 
+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);
+		msm_gem_vma_put(fb->obj[i]);
+	}
+
+	drm_framebuffer_put(fb);
+}
+
+static void msm_kms_fb_unpin_vblank(struct kthread_work *work)
+{
+	struct msm_kms_fb_unpin *fb_unpin =
+		container_of(to_drm_vblank_work(work), struct msm_kms_fb_unpin,
+			     vblank_work);
+
+	drm_flip_work_commit(&fb_unpin->work, fb_unpin->kms->wq);
+}
+
+int msm_kms_init_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))
+			return -EINVAL;
+
+		drm_vblank_work_init(&kms->fb_unpin[idx].vblank_work, crtc,
+				     msm_kms_fb_unpin_vblank);
+	}
+
+	return 0;
+}
+
+/*
+ * drm_crtc_vblank_off() drops pending vblank works without running them, so
+ * flush the retired framebuffers first.  The encoder is disabled before the
+ * crtc, so the hardware has already stopped fetching by this point.
+ */
+void msm_crtc_vblank_off(struct drm_crtc *crtc)
+{
+	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)) {
+		drm_vblank_work_cancel_sync(&kms->fb_unpin[idx].vblank_work);
+		drm_flip_work_commit(&kms->fb_unpin[idx].work, kms->wq);
+	}
+
+	drm_crtc_vblank_off(crtc);
+}
+
+/* Returns false if the caller should release @fb itself */
+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;
+}
+
 static int msm_kms_fault_handler(void *arg, unsigned long iova, int flags, void *data)
 {
 	struct msm_kms *kms = arg;
@@ -323,6 +407,10 @@ int msm_drm_kms_init(struct device *dev, const struct drm_driver *drv)
 		goto err_msm_uninit;
 	}
 
+	ret = msm_kms_init_fb_unpin(ddev);
+	if (ret)
+		goto err_msm_uninit;
+
 	pm_runtime_get_sync(dev);
 	ret = msm_irq_install(ddev, kms->irq);
 	pm_runtime_put_sync(dev);
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index f25b31e502d2..4b73132b5f6e 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -11,6 +11,9 @@
 #include <linux/clk.h>
 #include <linux/regulator/consumer.h>
 
+#include <drm/drm_flip_work.h>
+#include <drm/drm_vblank_work.h>
+
 #include "msm_drv.h"
 
 #ifdef CONFIG_DRM_MSM_KMS
@@ -135,6 +138,16 @@ struct msm_drm_thread {
 	struct kthread_worker *worker;
 };
 
+/* Retired scanout framebuffers, released after the crtc's next vblank */
+struct msm_kms_fb_unpin {
+	struct drm_flip_work work;
+	struct drm_vblank_work vblank_work;
+	struct msm_kms *kms;
+};
+
+void msm_kms_fb_unpin_worker(struct drm_flip_work *work, void *val);
+int msm_kms_init_fb_unpin(struct drm_device *dev);
+
 struct msm_kms {
 	const struct msm_kms_funcs *funcs;
 	struct drm_device *dev;
@@ -170,8 +183,13 @@ struct msm_kms {
 
 	struct workqueue_struct *wq;
 	struct msm_drm_thread event_thread[MAX_CRTCS];
+
+	struct msm_kms_fb_unpin fb_unpin[MAX_CRTCS];
 };
 
+bool msm_crtc_queue_fb_unpin(struct drm_crtc *crtc, struct drm_framebuffer *fb);
+void msm_crtc_vblank_off(struct drm_crtc *crtc);
+
 static inline int msm_kms_init(struct msm_kms *kms,
 		const struct msm_kms_funcs *funcs)
 {
@@ -193,6 +211,12 @@ static inline int msm_kms_init(struct msm_kms *kms,
 		}
 	}
 
+	for (i = 0; i < ARRAY_SIZE(kms->fb_unpin); i++) {
+		kms->fb_unpin[i].kms = kms;
+		drm_flip_work_init(&kms->fb_unpin[i].work, "fb unpin",
+				   msm_kms_fb_unpin_worker);
+	}
+
 	return 0;
 }
 
@@ -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);
+	}
+
 	destroy_workqueue(kms->wq);
 }
 

-- 
2.47.3

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

[sashiko-bot]

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