[PATCH] usb: phy: isp1301: add release func to dev to avoid memory leak

[PATCH] usb: phy: isp1301: add release func to dev to avoid memory leak

[Yang Yingliang]

After calling usb_add_phy_dev(), client->dev.type will be changed
to 'usb_pyh_dev_type', the release() func is null, it cause the
following WARNING:

Device '1-001c' does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.
WARNING: CPU: 1 PID: 405 at device_release+0x1b7/0x240
Call Trace:
 kobject_put+0x1e5/0x540
 device_unregister+0x35/0xc0
 i2c_unregister_device+0x114/0x1f0

It cause 'client' leaked which is allocated in i2c_new_client_device():

unreferenced object 0xffff88800670b000 (size 2048):
  comm "xrun", pid 429, jiffies 4294946742 (age 235.248s)
  hex dump (first 32 bytes):
    00 00 1c 00 69 73 70 31 33 30 31 00 00 00 00 00  ....isp1301.....
    00 00 00 00 00 00 00 00 c0 e4 17 c1 ff ff ff ff  ................
  backtrace:
    [<00000000a4641100>] kmem_cache_alloc_trace+0x186/0x2b0
    [<00000000d9d933e7>] i2c_new_client_device+0x56/0xb40
    [<000000007255bed2>] new_device_store+0x1f4/0x410

So add release func to dev to avoid this memory leak.

Reported-by: Hulk Robot <hulkci@huawei.com>
Fixes: 790d3a5ab6e36 ("usb: phy: isp1301: give it a context structure")
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 drivers/usb/phy/phy-isp1301.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/phy/phy-isp1301.c b/drivers/usb/phy/phy-isp1301.c
index ad3d57f1c273..04f005572484 100644
--- a/drivers/usb/phy/phy-isp1301.c
+++ b/drivers/usb/phy/phy-isp1301.c
@@ -111,6 +111,7 @@ static int isp1301_probe(struct i2c_client *client,
 	phy->init = isp1301_phy_init;
 	phy->set_vbus = isp1301_phy_set_vbus;
 	phy->type = USB_PHY_TYPE_USB2;
+	client->dev.release = client->dev.type->release;
 
 	i2c_set_clientdata(client, isp);
 	usb_add_phy_dev(phy);
-- 
2.25.1

Re: [PATCH] usb: phy: isp1301: add release func to dev to avoid memory leak

[Greg KH]

On Fri, Oct 15, 2021 at 01:16:24PM +0800, Yang Yingliang wrote:
> After calling usb_add_phy_dev(), client->dev.type will be changed
> to 'usb_pyh_dev_type', the release() func is null, it cause the
> following WARNING:
> 
> Device '1-001c' does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.
> WARNING: CPU: 1 PID: 405 at device_release+0x1b7/0x240
> Call Trace:
>  kobject_put+0x1e5/0x540
>  device_unregister+0x35/0xc0
>  i2c_unregister_device+0x114/0x1f0
> 
> It cause 'client' leaked which is allocated in i2c_new_client_device():
> 
> unreferenced object 0xffff88800670b000 (size 2048):
>   comm "xrun", pid 429, jiffies 4294946742 (age 235.248s)
>   hex dump (first 32 bytes):
>     00 00 1c 00 69 73 70 31 33 30 31 00 00 00 00 00  ....isp1301.....
>     00 00 00 00 00 00 00 00 c0 e4 17 c1 ff ff ff ff  ................
>   backtrace:
>     [<00000000a4641100>] kmem_cache_alloc_trace+0x186/0x2b0
>     [<00000000d9d933e7>] i2c_new_client_device+0x56/0xb40
>     [<000000007255bed2>] new_device_store+0x1f4/0x410
> 
> So add release func to dev to avoid this memory leak.
> 
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Fixes: 790d3a5ab6e36 ("usb: phy: isp1301: give it a context structure")
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> ---
>  drivers/usb/phy/phy-isp1301.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/phy/phy-isp1301.c b/drivers/usb/phy/phy-isp1301.c
> index ad3d57f1c273..04f005572484 100644
> --- a/drivers/usb/phy/phy-isp1301.c
> +++ b/drivers/usb/phy/phy-isp1301.c
> @@ -111,6 +111,7 @@ static int isp1301_probe(struct i2c_client *client,
>  	phy->init = isp1301_phy_init;
>  	phy->set_vbus = isp1301_phy_set_vbus;
>  	phy->type = USB_PHY_TYPE_USB2;
> +	client->dev.release = client->dev.type->release;

messing with a release pointer is almost never a good idea, and a sign
that something is really wrong.

Why is the type not set properly here so as to get the correct release
callback when the device is created?  Why do you have to manually change
this now after the fact?

thanks,

greg k-h