diff mbox series

[1/3] thermal: of: Fix OF node leak in thermal_of_trips_init() error path

Message ID 20240814195823.437597-1-krzysztof.kozlowski@linaro.org
State Accepted
Commit afc954fd223ded70b1fa000767e2531db55cce58
Headers show
Series [1/3] thermal: of: Fix OF node leak in thermal_of_trips_init() error path | expand

Commit Message

Krzysztof Kozlowski Aug. 14, 2024, 7:58 p.m. UTC 
Terminating for_each_child_of_node() loop requires dropping OF node
reference, so bailing out after thermal_of_populate_trip() error misses
this.  Solve the OF node reference leak with scoped
for_each_child_of_node_scoped().

Fixes: d0c75fa2c17f ("thermal/of: Initialize trip points separately")
Cc: <stable@vger.kernel.org>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/thermal/thermal_of.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Chen-Yu Tsai Aug. 15, 2024, 4:48 a.m. UTC | #1 
On Wed, Aug 14, 2024 at 09:58:21PM +0200, Krzysztof Kozlowski wrote:
> Terminating for_each_child_of_node() loop requires dropping OF node
> reference, so bailing out after thermal_of_populate_trip() error misses
> this.  Solve the OF node reference leak with scoped
> for_each_child_of_node_scoped().
> 
> Fixes: d0c75fa2c17f ("thermal/of: Initialize trip points separately")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>

> ---
>  drivers/thermal/thermal_of.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
> index aa34b6e82e26..30f8d6e70484 100644
> --- a/drivers/thermal/thermal_of.c
> +++ b/drivers/thermal/thermal_of.c
> @@ -125,7 +125,7 @@ static int thermal_of_populate_trip(struct device_node *np,
>  static struct thermal_trip *thermal_of_trips_init(struct device_node *np, int *ntrips)
>  {
>  	struct thermal_trip *tt;
> -	struct device_node *trips, *trip;
> +	struct device_node *trips;
>  	int ret, count;
>  
>  	trips = of_get_child_by_name(np, "trips");
> @@ -150,7 +150,7 @@ static struct thermal_trip *thermal_of_trips_init(struct device_node *np, int *n
>  	*ntrips = count;
>  
>  	count = 0;
> -	for_each_child_of_node(trips, trip) {
> +	for_each_child_of_node_scoped(trips, trip) {
>  		ret = thermal_of_populate_trip(trip, &tt[count++]);
>  		if (ret)
>  			goto out_kfree;
> -- 
> 2.43.0
>
Chen-Yu Tsai Aug. 15, 2024, 4:51 a.m. UTC | #2 
On Wed, Aug 14, 2024 at 09:58:22PM +0200, Krzysztof Kozlowski wrote:
> thermal_of_zone_register() calls of_thermal_zone_find() which will
> iterate over OF nodes with for_each_available_child_of_node() to find
> matching thermal zone node.  When it finds such, it exits the loop and
> returns the node.  Prematurely ending for_each_available_child_of_node()
> loops requires dropping OF node reference, thus success of
> of_thermal_zone_find() means that caller must drop the reference.
> 
> Fixes: 3fd6d6e2b4e8 ("thermal/of: Rework the thermal device tree initialization")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>

> ---
>  drivers/thermal/thermal_of.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
> index 30f8d6e70484..b08a9b64718d 100644
> --- a/drivers/thermal/thermal_of.c
> +++ b/drivers/thermal/thermal_of.c
> @@ -491,7 +491,8 @@ static struct thermal_zone_device *thermal_of_zone_register(struct device_node *
>  	trips = thermal_of_trips_init(np, &ntrips);
>  	if (IS_ERR(trips)) {
>  		pr_err("Failed to find trip points for %pOFn id=%d\n", sensor, id);
> -		return ERR_CAST(trips);
> +		ret = PTR_ERR(trips);
> +		goto out_of_node_put;
>  	}
>  
>  	ret = thermal_of_monitor_init(np, &delay, &pdelay);
> @@ -519,6 +520,7 @@ static struct thermal_zone_device *thermal_of_zone_register(struct device_node *
>  		goto out_kfree_trips;
>  	}
>  
> +	of_node_put(np);
>  	kfree(trips);
>  
>  	ret = thermal_zone_device_enable(tz);
> @@ -533,6 +535,8 @@ static struct thermal_zone_device *thermal_of_zone_register(struct device_node *
>  
>  out_kfree_trips:
>  	kfree(trips);
> +out_of_node_put:
> +	of_node_put(np);
>  
>  	return ERR_PTR(ret);
>  }
> -- 
> 2.43.0
>
Markus Elfring Aug. 15, 2024, 6:48 a.m. UTC | #3 
> Terminating for_each_child_of_node() loop requires dropping OF node
…

Can a cover letter be helpful for such patch series?

Regards,
Markus
Daniel Lezcano Aug. 19, 2024, 10:12 a.m. UTC | #4 
On 14/08/2024 21:58, Krzysztof Kozlowski wrote:
> Terminating for_each_child_of_node() loop requires dropping OF node
> reference, so bailing out after thermal_of_populate_trip() error misses
> this.  Solve the OF node reference leak with scoped
> for_each_child_of_node_scoped().
> 
> Fixes: d0c75fa2c17f ("thermal/of: Initialize trip points separately")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---

Applied, thanks for the fixes
Rafael J. Wysocki Aug. 19, 2024, 1:20 p.m. UTC | #5 
On Mon, Aug 19, 2024 at 12:12 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 14/08/2024 21:58, Krzysztof Kozlowski wrote:
> > Terminating for_each_child_of_node() loop requires dropping OF node
> > reference, so bailing out after thermal_of_populate_trip() error misses
> > this.  Solve the OF node reference leak with scoped
> > for_each_child_of_node_scoped().
> >
> > Fixes: d0c75fa2c17f ("thermal/of: Initialize trip points separately")
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > ---
>
> Applied, thanks for the fixes

Is there a place from which I can pull these?

It would be good to include them into 6.11 as they are -stable material.

Alternatively, I can pick them up from the list.
Daniel Lezcano Aug. 19, 2024, 1:22 p.m. UTC | #6 
On 19/08/2024 15:20, Rafael J. Wysocki wrote:
> On Mon, Aug 19, 2024 at 12:12 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 14/08/2024 21:58, Krzysztof Kozlowski wrote:
>>> Terminating for_each_child_of_node() loop requires dropping OF node
>>> reference, so bailing out after thermal_of_populate_trip() error misses
>>> this.  Solve the OF node reference leak with scoped
>>> for_each_child_of_node_scoped().
>>>
>>> Fixes: d0c75fa2c17f ("thermal/of: Initialize trip points separately")
>>> Cc: <stable@vger.kernel.org>
>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>> ---
>>
>> Applied, thanks for the fixes
> 
> Is there a place from which I can pull these?
> 
> It would be good to include them into 6.11 as they are -stable material.
> 
> Alternatively, I can pick them up from the list.

I'll send a PR for fixes only. Let me double check if there are other 
fixes to go along with those
Rafael J. Wysocki Aug. 19, 2024, 1:31 p.m. UTC | #7 
On Mon, Aug 19, 2024 at 3:22 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 19/08/2024 15:20, Rafael J. Wysocki wrote:
> > On Mon, Aug 19, 2024 at 12:12 PM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> On 14/08/2024 21:58, Krzysztof Kozlowski wrote:
> >>> Terminating for_each_child_of_node() loop requires dropping OF node
> >>> reference, so bailing out after thermal_of_populate_trip() error misses
> >>> this.  Solve the OF node reference leak with scoped
> >>> for_each_child_of_node_scoped().
> >>>
> >>> Fixes: d0c75fa2c17f ("thermal/of: Initialize trip points separately")
> >>> Cc: <stable@vger.kernel.org>
> >>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >>> ---
> >>
> >> Applied, thanks for the fixes
> >
> > Is there a place from which I can pull these?
> >
> > It would be good to include them into 6.11 as they are -stable material.
> >
> > Alternatively, I can pick them up from the list.
>
> I'll send a PR for fixes only. Let me double check if there are other
> fixes to go along with those

Sure, thanks!
Rafael J. Wysocki Aug. 22, 2024, 3:54 p.m. UTC | #8 
On Mon, Aug 19, 2024 at 3:31 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Mon, Aug 19, 2024 at 3:22 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
> >
> > On 19/08/2024 15:20, Rafael J. Wysocki wrote:
> > > On Mon, Aug 19, 2024 at 12:12 PM Daniel Lezcano
> > > <daniel.lezcano@linaro.org> wrote:
> > >>
> > >> On 14/08/2024 21:58, Krzysztof Kozlowski wrote:
> > >>> Terminating for_each_child_of_node() loop requires dropping OF node
> > >>> reference, so bailing out after thermal_of_populate_trip() error misses
> > >>> this.  Solve the OF node reference leak with scoped
> > >>> for_each_child_of_node_scoped().
> > >>>
> > >>> Fixes: d0c75fa2c17f ("thermal/of: Initialize trip points separately")
> > >>> Cc: <stable@vger.kernel.org>
> > >>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > >>> ---
> > >>
> > >> Applied, thanks for the fixes
> > >
> > > Is there a place from which I can pull these?
> > >
> > > It would be good to include them into 6.11 as they are -stable material.
> > >
> > > Alternatively, I can pick them up from the list.
> >
> > I'll send a PR for fixes only. Let me double check if there are other
> > fixes to go along with those
>
> Sure, thanks!

Sorry for pressing, but it would be good to get this material into -rc5.

I can still pick up the patches, I don't believe they are controversial.
Daniel Lezcano Aug. 22, 2024, 4:09 p.m. UTC | #9 
On 22/08/2024 17:54, Rafael J. Wysocki wrote:
> On Mon, Aug 19, 2024 at 3:31 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>>
>> On Mon, Aug 19, 2024 at 3:22 PM Daniel Lezcano
>> <daniel.lezcano@linaro.org> wrote:
>>>
>>> On 19/08/2024 15:20, Rafael J. Wysocki wrote:
>>>> On Mon, Aug 19, 2024 at 12:12 PM Daniel Lezcano
>>>> <daniel.lezcano@linaro.org> wrote:
>>>>>
>>>>> On 14/08/2024 21:58, Krzysztof Kozlowski wrote:
>>>>>> Terminating for_each_child_of_node() loop requires dropping OF node
>>>>>> reference, so bailing out after thermal_of_populate_trip() error misses
>>>>>> this.  Solve the OF node reference leak with scoped
>>>>>> for_each_child_of_node_scoped().
>>>>>>
>>>>>> Fixes: d0c75fa2c17f ("thermal/of: Initialize trip points separately")
>>>>>> Cc: <stable@vger.kernel.org>
>>>>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>>>> ---
>>>>>
>>>>> Applied, thanks for the fixes
>>>>
>>>> Is there a place from which I can pull these?
>>>>
>>>> It would be good to include them into 6.11 as they are -stable material.
>>>>
>>>> Alternatively, I can pick them up from the list.
>>>
>>> I'll send a PR for fixes only. Let me double check if there are other
>>> fixes to go along with those
>>
>> Sure, thanks!
> 
> Sorry for pressing, but it would be good to get this material into -rc5.
> 
> I can still pick up the patches, I don't believe they are controversial.

Ok, feel free to pick them. Give me a few minutes to add my tags
Daniel Lezcano Aug. 22, 2024, 4:09 p.m. UTC | #10 
On 14/08/2024 21:58, Krzysztof Kozlowski wrote:
> Terminating for_each_child_of_node() loop requires dropping OF node
> reference, so bailing out after thermal_of_populate_trip() error misses
> this.  Solve the OF node reference leak with scoped
> for_each_child_of_node_scoped().
> 
> Fixes: d0c75fa2c17f ("thermal/of: Initialize trip points separately")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
diff mbox series

Patch

diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
index aa34b6e82e26..30f8d6e70484 100644
--- a/drivers/thermal/thermal_of.c
+++ b/drivers/thermal/thermal_of.c
@@ -125,7 +125,7 @@  static int thermal_of_populate_trip(struct device_node *np,
 static struct thermal_trip *thermal_of_trips_init(struct device_node *np, int *ntrips)
 {
 	struct thermal_trip *tt;
-	struct device_node *trips, *trip;
+	struct device_node *trips;
 	int ret, count;
 
 	trips = of_get_child_by_name(np, "trips");
@@ -150,7 +150,7 @@  static struct thermal_trip *thermal_of_trips_init(struct device_node *np, int *n
 	*ntrips = count;
 
 	count = 0;
-	for_each_child_of_node(trips, trip) {
+	for_each_child_of_node_scoped(trips, trip) {
 		ret = thermal_of_populate_trip(trip, &tt[count++]);
 		if (ret)
 			goto out_kfree;