[PATCH] i2c: fix a potential kmemleak of adapter device

[PATCH] i2c: fix a potential kmemleak of adapter device

[Gu Zheng]

When running with the latest kernel, we get the following kmemleak message:
unreferenced object 0xffff8800c2a36100 (size 256):
  comm "modprobe", pid 629, jiffies 4294676002 (age 1531.115s)
  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 78 21 85 82 ff ff ff ff  ........x!......
  backtrace:
    [<ffffffff815449ce>] kmemleak_alloc+0x73/0x98
    [<ffffffff8113fa2b>] slab_post_alloc_hook+0x28/0x2a
    [<ffffffff81142004>] __kmalloc+0x12c/0x158
    [<ffffffff8134c60b>] kzalloc.constprop.6+0xe/0x10
    [<ffffffff8134cb71>] device_private_init+0x18/0x66
    [<ffffffff8134f1e8>] dev_set_drvdata+0x1e/0x34
    [<ffffffffa01e643e>] i801_probe+0x5d/0x447 [i2c_i801]
    [<ffffffff812a78be>] local_pci_probe+0x41/0x84
    [<ffffffff812a7feb>] pci_device_probe+0xdf/0x10c
    [<ffffffff8134f90d>] driver_probe_device+0x12f/0x2ee
    [<ffffffff8134fb2b>] __driver_attach+0x5f/0x83
    [<ffffffff8134de28>] bus_for_each_dev+0x64/0x90
    [<ffffffff8134f36b>] driver_attach+0x1e/0x20
    [<ffffffff8134ef34>] bus_add_driver+0xf2/0x1f0
    [<ffffffff8135013e>] driver_register+0x8c/0xc3
    [<ffffffff812a7bb4>] __pci_register_driver+0x62/0x67
According to the backtrace, it seems that we do not release the
device_private field of adapter device in the fail path of i801_probe
or the flow of i2c_del_adapter, so add the cleanup step to fix it.

Signed-off-by: Gu Zheng <guz.fnst-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
---
 drivers/i2c/busses/i2c-i801.c |    1 +
 drivers/i2c/i2c-core.c        |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 737e298..4bd4fba 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1246,6 +1246,7 @@ exit_free_irq:
 exit_release:
 	pci_release_region(dev, SMBBAR);
 exit:
+	kfree(priv->adapter.dev.p);
 	kfree(priv);
 	return err;
 }
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index d74c0b3..a7bab9a 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1491,6 +1491,7 @@ void i2c_del_adapter(struct i2c_adapter *adap)
 	idr_remove(&i2c_adapter_idr, adap->nr);
 	mutex_unlock(&core_lock);
 
+	kfree(adap->dev.p);
 	/* Clear the device structure in case this adapter is ever going to be
 	   added again */
 	memset(&adap->dev, 0, sizeof(adap->dev));
-- 
1.7.7

Re: [PATCH] i2c: fix a potential kmemleak of adapter device

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2764 bytes --]

On Wed, Dec 18, 2013 at 09:18:08AM +0800, Gu Zheng wrote:
> When running with the latest kernel, we get the following kmemleak message:
> unreferenced object 0xffff8800c2a36100 (size 256):
>   comm "modprobe", pid 629, jiffies 4294676002 (age 1531.115s)
>   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 78 21 85 82 ff ff ff ff  ........x!......
>   backtrace:
>     [<ffffffff815449ce>] kmemleak_alloc+0x73/0x98
>     [<ffffffff8113fa2b>] slab_post_alloc_hook+0x28/0x2a
>     [<ffffffff81142004>] __kmalloc+0x12c/0x158
>     [<ffffffff8134c60b>] kzalloc.constprop.6+0xe/0x10
>     [<ffffffff8134cb71>] device_private_init+0x18/0x66
>     [<ffffffff8134f1e8>] dev_set_drvdata+0x1e/0x34
>     [<ffffffffa01e643e>] i801_probe+0x5d/0x447 [i2c_i801]
>     [<ffffffff812a78be>] local_pci_probe+0x41/0x84
>     [<ffffffff812a7feb>] pci_device_probe+0xdf/0x10c
>     [<ffffffff8134f90d>] driver_probe_device+0x12f/0x2ee
>     [<ffffffff8134fb2b>] __driver_attach+0x5f/0x83
>     [<ffffffff8134de28>] bus_for_each_dev+0x64/0x90
>     [<ffffffff8134f36b>] driver_attach+0x1e/0x20
>     [<ffffffff8134ef34>] bus_add_driver+0xf2/0x1f0
>     [<ffffffff8135013e>] driver_register+0x8c/0xc3
>     [<ffffffff812a7bb4>] __pci_register_driver+0x62/0x67
> According to the backtrace, it seems that we do not release the
> device_private field of adapter device in the fail path of i801_probe
> or the flow of i2c_del_adapter, so add the cleanup step to fix it.
> 
> Signed-off-by: Gu Zheng <guz.fnst-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>

? Why should we free something we don't have allocated? The fix must be
somewhere else probably...

> ---
>  drivers/i2c/busses/i2c-i801.c |    1 +
>  drivers/i2c/i2c-core.c        |    1 +
>  2 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 737e298..4bd4fba 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1246,6 +1246,7 @@ exit_free_irq:
>  exit_release:
>  	pci_release_region(dev, SMBBAR);
>  exit:
> +	kfree(priv->adapter.dev.p);
>  	kfree(priv);
>  	return err;
>  }
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index d74c0b3..a7bab9a 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -1491,6 +1491,7 @@ void i2c_del_adapter(struct i2c_adapter *adap)
>  	idr_remove(&i2c_adapter_idr, adap->nr);
>  	mutex_unlock(&core_lock);
>  
> +	kfree(adap->dev.p);
>  	/* Clear the device structure in case this adapter is ever going to be
>  	   added again */
>  	memset(&adap->dev, 0, sizeof(adap->dev));
> -- 
> 1.7.7
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] i2c: fix a potential kmemleak of adapter device

[Gu Zheng]

Hi Wolfram,
On 12/21/2013 01:31 AM, Wolfram Sang wrote:

> On Wed, Dec 18, 2013 at 09:18:08AM +0800, Gu Zheng wrote:
>> When running with the latest kernel, we get the following kmemleak message:
>> unreferenced object 0xffff8800c2a36100 (size 256):
>>   comm "modprobe", pid 629, jiffies 4294676002 (age 1531.115s)
>>   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 78 21 85 82 ff ff ff ff  ........x!......
>>   backtrace:
>>     [<ffffffff815449ce>] kmemleak_alloc+0x73/0x98
>>     [<ffffffff8113fa2b>] slab_post_alloc_hook+0x28/0x2a
>>     [<ffffffff81142004>] __kmalloc+0x12c/0x158
>>     [<ffffffff8134c60b>] kzalloc.constprop.6+0xe/0x10
>>     [<ffffffff8134cb71>] device_private_init+0x18/0x66
>>     [<ffffffff8134f1e8>] dev_set_drvdata+0x1e/0x34
>>     [<ffffffffa01e643e>] i801_probe+0x5d/0x447 [i2c_i801]
>>     [<ffffffff812a78be>] local_pci_probe+0x41/0x84
>>     [<ffffffff812a7feb>] pci_device_probe+0xdf/0x10c
>>     [<ffffffff8134f90d>] driver_probe_device+0x12f/0x2ee
>>     [<ffffffff8134fb2b>] __driver_attach+0x5f/0x83
>>     [<ffffffff8134de28>] bus_for_each_dev+0x64/0x90
>>     [<ffffffff8134f36b>] driver_attach+0x1e/0x20
>>     [<ffffffff8134ef34>] bus_add_driver+0xf2/0x1f0
>>     [<ffffffff8135013e>] driver_register+0x8c/0xc3
>>     [<ffffffff812a7bb4>] __pci_register_driver+0x62/0x67
>> According to the backtrace, it seems that we do not release the
>> device_private field of adapter device in the fail path of i801_probe
>> or the flow of i2c_del_adapter, so add the cleanup step to fix it.
>>
>> Signed-off-by: Gu Zheng <guz.fnst-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> 
> ? Why should we free something we don't have allocated? The fix must be
> somewhere else probably...

We do allocate the device_private in device_private_init() when calling
dev_set_drvdata() to set adapter data in probe flow, and the calltrace
also shows this. Am I missing something?

Regards,
Gu
 

> 
>> ---
>>  drivers/i2c/busses/i2c-i801.c |    1 +
>>  drivers/i2c/i2c-core.c        |    1 +
>>  2 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index 737e298..4bd4fba 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -1246,6 +1246,7 @@ exit_free_irq:
>>  exit_release:
>>  	pci_release_region(dev, SMBBAR);
>>  exit:
>> +	kfree(priv->adapter.dev.p);
>>  	kfree(priv);
>>  	return err;
>>  }
>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>> index d74c0b3..a7bab9a 100644
>> --- a/drivers/i2c/i2c-core.c
>> +++ b/drivers/i2c/i2c-core.c
>> @@ -1491,6 +1491,7 @@ void i2c_del_adapter(struct i2c_adapter *adap)
>>  	idr_remove(&i2c_adapter_idr, adap->nr);
>>  	mutex_unlock(&core_lock);
>>  
>> +	kfree(adap->dev.p);
>>  	/* Clear the device structure in case this adapter is ever going to be
>>  	   added again */
>>  	memset(&adap->dev, 0, sizeof(adap->dev));
>> -- 
>> 1.7.7
>>

Re: [PATCH] i2c: fix a potential kmemleak of adapter device

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2421 bytes --]

On Mon, Dec 23, 2013 at 09:47:09AM +0800, Gu Zheng wrote:
> Hi Wolfram,
> On 12/21/2013 01:31 AM, Wolfram Sang wrote:
> 
> > On Wed, Dec 18, 2013 at 09:18:08AM +0800, Gu Zheng wrote:
> >> When running with the latest kernel, we get the following kmemleak message:
> >> unreferenced object 0xffff8800c2a36100 (size 256):
> >>   comm "modprobe", pid 629, jiffies 4294676002 (age 1531.115s)
> >>   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 78 21 85 82 ff ff ff ff  ........x!......
> >>   backtrace:
> >>     [<ffffffff815449ce>] kmemleak_alloc+0x73/0x98
> >>     [<ffffffff8113fa2b>] slab_post_alloc_hook+0x28/0x2a
> >>     [<ffffffff81142004>] __kmalloc+0x12c/0x158
> >>     [<ffffffff8134c60b>] kzalloc.constprop.6+0xe/0x10
> >>     [<ffffffff8134cb71>] device_private_init+0x18/0x66
> >>     [<ffffffff8134f1e8>] dev_set_drvdata+0x1e/0x34
> >>     [<ffffffffa01e643e>] i801_probe+0x5d/0x447 [i2c_i801]
> >>     [<ffffffff812a78be>] local_pci_probe+0x41/0x84
> >>     [<ffffffff812a7feb>] pci_device_probe+0xdf/0x10c
> >>     [<ffffffff8134f90d>] driver_probe_device+0x12f/0x2ee
> >>     [<ffffffff8134fb2b>] __driver_attach+0x5f/0x83
> >>     [<ffffffff8134de28>] bus_for_each_dev+0x64/0x90
> >>     [<ffffffff8134f36b>] driver_attach+0x1e/0x20
> >>     [<ffffffff8134ef34>] bus_add_driver+0xf2/0x1f0
> >>     [<ffffffff8135013e>] driver_register+0x8c/0xc3
> >>     [<ffffffff812a7bb4>] __pci_register_driver+0x62/0x67
> >> According to the backtrace, it seems that we do not release the
> >> device_private field of adapter device in the fail path of i801_probe
> >> or the flow of i2c_del_adapter, so add the cleanup step to fix it.
> >>
> >> Signed-off-by: Gu Zheng <guz.fnst-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> > 
> > ? Why should we free something we don't have allocated? The fix must be
> > somewhere else probably...
> 
> We do allocate the device_private in device_private_init() when calling
> dev_set_drvdata()

dev_set_drvdata only does the following and doesn't call anything:

	dev->driver_data = data;

> to set adapter data in probe flow, and the calltrace
> also shows this. Am I missing something?

It should be freed by the driver core in device_release() again. Dunno
why it does not happen and you are seeing the leak, but the fix is not
correct.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] i2c: fix a potential kmemleak of adapter device

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 168 bytes --]


Okay, there was another thread about it with more details. Still, this
patch addresses the problem on the wrong level. No driver should mess
with driver core basics.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]