Re: [PATCH v1] thermal: hwmon: Remove hwmon class device along with its parent

Thank you for your contribution! Sashiko AI review found 1 potential issue(=
s) to consider:

Pre-existing issues:
- [High] In the error handling path of `thermal_add_hwmon_sysfs()`, an exis=
ting shared `hwmon` device is unconditionally freed if subsequent sysfs att=
ribute creations fail, leading to a use-after-free and list corruption.
--

--- Patch [1]: [PATCH v1] thermal: hwmon: Remove hwmon class device along w=
ith its parent ---
commit b487e50bcd7169eacc3dd2a937306e759abd531d
Author: Rafael J. Wysocki <[email protected]>

thermal: hwmon: Remove hwmon class device along with its parent

This commit modifies thermal_remove_hwmon_sysfs() to remove the entire
hwmon class device interface for a thermal zone type when the device
holding it is removed. It also serializes the addition and removal of
hwmon sysfs interfaces under the thermal_hwmon_list_lock.

Link: https://lore.kernel.org/linux-pm/20260402021828.16556-1-liujia6264@gm=
ail.com/

> diff --git a/drivers/thermal/thermal_hwmon.c b/drivers/thermal/thermal_hw=
mon.c
> index 597c33c8a5550..2233ceef7f140 100644
> --- a/drivers/thermal/thermal_hwmon.c
> +++ b/drivers/thermal/thermal_hwmon.c
[ ... ]
> @@ -196,23 +178,23 @@ int thermal_add_hwmon_sysfs(struct thermal_zone_dev=
ice *tz)
[ ... ]
> - unregister_input:
> +unregister_input:
>  	device_remove_file(hwmon->device, &temp->temp_input.attr);
> - free_temp_mem:
> +free_temp_mem:
>  	kfree(temp);
> - unregister_name:
> +unregister_name:
>  	if (new_hwmon_device)
>  		hwmon_device_unregister(hwmon->device);
> - free_mem:
> +free_mem:
>  	kfree(hwmon);

[Severity: High]
This is a pre-existing issue, but if new_hwmon_device is 0 (meaning hwmon
was retrieved from the list and is still active), does falling through to
free_mem unconditionally free the shared hwmon device?

If memory allocation for temp fails, or if device_create_file() fails, the
error path jumps to these labels. Because kfree(hwmon) is outside the
new_hwmon_device check, it seems like this would lead to a use-after-free
and corrupt the thermal_hwmon_list. Should kfree(hwmon) also be guarded by
new_hwmon_device?

> +unlock:
> +	mutex_unlock(&thermal_hwmon_list_lock);
> =20
>  	return result;
>  }

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/23195575.EfDdHjke4D=
@rafael.j.wysocki?part=3D1