| Message ID | 20250131012830.22394-1-jiashengjiangcool@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: mediatek: vcodec: Add scp_put() to free the scp | expand |
Hey Jiasheng, ---- On Fri, 31 Jan 2025 02:28:30 +0100 Jiasheng Jiang <jiashengjiangcool@gmail.com> wrote --- > Add scp_put() to free the scp if devm_kzalloc() fails to avoid memory > leak. Your commit message sounds a bit like you copy-pasted your code into the commit message. What kind of memory is leaking here? Are we talking about SRAM memory? I'd reword the commit message to something like this to give a bit more context: On Mediatek devices with a system companion processor (SCP) the mtk_scp structure has to be removed explicitly to avoid a memory leak. Free the structure in case the allocation of the firmware structure fails during the firmware initialization. --- Additionally, the commit title says close to nothing to the reader as well. How about: Fix memory leak in FW initialization But as I stated above you should clarify what kind of memory we are talking about here. Also just out of interest have you ever actually experienced issues with this? It seems to me that the situation where you run out of Kernel memory should be quite rare. Regards, Sebastian > > Fixes: 53dbe0850444 ("media: mtk-vcodec: potential null pointer deference in SCP") > Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com> > --- > .../platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c > index ff23b225db70..1b0bc47355c0 100644 > --- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c > +++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c > @@ -79,8 +79,11 @@ struct mtk_vcodec_fw *mtk_vcodec_fw_scp_init(void *priv, enum mtk_vcodec_fw_use > } > > fw = devm_kzalloc(&plat_dev->dev, sizeof(*fw), GFP_KERNEL); > - if (!fw) > + if (!fw) { > + scp_put(scp); > return ERR_PTR(-ENOMEM); > + } > + > fw->type = SCP; > fw->ops = &mtk_vcodec_rproc_msg; > fw->scp = scp; > -- > 2.25.1 > > >
Hi Sebastian, On Fri, Feb 14, 2025 at 3:20 PM Sebastian Fricke <sebastian.fricke@collabora.com> wrote: > > Hey Jiasheng, > > ---- On Fri, 31 Jan 2025 02:28:30 +0100 Jiasheng Jiang <jiashengjiangcool@gmail.com> wrote --- > > Add scp_put() to free the scp if devm_kzalloc() fails to avoid memory > > leak. > > Your commit message sounds a bit like you copy-pasted your code into the commit message. > What kind of memory is leaking here? Are we talking about SRAM memory? > I'd reword the commit message to something like this to give a bit more context: > > On Mediatek devices with a system companion processor (SCP) the mtk_scp structure has to be removed explicitly to avoid a memory leak. > Free the structure in case the allocation of the firmware structure fails during the firmware initialization. > > --- > > Additionally, the commit title says close to nothing to the reader as well. > How about: Fix memory leak in FW initialization > > But as I stated above you should clarify what kind of memory we are talking about here. > > Also just out of interest have you ever actually experienced issues with this? > It seems to me that the situation where you run out of Kernel memory should be quite rare. Yes, I just detected such a bug with a static analysis tool and I have no idea how to trigger it. I will submit a v2 to reword the commit message. Thanks, Jiasheng > > Regards, > Sebastian > > > > > Fixes: 53dbe0850444 ("media: mtk-vcodec: potential null pointer deference in SCP") > > Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com> > > --- > > .../platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c > > index ff23b225db70..1b0bc47355c0 100644 > > --- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c > > +++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c > > @@ -79,8 +79,11 @@ struct mtk_vcodec_fw *mtk_vcodec_fw_scp_init(void *priv, enum mtk_vcodec_fw_use > > } > > > > fw = devm_kzalloc(&plat_dev->dev, sizeof(*fw), GFP_KERNEL); > > - if (!fw) > > + if (!fw) { > > + scp_put(scp); > > return ERR_PTR(-ENOMEM); > > + } > > + > > fw->type = SCP; > > fw->ops = &mtk_vcodec_rproc_msg; > > fw->scp = scp; > > -- > > 2.25.1 > > > > > > >
diff --git a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c index ff23b225db70..1b0bc47355c0 100644 --- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c +++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c @@ -79,8 +79,11 @@ struct mtk_vcodec_fw *mtk_vcodec_fw_scp_init(void *priv, enum mtk_vcodec_fw_use } fw = devm_kzalloc(&plat_dev->dev, sizeof(*fw), GFP_KERNEL); - if (!fw) + if (!fw) { + scp_put(scp); return ERR_PTR(-ENOMEM); + } + fw->type = SCP; fw->ops = &mtk_vcodec_rproc_msg; fw->scp = scp;
Add scp_put() to free the scp if devm_kzalloc() fails to avoid memory leak. Fixes: 53dbe0850444 ("media: mtk-vcodec: potential null pointer deference in SCP") Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com> --- .../platform/mediatek/vcodec/common/mtk_vcodec_fw_scp.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)