diff mbox series

PNP: fix name memory leak in pnp_register_protocol()

Message ID 20221122125425.1107474-1-yangyingliang@huawei.com
State New
Headers show
Series PNP: fix name memory leak in pnp_register_protocol() | expand

Commit Message

Yang Yingliang Nov. 22, 2022, 12:54 p.m. UTC
After commit 1fa5ae857bb1 ("driver core: get rid of struct device's
bus_id string array"), the name of device is allocated dynamically,
it need be freed in error path.

Current all protocols used in pnp_register_protocol() is static, they
don't have a release function(), so just call kfree_const() to free
the name.

Fixes: 1fa5ae857bb1 ("driver core: get rid of struct device's bus_id string array")
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 drivers/pnp/core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Rafael J. Wysocki Nov. 23, 2022, 6:50 p.m. UTC | #1
On Tue, Nov 22, 2022 at 1:57 PM Yang Yingliang <yangyingliang@huawei.com> wrote:
>
> After commit 1fa5ae857bb1 ("driver core: get rid of struct device's
> bus_id string array"), the name of device is allocated dynamically,
> it need be freed in error path.
>
> Current all protocols used in pnp_register_protocol() is static, they
> don't have a release function(), so just call kfree_const() to free
> the name.
>
> Fixes: 1fa5ae857bb1 ("driver core: get rid of struct device's bus_id string array")
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> ---
>  drivers/pnp/core.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pnp/core.c b/drivers/pnp/core.c
> index 6a60c5d83383..390e449c17ef 100644
> --- a/drivers/pnp/core.c
> +++ b/drivers/pnp/core.c
> @@ -72,8 +72,10 @@ int pnp_register_protocol(struct pnp_protocol *protocol)
>         mutex_unlock(&pnp_lock);
>
>         ret = device_register(&protocol->dev);
> -       if (ret)
> +       if (ret) {
>                 pnp_remove_protocol(protocol);
> +               kfree_const(protocol->dev.kobj.name);

Having to refer to dev.kobj here doesn't seem right.

Shouldn't this just use dev->init_name and allow the core to sort out
the error case?

> +       }
>
>         return ret;
>  }
> --
Yang Yingliang Nov. 24, 2022, 2:52 a.m. UTC | #2
On 2022/11/24 2:50, Rafael J. Wysocki wrote:
> On Tue, Nov 22, 2022 at 1:57 PM Yang Yingliang <yangyingliang@huawei.com> wrote:
>> After commit 1fa5ae857bb1 ("driver core: get rid of struct device's
>> bus_id string array"), the name of device is allocated dynamically,
>> it need be freed in error path.
>>
>> Current all protocols used in pnp_register_protocol() is static, they
>> don't have a release function(), so just call kfree_const() to free
>> the name.
>>
>> Fixes: 1fa5ae857bb1 ("driver core: get rid of struct device's bus_id string array")
>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
>> ---
>>   drivers/pnp/core.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pnp/core.c b/drivers/pnp/core.c
>> index 6a60c5d83383..390e449c17ef 100644
>> --- a/drivers/pnp/core.c
>> +++ b/drivers/pnp/core.c
>> @@ -72,8 +72,10 @@ int pnp_register_protocol(struct pnp_protocol *protocol)
>>          mutex_unlock(&pnp_lock);
>>
>>          ret = device_register(&protocol->dev);
>> -       if (ret)
>> +       if (ret) {
>>                  pnp_remove_protocol(protocol);
>> +               kfree_const(protocol->dev.kobj.name);
> Having to refer to dev.kobj here doesn't seem right.
After calling dev_set_name(), the 'dev.kobj.name' pointer a allocated name.
>
> Shouldn't this just use dev->init_name and allow the core to sort out
> the error case?
dev_set_name(&protocol->dev, "pnp%d", nodenum) is called outside 
device_register(),
the name should be freed by calling put_device() (kobject_cleanup()).

Comment of device_register says:
   NOTE: _Never_ directly free @dev after calling this function, even
   if it returned an error! Always use put_device() to give up the
   reference initialized in this function instead.

Current all caller's device(pnpacpi_protocol.dev and 
pnpbios_protocol.dev) is const, they
don't need be freed, and don't have a release() function. So I just call 
kfree(name) instead
of calling put_device() here.

Thanks,
Yang
>
>> +       }
>>
>>          return ret;
>>   }
>> --
> .
diff mbox series

Patch

diff --git a/drivers/pnp/core.c b/drivers/pnp/core.c
index 6a60c5d83383..390e449c17ef 100644
--- a/drivers/pnp/core.c
+++ b/drivers/pnp/core.c
@@ -72,8 +72,10 @@  int pnp_register_protocol(struct pnp_protocol *protocol)
 	mutex_unlock(&pnp_lock);
 
 	ret = device_register(&protocol->dev);
-	if (ret)
+	if (ret) {
 		pnp_remove_protocol(protocol);
+		kfree_const(protocol->dev.kobj.name);
+	}
 
 	return ret;
 }