i2c: fix a potential kmemleak of adapter device

PATCH In review Gu Zheng 2 participants 5 messages Dec 18, 2013 – Oct 7, 2014

Subsystems (3)

I2C subsystem host drivers I2C I2C/smbus controller drivers for pc

lore

Share & tools

lore

Gu Zheng

author

2013-12-18 01:18 UTC #0 lore raw

PATCH 1/1 i2c: fix a potential kmemleak of adapter device

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 <[email protected]>

---
 drivers/i2c/busses/i2c-i801.c |    1 +
 drivers/i2c/i2c-core.c        |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

drivers/i2c/busses/i2c-i801.c +1 -0

index 737e298..4bd4fba 100644
@@ -1246,6 +1246,7 @@ exit_free_irq:
 exit_release:
 	pci_release_region(dev, SMBBAR);
 exit:
+	kfree(priv->adapter.dev.p);
 	kfree(priv);
 	return err;
 }

drivers/i2c/i2c-core.c +1 -0

index d74c0b3..a7bab9a 100644
@@ -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));

Signature · 1 line

1.7.7

Wolfram Sang

2013-12-20 17:31 UTC #1 lore raw

Quoted 23 lines · Gu Zheng

    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

Quoted 5 lines

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 <[email protected]>

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

Quoted 32 lines

---
 drivers/i2c/busses/i2c-i801.c |    1 +
 drivers/i2c/i2c-core.c        |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

drivers/i2c/busses/i2c-i801.c +1 -0

index 737e298..4bd4fba 100644
@@ -1246,6 +1246,7 @@ exit_free_irq:
 exit_release:
 	pci_release_region(dev, SMBBAR);
 exit:
+	kfree(priv->adapter.dev.p);
 	kfree(priv);
 	return err;
 }

drivers/i2c/i2c-core.c +1 -0

index d74c0b3..a7bab9a 100644
@@ -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));

Signature · 1 line

1.7.7

Gu Zheng

author

2013-12-23 01:47 UTC #2 lore raw

Hi Wolfram,

Quoted 28 lines · Wolfram Sang

Quoted 27 lines · 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.

Quoted 4 lines

Quoted 1 line
Signed-off-by Gu Zheng <[email protected]>
? 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

Quoted 33 lines

---
 drivers/i2c/busses/i2c-i801.c |    1 +
 drivers/i2c/i2c-core.c        |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

drivers/i2c/busses/i2c-i801.c +1 -0

index 737e298..4bd4fba 100644
@@ -1246,6 +1246,7 @@ exit_free_irq:
 exit_release:
 	pci_release_region(dev, SMBBAR);
 exit:
+	kfree(priv->adapter.dev.p);
 	kfree(priv);
 	return err;
 }

drivers/i2c/i2c-core.c +1 -0

index d74c0b3..a7bab9a 100644
@@ -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));

Signature · 1 line

1.7.7

Wolfram Sang

2014-10-07 17:34 UTC #3 lore raw

Quoted 31 lines · Gu Zheng

Hi Wolfram,
Quoted 28 lines · Wolfram Sang
Quoted 27 lines · 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.

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
Signed-off-by Gu Zheng <[email protected]>
? 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;

Quoted 2 lines

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.

Wolfram Sang

2014-10-07 20:23 UTC #4 lore raw

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.