Received-SPF: pass (google.com: domain of linux-kernel+bounces-158634-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: "Rafael J. Wysocki" <rjw@rjwysocki.net>
To: Lukasz Luba <lukasz.luba@arm.com>,
 Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Linux PM <linux-pm@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>,
 "Rafael J. Wysocki" <rafael@kernel.org>
Subject: [PATCH v1 2/3] thermal/debugfs: Fix thermal zone locking
Date: Thu, 25 Apr 2024 15:55:45 +0200
Message-ID: <1888579.tdWV9SEqCh@kreacher>
In-Reply-To: <12427744.O9o76ZdvQC@kreacher>
References: <12427744.O9o76ZdvQC@kreacher>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="UTF-8"

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);
 }