clk: imx: clk-imx8mm: fix memory leak issue in 'imx8mm_clocks_probe'

[PATCH] clk: imx: clk-imx8mm: fix memory leak issue in 'imx8mm_clocks_probe'

Posted by Zhou Shide 2 years, 10 months ago

The function imx8mm_clocks_probe() has two main issues:
- The of_iomap() function may cause a memory leak.
- Memory allocated for 'clk_hw_data' may not be freed properly
in some paths.

To fix these issues, this commit replaces the use of of_iomap()
with devm_of_iomap() and replaces kzalloc() with devm_kzalloc().
This ensures that all memory is properly managed and automatically
freed when the device is removed.

In addition, when devm_of_iomap() allocates memory with an error,
it will first jump to label "unregister_hws" and
then return PTR_ ERR(base).

Fixes: 9c71f9ea35d7 ("clk: imx: imx8mm: Switch to clk_hw based API")
Fixes: ba5625c3e272 ("clk: imx: Add clock driver support for imx8mm")
Signed-off-by: Zhou Shide <u201911681@hust.edu.cn>
---
The issue is discovered by static analysis, and the patch is not tested yet.

 drivers/clk/imx/clk-imx8mm.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
index b618892170f2..e6e0bb4805a6 100644
--- a/drivers/clk/imx/clk-imx8mm.c
+++ b/drivers/clk/imx/clk-imx8mm.c
@@ -303,7 +303,7 @@ static int imx8mm_clocks_probe(struct platform_device *pdev)
 	void __iomem *base;
 	int ret;
 
-	clk_hw_data = kzalloc(struct_size(clk_hw_data, hws,
+	clk_hw_data = devm_kzalloc(dev, struct_size(clk_hw_data, hws,
 					  IMX8MM_CLK_END), GFP_KERNEL);
 	if (WARN_ON(!clk_hw_data))
 		return -ENOMEM;
@@ -320,10 +320,12 @@ static int imx8mm_clocks_probe(struct platform_device *pdev)
 	hws[IMX8MM_CLK_EXT4] = imx_get_clk_hw_by_name(np, "clk_ext4");
 
 	np = of_find_compatible_node(NULL, NULL, "fsl,imx8mm-anatop");
-	base = of_iomap(np, 0);
+	base = devm_of_iomap(dev, np, 0, NULL);
 	of_node_put(np);
-	if (WARN_ON(!base))
-		return -ENOMEM;
+	if (WARN_ON(IS_ERR(base))) {
+		ret = PTR_ERR(base);
+		goto unregister_hws;
+	}
 
 	hws[IMX8MM_AUDIO_PLL1_REF_SEL] = imx_clk_hw_mux("audio_pll1_ref_sel", base + 0x0, 0, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels));
 	hws[IMX8MM_AUDIO_PLL2_REF_SEL] = imx_clk_hw_mux("audio_pll2_ref_sel", base + 0x14, 0, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels));
@@ -399,8 +401,10 @@ static int imx8mm_clocks_probe(struct platform_device *pdev)
 
 	np = dev->of_node;
 	base = devm_platform_ioremap_resource(pdev, 0);
-	if (WARN_ON(IS_ERR(base)))
-		return PTR_ERR(base);
+	if (WARN_ON(IS_ERR(base))) {
+		ret = PTR_ERR(base);
+		goto unregister_hws;
+	}
 
 	/* Core Slice */
 	hws[IMX8MM_CLK_A53_DIV] = imx8m_clk_hw_composite_core("arm_a53_div", imx8mm_a53_sels, base + 0x8000);
-- 
2.34.1

Re: [PATCH] clk: imx: clk-imx8mm: fix memory leak issue in 'imx8mm_clocks_probe'

Posted by Peng Fan 2 years, 9 months ago


On 4/13/2023 11:24 AM, Zhou Shide wrote:
> The function imx8mm_clocks_probe() has two main issues:
> - The of_iomap() function may cause a memory leak.
> - Memory allocated for 'clk_hw_data' may not be freed properly
> in some paths.
> 
> To fix these issues, this commit replaces the use of of_iomap()
> with devm_of_iomap() and replaces kzalloc() with devm_kzalloc().
> This ensures that all memory is properly managed and automatically
> freed when the device is removed.
> 
> In addition, when devm_of_iomap() allocates memory with an error,
> it will first jump to label "unregister_hws" and
> then return PTR_ ERR(base).
> 
> Fixes: 9c71f9ea35d7 ("clk: imx: imx8mm: Switch to clk_hw based API")
> Fixes: ba5625c3e272 ("clk: imx: Add clock driver support for imx8mm")
> Signed-off-by: Zhou Shide<u201911681@hust.edu.cn>


Reviewed-by: Peng Fan <peng.fan@nxp.com>

Re: [PATCH] clk: imx: clk-imx8mm: fix memory leak issue in 'imx8mm_clocks_probe'

Posted by 周师德 2 years, 9 months ago



> -----原始邮件-----
> 发件人: "Zhou Shide" <u201911681@hust.edu.cn>
> 发送时间: 2023-04-13 11:24:39 (星期四)
> 收件人: "Abel Vesa" <abelvesa@kernel.org>, "Peng Fan" <peng.fan@nxp.com>, "Michael Turquette" <mturquette@baylibre.com>, "Stephen Boyd" <sboyd@kernel.org>, "Shawn Guo" <shawnguo@kernel.org>, "Sascha Hauer" <s.hauer@pengutronix.de>, "Pengutronix Kernel Team" <kernel@pengutronix.de>, "Fabio Estevam" <festevam@gmail.com>, "NXP Linux Team" <linux-imx@nxp.com>, "Bai Ping" <ping.bai@nxp.com>
> 抄送: hust-os-kernel-patches@googlegroups.com, "Zhou Shide" <u201911681@hust.edu.cn>, linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
> 主题: [PATCH] clk: imx: clk-imx8mm: fix memory leak issue in 'imx8mm_clocks_probe'
> 
> The function imx8mm_clocks_probe() has two main issues:
> - The of_iomap() function may cause a memory leak.
> - Memory allocated for 'clk_hw_data' may not be freed properly
> in some paths.
> 
> To fix these issues, this commit replaces the use of of_iomap()
> with devm_of_iomap() and replaces kzalloc() with devm_kzalloc().
> This ensures that all memory is properly managed and automatically
> freed when the device is removed.
> 
> In addition, when devm_of_iomap() allocates memory with an error,
> it will first jump to label "unregister_hws" and
> then return PTR_ ERR(base).
> 
> Fixes: 9c71f9ea35d7 ("clk: imx: imx8mm: Switch to clk_hw based API")
> Fixes: ba5625c3e272 ("clk: imx: Add clock driver support for imx8mm")
> Signed-off-by: Zhou Shide <u201911681@hust.edu.cn>
> ---
> The issue is discovered by static analysis, and the patch is not tested yet.
> 
>  drivers/clk/imx/clk-imx8mm.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
> index b618892170f2..e6e0bb4805a6 100644
> --- a/drivers/clk/imx/clk-imx8mm.c
> +++ b/drivers/clk/imx/clk-imx8mm.c
> @@ -303,7 +303,7 @@ static int imx8mm_clocks_probe(struct platform_device *pdev)
>  	void __iomem *base;
>  	int ret;
>  
> -	clk_hw_data = kzalloc(struct_size(clk_hw_data, hws,
> +	clk_hw_data = devm_kzalloc(dev, struct_size(clk_hw_data, hws,
>  					  IMX8MM_CLK_END), GFP_KERNEL);
>  	if (WARN_ON(!clk_hw_data))
>  		return -ENOMEM;
> @@ -320,10 +320,12 @@ static int imx8mm_clocks_probe(struct platform_device *pdev)
>  	hws[IMX8MM_CLK_EXT4] = imx_get_clk_hw_by_name(np, "clk_ext4");
>  
>  	np = of_find_compatible_node(NULL, NULL, "fsl,imx8mm-anatop");
> -	base = of_iomap(np, 0);
> +	base = devm_of_iomap(dev, np, 0, NULL);
>  	of_node_put(np);
> -	if (WARN_ON(!base))
> -		return -ENOMEM;
> +	if (WARN_ON(IS_ERR(base))) {
> +		ret = PTR_ERR(base);
> +		goto unregister_hws;
> +	}
>  
>  	hws[IMX8MM_AUDIO_PLL1_REF_SEL] = imx_clk_hw_mux("audio_pll1_ref_sel", base + 0x0, 0, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels));
>  	hws[IMX8MM_AUDIO_PLL2_REF_SEL] = imx_clk_hw_mux("audio_pll2_ref_sel", base + 0x14, 0, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels));
> @@ -399,8 +401,10 @@ static int imx8mm_clocks_probe(struct platform_device *pdev)
>  
>  	np = dev->of_node;
>  	base = devm_platform_ioremap_resource(pdev, 0);
> -	if (WARN_ON(IS_ERR(base)))
> -		return PTR_ERR(base);
> +	if (WARN_ON(IS_ERR(base))) {
> +		ret = PTR_ERR(base);
> +		goto unregister_hws;
> +	}
>  
>  	/* Core Slice */
>  	hws[IMX8MM_CLK_A53_DIV] = imx8m_clk_hw_composite_core("arm_a53_div", imx8mm_a53_sels, base + 0x8000);
> -- 
> 2.34.1
ping？

Re: [PATCH] clk: imx: clk-imx8mm: fix memory leak issue in 'imx8mm_clocks_probe'

Posted by Stephen Boyd 2 years, 10 months ago

Quoting Zhou Shide (2023-04-12 20:24:39)
> The function imx8mm_clocks_probe() has two main issues:
> - The of_iomap() function may cause a memory leak.
> - Memory allocated for 'clk_hw_data' may not be freed properly
> in some paths.
> 
> To fix these issues, this commit replaces the use of of_iomap()
> with devm_of_iomap() and replaces kzalloc() with devm_kzalloc().
> This ensures that all memory is properly managed and automatically
> freed when the device is removed.
> 
> In addition, when devm_of_iomap() allocates memory with an error,
> it will first jump to label "unregister_hws" and
> then return PTR_ ERR(base).
> 
> Fixes: 9c71f9ea35d7 ("clk: imx: imx8mm: Switch to clk_hw based API")
> Fixes: ba5625c3e272 ("clk: imx: Add clock driver support for imx8mm")
> Signed-off-by: Zhou Shide <u201911681@hust.edu.cn>
> ---
> The issue is discovered by static analysis, and the patch is not tested yet.

And you're not coordinating with each other?

Re: [PATCH] clk: imx: clk-imx8mm: fix memory leak issue in 'imx8mm_clocks_probe'

Posted by Dan Carpenter 2 years, 10 months ago

On Thu, Apr 13, 2023 at 12:06:59PM -0700, Stephen Boyd wrote:
> Quoting Zhou Shide (2023-04-12 20:24:39)
> > The function imx8mm_clocks_probe() has two main issues:
> > - The of_iomap() function may cause a memory leak.
> > - Memory allocated for 'clk_hw_data' may not be freed properly
> > in some paths.
> > 
> > To fix these issues, this commit replaces the use of of_iomap()
> > with devm_of_iomap() and replaces kzalloc() with devm_kzalloc().
> > This ensures that all memory is properly managed and automatically
> > freed when the device is removed.
> > 
> > In addition, when devm_of_iomap() allocates memory with an error,
> > it will first jump to label "unregister_hws" and
> > then return PTR_ ERR(base).
> > 
> > Fixes: 9c71f9ea35d7 ("clk: imx: imx8mm: Switch to clk_hw based API")
> > Fixes: ba5625c3e272 ("clk: imx: Add clock driver support for imx8mm")
> > Signed-off-by: Zhou Shide <u201911681@hust.edu.cn>
> > ---
> > The issue is discovered by static analysis, and the patch is not tested yet.
> 
> And you're not coordinating with each other?
> 

This is a university program.  The patches are reviewed by his professor
and teaching assistants etc.  I've been reviewing some of these patches
as well because of they're using Smatch.

regards,
dan carpenter

Re: [PATCH] clk: imx: clk-imx8mm: fix memory leak issue in 'imx8mm_clocks_probe'

Posted by Dongliang Mu 2 years, 10 months ago

On 2023/4/15 00:38, Dan Carpenter wrote:
> On Thu, Apr 13, 2023 at 12:06:59PM -0700, Stephen Boyd wrote:
>> Quoting Zhou Shide (2023-04-12 20:24:39)
>>> The function imx8mm_clocks_probe() has two main issues:
>>> - The of_iomap() function may cause a memory leak.
>>> - Memory allocated for 'clk_hw_data' may not be freed properly
>>> in some paths.
>>>
>>> To fix these issues, this commit replaces the use of of_iomap()
>>> with devm_of_iomap() and replaces kzalloc() with devm_kzalloc().
>>> This ensures that all memory is properly managed and automatically
>>> freed when the device is removed.
>>>
>>> In addition, when devm_of_iomap() allocates memory with an error,
>>> it will first jump to label "unregister_hws" and
>>> then return PTR_ ERR(base).
>>>
>>> Fixes: 9c71f9ea35d7 ("clk: imx: imx8mm: Switch to clk_hw based API")
>>> Fixes: ba5625c3e272 ("clk: imx: Add clock driver support for imx8mm")
>>> Signed-off-by: Zhou Shide <u201911681@hust.edu.cn>
>>> ---
>>> The issue is discovered by static analysis, and the patch is not tested yet.
>> And you're not coordinating with each other?
>>
> This is a university program.  The patches are reviewed by his professor
> and teaching assistants etc.  I've been reviewing some of these patches
> as well because of they're using Smatch.

Thanks for your explanation, Dan. We are from Huazhong University of 
Science and Technology.

Some undergraduate and graduatestudents who are interested in Linux 
Kernel are guided by me [1] and Dan to contribute into our kernel community.

We found Smatch is really great in finding kernel issues and these 
issues are suitable for undergraduate students. Therefore, I contacted 
Dan to do a favor for patch interview. And our internal review are 
publicly hosted in a google group [2].

Please let me know if you have any questions.

[1] https://mudongliang.github.io/about/

[2] https://groups.google.com/g/hust-os-kernel-patches

>
> regards,
> dan carpenter
>

Re: Re: [PATCH] clk: imx: clk-imx8mm: fix memory leak issue in 'imx8mm_clocks_probe'

Posted by 周师德 2 years, 10 months ago



> -----原始邮件-----
> 发件人: "Stephen Boyd" <sboyd@kernel.org>
> 发送时间: 2023-04-14 03:06:59 (星期五)
> 收件人: "Abel Vesa" <abelvesa@kernel.org>, "Bai Ping" <ping.bai@nxp.com>, "Fabio Estevam" <festevam@gmail.com>, "Michael Turquette" <mturquette@baylibre.com>, "NXP Linux Team" <linux-imx@nxp.com>, "Peng Fan" <peng.fan@nxp.com>, "Pengutronix Kernel Team" <kernel@pengutronix.de>, "Sascha Hauer" <s.hauer@pengutronix.de>, "Shawn Guo" <shawnguo@kernel.org>, "Zhou Shide" <u201911681@hust.edu.cn>
> 抄送: hust-os-kernel-patches@googlegroups.com, "Zhou Shide" <u201911681@hust.edu.cn>, linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, "Hao Luo" <m202171776@hust.edu.cn>
> 主题: Re: [PATCH] clk: imx: clk-imx8mm: fix memory leak issue in 'imx8mm_clocks_probe'
> 
> Quoting Zhou Shide (2023-04-12 20:24:39)
> > The function imx8mm_clocks_probe() has two main issues:
> > - The of_iomap() function may cause a memory leak.
> > - Memory allocated for 'clk_hw_data' may not be freed properly
> > in some paths.
> > 
> > To fix these issues, this commit replaces the use of of_iomap()
> > with devm_of_iomap() and replaces kzalloc() with devm_kzalloc().
> > This ensures that all memory is properly managed and automatically
> > freed when the device is removed.
> > 
> > In addition, when devm_of_iomap() allocates memory with an error,
> > it will first jump to label "unregister_hws" and
> > then return PTR_ ERR(base).
> > 
> > Fixes: 9c71f9ea35d7 ("clk: imx: imx8mm: Switch to clk_hw based API")
> > Fixes: ba5625c3e272 ("clk: imx: Add clock driver support for imx8mm")
> > Signed-off-by: Zhou Shide <u201911681@hust.edu.cn>
> > ---
> > The issue is discovered by static analysis, and the patch is not tested yet.
> 
> And you're not coordinating with each other?
What do you mean by "coordinating with each other"?

regards,
Zhou Shide

Re: Re: [PATCH] clk: imx: clk-imx8mm: fix memory leak issue in 'imx8mm_clocks_probe'

Posted by Stephen Boyd 2 years, 9 months ago

Quoting 周师德 (2023-04-13 19:02:19)
> 
> 
> 
> > -----原始邮件-----
> > 发件人: "Stephen Boyd" <sboyd@kernel.org>
> > 发送时间: 2023-04-14 03:06:59 (星期五)
> > 收件人: "Abel Vesa" <abelvesa@kernel.org>, "Bai Ping" <ping.bai@nxp.com>, "Fabio Estevam" <festevam@gmail.com>, "Michael Turquette" <mturquette@baylibre.com>, "NXP Linux Team" <linux-imx@nxp.com>, "Peng Fan" <peng.fan@nxp.com>, "Pengutronix Kernel Team" <kernel@pengutronix.de>, "Sascha Hauer" <s.hauer@pengutronix.de>, "Shawn Guo" <shawnguo@kernel.org>, "Zhou Shide" <u201911681@hust.edu.cn>
> > 抄送: hust-os-kernel-patches@googlegroups.com, "Zhou Shide" <u201911681@hust.edu.cn>, linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, "Hao Luo" <m202171776@hust.edu.cn>
> > 主题: Re: [PATCH] clk: imx: clk-imx8mm: fix memory leak issue in 'imx8mm_clocks_probe'
> > 
> > Quoting Zhou Shide (2023-04-12 20:24:39)
> > > The function imx8mm_clocks_probe() has two main issues:
> > > - The of_iomap() function may cause a memory leak.
> > > - Memory allocated for 'clk_hw_data' may not be freed properly
> > > in some paths.
> > > 
> > > To fix these issues, this commit replaces the use of of_iomap()
> > > with devm_of_iomap() and replaces kzalloc() with devm_kzalloc().
> > > This ensures that all memory is properly managed and automatically
> > > freed when the device is removed.
> > > 
> > > In addition, when devm_of_iomap() allocates memory with an error,
> > > it will first jump to label "unregister_hws" and
> > > then return PTR_ ERR(base).
> > > 
> > > Fixes: 9c71f9ea35d7 ("clk: imx: imx8mm: Switch to clk_hw based API")
> > > Fixes: ba5625c3e272 ("clk: imx: Add clock driver support for imx8mm")
> > > Signed-off-by: Zhou Shide <u201911681@hust.edu.cn>
> > > ---
> > > The issue is discovered by static analysis, and the patch is not tested yet.
> > 
> > And you're not coordinating with each other?
> What do you mean by "coordinating with each other"?
> 

I see two patches to the same driver from the same university on the
list. Preferably you coordinate and decide who will fix what smatch
warnings.

Re: [PATCH] clk: imx: clk-imx8mm: fix memory leak issue in 'imx8mm_clocks_probe'

Posted by Dongliang Mu 2 years, 9 months ago

On 2023/4/19 03:57, Stephen Boyd wrote:
> Quoting 周师德 (2023-04-13 19:02:19)
>>
>>
>>> -----原始邮件-----
>>> 发件人: "Stephen Boyd" <sboyd@kernel.org>
>>> 发送时间: 2023-04-14 03:06:59 (星期五)
>>> 收件人: "Abel Vesa" <abelvesa@kernel.org>, "Bai Ping" <ping.bai@nxp.com>, "Fabio Estevam" <festevam@gmail.com>, "Michael Turquette" <mturquette@baylibre.com>, "NXP Linux Team" <linux-imx@nxp.com>, "Peng Fan" <peng.fan@nxp.com>, "Pengutronix Kernel Team" <kernel@pengutronix.de>, "Sascha Hauer" <s.hauer@pengutronix.de>, "Shawn Guo" <shawnguo@kernel.org>, "Zhou Shide" <u201911681@hust.edu.cn>
>>> 抄送: hust-os-kernel-patches@googlegroups.com, "Zhou Shide" <u201911681@hust.edu.cn>, linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, "Hao Luo" <m202171776@hust.edu.cn>
>>> 主题: Re: [PATCH] clk: imx: clk-imx8mm: fix memory leak issue in 'imx8mm_clocks_probe'
>>>
>>> Quoting Zhou Shide (2023-04-12 20:24:39)
>>>> The function imx8mm_clocks_probe() has two main issues:
>>>> - The of_iomap() function may cause a memory leak.
>>>> - Memory allocated for 'clk_hw_data' may not be freed properly
>>>> in some paths.
>>>>
>>>> To fix these issues, this commit replaces the use of of_iomap()
>>>> with devm_of_iomap() and replaces kzalloc() with devm_kzalloc().
>>>> This ensures that all memory is properly managed and automatically
>>>> freed when the device is removed.
>>>>
>>>> In addition, when devm_of_iomap() allocates memory with an error,
>>>> it will first jump to label "unregister_hws" and
>>>> then return PTR_ ERR(base).
>>>>
>>>> Fixes: 9c71f9ea35d7 ("clk: imx: imx8mm: Switch to clk_hw based API")
>>>> Fixes: ba5625c3e272 ("clk: imx: Add clock driver support for imx8mm")
>>>> Signed-off-by: Zhou Shide <u201911681@hust.edu.cn>
>>>> ---
>>>> The issue is discovered by static analysis, and the patch is not tested yet.
>>> And you're not coordinating with each other?
>> What do you mean by "coordinating with each other"?
>>
> I see two patches to the same driver from the same university on the
> list. Preferably you coordinate and decide who will fix what smatch
> warnings.

Hi Stephen,

As their advisor, I coordinate and assign smatch warnings to each 
student. I double check our assignment table:

drivers/clk/imx/clk-imx8mn.c:612 imx8mn_clocks_probe() warn: 'base' from 
of_iomap() not released on lines: 612.
drivers/clk/imx/clk-imxrt1050.c:154 imxrt1050_clocks_probe() warn: 
'pll_base' from of_iomap() not released on lines: 108,154.
drivers/clk/imx/clk-imx8mm.c:619 imx8mm_clocks_probe() warn: 'base' from 
of_iomap() not released on lines: 403,619.
drivers/clk/imx/clk-imx8mq.c:611 imx8mq_clocks_probe() warn: 'base' from 
of_iomap() not released on lines: 399,611.

There are four similar warnings from your subsystem. If I understand 
correctly, two students are patching issues from different probe 
functions in the different files since we assign all issues in one file 
to one student. Maybe you mix clk-imx8mn (Hao Luo) and clk-imx8mm(Shide 
Zhou). They only differ in one char.

If I miss anything, please let me know. Next time, I will ask one 
student to fix the issues in one subsystem. This can simply the effort 
spent by other student.

Dongliang Mu

>