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

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

[Hendrik Hamerlinck]

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

[Stephen Boyd]

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

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

[Christophe JAILLET]

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

[Yixun Lan]

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

[Alex Elder]

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

[Hendrik Hamerlinck]

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