| Message ID | 20221020080048.56377-1-zhangqilong3@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Daniel Lezcano |
| Headers | show |
| Series | thermal/of: Fix possible memleak in thermal_of_zone_register() | expand |
On Thu, Oct 20, 2022 at 04:00:48PM +0800, Zhang Qilong wrote: > diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c > index d4b6335ace15..fc8fa27480a1 100644 > --- a/drivers/thermal/thermal_of.c > +++ b/drivers/thermal/thermal_of.c > @@ -596,10 +596,6 @@ struct thermal_zone_device *thermal_of_zone_register(struct device_node *sensor, > int ntrips, mask; > int ret; > > - of_ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL); > - if (!of_ops) > - return ERR_PTR(-ENOMEM); > - > np = of_thermal_zone_find(sensor, id); > if (IS_ERR(np)) { > if (PTR_ERR(np) != -ENODEV) > @@ -626,6 +622,12 @@ struct thermal_zone_device *thermal_of_zone_register(struct device_node *sensor, > goto out_kfree_trips; > } > > + of_ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL); > + if (!of_ops) { > + ret = -ENOMEM; > + goto out_kfree_tzp; > + } > + > of_ops->get_trip_type = of_ops->get_trip_type ? : of_thermal_get_trip_type; > of_ops->get_trip_temp = of_ops->get_trip_temp ? : of_thermal_get_trip_temp; > of_ops->get_trip_hyst = of_ops->get_trip_hyst ? : of_thermal_get_trip_hyst; > @@ -656,6 +658,7 @@ struct thermal_zone_device *thermal_of_zone_register(struct device_node *sensor, > return tz; > > out_kfree_tzp: > + kfree(of_ops); > kfree(tzp); > out_kfree_trips: > kfree(trips); The patch looks correct, but it can be cleaner. Ideally, you would have a separate label to free 'of_ops' like we have for other variables. Also, the error path is not symmetric with thermal_of_zone_unregister() where this variable is the last to be freed, not the first. I also encountered this issue and posted a patch: https://lore.kernel.org/linux-pm/20221020103658.802457-1-idosch@nvidia.com/ Unless you see something wrong with it, can you please test and see if it fixes your issue? Thanks
> On Thu, Oct 20, 2022 at 04:00:48PM +0800, Zhang Qilong wrote: > > diff --git a/drivers/thermal/thermal_of.c > > b/drivers/thermal/thermal_of.c index d4b6335ace15..fc8fa27480a1 100644 > > --- a/drivers/thermal/thermal_of.c > > +++ b/drivers/thermal/thermal_of.c > > @@ -596,10 +596,6 @@ struct thermal_zone_device > *thermal_of_zone_register(struct device_node *sensor, > > int ntrips, mask; > > int ret; > > > > - of_ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL); > > - if (!of_ops) > > - return ERR_PTR(-ENOMEM); > > - > > np = of_thermal_zone_find(sensor, id); > > if (IS_ERR(np)) { > > if (PTR_ERR(np) != -ENODEV) > > @@ -626,6 +622,12 @@ struct thermal_zone_device > *thermal_of_zone_register(struct device_node *sensor, > > goto out_kfree_trips; > > } > > > > + of_ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL); > > + if (!of_ops) { > > + ret = -ENOMEM; > > + goto out_kfree_tzp; > > + } > > + > > of_ops->get_trip_type = of_ops->get_trip_type ? : > of_thermal_get_trip_type; > > of_ops->get_trip_temp = of_ops->get_trip_temp ? : > of_thermal_get_trip_temp; > > of_ops->get_trip_hyst = of_ops->get_trip_hyst ? : > > of_thermal_get_trip_hyst; @@ -656,6 +658,7 @@ struct > thermal_zone_device *thermal_of_zone_register(struct device_node > *sensor, > > return tz; > > > > out_kfree_tzp: > > + kfree(of_ops); > > kfree(tzp); > > out_kfree_trips: > > kfree(trips); > > The patch looks correct, but it can be cleaner. Ideally, you would have a > separate label to free 'of_ops' like we have for other variables. > Also, the error path is not symmetric with thermal_of_zone_unregister() > where this variable is the last to be freed, not the first. > > I also encountered this issue and posted a patch: > https://lore.kernel.org/linux-pm/20221020103658.802457-1- > idosch@nvidia.com/ > > Unless you see something wrong with it, can you please test and see if it > fixes your issue? > Hi, It looks good to me! Thanks, Zhang > Thanks
diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c index d4b6335ace15..fc8fa27480a1 100644 --- a/drivers/thermal/thermal_of.c +++ b/drivers/thermal/thermal_of.c @@ -596,10 +596,6 @@ struct thermal_zone_device *thermal_of_zone_register(struct device_node *sensor, int ntrips, mask; int ret; - of_ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL); - if (!of_ops) - return ERR_PTR(-ENOMEM); - np = of_thermal_zone_find(sensor, id); if (IS_ERR(np)) { if (PTR_ERR(np) != -ENODEV) @@ -626,6 +622,12 @@ struct thermal_zone_device *thermal_of_zone_register(struct device_node *sensor, goto out_kfree_trips; } + of_ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL); + if (!of_ops) { + ret = -ENOMEM; + goto out_kfree_tzp; + } + of_ops->get_trip_type = of_ops->get_trip_type ? : of_thermal_get_trip_type; of_ops->get_trip_temp = of_ops->get_trip_temp ? : of_thermal_get_trip_temp; of_ops->get_trip_hyst = of_ops->get_trip_hyst ? : of_thermal_get_trip_hyst; @@ -656,6 +658,7 @@ struct thermal_zone_device *thermal_of_zone_register(struct device_node *sensor, return tz; out_kfree_tzp: + kfree(of_ops); kfree(tzp); out_kfree_trips: kfree(trips);
In the error path, we forget to free the memory that allocated to of_ops in thermal_of_zone_register(), it can cause memleak when error returns. We fix it by moving kmemdup to the front of using it and freeing it in the later error path. Fixes: 3fd6d6e2b4e8 ("thermal/of: Rework the thermal device tree initialization") Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com> --- drivers/thermal/thermal_of.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)