| Message ID | 20221122143949.3493104-1-nfraprado@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/mediatek: Clean dangling pointer on bind error path | expand |
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; ...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
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 >
Il 25/11/22 17:34, Nícolas F. R. A. Prado ha scritto: > 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/ > Sorry about that, you're right - the proposed fix is for the commit you linked, I got confused somehow. This means that you get my Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> Cheers!
Hi, Nicolas: Nícolas F. R. A. Prado <nfraprado@collabora.com> 於 2022年11月22日 週二 下午10:39寫道： > > 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. Applied to mediatek-drm-next [1], thanks. [1] https://git.kernel.org/pub/scm/linux/kernel/git/chunkuang.hu/linux.git/log/?h=mediatek-drm-next Regards, Chun-Kuang. > > 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; > drm_dev_put(drm); > return ret; > } > -- > 2.38.1 >
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; drm_dev_put(drm); return ret; }
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(+)