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 <[email protected]>
Signed-off-by: Viresh Kumar <[email protected]>
---
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);
--
2.31.1.272.g89b43f80a514
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 <[email protected]>
> Signed-off-by: Viresh Kumar <[email protected]>
> ---
> 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 <[email protected]>
On Wed, Jan 18, 2023 at 9:38 AM Viresh Kumar <[email protected]> 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 <[email protected]>
> Signed-off-by: Viresh Kumar <[email protected]>
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?
> ---
> 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);
> --
> 2.31.1.272.g89b43f80a514
>
On 18-01-23, 20:58, Rafael J. Wysocki wrote:
> On Wed, Jan 18, 2023 at 9:38 AM Viresh Kumar <[email protected]> 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 <[email protected]>
> > Signed-off-by: Viresh Kumar <[email protected]>
>
> 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.
--
viresh
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 <[email protected]>
> Signed-off-by: Viresh Kumar <[email protected]>
> ---
Reviewed-by: Yang Yingliang <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Viresh Kumar <[email protected]>
Tested-by: Caleb Connolly <[email protected]>
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);
--
Kind Regards,
Caleb (they/them)
On Thu, Jan 19, 2023 at 6:16 AM Viresh Kumar <[email protected]> wrote:
>
> On 18-01-23, 20:58, Rafael J. Wysocki wrote:
> > On Wed, Jan 18, 2023 at 9:38 AM Viresh Kumar <[email protected]> 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 <[email protected]>
> > > Signed-off-by: Viresh Kumar <[email protected]>
> >
> > 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 <[email protected]> wrote:
>
> On Thu, Jan 19, 2023 at 6:16 AM Viresh Kumar <[email protected]> wrote:
> >
> > On 18-01-23, 20:58, Rafael J. Wysocki wrote:
> > > On Wed, Jan 18, 2023 at 9:38 AM Viresh Kumar <[email protected]> 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 <[email protected]>
> > > > Signed-off-by: Viresh Kumar <[email protected]>
> > >
> > > 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!