| Message ID | d6e5d4fcca5f66d290e907d10c45cb2e7bbb09e5.1674030722.git.viresh.kumar@linaro.org |
|---|---|
| State | Accepted |
| Commit | 6c54b7bc8a31ce0f7cc7f8deef05067df414f1d8 |
| Headers | show |
| Series | thermal: Fix/cleanup error paths in __thermal_cooling_device_register() | expand |
On 1/18/23 02:38, Viresh Kumar wrote: > put_device() shouldn't be called before a prior call to > device_register(). __thermal_cooling_device_register() doesn't follow > that properly and needs fixing. Also > thermal_cooling_device_destroy_sysfs() is getting called unnecessarily > on few error paths. > > Fix all this by placing the calls at the right place. > > Based on initial work done by Caleb Connolly. > > Fixes: 4748f9687caa ("thermal: core: fix some possible name leaks in error paths") > Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()") > Reported-by: Caleb Connolly <caleb.connolly@linaro.org> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > For v6.2-rc. > > V3->V4: > - The first three versions were sent by Caleb. > - The new version fixes the current bugs, without looking to optimize the > code any further, which is done separately in the next two patches. > > drivers/thermal/thermal_core.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c > index f17ab2316dbd..77bd47d976a2 100644 > --- a/drivers/thermal/thermal_core.c > +++ b/drivers/thermal/thermal_core.c > @@ -909,15 +909,20 @@ __thermal_cooling_device_register(struct device_node *np, > cdev->devdata = devdata; > > ret = cdev->ops->get_max_state(cdev, &cdev->max_state); > - if (ret) > - goto out_kfree_type; > + if (ret) { > + kfree(cdev->type); > + goto out_ida_remove; > + } > > thermal_cooling_device_setup_sysfs(cdev); > + > ret = dev_set_name(&cdev->device, "cooling_device%d", cdev->id); > if (ret) { > + kfree(cdev->type); > thermal_cooling_device_destroy_sysfs(cdev); > - goto out_kfree_type; > + goto out_ida_remove; > } > + > ret = device_register(&cdev->device); > if (ret) > goto out_kfree_type; > @@ -943,6 +948,8 @@ __thermal_cooling_device_register(struct device_node *np, > thermal_cooling_device_destroy_sysfs(cdev); > kfree(cdev->type); > put_device(&cdev->device); > + > + /* thermal_release() takes care of the rest */ > cdev = NULL; > out_ida_remove: > ida_free(&thermal_cdev_ida, id); My testing: Applied on top of v6.2-rc1 The configuration is qcom_defconfig The system is a Qualcomm Dragon 8074 The two WARNING stack traces no longer occur after applying the patch. Tested-by: Frank Rowand <frowand.list@gmail.com>
On 18-01-23, 20:58, Rafael J. Wysocki wrote: > On Wed, Jan 18, 2023 at 9:38 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > put_device() shouldn't be called before a prior call to > > device_register(). __thermal_cooling_device_register() doesn't follow > > that properly and needs fixing. Also > > thermal_cooling_device_destroy_sysfs() is getting called unnecessarily > > on few error paths. > > > > Fix all this by placing the calls at the right place. > > > > Based on initial work done by Caleb Connolly. > > > > Fixes: 4748f9687caa ("thermal: core: fix some possible name leaks in error paths") > > Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()") > > Reported-by: Caleb Connolly <caleb.connolly@linaro.org> > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > OK, so I think that this patch is needed for 6.2 and the other two may > be queued up for later (they do depend on this one, though, of > course). Is my understanding correct? Right.
On 2023/1/18 16:38, Viresh Kumar wrote: > put_device() shouldn't be called before a prior call to > device_register(). __thermal_cooling_device_register() doesn't follow > that properly and needs fixing. Also > thermal_cooling_device_destroy_sysfs() is getting called unnecessarily > on few error paths. > > Fix all this by placing the calls at the right place. > > Based on initial work done by Caleb Connolly. > > Fixes: 4748f9687caa ("thermal: core: fix some possible name leaks in error paths") > Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()") > Reported-by: Caleb Connolly <caleb.connolly@linaro.org> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- Reviewed-by: Yang Yingliang <yangyingliang@huawei.com> > For v6.2-rc. > > V3->V4: > - The first three versions were sent by Caleb. > - The new version fixes the current bugs, without looking to optimize the > code any further, which is done separately in the next two patches. > > drivers/thermal/thermal_core.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c > index f17ab2316dbd..77bd47d976a2 100644 > --- a/drivers/thermal/thermal_core.c > +++ b/drivers/thermal/thermal_core.c > @@ -909,15 +909,20 @@ __thermal_cooling_device_register(struct device_node *np, > cdev->devdata = devdata; > > ret = cdev->ops->get_max_state(cdev, &cdev->max_state); > - if (ret) > - goto out_kfree_type; > + if (ret) { > + kfree(cdev->type); > + goto out_ida_remove; > + } > > thermal_cooling_device_setup_sysfs(cdev); > + > ret = dev_set_name(&cdev->device, "cooling_device%d", cdev->id); > if (ret) { > + kfree(cdev->type); > thermal_cooling_device_destroy_sysfs(cdev); > - goto out_kfree_type; > + goto out_ida_remove; > } > + > ret = device_register(&cdev->device); > if (ret) > goto out_kfree_type; > @@ -943,6 +948,8 @@ __thermal_cooling_device_register(struct device_node *np, > thermal_cooling_device_destroy_sysfs(cdev); > kfree(cdev->type); > put_device(&cdev->device); > + > + /* thermal_release() takes care of the rest */ > cdev = NULL; > out_ida_remove: > ida_free(&thermal_cdev_ida, id);
On 18/01/2023 08:38, Viresh Kumar wrote: > put_device() shouldn't be called before a prior call to > device_register(). __thermal_cooling_device_register() doesn't follow > that properly and needs fixing. Also > thermal_cooling_device_destroy_sysfs() is getting called unnecessarily > on few error paths. > > Fix all this by placing the calls at the right place. > > Based on initial work done by Caleb Connolly. > > Fixes: 4748f9687caa ("thermal: core: fix some possible name leaks in error paths") > Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()") > Reported-by: Caleb Connolly <caleb.connolly@linaro.org> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> Tested-by: Caleb Connolly <caleb.connolly@linaro.org> Thanks for sending this, with this I no longer hit the splats when get_max_state() fails. > --- > For v6.2-rc. > > V3->V4: > - The first three versions were sent by Caleb. > - The new version fixes the current bugs, without looking to optimize the > code any further, which is done separately in the next two patches. > > drivers/thermal/thermal_core.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c > index f17ab2316dbd..77bd47d976a2 100644 > --- a/drivers/thermal/thermal_core.c > +++ b/drivers/thermal/thermal_core.c > @@ -909,15 +909,20 @@ __thermal_cooling_device_register(struct device_node *np, > cdev->devdata = devdata; > > ret = cdev->ops->get_max_state(cdev, &cdev->max_state); > - if (ret) > - goto out_kfree_type; > + if (ret) { > + kfree(cdev->type); > + goto out_ida_remove; > + } > > thermal_cooling_device_setup_sysfs(cdev); > + > ret = dev_set_name(&cdev->device, "cooling_device%d", cdev->id); > if (ret) { > + kfree(cdev->type); > thermal_cooling_device_destroy_sysfs(cdev); > - goto out_kfree_type; > + goto out_ida_remove; > } > + > ret = device_register(&cdev->device); > if (ret) > goto out_kfree_type; > @@ -943,6 +948,8 @@ __thermal_cooling_device_register(struct device_node *np, > thermal_cooling_device_destroy_sysfs(cdev); > kfree(cdev->type); > put_device(&cdev->device); > + > + /* thermal_release() takes care of the rest */ > cdev = NULL; > out_ida_remove: > ida_free(&thermal_cdev_ida, id);
On Thu, Jan 19, 2023 at 6:16 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 18-01-23, 20:58, Rafael J. Wysocki wrote: > > On Wed, Jan 18, 2023 at 9:38 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > > > put_device() shouldn't be called before a prior call to > > > device_register(). __thermal_cooling_device_register() doesn't follow > > > that properly and needs fixing. Also > > > thermal_cooling_device_destroy_sysfs() is getting called unnecessarily > > > on few error paths. > > > > > > Fix all this by placing the calls at the right place. > > > > > > Based on initial work done by Caleb Connolly. > > > > > > Fixes: 4748f9687caa ("thermal: core: fix some possible name leaks in error paths") > > > Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()") > > > Reported-by: Caleb Connolly <caleb.connolly@linaro.org> > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > > > OK, so I think that this patch is needed for 6.2 and the other two may > > be queued up for later (they do depend on this one, though, of > > course). Is my understanding correct? > > Right. OK, applied as 6.2-rc material and I'll get to the other two when this goes in.
On Thu, Jan 19, 2023 at 9:09 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Thu, Jan 19, 2023 at 6:16 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > On 18-01-23, 20:58, Rafael J. Wysocki wrote: > > > On Wed, Jan 18, 2023 at 9:38 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > > > > > put_device() shouldn't be called before a prior call to > > > > device_register(). __thermal_cooling_device_register() doesn't follow > > > > that properly and needs fixing. Also > > > > thermal_cooling_device_destroy_sysfs() is getting called unnecessarily > > > > on few error paths. > > > > > > > > Fix all this by placing the calls at the right place. > > > > > > > > Based on initial work done by Caleb Connolly. > > > > > > > > Fixes: 4748f9687caa ("thermal: core: fix some possible name leaks in error paths") > > > > Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()") > > > > Reported-by: Caleb Connolly <caleb.connolly@linaro.org> > > > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > > > > > OK, so I think that this patch is needed for 6.2 and the other two may > > > be queued up for later (they do depend on this one, though, of > > > course). Is my understanding correct? > > > > Right. > > OK, applied as 6.2-rc material and I'll get to the other two when this goes in. Patches [2-3/3] from this series have been applied as 6.3 material now, thanks!
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c index f17ab2316dbd..77bd47d976a2 100644 --- a/drivers/thermal/thermal_core.c +++ b/drivers/thermal/thermal_core.c @@ -909,15 +909,20 @@ __thermal_cooling_device_register(struct device_node *np, cdev->devdata = devdata; ret = cdev->ops->get_max_state(cdev, &cdev->max_state); - if (ret) - goto out_kfree_type; + if (ret) { + kfree(cdev->type); + goto out_ida_remove; + } thermal_cooling_device_setup_sysfs(cdev); + ret = dev_set_name(&cdev->device, "cooling_device%d", cdev->id); if (ret) { + kfree(cdev->type); thermal_cooling_device_destroy_sysfs(cdev); - goto out_kfree_type; + goto out_ida_remove; } + ret = device_register(&cdev->device); if (ret) goto out_kfree_type; @@ -943,6 +948,8 @@ __thermal_cooling_device_register(struct device_node *np, thermal_cooling_device_destroy_sysfs(cdev); kfree(cdev->type); put_device(&cdev->device); + + /* thermal_release() takes care of the rest */ cdev = NULL; out_ida_remove: ida_free(&thermal_cdev_ida, id);
put_device() shouldn't be called before a prior call to device_register(). __thermal_cooling_device_register() doesn't follow that properly and needs fixing. Also thermal_cooling_device_destroy_sysfs() is getting called unnecessarily on few error paths. Fix all this by placing the calls at the right place. Based on initial work done by Caleb Connolly. Fixes: 4748f9687caa ("thermal: core: fix some possible name leaks in error paths") Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()") Reported-by: Caleb Connolly <caleb.connolly@linaro.org> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- For v6.2-rc. V3->V4: - The first three versions were sent by Caleb. - The new version fixes the current bugs, without looking to optimize the code any further, which is done separately in the next two patches. drivers/thermal/thermal_core.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)