* [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; 5+ 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] 5+ 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; 5+ 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] 5+ 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; 5+ 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] 5+ messages in thread
* Re: [PATCH] i2c: core: Fix possible memleak in i2c_new_client_device()
2022-11-24 8:54 Yang Yingliang
@ 2023-09-29 9:15 ` Wolfram Sang
0 siblings, 0 replies; 5+ messages in thread
From: Wolfram Sang @ 2023-09-29 9:15 UTC (permalink / raw)
To: Yang Yingliang; +Cc: linux-i2c
[-- Attachment #1: Type: text/plain, Size: 1627 bytes --]
On Thu, Nov 24, 2022 at 04:54:48PM +0800, Yang Yingliang wrote:
> 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
>
> If device_register() returns error in i2c_new_client_device(),
> the name allocated by i2c_dev_set_name() need be freed. As
> comment of device_register() says, it should use put_device()
> to give up the reference in the error path.
>
> Moving i2c_dev_set_name() to front of device_register(), so
> it's managed by device core, then call put_device(), when the
> refcount is 0, the name will be freed in kobject_cleanup() and
> the 'client' will be freed in i2c_client_dev_release().
>
> Because the 'client' is freed by calling put_device(), so it
> don't need goto error label, release other resources before
> put_device() and return directly.
>
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
The report seems correct to me, yet the solution is copying code a tad
too much IMO. I'll send another approach I'd suggest to spark some
discussion. Thanks for the report in any case!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] i2c: core: Fix possible memleak in i2c_new_client_device()
@ 2022-11-24 8:54 Yang Yingliang
2023-09-29 9:15 ` Wolfram Sang
0 siblings, 1 reply; 5+ messages in thread
From: Yang Yingliang @ 2022-11-24 8:54 UTC (permalink / raw)
To: linux-i2c, wsa; +Cc: yangyingliang
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
If device_register() returns error in i2c_new_client_device(),
the name allocated by i2c_dev_set_name() need be freed. As
comment of device_register() says, it should use put_device()
to give up the reference in the error path.
Moving i2c_dev_set_name() to front of device_register(), so
it's managed by device core, then call put_device(), when the
refcount is 0, the name will be freed in kobject_cleanup() and
the 'client' will be freed in i2c_client_dev_release().
Because the 'client' is freed by calling put_device(), so it
don't need goto error label, release other resources before
put_device() and return directly.
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
drivers/i2c/i2c-core-base.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index b4edf10e8fd0..161bf28f2d87 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -954,7 +954,6 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf
client->dev.fwnode = info->fwnode;
device_enable_async_suspend(&client->dev);
- i2c_dev_set_name(adap, client, info);
if (info->swnode) {
status = device_add_software_node(&client->dev, info->swnode);
@@ -966,17 +965,23 @@ 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);
+ dev_err(&adap->dev,
+ "Failed to register i2c client %s at 0x%02x (%d)\n",
+ client->name, client->addr, status);
+ 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] 5+ messages in thread
end of thread, other threads:[~2023-09-29 9:16 UTC | newest]
Thread overview: 5+ 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
2022-11-24 8:54 Yang Yingliang
2023-09-29 9:15 ` Wolfram Sang
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.