linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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;
as well as URLs for NNTP newsgroup(s).