[v2] clk: spacemit: fix resource leak in spacemit_ccu_reset_register

[PATCH v2] clk: spacemit: fix resource leak in spacemit_ccu_reset_register

Posted by Hendrik Hamerlinck 2 months, 2 weeks ago

The function spacemit_ccu_reset_register() allocates memory for an
auxiliary device. If auxiliary_device_add() fails, it skips cleanup of
these resources, resulting in leaks.

Fix this by using the appropriate error handling path.

Fixes: 988543522ebd ("clk: spacemit: set up reset auxiliary devices")
Signed-off-by: Hendrik Hamerlinck <hendrik.hamerlinck@hammernet.be>
Reviewed-by: Yixun Lan <dlan@gentoo.org>
---
Changes in v2:
- Properly place the Fixes tip.
---
 drivers/clk/spacemit/ccu-k1.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c
index 65e6de030717..5bb85e32c6cf 100644
--- a/drivers/clk/spacemit/ccu-k1.c
+++ b/drivers/clk/spacemit/ccu-k1.c
@@ -1059,7 +1059,7 @@ static int spacemit_ccu_reset_register(struct device *dev,
 	ret = auxiliary_device_add(adev);
 	if (ret) {
 		auxiliary_device_uninit(adev);
-		return ret;
+		goto err_free_aux_id;
 	}
 
 	return devm_add_action_or_reset(dev, spacemit_adev_unregister, adev);
-- 
2.43.0

Re: [PATCH v2] clk: spacemit: fix resource leak in spacemit_ccu_reset_register

Posted by Christophe JAILLET 2 months, 1 week ago

Le 23/07/2025 à 15:25, Hendrik Hamerlinck a écrit :
> The function spacemit_ccu_reset_register() allocates memory for an
> auxiliary device. If auxiliary_device_add() fails, it skips cleanup of
> these resources, resulting in leaks.
> 
> Fix this by using the appropriate error handling path.
> 
> Fixes: 988543522ebd ("clk: spacemit: set up reset auxiliary devices")
> Signed-off-by: Hendrik Hamerlinck <hendrik.hamerlinck@hammernet.be>
> Reviewed-by: Yixun Lan <dlan@gentoo.org>
> ---
> Changes in v2:
> - Properly place the Fixes tip.
> ---
>   drivers/clk/spacemit/ccu-k1.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c
> index 65e6de030717..5bb85e32c6cf 100644
> --- a/drivers/clk/spacemit/ccu-k1.c
> +++ b/drivers/clk/spacemit/ccu-k1.c
> @@ -1059,7 +1059,7 @@ static int spacemit_ccu_reset_register(struct device *dev,
>   	ret = auxiliary_device_add(adev);
>   	if (ret) {
>   		auxiliary_device_uninit(adev);
> -		return ret;
> +		goto err_free_aux_id;
>   	}
>   
>   	return devm_add_action_or_reset(dev, spacemit_adev_unregister, adev);


Hi,

I'm not sure this is correct.

auxiliary_device_uninit() which is called if auxiliary_device_add() 
fails should already do the clean-up.

So I would say that this patch adds a double-free issue and should not 
be applied.

If I'm right, note that Stephen has already added a "Applied to clk-next"

CJ

Re: [PATCH v2] clk: spacemit: fix resource leak in spacemit_ccu_reset_register

Posted by Alex Elder 2 months, 1 week ago

On 7/26/25 4:16 AM, Christophe JAILLET wrote:
> Le 23/07/2025 à 15:25, Hendrik Hamerlinck a écrit :
>> The function spacemit_ccu_reset_register() allocates memory for an
>> auxiliary device. If auxiliary_device_add() fails, it skips cleanup of
>> these resources, resulting in leaks.
>>
>> Fix this by using the appropriate error handling path.
>>
>> Fixes: 988543522ebd ("clk: spacemit: set up reset auxiliary devices")
>> Signed-off-by: Hendrik Hamerlinck <hendrik.hamerlinck@hammernet.be>
>> Reviewed-by: Yixun Lan <dlan@gentoo.org>
>> ---
>> Changes in v2:
>> - Properly place the Fixes tip.
>> ---
>>   drivers/clk/spacemit/ccu-k1.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu- 
>> k1.c
>> index 65e6de030717..5bb85e32c6cf 100644
>> --- a/drivers/clk/spacemit/ccu-k1.c
>> +++ b/drivers/clk/spacemit/ccu-k1.c
>> @@ -1059,7 +1059,7 @@ static int spacemit_ccu_reset_register(struct 
>> device *dev,
>>       ret = auxiliary_device_add(adev);
>>       if (ret) {
>>           auxiliary_device_uninit(adev);
>> -        return ret;
>> +        goto err_free_aux_id;
>>       }
>>       return devm_add_action_or_reset(dev, spacemit_adev_unregister, 
>> adev);
> 
> 
> Hi,
> 
> I'm not sure this is correct.

I'm sure this patch is incorrect, and the original code did
not have the stated problem.  Thank you for calling attention
to this Christophe.

Stephen, could you please revert your commit of this patch?

Thank you.

					-Alex

> auxiliary_device_uninit() which is called if auxiliary_device_add() 
> fails should already do the clean-up.
> 
> So I would say that this patch adds a double-free issue and should not 
> be applied.
> 
> If I'm right, note that Stephen has already added a "Applied to clk-next"
> 
> CJ
> 
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2] clk: spacemit: fix resource leak in spacemit_ccu_reset_register

Posted by Hendrik Hamerlinck 2 months, 1 week ago


On 7/26/25 14:57, Alex Elder wrote:
> On 7/26/25 4:16 AM, Christophe JAILLET wrote:
>> Le 23/07/2025 à 15:25, Hendrik Hamerlinck a écrit :
>>> The function spacemit_ccu_reset_register() allocates memory for an
>>> auxiliary device. If auxiliary_device_add() fails, it skips cleanup of
>>> these resources, resulting in leaks.
>>>
>>> Fix this by using the appropriate error handling path.
>>>
>>> Fixes: 988543522ebd ("clk: spacemit: set up reset auxiliary devices")
>>> Signed-off-by: Hendrik Hamerlinck <hendrik.hamerlinck@hammernet.be>
>>> Reviewed-by: Yixun Lan <dlan@gentoo.org>
>>> ---
>>> Changes in v2:
>>> - Properly place the Fixes tip.
>>> ---
>>>   drivers/clk/spacemit/ccu-k1.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu- k1.c
>>> index 65e6de030717..5bb85e32c6cf 100644
>>> --- a/drivers/clk/spacemit/ccu-k1.c
>>> +++ b/drivers/clk/spacemit/ccu-k1.c
>>> @@ -1059,7 +1059,7 @@ static int spacemit_ccu_reset_register(struct device *dev,
>>>       ret = auxiliary_device_add(adev);
>>>       if (ret) {
>>>           auxiliary_device_uninit(adev);
>>> -        return ret;
>>> +        goto err_free_aux_id;
>>>       }
>>>       return devm_add_action_or_reset(dev, spacemit_adev_unregister, adev);
>>
>>
>> Hi,
>>
>> I'm not sure this is correct.
>
> I'm sure this patch is incorrect, and the original code did
> not have the stated problem.  Thank you for calling attention
> to this Christophe.
>
> Stephen, could you please revert your commit of this patch?
>
> Thank you.
>
>                     -Alex
>
My apologies, I am terribly sorry for this mistake. I was convinced that
this was a leak but was not thorough enough in checking it.

Thank you for catching this issue in time. I will make sure to be more
careful in the future. Sorry for your troubles on this.

Kind regards,
Hendrik

Re: [PATCH v2] clk: spacemit: fix resource leak in spacemit_ccu_reset_register

Posted by Yixun Lan 2 months, 1 week ago

Hi Christophe, 

On 11:16 Sat 26 Jul     , Christophe JAILLET wrote:
> Le 23/07/2025 à 15:25, Hendrik Hamerlinck a écrit :
> > The function spacemit_ccu_reset_register() allocates memory for an
> > auxiliary device. If auxiliary_device_add() fails, it skips cleanup of
> > these resources, resulting in leaks.
> > 
> > Fix this by using the appropriate error handling path.
> > 
> > Fixes: 988543522ebd ("clk: spacemit: set up reset auxiliary devices")
> > Signed-off-by: Hendrik Hamerlinck <hendrik.hamerlinck@hammernet.be>
> > Reviewed-by: Yixun Lan <dlan@gentoo.org>
> > ---
> > Changes in v2:
> > - Properly place the Fixes tip.
> > ---
> >   drivers/clk/spacemit/ccu-k1.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c
> > index 65e6de030717..5bb85e32c6cf 100644
> > --- a/drivers/clk/spacemit/ccu-k1.c
> > +++ b/drivers/clk/spacemit/ccu-k1.c
> > @@ -1059,7 +1059,7 @@ static int spacemit_ccu_reset_register(struct device *dev,
> >   	ret = auxiliary_device_add(adev);
> >   	if (ret) {
> >   		auxiliary_device_uninit(adev);
> > -		return ret;
> > +		goto err_free_aux_id;
> >   	}
> >   
> >   	return devm_add_action_or_reset(dev, spacemit_adev_unregister, adev);
> 
> 
> Hi,
> 
> I'm not sure this is correct.
> 
> auxiliary_device_uninit() which is called if auxiliary_device_add() 
> fails should already do the clean-up.
> 
thanks for your review,

auxiliary_device_uninit() call put_device() and will trigger release callback,
which then call spacemit_cadev_release() and do the ida_free()

> So I would say that this patch adds a double-free issue and should not 
> be applied.
> 
I agree

> If I'm right, note that Stephen has already added a "Applied to clk-next"
> 
Stephen, can you drop this patch? or simply revert it if too late..

> CJ
> 
> 

-- 
Yixun Lan (dlan)

Re: [PATCH v2] clk: spacemit: fix resource leak in spacemit_ccu_reset_register

Posted by Stephen Boyd 2 months, 1 week ago

Quoting Hendrik Hamerlinck (2025-07-23 06:25:04)
> The function spacemit_ccu_reset_register() allocates memory for an
> auxiliary device. If auxiliary_device_add() fails, it skips cleanup of
> these resources, resulting in leaks.
> 
> Fix this by using the appropriate error handling path.
> 
> Fixes: 988543522ebd ("clk: spacemit: set up reset auxiliary devices")
> Signed-off-by: Hendrik Hamerlinck <hendrik.hamerlinck@hammernet.be>
> Reviewed-by: Yixun Lan <dlan@gentoo.org>
> ---

Applied to clk-next