[PATCH v1 0/3] thermal/debugfs: Fix a memory leak on removal and locking

[PATCH v1 0/3] thermal/debugfs: Fix a memory leak on removal and locking

[Rafael J. Wysocki]

Hi Everyone,

This series fixes a memory leak on thermal zone removal caused by the
thermal debug code and two locking issues that may cause the kernel to
crash due to race conditions.

Please review urgently.

The series applies on top of the linux-next branch in linux-pm.git.

It is also present in the thermal-core-next branch in that tree.

Thanks!

[PATCH v1 1/3] thermal/debugfs: Free all thermal zone debug memory on zone removal

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Because thermal_debug_tz_remove() does not free all memory allocated for
thermal zone diagnostics, some of that memory becomes unreachable after
freeing the thermal zone's struct thermal_debugfs object.

Address this by making thermal_debug_tz_remove() free all of the memory
in question.

Fixes: 7ef01f228c9f ("thermal/debugfs: Add thermal debugfs information for mitigation episodes")
Cc :6.8+ <stable@vger.kernel.org> # 6.8+
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/thermal_debugfs.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

Index: linux-pm/drivers/thermal/thermal_debugfs.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_debugfs.c
+++ linux-pm/drivers/thermal/thermal_debugfs.c
@@ -832,15 +832,28 @@ void thermal_debug_tz_add(struct thermal
 void thermal_debug_tz_remove(struct thermal_zone_device *tz)
 {
 	struct thermal_debugfs *thermal_dbg = tz->debugfs;
+	struct tz_episode *tze, *tmp;
+	struct tz_debugfs *tz_dbg;
+	int *trips_crossed;
 
 	if (!thermal_dbg)
 		return;
 
+	tz_dbg = &thermal_dbg->tz_dbg;
+
 	mutex_lock(&thermal_dbg->lock);
 
+	trips_crossed = tz_dbg->trips_crossed;
+
+	list_for_each_entry_safe(tze, tmp, &tz_dbg->tz_episodes, node) {
+		list_del(&tze->node);
+		kfree(tze);
+	}
+
 	tz->debugfs = NULL;
 
 	mutex_unlock(&thermal_dbg->lock);
 
 	thermal_debugfs_remove_id(thermal_dbg);
+	kfree(trips_crossed);
 }

[PATCH v1 2/3] thermal/debugfs: Fix thermal zone locking

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

With the current thermal zone locking arrangement in the debugfs code,
user space can open the "mitigations" file for a thermal zone before
the zone's debugfs pointer is set which will result in a NULL pointer
dereference in tze_seq_start().

While this could be addressed by putting the creation of the
"mitigations" file under thermal_dbg->lock, there is still a problem
with thermal_debug_tz_remove() that is not called under the thermal
zone lock and can run in parallel with the other functions accessing
the thermal zone's struct thermal_debugfs object.  Then, it may
clear tz->debugfs after one of those functions has checked it and the
struct thermal_debugfs object may be freed prematurely.

To address both problems described above at once, use the observation
that thermal_debug_tz_trip_up(), thermal_debug_tz_trip_down(), and
thermal_debug_update_trip_stats() all run under the thermal zone
lock and because they all acquire thermal_dbg->lock for the thermal
zone's struct thermal_debugfs object, they must wait on that lock if it
is held while they are running and their callers (holding the thermal
zone lock) must wait along with them.  This means that tze_seq_start()
may as well acquire tz->lock instead of thermal_dbg->lock and check the
struct thermal_debugfs object pointer retrieved from the thermal zone
against NULL under it.

Then, tz->lock can also be acquired by thermal_debug_tz_add() and
thermal_debug_tz_remove() to eliminate the race conditions at hand.

Rearrange the code in question accordingly and remove the
thermal_dbg->lock locking, which is now redundant, from it.

Fixes: 7ef01f228c9f ("thermal/debugfs: Add thermal debugfs information for mitigation episodes")
Cc :6.8+ <stable@vger.kernel.org> # 6.8+
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/thermal_debugfs.c |   60 +++++++++++++++++---------------------
 1 file changed, 28 insertions(+), 32 deletions(-)

Index: linux-pm/drivers/thermal/thermal_debugfs.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_debugfs.c
+++ linux-pm/drivers/thermal/thermal_debugfs.c
@@ -551,8 +551,6 @@ void thermal_debug_tz_trip_up(struct the
 	if (!thermal_dbg)
 		return;
 
-	mutex_lock(&thermal_dbg->lock);
-
 	tz_dbg = &thermal_dbg->tz_dbg;
 
 	/*
@@ -591,7 +589,7 @@ void thermal_debug_tz_trip_up(struct the
 	if (!tz_dbg->nr_trips) {
 		tze = thermal_debugfs_tz_event_alloc(tz, now);
 		if (!tze)
-			goto unlock;
+			return;
 
 		list_add(&tze->node, &tz_dbg->tz_episodes);
 	}
@@ -613,9 +611,6 @@ void thermal_debug_tz_trip_up(struct the
 
 	tze = list_first_entry(&tz_dbg->tz_episodes, struct tz_episode, node);
 	tze->trip_stats[trip_id].timestamp = now;
-
-unlock:
-	mutex_unlock(&thermal_dbg->lock);
 }
 
 void thermal_debug_tz_trip_down(struct thermal_zone_device *tz,
@@ -631,8 +626,6 @@ void thermal_debug_tz_trip_down(struct t
 	if (!thermal_dbg)
 		return;
 
-	mutex_lock(&thermal_dbg->lock);
-
 	tz_dbg = &thermal_dbg->tz_dbg;
 
 	/*
@@ -643,7 +636,7 @@ void thermal_debug_tz_trip_down(struct t
 	 * no mitigation mechanism yet at boot time.
 	 */
 	if (!tz_dbg->nr_trips)
-		goto out;
+		return;
 
 	for (i = tz_dbg->nr_trips - 1; i >= 0; i--) {
 		if (tz_dbg->trips_crossed[i] == trip_id)
@@ -651,7 +644,7 @@ void thermal_debug_tz_trip_down(struct t
 	}
 
 	if (i < 0)
-		goto out;
+		return;
 
 	tz_dbg->nr_trips--;
 
@@ -671,9 +664,6 @@ void thermal_debug_tz_trip_down(struct t
 	 */
 	if (!tz_dbg->nr_trips)
 		tze->duration = ktime_sub(now, tze->timestamp);
-
-out:
-	mutex_unlock(&thermal_dbg->lock);
 }
 
 void thermal_debug_update_trip_stats(struct thermal_zone_device *tz)
@@ -686,12 +676,10 @@ void thermal_debug_update_trip_stats(str
 	if (!thermal_dbg)
 		return;
 
-	mutex_lock(&thermal_dbg->lock);
-
 	tz_dbg = &thermal_dbg->tz_dbg;
 
 	if (!tz_dbg->nr_trips)
-		goto out;
+		return;
 
 	tze = list_first_entry(&tz_dbg->tz_episodes, struct tz_episode, node);
 
@@ -704,19 +692,22 @@ void thermal_debug_update_trip_stats(str
 		trip_stats->avg += (tz->temperature - trip_stats->avg) /
 					++trip_stats->count;
 	}
-out:
-	mutex_unlock(&thermal_dbg->lock);
 }
 
 static void *tze_seq_start(struct seq_file *s, loff_t *pos)
 {
 	struct thermal_zone_device *tz = s->private;
-	struct thermal_debugfs *thermal_dbg = tz->debugfs;
-	struct tz_debugfs *tz_dbg = &thermal_dbg->tz_dbg;
+	struct thermal_debugfs *thermal_dbg;
 
-	mutex_lock(&thermal_dbg->lock);
+	mutex_lock(&tz->lock);
 
-	return seq_list_start(&tz_dbg->tz_episodes, *pos);
+	thermal_dbg = tz->debugfs;
+	if (!thermal_dbg) {
+		mutex_unlock(&tz->lock);
+		return NULL;
+	}
+
+	return seq_list_start(&thermal_dbg->tz_dbg.tz_episodes, *pos);
 }
 
 static void *tze_seq_next(struct seq_file *s, void *v, loff_t *pos)
@@ -731,9 +722,8 @@ static void *tze_seq_next(struct seq_fil
 static void tze_seq_stop(struct seq_file *s, void *v)
 {
 	struct thermal_zone_device *tz = s->private;
-	struct thermal_debugfs *thermal_dbg = tz->debugfs;
 
-	mutex_unlock(&thermal_dbg->lock);
+	mutex_unlock(&tz->lock);
 }
 
 static int tze_seq_show(struct seq_file *s, void *v)
@@ -826,23 +816,33 @@ void thermal_debug_tz_add(struct thermal
 
 	debugfs_create_file("mitigations", 0400, thermal_dbg->d_top, tz, &tze_fops);
 
+	mutex_lock(&tz->lock);
+
 	tz->debugfs = thermal_dbg;
+
+	mutex_unlock(&tz->lock);
 }
 
 void thermal_debug_tz_remove(struct thermal_zone_device *tz)
 {
-	struct thermal_debugfs *thermal_dbg = tz->debugfs;
+	struct thermal_debugfs *thermal_dbg;
 	struct tz_episode *tze, *tmp;
 	struct tz_debugfs *tz_dbg;
 	int *trips_crossed;
 
-	if (!thermal_dbg)
+	mutex_lock(&tz->lock);
+
+	thermal_dbg = tz->debugfs;
+	if (!thermal_dbg) {
+		mutex_unlock(&tz->lock);
 		return;
+	}
 
-	tz_dbg = &thermal_dbg->tz_dbg;
+	tz->debugfs = NULL;
 
-	mutex_lock(&thermal_dbg->lock);
+	mutex_unlock(&tz->lock);
 
+	tz_dbg = &thermal_dbg->tz_dbg;
 	trips_crossed = tz_dbg->trips_crossed;
 
 	list_for_each_entry_safe(tze, tmp, &tz_dbg->tz_episodes, node) {
@@ -850,10 +850,6 @@ void thermal_debug_tz_remove(struct ther
 		kfree(tze);
 	}
 
-	tz->debugfs = NULL;
-
-	mutex_unlock(&thermal_dbg->lock);
-
 	thermal_debugfs_remove_id(thermal_dbg);
 	kfree(trips_crossed);
 }

[PATCH v1 3/3] thermal/debugfs: Prevent use-after-free from occurring after cdev removal

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Since thermal_debug_cdev_remove() does not run under cdev->lock, it can
run in parallel with thermal_debug_cdev_state_update() and it may free
the struct thermal_debugfs object used by the latter after it has been
checked against NULL.

If that happens, thermal_debug_cdev_state_update() will access memory
that has been freed already causing the kernel to crash.

Address this by using cdev->lock in thermal_debug_cdev_remove() around
the cdev->debugfs value check (in case the same cdev is removed at the
same time in two differet threads) and its reset to NULL.

Fixes: 755113d76786 ("thermal/debugfs: Add thermal cooling device debugfs information")
Cc :6.8+ <stable@vger.kernel.org> # 6.8+
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/thermal_debugfs.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Index: linux-pm/drivers/thermal/thermal_debugfs.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_debugfs.c
+++ linux-pm/drivers/thermal/thermal_debugfs.c
@@ -503,15 +503,21 @@ void thermal_debug_cdev_add(struct therm
  */
 void thermal_debug_cdev_remove(struct thermal_cooling_device *cdev)
 {
-	struct thermal_debugfs *thermal_dbg = cdev->debugfs;
+	struct thermal_debugfs *thermal_dbg;
 
+	mutex_lock(&cdev->lock);
+
+	thermal_dbg = cdev->debugfs;
 	if (!thermal_dbg)
 		return;
 
+	cdev->debugfs = NULL;
+
+	mutex_unlock(&cdev->lock);
+
 	mutex_lock(&thermal_dbg->lock);
 
 	thermal_debugfs_cdev_clear(&thermal_dbg->cdev_dbg);
-	cdev->debugfs = NULL;
 
 	mutex_unlock(&thermal_dbg->lock);
 

[Alternative][PATCH v1 2/3] thermal/debugfs: Fix two locking issues with thermal zone debug

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

With the current thermal zone locking arrangement in the debugfs code,
user space can open the "mitigations" file for a thermal zone before
the zone's debugfs pointer is set which will result in a NULL pointer
dereference in tze_seq_start().

Moreover, thermal_debug_tz_remove() is not called under the thermal
zone lock, so can run in parallel with the other functions accessing
the thermal zone's struct thermal_debugfs object.  Then, it may clear
tz->debugfs after one of those functions has checked it and the
struct thermal_debugfs object may be freed prematurely.

To address the first problem, pass a pointer to the thermal zone's
struct thermal_debugfs object to debugfs_create_file() in
thermal_debug_tz_add() and make tze_seq_start(), tze_seq_next(),
tze_seq_stop(), and tze_seq_show() retrieve it from s->private
instead of a pointer to the thermal zone object.  This will ensure
that tz_debugfs will be valid across the "mitigations" file accesses
until thermal_debugfs_remove_id() called by thermal_debug_tz_remove()
removes that file.

To address the second problem, use tz->lock in thermal_debug_tz_remove()
around the tz->debugfs value check (in case the same thermal zone is
removed at the same time in two differet threads) and its reset to NULL.

Fixes: 7ef01f228c9f ("thermal/debugfs: Add thermal debugfs information for mitigation episodes")
Cc :6.8+ <stable@vger.kernel.org> # 6.8+
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

This is an alternative fix for the issues addressed by

https://lore.kernel.org/linux-pm/1888579.tdWV9SEqCh@kreacher/

and I slightly prefer it, because it is less intrusive and makes
the thermal zone debug code more consistent with the analogous code
for cdevs.

Accordingly, I've replace the above with this patch in the
thermal-core-next branch in linux-pm.git.

---
 drivers/thermal/thermal_debugfs.c |   34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

Index: linux-pm/drivers/thermal/thermal_debugfs.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_debugfs.c
+++ linux-pm/drivers/thermal/thermal_debugfs.c
@@ -139,11 +139,13 @@ struct tz_episode {
  * we keep track of the current position in the history array.
  *
  * @tz_episodes: a list of thermal mitigation episodes
+ * @tz: thermal zone this object belongs to
  * @trips_crossed: an array of trip points crossed by id
  * @nr_trips: the number of trip points currently being crossed
  */
 struct tz_debugfs {
 	struct list_head tz_episodes;
+	struct thermal_zone_device *tz;
 	int *trips_crossed;
 	int nr_trips;
 };
@@ -710,8 +712,7 @@ out:
 
 static void *tze_seq_start(struct seq_file *s, loff_t *pos)
 {
-	struct thermal_zone_device *tz = s->private;
-	struct thermal_debugfs *thermal_dbg = tz->debugfs;
+	struct thermal_debugfs *thermal_dbg = s->private;
 	struct tz_debugfs *tz_dbg = &thermal_dbg->tz_dbg;
 
 	mutex_lock(&thermal_dbg->lock);
@@ -721,8 +722,7 @@ static void *tze_seq_start(struct seq_fi
 
 static void *tze_seq_next(struct seq_file *s, void *v, loff_t *pos)
 {
-	struct thermal_zone_device *tz = s->private;
-	struct thermal_debugfs *thermal_dbg = tz->debugfs;
+	struct thermal_debugfs *thermal_dbg = s->private;
 	struct tz_debugfs *tz_dbg = &thermal_dbg->tz_dbg;
 
 	return seq_list_next(v, &tz_dbg->tz_episodes, pos);
@@ -730,15 +730,15 @@ static void *tze_seq_next(struct seq_fil
 
 static void tze_seq_stop(struct seq_file *s, void *v)
 {
-	struct thermal_zone_device *tz = s->private;
-	struct thermal_debugfs *thermal_dbg = tz->debugfs;
+	struct thermal_debugfs *thermal_dbg = s->private;
 
 	mutex_unlock(&thermal_dbg->lock);
 }
 
 static int tze_seq_show(struct seq_file *s, void *v)
 {
-	struct thermal_zone_device *tz = s->private;
+	struct thermal_debugfs *thermal_dbg = s->private;
+	struct thermal_zone_device *tz = thermal_dbg->tz_dbg.tz;
 	struct thermal_trip_desc *td;
 	struct tz_episode *tze;
 	const char *type;
@@ -816,6 +816,8 @@ void thermal_debug_tz_add(struct thermal
 
 	tz_dbg = &thermal_dbg->tz_dbg;
 
+	tz_dbg->tz = tz;
+
 	tz_dbg->trips_crossed = kzalloc(sizeof(int) * tz->num_trips, GFP_KERNEL);
 	if (!tz_dbg->trips_crossed) {
 		thermal_debugfs_remove_id(thermal_dbg);
@@ -824,20 +826,30 @@ void thermal_debug_tz_add(struct thermal
 
 	INIT_LIST_HEAD(&tz_dbg->tz_episodes);
 
-	debugfs_create_file("mitigations", 0400, thermal_dbg->d_top, tz, &tze_fops);
+	debugfs_create_file("mitigations", 0400, thermal_dbg->d_top,
+			    thermal_dbg, &tze_fops);
 
 	tz->debugfs = thermal_dbg;
 }
 
 void thermal_debug_tz_remove(struct thermal_zone_device *tz)
 {
-	struct thermal_debugfs *thermal_dbg = tz->debugfs;
+	struct thermal_debugfs *thermal_dbg;
 	struct tz_episode *tze, *tmp;
 	struct tz_debugfs *tz_dbg;
 	int *trips_crossed;
 
-	if (!thermal_dbg)
+	mutex_lock(&tz->lock);
+
+	thermal_dbg = tz->debugfs;
+	if (!thermal_dbg) {
+		mutex_unlock(&tz->lock);
 		return;
+	}
+
+	tz->debugfs = NULL;
+
+	mutex_unlock(&tz->lock);
 
 	tz_dbg = &thermal_dbg->tz_dbg;
 
@@ -850,8 +862,6 @@ void thermal_debug_tz_remove(struct ther
 		kfree(tze);
 	}
 
-	tz->debugfs = NULL;
-
 	mutex_unlock(&thermal_dbg->lock);
 
 	thermal_debugfs_remove_id(thermal_dbg);

Re: [PATCH v1 1/3] thermal/debugfs: Free all thermal zone debug memory on zone removal

[Lukasz Luba]

On 4/25/24 14:49, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Because thermal_debug_tz_remove() does not free all memory allocated for
> thermal zone diagnostics, some of that memory becomes unreachable after
> freeing the thermal zone's struct thermal_debugfs object.
> 
> Address this by making thermal_debug_tz_remove() free all of the memory
> in question.
> 
> Fixes: 7ef01f228c9f ("thermal/debugfs: Add thermal debugfs information for mitigation episodes")
> Cc :6.8+ <stable@vger.kernel.org> # 6.8+
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/thermal_debugfs.c |   13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> Index: linux-pm/drivers/thermal/thermal_debugfs.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_debugfs.c
> +++ linux-pm/drivers/thermal/thermal_debugfs.c
> @@ -832,15 +832,28 @@ void thermal_debug_tz_add(struct thermal
>   void thermal_debug_tz_remove(struct thermal_zone_device *tz)
>   {
>   	struct thermal_debugfs *thermal_dbg = tz->debugfs;
> +	struct tz_episode *tze, *tmp;
> +	struct tz_debugfs *tz_dbg;
> +	int *trips_crossed;
>   
>   	if (!thermal_dbg)
>   		return;
>   
> +	tz_dbg = &thermal_dbg->tz_dbg;
> +
>   	mutex_lock(&thermal_dbg->lock);
>   
> +	trips_crossed = tz_dbg->trips_crossed;
> +
> +	list_for_each_entry_safe(tze, tmp, &tz_dbg->tz_episodes, node) {
> +		list_del(&tze->node);
> +		kfree(tze);
> +	}
> +
>   	tz->debugfs = NULL;
>   
>   	mutex_unlock(&thermal_dbg->lock);
>   
>   	thermal_debugfs_remove_id(thermal_dbg);
> +	kfree(trips_crossed);
>   }
> 
> 
> 
> 

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Re: [PATCH v1 3/3] thermal/debugfs: Prevent use-after-free from occurring after cdev removal

[Lukasz Luba]

On 4/25/24 14:57, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Since thermal_debug_cdev_remove() does not run under cdev->lock, it can
> run in parallel with thermal_debug_cdev_state_update() and it may free
> the struct thermal_debugfs object used by the latter after it has been
> checked against NULL.
> 
> If that happens, thermal_debug_cdev_state_update() will access memory
> that has been freed already causing the kernel to crash.
> 
> Address this by using cdev->lock in thermal_debug_cdev_remove() around
> the cdev->debugfs value check (in case the same cdev is removed at the
> same time in two differet threads) and its reset to NULL.

s/differet/different/

> 
> Fixes: 755113d76786 ("thermal/debugfs: Add thermal cooling device debugfs information")
> Cc :6.8+ <stable@vger.kernel.org> # 6.8+
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/thermal/thermal_debugfs.c |   10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> Index: linux-pm/drivers/thermal/thermal_debugfs.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_debugfs.c
> +++ linux-pm/drivers/thermal/thermal_debugfs.c
> @@ -503,15 +503,21 @@ void thermal_debug_cdev_add(struct therm
>    */
>   void thermal_debug_cdev_remove(struct thermal_cooling_device *cdev)
>   {
> -	struct thermal_debugfs *thermal_dbg = cdev->debugfs;
> +	struct thermal_debugfs *thermal_dbg;
>   
> +	mutex_lock(&cdev->lock);
> +
> +	thermal_dbg = cdev->debugfs;
>   	if (!thermal_dbg)

mutex_unlock(&cdev->lock) missing here

>   		return;
>   
> +	cdev->debugfs = NULL;
> +
> +	mutex_unlock(&cdev->lock);
> +
>   	mutex_lock(&thermal_dbg->lock);
>   
>   	thermal_debugfs_cdev_clear(&thermal_dbg->cdev_dbg);
> -	cdev->debugfs = NULL;
>   
>   	mutex_unlock(&thermal_dbg->lock);
>   
> 
> 
> 
> 

Re: [Alternative][PATCH v1 2/3] thermal/debugfs: Fix two locking issues with thermal zone debug

[Lukasz Luba]

On 4/25/24 16:47, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> With the current thermal zone locking arrangement in the debugfs code,
> user space can open the "mitigations" file for a thermal zone before
> the zone's debugfs pointer is set which will result in a NULL pointer
> dereference in tze_seq_start().
> 
> Moreover, thermal_debug_tz_remove() is not called under the thermal
> zone lock, so can run in parallel with the other functions accessing
> the thermal zone's struct thermal_debugfs object.  Then, it may clear
> tz->debugfs after one of those functions has checked it and the
> struct thermal_debugfs object may be freed prematurely.
> 
> To address the first problem, pass a pointer to the thermal zone's
> struct thermal_debugfs object to debugfs_create_file() in
> thermal_debug_tz_add() and make tze_seq_start(), tze_seq_next(),
> tze_seq_stop(), and tze_seq_show() retrieve it from s->private
> instead of a pointer to the thermal zone object.  This will ensure
> that tz_debugfs will be valid across the "mitigations" file accesses
> until thermal_debugfs_remove_id() called by thermal_debug_tz_remove()
> removes that file.
> 
> To address the second problem, use tz->lock in thermal_debug_tz_remove()
> around the tz->debugfs value check (in case the same thermal zone is
> removed at the same time in two differet threads) and its reset to NULL.

s/differet/different/

> 
> Fixes: 7ef01f228c9f ("thermal/debugfs: Add thermal debugfs information for mitigation episodes")
> Cc :6.8+ <stable@vger.kernel.org> # 6.8+
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> This is an alternative fix for the issues addressed by
> 
> https://lore.kernel.org/linux-pm/1888579.tdWV9SEqCh@kreacher/
> 
> and I slightly prefer it, because it is less intrusive and makes
> the thermal zone debug code more consistent with the analogous code
> for cdevs.

I also prefer this one.

> 
> Accordingly, I've replace the above with this patch in the
> thermal-core-next branch in linux-pm.git.
> 
> ---
>   drivers/thermal/thermal_debugfs.c |   34 ++++++++++++++++++++++------------
>   1 file changed, 22 insertions(+), 12 deletions(-)
> 
> Index: linux-pm/drivers/thermal/thermal_debugfs.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_debugfs.c
> +++ linux-pm/drivers/thermal/thermal_debugfs.c
> @@ -139,11 +139,13 @@ struct tz_episode {
>    * we keep track of the current position in the history array.
>    *
>    * @tz_episodes: a list of thermal mitigation episodes
> + * @tz: thermal zone this object belongs to
>    * @trips_crossed: an array of trip points crossed by id
>    * @nr_trips: the number of trip points currently being crossed
>    */
>   struct tz_debugfs {
>   	struct list_head tz_episodes;
> +	struct thermal_zone_device *tz;
>   	int *trips_crossed;
>   	int nr_trips;
>   };
> @@ -710,8 +712,7 @@ out:
>   
>   static void *tze_seq_start(struct seq_file *s, loff_t *pos)
>   {
> -	struct thermal_zone_device *tz = s->private;
> -	struct thermal_debugfs *thermal_dbg = tz->debugfs;
> +	struct thermal_debugfs *thermal_dbg = s->private;
>   	struct tz_debugfs *tz_dbg = &thermal_dbg->tz_dbg;
>   
>   	mutex_lock(&thermal_dbg->lock);
> @@ -721,8 +722,7 @@ static void *tze_seq_start(struct seq_fi
>   
>   static void *tze_seq_next(struct seq_file *s, void *v, loff_t *pos)
>   {
> -	struct thermal_zone_device *tz = s->private;
> -	struct thermal_debugfs *thermal_dbg = tz->debugfs;
> +	struct thermal_debugfs *thermal_dbg = s->private;
>   	struct tz_debugfs *tz_dbg = &thermal_dbg->tz_dbg;
>   
>   	return seq_list_next(v, &tz_dbg->tz_episodes, pos);
> @@ -730,15 +730,15 @@ static void *tze_seq_next(struct seq_fil
>   
>   static void tze_seq_stop(struct seq_file *s, void *v)
>   {
> -	struct thermal_zone_device *tz = s->private;
> -	struct thermal_debugfs *thermal_dbg = tz->debugfs;
> +	struct thermal_debugfs *thermal_dbg = s->private;
>   
>   	mutex_unlock(&thermal_dbg->lock);
>   }
>   
>   static int tze_seq_show(struct seq_file *s, void *v)
>   {
> -	struct thermal_zone_device *tz = s->private;
> +	struct thermal_debugfs *thermal_dbg = s->private;
> +	struct thermal_zone_device *tz = thermal_dbg->tz_dbg.tz;
>   	struct thermal_trip_desc *td;
>   	struct tz_episode *tze;
>   	const char *type;
> @@ -816,6 +816,8 @@ void thermal_debug_tz_add(struct thermal
>   
>   	tz_dbg = &thermal_dbg->tz_dbg;
>   
> +	tz_dbg->tz = tz;
> +
>   	tz_dbg->trips_crossed = kzalloc(sizeof(int) * tz->num_trips, GFP_KERNEL);
>   	if (!tz_dbg->trips_crossed) {
>   		thermal_debugfs_remove_id(thermal_dbg);
> @@ -824,20 +826,30 @@ void thermal_debug_tz_add(struct thermal
>   
>   	INIT_LIST_HEAD(&tz_dbg->tz_episodes);
>   
> -	debugfs_create_file("mitigations", 0400, thermal_dbg->d_top, tz, &tze_fops);
> +	debugfs_create_file("mitigations", 0400, thermal_dbg->d_top,
> +			    thermal_dbg, &tze_fops);
>   
>   	tz->debugfs = thermal_dbg;
>   }
>   
>   void thermal_debug_tz_remove(struct thermal_zone_device *tz)
>   {
> -	struct thermal_debugfs *thermal_dbg = tz->debugfs;
> +	struct thermal_debugfs *thermal_dbg;
>   	struct tz_episode *tze, *tmp;
>   	struct tz_debugfs *tz_dbg;
>   	int *trips_crossed;
>   
> -	if (!thermal_dbg)
> +	mutex_lock(&tz->lock);
> +
> +	thermal_dbg = tz->debugfs;
> +	if (!thermal_dbg) {
> +		mutex_unlock(&tz->lock);
>   		return;
> +	}
> +
> +	tz->debugfs = NULL;
> +
> +	mutex_unlock(&tz->lock);
>   
>   	tz_dbg = &thermal_dbg->tz_dbg;
>   
> @@ -850,8 +862,6 @@ void thermal_debug_tz_remove(struct ther
>   		kfree(tze);
>   	}
>   
> -	tz->debugfs = NULL;
> -
>   	mutex_unlock(&thermal_dbg->lock);
>   
>   	thermal_debugfs_remove_id(thermal_dbg);
> 
> 
> 

LGTM, with that minor spelling fixed:

Reviewed-by: Lukasz Luba <lukasz.luba@arm..com>

Re: [PATCH v1 3/3] thermal/debugfs: Prevent use-after-free from occurring after cdev removal

[Rafael J. Wysocki]

On Fri, Apr 26, 2024 at 12:05 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
>
>
> On 4/25/24 14:57, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Since thermal_debug_cdev_remove() does not run under cdev->lock, it can
> > run in parallel with thermal_debug_cdev_state_update() and it may free
> > the struct thermal_debugfs object used by the latter after it has been
> > checked against NULL.
> >
> > If that happens, thermal_debug_cdev_state_update() will access memory
> > that has been freed already causing the kernel to crash.
> >
> > Address this by using cdev->lock in thermal_debug_cdev_remove() around
> > the cdev->debugfs value check (in case the same cdev is removed at the
> > same time in two differet threads) and its reset to NULL.
>
> s/differet/different/
>
> >
> > Fixes: 755113d76786 ("thermal/debugfs: Add thermal cooling device debugfs information")
> > Cc :6.8+ <stable@vger.kernel.org> # 6.8+
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >   drivers/thermal/thermal_debugfs.c |   10 ++++++++--
> >   1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > Index: linux-pm/drivers/thermal/thermal_debugfs.c
> > ===================================================================
> > --- linux-pm.orig/drivers/thermal/thermal_debugfs.c
> > +++ linux-pm/drivers/thermal/thermal_debugfs.c
> > @@ -503,15 +503,21 @@ void thermal_debug_cdev_add(struct therm
> >    */
> >   void thermal_debug_cdev_remove(struct thermal_cooling_device *cdev)
> >   {
> > -     struct thermal_debugfs *thermal_dbg = cdev->debugfs;
> > +     struct thermal_debugfs *thermal_dbg;
> >
> > +     mutex_lock(&cdev->lock);
> > +
> > +     thermal_dbg = cdev->debugfs;
> >       if (!thermal_dbg)
>
> mutex_unlock(&cdev->lock) missing here

Good catch, thanks!

Ho-hum, I'm not sure why I haven't added it here ...

I'll send a v2 of this patch shortly.

> >               return;
> >
> > +     cdev->debugfs = NULL;
> > +
> > +     mutex_unlock(&cdev->lock);
> > +
> >       mutex_lock(&thermal_dbg->lock);
> >
> >       thermal_debugfs_cdev_clear(&thermal_dbg->cdev_dbg);
> > -     cdev->debugfs = NULL;
> >
> >       mutex_unlock(&thermal_dbg->lock);
> >
> >
> >
> >
> >
>

[PATCH v2 3/3] thermal/debugfs: Prevent use-after-free from occurring after cdev removal

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Since thermal_debug_cdev_remove() does not run under cdev->lock, it can
run in parallel with thermal_debug_cdev_state_update() and it may free
the struct thermal_debugfs object used by the latter after it has been
checked against NULL.

If that happens, thermal_debug_cdev_state_update() will access memory
that has been freed already causing the kernel to crash.

Address this by using cdev->lock in thermal_debug_cdev_remove() around
the cdev->debugfs value check (in case the same cdev is removed at the
same time in two different threads) and its reset to NULL.

Fixes: 755113d76786 ("thermal/debugfs: Add thermal cooling device debugfs information")
Cc :6.8+ <stable@vger.kernel.org> # 6.8+
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v1 -> v2: Add missing mutex_unlock() (Lukasz).

---
 drivers/thermal/thermal_debugfs.c |   14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Index: linux-pm/drivers/thermal/thermal_debugfs.c
===================================================================
--- linux-pm.orig/drivers/thermal/thermal_debugfs.c
+++ linux-pm/drivers/thermal/thermal_debugfs.c
@@ -505,15 +505,23 @@ void thermal_debug_cdev_add(struct therm
  */
 void thermal_debug_cdev_remove(struct thermal_cooling_device *cdev)
 {
-	struct thermal_debugfs *thermal_dbg = cdev->debugfs;
+	struct thermal_debugfs *thermal_dbg;
 
-	if (!thermal_dbg)
+	mutex_lock(&cdev->lock);
+
+	thermal_dbg = cdev->debugfs;
+	if (!thermal_dbg) {
+		mutex_unlock(&cdev->lock);
 		return;
+	}
+
+	cdev->debugfs = NULL;
+
+	mutex_unlock(&cdev->lock);
 
 	mutex_lock(&thermal_dbg->lock);
 
 	thermal_debugfs_cdev_clear(&thermal_dbg->cdev_dbg);
-	cdev->debugfs = NULL;
 
 	mutex_unlock(&thermal_dbg->lock);
 

Re: [PATCH v2 3/3] thermal/debugfs: Prevent use-after-free from occurring after cdev removal

[Lukasz Luba]

On 4/26/24 10:28, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Since thermal_debug_cdev_remove() does not run under cdev->lock, it can
> run in parallel with thermal_debug_cdev_state_update() and it may free
> the struct thermal_debugfs object used by the latter after it has been
> checked against NULL.
> 
> If that happens, thermal_debug_cdev_state_update() will access memory
> that has been freed already causing the kernel to crash.
> 
> Address this by using cdev->lock in thermal_debug_cdev_remove() around
> the cdev->debugfs value check (in case the same cdev is removed at the
> same time in two different threads) and its reset to NULL.
> 
> Fixes: 755113d76786 ("thermal/debugfs: Add thermal cooling device debugfs information")
> Cc :6.8+ <stable@vger.kernel.org> # 6.8+
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> v1 -> v2: Add missing mutex_unlock() (Lukasz).
> 
> ---
>   drivers/thermal/thermal_debugfs.c |   14 +++++++++++---
>   1 file changed, 11 insertions(+), 3 deletions(-)
> 
> Index: linux-pm/drivers/thermal/thermal_debugfs.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/thermal_debugfs.c
> +++ linux-pm/drivers/thermal/thermal_debugfs.c
> @@ -505,15 +505,23 @@ void thermal_debug_cdev_add(struct therm
>    */
>   void thermal_debug_cdev_remove(struct thermal_cooling_device *cdev)
>   {
> -	struct thermal_debugfs *thermal_dbg = cdev->debugfs;
> +	struct thermal_debugfs *thermal_dbg;
>   
> -	if (!thermal_dbg)
> +	mutex_lock(&cdev->lock);
> +
> +	thermal_dbg = cdev->debugfs;
> +	if (!thermal_dbg) {
> +		mutex_unlock(&cdev->lock);
>   		return;
> +	}
> +
> +	cdev->debugfs = NULL;
> +
> +	mutex_unlock(&cdev->lock);
>   
>   	mutex_lock(&thermal_dbg->lock);
>   
>   	thermal_debugfs_cdev_clear(&thermal_dbg->cdev_dbg);
> -	cdev->debugfs = NULL;
>   
>   	mutex_unlock(&thermal_dbg->lock);
>   
> 
> 
> 
> 

It looks good now

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Re: [PATCH v2 3/3] thermal/debugfs: Prevent use-after-free from occurring after cdev removal

[Rafael J. Wysocki]

On Fri, Apr 26, 2024 at 11:35 AM Lukasz Luba <lukasz.luba@arm.com> wrote:
>
>
>
> On 4/26/24 10:28, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Since thermal_debug_cdev_remove() does not run under cdev->lock, it can
> > run in parallel with thermal_debug_cdev_state_update() and it may free
> > the struct thermal_debugfs object used by the latter after it has been
> > checked against NULL.
> >
> > If that happens, thermal_debug_cdev_state_update() will access memory
> > that has been freed already causing the kernel to crash.
> >
> > Address this by using cdev->lock in thermal_debug_cdev_remove() around
> > the cdev->debugfs value check (in case the same cdev is removed at the
> > same time in two different threads) and its reset to NULL.
> >
> > Fixes: 755113d76786 ("thermal/debugfs: Add thermal cooling device debugfs information")
> > Cc :6.8+ <stable@vger.kernel.org> # 6.8+
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >
> > v1 -> v2: Add missing mutex_unlock() (Lukasz).
> >
> > ---
> >   drivers/thermal/thermal_debugfs.c |   14 +++++++++++---
> >   1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > Index: linux-pm/drivers/thermal/thermal_debugfs.c
> > ===================================================================
> > --- linux-pm.orig/drivers/thermal/thermal_debugfs.c
> > +++ linux-pm/drivers/thermal/thermal_debugfs.c
> > @@ -505,15 +505,23 @@ void thermal_debug_cdev_add(struct therm
> >    */
> >   void thermal_debug_cdev_remove(struct thermal_cooling_device *cdev)
> >   {
> > -     struct thermal_debugfs *thermal_dbg = cdev->debugfs;
> > +     struct thermal_debugfs *thermal_dbg;
> >
> > -     if (!thermal_dbg)
> > +     mutex_lock(&cdev->lock);
> > +
> > +     thermal_dbg = cdev->debugfs;
> > +     if (!thermal_dbg) {
> > +             mutex_unlock(&cdev->lock);
> >               return;
> > +     }
> > +
> > +     cdev->debugfs = NULL;
> > +
> > +     mutex_unlock(&cdev->lock);
> >
> >       mutex_lock(&thermal_dbg->lock);
> >
> >       thermal_debugfs_cdev_clear(&thermal_dbg->cdev_dbg);
> > -     cdev->debugfs = NULL;
> >
> >       mutex_unlock(&thermal_dbg->lock);
> >
> >
> >
> >
> >
>
> It looks good now
>
> Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

Thanks!