Re: [PATCH v5 02/10] drm/msm: fix framebuffer pin refcount leak on prepare failure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/msm/dpu: prepare_count underflow on writeback prepare failure
--

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

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

This commit unwinds framebuffer preparation state on failure in
msm_framebuffer_prepare(), unpinning any already-pinned planes, dropping
VMA references, zeroing iova[], and decrementing prepare_count and dirtyfb.

> diff --git a/drivers/gpu/drm/msm/msm_fb.c b/drivers/gpu/drm/msm/msm_fb.c
> index a38e5b3a1d1e9..792d47b4acc43 100644
> --- a/drivers/gpu/drm/msm/msm_fb.c
> +++ b/drivers/gpu/drm/msm/msm_fb.c
> @@ -80,6 +80,18 @@ void msm_framebuffer_describe(struct drm_framebuffer *fb, struct seq_file *m)
>  }
>  #endif
>  
> +static void msm_framebuffer_unpin_planes(struct drm_framebuffer *fb, int n)
> +{
> +	struct msm_drm_private *priv = fb->dev->dev_private;
> +	struct drm_gpuvm *vm = priv->kms->vm;
> +	int i;
> +
> +	for (i = 0; i < n; i++) {
> +		msm_gem_unpin_iova(fb->obj[i], vm);
> +		msm_gem_vma_put(fb->obj[i]);
> +	}
> +}
> +
>  /* prepare/pin all the fb's bo's for scanout.
>   */
>  int msm_framebuffer_prepare(struct drm_framebuffer *fb, bool needs_dirtyfb)
> @@ -102,8 +114,17 @@ 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)
> +		if (ret) {
> +			msm_gem_vma_put(fb->obj[i]);
> +			msm_framebuffer_unpin_planes(fb, i);
> +			memset(msm_fb->iova, 0, sizeof(msm_fb->iova));
> +			msm_fb->prepare_count--;
> +
> +			if (needs_dirtyfb)
> +				refcount_dec(&msm_fb->dirtyfb);
> +
>  			return ret;
> +		}

[Severity: High]
Can this underflow msm_fb->prepare_count when msm_framebuffer_prepare() is
called from the DPU writeback path?

During atomic commit preparation, drm_atomic_helper_prepare_planes() calls:

  drm_writeback_prepare_job()
    dpu_wb_conn_prepare_job()
      dpu_encoder_prepare_wb_job()
        dpu_encoder_phys_wb_prepare_wb_job()

In drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c,
dpu_encoder_phys_wb_prepare_wb_job() drops the error returned by
msm_framebuffer_prepare():

	wb_enc->wb_job = job;
	wb_enc->wb_conn = job->connector;

	wb_cfg = &wb_enc->wb_cfg;

	memset(wb_cfg, 0, sizeof(struct dpu_hw_wb_cfg));

	ret = msm_framebuffer_prepare(job->fb, false);
	if (ret) {
		DPU_ERROR("prep fb failed, %d\n", ret);
		return;
	}

And in drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c,
dpu_wb_conn_prepare_job() unconditionally returns 0:

	dpu_encoder_prepare_wb_job(dpu_wb_conn->wb_enc, job);

	return 0;

Because dpu_wb_conn_prepare_job() returns 0, drm_writeback_prepare_job()
sets job->prepared = true, and the commit proceeds with wb_cfg->dest.plane_addr[]
left at 0.

When drm_writeback_cleanup_job() later runs (either on commit completion or
if a subsequent plane's prepare_fb() fails), job->prepared is true, so it
calls:

  dpu_wb_conn_cleanup_job()
    dpu_encoder_cleanup_wb_job()
      dpu_encoder_phys_wb_cleanup_wb_job()
        msm_framebuffer_cleanup(job->fb, false)

Since msm_framebuffer_prepare() now decrements msm_fb->prepare_count back to
0 on error, won't the subsequent call to msm_framebuffer_cleanup() decrement
msm_fb->prepare_count a second time, underflowing it to UINT_MAX (or
prematurely unpinning the framebuffer if another plane prepared it in the
meantime)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261024-fd-kms-fix-smmu-v5-0-a5b0b284aead@oss.qualcomm.com?part=2