* [PATCH] i2c: core: Fix possible memleak in i2c_new_client_device()
@ 2021-10-15 9:55 Yang Yingliang
2021-10-17 15:27 ` Wolfram Sang
0 siblings, 1 reply; 3+ messages in thread
From: Yang Yingliang @ 2021-10-15 9:55 UTC (permalink / raw)
To: linux-kernel, linux-i2c; +Cc: wsa
I got memory leak as follows when doing fault injection test:
unreferenced object 0xffff888014aec078 (size 8):
comm "xrun", pid 356, jiffies 4294910619 (age 16.332s)
hex dump (first 8 bytes):
31 2d 30 30 31 63 00 00 1-001c..
backtrace:
[<00000000eb56c0a9>] __kmalloc_track_caller+0x1a6/0x300
[<000000000b220ea3>] kvasprintf+0xad/0x140
[<00000000b83203e5>] kvasprintf_const+0x62/0x190
[<000000002a5eab37>] kobject_set_name_vargs+0x56/0x140
[<00000000300ac279>] dev_set_name+0xb0/0xe0
[<00000000b66ebd6f>] i2c_new_client_device+0x7e4/0x9a0
In error path after calling i2c_dev_set_name(), the put_device()
should be used to give up the device reference, then the name
allocated in dev_set_name() will be freed in kobject_cleanup().
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
drivers/i2c/i2c-core-base.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 54964fbe3f03..190d4fd5e594 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1047,8 +1047,6 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf
client->dev.of_node = of_node_get(info->of_node);
client->dev.fwnode = info->fwnode;
- i2c_dev_set_name(adap, client, info);
-
if (info->swnode) {
status = device_add_software_node(&client->dev, info->swnode);
if (status) {
@@ -1059,17 +1057,20 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf
}
}
+ i2c_dev_set_name(adap, client, info);
status = device_register(&client->dev);
- if (status)
- goto out_remove_swnode;
+ if (status) {
+ device_remove_software_node(&client->dev);
+ of_node_put(info->of_node);
+ put_device(&client->dev);
+ return ERR_PTR(status);
+ }
dev_dbg(&adap->dev, "client [%s] registered with bus id %s\n",
client->name, dev_name(&client->dev));
return client;
-out_remove_swnode:
- device_remove_software_node(&client->dev);
out_err_put_of_node:
of_node_put(info->of_node);
out_err:
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] i2c: core: Fix possible memleak in i2c_new_client_device()
2021-10-15 9:55 [PATCH] i2c: core: Fix possible memleak in i2c_new_client_device() Yang Yingliang
@ 2021-10-17 15:27 ` Wolfram Sang
2021-10-18 3:03 ` Yang Yingliang
0 siblings, 1 reply; 3+ messages in thread
From: Wolfram Sang @ 2021-10-17 15:27 UTC (permalink / raw)
To: Yang Yingliang; +Cc: linux-kernel, linux-i2c
[-- Attachment #1: Type: text/plain, Size: 1970 bytes --]
> In error path after calling i2c_dev_set_name(), the put_device()
> should be used to give up the device reference, then the name
> allocated in dev_set_name() will be freed in kobject_cleanup().
I don't see it. dev_set_name does not call device_get, so why should we
call device_put on failure? No other user of dev_set_name seems to do
this. So, if this is an imbalance, where does the unmatched get_device
really come from?
>
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> ---
> drivers/i2c/i2c-core-base.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 54964fbe3f03..190d4fd5e594 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1047,8 +1047,6 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf
> client->dev.of_node = of_node_get(info->of_node);
> client->dev.fwnode = info->fwnode;
>
> - i2c_dev_set_name(adap, client, info);
> -
> if (info->swnode) {
> status = device_add_software_node(&client->dev, info->swnode);
> if (status) {
> @@ -1059,17 +1057,20 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf
> }
> }
>
> + i2c_dev_set_name(adap, client, info);
> status = device_register(&client->dev);
> - if (status)
> - goto out_remove_swnode;
> + if (status) {
> + device_remove_software_node(&client->dev);
> + of_node_put(info->of_node);
> + put_device(&client->dev);
> + return ERR_PTR(status);
> + }
>
> dev_dbg(&adap->dev, "client [%s] registered with bus id %s\n",
> client->name, dev_name(&client->dev));
>
> return client;
>
> -out_remove_swnode:
> - device_remove_software_node(&client->dev);
> out_err_put_of_node:
> of_node_put(info->of_node);
> out_err:
> --
> 2.25.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] i2c: core: Fix possible memleak in i2c_new_client_device()
2021-10-17 15:27 ` Wolfram Sang
@ 2021-10-18 3:03 ` Yang Yingliang
0 siblings, 0 replies; 3+ messages in thread
From: Yang Yingliang @ 2021-10-18 3:03 UTC (permalink / raw)
To: Wolfram Sang, linux-kernel, linux-i2c
Hi,
On 2021/10/17 23:27, Wolfram Sang wrote:
>> In error path after calling i2c_dev_set_name(), the put_device()
>> should be used to give up the device reference, then the name
>> allocated in dev_set_name() will be freed in kobject_cleanup().
> I don't see it. dev_set_name does not call device_get, so why should we
> call device_put on failure? No other user of dev_set_name seems to do
> this. So, if this is an imbalance, where does the unmatched get_device
> really come from?
The reference is initialized in device_initialize() called in
device_register(), if device_register()
fails, the 'kobj->name' is leaked.
Thanks,
Yang
>
>> Reported-by: Hulk Robot <hulkci@huawei.com>
>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
>> ---
>> drivers/i2c/i2c-core-base.c | 13 +++++++------
>> 1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
>> index 54964fbe3f03..190d4fd5e594 100644
>> --- a/drivers/i2c/i2c-core-base.c
>> +++ b/drivers/i2c/i2c-core-base.c
>> @@ -1047,8 +1047,6 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf
>> client->dev.of_node = of_node_get(info->of_node);
>> client->dev.fwnode = info->fwnode;
>>
>> - i2c_dev_set_name(adap, client, info);
>> -
>> if (info->swnode) {
>> status = device_add_software_node(&client->dev, info->swnode);
>> if (status) {
>> @@ -1059,17 +1057,20 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf
>> }
>> }
>>
>> + i2c_dev_set_name(adap, client, info);
>> status = device_register(&client->dev);
>> - if (status)
>> - goto out_remove_swnode;
>> + if (status) {
>> + device_remove_software_node(&client->dev);
>> + of_node_put(info->of_node);
>> + put_device(&client->dev);
>> + return ERR_PTR(status);
>> + }
>>
>> dev_dbg(&adap->dev, "client [%s] registered with bus id %s\n",
>> client->name, dev_name(&client->dev));
>>
>> return client;
>>
>> -out_remove_swnode:
>> - device_remove_software_node(&client->dev);
>> out_err_put_of_node:
>> of_node_put(info->of_node);
>> out_err:
>> --
>> 2.25.1
>>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-10-18 3:03 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-10-15 9:55 [PATCH] i2c: core: Fix possible memleak in i2c_new_client_device() Yang Yingliang
2021-10-17 15:27 ` Wolfram Sang
2021-10-18 3:03 ` Yang Yingliang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox