Patchwork i2c: i801: fix memleak on probe error

login
register
mail settings
Submitter Peter Wu
Date Dec. 23, 2013, 9:39 a.m.
Message ID <1387791578-1372-1-git-send-email-lekensteyn@gmail.com>
Download mbox | patch
Permalink /patch/304741/
State Not Applicable
Headers show

Comments

Peter Wu - Dec. 23, 2013, 9:39 a.m.
The driver-specific data for i801 was only set for the device on
success, that led to a memory leak on error paths (for instance, when
there is a resource conflict with ACPI). (The driver core clears the
driver data (if set) if the probe routine fails).

Fix it by setting the driver data right after successful memory
allocation, before reaching any error paths.

References: http://lkml.org/lkml/2013/1/23/191
Reported-by: Martin Mokrejs <mmokrejs@fold.natur.cuni.cz>
Tested-by: Peter Wu <lekensteyn@gmail.com> [ACPI conflict error path]
Signed-off-by: Peter Wu <lekensteyn@gmail.com>
---
Hi Jean,

This memleak issue is still present in v3.13-rc4-256-gb7000ad.
From kmemleak:

unreferenced object 0xffff88022f501a00 (size 256):
  comm "systemd-udevd", pid 209, jiffies 4294896115 (age 2872.520s)
  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 f4 e2 53 82 ff ff ff ff  ..........S.....
  backtrace:
    [<ffffffff815d29ce>] kmemleak_alloc+0x4e/0xb0
    [<ffffffff8116ea5a>] kmem_cache_alloc_trace+0xfa/0x1e0
    [<ffffffff813efc63>] device_private_init+0x23/0x80
    [<ffffffff813f2b49>] dev_set_drvdata+0x39/0x50
    [<ffffffffa0294539>] i801_probe+0x59/0x528 [i2c_i801]
    [<ffffffff81332d95>] local_pci_probe+0x45/0xa0
    [<ffffffff81333be9>] pci_device_probe+0xd9/0x130
    [<ffffffff813f30e7>] driver_probe_device+0x87/0x390
    [<ffffffff813f34c3>] __driver_attach+0x93/0xa0
    [<ffffffff813f102b>] bus_for_each_dev+0x6b/0xb0
    [<ffffffff813f2b0e>] driver_attach+0x1e/0x20
    [<ffffffff813f26e8>] bus_add_driver+0x188/0x260
    [<ffffffff813f3b04>] driver_register+0x64/0xf0
    [<ffffffff81332930>] __pci_register_driver+0x60/0x70
    [<ffffffffa02990af>] 0xffffffffa02990af
    [<ffffffff81000312>] do_one_initcall+0xf2/0x1a0

The dmesg for this laptop also contains a resource conflict message,
just like the reporter (Martin Mokrejs):

    [   15.409772] ACPI Warning: 0x0000000000001840-0x000000000000185f SystemIO conflicts with Region \_SB_.PCI0.SBUS.SMBI 1 (20131115/utaddress-251)
    [   15.413439] ACPI: If an ACPI driver is available for this device, you should use it instead of the native driver

With this patch applied on top of almost 3.13-rc5 (v3.13-rc4-256-gb7000ad),
the memleak is gone.

Regards,
Peter
---
 drivers/i2c/busses/i2c-i801.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
Peter Wu - Dec. 23, 2013, 10:43 a.m.
Nevermind this patch, it does not really fix the memleak because
i2c_set_adapdata() calls dev_set_drvdata() which allocates memory.
(I must have ran kmemleak too early, right after boot it did not
give any warnings, now it does).

RFC: what about dropping i2c_set_adapdata() from the probe function
and replacing i2c_get_adapdata(adapter) by
pci_get_drvdata(adapter->pci_dev) on top of this patch? I am not
sure what the purpose is for i2c_set_adapdata, hence this question.

Regards,
Peter

On Monday 23 December 2013 10:39:38 Peter Wu wrote:
> The driver-specific data for i801 was only set for the device on
> success, that led to a memory leak on error paths (for instance, when
> there is a resource conflict with ACPI). (The driver core clears the
> driver data (if set) if the probe routine fails).
> 
> Fix it by setting the driver data right after successful memory
> allocation, before reaching any error paths.
> 
> References: http://lkml.org/lkml/2013/1/23/191
> Reported-by: Martin Mokrejs <mmokrejs@fold.natur.cuni.cz>
> Tested-by: Peter Wu <lekensteyn@gmail.com> [ACPI conflict error path]
> Signed-off-by: Peter Wu <lekensteyn@gmail.com>
> ---
> Hi Jean,
> 
> This memleak issue is still present in v3.13-rc4-256-gb7000ad.
> From kmemleak:
> 
> unreferenced object 0xffff88022f501a00 (size 256):
>   comm "systemd-udevd", pid 209, jiffies 4294896115 (age 2872.520s)
>   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 f4 e2 53 82 ff ff ff ff  ..........S.....
>   backtrace:
>     [<ffffffff815d29ce>] kmemleak_alloc+0x4e/0xb0
>     [<ffffffff8116ea5a>] kmem_cache_alloc_trace+0xfa/0x1e0
>     [<ffffffff813efc63>] device_private_init+0x23/0x80
>     [<ffffffff813f2b49>] dev_set_drvdata+0x39/0x50
>     [<ffffffffa0294539>] i801_probe+0x59/0x528 [i2c_i801]
>     [<ffffffff81332d95>] local_pci_probe+0x45/0xa0
>     [<ffffffff81333be9>] pci_device_probe+0xd9/0x130
>     [<ffffffff813f30e7>] driver_probe_device+0x87/0x390
>     [<ffffffff813f34c3>] __driver_attach+0x93/0xa0
>     [<ffffffff813f102b>] bus_for_each_dev+0x6b/0xb0
>     [<ffffffff813f2b0e>] driver_attach+0x1e/0x20
>     [<ffffffff813f26e8>] bus_add_driver+0x188/0x260
>     [<ffffffff813f3b04>] driver_register+0x64/0xf0
>     [<ffffffff81332930>] __pci_register_driver+0x60/0x70
>     [<ffffffffa02990af>] 0xffffffffa02990af
>     [<ffffffff81000312>] do_one_initcall+0xf2/0x1a0
> 
> The dmesg for this laptop also contains a resource conflict message,
> just like the reporter (Martin Mokrejs):
> 
>     [   15.409772] ACPI Warning: 0x0000000000001840-0x000000000000185f SystemIO conflicts with Region \_SB_.PCI0.SBUS.SMBI 1 (20131115/utaddress-251)
>     [   15.413439] ACPI: If an ACPI driver is available for this device, you should use it instead of the native driver
> 
> With this patch applied on top of almost 3.13-rc5 (v3.13-rc4-256-gb7000ad),
> the memleak is gone.
> 
> Regards,
> Peter
> ---
>  drivers/i2c/busses/i2c-i801.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 737e298..a7096bf 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1117,6 +1117,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	if (!priv)
>  		return -ENOMEM;
>  
> +	pci_set_drvdata(dev, priv);
>  	i2c_set_adapdata(&priv->adapter, priv);
>  	priv->adapter.owner = THIS_MODULE;
>  	priv->adapter.class = i801_get_adapter_class(priv);
> @@ -1236,8 +1237,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	/* We ignore errors - multiplexing is optional */
>  	i801_add_mux(priv);
>  
> -	pci_set_drvdata(dev, priv);
> -
>  	return 0;
>  
>  exit_free_irq:
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Martin Mokrejs - Dec. 23, 2013, 10:51 a.m.
Thanks for the note, was just compiling a new 3.10.24 kernel to test it.
;-)

So far just booted an old 3.9 kernel and after plugging in an external
USB3 drive I got the message, just to be sure I am still able to reproduce
the error and that I have the right .config in the running kernel.

Will wait for another fix instead.
Martin

Peter Wu wrote:
> Nevermind this patch, it does not really fix the memleak because
> i2c_set_adapdata() calls dev_set_drvdata() which allocates memory.
> (I must have ran kmemleak too early, right after boot it did not
> give any warnings, now it does).
>
> RFC: what about dropping i2c_set_adapdata() from the probe function
> and replacing i2c_get_adapdata(adapter) by
> pci_get_drvdata(adapter->pci_dev) on top of this patch? I am not
> sure what the purpose is for i2c_set_adapdata, hence this question.
>
> Regards,
> Peter
>
> On Monday 23 December 2013 10:39:38 Peter Wu wrote:
>> The driver-specific data for i801 was only set for the device on
>> success, that led to a memory leak on error paths (for instance, when
>> there is a resource conflict with ACPI). (The driver core clears the
>> driver data (if set) if the probe routine fails).
>>
>> Fix it by setting the driver data right after successful memory
>> allocation, before reaching any error paths.
>>
>> References: http://lkml.org/lkml/2013/1/23/191
>> Reported-by: Martin Mokrejs <mmokrejs@fold.natur.cuni.cz>
>> Tested-by: Peter Wu <lekensteyn@gmail.com> [ACPI conflict error path]
>> Signed-off-by: Peter Wu <lekensteyn@gmail.com>
>> ---
>> Hi Jean,
>>
>> This memleak issue is still present in v3.13-rc4-256-gb7000ad.
>>  From kmemleak:
>>
>> unreferenced object 0xffff88022f501a00 (size 256):
>>    comm "systemd-udevd", pid 209, jiffies 4294896115 (age 2872.520s)
>>    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 f4 e2 53 82 ff ff ff ff  ..........S.....
>>    backtrace:
>>      [<ffffffff815d29ce>] kmemleak_alloc+0x4e/0xb0
>>      [<ffffffff8116ea5a>] kmem_cache_alloc_trace+0xfa/0x1e0
>>      [<ffffffff813efc63>] device_private_init+0x23/0x80
>>      [<ffffffff813f2b49>] dev_set_drvdata+0x39/0x50
>>      [<ffffffffa0294539>] i801_probe+0x59/0x528 [i2c_i801]
>>      [<ffffffff81332d95>] local_pci_probe+0x45/0xa0
>>      [<ffffffff81333be9>] pci_device_probe+0xd9/0x130
>>      [<ffffffff813f30e7>] driver_probe_device+0x87/0x390
>>      [<ffffffff813f34c3>] __driver_attach+0x93/0xa0
>>      [<ffffffff813f102b>] bus_for_each_dev+0x6b/0xb0
>>      [<ffffffff813f2b0e>] driver_attach+0x1e/0x20
>>      [<ffffffff813f26e8>] bus_add_driver+0x188/0x260
>>      [<ffffffff813f3b04>] driver_register+0x64/0xf0
>>      [<ffffffff81332930>] __pci_register_driver+0x60/0x70
>>      [<ffffffffa02990af>] 0xffffffffa02990af
>>      [<ffffffff81000312>] do_one_initcall+0xf2/0x1a0
>>
>> The dmesg for this laptop also contains a resource conflict message,
>> just like the reporter (Martin Mokrejs):
>>
>>      [   15.409772] ACPI Warning: 0x0000000000001840-0x000000000000185f SystemIO conflicts with Region \_SB_.PCI0.SBUS.SMBI 1 (20131115/utaddress-251)
>>      [   15.413439] ACPI: If an ACPI driver is available for this device, you should use it instead of the native driver
>>
>> With this patch applied on top of almost 3.13-rc5 (v3.13-rc4-256-gb7000ad),
>> the memleak is gone.
>>
>> Regards,
>> Peter
>> ---
>>   drivers/i2c/busses/i2c-i801.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index 737e298..a7096bf 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -1117,6 +1117,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>   	if (!priv)
>>   		return -ENOMEM;
>>
>> +	pci_set_drvdata(dev, priv);
>>   	i2c_set_adapdata(&priv->adapter, priv);
>>   	priv->adapter.owner = THIS_MODULE;
>>   	priv->adapter.class = i801_get_adapter_class(priv);
>> @@ -1236,8 +1237,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>   	/* We ignore errors - multiplexing is optional */
>>   	i801_add_mux(priv);
>>
>> -	pci_set_drvdata(dev, priv);
>> -
>>   	return 0;
>>
>>   exit_free_irq:
>>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Wu - Dec. 23, 2013, 3:49 p.m.
On Monday 23 December 2013 11:51:12 Martin Mokrejs wrote:
> Thanks for the note, was just compiling a new 3.10.24 kernel to test it.
> ;-)
> 
> So far just booted an old 3.9 kernel and after plugging in an external
> USB3 drive I got the message, just to be sure I am still able to reproduce
> the error and that I have the right .config in the running kernel.
> 
> Will wait for another fix instead.
> Martin

There is still one thing I do not fully understand, how should
dev_set_drvdata and dev_get_drvdata be used? For the devices passed
to probe functions, the core takes care of setting to NULL on error.
Then device_unregister frees the memory, right?

Now, what if the dev_set_drvdata (or aliases such as pci_set_drvdata,
i2c_set_adapinfo, etc.) are manually called outside probe functions?
Or inside the probe function, but not for the device that is being
probed (such as is the case with the i801 i2c driver)?

The VFIO driver also does something odd, it clears the driver data,
but the device holding it is freed using kfree():

    static void vfio_device_release(struct kref *kref) {
        struct vfio_device *device = container_of(kref,
                                                  struct vfio_device, kref);
        struct vfio_group *group = device->group;

        list_del(&device->group_next);
        mutex_unlock(&group->device_lock);

        dev_set_drvdata(device->dev, NULL);

        kfree(device);

Is a memory leak also present here since dev_set_drvdata() always tries to
allocate memory?

Regards,
Peter

> Peter Wu wrote:
> > Nevermind this patch, it does not really fix the memleak because
> > i2c_set_adapdata() calls dev_set_drvdata() which allocates memory.
> > (I must have ran kmemleak too early, right after boot it did not
> > give any warnings, now it does).
> >
> > RFC: what about dropping i2c_set_adapdata() from the probe function
> > and replacing i2c_get_adapdata(adapter) by
> > pci_get_drvdata(adapter->pci_dev) on top of this patch? I am not
> > sure what the purpose is for i2c_set_adapdata, hence this question.
> >
> > Regards,
> > Peter
> >
> > On Monday 23 December 2013 10:39:38 Peter Wu wrote:
> >> The driver-specific data for i801 was only set for the device on
> >> success, that led to a memory leak on error paths (for instance, when
> >> there is a resource conflict with ACPI). (The driver core clears the
> >> driver data (if set) if the probe routine fails).
> >>
> >> Fix it by setting the driver data right after successful memory
> >> allocation, before reaching any error paths.
> >>
> >> References: http://lkml.org/lkml/2013/1/23/191
> >> Reported-by: Martin Mokrejs <mmokrejs@fold.natur.cuni.cz>
> >> Tested-by: Peter Wu <lekensteyn@gmail.com> [ACPI conflict error path]
> >> Signed-off-by: Peter Wu <lekensteyn@gmail.com>
> >> ---
> >> Hi Jean,
> >>
> >> This memleak issue is still present in v3.13-rc4-256-gb7000ad.
> >>  From kmemleak:
> >>
> >> unreferenced object 0xffff88022f501a00 (size 256):
> >>    comm "systemd-udevd", pid 209, jiffies 4294896115 (age 2872.520s)
> >>    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 f4 e2 53 82 ff ff ff ff  ..........S.....
> >>    backtrace:
> >>      [<ffffffff815d29ce>] kmemleak_alloc+0x4e/0xb0
> >>      [<ffffffff8116ea5a>] kmem_cache_alloc_trace+0xfa/0x1e0
> >>      [<ffffffff813efc63>] device_private_init+0x23/0x80
> >>      [<ffffffff813f2b49>] dev_set_drvdata+0x39/0x50
> >>      [<ffffffffa0294539>] i801_probe+0x59/0x528 [i2c_i801]
> >>      [<ffffffff81332d95>] local_pci_probe+0x45/0xa0
> >>      [<ffffffff81333be9>] pci_device_probe+0xd9/0x130
> >>      [<ffffffff813f30e7>] driver_probe_device+0x87/0x390
> >>      [<ffffffff813f34c3>] __driver_attach+0x93/0xa0
> >>      [<ffffffff813f102b>] bus_for_each_dev+0x6b/0xb0
> >>      [<ffffffff813f2b0e>] driver_attach+0x1e/0x20
> >>      [<ffffffff813f26e8>] bus_add_driver+0x188/0x260
> >>      [<ffffffff813f3b04>] driver_register+0x64/0xf0
> >>      [<ffffffff81332930>] __pci_register_driver+0x60/0x70
> >>      [<ffffffffa02990af>] 0xffffffffa02990af
> >>      [<ffffffff81000312>] do_one_initcall+0xf2/0x1a0
> >>
> >> The dmesg for this laptop also contains a resource conflict message,
> >> just like the reporter (Martin Mokrejs):
> >>
> >>      [   15.409772] ACPI Warning: 0x0000000000001840-0x000000000000185f SystemIO conflicts with Region \_SB_.PCI0.SBUS.SMBI 1 (20131115/utaddress-251)
> >>      [   15.413439] ACPI: If an ACPI driver is available for this device, you should use it instead of the native driver
> >>
> >> With this patch applied on top of almost 3.13-rc5 (v3.13-rc4-256-gb7000ad),
> >> the memleak is gone.
> >>
> >> Regards,
> >> Peter
> >> ---
> >>   drivers/i2c/busses/i2c-i801.c | 3 +--
> >>   1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> >> index 737e298..a7096bf 100644
> >> --- a/drivers/i2c/busses/i2c-i801.c
> >> +++ b/drivers/i2c/busses/i2c-i801.c
> >> @@ -1117,6 +1117,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
> >>   	if (!priv)
> >>   		return -ENOMEM;
> >>
> >> +	pci_set_drvdata(dev, priv);
> >>   	i2c_set_adapdata(&priv->adapter, priv);
> >>   	priv->adapter.owner = THIS_MODULE;
> >>   	priv->adapter.class = i801_get_adapter_class(priv);
> >> @@ -1236,8 +1237,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
> >>   	/* We ignore errors - multiplexing is optional */
> >>   	i801_add_mux(priv);
> >>
> >> -	pci_set_drvdata(dev, priv);
> >> -
> >>   	return 0;
> >>
> >>   exit_free_irq:
> >>
> >
> >

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alex Williamson - Dec. 23, 2013, 5:37 p.m.
On Mon, 2013-12-23 at 16:49 +0100, Peter Wu wrote:
> On Monday 23 December 2013 11:51:12 Martin Mokrejs wrote:
> > Thanks for the note, was just compiling a new 3.10.24 kernel to test it.
> > ;-)
> > 
> > So far just booted an old 3.9 kernel and after plugging in an external
> > USB3 drive I got the message, just to be sure I am still able to reproduce
> > the error and that I have the right .config in the running kernel.
> > 
> > Will wait for another fix instead.
> > Martin
> 
> There is still one thing I do not fully understand, how should
> dev_set_drvdata and dev_get_drvdata be used? For the devices passed
> to probe functions, the core takes care of setting to NULL on error.
> Then device_unregister frees the memory, right?
> 
> Now, what if the dev_set_drvdata (or aliases such as pci_set_drvdata,
> i2c_set_adapinfo, etc.) are manually called outside probe functions?
> Or inside the probe function, but not for the device that is being
> probed (such as is the case with the i801 i2c driver)?
> 
> The VFIO driver also does something odd, it clears the driver data,
> but the device holding it is freed using kfree():
> 
>     static void vfio_device_release(struct kref *kref) {
>         struct vfio_device *device = container_of(kref,
>                                                   struct vfio_device, kref);
>         struct vfio_group *group = device->group;
> 
>         list_del(&device->group_next);
>         mutex_unlock(&group->device_lock);
> 
>         dev_set_drvdata(device->dev, NULL);
> 
>         kfree(device);
> 
> Is a memory leak also present here since dev_set_drvdata() always tries to
> allocate memory?

But it doesn't:

int dev_set_drvdata(struct device *dev, void *data)
{
        int error;

        if (!dev->p) {
                error = device_private_init(dev);
                if (error)
                        return error;
        }
        dev->p->driver_data = data;
        return 0;
}

Also, the code referenced is kfree'ing a struct vfio_device, not the
struct device.  VFIO uses the drvdata to provide a back pointer to the
vfio specific structure, which also includes a pointer to the struct
device.  We obviously want to clear drvdata when the vfio specific
structure is being released.  Thanks,

Alex

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Wu - Dec. 24, 2013, 12:18 a.m.
On Monday 23 December 2013 10:37:21 Alex Williamson wrote:
> On Mon, 2013-12-23 at 16:49 +0100, Peter Wu wrote:
[..]
> > 
> > There is still one thing I do not fully understand, how should
> > dev_set_drvdata and dev_get_drvdata be used? For the devices passed
> > to probe functions, the core takes care of setting to NULL on error.
> > Then device_unregister frees the memory, right?
> > 
> > Now, what if the dev_set_drvdata (or aliases such as pci_set_drvdata,
> > i2c_set_adapinfo, etc.) are manually called outside probe functions?
> > Or inside the probe function, but not for the device that is being
> > probed (such as is the case with the i801 i2c driver)?
> > 
> > The VFIO driver also does something odd, it clears the driver data,
> > but the device holding it is freed using kfree():
> > 
> >     static void vfio_device_release(struct kref *kref) {
> >         struct vfio_device *device = container_of(kref,
> >                                                   struct vfio_device, kref);
> >         struct vfio_group *group = device->group;
> > 
> >         list_del(&device->group_next);
> >         mutex_unlock(&group->device_lock);
> > 
> >         dev_set_drvdata(device->dev, NULL);
> > 
> >         kfree(device);
> > 
> > Is a memory leak also present here since dev_set_drvdata() always tries to
> > allocate memory?
> 
> But it doesn't:
> 
> int dev_set_drvdata(struct device *dev, void *data)
> {
>         int error;
> 
>         if (!dev->p) {
>                 error = device_private_init(dev);
>                 if (error)
>                         return error;
>         }
>         dev->p->driver_data = data;
>         return 0;
> }

It does:

int device_private_init(struct device *dev)
{
        dev->p = kzalloc(sizeof(*dev->p), GFP_KERNEL);
        if (!dev->p)
                return -ENOMEM;
        dev->p->device = dev; 
        klist_init(&dev->p->klist_children, klist_children_get,
                   klist_children_put);
        INIT_LIST_HEAD(&dev->p->deferred_probe);
        return 0;
}

and if it doesn't, then I must be missing something in this non-obvious
code. I scanned the interwebs and Documentation/, but could not really
find a great example on how this is supposed to work. The dev_set_drvdata
function existed since Linus moved to git.

> Also, the code referenced is kfree'ing a struct vfio_device, not the
> struct device.  VFIO uses the drvdata to provide a back pointer to the
> vfio specific structure, which also includes a pointer to the struct
> device.  We obviously want to clear drvdata when the vfio specific
> structure is being released.

Ah, I see. "device->dev" is not freed, but "device". And the data is
cleared for "device->dev". Thanks for correcting.

Clear examples of how to use dev_{s,g}et_drvdata correctly in i2c is
still wanted. I stepped in it yesterday, i2c seems to have its own
way to register new devices. More specifically, how can the memory
associated with dev_set_drvdata be free'd on error paths if the
device is not registered with device_register (as is done in the
probe function of the i801 i2c driver)?

Regards,
Peter

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alex Williamson - Dec. 24, 2013, 1:51 a.m.
On Tue, 2013-12-24 at 01:18 +0100, Peter Wu wrote:
> On Monday 23 December 2013 10:37:21 Alex Williamson wrote:
> > On Mon, 2013-12-23 at 16:49 +0100, Peter Wu wrote:
> [..]
> > > 
> > > There is still one thing I do not fully understand, how should
> > > dev_set_drvdata and dev_get_drvdata be used? For the devices passed
> > > to probe functions, the core takes care of setting to NULL on error.
> > > Then device_unregister frees the memory, right?
> > > 
> > > Now, what if the dev_set_drvdata (or aliases such as pci_set_drvdata,
> > > i2c_set_adapinfo, etc.) are manually called outside probe functions?
> > > Or inside the probe function, but not for the device that is being
> > > probed (such as is the case with the i801 i2c driver)?
> > > 
> > > The VFIO driver also does something odd, it clears the driver data,
> > > but the device holding it is freed using kfree():
> > > 
> > >     static void vfio_device_release(struct kref *kref) {
> > >         struct vfio_device *device = container_of(kref,
> > >                                                   struct vfio_device, kref);
> > >         struct vfio_group *group = device->group;
> > > 
> > >         list_del(&device->group_next);
> > >         mutex_unlock(&group->device_lock);
> > > 
> > >         dev_set_drvdata(device->dev, NULL);
> > > 
> > >         kfree(device);
> > > 
> > > Is a memory leak also present here since dev_set_drvdata() always tries to
> > > allocate memory?
> > 
> > But it doesn't:
> > 
> > int dev_set_drvdata(struct device *dev, void *data)
> > {
> >         int error;
> > 
> >         if (!dev->p) {
> >                 error = device_private_init(dev);
> >                 if (error)
> >                         return error;
> >         }
> >         dev->p->driver_data = data;
> >         return 0;
> > }
> 
> It does:
> 
> int device_private_init(struct device *dev)
> {
>         dev->p = kzalloc(sizeof(*dev->p), GFP_KERNEL);
>         if (!dev->p)
>                 return -ENOMEM;
>         dev->p->device = dev; 
>         klist_init(&dev->p->klist_children, klist_children_get,
>                    klist_children_put);
>         INIT_LIST_HEAD(&dev->p->deferred_probe);
>         return 0;
> }
> 
> and if it doesn't, then I must be missing something in this non-obvious
> code. I scanned the interwebs and Documentation/, but could not really
> find a great example on how this is supposed to work. The dev_set_drvdata
> function existed since Linus moved to git.

You're missing that device_private_init() is only called if (!dev->p).
It's a one time initializer and after that we only set the driver_data.

> > Also, the code referenced is kfree'ing a struct vfio_device, not the
> > struct device.  VFIO uses the drvdata to provide a back pointer to the
> > vfio specific structure, which also includes a pointer to the struct
> > device.  We obviously want to clear drvdata when the vfio specific
> > structure is being released.
> 
> Ah, I see. "device->dev" is not freed, but "device". And the data is
> cleared for "device->dev". Thanks for correcting.
> 
> Clear examples of how to use dev_{s,g}et_drvdata correctly in i2c is
> still wanted. I stepped in it yesterday, i2c seems to have its own
> way to register new devices. More specifically, how can the memory
> associated with dev_set_drvdata be free'd on error paths if the
> device is not registered with device_register (as is done in the
> probe function of the i801 i2c driver)?
> 
> Regards,
> Peter
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/


--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Wu - Dec. 24, 2013, 9:44 a.m.
On Monday 23 December 2013 18:51:09 Alex Williamson wrote:
> > It does:
> > 
> > int device_private_init(struct device *dev)
> > {
> >         dev->p = kzalloc(sizeof(*dev->p), GFP_KERNEL);
> >         if (!dev->p)
> >                 return -ENOMEM;
> >         dev->p->device = dev; 
> >         klist_init(&dev->p->klist_children, klist_children_get,
> >                    klist_children_put);
> >         INIT_LIST_HEAD(&dev->p->deferred_probe);
> >         return 0;
> > }
> > 
> > and if it doesn't, then I must be missing something in this non-obvious
> > code. I scanned the interwebs and Documentation/, but could not really
> > find a great example on how this is supposed to work. The dev_set_drvdata
> > function existed since Linus moved to git.
> 
> You're missing that device_private_init() is only called if (!dev->p).
> It's a one time initializer and after that we only set the driver_data.

Yes I am aware of that, the question is about the memory that got allocated
in vfio_group_create_device().

I think I have found the answer. The "dev" device is passed from the PCI
layer, that will probably take care of freeing the resources when the
device is gone. Is this view correct?

Thanks for your replies!

Peter

> > > Also, the code referenced is kfree'ing a struct vfio_device, not the
> > > struct device.  VFIO uses the drvdata to provide a back pointer to the
> > > vfio specific structure, which also includes a pointer to the struct
> > > device.  We obviously want to clear drvdata when the vfio specific
> > > structure is being released.
> > 
> > Ah, I see. "device->dev" is not freed, but "device". And the data is
> > cleared for "device->dev". Thanks for correcting.
> > 
> > Clear examples of how to use dev_{s,g}et_drvdata correctly in i2c is
> > still wanted. I stepped in it yesterday, i2c seems to have its own
> > way to register new devices. More specifically, how can the memory
> > associated with dev_set_drvdata be free'd on error paths if the
> > device is not registered with device_register (as is done in the
> > probe function of the i801 i2c driver)?

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jean Delvare - Jan. 8, 2014, 9:05 a.m.
Hi Peter,

Joining the discussion late as I was on vacation...

On Mon, 23 Dec 2013 11:43:21 +0100, Peter Wu wrote:
> Nevermind this patch, it does not really fix the memleak because
> i2c_set_adapdata() calls dev_set_drvdata() which allocates memory.
> (I must have ran kmemleak too early, right after boot it did not
> give any warnings, now it does).

I can confirm that your proposed patch doesn't fix anything. It makes
no difference if pci_set_drvdata(dev, priv) is called earlier or later.

> RFC: what about dropping i2c_set_adapdata() from the probe function
> and replacing i2c_get_adapdata(adapter) by
> pci_get_drvdata(adapter->pci_dev) on top of this patch? I am not
> sure what the purpose is for i2c_set_adapdata, hence this question.

The purpose of i2c_set_adapdata is to attach a data structure to a
specific i2c_adapter device. In the case of the i2c-i801 driver,
there's only one such (class) device per PCI device so we could indeed
reuse the PCI device data pointer as you suggest above. But in the
general case there can be several i2c_adapter devices and each needs
its own data.

Also for performance reasons we don't want to do that because
pci_get_drvdata(adapter->pci_dev) is slower than
i2c_get_adapdata(adapter) (due to the extra pointer deference.) As this
happens repeatedly at run-time (in function i801_access) we want it to
be as fast as possible.

Note that we could (ab)use i2c_adapter.algo_data to store the
i2c_adapter device-specific data. Some drivers are doing exactly that,
for example i2c-nforce2. I suspect this legacy field is now somewhat
redundant with i2c_set_adapdata() as I couldn't find any i2c bus
driver using both.

Patch

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 737e298..a7096bf 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1117,6 +1117,7 @@  static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	if (!priv)
 		return -ENOMEM;
 
+	pci_set_drvdata(dev, priv);
 	i2c_set_adapdata(&priv->adapter, priv);
 	priv->adapter.owner = THIS_MODULE;
 	priv->adapter.class = i801_get_adapter_class(priv);
@@ -1236,8 +1237,6 @@  static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	/* We ignore errors - multiplexing is optional */
 	i801_add_mux(priv);
 
-	pci_set_drvdata(dev, priv);
-
 	return 0;
 
 exit_free_irq: