[PATCH 0/2] cpuidle: qcom-spm: fix resource release in spm_cpuidle_register

[PATCH 0/2] cpuidle: qcom-spm: fix resource release in spm_cpuidle_register

[Javier Carrasco]

This series addresses two issues in spm_cpuidle_register: a missing
device_node release in an error path, and releasing a reference to a
device after its usage.

These issues were found while analyzing the code, and the patches have
been successfully compiled and statically analyzed, but not tested on
real hardware as I don't have access to it. Any volunteering for testing
is always more than welcome.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
Javier Carrasco (2):
      cpuidle: qcom-spm: fix device node release in spm_cpuidle_register
      cpuidle: qcom-spm: fix platform device release in spm_cpuidle_register

 drivers/cpuidle/cpuidle-qcom-spm.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
---
base-commit: 6fb2fa9805c501d9ade047fc511961f3273cdcb5
change-id: 20241029-cpuidle-qcom-spm-cleanup-6f03669bcd70

Best regards,
-- 
Javier Carrasco <javier.carrasco.cruz@gmail.com>

[PATCH 1/2] cpuidle: qcom-spm: fix device node release in spm_cpuidle_register

[Javier Carrasco]

If of_find_device_by_node() fails, its error path does not include a
call to of_node_put(cpu_node), which has been successfully acquired at
this point.

Move the existing of_node_put(cpu_node) to the point where cpu_node is
no longer required, covering all code paths and avoiding leaking the
resource in any case.

Cc: stable@vger.kernel.org
Fixes: 60f3692b5f0b ("cpuidle: qcom_spm: Detach state machine from main SPM handling")
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/cpuidle/cpuidle-qcom-spm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpuidle/cpuidle-qcom-spm.c b/drivers/cpuidle/cpuidle-qcom-spm.c
index 3ab240e0e122..c9ab49b310fd 100644
--- a/drivers/cpuidle/cpuidle-qcom-spm.c
+++ b/drivers/cpuidle/cpuidle-qcom-spm.c
@@ -96,12 +96,12 @@ static int spm_cpuidle_register(struct device *cpuidle_dev, int cpu)
 		return -ENODEV;
 
 	saw_node = of_parse_phandle(cpu_node, "qcom,saw", 0);
+	of_node_put(cpu_node);
 	if (!saw_node)
 		return -ENODEV;
 
 	pdev = of_find_device_by_node(saw_node);
 	of_node_put(saw_node);
-	of_node_put(cpu_node);
 	if (!pdev)
 		return -ENODEV;
 

-- 
2.43.0

[PATCH 2/2] cpuidle: qcom-spm: fix platform device release in spm_cpuidle_register

[Javier Carrasco]

A reference to a device obtained via of_find_device_by_node() requires
explicit calls to put_device() when it is no longer required to avoid
leaking the resource.

Add the missing calls to put_device(&pdev->dev) in the success path as
well as in the only error path before the device is no longer required.

Note that the acquired device is neither assigned nor used to manage
additional resources, and it can be released right after using it.

Cc: stable@vger.kernel.org
Fixes: 60f3692b5f0b ("cpuidle: qcom_spm: Detach state machine from main SPM handling")
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/cpuidle/cpuidle-qcom-spm.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/cpuidle/cpuidle-qcom-spm.c b/drivers/cpuidle/cpuidle-qcom-spm.c
index c9ab49b310fd..601aa81ffff3 100644
--- a/drivers/cpuidle/cpuidle-qcom-spm.c
+++ b/drivers/cpuidle/cpuidle-qcom-spm.c
@@ -106,10 +106,13 @@ static int spm_cpuidle_register(struct device *cpuidle_dev, int cpu)
 		return -ENODEV;
 
 	data = devm_kzalloc(cpuidle_dev, sizeof(*data), GFP_KERNEL);
-	if (!data)
+	if (!data) {
+		put_device(&pdev->dev);
 		return -ENOMEM;
+	}
 
 	data->spm = dev_get_drvdata(&pdev->dev);
+	put_device(&pdev->dev);
 	if (!data->spm)
 		return -EINVAL;
 

-- 
2.43.0

Re: [PATCH 1/2] cpuidle: qcom-spm: fix device node release in spm_cpuidle_register

[Dmitry Baryshkov]

On Wed, Oct 30, 2024 at 07:38:32AM +0100, Javier Carrasco wrote:
> If of_find_device_by_node() fails, its error path does not include a
> call to of_node_put(cpu_node), which has been successfully acquired at
> this point.
> 
> Move the existing of_node_put(cpu_node) to the point where cpu_node is
> no longer required, covering all code paths and avoiding leaking the
> resource in any case.
> 
> Cc: stable@vger.kernel.org
> Fixes: 60f3692b5f0b ("cpuidle: qcom_spm: Detach state machine from main SPM handling")
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
>  drivers/cpuidle/cpuidle-qcom-spm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 


Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH 2/2] cpuidle: qcom-spm: fix platform device release in spm_cpuidle_register

[Dmitry Baryshkov]

On Wed, Oct 30, 2024 at 07:38:33AM +0100, Javier Carrasco wrote:
> A reference to a device obtained via of_find_device_by_node() requires
> explicit calls to put_device() when it is no longer required to avoid
> leaking the resource.
> 
> Add the missing calls to put_device(&pdev->dev) in the success path as
> well as in the only error path before the device is no longer required.
> 
> Note that the acquired device is neither assigned nor used to manage
> additional resources, and it can be released right after using it.

Well... This raises one question: if the device is put, then its drvdata
can go away. But at the same time the drvdata can also go away if the
SPM device is just unbound from the driver. Granted the age of those
platforms it's probably not worth refactoring the drivers too much.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> 
> Cc: stable@vger.kernel.org
> Fixes: 60f3692b5f0b ("cpuidle: qcom_spm: Detach state machine from main SPM handling")
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
>  drivers/cpuidle/cpuidle-qcom-spm.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpuidle/cpuidle-qcom-spm.c b/drivers/cpuidle/cpuidle-qcom-spm.c
> index c9ab49b310fd..601aa81ffff3 100644
> --- a/drivers/cpuidle/cpuidle-qcom-spm.c
> +++ b/drivers/cpuidle/cpuidle-qcom-spm.c
> @@ -106,10 +106,13 @@ static int spm_cpuidle_register(struct device *cpuidle_dev, int cpu)
>  		return -ENODEV;
>  
>  	data = devm_kzalloc(cpuidle_dev, sizeof(*data), GFP_KERNEL);
> -	if (!data)
> +	if (!data) {
> +		put_device(&pdev->dev);
>  		return -ENOMEM;
> +	}
>  
>  	data->spm = dev_get_drvdata(&pdev->dev);
> +	put_device(&pdev->dev);
>  	if (!data->spm)
>  		return -EINVAL;
>  
> 
> -- 
> 2.43.0
> 

-- 
With best wishes
Dmitry