[PATCH v1 0/2] thermal: hwmon: Restore automatic hwmon device registration

[PATCH v1 0/2] thermal: hwmon: Restore automatic hwmon device registration

[Rafael J. Wysocki]

Hi All,

This reverts two recent commits that changed the sysfs interface for
hwmon class devices automatically registered by the thermal subsystem
that were part of the series at:

https://lore.kernel.org/linux-pm/6017595.DvuYhMxLoT@rafael.j.wysocki/

Since they generally have made user space unhappy:

https://lore.kernel.org/linux-pm/cafd8af9-c6e9-4bf2-b496-23e796fbc9a6@linux.dev/
https://lore.kernel.org/linux-hwmon/ab8b093b-46e6-4738-afcf-4b97c9ad5af9@googlemail.com/

and it is late in the cycle, it is better to revert them now and
address the issue they were attempting to address later.

Thanks!

[PATCH v1 1/2] Revert "thermal: hwmon: Use extra_groups for adding temperature attributes"

[Rafael J. Wysocki]

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

Revert commit cfb5dc0f60fb ("thermal: hwmon: Use extra_groups for adding
temperature attributes") because it is depended on by another one that
turned out to be problematic.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/hwmon/hwmon.c           |   6 +-
 drivers/thermal/thermal_hwmon.c | 122 ++++++++++++++++++++------------
 include/linux/hwmon.h           |   3 +-
 3 files changed, 80 insertions(+), 51 deletions(-)

diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 55a9a3ddd4aa..29dc90a2c3fe 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -1083,7 +1083,6 @@ EXPORT_SYMBOL_GPL(hwmon_device_register_with_info);
  * @dev: the parent device
  * @name: hwmon name attribute
  * @drvdata: driver data to attach to created device
- * @extra_groups: pointer to list of additional non-standard attribute groups
  *
  * The use of this function is restricted. It is provided for legacy reasons
  * and must only be called from the thermal subsystem.
@@ -1095,13 +1094,12 @@ EXPORT_SYMBOL_GPL(hwmon_device_register_with_info);
  */
 struct device *
 hwmon_device_register_for_thermal(struct device *dev, const char *name,
-				  void *drvdata,
-				  const struct attribute_group **extra_groups)
+				  void *drvdata)
 {
 	if (!name || !dev)
 		return ERR_PTR(-EINVAL);
 
-	return __hwmon_device_register(dev, name, drvdata, NULL, extra_groups);
+	return __hwmon_device_register(dev, name, drvdata, NULL, NULL);
 }
 EXPORT_SYMBOL_NS_GPL(hwmon_device_register_for_thermal, "HWMON_THERMAL");
 
diff --git a/drivers/thermal/thermal_hwmon.c b/drivers/thermal/thermal_hwmon.c
index 386dfb9f559e..223ae1571655 100644
--- a/drivers/thermal/thermal_hwmon.c
+++ b/drivers/thermal/thermal_hwmon.c
@@ -25,13 +25,25 @@
  */
 #define THERMAL_HWMON_NAME_LENGTH (THERMAL_NAME_LENGTH + 11)
 
+struct thermal_hwmon_attr {
+	struct device_attribute attr;
+};
+
+/* one temperature input for each thermal zone */
+struct thermal_hwmon_temp {
+	struct thermal_zone_device *tz;
+	struct thermal_hwmon_attr temp_input;	/* hwmon sys attr */
+	struct thermal_hwmon_attr temp_crit;	/* hwmon sys attr */
+	bool temp_crit_present;
+};
+
 /* hwmon sys I/F */
 /* thermal zone devices with the same type share one hwmon device */
 struct thermal_hwmon_device {
 	char name[THERMAL_HWMON_NAME_LENGTH];
 	struct device *device;
 	struct list_head node;
-	struct thermal_zone_device *tz;
+	struct thermal_hwmon_temp tz_temp;
 };
 
 static LIST_HEAD(thermal_hwmon_list);
@@ -39,14 +51,19 @@ static LIST_HEAD(thermal_hwmon_list);
 static DEFINE_MUTEX(thermal_hwmon_list_lock);
 
 static ssize_t
-temp1_input_show(struct device *dev, struct device_attribute *attr, char *buf)
+temp_input_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
-	struct thermal_hwmon_device *hwmon = dev_get_drvdata(dev);
-	struct thermal_zone_device *tz = hwmon->tz;
 	int temperature;
 	int ret;
+	struct thermal_hwmon_attr *hwmon_attr
+			= container_of(attr, struct thermal_hwmon_attr, attr);
+	struct thermal_hwmon_temp *temp
+			= container_of(hwmon_attr, struct thermal_hwmon_temp,
+				       temp_input);
+	struct thermal_zone_device *tz = temp->tz;
 
 	ret = thermal_zone_get_temp(tz, &temperature);
+
 	if (ret)
 		return ret;
 
@@ -54,10 +71,14 @@ temp1_input_show(struct device *dev, struct device_attribute *attr, char *buf)
 }
 
 static ssize_t
-temp1_crit_show(struct device *dev, struct device_attribute *attr, char *buf)
+temp_crit_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
-	struct thermal_hwmon_device *hwmon = dev_get_drvdata(dev);
-	struct thermal_zone_device *tz = hwmon->tz;
+	struct thermal_hwmon_attr *hwmon_attr
+			= container_of(attr, struct thermal_hwmon_attr, attr);
+	struct thermal_hwmon_temp *temp
+			= container_of(hwmon_attr, struct thermal_hwmon_temp,
+				       temp_crit);
+	struct thermal_zone_device *tz = temp->tz;
 	int temperature;
 	int ret;
 
@@ -70,49 +91,22 @@ temp1_crit_show(struct device *dev, struct device_attribute *attr, char *buf)
 	return sysfs_emit(buf, "%d\n", temperature);
 }
 
-static DEVICE_ATTR_RO(temp1_input);
-static DEVICE_ATTR_RO(temp1_crit);
-
-static struct attribute *thermal_hwmon_attrs[] = {
-	&dev_attr_temp1_input.attr,
-	&dev_attr_temp1_crit.attr,
-	NULL,
-};
-
-static umode_t thermal_hwmon_attr_is_visible(struct kobject *kobj,
-					     struct attribute *a, int n)
+static bool thermal_zone_crit_temp_valid(struct thermal_zone_device *tz)
 {
-	if (a == &dev_attr_temp1_input.attr)
-		return a->mode;
-
-	if (a == &dev_attr_temp1_crit.attr) {
-		struct thermal_hwmon_device *hwmon = dev_get_drvdata(kobj_to_dev(kobj));
-		struct thermal_zone_device *tz = hwmon->tz;
-		int dummy;
-
-		if (tz->ops.get_crit_temp && !tz->ops.get_crit_temp(tz, &dummy))
-			return a->mode;
-	}
-
-	return 0;
+	int temp;
+	return tz->ops.get_crit_temp && !tz->ops.get_crit_temp(tz, &temp);
 }
 
-static const struct attribute_group thermal_hwmon_group = {
-	.attrs	= thermal_hwmon_attrs,
-	.is_visible = thermal_hwmon_attr_is_visible,
-};
-
-__ATTRIBUTE_GROUPS(thermal_hwmon);
-
 int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz)
 {
 	struct thermal_hwmon_device *hwmon;
+	struct thermal_hwmon_temp *temp;
+	int result;
 
 	hwmon = kzalloc_obj(*hwmon);
 	if (!hwmon)
 		return -ENOMEM;
 
-	hwmon->tz = tz;
 	/*
 	 * Append the thermal zone ID preceded by an underline character to the
 	 * type to disambiguate the sensors command output.
@@ -120,13 +114,35 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz)
 	scnprintf(hwmon->name, THERMAL_HWMON_NAME_LENGTH, "%s_%d", tz->type, tz->id);
 	strreplace(hwmon->name, '-', '_');
 	hwmon->device = hwmon_device_register_for_thermal(&tz->device,
-							  hwmon->name, hwmon,
-							  thermal_hwmon_groups);
+							  hwmon->name, hwmon);
 	if (IS_ERR(hwmon->device)) {
-		int result = PTR_ERR(hwmon->device);
+		result = PTR_ERR(hwmon->device);
+		goto free_mem;
+	}
 
-		kfree(hwmon);
-		return result;
+	temp = &hwmon->tz_temp;
+
+	temp->tz = tz;
+
+	temp->temp_input.attr.attr.name = "temp1_input";
+	temp->temp_input.attr.attr.mode = 0444;
+	temp->temp_input.attr.show = temp_input_show;
+	sysfs_attr_init(&temp->temp_input.attr.attr);
+	result = device_create_file(hwmon->device, &temp->temp_input.attr);
+	if (result)
+		goto unregister_name;
+
+	if (thermal_zone_crit_temp_valid(tz)) {
+		temp->temp_crit.attr.attr.name = "temp1_crit";
+		temp->temp_crit.attr.attr.mode = 0444;
+		temp->temp_crit.attr.show = temp_crit_show;
+		sysfs_attr_init(&temp->temp_crit.attr.attr);
+		result = device_create_file(hwmon->device,
+					    &temp->temp_crit.attr);
+		if (result)
+			goto unregister_input;
+
+		temp->temp_crit_present = true;
 	}
 
 	/* The list is needed for hwmon lookup during removal. */
@@ -135,6 +151,15 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz)
 	mutex_unlock(&thermal_hwmon_list_lock);
 
 	return 0;
+
+ unregister_input:
+	device_remove_file(hwmon->device, &temp->temp_input.attr);
+ unregister_name:
+	hwmon_device_unregister(hwmon->device);
+ free_mem:
+	kfree(hwmon);
+
+	return result;
 }
 EXPORT_SYMBOL_GPL(thermal_add_hwmon_sysfs);
 
@@ -144,7 +169,7 @@ thermal_hwmon_lookup(const struct thermal_zone_device *tz)
 	struct thermal_hwmon_device *hwmon;
 
 	list_for_each_entry(hwmon, &thermal_hwmon_list, node) {
-		if (hwmon->tz == tz)
+		if (hwmon->tz_temp.tz == tz)
 			return hwmon;
 	}
 	return NULL;
@@ -153,6 +178,7 @@ thermal_hwmon_lookup(const struct thermal_zone_device *tz)
 void thermal_remove_hwmon_sysfs(struct thermal_zone_device *tz)
 {
 	struct thermal_hwmon_device *hwmon;
+	struct thermal_hwmon_temp *temp;
 
 	scoped_guard(mutex, &thermal_hwmon_list_lock) {
 		hwmon = thermal_hwmon_lookup(tz);
@@ -162,6 +188,12 @@ void thermal_remove_hwmon_sysfs(struct thermal_zone_device *tz)
 		list_del(&hwmon->node);
 	}
 
+	temp = &hwmon->tz_temp;
+
+	device_remove_file(hwmon->device, &temp->temp_input.attr);
+	if (temp->temp_crit_present)
+		device_remove_file(hwmon->device, &temp->temp_crit.attr);
+
 	hwmon_device_unregister(hwmon->device);
 	kfree(hwmon);
 }
diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index 77a6f2bffcba..dd713e193d0c 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -480,8 +480,7 @@ hwmon_device_register_with_info(struct device *dev,
 				const struct attribute_group **extra_groups);
 struct device *
 hwmon_device_register_for_thermal(struct device *dev, const char *name,
-				  void *drvdata,
-				  const struct attribute_group **extra_groups);
+				  void *drvdata);
 struct device *
 devm_hwmon_device_register_with_info(struct device *dev,
 				const char *name, void *drvdata,
-- 
2.51.0

Re: [PATCH v1 1/2] Revert "thermal: hwmon: Use extra_groups for adding temperature attributes"

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Adding sysfs attributes via `device_create_file()` after device registration creates a race condition with userspace uevents.
--

--- Patch [1]: [PATCH v1 1/2] Revert "thermal: hwmon: Use extra_groups for adding temperature attributes" ---
commit fd1537461eb002e63315e0164251174bf300f7cc
Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Revert "thermal: hwmon: Use extra_groups for adding temperature attributes"

This reverts commit cfb5dc0f60fb because it was depended on by another commit
that turned out to be problematic.

> diff --git a/drivers/thermal/thermal_hwmon.c b/drivers/thermal/thermal_hwmon.c
> index 386dfb9f559e3..223ae1571655b 100644
> --- a/drivers/thermal/thermal_hwmon.c
> +++ b/drivers/thermal/thermal_hwmon.c
[ ... ]
> @@ -120,13 +114,35 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz)
>  	scnprintf(hwmon->name, THERMAL_HWMON_NAME_LENGTH, "%s_%d", tz->type, tz->id);
>  	strreplace(hwmon->name, '-', '_');
>  	hwmon->device = hwmon_device_register_for_thermal(&tz->device,
> -							  hwmon->name, hwmon,
> -							  thermal_hwmon_groups);
> +							  hwmon->name, hwmon);
>  	if (IS_ERR(hwmon->device)) {
> -		int result = PTR_ERR(hwmon->device);
> +		result = PTR_ERR(hwmon->device);
> +		goto free_mem;
> +	}
>  
> -		kfree(hwmon);
> -		return result;
> +	temp = &hwmon->tz_temp;
> +
> +	temp->tz = tz;
> +
> +	temp->temp_input.attr.attr.name = "temp1_input";
> +	temp->temp_input.attr.attr.mode = 0444;
> +	temp->temp_input.attr.show = temp_input_show;
> +	sysfs_attr_init(&temp->temp_input.attr.attr);
> +	result = device_create_file(hwmon->device, &temp->temp_input.attr);

[Severity: Medium]
Does this sequence create a race condition with userspace uevents? 

When thermal_add_hwmon_sysfs() calls hwmon_device_register_for_thermal(),
the device is registered and the KOBJ_ADD uevent fires immediately.

If userspace monitoring daemons or udev rules respond to this uevent and 
read the hwmon device directory right away, they might query it before 
device_create_file() finishes adding the temp1_input and temp1_crit 
attributes.

Could this lead to missing sensor attributes in userspace during device
initialization?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/6319276.lOV4Wx5bFT@rafael.j.wysocki?part=1

[PATCH v1 2/2] Revert "thermal: hwmon: Register a hwmon device for each thermal zone"

[Rafael J. Wysocki]

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

Revert commit d6323469bcfb ("thermal: hwmon: Register a hwmon device
for each thermal zone") that changed the names of hwmon class devices
associated with thermal zones and their sysfs layout which made user
space unhappy.

Closes: https://lore.kernel.org/linux-pm/cafd8af9-c6e9-4bf2-b496-23e796fbc9a6@linux.dev/
Closes: https://lore.kernel.org/linux-hwmon/ab8b093b-46e6-4738-afcf-4b97c9ad5af9@googlemail.com/
Cc: stable@vger.kernel.org
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/thermal_hwmon.c | 151 ++++++++++++++++++++++----------
 1 file changed, 104 insertions(+), 47 deletions(-)

diff --git a/drivers/thermal/thermal_hwmon.c b/drivers/thermal/thermal_hwmon.c
index 223ae1571655..597c33c8a555 100644
--- a/drivers/thermal/thermal_hwmon.c
+++ b/drivers/thermal/thermal_hwmon.c
@@ -19,33 +19,30 @@
 #include "thermal_hwmon.h"
 #include "thermal_core.h"
 
-/*
- * Needs to be large enough to hold a thermal zone type string followed by an
- * underline character and a 32-bit integer in decimal representation.
- */
-#define THERMAL_HWMON_NAME_LENGTH (THERMAL_NAME_LENGTH + 11)
+/* hwmon sys I/F */
+/* thermal zone devices with the same type share one hwmon device */
+struct thermal_hwmon_device {
+	char type[THERMAL_NAME_LENGTH];
+	struct device *device;
+	int count;
+	struct list_head tz_list;
+	struct list_head node;
+};
 
 struct thermal_hwmon_attr {
 	struct device_attribute attr;
+	char name[16];
 };
 
 /* one temperature input for each thermal zone */
 struct thermal_hwmon_temp {
+	struct list_head hwmon_node;
 	struct thermal_zone_device *tz;
 	struct thermal_hwmon_attr temp_input;	/* hwmon sys attr */
 	struct thermal_hwmon_attr temp_crit;	/* hwmon sys attr */
 	bool temp_crit_present;
 };
 
-/* hwmon sys I/F */
-/* thermal zone devices with the same type share one hwmon device */
-struct thermal_hwmon_device {
-	char name[THERMAL_HWMON_NAME_LENGTH];
-	struct device *device;
-	struct list_head node;
-	struct thermal_hwmon_temp tz_temp;
-};
-
 static LIST_HEAD(thermal_hwmon_list);
 
 static DEFINE_MUTEX(thermal_hwmon_list_lock);
@@ -91,6 +88,45 @@ temp_crit_show(struct device *dev, struct device_attribute *attr, char *buf)
 	return sysfs_emit(buf, "%d\n", temperature);
 }
 
+
+static struct thermal_hwmon_device *
+thermal_hwmon_lookup_by_type(const struct thermal_zone_device *tz)
+{
+	struct thermal_hwmon_device *hwmon;
+	char type[THERMAL_NAME_LENGTH];
+
+	mutex_lock(&thermal_hwmon_list_lock);
+	list_for_each_entry(hwmon, &thermal_hwmon_list, node) {
+		strscpy(type, tz->type);
+		strreplace(type, '-', '_');
+		if (!strcmp(hwmon->type, type)) {
+			mutex_unlock(&thermal_hwmon_list_lock);
+			return hwmon;
+		}
+	}
+	mutex_unlock(&thermal_hwmon_list_lock);
+
+	return NULL;
+}
+
+/* Find the temperature input matching a given thermal zone */
+static struct thermal_hwmon_temp *
+thermal_hwmon_lookup_temp(const struct thermal_hwmon_device *hwmon,
+			  const struct thermal_zone_device *tz)
+{
+	struct thermal_hwmon_temp *temp;
+
+	mutex_lock(&thermal_hwmon_list_lock);
+	list_for_each_entry(temp, &hwmon->tz_list, hwmon_node)
+		if (temp->tz == tz) {
+			mutex_unlock(&thermal_hwmon_list_lock);
+			return temp;
+		}
+	mutex_unlock(&thermal_hwmon_list_lock);
+
+	return NULL;
+}
+
 static bool thermal_zone_crit_temp_valid(struct thermal_zone_device *tz)
 {
 	int temp;
@@ -101,39 +137,54 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz)
 {
 	struct thermal_hwmon_device *hwmon;
 	struct thermal_hwmon_temp *temp;
+	int new_hwmon_device = 1;
 	int result;
 
+	hwmon = thermal_hwmon_lookup_by_type(tz);
+	if (hwmon) {
+		new_hwmon_device = 0;
+		goto register_sys_interface;
+	}
+
 	hwmon = kzalloc_obj(*hwmon);
 	if (!hwmon)
 		return -ENOMEM;
 
-	/*
-	 * Append the thermal zone ID preceded by an underline character to the
-	 * type to disambiguate the sensors command output.
-	 */
-	scnprintf(hwmon->name, THERMAL_HWMON_NAME_LENGTH, "%s_%d", tz->type, tz->id);
-	strreplace(hwmon->name, '-', '_');
+	INIT_LIST_HEAD(&hwmon->tz_list);
+	strscpy(hwmon->type, tz->type, THERMAL_NAME_LENGTH);
+	strreplace(hwmon->type, '-', '_');
 	hwmon->device = hwmon_device_register_for_thermal(&tz->device,
-							  hwmon->name, hwmon);
+							  hwmon->type, hwmon);
 	if (IS_ERR(hwmon->device)) {
 		result = PTR_ERR(hwmon->device);
 		goto free_mem;
 	}
 
-	temp = &hwmon->tz_temp;
+ register_sys_interface:
+	temp = kzalloc_obj(*temp);
+	if (!temp) {
+		result = -ENOMEM;
+		goto unregister_name;
+	}
 
 	temp->tz = tz;
+	hwmon->count++;
 
-	temp->temp_input.attr.attr.name = "temp1_input";
+	snprintf(temp->temp_input.name, sizeof(temp->temp_input.name),
+		 "temp%d_input", hwmon->count);
+	temp->temp_input.attr.attr.name = temp->temp_input.name;
 	temp->temp_input.attr.attr.mode = 0444;
 	temp->temp_input.attr.show = temp_input_show;
 	sysfs_attr_init(&temp->temp_input.attr.attr);
 	result = device_create_file(hwmon->device, &temp->temp_input.attr);
 	if (result)
-		goto unregister_name;
+		goto free_temp_mem;
 
 	if (thermal_zone_crit_temp_valid(tz)) {
-		temp->temp_crit.attr.attr.name = "temp1_crit";
+		snprintf(temp->temp_crit.name,
+				sizeof(temp->temp_crit.name),
+				"temp%d_crit", hwmon->count);
+		temp->temp_crit.attr.attr.name = temp->temp_crit.name;
 		temp->temp_crit.attr.attr.mode = 0444;
 		temp->temp_crit.attr.show = temp_crit_show;
 		sysfs_attr_init(&temp->temp_crit.attr.attr);
@@ -145,17 +196,21 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz)
 		temp->temp_crit_present = true;
 	}
 
-	/* The list is needed for hwmon lookup during removal. */
 	mutex_lock(&thermal_hwmon_list_lock);
-	list_add_tail(&hwmon->node, &thermal_hwmon_list);
+	if (new_hwmon_device)
+		list_add_tail(&hwmon->node, &thermal_hwmon_list);
+	list_add_tail(&temp->hwmon_node, &hwmon->tz_list);
 	mutex_unlock(&thermal_hwmon_list_lock);
 
 	return 0;
 
  unregister_input:
 	device_remove_file(hwmon->device, &temp->temp_input.attr);
+ free_temp_mem:
+	kfree(temp);
  unregister_name:
-	hwmon_device_unregister(hwmon->device);
+	if (new_hwmon_device)
+		hwmon_device_unregister(hwmon->device);
  free_mem:
 	kfree(hwmon);
 
@@ -163,37 +218,39 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz)
 }
 EXPORT_SYMBOL_GPL(thermal_add_hwmon_sysfs);
 
-static struct thermal_hwmon_device *
-thermal_hwmon_lookup(const struct thermal_zone_device *tz)
-{
-	struct thermal_hwmon_device *hwmon;
-
-	list_for_each_entry(hwmon, &thermal_hwmon_list, node) {
-		if (hwmon->tz_temp.tz == tz)
-			return hwmon;
-	}
-	return NULL;
-}
-
 void thermal_remove_hwmon_sysfs(struct thermal_zone_device *tz)
 {
 	struct thermal_hwmon_device *hwmon;
 	struct thermal_hwmon_temp *temp;
 
-	scoped_guard(mutex, &thermal_hwmon_list_lock) {
-		hwmon = thermal_hwmon_lookup(tz);
-		if (!hwmon)
-			return;
-
-		list_del(&hwmon->node);
+	hwmon = thermal_hwmon_lookup_by_type(tz);
+	if (unlikely(!hwmon)) {
+		/* Should never happen... */
+		dev_dbg(&tz->device, "hwmon device lookup failed!\n");
+		return;
 	}
 
-	temp = &hwmon->tz_temp;
+	temp = thermal_hwmon_lookup_temp(hwmon, tz);
+	if (unlikely(!temp)) {
+		/* Should never happen... */
+		dev_dbg(&tz->device, "temperature input lookup failed!\n");
+		return;
+	}
 
 	device_remove_file(hwmon->device, &temp->temp_input.attr);
 	if (temp->temp_crit_present)
 		device_remove_file(hwmon->device, &temp->temp_crit.attr);
 
+	mutex_lock(&thermal_hwmon_list_lock);
+	list_del(&temp->hwmon_node);
+	kfree(temp);
+	if (!list_empty(&hwmon->tz_list)) {
+		mutex_unlock(&thermal_hwmon_list_lock);
+		return;
+	}
+	list_del(&hwmon->node);
+	mutex_unlock(&thermal_hwmon_list_lock);
+
 	hwmon_device_unregister(hwmon->device);
 	kfree(hwmon);
 }
-- 
2.51.0

Re: [PATCH v1 2/2] Revert "thermal: hwmon: Register a hwmon device for each thermal zone"

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The error handling path in `thermal_add_hwmon_sysfs()` unconditionally frees a shared `hwmon` device, causing a Use-After-Free vulnerability.
- [High] Lack of reference counting and inadequate lock scope for shared `hwmon` devices lead to TOCTOU races, duplicate device registration, and Use-After-Free vulnerabilities.
- [Medium] Unprotected concurrent modification of `hwmon->count` causes data races and sysfs file name collisions.
--

--- Patch [2]: [PATCH v1 2/2] Revert "thermal: hwmon: Register a hwmon device for each thermal zone" ---
commit 811b1574f43c922402d0fcdcdca33812b6bd90b8
Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Revert "thermal: hwmon: Register a hwmon device for each thermal zone"

This commit reverts a previous change to thermal zone hwmon device names and
sysfs layout, restoring the behavior where thermal zone devices with the same
type share one hwmon device.

Link: https://lore.kernel.org/linux-pm/cafd8af9-c6e9-4bf2-b496-23e796fbc9a6@linux.dev/
Link: https://lore.kernel.org/linux-hwmon/ab8b093b-46e6-4738-afcf-4b97c9ad5af9@googlemail.com/

> diff --git a/drivers/thermal/thermal_hwmon.c b/drivers/thermal/thermal_hwmon.c
> index 223ae1571655b..597c33c8a5550 100644
> --- a/drivers/thermal/thermal_hwmon.c
> +++ b/drivers/thermal/thermal_hwmon.c

[ ... ]

> @@ -91,6 +88,45 @@ temp_crit_show(struct device *dev, struct device_attribute *attr, char *buf)
>  	return sysfs_emit(buf, "%d\n", temperature);
>  }
>  
> +
> +static struct thermal_hwmon_device *
> +thermal_hwmon_lookup_by_type(const struct thermal_zone_device *tz)
> +{
> +	struct thermal_hwmon_device *hwmon;
> +	char type[THERMAL_NAME_LENGTH];
> +
> +	mutex_lock(&thermal_hwmon_list_lock);
> +	list_for_each_entry(hwmon, &thermal_hwmon_list, node) {
> +		strscpy(type, tz->type);
> +		strreplace(type, '-', '_');
> +		if (!strcmp(hwmon->type, type)) {
> +			mutex_unlock(&thermal_hwmon_list_lock);
> +			return hwmon;

[Severity: High]
Does this introduce a race condition resulting in a use-after-free?

If a thread calls thermal_hwmon_lookup_by_type(), the lock is dropped before
returning the shared hwmon pointer. If another thread concurrently removes the
last thermal zone for this type via thermal_remove_hwmon_sysfs(), it could
free the hwmon device while the first thread is still executing
thermal_add_hwmon_sysfs().

Would it be safer to hold a reference count on the shared hwmon object before
dropping thermal_hwmon_list_lock?

> +		}
> +	}
> +	mutex_unlock(&thermal_hwmon_list_lock);
> +
> +	return NULL;
> +}
> +

[ ... ]

> @@ -101,39 +137,54 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz)

[ ... ]

> + register_sys_interface:
> +	temp = kzalloc_obj(*temp);
> +	if (!temp) {
> +		result = -ENOMEM;
> +		goto unregister_name;
> +	}
> +
> +	temp->tz = tz;
> +	hwmon->count++;
> +
> +	snprintf(temp->temp_input.name, sizeof(temp->temp_input.name),
> +		 "temp%d_input", hwmon->count);

[Severity: Medium]
Is it possible for concurrent probes to race on hwmon->count?

If two threads execute thermal_add_hwmon_sysfs() for the same thermal zone
type simultaneously, they could increment hwmon->count outside of any lock.
Could this lead to both threads formatting identical sysfs attribute names
and causing device_create_file() to fail with -EEXIST?

> +	temp->temp_input.attr.attr.name = temp->temp_input.name;
> +	temp->temp_input.attr.attr.mode = 0444;

[ ... ]

> @@ -145,17 +196,21 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_device *tz)

[ ... ]

>   unregister_input:
>  	device_remove_file(hwmon->device, &temp->temp_input.attr);
> + free_temp_mem:
> +	kfree(temp);
>   unregister_name:
> -	hwmon_device_unregister(hwmon->device);
> +	if (new_hwmon_device)
> +		hwmon_device_unregister(hwmon->device);
>   free_mem:
>  	kfree(hwmon);

[Severity: High]
Will this unconditionally free the shared hwmon object on an error path?

If new_hwmon_device is 0 (meaning the hwmon device was found via
thermal_hwmon_lookup_by_type and is shared), and an error occurs during sysfs
file creation, execution jumps to free_mem. Calling kfree(hwmon) here would
destroy the shared object while it is still in use by other thermal zones.

Should this kfree() be conditional on new_hwmon_device being true?

> -	return 0;
> +	return result;
>  }
>  EXPORT_SYMBOL_GPL(thermal_add_hwmon_sysfs);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/6319276.lOV4Wx5bFT@rafael.j.wysocki?part=2