Message ID | 20221231210301.6968-1-caleb.connolly@linaro.org |
---|---|
State | Superseded |
Headers | show |
Series | thermal/core: fix potential unbalanced put_device during register | expand |
On 31-12-22, 21:03, Caleb Connolly wrote: > Commit c408b3d1d9bb ("thermal: Validate new state in cur_state_store()") > causes device_put() to be called if the get_max_state() callback fails > during __thermal_cooling_device_register(). > > Fix the cleanup ordering to only call device_put() if initialization > fails after the matching device_register() call. > > Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()") > Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> > --- > drivers/thermal/thermal_core.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c > index f17ab2316dbd..2c6995b5dcb0 100644 > --- a/drivers/thermal/thermal_core.c > +++ b/drivers/thermal/thermal_core.c > @@ -920,7 +920,7 @@ __thermal_cooling_device_register(struct device_node *np, > } > ret = device_register(&cdev->device); > if (ret) > - goto out_kfree_type; > + goto out_put_device; > > /* Add 'this' new cdev to the global cdev list */ > mutex_lock(&thermal_list_lock); > @@ -939,10 +939,11 @@ __thermal_cooling_device_register(struct device_node *np, > > return cdev; > > +out_put_device: > + put_device(&cdev->device); > out_kfree_type: > thermal_cooling_device_destroy_sysfs(cdev); What about this one ? This shouldn't be called in case get_max_state() fails, right ? > kfree(cdev->type); > - put_device(&cdev->device); > cdev = NULL; > out_ida_remove: > ida_free(&thermal_cdev_ida, id);
On 02/01/2023 05:31, Viresh Kumar wrote: > On 31-12-22, 21:03, Caleb Connolly wrote: >> Commit c408b3d1d9bb ("thermal: Validate new state in cur_state_store()") >> causes device_put() to be called if the get_max_state() callback fails >> during __thermal_cooling_device_register(). >> >> Fix the cleanup ordering to only call device_put() if initialization >> fails after the matching device_register() call. >> >> Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()") >> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> >> --- >> drivers/thermal/thermal_core.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c >> index f17ab2316dbd..2c6995b5dcb0 100644 >> --- a/drivers/thermal/thermal_core.c >> +++ b/drivers/thermal/thermal_core.c >> @@ -920,7 +920,7 @@ __thermal_cooling_device_register(struct device_node *np, >> } >> ret = device_register(&cdev->device); >> if (ret) >> - goto out_kfree_type; >> + goto out_put_device; >> >> /* Add 'this' new cdev to the global cdev list */ >> mutex_lock(&thermal_list_lock); >> @@ -939,10 +939,11 @@ __thermal_cooling_device_register(struct device_node *np, >> >> return cdev; >> >> +out_put_device: >> + put_device(&cdev->device); >> out_kfree_type: >> thermal_cooling_device_destroy_sysfs(cdev); > > What about this one ? This shouldn't be called in case get_max_state() fails, > right ? Right, I missed that one! It even gets called twice if dev_set_name() fails... > >> kfree(cdev->type); >> - put_device(&cdev->device); >> cdev = NULL; >> out_ida_remove: >> ida_free(&thermal_cdev_ida, id); >
On 31-12-22, 21:03, Caleb Connolly wrote: > Commit c408b3d1d9bb ("thermal: Validate new state in cur_state_store()") > causes device_put() to be called if the get_max_state() callback fails > during __thermal_cooling_device_register(). > > Fix the cleanup ordering to only call device_put() if initialization > fails after the matching device_register() call. > > Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()") > Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> > --- > drivers/thermal/thermal_core.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) Should it be like this instead ? diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c index f17ab2316dbd..18db011d4d68 100644 --- a/drivers/thermal/thermal_core.c +++ b/drivers/thermal/thermal_core.c @@ -910,17 +910,15 @@ __thermal_cooling_device_register(struct device_node *np, ret = cdev->ops->get_max_state(cdev, &cdev->max_state); if (ret) - goto out_kfree_type; + goto out_cdev_type; thermal_cooling_device_setup_sysfs(cdev); ret = dev_set_name(&cdev->device, "cooling_device%d", cdev->id); - if (ret) { - thermal_cooling_device_destroy_sysfs(cdev); - goto out_kfree_type; - } + if (ret) + goto out_cooling_dev; ret = device_register(&cdev->device); if (ret) - goto out_kfree_type; + goto out_put_device; /* Add 'this' new cdev to the global cdev list */ mutex_lock(&thermal_list_lock); @@ -939,11 +937,12 @@ __thermal_cooling_device_register(struct device_node *np, return cdev; -out_kfree_type: +out_put_device: + put_device(&cdev->device); +out_cooling_dev: thermal_cooling_device_destroy_sysfs(cdev); +out_cdev_type: kfree(cdev->type); - put_device(&cdev->device); - cdev = NULL; out_ida_remove: ida_free(&thermal_cdev_ida, id); out_kfree_cdev:
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c index f17ab2316dbd..2c6995b5dcb0 100644 --- a/drivers/thermal/thermal_core.c +++ b/drivers/thermal/thermal_core.c @@ -920,7 +920,7 @@ __thermal_cooling_device_register(struct device_node *np, } ret = device_register(&cdev->device); if (ret) - goto out_kfree_type; + goto out_put_device; /* Add 'this' new cdev to the global cdev list */ mutex_lock(&thermal_list_lock); @@ -939,10 +939,11 @@ __thermal_cooling_device_register(struct device_node *np, return cdev; +out_put_device: + put_device(&cdev->device); out_kfree_type: thermal_cooling_device_destroy_sysfs(cdev); kfree(cdev->type); - put_device(&cdev->device); cdev = NULL; out_ida_remove: ida_free(&thermal_cdev_ida, id);
Commit c408b3d1d9bb ("thermal: Validate new state in cur_state_store()") causes device_put() to be called if the get_max_state() callback fails during __thermal_cooling_device_register(). Fix the cleanup ordering to only call device_put() if initialization fails after the matching device_register() call. Fixes: c408b3d1d9bb ("thermal: Validate new state in cur_state_store()") Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> --- drivers/thermal/thermal_core.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)