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
> <[email protected]> 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 <[email protected]>
> > ---
> > 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
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
>> <[email protected]> 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 <[email protected]>
>>> ---
>>> 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
> .
On Thu, Nov 24, 2022 at 3:16 AM Yang Yingliang <[email protected]> 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
> >> <[email protected]> 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 <[email protected]>
> >>> ---
> >>> 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?
On 2022/11/26 2:45, Rafael J. Wysocki wrote:
> On Thu, Nov 24, 2022 at 3:16 AM Yang Yingliang <[email protected]> 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
>>>> <[email protected]> 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 <[email protected]>
>>>>> ---
>>>>> 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
>
> .