| Message ID | 20220511012738.3031346-1-yangyingliang@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] i2c: core: Fix possible memleak in i2c_new_client_device() | expand |
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@huawei.com> > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > --- > 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);
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);
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@huawei.com> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> --- v2: split device_register() into device_initialize() and device_add() --- drivers/i2c/i2c-core-base.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)