Re: [PATCH] bus: ti-sysc: Fix potential double free in sysc_add_named_clock_from_child()

Re: [PATCH] bus: ti-sysc: Fix potential double free in sysc_add_named_clock_from_child()

[Andreas Kemnade]

Hi,

Am Mon,  4 Aug 2025 20:04:03 +0800
schrieb Miaoqian Lin <linmq006@gmail.com>:

> The devm_get_clk_from_child() function uses device-managed resources
> that are automatically cleaned up. The clk_put() call after
> devm_get_clk_from_child() is redundant and
> may lead to double-free issues.
> 
> Fixes: a54275f4ab20 ("bus: ti-sysc: Add quirk handling for external optional functional clock")
> Signed-off-by: Miaoqian Lin <linmq006@gmail.com>
> ---
>  drivers/bus/ti-sysc.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/bus/ti-sysc.c b/drivers/bus/ti-sysc.c
> index 9f624e5da991..5441b0739faa 100644
> --- a/drivers/bus/ti-sysc.c
> +++ b/drivers/bus/ti-sysc.c
> @@ -362,7 +362,6 @@ static int sysc_add_named_clock_from_child(struct sysc *ddata,
>  	cl->clk = clock;
>  	clkdev_add(cl);
>  
> -	clk_put(clock);
>  
>  	return 0;
>  }
I understand the double-free issue, but I have some questions to make
sure I understand it correctly what we are doing here. So lets ask the
possibly stupid questions and check assumptions:

- clk_hw hardware still lives after clk_put(), so we do not have
  problems normally here after that put when we do not remove the
  device?

- With your patch the put is delayed, so things live longer. So why
we do not use devm_clk_put() or avoid using devres at all here?

Regards,
Andreas

Re: [PATCH] bus: ti-sysc: Fix potential double free in sysc_add_named_clock_from_child()

[林妙倩]

Hi, Andreas

Thanks for your reply.

Andreas Kemnade <andreas@kemnade.info> 于2025年8月11日周一 22:56写道：
>
> Hi,
>
> Am Mon,  4 Aug 2025 20:04:03 +0800
> schrieb Miaoqian Lin <linmq006@gmail.com>:
>
> > The devm_get_clk_from_child() function uses device-managed resources
> > that are automatically cleaned up. The clk_put() call after
> > devm_get_clk_from_child() is redundant and
> > may lead to double-free issues.
> >
> > Fixes: a54275f4ab20 ("bus: ti-sysc: Add quirk handling for external optional functional clock")
> > Signed-off-by: Miaoqian Lin <linmq006@gmail.com>
> > ---
> >  drivers/bus/ti-sysc.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/bus/ti-sysc.c b/drivers/bus/ti-sysc.c
> > index 9f624e5da991..5441b0739faa 100644
> > --- a/drivers/bus/ti-sysc.c
> > +++ b/drivers/bus/ti-sysc.c
> > @@ -362,7 +362,6 @@ static int sysc_add_named_clock_from_child(struct sysc *ddata,
> >       cl->clk = clock;
> >       clkdev_add(cl);
> >
> > -     clk_put(clock);
> >
> >       return 0;
> >  }
> I understand the double-free issue, but I have some questions to make
> sure I understand it correctly what we are doing here. So lets ask the
> possibly stupid questions and check assumptions:
>
> - clk_hw hardware still lives after clk_put(), so we do not have
>   problems normally here after that put when we do not remove the
>   device?
>

Yes, the main problem caused here is reference counting imbalance.
After the manual clk_put(clock), the devm framework calls clk_put()
again during device removal, causing issues.

> - With your patch the put is delayed, so things live longer. So why
> we do not use devm_clk_put() or avoid using devres at all here?
>

Since cl->clk = clock; passes the clock pointer to the lookup table,
I'm not sure if calling devm_clk_put() early here is appropriate.
So I just remove the clk_put(clock) and let the devm framework handle this.

If you think there's a better way, I can submit an updated patch.

> Regards,
> Andreas