Re: [PATCH v2] i2c: core: Fix possible memleak in i2c_new_client_device()

[Yang Yingliang <yangyingliang@xxxxxxxxxx>]

Hi, Sang

According the document of device_register(), it shouldn't 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.
It will cleanup the name of device. Is this patch good to you ?

Thanks,
Yang
On 2022/5/11 9:27, 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
>
> In error path after calling dev_set_name() which called by
> 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().
> In this patch, I splited device_register() into device_initialize()
> and device_add() to make the code more clear.
>
> Reported-by: Hulk Robot <hulkci@xxxxxxxxxx>
> Signed-off-by: Yang Yingliang <yangyingliang@xxxxxxxxxx>
> ---
> v2:
>    split device_register() into device_initialize() and device_add()
> ---
>   drivers/i2c/i2c-core-base.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index d43db2c3876e..e7dded8b037b 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -928,6 +928,11 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf
>   	client->flags = info->flags;
>   	client->addr = info->addr;
>   
> +	client->dev.parent = &client->adapter->dev;
> +	client->dev.bus = &i2c_bus_type;
> +	client->dev.type = &i2c_client_type;
> +	device_initialize(&client->dev);
> +
>   	client->init_irq = info->irq;
>   	if (!client->init_irq)
>   		client->init_irq = i2c_dev_irq_from_resources(info->resources,
> @@ -947,9 +952,6 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf
>   	if (status)
>   		goto out_err;
>   
> -	client->dev.parent = &client->adapter->dev;
> -	client->dev.bus = &i2c_bus_type;
> -	client->dev.type = &i2c_client_type;
>   	client->dev.of_node = of_node_get(info->of_node);
>   	client->dev.fwnode = info->fwnode;
>   
> @@ -966,7 +968,7 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf
>   		}
>   	}
>   
> -	status = device_register(&client->dev);
> +	status = device_add(&client->dev);
>   	if (status)
>   		goto out_remove_swnode;
>   
> @@ -984,7 +986,7 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf
>   		"Failed to register i2c client %s at 0x%02x (%d)\n",
>   		client->name, client->addr, status);
>   out_err_silent:
> -	kfree(client);
> +	put_device(&client->dev);
>   	return ERR_PTR(status);
>   }
>   EXPORT_SYMBOL_GPL(i2c_new_client_device);