Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 25 Nov 2022 11:34:13 -0500
From:   =?utf-8?B?TsOtY29sYXMgRi4gUi4gQS4=?= Prado 
        <nfraprado@collabora.com>
To:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>
Cc:     Chun-Kuang Hu <chunkuang.hu@kernel.org>, kernel@collabora.com,
        "Nancy . Lin" <nancy.lin@mediatek.com>, CK Hu <ck.hu@mediatek.com>,
        Daniel Kurtz <djkurtz@chromium.org>,
        Daniel Vetter <daniel@ffwll.ch>,
        David Airlie <airlied@gmail.com>,
        Mao Huang <littlecvr@chromium.org>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        YT Shen <yt.shen@mediatek.com>,
        dri-devel@lists.freedesktop.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-mediatek@lists.infradead.org
Subject: Re: [PATCH v2] drm/mediatek: Clean dangling pointer on bind error
 path
Message-ID: <20221125163413.jwutjr5uxey6b32o@notapiano>
References: <20221122143949.3493104-1-nfraprado@collabora.com>
 <a67594cf-eb7f-873f-1c11-ccb4317b6bdf@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <a67594cf-eb7f-873f-1c11-ccb4317b6bdf@collabora.com>
Precedence: bulk

On Wed, Nov 23, 2022 at 10:15:25AM +0100, AngeloGioacchino Del Regno wrote:
> Il 22/11/22 15:39, N?colas F. R. A. Prado ha scritto:
> > mtk_drm_bind() can fail, in which case drm_dev_put() is called,
> > destroying the drm_device object. However a pointer to it was still
> > being held in the private object, and that pointer would be passed along
> > to DRM in mtk_drm_sys_prepare() if a suspend were triggered at that
> > point, resulting in a panic. Clean the pointer when destroying the
> > object in the error path to prevent this from happening.
> > 
> > Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.")
> > Signed-off-by: N?colas F. R. A. Prado <nfraprado@collabora.com>
> > 
> > ---
> > 
> > Changes in v2:
> > - Added Fixes tag
> > 
> >   drivers/gpu/drm/mediatek/mtk_drm_drv.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > index 39a42dc8fb85..a21ff1b3258c 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > @@ -514,6 +514,7 @@ static int mtk_drm_bind(struct device *dev)
> >   err_deinit:
> >   	mtk_drm_kms_deinit(drm);
> >   err_free:
> > +	private->drm = NULL;
> 
> Sorry for not noticing that in v1, but I've rechecked this function and, while this
> commit does indeed actually solve the described issue, I think it's incomplete.
> 
> A few lines before, we have a loop that sets
> 
> 		private->all_drm_private[i]->drm = drm;

Actually that line only exists with [1] applied, but that commit hasn't been
merged as of yet. I debated whether it would be better to send this fix as is,
or ask Nancy Lin to add the tweaked fix you mention below to that series, but
sending this fix as is seemed better since it can be backported to older kernel
versions.

So assuming this commit gets merged first, Nancy should make that tweak you
mentioned below to ensure all the pointers are zeroed out, which is why I've
added Nancy to CC. (As a side note, given that only the mmsys with drm_master =
true would ever call the drm suspend helper, even this patch as is is enough to
avoid the panic even with that series applied, still we should zero out all
pointers for good measure)

[1] https://lore.kernel.org/linux-mediatek/20221107072413.16178-6-nancy.lin@mediatek.com/

Thanks,
N?colas

> 
> ...so here you should do...
> 
> 	private->drm = NULL;
> 
> 	while (i--) /* a for loop will also do, your choice */
> 		private->all_drm_private[i]->drm = NULL;
> 		
> That makes sure that you cleanup *everything* :-)
> 
> Cheers,
> Angelo
>