Since commit 3d439b1a2ad3 ("thermal/core: Alloc-copy-free the thermal zone
parameters structure"), thermal_zone_device_register() allocates a copy
of the tzp argument and callers need not explicitly manage its lifetime.
This means the function no longer cares about the parameter being
mutable, so constify it.
No functional change.
Fixes: 3d439b1a2ad3 ("thermal/core: Alloc-copy-free the thermal zone parameters structure")
Signed-off-by: Ahmad Fatoum <[email protected]>
---
drivers/thermal/thermal_core.c | 4 ++--
include/linux/thermal.h | 6 +++---
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 842f678c1c3e..cc2b5e81c620 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -1203,7 +1203,7 @@ EXPORT_SYMBOL_GPL(thermal_zone_get_crit_temp);
struct thermal_zone_device *
thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *trips, int num_trips, int mask,
void *devdata, struct thermal_zone_device_ops *ops,
- struct thermal_zone_params *tzp, int passive_delay,
+ const struct thermal_zone_params *tzp, int passive_delay,
int polling_delay)
{
struct thermal_zone_device *tz;
@@ -1371,7 +1371,7 @@ EXPORT_SYMBOL_GPL(thermal_zone_device_register_with_trips);
struct thermal_zone_device *thermal_zone_device_register(const char *type, int ntrips, int mask,
void *devdata, struct thermal_zone_device_ops *ops,
- struct thermal_zone_params *tzp, int passive_delay,
+ const struct thermal_zone_params *tzp, int passive_delay,
int polling_delay)
{
return thermal_zone_device_register_with_trips(type, NULL, ntrips, mask,
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 87837094d549..dee66ade89a0 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -301,14 +301,14 @@ int thermal_acpi_critical_trip_temp(struct acpi_device *adev, int *ret_temp);
#ifdef CONFIG_THERMAL
struct thermal_zone_device *thermal_zone_device_register(const char *, int, int,
void *, struct thermal_zone_device_ops *,
- struct thermal_zone_params *, int, int);
+ const struct thermal_zone_params *, int, int);
void thermal_zone_device_unregister(struct thermal_zone_device *);
struct thermal_zone_device *
thermal_zone_device_register_with_trips(const char *, struct thermal_trip *, int, int,
void *, struct thermal_zone_device_ops *,
- struct thermal_zone_params *, int, int);
+ const struct thermal_zone_params *, int, int);
void *thermal_zone_device_priv(struct thermal_zone_device *tzd);
const char *thermal_zone_device_type(struct thermal_zone_device *tzd);
@@ -348,7 +348,7 @@ void thermal_zone_device_critical(struct thermal_zone_device *tz);
static inline struct thermal_zone_device *thermal_zone_device_register(
const char *type, int trips, int mask, void *devdata,
struct thermal_zone_device_ops *ops,
- struct thermal_zone_params *tzp,
+ const struct thermal_zone_params *tzp,
int passive_delay, int polling_delay)
{ return ERR_PTR(-ENODEV); }
static inline void thermal_zone_device_unregister(
--
2.39.2
Since commit 3d439b1a2ad3 ("thermal/core: Alloc-copy-free the thermal
zone parameters structure"), thermal_zone_device_register() allocates
a copy of the tzp argument and frees it when unregistering, so
thermal_of_zone_register() now ends up leaking its original tzp and
double-freeing the tzp copy. Fix this by locating tzp on stack instead.
Fixes: 3d439b1a2ad3 ("thermal/core: Alloc-copy-free the thermal zone parameters structure")
Signed-off-by: Ahmad Fatoum <[email protected]>
---
drivers/thermal/thermal_of.c | 27 ++++++---------------------
1 file changed, 6 insertions(+), 21 deletions(-)
diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
index 6fb14e521197..bc07ae1c284c 100644
--- a/drivers/thermal/thermal_of.c
+++ b/drivers/thermal/thermal_of.c
@@ -238,17 +238,13 @@ static int thermal_of_monitor_init(struct device_node *np, int *delay, int *pdel
return 0;
}
-static struct thermal_zone_params *thermal_of_parameters_init(struct device_node *np)
+static void thermal_of_parameters_init(struct device_node *np,
+ struct thermal_zone_params *tzp)
{
- struct thermal_zone_params *tzp;
int coef[2];
int ncoef = ARRAY_SIZE(coef);
int prop, ret;
- tzp = kzalloc(sizeof(*tzp), GFP_KERNEL);
- if (!tzp)
- return ERR_PTR(-ENOMEM);
-
tzp->no_hwmon = true;
if (!of_property_read_u32(np, "sustainable-power", &prop))
@@ -267,8 +263,6 @@ static struct thermal_zone_params *thermal_of_parameters_init(struct device_node
tzp->slope = coef[0];
tzp->offset = coef[1];
-
- return tzp;
}
static struct device_node *thermal_of_zone_get_by_name(struct thermal_zone_device *tz)
@@ -442,13 +436,11 @@ static int thermal_of_unbind(struct thermal_zone_device *tz,
static void thermal_of_zone_unregister(struct thermal_zone_device *tz)
{
struct thermal_trip *trips = tz->trips;
- struct thermal_zone_params *tzp = tz->tzp;
struct thermal_zone_device_ops *ops = tz->ops;
thermal_zone_device_disable(tz);
thermal_zone_device_unregister(tz);
kfree(trips);
- kfree(tzp);
kfree(ops);
}
@@ -477,7 +469,7 @@ static struct thermal_zone_device *thermal_of_zone_register(struct device_node *
{
struct thermal_zone_device *tz;
struct thermal_trip *trips;
- struct thermal_zone_params *tzp;
+ struct thermal_zone_params tzp = {};
struct thermal_zone_device_ops *of_ops;
struct device_node *np;
int delay, pdelay;
@@ -509,12 +501,7 @@ static struct thermal_zone_device *thermal_of_zone_register(struct device_node *
goto out_kfree_trips;
}
- tzp = thermal_of_parameters_init(np);
- if (IS_ERR(tzp)) {
- ret = PTR_ERR(tzp);
- pr_err("Failed to initialize parameter from %pOFn: %d\n", np, ret);
- goto out_kfree_trips;
- }
+ thermal_of_parameters_init(np, &tzp);
of_ops->bind = thermal_of_bind;
of_ops->unbind = thermal_of_unbind;
@@ -522,12 +509,12 @@ static struct thermal_zone_device *thermal_of_zone_register(struct device_node *
mask = GENMASK_ULL((ntrips) - 1, 0);
tz = thermal_zone_device_register_with_trips(np->name, trips, ntrips,
- mask, data, of_ops, tzp,
+ mask, data, of_ops, &tzp,
pdelay, delay);
if (IS_ERR(tz)) {
ret = PTR_ERR(tz);
pr_err("Failed to register thermal zone %pOFn: %d\n", np, ret);
- goto out_kfree_tzp;
+ goto out_kfree_trips;
}
ret = thermal_zone_device_enable(tz);
@@ -540,8 +527,6 @@ static struct thermal_zone_device *thermal_of_zone_register(struct device_node *
return tz;
-out_kfree_tzp:
- kfree(tzp);
out_kfree_trips:
kfree(trips);
out_kfree_of_ops:
--
2.39.2
On Sat, Jul 8, 2023 at 1:27 PM Ahmad Fatoum <[email protected]> wrote:
>
> Since commit 3d439b1a2ad3 ("thermal/core: Alloc-copy-free the thermal zone
> parameters structure"), thermal_zone_device_register() allocates a copy
> of the tzp argument and callers need not explicitly manage its lifetime.
>
> This means the function no longer cares about the parameter being
> mutable, so constify it.
>
> No functional change.
>
> Fixes: 3d439b1a2ad3 ("thermal/core: Alloc-copy-free the thermal zone parameters structure")
Why is this particular patch regarded as a fix?
> Signed-off-by: Ahmad Fatoum <[email protected]>
> ---
> drivers/thermal/thermal_core.c | 4 ++--
> include/linux/thermal.h | 6 +++---
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index 842f678c1c3e..cc2b5e81c620 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -1203,7 +1203,7 @@ EXPORT_SYMBOL_GPL(thermal_zone_get_crit_temp);
> struct thermal_zone_device *
> thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *trips, int num_trips, int mask,
> void *devdata, struct thermal_zone_device_ops *ops,
> - struct thermal_zone_params *tzp, int passive_delay,
> + const struct thermal_zone_params *tzp, int passive_delay,
> int polling_delay)
> {
> struct thermal_zone_device *tz;
> @@ -1371,7 +1371,7 @@ EXPORT_SYMBOL_GPL(thermal_zone_device_register_with_trips);
>
> struct thermal_zone_device *thermal_zone_device_register(const char *type, int ntrips, int mask,
> void *devdata, struct thermal_zone_device_ops *ops,
> - struct thermal_zone_params *tzp, int passive_delay,
> + const struct thermal_zone_params *tzp, int passive_delay,
> int polling_delay)
> {
> return thermal_zone_device_register_with_trips(type, NULL, ntrips, mask,
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 87837094d549..dee66ade89a0 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -301,14 +301,14 @@ int thermal_acpi_critical_trip_temp(struct acpi_device *adev, int *ret_temp);
> #ifdef CONFIG_THERMAL
> struct thermal_zone_device *thermal_zone_device_register(const char *, int, int,
> void *, struct thermal_zone_device_ops *,
> - struct thermal_zone_params *, int, int);
> + const struct thermal_zone_params *, int, int);
>
> void thermal_zone_device_unregister(struct thermal_zone_device *);
>
> struct thermal_zone_device *
> thermal_zone_device_register_with_trips(const char *, struct thermal_trip *, int, int,
> void *, struct thermal_zone_device_ops *,
> - struct thermal_zone_params *, int, int);
> + const struct thermal_zone_params *, int, int);
>
> void *thermal_zone_device_priv(struct thermal_zone_device *tzd);
> const char *thermal_zone_device_type(struct thermal_zone_device *tzd);
> @@ -348,7 +348,7 @@ void thermal_zone_device_critical(struct thermal_zone_device *tz);
> static inline struct thermal_zone_device *thermal_zone_device_register(
> const char *type, int trips, int mask, void *devdata,
> struct thermal_zone_device_ops *ops,
> - struct thermal_zone_params *tzp,
> + const struct thermal_zone_params *tzp,
> int passive_delay, int polling_delay)
> { return ERR_PTR(-ENODEV); }
> static inline void thermal_zone_device_unregister(
> --
> 2.39.2
>
Hello,
On 10.07.23 19:33, Rafael J. Wysocki wrote:
> On Sat, Jul 8, 2023 at 1:27 PM Ahmad Fatoum <[email protected]> wrote:
>>
>> Since commit 3d439b1a2ad3 ("thermal/core: Alloc-copy-free the thermal zone
>> parameters structure"), thermal_zone_device_register() allocates a copy
>> of the tzp argument and callers need not explicitly manage its lifetime.
>>
>> This means the function no longer cares about the parameter being
>> mutable, so constify it.
>>
>> No functional change.
>>
>> Fixes: 3d439b1a2ad3 ("thermal/core: Alloc-copy-free the thermal zone parameters structure")
>
> Why is this particular patch regarded as a fix?
That the prototype wasn't adjust in aforementioned commit is IMO an oversight,
so this commit fixes that. As the commit is already referenced in the commit
message body, I don't mind dropping the Fixes: line. Please let me know if
I should resend.
Cheers,
Ahmad
>> Signed-off-by: Ahmad Fatoum <[email protected]>
>> ---
>> drivers/thermal/thermal_core.c | 4 ++--
>> include/linux/thermal.h | 6 +++---
>> 2 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>> index 842f678c1c3e..cc2b5e81c620 100644
>> --- a/drivers/thermal/thermal_core.c
>> +++ b/drivers/thermal/thermal_core.c
>> @@ -1203,7 +1203,7 @@ EXPORT_SYMBOL_GPL(thermal_zone_get_crit_temp);
>> struct thermal_zone_device *
>> thermal_zone_device_register_with_trips(const char *type, struct thermal_trip *trips, int num_trips, int mask,
>> void *devdata, struct thermal_zone_device_ops *ops,
>> - struct thermal_zone_params *tzp, int passive_delay,
>> + const struct thermal_zone_params *tzp, int passive_delay,
>> int polling_delay)
>> {
>> struct thermal_zone_device *tz;
>> @@ -1371,7 +1371,7 @@ EXPORT_SYMBOL_GPL(thermal_zone_device_register_with_trips);
>>
>> struct thermal_zone_device *thermal_zone_device_register(const char *type, int ntrips, int mask,
>> void *devdata, struct thermal_zone_device_ops *ops,
>> - struct thermal_zone_params *tzp, int passive_delay,
>> + const struct thermal_zone_params *tzp, int passive_delay,
>> int polling_delay)
>> {
>> return thermal_zone_device_register_with_trips(type, NULL, ntrips, mask,
>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>> index 87837094d549..dee66ade89a0 100644
>> --- a/include/linux/thermal.h
>> +++ b/include/linux/thermal.h
>> @@ -301,14 +301,14 @@ int thermal_acpi_critical_trip_temp(struct acpi_device *adev, int *ret_temp);
>> #ifdef CONFIG_THERMAL
>> struct thermal_zone_device *thermal_zone_device_register(const char *, int, int,
>> void *, struct thermal_zone_device_ops *,
>> - struct thermal_zone_params *, int, int);
>> + const struct thermal_zone_params *, int, int);
>>
>> void thermal_zone_device_unregister(struct thermal_zone_device *);
>>
>> struct thermal_zone_device *
>> thermal_zone_device_register_with_trips(const char *, struct thermal_trip *, int, int,
>> void *, struct thermal_zone_device_ops *,
>> - struct thermal_zone_params *, int, int);
>> + const struct thermal_zone_params *, int, int);
>>
>> void *thermal_zone_device_priv(struct thermal_zone_device *tzd);
>> const char *thermal_zone_device_type(struct thermal_zone_device *tzd);
>> @@ -348,7 +348,7 @@ void thermal_zone_device_critical(struct thermal_zone_device *tz);
>> static inline struct thermal_zone_device *thermal_zone_device_register(
>> const char *type, int trips, int mask, void *devdata,
>> struct thermal_zone_device_ops *ops,
>> - struct thermal_zone_params *tzp,
>> + const struct thermal_zone_params *tzp,
>> int passive_delay, int polling_delay)
>> { return ERR_PTR(-ENODEV); }
>> static inline void thermal_zone_device_unregister(
>> --
>> 2.39.2
>>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Sat, Jul 8, 2023 at 1:27 PM Ahmad Fatoum <[email protected]> wrote:
>
> Since commit 3d439b1a2ad3 ("thermal/core: Alloc-copy-free the thermal
> zone parameters structure"), thermal_zone_device_register() allocates
> a copy of the tzp argument and frees it when unregistering, so
> thermal_of_zone_register() now ends up leaking its original tzp and
> double-freeing the tzp copy. Fix this by locating tzp on stack instead.
>
> Fixes: 3d439b1a2ad3 ("thermal/core: Alloc-copy-free the thermal zone parameters structure")
> Signed-off-by: Ahmad Fatoum <[email protected]>
Daniel, this looks like a genuine fix to me, so I'm inclined to pick
it up directly.
Any objections?
> ---
> drivers/thermal/thermal_of.c | 27 ++++++---------------------
> 1 file changed, 6 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
> index 6fb14e521197..bc07ae1c284c 100644
> --- a/drivers/thermal/thermal_of.c
> +++ b/drivers/thermal/thermal_of.c
> @@ -238,17 +238,13 @@ static int thermal_of_monitor_init(struct device_node *np, int *delay, int *pdel
> return 0;
> }
>
> -static struct thermal_zone_params *thermal_of_parameters_init(struct device_node *np)
> +static void thermal_of_parameters_init(struct device_node *np,
> + struct thermal_zone_params *tzp)
> {
> - struct thermal_zone_params *tzp;
> int coef[2];
> int ncoef = ARRAY_SIZE(coef);
> int prop, ret;
>
> - tzp = kzalloc(sizeof(*tzp), GFP_KERNEL);
> - if (!tzp)
> - return ERR_PTR(-ENOMEM);
> -
> tzp->no_hwmon = true;
>
> if (!of_property_read_u32(np, "sustainable-power", &prop))
> @@ -267,8 +263,6 @@ static struct thermal_zone_params *thermal_of_parameters_init(struct device_node
>
> tzp->slope = coef[0];
> tzp->offset = coef[1];
> -
> - return tzp;
> }
>
> static struct device_node *thermal_of_zone_get_by_name(struct thermal_zone_device *tz)
> @@ -442,13 +436,11 @@ static int thermal_of_unbind(struct thermal_zone_device *tz,
> static void thermal_of_zone_unregister(struct thermal_zone_device *tz)
> {
> struct thermal_trip *trips = tz->trips;
> - struct thermal_zone_params *tzp = tz->tzp;
> struct thermal_zone_device_ops *ops = tz->ops;
>
> thermal_zone_device_disable(tz);
> thermal_zone_device_unregister(tz);
> kfree(trips);
> - kfree(tzp);
> kfree(ops);
> }
>
> @@ -477,7 +469,7 @@ static struct thermal_zone_device *thermal_of_zone_register(struct device_node *
> {
> struct thermal_zone_device *tz;
> struct thermal_trip *trips;
> - struct thermal_zone_params *tzp;
> + struct thermal_zone_params tzp = {};
> struct thermal_zone_device_ops *of_ops;
> struct device_node *np;
> int delay, pdelay;
> @@ -509,12 +501,7 @@ static struct thermal_zone_device *thermal_of_zone_register(struct device_node *
> goto out_kfree_trips;
> }
>
> - tzp = thermal_of_parameters_init(np);
> - if (IS_ERR(tzp)) {
> - ret = PTR_ERR(tzp);
> - pr_err("Failed to initialize parameter from %pOFn: %d\n", np, ret);
> - goto out_kfree_trips;
> - }
> + thermal_of_parameters_init(np, &tzp);
>
> of_ops->bind = thermal_of_bind;
> of_ops->unbind = thermal_of_unbind;
> @@ -522,12 +509,12 @@ static struct thermal_zone_device *thermal_of_zone_register(struct device_node *
> mask = GENMASK_ULL((ntrips) - 1, 0);
>
> tz = thermal_zone_device_register_with_trips(np->name, trips, ntrips,
> - mask, data, of_ops, tzp,
> + mask, data, of_ops, &tzp,
> pdelay, delay);
> if (IS_ERR(tz)) {
> ret = PTR_ERR(tz);
> pr_err("Failed to register thermal zone %pOFn: %d\n", np, ret);
> - goto out_kfree_tzp;
> + goto out_kfree_trips;
> }
>
> ret = thermal_zone_device_enable(tz);
> @@ -540,8 +527,6 @@ static struct thermal_zone_device *thermal_of_zone_register(struct device_node *
>
> return tz;
>
> -out_kfree_tzp:
> - kfree(tzp);
> out_kfree_trips:
> kfree(trips);
> out_kfree_of_ops:
> --
On 11/07/2023 20:05, Rafael J. Wysocki wrote:
> On Sat, Jul 8, 2023 at 1:27 PM Ahmad Fatoum <[email protected]> wrote:
>>
>> Since commit 3d439b1a2ad3 ("thermal/core: Alloc-copy-free the thermal
>> zone parameters structure"), thermal_zone_device_register() allocates
>> a copy of the tzp argument and frees it when unregistering, so
>> thermal_of_zone_register() now ends up leaking its original tzp and
>> double-freeing the tzp copy. Fix this by locating tzp on stack instead.
>>
>> Fixes: 3d439b1a2ad3 ("thermal/core: Alloc-copy-free the thermal zone parameters structure")
>> Signed-off-by: Ahmad Fatoum <[email protected]>
>
> Daniel, this looks like a genuine fix to me, so I'm inclined to pick
> it up directly.
>
> Any objections?
>
No objection
Acked-by: Daniel Lezcano <[email protected]>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On 08/07/2023 13:27, Ahmad Fatoum wrote:
> Since commit 3d439b1a2ad3 ("thermal/core: Alloc-copy-free the thermal zone
> parameters structure"), thermal_zone_device_register() allocates a copy
> of the tzp argument and callers need not explicitly manage its lifetime.
>
> This means the function no longer cares about the parameter being
> mutable, so constify it.
>
> No functional change.
>
> Fixes: 3d439b1a2ad3 ("thermal/core: Alloc-copy-free the thermal zone parameters structure")
> Signed-off-by: Ahmad Fatoum <[email protected]>
> ---
Acked-by: Daniel Lezcano <[email protected]>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On Thu, Jul 13, 2023 at 2:55 PM Daniel Lezcano
<[email protected]> wrote:
>
> On 08/07/2023 13:27, Ahmad Fatoum wrote:
> > Since commit 3d439b1a2ad3 ("thermal/core: Alloc-copy-free the thermal zone
> > parameters structure"), thermal_zone_device_register() allocates a copy
> > of the tzp argument and callers need not explicitly manage its lifetime.
> >
> > This means the function no longer cares about the parameter being
> > mutable, so constify it.
> >
> > No functional change.
> >
> > Fixes: 3d439b1a2ad3 ("thermal/core: Alloc-copy-free the thermal zone parameters structure")
> > Signed-off-by: Ahmad Fatoum <[email protected]>
> > ---
>
> Acked-by: Daniel Lezcano <[email protected]>
Both patches in the series applied as 6.5-rc material, thanks!