[PATCH v1] clk: davinci: Add NULL check in davinci_lpsc_clk_register()

[PATCH v1] clk: davinci: Add NULL check in davinci_lpsc_clk_register()

[Henry Martin]

devm_kasprintf() returns NULL when memory allocation fails. Currently,
davinci_lpsc_clk_register() does not check for this case, which results
in a NULL pointer dereference.

Add NULL check after devm_kasprintf() to prevent this issue and ensuring
no resources are left allocated.

Fixes: c6ed4d734bc7 ("clk: davinci: New driver for davinci PSC clocks")
Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>
---
 drivers/clk/davinci/psc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/clk/davinci/psc.c b/drivers/clk/davinci/psc.c
index b48322176c21..f3ee9397bb0c 100644
--- a/drivers/clk/davinci/psc.c
+++ b/drivers/clk/davinci/psc.c
@@ -277,6 +277,11 @@ davinci_lpsc_clk_register(struct device *dev, const char *name,
 
 	lpsc->pm_domain.name = devm_kasprintf(dev, GFP_KERNEL, "%s: %s",
 					      best_dev_name(dev), name);
+	if (!lpsc->pm_domain.name) {
+		clk_hw_unregister(&lpsc->hw);
+		kfree(lpsc);
+		return ERR_PTR(-ENOMEM);
+	}
 	lpsc->pm_domain.attach_dev = davinci_psc_genpd_attach_dev;
 	lpsc->pm_domain.detach_dev = davinci_psc_genpd_detach_dev;
 	lpsc->pm_domain.flags = GENPD_FLAG_PM_CLK;
-- 
2.34.1

Re: [PATCH v1] clk: davinci: Add NULL check in davinci_lpsc_clk_register()

[David Lechner]

On 4/1/25 8:13 AM, Henry Martin wrote:
> devm_kasprintf() returns NULL when memory allocation fails. Currently,
> davinci_lpsc_clk_register() does not check for this case, which results
> in a NULL pointer dereference.
> 
> Add NULL check after devm_kasprintf() to prevent this issue and ensuring
> no resources are left allocated.
> 
> Fixes: c6ed4d734bc7 ("clk: davinci: New driver for davinci PSC clocks")
> Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>
> ---

Reviewed-by: David Lechner <david@lechnology.com>

Re: [PATCH] clk: davinci: Add NULL check in davinci_lpsc_clk_register()

[Markus Elfring]

> devm_kasprintf() return NULL if memory allocation fails. Currently,
…
                call?                               failed?


> Add NULL check after devm_kasprintf() to prevent this issue.

I propose to avoid duplicate source code also for the completion of
the corresponding exception handling.

* You may avoid repeated function calls by using another label instead.
  https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources#MEM12C.Considerusingagotochainwhenleavingafunctiononerrorwhenusingandreleasingresources-CompliantSolution(copy_process()fromLinuxkernel)

* How do you think about to benefit any more from the application of the attribute “__free”?
  https://elixir.bootlin.com/linux/v6.14-rc6/source/include/linux/slab.h#L472


Regards,
Markus

Re: [PATCH] clk: davinci: Add NULL check in davinci_lpsc_clk_register()

[David Lechner]

On 4/1/25 11:46 AM, Markus Elfring wrote:
>> devm_kasprintf() return NULL if memory allocation fails. Currently,
> …
>                 call?                               failed?
> 
> 
>> Add NULL check after devm_kasprintf() to prevent this issue.
> 
> I propose to avoid duplicate source code also for the completion of
> the corresponding exception handling.
> 
> * You may avoid repeated function calls by using another label instead.
>   https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources#MEM12C.Considerusingagotochainwhenleavingafunctiononerrorwhenusingandreleasingresources-CompliantSolution(copy_process()fromLinuxkernel)

That would be OK too. I didn't worry about it in this case though
since we are only duplicating 1 very short line of code. And the
smaller diff has a better chance of successfully backporting to older
stable kernels that will also pick up this patch.

> 
> * How do you think about to benefit any more from the application of the attribute “__free”?
>   https://elixir.bootlin.com/linux/v6.14-rc6/source/include/linux/slab.h#L472

Not a good fit for this specific use case.

> 
> 
> Regards,
> Markus

Re: clk: davinci: Add NULL check in davinci_lpsc_clk_register()

[Markus Elfring]

>> I propose to avoid duplicate source code also for the completion of
>> the corresponding exception handling.
>>
>> * You may avoid repeated function calls by using another label instead.
>>   https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources#MEM12C.Considerusingagotochainwhenleavingafunctiononerrorwhenusingandreleasingresources-CompliantSolution(copy_process()fromLinuxkernel)
>
> That would be OK too.

Thanks for another bit of positive feedback.


>                       I didn't worry about it in this case though
> since we are only duplicating 1 very short line of code. And the
> smaller diff has a better chance of successfully backporting to older
> stable kernels that will also pick up this patch.

Such development concerns can be clarified further.


>> * How do you think about to benefit any more from the application of the attribute “__free”?
>>   https://elixir.bootlin.com/linux/v6.14-rc6/source/include/linux/slab.h#L472
>
> Not a good fit for this specific use case.

The acceptance might be growing for such a software design option.


Would you like to clarify any more why the function “kzalloc” is still called here
(instead of the variant “devm_kzalloc”)?
https://elixir.bootlin.com/linux/v6.14-rc6/source/drivers/clk/davinci/psc.c#L242

Regards,
Markus

Re: clk: davinci: Add NULL check in davinci_lpsc_clk_register()

[David Lechner]

On 4/2/25 2:48 AM, Markus Elfring wrote:

> 
> Would you like to clarify any more why the function “kzalloc” is still called here
> (instead of the variant “devm_kzalloc”)?
> https://elixir.bootlin.com/linux/v6.14-rc6/source/drivers/clk/davinci/psc.c#L242

It is a case of "if it isn't broke, don't fix it". While there is room for
some small improvements like that, it does come at a cost of time and energy
to make those improvements.

> 
> Regards,
> Markus

Re: [PATCH v1] clk: davinci: Add NULL check in davinci_lpsc_clk_register()

[henry martin]

Hi Michael, Stephen,

I wanted to follow up on my previous patch submission to check if there are any
additional feedback or changes you'd like me to address. If so, I’d be happy to
incorporate them and send a v2.

Regards,
Henry

Henry Martin <bsdhenrymartin@gmail.com> 于2025年4月1日周二 21:13写道：
>
> devm_kasprintf() returns NULL when memory allocation fails. Currently,
> davinci_lpsc_clk_register() does not check for this case, which results
> in a NULL pointer dereference.
>
> Add NULL check after devm_kasprintf() to prevent this issue and ensuring
> no resources are left allocated.
>
> Fixes: c6ed4d734bc7 ("clk: davinci: New driver for davinci PSC clocks")
> Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>
> ---
>  drivers/clk/davinci/psc.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/clk/davinci/psc.c b/drivers/clk/davinci/psc.c
> index b48322176c21..f3ee9397bb0c 100644
> --- a/drivers/clk/davinci/psc.c
> +++ b/drivers/clk/davinci/psc.c
> @@ -277,6 +277,11 @@ davinci_lpsc_clk_register(struct device *dev, const char *name,
>
>         lpsc->pm_domain.name = devm_kasprintf(dev, GFP_KERNEL, "%s: %s",
>                                               best_dev_name(dev), name);
> +       if (!lpsc->pm_domain.name) {
> +               clk_hw_unregister(&lpsc->hw);
> +               kfree(lpsc);
> +               return ERR_PTR(-ENOMEM);
> +       }
>         lpsc->pm_domain.attach_dev = davinci_psc_genpd_attach_dev;
>         lpsc->pm_domain.detach_dev = davinci_psc_genpd_detach_dev;
>         lpsc->pm_domain.flags = GENPD_FLAG_PM_CLK;
> --
> 2.34.1
>

Re: [PATCH v1] clk: davinci: Add NULL check in davinci_lpsc_clk_register()

[Stephen Boyd]

Quoting Henry Martin (2025-04-01 06:13:41)
> devm_kasprintf() returns NULL when memory allocation fails. Currently,
> davinci_lpsc_clk_register() does not check for this case, which results
> in a NULL pointer dereference.
> 
> Add NULL check after devm_kasprintf() to prevent this issue and ensuring
> no resources are left allocated.
> 
> Fixes: c6ed4d734bc7 ("clk: davinci: New driver for davinci PSC clocks")
> Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>
> ---

Applied to clk-next