[PATCH] clk: meson: meson8b: fix a memory leak in meson8b_clkc_init_common()

[PATCH] clk: meson: meson8b: fix a memory leak in meson8b_clkc_init_common()

[xkernel.wang]

From: Xiaoke Wang <xkernel.wang@foxmail.com>

`rstc` is allocated by kzalloc() for resetting the controller register,
however, if reset_controller_register() fails, `rstc` is not properly
released before returning, which can lead to memory leak.
Therefore, this patch adds kfree(rstc) on the above error path.

Signed-off-by: Xiaoke Wang <xkernel.wang@foxmail.com>
---
 drivers/clk/meson/meson8b.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/meson/meson8b.c b/drivers/clk/meson/meson8b.c
index a844d35..823eacc 100644
--- a/drivers/clk/meson/meson8b.c
+++ b/drivers/clk/meson/meson8b.c
@@ -3741,6 +3741,7 @@ static void __init meson8b_clkc_init_common(struct device_node *np,
 	if (ret) {
 		pr_err("%s: Failed to register clkc reset controller: %d\n",
 		       __func__, ret);
+		kfree(rstc);
 		return;
 	}
 
-- 

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH] clk: meson: meson8b: fix a memory leak in meson8b_clkc_init_common()

[Philipp Zabel]

On Do, 2022-04-07 at 17:28 +0800, xkernel.wang@foxmail.com wrote:
> From: Xiaoke Wang <xkernel.wang@foxmail.com>
> 
> `rstc` is allocated by kzalloc() for resetting the controller register,
> however, if reset_controller_register() fails, `rstc` is not properly
> released before returning, which can lead to memory leak.
> Therefore, this patch adds kfree(rstc) on the above error path.
> 
> Signed-off-by: Xiaoke Wang <xkernel.wang@foxmail.com>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH] clk: meson: meson8b: fix a memory leak in meson8b_clkc_init_common()

[Martin Blumenstingl]

Hello,

first of all: thank you for this patch!

On Thu, Apr 7, 2022 at 11:28 AM <xkernel.wang@foxmail.com> wrote:
>
> From: Xiaoke Wang <xkernel.wang@foxmail.com>
>
> `rstc` is allocated by kzalloc() for resetting the controller register,
> however, if reset_controller_register() fails, `rstc` is not properly
> released before returning, which can lead to memory leak.
> Therefore, this patch adds kfree(rstc) on the above error path.
In general I am fine with this approach. There's some more "return"
statements below. Should these be covered as well?

Also a note about meson8b_clkc_init_common() itself: failures in that
function will result in a non-working system.
If we can't register the reset controller then most devices won't
probe and CPU SMP cannot work.
If registering any clock or the clock controller doesn't work then the
system also won't work as clocks are not available to other drivers.
So freeing memory in case of an error is good to have, but the end
result is still the same: the system won't work.


Best regards,
Martin

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH] clk: meson: meson8b: fix a memory leak in meson8b_clkc_init_common()

[Stephen Boyd]

Quoting Martin Blumenstingl (2022-04-18 09:39:57)
> Hello,
> 
> first of all: thank you for this patch!
> 
> On Thu, Apr 7, 2022 at 11:28 AM <xkernel.wang@foxmail.com> wrote:
> >
> > From: Xiaoke Wang <xkernel.wang@foxmail.com>
> >
> > `rstc` is allocated by kzalloc() for resetting the controller register,
> > however, if reset_controller_register() fails, `rstc` is not properly
> > released before returning, which can lead to memory leak.
> > Therefore, this patch adds kfree(rstc) on the above error path.
> In general I am fine with this approach. There's some more "return"
> statements below. Should these be covered as well?

Probably!

> 
> Also a note about meson8b_clkc_init_common() itself: failures in that
> function will result in a non-working system.
> If we can't register the reset controller then most devices won't
> probe and CPU SMP cannot work.
> If registering any clock or the clock controller doesn't work then the
> system also won't work as clocks are not available to other drivers.
> So freeing memory in case of an error is good to have, but the end
> result is still the same: the system won't work.
> 

Can we get far enough to record this fact into either a pstore ramoops
location or the serial console? That would be ideal to make debugging
early problems easier.

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH] clk: meson: meson8b: fix a memory leak in meson8b_clkc_init_common()

[Martin Blumenstingl]

Hi Stephen,

On Sat, Apr 23, 2022 at 4:25 AM Stephen Boyd <sboyd@kernel.org> wrote:
[...]
> > Also a note about meson8b_clkc_init_common() itself: failures in that
> > function will result in a non-working system.
> > If we can't register the reset controller then most devices won't
> > probe and CPU SMP cannot work.
> > If registering any clock or the clock controller doesn't work then the
> > system also won't work as clocks are not available to other drivers.
> > So freeing memory in case of an error is good to have, but the end
> > result is still the same: the system won't work.
> >
>
> Can we get far enough to record this fact into either a pstore ramoops
> location or the serial console? That would be ideal to make debugging
> early problems easier.
earlycon shows these messages (as it's enabled by the bootloader)
while "normal" serial console won't come up without the corresponding
clocks.
I never tried ramoops but I expect it to be able to log these errors as well.


Best regards,
Martin

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic