| Message ID | 20221017130910.2307118-8-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | thermal/core: Protect thermal device operations against removal | expand |
On Mon, Oct 17, 2022 at 3:09 PM Guenter Roeck <linux@roeck-us.net> wrote: > > Protect access to thermal operations against thermal zone removal by > acquiring the thermal zone device mutex. After acquiring the mutex, check > if the thermal zone device is registered and abort the operation if not. > > With this change, we can call __thermal_zone_device_update() instead of > thermal_zone_device_update() from trip_point_temp_store() and from > emul_temp_store(). Similar, we can call __thermal_zone_set_trips() instead > of thermal_zone_set_trips() from trip_point_hyst_store(). > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/thermal/thermal_core.c | 4 +- > drivers/thermal/thermal_core.h | 2 + > drivers/thermal/thermal_sysfs.c | 77 ++++++++++++++++++++++++++++----- > 3 files changed, 69 insertions(+), 14 deletions(-) > > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c > index 9facd9c5b70f..b8e3b262b2bd 100644 > --- a/drivers/thermal/thermal_core.c > +++ b/drivers/thermal/thermal_core.c > @@ -403,8 +403,8 @@ static void thermal_zone_device_init(struct thermal_zone_device *tz) > pos->initialized = false; > } > > -static void __thermal_zone_device_update(struct thermal_zone_device *tz, > - enum thermal_notify_event event) > +void __thermal_zone_device_update(struct thermal_zone_device *tz, > + enum thermal_notify_event event) > { > int count; > > diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h > index 1571917bd3c8..7b51b9a22e7e 100644 > --- a/drivers/thermal/thermal_core.h > +++ b/drivers/thermal/thermal_core.h > @@ -109,6 +109,8 @@ int thermal_register_governor(struct thermal_governor *); > void thermal_unregister_governor(struct thermal_governor *); > int thermal_zone_device_set_policy(struct thermal_zone_device *, char *); > int thermal_build_list_of_policies(char *buf); > +void __thermal_zone_device_update(struct thermal_zone_device *tz, > + enum thermal_notify_event event); > > /* Helpers */ > void thermal_zone_set_trips(struct thermal_zone_device *tz); > diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c > index ec495c7dff03..68607e6070e8 100644 > --- a/drivers/thermal/thermal_sysfs.c > +++ b/drivers/thermal/thermal_sysfs.c > @@ -92,7 +92,15 @@ trip_point_type_show(struct device *dev, struct device_attribute *attr, > if (sscanf(attr->attr.name, "trip_point_%d_type", &trip) != 1) > return -EINVAL; > > + mutex_lock(&tz->lock); > + > + if (!device_is_registered(dev)) { > + mutex_unlock(&tz->lock); > + return -ENODEV; > + } > + > result = tz->ops->get_trip_type(tz, trip, &type); > + mutex_unlock(&tz->lock); > if (result) > return result; > > @@ -128,10 +136,17 @@ trip_point_temp_store(struct device *dev, struct device_attribute *attr, > if (kstrtoint(buf, 10, &temperature)) > return -EINVAL; > > + mutex_lock(&tz->lock); > + > + if (!device_is_registered(dev)) { > + ret = -ENODEV; > + goto unlock; > + } > + > if (tz->ops->set_trip_temp) { > ret = tz->ops->set_trip_temp(tz, trip, temperature); > if (ret) > - return ret; > + goto unlock; > } > > if (tz->trips) > @@ -140,18 +155,21 @@ trip_point_temp_store(struct device *dev, struct device_attribute *attr, > if (tz->ops->get_trip_hyst) { > ret = tz->ops->get_trip_hyst(tz, trip, &hyst); > if (ret) > - return ret; > + goto unlock; > } > > ret = tz->ops->get_trip_type(tz, trip, &type); > if (ret) > - return ret; > + goto unlock; > > thermal_notify_tz_trip_change(tz->id, trip, type, temperature, hyst); > > - thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); > + __thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); > > - return count; > +unlock: > + mutex_unlock(&tz->lock); > + > + return ret ? ret : count; > } > > static ssize_t > @@ -168,12 +186,19 @@ trip_point_temp_show(struct device *dev, struct device_attribute *attr, > if (sscanf(attr->attr.name, "trip_point_%d_temp", &trip) != 1) > return -EINVAL; > > + mutex_lock(&tz->lock); > + > + if (!device_is_registered(dev)) { > + ret = -ENODEV; > + goto unlock; > + } > + > ret = tz->ops->get_trip_temp(tz, trip, &temperature); > Well, this is a pattern I've already talked about twice, so let me just mention it here. > - if (ret) > - return ret; > +unlock: > + mutex_unlock(&tz->lock); > > - return sprintf(buf, "%d\n", temperature); > + return ret ? ret : sprintf(buf, "%d\n", temperature); And I wouldn't change this (like in the other patch I've commented). > } > > static ssize_t > @@ -193,6 +218,13 @@ trip_point_hyst_store(struct device *dev, struct device_attribute *attr, > if (kstrtoint(buf, 10, &temperature)) > return -EINVAL; > > + mutex_lock(&tz->lock); > + > + if (!device_is_registered(dev)) { > + ret = -ENODEV; > + goto unlock; > + } > + > /* > * We are not doing any check on the 'temperature' value > * here. The driver implementing 'set_trip_hyst' has to > @@ -201,7 +233,10 @@ trip_point_hyst_store(struct device *dev, struct device_attribute *attr, > ret = tz->ops->set_trip_hyst(tz, trip, temperature); > > if (!ret) > - thermal_zone_set_trips(tz); > + __thermal_zone_set_trips(tz); > + > +unlock: > + mutex_unlock(&tz->lock); > > return ret ? ret : count; > } > @@ -220,8 +255,18 @@ trip_point_hyst_show(struct device *dev, struct device_attribute *attr, > if (sscanf(attr->attr.name, "trip_point_%d_hyst", &trip) != 1) > return -EINVAL; > > + mutex_lock(&tz->lock); > + > + if (!device_is_registered(dev)) { > + ret = -ENODEV; > + goto unlock; > + } > + > ret = tz->ops->get_trip_hyst(tz, trip, &temperature); Same pattern again. > +unlock: > + mutex_unlock(&tz->lock); > + > return ret ? ret : sprintf(buf, "%d\n", temperature); > } > > @@ -269,16 +314,24 @@ emul_temp_store(struct device *dev, struct device_attribute *attr, > if (kstrtoint(buf, 10, &temperature)) > return -EINVAL; > > + mutex_lock(&tz->lock); > + > + if (!device_is_registered(dev)) { > + ret = -ENODEV; > + goto unlock; > + } > + > if (!tz->ops->set_emul_temp) { > - mutex_lock(&tz->lock); > tz->emul_temperature = temperature; > - mutex_unlock(&tz->lock); > } else { > ret = tz->ops->set_emul_temp(tz, temperature); > } Drop the leftover braces that are not necessary now? > > if (!ret) > - thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); > + __thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); > + > +unlock: > + mutex_unlock(&tz->lock); > > return ret ? ret : count; > } > --
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c index 9facd9c5b70f..b8e3b262b2bd 100644 --- a/drivers/thermal/thermal_core.c +++ b/drivers/thermal/thermal_core.c @@ -403,8 +403,8 @@ static void thermal_zone_device_init(struct thermal_zone_device *tz) pos->initialized = false; } -static void __thermal_zone_device_update(struct thermal_zone_device *tz, - enum thermal_notify_event event) +void __thermal_zone_device_update(struct thermal_zone_device *tz, + enum thermal_notify_event event) { int count; diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h index 1571917bd3c8..7b51b9a22e7e 100644 --- a/drivers/thermal/thermal_core.h +++ b/drivers/thermal/thermal_core.h @@ -109,6 +109,8 @@ int thermal_register_governor(struct thermal_governor *); void thermal_unregister_governor(struct thermal_governor *); int thermal_zone_device_set_policy(struct thermal_zone_device *, char *); int thermal_build_list_of_policies(char *buf); +void __thermal_zone_device_update(struct thermal_zone_device *tz, + enum thermal_notify_event event); /* Helpers */ void thermal_zone_set_trips(struct thermal_zone_device *tz); diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c index ec495c7dff03..68607e6070e8 100644 --- a/drivers/thermal/thermal_sysfs.c +++ b/drivers/thermal/thermal_sysfs.c @@ -92,7 +92,15 @@ trip_point_type_show(struct device *dev, struct device_attribute *attr, if (sscanf(attr->attr.name, "trip_point_%d_type", &trip) != 1) return -EINVAL; + mutex_lock(&tz->lock); + + if (!device_is_registered(dev)) { + mutex_unlock(&tz->lock); + return -ENODEV; + } + result = tz->ops->get_trip_type(tz, trip, &type); + mutex_unlock(&tz->lock); if (result) return result; @@ -128,10 +136,17 @@ trip_point_temp_store(struct device *dev, struct device_attribute *attr, if (kstrtoint(buf, 10, &temperature)) return -EINVAL; + mutex_lock(&tz->lock); + + if (!device_is_registered(dev)) { + ret = -ENODEV; + goto unlock; + } + if (tz->ops->set_trip_temp) { ret = tz->ops->set_trip_temp(tz, trip, temperature); if (ret) - return ret; + goto unlock; } if (tz->trips) @@ -140,18 +155,21 @@ trip_point_temp_store(struct device *dev, struct device_attribute *attr, if (tz->ops->get_trip_hyst) { ret = tz->ops->get_trip_hyst(tz, trip, &hyst); if (ret) - return ret; + goto unlock; } ret = tz->ops->get_trip_type(tz, trip, &type); if (ret) - return ret; + goto unlock; thermal_notify_tz_trip_change(tz->id, trip, type, temperature, hyst); - thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); + __thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); - return count; +unlock: + mutex_unlock(&tz->lock); + + return ret ? ret : count; } static ssize_t @@ -168,12 +186,19 @@ trip_point_temp_show(struct device *dev, struct device_attribute *attr, if (sscanf(attr->attr.name, "trip_point_%d_temp", &trip) != 1) return -EINVAL; + mutex_lock(&tz->lock); + + if (!device_is_registered(dev)) { + ret = -ENODEV; + goto unlock; + } + ret = tz->ops->get_trip_temp(tz, trip, &temperature); - if (ret) - return ret; +unlock: + mutex_unlock(&tz->lock); - return sprintf(buf, "%d\n", temperature); + return ret ? ret : sprintf(buf, "%d\n", temperature); } static ssize_t @@ -193,6 +218,13 @@ trip_point_hyst_store(struct device *dev, struct device_attribute *attr, if (kstrtoint(buf, 10, &temperature)) return -EINVAL; + mutex_lock(&tz->lock); + + if (!device_is_registered(dev)) { + ret = -ENODEV; + goto unlock; + } + /* * We are not doing any check on the 'temperature' value * here. The driver implementing 'set_trip_hyst' has to @@ -201,7 +233,10 @@ trip_point_hyst_store(struct device *dev, struct device_attribute *attr, ret = tz->ops->set_trip_hyst(tz, trip, temperature); if (!ret) - thermal_zone_set_trips(tz); + __thermal_zone_set_trips(tz); + +unlock: + mutex_unlock(&tz->lock); return ret ? ret : count; } @@ -220,8 +255,18 @@ trip_point_hyst_show(struct device *dev, struct device_attribute *attr, if (sscanf(attr->attr.name, "trip_point_%d_hyst", &trip) != 1) return -EINVAL; + mutex_lock(&tz->lock); + + if (!device_is_registered(dev)) { + ret = -ENODEV; + goto unlock; + } + ret = tz->ops->get_trip_hyst(tz, trip, &temperature); +unlock: + mutex_unlock(&tz->lock); + return ret ? ret : sprintf(buf, "%d\n", temperature); } @@ -269,16 +314,24 @@ emul_temp_store(struct device *dev, struct device_attribute *attr, if (kstrtoint(buf, 10, &temperature)) return -EINVAL; + mutex_lock(&tz->lock); + + if (!device_is_registered(dev)) { + ret = -ENODEV; + goto unlock; + } + if (!tz->ops->set_emul_temp) { - mutex_lock(&tz->lock); tz->emul_temperature = temperature; - mutex_unlock(&tz->lock); } else { ret = tz->ops->set_emul_temp(tz, temperature); } if (!ret) - thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); + __thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); + +unlock: + mutex_unlock(&tz->lock); return ret ? ret : count; }
Protect access to thermal operations against thermal zone removal by acquiring the thermal zone device mutex. After acquiring the mutex, check if the thermal zone device is registered and abort the operation if not. With this change, we can call __thermal_zone_device_update() instead of thermal_zone_device_update() from trip_point_temp_store() and from emul_temp_store(). Similar, we can call __thermal_zone_set_trips() instead of thermal_zone_set_trips() from trip_point_hyst_store(). Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- drivers/thermal/thermal_core.c | 4 +- drivers/thermal/thermal_core.h | 2 + drivers/thermal/thermal_sysfs.c | 77 ++++++++++++++++++++++++++++----- 3 files changed, 69 insertions(+), 14 deletions(-)