* [PATCH] powercap: fix possible name leak while device_register() fails
@ 2022-11-12 9:40 Yang Yingliang
2022-11-23 19:00 ` Rafael J. Wysocki
0 siblings, 1 reply; 6+ messages in thread
From: Yang Yingliang @ 2022-11-12 9:40 UTC (permalink / raw)
To: linux-pm; +Cc: rafael, yangyingliang
If device_register() returns error, the name allocated by
dev_set_name() need be freed. In technical, we should call
put_device() to give up the reference and free the name in
driver core, but in some cases the device is not intizalized,
put_device() can not be called, so don't complicate the code,
just call kfree_const() to free name in the error path.
Fixes: 75d2364ea0ca ("PowerCap: Add class driver")
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
drivers/powercap/powercap_sys.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/powercap/powercap_sys.c b/drivers/powercap/powercap_sys.c
index f0654a932b37..11e742dc83b9 100644
--- a/drivers/powercap/powercap_sys.c
+++ b/drivers/powercap/powercap_sys.c
@@ -572,6 +572,7 @@ struct powercap_zone *powercap_register_zone(
err_name_alloc:
idr_remove(power_zone->parent_idr, power_zone->id);
err_idr_alloc:
+ kfree_const(dev_name(&power_zone->dev));
if (power_zone->allocated)
kfree(power_zone);
mutex_unlock(&control_type->lock);
@@ -622,6 +623,7 @@ struct powercap_control_type *powercap_register_control_type(
dev_set_name(&control_type->dev, "%s", name);
result = device_register(&control_type->dev);
if (result) {
+ kfree_const(dev_name(&control_type->dev));
if (control_type->allocated)
kfree(control_type);
return ERR_PTR(result);
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] powercap: fix possible name leak while device_register() fails 2022-11-12 9:40 [PATCH] powercap: fix possible name leak while device_register() fails Yang Yingliang @ 2022-11-23 19:00 ` Rafael J. Wysocki 2022-11-23 19:25 ` Greg Kroah-Hartman 0 siblings, 1 reply; 6+ messages in thread From: Rafael J. Wysocki @ 2022-11-23 19:00 UTC (permalink / raw) To: Yang Yingliang Cc: linux-pm, rafael, Greg Kroah-Hartman, Linux Kernel Mailing List On Sat, Nov 12, 2022 at 10:42 AM Yang Yingliang <yangyingliang@huawei.com> wrote: > > If device_register() returns error, the name allocated by > dev_set_name() need be freed. In technical, we should call > put_device() to give up the reference and free the name in > driver core, but in some cases the device is not intizalized, > put_device() can not be called, so don't complicate the code, > just call kfree_const() to free name in the error path. > > Fixes: 75d2364ea0ca ("PowerCap: Add class driver") > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > --- > drivers/powercap/powercap_sys.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/powercap/powercap_sys.c b/drivers/powercap/powercap_sys.c > index f0654a932b37..11e742dc83b9 100644 > --- a/drivers/powercap/powercap_sys.c > +++ b/drivers/powercap/powercap_sys.c > @@ -572,6 +572,7 @@ struct powercap_zone *powercap_register_zone( > err_name_alloc: > idr_remove(power_zone->parent_idr, power_zone->id); > err_idr_alloc: > + kfree_const(dev_name(&power_zone->dev)); > if (power_zone->allocated) > kfree(power_zone); > mutex_unlock(&control_type->lock); > @@ -622,6 +623,7 @@ struct powercap_control_type *powercap_register_control_type( > dev_set_name(&control_type->dev, "%s", name); > result = device_register(&control_type->dev); > if (result) { > + kfree_const(dev_name(&control_type->dev)); Why is it necessary to free a device name explicitly after a failing device_register()? If it is really necessary, then there is a problem in device_register() itself AFAICS, because it uses dev_set_name() at least in the dev->init_name present case. > if (control_type->allocated) > kfree(control_type); > return ERR_PTR(result); > -- ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powercap: fix possible name leak while device_register() fails 2022-11-23 19:00 ` Rafael J. Wysocki @ 2022-11-23 19:25 ` Greg Kroah-Hartman 2022-11-24 2:16 ` Yang Yingliang 0 siblings, 1 reply; 6+ messages in thread From: Greg Kroah-Hartman @ 2022-11-23 19:25 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: Yang Yingliang, linux-pm, Linux Kernel Mailing List On Wed, Nov 23, 2022 at 08:00:14PM +0100, Rafael J. Wysocki wrote: > On Sat, Nov 12, 2022 at 10:42 AM Yang Yingliang > <yangyingliang@huawei.com> wrote: > > > > If device_register() returns error, the name allocated by > > dev_set_name() need be freed. In technical, we should call > > put_device() to give up the reference and free the name in > > driver core, but in some cases the device is not intizalized, > > put_device() can not be called, so don't complicate the code, > > just call kfree_const() to free name in the error path. > > > > Fixes: 75d2364ea0ca ("PowerCap: Add class driver") > > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > > --- > > drivers/powercap/powercap_sys.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/powercap/powercap_sys.c b/drivers/powercap/powercap_sys.c > > index f0654a932b37..11e742dc83b9 100644 > > --- a/drivers/powercap/powercap_sys.c > > +++ b/drivers/powercap/powercap_sys.c > > @@ -572,6 +572,7 @@ struct powercap_zone *powercap_register_zone( > > err_name_alloc: > > idr_remove(power_zone->parent_idr, power_zone->id); > > err_idr_alloc: > > + kfree_const(dev_name(&power_zone->dev)); > > if (power_zone->allocated) > > kfree(power_zone); > > mutex_unlock(&control_type->lock); > > @@ -622,6 +623,7 @@ struct powercap_control_type *powercap_register_control_type( > > dev_set_name(&control_type->dev, "%s", name); > > result = device_register(&control_type->dev); > > if (result) { > > + kfree_const(dev_name(&control_type->dev)); > > Why is it necessary to free a device name explicitly after a failing > device_register()? > > If it is really necessary, then there is a problem in > device_register() itself AFAICS, because it uses dev_set_name() at > least in the dev->init_name present case. I think we already fixed this in the driver core, so these types of patches should not be applied. Yang, can you make sure you respond to all of them and say "this is not needed anymore!" and if any got merged, send reverts for them? thanks, greg k-h ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powercap: fix possible name leak while device_register() fails 2022-11-23 19:25 ` Greg Kroah-Hartman @ 2022-11-24 2:16 ` Yang Yingliang 2022-11-25 18:45 ` Rafael J. Wysocki 0 siblings, 1 reply; 6+ messages in thread From: Yang Yingliang @ 2022-11-24 2:16 UTC (permalink / raw) To: Greg Kroah-Hartman, Rafael J. Wysocki; +Cc: linux-pm, Linux Kernel Mailing List On 2022/11/24 3:25, Greg Kroah-Hartman wrote: > On Wed, Nov 23, 2022 at 08:00:14PM +0100, Rafael J. Wysocki wrote: >> On Sat, Nov 12, 2022 at 10:42 AM Yang Yingliang >> <yangyingliang@huawei.com> wrote: >>> If device_register() returns error, the name allocated by Sorry, I didn't describe clearly here, it's not only after device_register() failure, but also in the error path before register, the name is not freed, see description below. >>> dev_set_name() need be freed. In technical, we should call >>> put_device() to give up the reference and free the name in >>> driver core, but in some cases the device is not intizalized, >>> put_device() can not be called, so don't complicate the code, >>> just call kfree_const() to free name in the error path. >>> >>> Fixes: 75d2364ea0ca ("PowerCap: Add class driver") >>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> >>> --- >>> drivers/powercap/powercap_sys.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/drivers/powercap/powercap_sys.c b/drivers/powercap/powercap_sys.c >>> index f0654a932b37..11e742dc83b9 100644 >>> --- a/drivers/powercap/powercap_sys.c >>> +++ b/drivers/powercap/powercap_sys.c >>> @@ -572,6 +572,7 @@ struct powercap_zone *powercap_register_zone( >>> err_name_alloc: >>> idr_remove(power_zone->parent_idr, power_zone->id); >>> err_idr_alloc: >>> + kfree_const(dev_name(&power_zone->dev)); >>> if (power_zone->allocated) >>> kfree(power_zone); >>> mutex_unlock(&control_type->lock); >>> @@ -622,6 +623,7 @@ struct powercap_control_type *powercap_register_control_type( >>> dev_set_name(&control_type->dev, "%s", name); >>> result = device_register(&control_type->dev); >>> if (result) { >>> + kfree_const(dev_name(&control_type->dev)); >> Why is it necessary to free a device name explicitly after a failing >> device_register()? powercap_register_zone() { ... dev_set_name() // allocate name ... if (!power_zone->constraints) goto err_const_alloc; //the name is leaked in this path ... if (!power_zone->zone_dev_attrs) goto err_attr_alloc; //the name is leaked in this path ... if (result) goto err_dev_ret; //the name is leaked in this path result = device_register(&power_zone->dev); if (result) goto err_dev_ret;//put_device() is not called, the name is leaked in this path ... err_dev_ret: kfree(power_zone->zone_dev_attrs); err_attr_alloc: kfree(power_zone->constraints); err_const_alloc: kfree(power_zone->name); err_name_alloc: idr_remove(power_zone->parent_idr, power_zone->id); err_idr_alloc: if (power_zone->allocated) kfree(power_zone); } >> >> If it is really necessary, then there is a problem in >> device_register() itself AFAICS, because it uses dev_set_name() at >> least in the dev->init_name present case. When the dev_set_name() called in device_register(), if register fails, the name is freed in its error path. But in this case, dev_set_name() is called outside the register, it needs call put_device() to free the name. > I think we already fixed this in the driver core, so these types of > patches should not be applied. driver core free the name by calling put_device(), but in these two functions, put_device() is not called. Thanks, Yang > > Yang, can you make sure you respond to all of them and say "this is not > needed anymore!" and if any got merged, send reverts for them? > > thanks, > > greg k-h > . ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powercap: fix possible name leak while device_register() fails 2022-11-24 2:16 ` Yang Yingliang @ 2022-11-25 18:45 ` Rafael J. Wysocki 2022-11-26 2:05 ` Yang Yingliang 0 siblings, 1 reply; 6+ messages in thread From: Rafael J. Wysocki @ 2022-11-25 18:45 UTC (permalink / raw) To: Yang Yingliang Cc: Greg Kroah-Hartman, Rafael J. Wysocki, linux-pm, Linux Kernel Mailing List On Thu, Nov 24, 2022 at 3:16 AM Yang Yingliang <yangyingliang@huawei.com> wrote: > > > On 2022/11/24 3:25, Greg Kroah-Hartman wrote: > > On Wed, Nov 23, 2022 at 08:00:14PM +0100, Rafael J. Wysocki wrote: > >> On Sat, Nov 12, 2022 at 10:42 AM Yang Yingliang > >> <yangyingliang@huawei.com> wrote: > >>> If device_register() returns error, the name allocated by > Sorry, > I didn't describe clearly here, it's not only after device_register() > failure, but also in the error path before register, the name is not > freed, see description below. So you would need to update the changelog at least. But see below. > >>> dev_set_name() need be freed. In technical, we should call > >>> put_device() to give up the reference and free the name in > >>> driver core, but in some cases the device is not intizalized, > >>> put_device() can not be called, so don't complicate the code, > >>> just call kfree_const() to free name in the error path. > >>> > >>> Fixes: 75d2364ea0ca ("PowerCap: Add class driver") > >>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > >>> --- > >>> drivers/powercap/powercap_sys.c | 2 ++ > >>> 1 file changed, 2 insertions(+) > >>> > >>> diff --git a/drivers/powercap/powercap_sys.c b/drivers/powercap/powercap_sys.c > >>> index f0654a932b37..11e742dc83b9 100644 > >>> --- a/drivers/powercap/powercap_sys.c > >>> +++ b/drivers/powercap/powercap_sys.c > >>> @@ -572,6 +572,7 @@ struct powercap_zone *powercap_register_zone( > >>> err_name_alloc: > >>> idr_remove(power_zone->parent_idr, power_zone->id); > >>> err_idr_alloc: > >>> + kfree_const(dev_name(&power_zone->dev)); > >>> if (power_zone->allocated) > >>> kfree(power_zone); > >>> mutex_unlock(&control_type->lock); > >>> @@ -622,6 +623,7 @@ struct powercap_control_type *powercap_register_control_type( > >>> dev_set_name(&control_type->dev, "%s", name); > >>> result = device_register(&control_type->dev); > >>> if (result) { > >>> + kfree_const(dev_name(&control_type->dev)); > >> Why is it necessary to free a device name explicitly after a failing > >> device_register()? > powercap_register_zone() > { > ... > dev_set_name() // allocate name > ... > if (!power_zone->constraints) > goto err_const_alloc; //the name is leaked in this path > ... > if (!power_zone->zone_dev_attrs) > goto err_attr_alloc; //the name is leaked in this path > ... > if (result) > goto err_dev_ret; //the name is leaked in this path > > result = device_register(&power_zone->dev); > if (result) > goto err_dev_ret;//put_device() is not called, the name is > leaked in this path > ... > err_dev_ret: > kfree(power_zone->zone_dev_attrs); > err_attr_alloc: > kfree(power_zone->constraints); > err_const_alloc: > kfree(power_zone->name); > err_name_alloc: > idr_remove(power_zone->parent_idr, power_zone->id); > err_idr_alloc: > if (power_zone->allocated) > kfree(power_zone); > } So can't the dev_set_name() be reordered closer to device_register(), so it is not necessary to worry about freeing the name? > >> > >> If it is really necessary, then there is a problem in > >> device_register() itself AFAICS, because it uses dev_set_name() at > >> least in the dev->init_name present case. > When the dev_set_name() called in device_register(), if register fails, the > name is freed in its error path. But in this case, dev_set_name() is called > outside the register, it needs call put_device() to free the name. In any case, device_register() needs to take care of it anyway, because it uses dev_set_name() itself in the dev->init_name case, doesn't it? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powercap: fix possible name leak while device_register() fails 2022-11-25 18:45 ` Rafael J. Wysocki @ 2022-11-26 2:05 ` Yang Yingliang 0 siblings, 0 replies; 6+ messages in thread From: Yang Yingliang @ 2022-11-26 2:05 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: Greg Kroah-Hartman, linux-pm, Linux Kernel Mailing List On 2022/11/26 2:45, Rafael J. Wysocki wrote: > On Thu, Nov 24, 2022 at 3:16 AM Yang Yingliang <yangyingliang@huawei.com> wrote: >> >> On 2022/11/24 3:25, Greg Kroah-Hartman wrote: >>> On Wed, Nov 23, 2022 at 08:00:14PM +0100, Rafael J. Wysocki wrote: >>>> On Sat, Nov 12, 2022 at 10:42 AM Yang Yingliang >>>> <yangyingliang@huawei.com> wrote: >>>>> If device_register() returns error, the name allocated by >> Sorry, >> I didn't describe clearly here, it's not only after device_register() >> failure, but also in the error path before register, the name is not >> freed, see description below. > So you would need to update the changelog at least. But see below. > >>>>> dev_set_name() need be freed. In technical, we should call >>>>> put_device() to give up the reference and free the name in >>>>> driver core, but in some cases the device is not intizalized, >>>>> put_device() can not be called, so don't complicate the code, >>>>> just call kfree_const() to free name in the error path. >>>>> >>>>> Fixes: 75d2364ea0ca ("PowerCap: Add class driver") >>>>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> >>>>> --- >>>>> drivers/powercap/powercap_sys.c | 2 ++ >>>>> 1 file changed, 2 insertions(+) >>>>> >>>>> diff --git a/drivers/powercap/powercap_sys.c b/drivers/powercap/powercap_sys.c >>>>> index f0654a932b37..11e742dc83b9 100644 >>>>> --- a/drivers/powercap/powercap_sys.c >>>>> +++ b/drivers/powercap/powercap_sys.c >>>>> @@ -572,6 +572,7 @@ struct powercap_zone *powercap_register_zone( >>>>> err_name_alloc: >>>>> idr_remove(power_zone->parent_idr, power_zone->id); >>>>> err_idr_alloc: >>>>> + kfree_const(dev_name(&power_zone->dev)); >>>>> if (power_zone->allocated) >>>>> kfree(power_zone); >>>>> mutex_unlock(&control_type->lock); >>>>> @@ -622,6 +623,7 @@ struct powercap_control_type *powercap_register_control_type( >>>>> dev_set_name(&control_type->dev, "%s", name); >>>>> result = device_register(&control_type->dev); >>>>> if (result) { >>>>> + kfree_const(dev_name(&control_type->dev)); >>>> Why is it necessary to free a device name explicitly after a failing >>>> device_register()? >> powercap_register_zone() >> { >> ... >> dev_set_name() // allocate name >> ... >> if (!power_zone->constraints) >> goto err_const_alloc; //the name is leaked in this path >> ... >> if (!power_zone->zone_dev_attrs) >> goto err_attr_alloc; //the name is leaked in this path >> ... >> if (result) >> goto err_dev_ret; //the name is leaked in this path >> >> result = device_register(&power_zone->dev); >> if (result) >> goto err_dev_ret;//put_device() is not called, the name is >> leaked in this path >> ... >> err_dev_ret: >> kfree(power_zone->zone_dev_attrs); >> err_attr_alloc: >> kfree(power_zone->constraints); >> err_const_alloc: >> kfree(power_zone->name); >> err_name_alloc: >> idr_remove(power_zone->parent_idr, power_zone->id); >> err_idr_alloc: >> if (power_zone->allocated) >> kfree(power_zone); >> } > So can't the dev_set_name() be reordered closer to device_register(), > so it is not necessary to worry about freeing the name? Just move dev_set_name() closer to device_register() is not enough to free the name, it should call put_device() after device_register() failure. I will try this. > >>>> If it is really necessary, then there is a problem in >>>> device_register() itself AFAICS, because it uses dev_set_name() at >>>> least in the dev->init_name present case. >> When the dev_set_name() called in device_register(), if register fails, the >> name is freed in its error path. But in this case, dev_set_name() is called >> outside the register, it needs call put_device() to free the name. > In any case, device_register() needs to take care of it anyway, > because it uses dev_set_name() itself in the dev->init_name case, > doesn't it? Yes, it's right. Thanks, Yang > > . ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-11-26 2:05 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-11-12 9:40 [PATCH] powercap: fix possible name leak while device_register() fails Yang Yingliang 2022-11-23 19:00 ` Rafael J. Wysocki 2022-11-23 19:25 ` Greg Kroah-Hartman 2022-11-24 2:16 ` Yang Yingliang 2022-11-25 18:45 ` Rafael J. Wysocki 2022-11-26 2:05 ` Yang Yingliang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.