[PATCH] gpiolib: fix memory leak in gpiochip_setup_dev

[PATCH] gpiolib: fix memory leak in gpiochip_setup_dev

[Zeng Heng]

gcdev_register & gcdev_unregister call device_add & device_del to
request/release source. But in device_add, the dev->p allocated by
device_private_init is not released by device_del.

So when calling gcdev_unregister to release gdev, it needs put_device
to release dev in the following.

Otherwise, kmemleak would report memory leak such as below:

unreferenced object 0xffff88810b406400 (size 512):
  comm "python3", pid 1682, jiffies 4295346908 (age 24.090s)
  hex dump (first 32 bytes):
    00 00 00 00 ad 4e ad de ff ff ff ff 00 00 00 00  .....N..........
    ff ff ff ff ff ff ff ff a0 5e 23 90 ff ff ff ff  .........^#.....
  backtrace:
    [<00000000a58ee5fe>] kmalloc_trace+0x22/0x110
    [<0000000045fe2058>] device_add+0xb34/0x1130
    [<00000000d778b45f>] cdev_device_add+0x83/0xe0
    [<0000000089f948ed>] gpiolib_cdev_register+0x73/0xa0
    [<00000000a3a8a316>] gpiochip_setup_dev+0x1c/0x70
    [<00000000787227b4>] gpiochip_add_data_with_key+0x10f6/0x1bf0
    [<000000009ac5742c>] devm_gpiochip_add_data_with_key+0x2e/0x80
    [<00000000bf2b23d9>] xra1403_probe+0x192/0x1b0 [gpio_xra1403]
    [<000000005b5ef2d4>] spi_probe+0xe1/0x140
    [<000000002b26f6f1>] really_probe+0x17c/0x3f0
    [<00000000dd2dad9c>] __driver_probe_device+0xe3/0x170
    [<000000005ca60d2a>] device_driver_attach+0x34/0x80
    [<00000000e9db90db>] bind_store+0x10b/0x1a0
    [<00000000e2650f8a>] drv_attr_store+0x49/0x70
    [<0000000080a80b2b>] sysfs_kf_write+0x8c/0xb0
    [<00000000a28b45b9>] kernfs_fop_write_iter+0x216/0x2e0

unreferenced object 0xffff888100de9800 (size 512):
  comm "python3", pid 264, jiffies 4294737615 (age 33.514s)
  hex dump (first 32 bytes):
    00 00 00 00 ad 4e ad de ff ff ff ff 00 00 00 00  .....N..........
    ff ff ff ff ff ff ff ff a0 5e 63 a1 ff ff ff ff  .........^c.....
  backtrace:
    [<00000000bcc571d0>] kmalloc_trace+0x22/0x110
    [<00000000eeb06124>] device_add+0xb34/0x1130
    [<000000007e5cd2fd>] cdev_device_add+0x83/0xe0
    [<000000008f6bcd3a>] gpiolib_cdev_register+0x73/0xa0
    [<0000000012c93b24>] gpiochip_setup_dev+0x1c/0x70
    [<00000000a24b646a>] gpiochip_add_data_with_key+0x10f6/0x1bf0
    [<000000000c225212>] tpic2810_probe+0x16e/0x196 [gpio_tpic2810]
    [<00000000b52d04ff>] i2c_device_probe+0x651/0x680
    [<0000000058d3ff6b>] really_probe+0x17c/0x3f0
    [<00000000586f43d3>] __driver_probe_device+0xe3/0x170
    [<000000003f428602>] device_driver_attach+0x34/0x80
    [<0000000040e91a1b>] bind_store+0x10b/0x1a0
    [<00000000c1d990b9>] drv_attr_store+0x49/0x70
    [<00000000a23bfc22>] sysfs_kf_write+0x8c/0xb0
    [<00000000064e6572>] kernfs_fop_write_iter+0x216/0x2e0
    [<00000000026ce093>] vfs_write+0x658/0x810

Because at the point of gpiochip_setup_dev here, where dev.release
does not set yet, calling put_device would cause the warning of
no release function and double-free in the following fault handler
route (when kfree dev_name). So directly calling kfree to release
dev->p here in case of memory leak.

Fixes: 1f5eb8b17f02 ("gpiolib: fix sysfs when cdev is not selected")
Signed-off-by: Zeng Heng <zengheng4@huawei.com>
---
 drivers/gpio/gpiolib.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 4756ea08894f..fa659af86d07 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -539,6 +539,7 @@ static int gpiochip_setup_dev(struct gpio_device *gdev)
 
 err_remove_device:
 	gcdev_unregister(gdev);
+	kfree(gdev->dev.p);
 	return ret;
 }
 
-- 
2.25.1

Re: [PATCH] gpiolib: fix memory leak in gpiochip_setup_dev

[Kent Gibson]

On Tue, Nov 08, 2022 at 07:53:24PM +0800, Zeng Heng wrote:
> gcdev_register & gcdev_unregister call device_add & device_del to
> request/release source. But in device_add, the dev->p allocated by
> device_private_init is not released by device_del.
> 
> So when calling gcdev_unregister to release gdev, it needs put_device
> to release dev in the following.
> 
> Otherwise, kmemleak would report memory leak such as below:
> 
> unreferenced object 0xffff88810b406400 (size 512):
>   comm "python3", pid 1682, jiffies 4295346908 (age 24.090s)
>   hex dump (first 32 bytes):
>     00 00 00 00 ad 4e ad de ff ff ff ff 00 00 00 00  .....N..........
>     ff ff ff ff ff ff ff ff a0 5e 23 90 ff ff ff ff  .........^#.....
>   backtrace:
>     [<00000000a58ee5fe>] kmalloc_trace+0x22/0x110
>     [<0000000045fe2058>] device_add+0xb34/0x1130
>     [<00000000d778b45f>] cdev_device_add+0x83/0xe0
>     [<0000000089f948ed>] gpiolib_cdev_register+0x73/0xa0
>     [<00000000a3a8a316>] gpiochip_setup_dev+0x1c/0x70
>     [<00000000787227b4>] gpiochip_add_data_with_key+0x10f6/0x1bf0
>     [<000000009ac5742c>] devm_gpiochip_add_data_with_key+0x2e/0x80
>     [<00000000bf2b23d9>] xra1403_probe+0x192/0x1b0 [gpio_xra1403]
>     [<000000005b5ef2d4>] spi_probe+0xe1/0x140
>     [<000000002b26f6f1>] really_probe+0x17c/0x3f0
>     [<00000000dd2dad9c>] __driver_probe_device+0xe3/0x170
>     [<000000005ca60d2a>] device_driver_attach+0x34/0x80
>     [<00000000e9db90db>] bind_store+0x10b/0x1a0
>     [<00000000e2650f8a>] drv_attr_store+0x49/0x70
>     [<0000000080a80b2b>] sysfs_kf_write+0x8c/0xb0
>     [<00000000a28b45b9>] kernfs_fop_write_iter+0x216/0x2e0
> 
> unreferenced object 0xffff888100de9800 (size 512):
>   comm "python3", pid 264, jiffies 4294737615 (age 33.514s)
>   hex dump (first 32 bytes):
>     00 00 00 00 ad 4e ad de ff ff ff ff 00 00 00 00  .....N..........
>     ff ff ff ff ff ff ff ff a0 5e 63 a1 ff ff ff ff  .........^c.....
>   backtrace:
>     [<00000000bcc571d0>] kmalloc_trace+0x22/0x110
>     [<00000000eeb06124>] device_add+0xb34/0x1130
>     [<000000007e5cd2fd>] cdev_device_add+0x83/0xe0
>     [<000000008f6bcd3a>] gpiolib_cdev_register+0x73/0xa0
>     [<0000000012c93b24>] gpiochip_setup_dev+0x1c/0x70
>     [<00000000a24b646a>] gpiochip_add_data_with_key+0x10f6/0x1bf0
>     [<000000000c225212>] tpic2810_probe+0x16e/0x196 [gpio_tpic2810]
>     [<00000000b52d04ff>] i2c_device_probe+0x651/0x680
>     [<0000000058d3ff6b>] really_probe+0x17c/0x3f0
>     [<00000000586f43d3>] __driver_probe_device+0xe3/0x170
>     [<000000003f428602>] device_driver_attach+0x34/0x80
>     [<0000000040e91a1b>] bind_store+0x10b/0x1a0
>     [<00000000c1d990b9>] drv_attr_store+0x49/0x70
>     [<00000000a23bfc22>] sysfs_kf_write+0x8c/0xb0
>     [<00000000064e6572>] kernfs_fop_write_iter+0x216/0x2e0
>     [<00000000026ce093>] vfs_write+0x658/0x810
> 
> Because at the point of gpiochip_setup_dev here, where dev.release
> does not set yet, calling put_device would cause the warning of
> no release function and double-free in the following fault handler
> route (when kfree dev_name). So directly calling kfree to release
> dev->p here in case of memory leak.
> 
> Fixes: 1f5eb8b17f02 ("gpiolib: fix sysfs when cdev is not selected")

I'm confused. You say "gcdev_register & gcdev_unregister call device_add
& device_del" - which is only the case when CONFIG_GPIO_CDEV is not set.

But your trace shows CONFIG_GPIO_CDEV is set, as otherwise
gpiolib_cdev_register() would not exist.

Can you clarify the configuration in which you are seeing the problem?

Assuming CONFIG_GPIO_CDEV is NOT set:

Provide a more appropriate trace.

From a reading of the device_add() documentation, there is a problem if
the device_add() fails - in that case put_device() should be called - and
it is not, instead gpiochip_setup_dev() returns immediately - not going
via the err_remove_device path where your fix is??.
The correct fix for that would be to change the gcdev_register() to call
put_device() if device_add() fails.

Cheers,
Kent.

> Signed-off-by: Zeng Heng <zengheng4@huawei.com>
> ---
>  drivers/gpio/gpiolib.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 4756ea08894f..fa659af86d07 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -539,6 +539,7 @@ static int gpiochip_setup_dev(struct gpio_device *gdev)
>  
>  err_remove_device:
>  	gcdev_unregister(gdev);
> +	kfree(gdev->dev.p);
>  	return ret;
>  }
>  
> -- 
> 2.25.1
> 

Re: [PATCH] gpiolib: fix memory leak in gpiochip_setup_dev

[Kent Gibson]

On Wed, Nov 09, 2022 at 12:57:48AM +0800, Kent Gibson wrote:
> On Tue, Nov 08, 2022 at 07:53:24PM +0800, Zeng Heng wrote:
> > gcdev_register & gcdev_unregister call device_add & device_del to
> > request/release source. But in device_add, the dev->p allocated by
> > device_private_init is not released by device_del.
> > 
> > So when calling gcdev_unregister to release gdev, it needs put_device
> > to release dev in the following.
> > 
> > Otherwise, kmemleak would report memory leak such as below:
> > 
> > unreferenced object 0xffff88810b406400 (size 512):
> >   comm "python3", pid 1682, jiffies 4295346908 (age 24.090s)
> >   hex dump (first 32 bytes):
> >     00 00 00 00 ad 4e ad de ff ff ff ff 00 00 00 00  .....N..........
> >     ff ff ff ff ff ff ff ff a0 5e 23 90 ff ff ff ff  .........^#.....
> >   backtrace:
> >     [<00000000a58ee5fe>] kmalloc_trace+0x22/0x110
> >     [<0000000045fe2058>] device_add+0xb34/0x1130
> >     [<00000000d778b45f>] cdev_device_add+0x83/0xe0
> >     [<0000000089f948ed>] gpiolib_cdev_register+0x73/0xa0
> >     [<00000000a3a8a316>] gpiochip_setup_dev+0x1c/0x70
> >     [<00000000787227b4>] gpiochip_add_data_with_key+0x10f6/0x1bf0
> >     [<000000009ac5742c>] devm_gpiochip_add_data_with_key+0x2e/0x80
> >     [<00000000bf2b23d9>] xra1403_probe+0x192/0x1b0 [gpio_xra1403]
> >     [<000000005b5ef2d4>] spi_probe+0xe1/0x140
> >     [<000000002b26f6f1>] really_probe+0x17c/0x3f0
> >     [<00000000dd2dad9c>] __driver_probe_device+0xe3/0x170
> >     [<000000005ca60d2a>] device_driver_attach+0x34/0x80
> >     [<00000000e9db90db>] bind_store+0x10b/0x1a0
> >     [<00000000e2650f8a>] drv_attr_store+0x49/0x70
> >     [<0000000080a80b2b>] sysfs_kf_write+0x8c/0xb0
> >     [<00000000a28b45b9>] kernfs_fop_write_iter+0x216/0x2e0
> > 
> > unreferenced object 0xffff888100de9800 (size 512):
> >   comm "python3", pid 264, jiffies 4294737615 (age 33.514s)
> >   hex dump (first 32 bytes):
> >     00 00 00 00 ad 4e ad de ff ff ff ff 00 00 00 00  .....N..........
> >     ff ff ff ff ff ff ff ff a0 5e 63 a1 ff ff ff ff  .........^c.....
> >   backtrace:
> >     [<00000000bcc571d0>] kmalloc_trace+0x22/0x110
> >     [<00000000eeb06124>] device_add+0xb34/0x1130
> >     [<000000007e5cd2fd>] cdev_device_add+0x83/0xe0
> >     [<000000008f6bcd3a>] gpiolib_cdev_register+0x73/0xa0
> >     [<0000000012c93b24>] gpiochip_setup_dev+0x1c/0x70
> >     [<00000000a24b646a>] gpiochip_add_data_with_key+0x10f6/0x1bf0
> >     [<000000000c225212>] tpic2810_probe+0x16e/0x196 [gpio_tpic2810]
> >     [<00000000b52d04ff>] i2c_device_probe+0x651/0x680
> >     [<0000000058d3ff6b>] really_probe+0x17c/0x3f0
> >     [<00000000586f43d3>] __driver_probe_device+0xe3/0x170
> >     [<000000003f428602>] device_driver_attach+0x34/0x80
> >     [<0000000040e91a1b>] bind_store+0x10b/0x1a0
> >     [<00000000c1d990b9>] drv_attr_store+0x49/0x70
> >     [<00000000a23bfc22>] sysfs_kf_write+0x8c/0xb0
> >     [<00000000064e6572>] kernfs_fop_write_iter+0x216/0x2e0
> >     [<00000000026ce093>] vfs_write+0x658/0x810
> > 
> > Because at the point of gpiochip_setup_dev here, where dev.release
> > does not set yet, calling put_device would cause the warning of
> > no release function and double-free in the following fault handler
> > route (when kfree dev_name). So directly calling kfree to release
> > dev->p here in case of memory leak.
> > 
> > Fixes: 1f5eb8b17f02 ("gpiolib: fix sysfs when cdev is not selected")
> 
> I'm confused. You say "gcdev_register & gcdev_unregister call device_add
> & device_del" - which is only the case when CONFIG_GPIO_CDEV is not set.
> 
> But your trace shows CONFIG_GPIO_CDEV is set, as otherwise
> gpiolib_cdev_register() would not exist.
> 
> Can you clarify the configuration in which you are seeing the problem?
> 
> Assuming CONFIG_GPIO_CDEV is NOT set:
> 
> Provide a more appropriate trace.
> 
> From a reading of the device_add() documentation, there is a problem if
> the device_add() fails - in that case put_device() should be called - and
> it is not, instead gpiochip_setup_dev() returns immediately - not going
> via the err_remove_device path where your fix is??.
> The correct fix for that would be to change the gcdev_register() to call
> put_device() if device_add() fails.
> 

Ignore that - as you mentioned the dev.release hasn't been set at that
point.

Having another look at this, I don't think the problem is related to the
Fixed commit at all - it looks more general.
How did you conclude that that commit introduced the problem?
Is it easily repeatable and have you bisected for it?

Cheers,
Kent.
 

Re: [PATCH] gpiolib: fix memory leak in gpiochip_setup_dev

[Zeng Heng]

On 2022/11/9 13:12, Kent Gibson wrote:
> On Wed, Nov 09, 2022 at 12:57:48AM +0800, Kent Gibson wrote:
>> On Tue, Nov 08, 2022 at 07:53:24PM +0800, Zeng Heng wrote:
>>> gcdev_register & gcdev_unregister call device_add & device_del to
>>> request/release source. But in device_add, the dev->p allocated by
>>> device_private_init is not released by device_del.
>>>
>>> So when calling gcdev_unregister to release gdev, it needs put_device
>>> to release dev in the following.
>>>
>>> Otherwise, kmemleak would report memory leak such as below:
>>>
>>> unreferenced object 0xffff88810b406400 (size 512):
>>>    comm "python3", pid 1682, jiffies 4295346908 (age 24.090s)
>>>    hex dump (first 32 bytes):
>>>      00 00 00 00 ad 4e ad de ff ff ff ff 00 00 00 00  .....N..........
>>>      ff ff ff ff ff ff ff ff a0 5e 23 90 ff ff ff ff  .........^#.....
>>>    backtrace:
>>>      [<00000000a58ee5fe>] kmalloc_trace+0x22/0x110
>>>      [<0000000045fe2058>] device_add+0xb34/0x1130
>>>      [<00000000d778b45f>] cdev_device_add+0x83/0xe0
>>>      [<0000000089f948ed>] gpiolib_cdev_register+0x73/0xa0
>>>      [<00000000a3a8a316>] gpiochip_setup_dev+0x1c/0x70
>>>      [<00000000787227b4>] gpiochip_add_data_with_key+0x10f6/0x1bf0
>>>      [<000000009ac5742c>] devm_gpiochip_add_data_with_key+0x2e/0x80
>>>      [<00000000bf2b23d9>] xra1403_probe+0x192/0x1b0 [gpio_xra1403]
>>>      [<000000005b5ef2d4>] spi_probe+0xe1/0x140
>>>      [<000000002b26f6f1>] really_probe+0x17c/0x3f0
>>>      [<00000000dd2dad9c>] __driver_probe_device+0xe3/0x170
>>>      [<000000005ca60d2a>] device_driver_attach+0x34/0x80
>>>      [<00000000e9db90db>] bind_store+0x10b/0x1a0
>>>      [<00000000e2650f8a>] drv_attr_store+0x49/0x70
>>>      [<0000000080a80b2b>] sysfs_kf_write+0x8c/0xb0
>>>      [<00000000a28b45b9>] kernfs_fop_write_iter+0x216/0x2e0
>>>
>>> unreferenced object 0xffff888100de9800 (size 512):
>>>    comm "python3", pid 264, jiffies 4294737615 (age 33.514s)
>>>    hex dump (first 32 bytes):
>>>      00 00 00 00 ad 4e ad de ff ff ff ff 00 00 00 00  .....N..........
>>>      ff ff ff ff ff ff ff ff a0 5e 63 a1 ff ff ff ff  .........^c.....
>>>    backtrace:
>>>      [<00000000bcc571d0>] kmalloc_trace+0x22/0x110
>>>      [<00000000eeb06124>] device_add+0xb34/0x1130
>>>      [<000000007e5cd2fd>] cdev_device_add+0x83/0xe0
>>>      [<000000008f6bcd3a>] gpiolib_cdev_register+0x73/0xa0
>>>      [<0000000012c93b24>] gpiochip_setup_dev+0x1c/0x70
>>>      [<00000000a24b646a>] gpiochip_add_data_with_key+0x10f6/0x1bf0
>>>      [<000000000c225212>] tpic2810_probe+0x16e/0x196 [gpio_tpic2810]
>>>      [<00000000b52d04ff>] i2c_device_probe+0x651/0x680
>>>      [<0000000058d3ff6b>] really_probe+0x17c/0x3f0
>>>      [<00000000586f43d3>] __driver_probe_device+0xe3/0x170
>>>      [<000000003f428602>] device_driver_attach+0x34/0x80
>>>      [<0000000040e91a1b>] bind_store+0x10b/0x1a0
>>>      [<00000000c1d990b9>] drv_attr_store+0x49/0x70
>>>      [<00000000a23bfc22>] sysfs_kf_write+0x8c/0xb0
>>>      [<00000000064e6572>] kernfs_fop_write_iter+0x216/0x2e0
>>>      [<00000000026ce093>] vfs_write+0x658/0x810
>>>
>>> Because at the point of gpiochip_setup_dev here, where dev.release
>>> does not set yet, calling put_device would cause the warning of
>>> no release function and double-free in the following fault handler
>>> route (when kfree dev_name). So directly calling kfree to release
>>> dev->p here in case of memory leak.
>>>
>>> Fixes: 1f5eb8b17f02 ("gpiolib: fix sysfs when cdev is not selected")
>> I'm confused. You say "gcdev_register & gcdev_unregister call device_add
>> & device_del" - which is only the case when CONFIG_GPIO_CDEV is not set.
>>
>> But your trace shows CONFIG_GPIO_CDEV is set, as otherwise
>> gpiolib_cdev_register() would not exist.
>>
>> Can you clarify the configuration in which you are seeing the problem?
>>
>> Assuming CONFIG_GPIO_CDEV is NOT set:
>>
>> Provide a more appropriate trace.
>>
>>  From a reading of the device_add() documentation, there is a problem if
>> the device_add() fails - in that case put_device() should be called - and
>> it is not, instead gpiochip_setup_dev() returns immediately - not going
>> via the err_remove_device path where your fix is??.
>> The correct fix for that would be to change the gcdev_register() to call
>> put_device() if device_add() fails.
>>
> Ignore that - as you mentioned the dev.release hasn't been set at that
> point.
>
> Having another look at this, I don't think the problem is related to the
> Fixed commit at all - it looks more general.
> How did you conclude that that commit introduced the problem?
> Is it easily repeatable and have you bisected for it?
>
> Cheers,
> Kent.
>   

Thanks to your patience for taking another look at this mail.

And allow me to claim that I indeed do the inject-fault test and 
regression test based on the patch.


My test environment has included CONFIG_GPIO_CDEV config, but no matter 
includes this config or not,

there is still memory leak in fault handle route about dev->p because of 
calling device_add.


Apologize for the fixed commit is not accurate, and exactly it's this one:

Fixes: 159f3cd92f17 ("gpiolib: Defer gpio device setup until after 
gpiolib initialization")


If everything is all right, I would send the second version patch and 
correct fix tag.

thanks all,

Zeng Heng

Re: [PATCH] gpiolib: fix memory leak in gpiochip_setup_dev

[Kent Gibson]

On Wed, Nov 09, 2022 at 04:27:26PM +0800, Zeng Heng wrote:
> 
> On 2022/11/9 13:12, Kent Gibson wrote:
> > On Wed, Nov 09, 2022 at 12:57:48AM +0800, Kent Gibson wrote:
> > > On Tue, Nov 08, 2022 at 07:53:24PM +0800, Zeng Heng wrote:
> > > > gcdev_register & gcdev_unregister call device_add & device_del to
> > > > request/release source. But in device_add, the dev->p allocated by
> > > > device_private_init is not released by device_del.
> > > > 
> > > > So when calling gcdev_unregister to release gdev, it needs put_device
> > > > to release dev in the following.
> > > > 
> > > > Otherwise, kmemleak would report memory leak such as below:
> > > > 
> > > > unreferenced object 0xffff88810b406400 (size 512):
> > > >    comm "python3", pid 1682, jiffies 4295346908 (age 24.090s)
> > > >    hex dump (first 32 bytes):
> > > >      00 00 00 00 ad 4e ad de ff ff ff ff 00 00 00 00  .....N..........
> > > >      ff ff ff ff ff ff ff ff a0 5e 23 90 ff ff ff ff  .........^#.....
> > > >    backtrace:
> > > >      [<00000000a58ee5fe>] kmalloc_trace+0x22/0x110
> > > >      [<0000000045fe2058>] device_add+0xb34/0x1130
> > > >      [<00000000d778b45f>] cdev_device_add+0x83/0xe0
> > > >      [<0000000089f948ed>] gpiolib_cdev_register+0x73/0xa0
> > > >      [<00000000a3a8a316>] gpiochip_setup_dev+0x1c/0x70
> > > >      [<00000000787227b4>] gpiochip_add_data_with_key+0x10f6/0x1bf0
> > > >      [<000000009ac5742c>] devm_gpiochip_add_data_with_key+0x2e/0x80
> > > >      [<00000000bf2b23d9>] xra1403_probe+0x192/0x1b0 [gpio_xra1403]
> > > >      [<000000005b5ef2d4>] spi_probe+0xe1/0x140
> > > >      [<000000002b26f6f1>] really_probe+0x17c/0x3f0
> > > >      [<00000000dd2dad9c>] __driver_probe_device+0xe3/0x170
> > > >      [<000000005ca60d2a>] device_driver_attach+0x34/0x80
> > > >      [<00000000e9db90db>] bind_store+0x10b/0x1a0
> > > >      [<00000000e2650f8a>] drv_attr_store+0x49/0x70
> > > >      [<0000000080a80b2b>] sysfs_kf_write+0x8c/0xb0
> > > >      [<00000000a28b45b9>] kernfs_fop_write_iter+0x216/0x2e0
> > > > 
> > > > unreferenced object 0xffff888100de9800 (size 512):
> > > >    comm "python3", pid 264, jiffies 4294737615 (age 33.514s)
> > > >    hex dump (first 32 bytes):
> > > >      00 00 00 00 ad 4e ad de ff ff ff ff 00 00 00 00  .....N..........
> > > >      ff ff ff ff ff ff ff ff a0 5e 63 a1 ff ff ff ff  .........^c.....
> > > >    backtrace:
> > > >      [<00000000bcc571d0>] kmalloc_trace+0x22/0x110
> > > >      [<00000000eeb06124>] device_add+0xb34/0x1130
> > > >      [<000000007e5cd2fd>] cdev_device_add+0x83/0xe0
> > > >      [<000000008f6bcd3a>] gpiolib_cdev_register+0x73/0xa0
> > > >      [<0000000012c93b24>] gpiochip_setup_dev+0x1c/0x70
> > > >      [<00000000a24b646a>] gpiochip_add_data_with_key+0x10f6/0x1bf0
> > > >      [<000000000c225212>] tpic2810_probe+0x16e/0x196 [gpio_tpic2810]
> > > >      [<00000000b52d04ff>] i2c_device_probe+0x651/0x680
> > > >      [<0000000058d3ff6b>] really_probe+0x17c/0x3f0
> > > >      [<00000000586f43d3>] __driver_probe_device+0xe3/0x170
> > > >      [<000000003f428602>] device_driver_attach+0x34/0x80
> > > >      [<0000000040e91a1b>] bind_store+0x10b/0x1a0
> > > >      [<00000000c1d990b9>] drv_attr_store+0x49/0x70
> > > >      [<00000000a23bfc22>] sysfs_kf_write+0x8c/0xb0
> > > >      [<00000000064e6572>] kernfs_fop_write_iter+0x216/0x2e0
> > > >      [<00000000026ce093>] vfs_write+0x658/0x810
> > > > 
> > > > Because at the point of gpiochip_setup_dev here, where dev.release
> > > > does not set yet, calling put_device would cause the warning of
> > > > no release function and double-free in the following fault handler
> > > > route (when kfree dev_name). So directly calling kfree to release
> > > > dev->p here in case of memory leak.
> > > > 
> > > > Fixes: 1f5eb8b17f02 ("gpiolib: fix sysfs when cdev is not selected")
> > > I'm confused. You say "gcdev_register & gcdev_unregister call device_add
> > > & device_del" - which is only the case when CONFIG_GPIO_CDEV is not set.
> > > 
> > > But your trace shows CONFIG_GPIO_CDEV is set, as otherwise
> > > gpiolib_cdev_register() would not exist.
> > > 
> > > Can you clarify the configuration in which you are seeing the problem?
> > > 
> > > Assuming CONFIG_GPIO_CDEV is NOT set:
> > > 
> > > Provide a more appropriate trace.
> > > 
> > >  From a reading of the device_add() documentation, there is a problem if
> > > the device_add() fails - in that case put_device() should be called - and
> > > it is not, instead gpiochip_setup_dev() returns immediately - not going
> > > via the err_remove_device path where your fix is??.
> > > The correct fix for that would be to change the gcdev_register() to call
> > > put_device() if device_add() fails.
> > > 
> > Ignore that - as you mentioned the dev.release hasn't been set at that
> > point.
> > 
> > Having another look at this, I don't think the problem is related to the
> > Fixed commit at all - it looks more general.
> > How did you conclude that that commit introduced the problem?
> > Is it easily repeatable and have you bisected for it?
> > 
> > Cheers,
> > Kent.
> 
> Thanks to your patience for taking another look at this mail.
> 

No problem - my bad for initially responding without giving it more
consideration.

> And allow me to claim that I indeed do the inject-fault test and regression
> test based on the patch.
> 
> 
> My test environment has included CONFIG_GPIO_CDEV config, but no matter
> includes this config or not,
> 
> there is still memory leak in fault handle route about dev->p because of
> calling device_add.
> 
> 
> Apologize for the fixed commit is not accurate, and exactly it's this one:
> 
> Fixes: 159f3cd92f17 ("gpiolib: Defer gpio device setup until after gpiolib
> initialization")
> 
> 
> If everything is all right, I would send the second version patch and
> correct fix tag.
> 

Yeah, that makes more sense - it has been there since that section of
code was reworked - quite some time ago.  Good catch.

Cheers,
Kent.

[PATCH v2] gpiolib: fix memory leak in gpiochip_setup_dev

[Zeng Heng]

gcdev_register & gcdev_unregister call device_add & device_del to
request/release source. But in device_add, the dev->p allocated by
device_private_init is not released by device_del.

So when calling gcdev_unregister to release gdev, it needs put_device
to release dev in the following.

Otherwise, kmemleak would report memory leak such as below:

unreferenced object 0xffff88810b406400 (size 512):
  comm "python3", pid 1682, jiffies 4295346908 (age 24.090s)
  hex dump (first 32 bytes):
    00 00 00 00 ad 4e ad de ff ff ff ff 00 00 00 00  .....N..........
    ff ff ff ff ff ff ff ff a0 5e 23 90 ff ff ff ff  .........^#.....
  backtrace:
    [<00000000a58ee5fe>] kmalloc_trace+0x22/0x110
    [<0000000045fe2058>] device_add+0xb34/0x1130
    [<00000000d778b45f>] cdev_device_add+0x83/0xe0
    [<0000000089f948ed>] gpiolib_cdev_register+0x73/0xa0
    [<00000000a3a8a316>] gpiochip_setup_dev+0x1c/0x70
    [<00000000787227b4>] gpiochip_add_data_with_key+0x10f6/0x1bf0
    [<000000009ac5742c>] devm_gpiochip_add_data_with_key+0x2e/0x80
    [<00000000bf2b23d9>] xra1403_probe+0x192/0x1b0 [gpio_xra1403]
    [<000000005b5ef2d4>] spi_probe+0xe1/0x140
    [<000000002b26f6f1>] really_probe+0x17c/0x3f0
    [<00000000dd2dad9c>] __driver_probe_device+0xe3/0x170
    [<000000005ca60d2a>] device_driver_attach+0x34/0x80
    [<00000000e9db90db>] bind_store+0x10b/0x1a0
    [<00000000e2650f8a>] drv_attr_store+0x49/0x70
    [<0000000080a80b2b>] sysfs_kf_write+0x8c/0xb0
    [<00000000a28b45b9>] kernfs_fop_write_iter+0x216/0x2e0

unreferenced object 0xffff888100de9800 (size 512):
  comm "python3", pid 264, jiffies 4294737615 (age 33.514s)
  hex dump (first 32 bytes):
    00 00 00 00 ad 4e ad de ff ff ff ff 00 00 00 00  .....N..........
    ff ff ff ff ff ff ff ff a0 5e 63 a1 ff ff ff ff  .........^c.....
  backtrace:
    [<00000000bcc571d0>] kmalloc_trace+0x22/0x110
    [<00000000eeb06124>] device_add+0xb34/0x1130
    [<000000007e5cd2fd>] cdev_device_add+0x83/0xe0
    [<000000008f6bcd3a>] gpiolib_cdev_register+0x73/0xa0
    [<0000000012c93b24>] gpiochip_setup_dev+0x1c/0x70
    [<00000000a24b646a>] gpiochip_add_data_with_key+0x10f6/0x1bf0
    [<000000000c225212>] tpic2810_probe+0x16e/0x196 [gpio_tpic2810]
    [<00000000b52d04ff>] i2c_device_probe+0x651/0x680
    [<0000000058d3ff6b>] really_probe+0x17c/0x3f0
    [<00000000586f43d3>] __driver_probe_device+0xe3/0x170
    [<000000003f428602>] device_driver_attach+0x34/0x80
    [<0000000040e91a1b>] bind_store+0x10b/0x1a0
    [<00000000c1d990b9>] drv_attr_store+0x49/0x70
    [<00000000a23bfc22>] sysfs_kf_write+0x8c/0xb0
    [<00000000064e6572>] kernfs_fop_write_iter+0x216/0x2e0
    [<00000000026ce093>] vfs_write+0x658/0x810

Because at the point of gpiochip_setup_dev here, where dev.release
does not set yet, calling put_device would cause the warning of
no release function and double-free in the following fault handler
route (when kfree dev_name). So directly calling kfree to release
dev->p here in case of memory leak.

Fixes: 159f3cd92f17 ("gpiolib: Defer gpio device setup until after gpiolib initialization")
Signed-off-by: Zeng Heng <zengheng4@huawei.com>
---
changes in v2:
  - correct fix tag

---
 drivers/gpio/gpiolib.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 4756ea08894f..fa659af86d07 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -539,6 +539,7 @@ static int gpiochip_setup_dev(struct gpio_device *gdev)
 
 err_remove_device:
 	gcdev_unregister(gdev);
+	kfree(gdev->dev.p);
 	return ret;
 }
 
-- 
2.25.1

Re: [PATCH v2] gpiolib: fix memory leak in gpiochip_setup_dev

[Andy Shevchenko]

On Wed, Nov 09, 2022 at 05:31:20PM +0800, Zeng Heng wrote:
> gcdev_register & gcdev_unregister call device_add & device_del to
> request/release source. But in device_add, the dev->p allocated by
> device_private_init is not released by device_del.

First of all, we refer to the functions like func().

> So when calling gcdev_unregister to release gdev, it needs put_device
> to release dev in the following.
> 
> Otherwise, kmemleak would report memory leak such as below:
> 
> unreferenced object 0xffff88810b406400 (size 512):
>   comm "python3", pid 1682, jiffies 4295346908 (age 24.090s)
>   hex dump (first 32 bytes):
>     00 00 00 00 ad 4e ad de ff ff ff ff 00 00 00 00  .....N..........
>     ff ff ff ff ff ff ff ff a0 5e 23 90 ff ff ff ff  .........^#.....
>   backtrace:

Second, read Submitting Patches on how to provide your backtraces in the
message body.

>     [<00000000a58ee5fe>] kmalloc_trace+0x22/0x110
>     [<0000000045fe2058>] device_add+0xb34/0x1130
>     [<00000000d778b45f>] cdev_device_add+0x83/0xe0
>     [<0000000089f948ed>] gpiolib_cdev_register+0x73/0xa0
>     [<00000000a3a8a316>] gpiochip_setup_dev+0x1c/0x70
>     [<00000000787227b4>] gpiochip_add_data_with_key+0x10f6/0x1bf0
>     [<000000009ac5742c>] devm_gpiochip_add_data_with_key+0x2e/0x80
>     [<00000000bf2b23d9>] xra1403_probe+0x192/0x1b0 [gpio_xra1403]
>     [<000000005b5ef2d4>] spi_probe+0xe1/0x140
>     [<000000002b26f6f1>] really_probe+0x17c/0x3f0
>     [<00000000dd2dad9c>] __driver_probe_device+0xe3/0x170
>     [<000000005ca60d2a>] device_driver_attach+0x34/0x80
>     [<00000000e9db90db>] bind_store+0x10b/0x1a0
>     [<00000000e2650f8a>] drv_attr_store+0x49/0x70
>     [<0000000080a80b2b>] sysfs_kf_write+0x8c/0xb0
>     [<00000000a28b45b9>] kernfs_fop_write_iter+0x216/0x2e0
> 
> unreferenced object 0xffff888100de9800 (size 512):
>   comm "python3", pid 264, jiffies 4294737615 (age 33.514s)
>   hex dump (first 32 bytes):
>     00 00 00 00 ad 4e ad de ff ff ff ff 00 00 00 00  .....N..........
>     ff ff ff ff ff ff ff ff a0 5e 63 a1 ff ff ff ff  .........^c.....
>   backtrace:
>     [<00000000bcc571d0>] kmalloc_trace+0x22/0x110
>     [<00000000eeb06124>] device_add+0xb34/0x1130
>     [<000000007e5cd2fd>] cdev_device_add+0x83/0xe0
>     [<000000008f6bcd3a>] gpiolib_cdev_register+0x73/0xa0
>     [<0000000012c93b24>] gpiochip_setup_dev+0x1c/0x70
>     [<00000000a24b646a>] gpiochip_add_data_with_key+0x10f6/0x1bf0
>     [<000000000c225212>] tpic2810_probe+0x16e/0x196 [gpio_tpic2810]
>     [<00000000b52d04ff>] i2c_device_probe+0x651/0x680
>     [<0000000058d3ff6b>] really_probe+0x17c/0x3f0
>     [<00000000586f43d3>] __driver_probe_device+0xe3/0x170
>     [<000000003f428602>] device_driver_attach+0x34/0x80
>     [<0000000040e91a1b>] bind_store+0x10b/0x1a0
>     [<00000000c1d990b9>] drv_attr_store+0x49/0x70
>     [<00000000a23bfc22>] sysfs_kf_write+0x8c/0xb0
>     [<00000000064e6572>] kernfs_fop_write_iter+0x216/0x2e0
>     [<00000000026ce093>] vfs_write+0x658/0x810
> 
> Because at the point of gpiochip_setup_dev here, where dev.release
> does not set yet, calling put_device would cause the warning of
> no release function and double-free in the following fault handler
> route (when kfree dev_name). So directly calling kfree to release
> dev->p here in case of memory leak.

...

> @@ -539,6 +539,7 @@ static int gpiochip_setup_dev(struct gpio_device *gdev)
>  
>  err_remove_device:
>  	gcdev_unregister(gdev);
> +	kfree(gdev->dev.p);
>  	return ret;

Third, I do not believe it's a correct fix.
Have you read comments around device_del() / etc.?

NAK.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] gpiolib: fix memory leak in gpiochip_setup_dev

[Kent Gibson]

On Wed, Nov 09, 2022 at 04:47:08PM +0200, Andy Shevchenko wrote:
> On Wed, Nov 09, 2022 at 05:31:20PM +0800, Zeng Heng wrote:
> > gcdev_register & gcdev_unregister call device_add & device_del to
> > request/release source. But in device_add, the dev->p allocated by
> > device_private_init is not released by device_del.
> 
> First of all, we refer to the functions like func().
> 

Further to this, the description of the problem could be clearer -
it would be helpful to indicate the code path that triggers the problem
- it is gpiochip_sysfs_register() returning an error?

> > So when calling gcdev_unregister to release gdev, it needs put_device
> > to release dev in the following.
> > 
> > Otherwise, kmemleak would report memory leak such as below:
> > 
> > unreferenced object 0xffff88810b406400 (size 512):
> >   comm "python3", pid 1682, jiffies 4295346908 (age 24.090s)
> >   hex dump (first 32 bytes):
> >     00 00 00 00 ad 4e ad de ff ff ff ff 00 00 00 00  .....N..........
> >     ff ff ff ff ff ff ff ff a0 5e 23 90 ff ff ff ff  .........^#.....
> >   backtrace:
> 
> Second, read Submitting Patches on how to provide your backtraces in the
> message body.
> 
> >     [<00000000a58ee5fe>] kmalloc_trace+0x22/0x110
> >     [<0000000045fe2058>] device_add+0xb34/0x1130
> >     [<00000000d778b45f>] cdev_device_add+0x83/0xe0
> >     [<0000000089f948ed>] gpiolib_cdev_register+0x73/0xa0
> >     [<00000000a3a8a316>] gpiochip_setup_dev+0x1c/0x70
> >     [<00000000787227b4>] gpiochip_add_data_with_key+0x10f6/0x1bf0
> >     [<000000009ac5742c>] devm_gpiochip_add_data_with_key+0x2e/0x80
> >     [<00000000bf2b23d9>] xra1403_probe+0x192/0x1b0 [gpio_xra1403]
> >     [<000000005b5ef2d4>] spi_probe+0xe1/0x140
> >     [<000000002b26f6f1>] really_probe+0x17c/0x3f0
> >     [<00000000dd2dad9c>] __driver_probe_device+0xe3/0x170
> >     [<000000005ca60d2a>] device_driver_attach+0x34/0x80
> >     [<00000000e9db90db>] bind_store+0x10b/0x1a0
> >     [<00000000e2650f8a>] drv_attr_store+0x49/0x70
> >     [<0000000080a80b2b>] sysfs_kf_write+0x8c/0xb0
> >     [<00000000a28b45b9>] kernfs_fop_write_iter+0x216/0x2e0
> > 
> > unreferenced object 0xffff888100de9800 (size 512):
> >   comm "python3", pid 264, jiffies 4294737615 (age 33.514s)
> >   hex dump (first 32 bytes):
> >     00 00 00 00 ad 4e ad de ff ff ff ff 00 00 00 00  .....N..........
> >     ff ff ff ff ff ff ff ff a0 5e 63 a1 ff ff ff ff  .........^c.....
> >   backtrace:
> >     [<00000000bcc571d0>] kmalloc_trace+0x22/0x110
> >     [<00000000eeb06124>] device_add+0xb34/0x1130
> >     [<000000007e5cd2fd>] cdev_device_add+0x83/0xe0
> >     [<000000008f6bcd3a>] gpiolib_cdev_register+0x73/0xa0
> >     [<0000000012c93b24>] gpiochip_setup_dev+0x1c/0x70
> >     [<00000000a24b646a>] gpiochip_add_data_with_key+0x10f6/0x1bf0
> >     [<000000000c225212>] tpic2810_probe+0x16e/0x196 [gpio_tpic2810]
> >     [<00000000b52d04ff>] i2c_device_probe+0x651/0x680
> >     [<0000000058d3ff6b>] really_probe+0x17c/0x3f0
> >     [<00000000586f43d3>] __driver_probe_device+0xe3/0x170
> >     [<000000003f428602>] device_driver_attach+0x34/0x80
> >     [<0000000040e91a1b>] bind_store+0x10b/0x1a0
> >     [<00000000c1d990b9>] drv_attr_store+0x49/0x70
> >     [<00000000a23bfc22>] sysfs_kf_write+0x8c/0xb0
> >     [<00000000064e6572>] kernfs_fop_write_iter+0x216/0x2e0
> >     [<00000000026ce093>] vfs_write+0x658/0x810
> > 
> > Because at the point of gpiochip_setup_dev here, where dev.release
> > does not set yet, calling put_device would cause the warning of
> > no release function and double-free in the following fault handler
> > route (when kfree dev_name). So directly calling kfree to release
> > dev->p here in case of memory leak.
> 

Again, this could be clearer.  The dev->p is normally freed by
device_release() - why is that not happening in this case?
(as put_device() is never called in this path)

The double free you see if you do call put_device() appears to be due to
different expectations as to the cleanup that gpiochip_setup_dev() will
perform on error, depending on where it is called. gpiochip_setup_devs()
assumes any cleanup is performed by gpiochip_setup_dev(), while
gpiochip_add_data_with_key() assumes that it hasn't performed any cleanup.

Having gpiochip_setup_dev() perform its own cleanup makes the most sense
to me, so gpiochip_add_data_with_key() should be changed to allow for
that.

> ...
> 
> > @@ -539,6 +539,7 @@ static int gpiochip_setup_dev(struct gpio_device *gdev)
> >  
> >  err_remove_device:
> >  	gcdev_unregister(gdev);
> > +	kfree(gdev->dev.p);
> >  	return ret;
> 
> Third, I do not believe it's a correct fix.
> Have you read comments around device_del() / etc.?
> 

I agree - this is not the correct fix.  The correct fix is to trigger the
normal cleanup mechanism, so put_device().
The fact that that triggers a warning:

"Device '%s' does not have a release() function, it is broken and must be
fixed. See Documentation/core-api/kobject.rst.\n"

is an indicator that dev.release should be set earlier.
If gpiodevice_release() is not appropriate, or cannot be modified to deal
with the device state at that point, then an appropriate interim release
function should be set.

And, as mentioned above, gpiochip_add_data_with_key() needs to be modified
to allow for gpiochip_setup_dev() cleaning up its own mess.

That is my take, but that is just from perusing the code so I may be
totally off base.  Either way, an ACK/NACK on this from a maintainer or
other gpiolib expert would be helpful to expiditing a solution.

Cheers,
Kent.

Re: [PATCH v2] gpiolib: fix memory leak in gpiochip_setup_dev

[Zeng Heng]

On 2022/11/10 9:26, Kent Gibson wrote:
> On Wed, Nov 09, 2022 at 04:47:08PM +0200, Andy Shevchenko wrote:
>> On Wed, Nov 09, 2022 at 05:31:20PM +0800, Zeng Heng wrote:
>>> gcdev_register & gcdev_unregister call device_add & device_del to
>>> request/release source. But in device_add, the dev->p allocated by
>>> device_private_init is not released by device_del.
>> First of all, we refer to the functions like func().
Thanks, it would be updated in next version.
> Further to this, the description of the problem could be clearer -
> it would be helpful to indicate the code path that triggers the problem
> - it is gpiochip_sysfs_register() returning an error?
>
>>> So when calling gcdev_unregister to release gdev, it needs put_device
>>> to release dev in the following.
>>>
>>> Otherwise, kmemleak would report memory leak such as below:
>>>
>>> unreferenced object 0xffff88810b406400 (size 512):
>>>    comm "python3", pid 1682, jiffies 4295346908 (age 24.090s)
>>>    hex dump (first 32 bytes):
>>>      00 00 00 00 ad 4e ad de ff ff ff ff 00 00 00 00  .....N..........
>>>      ff ff ff ff ff ff ff ff a0 5e 23 90 ff ff ff ff  .........^#.....
>>>    backtrace:
>> Second, read Submitting Patches on how to provide your backtraces in the
>> message body.

Thanks, it would be updated in next version.

>>
>>>      [<00000000a58ee5fe>] kmalloc_trace+0x22/0x110
>>>      [<0000000045fe2058>] device_add+0xb34/0x1130
>>>      [<00000000d778b45f>] cdev_device_add+0x83/0xe0
>>>      [<0000000089f948ed>] gpiolib_cdev_register+0x73/0xa0
>>>      [<00000000a3a8a316>] gpiochip_setup_dev+0x1c/0x70
>>>      [<00000000787227b4>] gpiochip_add_data_with_key+0x10f6/0x1bf0
>>>      [<000000009ac5742c>] devm_gpiochip_add_data_with_key+0x2e/0x80
>>>      [<00000000bf2b23d9>] xra1403_probe+0x192/0x1b0 [gpio_xra1403]
>>>      [<000000005b5ef2d4>] spi_probe+0xe1/0x140
>>>      [<000000002b26f6f1>] really_probe+0x17c/0x3f0
>>>      [<00000000dd2dad9c>] __driver_probe_device+0xe3/0x170
>>>      [<000000005ca60d2a>] device_driver_attach+0x34/0x80
>>>      [<00000000e9db90db>] bind_store+0x10b/0x1a0
>>>      [<00000000e2650f8a>] drv_attr_store+0x49/0x70
>>>      [<0000000080a80b2b>] sysfs_kf_write+0x8c/0xb0
>>>      [<00000000a28b45b9>] kernfs_fop_write_iter+0x216/0x2e0
>>>
>>> unreferenced object 0xffff888100de9800 (size 512):
>>>    comm "python3", pid 264, jiffies 4294737615 (age 33.514s)
>>>    hex dump (first 32 bytes):
>>>      00 00 00 00 ad 4e ad de ff ff ff ff 00 00 00 00  .....N..........
>>>      ff ff ff ff ff ff ff ff a0 5e 63 a1 ff ff ff ff  .........^c.....
>>>    backtrace:
>>>      [<00000000bcc571d0>] kmalloc_trace+0x22/0x110
>>>      [<00000000eeb06124>] device_add+0xb34/0x1130
>>>      [<000000007e5cd2fd>] cdev_device_add+0x83/0xe0
>>>      [<000000008f6bcd3a>] gpiolib_cdev_register+0x73/0xa0
>>>      [<0000000012c93b24>] gpiochip_setup_dev+0x1c/0x70
>>>      [<00000000a24b646a>] gpiochip_add_data_with_key+0x10f6/0x1bf0
>>>      [<000000000c225212>] tpic2810_probe+0x16e/0x196 [gpio_tpic2810]
>>>      [<00000000b52d04ff>] i2c_device_probe+0x651/0x680
>>>      [<0000000058d3ff6b>] really_probe+0x17c/0x3f0
>>>      [<00000000586f43d3>] __driver_probe_device+0xe3/0x170
>>>      [<000000003f428602>] device_driver_attach+0x34/0x80
>>>      [<0000000040e91a1b>] bind_store+0x10b/0x1a0
>>>      [<00000000c1d990b9>] drv_attr_store+0x49/0x70
>>>      [<00000000a23bfc22>] sysfs_kf_write+0x8c/0xb0
>>>      [<00000000064e6572>] kernfs_fop_write_iter+0x216/0x2e0
>>>      [<00000000026ce093>] vfs_write+0x658/0x810
>>>
>>> Because at the point of gpiochip_setup_dev here, where dev.release
>>> does not set yet, calling put_device would cause the warning of
>>> no release function and double-free in the following fault handler
>>> route (when kfree dev_name). So directly calling kfree to release
>>> dev->p here in case of memory leak.
> Again, this could be clearer.  The dev->p is normally freed by
> device_release() - why is that not happening in this case?
> (as put_device() is never called in this path)
>
> The double free you see if you do call put_device() appears to be due to
> different expectations as to the cleanup that gpiochip_setup_dev() will
> perform on error, depending on where it is called. gpiochip_setup_devs()
> assumes any cleanup is performed by gpiochip_setup_dev(), while
> gpiochip_add_data_with_key() assumes that it hasn't performed any cleanup.
>
> Having gpiochip_setup_dev() perform its own cleanup makes the most sense
> to me, so gpiochip_add_data_with_key() should be changed to allow for
> that.


Right, the cleanup route of gpiochip_add_data_with_key() & 
gpiochip_setup_dev()

has to be considered comprehensively after any possible cases of fault 
injections.


>> ...
>>
>>> @@ -539,6 +539,7 @@ static int gpiochip_setup_dev(struct gpio_device *gdev)
>>>   
>>>   err_remove_device:
>>>   	gcdev_unregister(gdev);
>>> +	kfree(gdev->dev.p);
>>>   	return ret;
>> Third, I do not believe it's a correct fix.
>> Have you read comments around device_del() / etc.?

Yes, not only the comments I read, but also the device_del() implement code.

Releasing the dev->p pointer is not the business with device_del(), but 
it's relied on

put_device() calling release function.

Turning back here, the release function is not set yet at this point, 
there is a gap

between device_add() and set release function pointer.

That's the reason why choose to free dev->p explicitly as the mail 
mentioned above.

> I agree - this is not the correct fix.  The correct fix is to trigger the
> normal cleanup mechanism, so put_device().
> The fact that that triggers a warning:
>
> "Device '%s' does not have a release() function, it is broken and must be
> fixed. See Documentation/core-api/kobject.rst.\n"
>
> is an indicator that dev.release should be set earlier.
> If gpiodevice_release() is not appropriate, or cannot be modified to deal
> with the device state at that point, then an appropriate interim release
> function should be set.
>
> And, as mentioned above, gpiochip_add_data_with_key() needs to be modified
> to allow for gpiochip_setup_dev() cleaning up its own mess.
>
> That is my take, but that is just from perusing the code so I may be
> totally off base.  Either way, an ACK/NACK on this from a maintainer or
> other gpiolib expert would be helpful to expiditing a solution.
>
> Cheers,
> Kent.

Yes, exactly.

Thanks to all,

Zeng Heng

[PATCH v3] gpiolib: fix memory leak in gpiochip_setup_dev()

[Zeng Heng]

Here is a report about memory leak detected in gpiochip_setup_dev():

unreferenced object 0xffff88810b406400 (size 512):
  comm "python3", pid 1682, jiffies 4295346908 (age 24.090s)
  backtrace:
    kmalloc_trace
    device_add 		device_private_init at drivers/base/core.c:3361
			(inlined by) device_add at drivers/base/core.c:3411
    cdev_device_add
    gpiolib_cdev_register
    gpiochip_setup_dev
    gpiochip_add_data_with_key

gcdev_register() & gcdev_unregister() would call device_add() &
device_del() (no matter CONFIG_GPIO_CDEV is enabled or not) to
register/unregister device.

However, if device_add() succeeds, some resource (like
struct device_private allocated by device_private_init())
is not released by device_del().

Therefore, after device_add() succeeds by gcdev_register(), it
needs to call put_device() to release resource in the error handle
path.

Here we move forward the register of release function, and let it
release every piece of resource by put_device() instead of kfree().

Fixes: 159f3cd92f17 ("gpiolib: Defer gpio device setup until after gpiolib initialization")
Signed-off-by: Zeng Heng <zengheng4@huawei.com>
---
 drivers/gpio/gpiolib.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 4756ea08894f..bb4dedc154b7 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -526,12 +526,13 @@ static int gpiochip_setup_dev(struct gpio_device *gdev)
 	if (ret)
 		return ret;
 
+	/* From this point, the .release() function cleans up gpio_device */
+	gdev->dev.release = gpiodevice_release;
+
 	ret = gpiochip_sysfs_register(gdev);
 	if (ret)
 		goto err_remove_device;
 
-	/* From this point, the .release() function cleans up gpio_device */
-	gdev->dev.release = gpiodevice_release;
 	dev_dbg(&gdev->dev, "registered GPIOs %d to %d on %s\n", gdev->base,
 		gdev->base + gdev->ngpio - 1, gdev->chip->label ? : "generic");
 
@@ -816,6 +817,13 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 err_free_gpiochip_mask:
 	gpiochip_remove_pin_ranges(gc);
 	gpiochip_free_valid_mask(gc);
+	/*
+	 * If gdev->dev.release has been registered by
+	 * gpiochip_setup_dev(), print err msg and
+	 * call put_device() to release all.
+	 */
+	if (gdev->dev.release)
+		goto err_free_gdev;
 err_remove_from_list:
 	spin_lock_irqsave(&gpio_lock, flags);
 	list_del(&gdev->list);
@@ -835,7 +843,10 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 		       gdev->base, gdev->base + gdev->ngpio - 1,
 		       gc->label ? : "generic", ret);
 	}
-	kfree(gdev);
+	if (gdev->dev.release)
+		put_device(&gdev->dev);
+	else
+		kfree(gdev);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(gpiochip_add_data_with_key);
-- 
2.25.1

Re: [PATCH v3] gpiolib: fix memory leak in gpiochip_setup_dev()

[Andy Shevchenko]

On Thu, Nov 17, 2022 at 05:02:47PM +0800, Zeng Heng wrote:
> Here is a report about memory leak detected in gpiochip_setup_dev():
> 
> unreferenced object 0xffff88810b406400 (size 512):
>   comm "python3", pid 1682, jiffies 4295346908 (age 24.090s)
>   backtrace:
>     kmalloc_trace
>     device_add 		device_private_init at drivers/base/core.c:3361

Seems like unneeded space after device_add. Also note, we refer to
the functions as func().

> 			(inlined by) device_add at drivers/base/core.c:3411
>     cdev_device_add
>     gpiolib_cdev_register
>     gpiochip_setup_dev
>     gpiochip_add_data_with_key
> 
> gcdev_register() & gcdev_unregister() would call device_add() &
> device_del() (no matter CONFIG_GPIO_CDEV is enabled or not) to
> register/unregister device.
> 
> However, if device_add() succeeds, some resource (like
> struct device_private allocated by device_private_init())
> is not released by device_del().
> 
> Therefore, after device_add() succeeds by gcdev_register(), it
> needs to call put_device() to release resource in the error handle
> path.
> 
> Here we move forward the register of release function, and let it
> release every piece of resource by put_device() instead of kfree().
> 
> Fixes: 159f3cd92f17 ("gpiolib: Defer gpio device setup until after gpiolib initialization")
> Signed-off-by: Zeng Heng <zengheng4@huawei.com>
> ---

Where is changelog since we see this as v3?

...

>  err_free_gpiochip_mask:
>  	gpiochip_remove_pin_ranges(gc);
>  	gpiochip_free_valid_mask(gc);
> +	/*
> +	 * If gdev->dev.release has been registered by
> +	 * gpiochip_setup_dev(), print err msg and
> +	 * call put_device() to release all.
> +	 */
> +	if (gdev->dev.release)
> +		goto err_free_gdev;

(1)

>  err_remove_from_list:
>  	spin_lock_irqsave(&gpio_lock, flags);
>  	list_del(&gdev->list);

...

> -	kfree(gdev);

> +	if (gdev->dev.release)
> +		put_device(&gdev->dev);

Why you can't do this above at (1)?
Is there any other hidden way to get here with release set?

> +	else
> +		kfree(gdev);
>  	return ret;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] gpiolib: fix memory leak in gpiochip_setup_dev()

[Zeng Heng]

On 2022/11/17 18:49, Andy Shevchenko wrote:
> On Thu, Nov 17, 2022 at 05:02:47PM +0800, Zeng Heng wrote:
>> Here is a report about memory leak detected in gpiochip_setup_dev():
>>
>> unreferenced object 0xffff88810b406400 (size 512):
>>    comm "python3", pid 1682, jiffies 4295346908 (age 24.090s)
>>    backtrace:
>>      kmalloc_trace
>>      device_add 		device_private_init at drivers/base/core.c:3361
> Seems like unneeded space after device_add. Also note, we refer to
> the functions as func().
Just emphasize the location of memory leak happened.
>> 			(inlined by) device_add at drivers/base/core.c:3411
>>      cdev_device_add
>>      gpiolib_cdev_register
>>      gpiochip_setup_dev
>>      gpiochip_add_data_with_key
>>
>> gcdev_register() & gcdev_unregister() would call device_add() &
>> device_del() (no matter CONFIG_GPIO_CDEV is enabled or not) to
>> register/unregister device.
>>
>> However, if device_add() succeeds, some resource (like
>> struct device_private allocated by device_private_init())
>> is not released by device_del().
>>
>> Therefore, after device_add() succeeds by gcdev_register(), it
>> needs to call put_device() to release resource in the error handle
>> path.
>>
>> Here we move forward the register of release function, and let it
>> release every piece of resource by put_device() instead of kfree().
>>
>> Fixes: 159f3cd92f17 ("gpiolib: Defer gpio device setup until after gpiolib initialization")
>> Signed-off-by: Zeng Heng <zengheng4@huawei.com>
>> ---
> Where is changelog since we see this as v3?
>
> ...

change in v3:

     - use put_device() instead of kfree()

>>   err_free_gpiochip_mask:
>>   	gpiochip_remove_pin_ranges(gc);
>>   	gpiochip_free_valid_mask(gc);
>> +	/*
>> +	 * If gdev->dev.release has been registered by
>> +	 * gpiochip_setup_dev(), print err msg and
>> +	 * call put_device() to release all.
>> +	 */
>> +	if (gdev->dev.release)
>> +		goto err_free_gdev;
> (1)
>
>>   err_remove_from_list:
>>   	spin_lock_irqsave(&gpio_lock, flags);
>>   	list_del(&gdev->list);
> ...
>
>> -	kfree(gdev);
>> +	if (gdev->dev.release)
>> +		put_device(&gdev->dev);
> Why you can't do this above at (1)?
> Is there any other hidden way to get here with release set?

As already mentioned in the mail, keep the error print info.

B.R.，

Zeng Heng

>> +	else
>> +		kfree(gdev);
>>   	return ret;

Re: [PATCH v3] gpiolib: fix memory leak in gpiochip_setup_dev()

[Andy Shevchenko]

On Thu, Nov 17, 2022 at 10:12:31PM +0800, Zeng Heng wrote:
> On 2022/11/17 18:49, Andy Shevchenko wrote:
> > On Thu, Nov 17, 2022 at 05:02:47PM +0800, Zeng Heng wrote:

...

> > > +	/*
> > > +	 * If gdev->dev.release has been registered by
> > > +	 * gpiochip_setup_dev(), print err msg and
> > > +	 * call put_device() to release all.
> > > +	 */
> > > +	if (gdev->dev.release)
> > > +		goto err_free_gdev;
> > (1)
> > 
> > >   err_remove_from_list:
> > >   	spin_lock_irqsave(&gpio_lock, flags);
> > >   	list_del(&gdev->list);
> > ...
> > 
> > > -	kfree(gdev);
> > > +	if (gdev->dev.release)
> > > +		put_device(&gdev->dev);
> > Why you can't do this above at (1)?
> > Is there any other hidden way to get here with release set?
> 
> As already mentioned in the mail, keep the error print info.

Can you refactor that to avoid double condition on the ->release() presence?

> > > +	else
> > > +		kfree(gdev);
> > >   	return ret;

-- 
With Best Regards,
Andy Shevchenko

[PATCH v4] gpiolib: fix memory leak in gpiochip_setup_dev()

[Zeng Heng]

Here is a backtrace report about memory leak detected in
gpiochip_setup_dev():

unreferenced object 0xffff88810b406400 (size 512):
  comm "python3", pid 1682, jiffies 4295346908 (age 24.090s)
  backtrace:
    kmalloc_trace
    device_add 		device_private_init at drivers/base/core.c:3361
			(inlined by) device_add at drivers/base/core.c:3411
    cdev_device_add
    gpiolib_cdev_register
    gpiochip_setup_dev
    gpiochip_add_data_with_key

gcdev_register() & gcdev_unregister() would call device_add() &
device_del() (no matter CONFIG_GPIO_CDEV is enabled or not) to
register/unregister device.

However, if device_add() succeeds, some resource (like
struct device_private allocated by device_private_init())
is not released by device_del().

Therefore, after device_add() succeeds by gcdev_register(), it
needs to call put_device() to release resource in the error handle
path.

Here we move forward the register of release function, and let it
release every piece of resource by put_device() instead of kfree().

Fixes: 159f3cd92f17 ("gpiolib: Defer gpio device setup until after gpiolib initialization")
Signed-off-by: Zeng Heng <zengheng4@huawei.com>
---
changes in v4:
  - add gpiochip_print_register_fail()
changes in v3:
  - use put_device() instead of kfree() explicitly
changes in v2:
  - correct fixes tag
---
 drivers/gpio/gpiolib.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 4756ea08894f..c7a55f4f7ef6 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -526,12 +526,13 @@ static int gpiochip_setup_dev(struct gpio_device *gdev)
 	if (ret)
 		return ret;
 
+	/* From this point, the .release() function cleans up gpio_device */
+	gdev->dev.release = gpiodevice_release;
+
 	ret = gpiochip_sysfs_register(gdev);
 	if (ret)
 		goto err_remove_device;
 
-	/* From this point, the .release() function cleans up gpio_device */
-	gdev->dev.release = gpiodevice_release;
 	dev_dbg(&gdev->dev, "registered GPIOs %d to %d on %s\n", gdev->base,
 		gdev->base + gdev->ngpio - 1, gdev->chip->label ? : "generic");
 
@@ -590,6 +591,18 @@ static void gpiochip_setup_devs(void)
 	}
 }
 
+static void gpiochip_print_register_fail(struct gpio_chip *gc,
+					 struct gpio_device *gdev,
+					 const char *func, int ret)
+{
+	/* failures here can mean systems won't boot... */
+	if (ret != -EPROBE_DEFER) {
+		pr_err("%s: GPIOs %d..%d (%s) failed to register, %d\n", func,
+			gdev->base, gdev->base + gdev->ngpio - 1,
+			gc->label ? : "generic", ret);
+	}
+}
+
 int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 			       struct lock_class_key *lock_key,
 			       struct lock_class_key *request_key)
@@ -816,6 +829,12 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 err_free_gpiochip_mask:
 	gpiochip_remove_pin_ranges(gc);
 	gpiochip_free_valid_mask(gc);
+	if (gdev->dev.release) {
+		/* release() has been registered by gpiochip_setup_dev() */
+		gpiochip_print_register_fail(gc, gdev, __func__, ret);
+		put_device(&gdev->dev);
+		return ret;
+	}
 err_remove_from_list:
 	spin_lock_irqsave(&gpio_lock, flags);
 	list_del(&gdev->list);
@@ -829,12 +848,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 err_free_ida:
 	ida_free(&gpio_ida, gdev->id);
 err_free_gdev:
-	/* failures here can mean systems won't boot... */
-	if (ret != -EPROBE_DEFER) {
-		pr_err("%s: GPIOs %d..%d (%s) failed to register, %d\n", __func__,
-		       gdev->base, gdev->base + gdev->ngpio - 1,
-		       gc->label ? : "generic", ret);
-	}
+	gpiochip_print_register_fail(gc, gdev, __func__, ret);
 	kfree(gdev);
 	return ret;
 }
-- 
2.25.1

Re: [PATCH v3] gpiolib: fix memory leak in gpiochip_setup_dev()

[Zeng Heng]

On 2022/11/17 23:31, Andy Shevchenko wrote:
> On Thu, Nov 17, 2022 at 10:12:31PM +0800, Zeng Heng wrote:
>> On 2022/11/17 18:49, Andy Shevchenko wrote:
>>> On Thu, Nov 17, 2022 at 05:02:47PM +0800, Zeng Heng wrote:
> ...
>
>>>> +	/*
>>>> +	 * If gdev->dev.release has been registered by
>>>> +	 * gpiochip_setup_dev(), print err msg and
>>>> +	 * call put_device() to release all.
>>>> +	 */
>>>> +	if (gdev->dev.release)
>>>> +		goto err_free_gdev;
>>> (1)
>>>
>>>>    err_remove_from_list:
>>>>    	spin_lock_irqsave(&gpio_lock, flags);
>>>>    	list_del(&gdev->list);
>>> ...
>>>
>>>> -	kfree(gdev);
>>>> +	if (gdev->dev.release)
>>>> +		put_device(&gdev->dev);
>>> Why you can't do this above at (1)?
>>> Is there any other hidden way to get here with release set?
>> As already mentioned in the mail, keep the error print info.
> Can you refactor that to avoid double condition on the ->release() presence?

Sure.

Zeng Heng

>
>>>> +	else
>>>> +		kfree(gdev);
>>>>    	return ret;

Re: [PATCH v4] gpiolib: fix memory leak in gpiochip_setup_dev()

[Andy Shevchenko]

On Fri, Nov 18, 2022 at 10:22:36AM +0800, Zeng Heng wrote:
> Here is a backtrace report about memory leak detected in
> gpiochip_setup_dev():
> 
> unreferenced object 0xffff88810b406400 (size 512):
>   comm "python3", pid 1682, jiffies 4295346908 (age 24.090s)
>   backtrace:
>     kmalloc_trace
>     device_add 		device_private_init at drivers/base/core.c:3361

You missed my comment here about extra space.

> 			(inlined by) device_add at drivers/base/core.c:3411
>     cdev_device_add
>     gpiolib_cdev_register
>     gpiochip_setup_dev
>     gpiochip_add_data_with_key
> 
> gcdev_register() & gcdev_unregister() would call device_add() &
> device_del() (no matter CONFIG_GPIO_CDEV is enabled or not) to
> register/unregister device.
> 
> However, if device_add() succeeds, some resource (like
> struct device_private allocated by device_private_init())
> is not released by device_del().
> 
> Therefore, after device_add() succeeds by gcdev_register(), it
> needs to call put_device() to release resource in the error handle
> path.
> 
> Here we move forward the register of release function, and let it
> release every piece of resource by put_device() instead of kfree().

...

> +static void gpiochip_print_register_fail(struct gpio_chip *gc,
> +					 struct gpio_device *gdev,
> +					 const char *func, int ret)
> +{
> +	/* failures here can mean systems won't boot... */
> +	if (ret != -EPROBE_DEFER) {

Wouldn't the following work better?

	if (ret == -EPROBE_DEFER)
		return;

	/* failures here can mean systems won't boot... */
	pr_err(...);

> +		pr_err("%s: GPIOs %d..%d (%s) failed to register, %d\n", func,
> +			gdev->base, gdev->base + gdev->ngpio - 1,
> +			gc->label ? : "generic", ret);
> +	}
> +}

...

>  err_free_gpiochip_mask:
>  	gpiochip_remove_pin_ranges(gc);
>  	gpiochip_free_valid_mask(gc);
> +	if (gdev->dev.release) {

> +		/* release() has been registered by gpiochip_setup_dev() */

This comment is most probably in a wrong line and should be one line below.

> +		gpiochip_print_register_fail(gc, gdev, __func__, ret);
> +		put_device(&gdev->dev);
> +		return ret;
> +	}

...

>  err_free_gdev:
> -	/* failures here can mean systems won't boot... */
> -	if (ret != -EPROBE_DEFER) {
> -		pr_err("%s: GPIOs %d..%d (%s) failed to register, %d\n", __func__,
> -		       gdev->base, gdev->base + gdev->ngpio - 1,
> -		       gc->label ? : "generic", ret);
> -	}
> +	gpiochip_print_register_fail(gc, gdev, __func__, ret);
>  	kfree(gdev);
>  	return ret;

Now it looks cleaner, but why you can't use the same return point with the
message? What you need is to neep the range on the stack (which is almost there).

...

Okay, let's leave this for a while, I will think how it can be improved and then
I come up with particular suggestions.

-- 
With Best Regards,
Andy Shevchenko