Hi,
I get a couple kmemleak reports like below which I think happen on the
failure path (-ENOENT) of a request_firmware() call:
unreferenced object 0xc355fdb0 (size 256):
comm "NetworkManager", pid 2606, jiffies 4294902882
backtrace:
[<c01e0c3a>] create_object+0xfa/0x250
[<c01e1e7d>] kmemleak_alloc+0x5d/0x70
[<c01dac1b>] kmem_cache_alloc+0x14b/0x190
[<c03a0c4c>] _request_firmware+0x11c/0x530
[<c03a1102>] request_firmware+0x12/0x20
[<f95f6591>] iwl_mac_start+0xa1/0x850 [iwlagn]
[<f8fb08c1>] ieee80211_open+0x2e1/0x860 [mac80211]
[<c048459a>] dev_open+0xba/0x100
[<c0483ab9>] dev_change_flags+0x139/0x1d0
[<c048d392>] do_setlink+0x282/0x410
[<c048ea81>] rtnl_setlink+0xf1/0x130
[<c048e285>] rtnetlink_rcv_msg+0x165/0x200
[<c049fac6>] netlink_rcv_skb+0x76/0xa0
[<c048e10e>] rtnetlink_rcv+0x1e/0x30
[<c049f7fb>] netlink_unicast+0x23b/0x250
[<c04a02db>] netlink_sendmsg+0x1db/0x2d0
The f_dev in _request_firmware() is allocated via the fw_setup_device()
and fw_register_device() calls and its class set to firmware_class (the
class release function is fw_dev_release).
Commit 6acf70f078ca replaced the kfree(dev) in fw_dev_release() with a
put_device() call but my understanding is that the release function is
called via put_device -> kobject_put -> kref_put -> koject_release etc.
and it should call kfree since it's the last to see this device
structure alive.
Because of that, the _request_firmware() function on its -ENOENT error
path only calls device_unregister(f_dev) which would eventually call
fw_dev_release() but there is no kfree (the subsequent put_device call
would just make the kref negative).
The patch below may fix the problem but it's only later tonight that I
can test it and confirm:
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index ddeb819..12e6e64 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -357,7 +357,7 @@ static void fw_dev_release(struct device *dev)
kfree(fw_priv->pages);
kfree(fw_priv->fw_id);
kfree(fw_priv);
- put_device(dev);
+ kfree(dev);
module_put(THIS_MODULE);
}
@@ -407,14 +407,13 @@ static int fw_register_device(struct device **dev_p, const char *fw_name,
retval = device_register(f_dev);
if (retval) {
dev_err(device, "%s: device_register failed\n", __func__);
+ kfree(fw_priv->fw_id);
put_device(f_dev);
- goto error_kfree_fw_id;
+ return retval;
}
*dev_p = f_dev;
return 0;
-error_kfree_fw_id:
- kfree(fw_priv->fw_id);
error_kfree:
kfree(f_dev);
kfree(fw_priv);
--
Catalin
On Tue, 07 Jul 2009 16:17:00 +0100,
Catalin Marinas <[email protected]> wrote:
> The patch below may fix the problem but it's only later tonight that I
> can test it and confirm:
Your patch looks fine to me (didn't test it either), just one minor nit:
>
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index ddeb819..12e6e64 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -357,7 +357,7 @@ static void fw_dev_release(struct device *dev)
> kfree(fw_priv->pages);
> kfree(fw_priv->fw_id);
> kfree(fw_priv);
> - put_device(dev);
> + kfree(dev);
>
> module_put(THIS_MODULE);
> }
> @@ -407,14 +407,13 @@ static int fw_register_device(struct device **dev_p, const char *fw_name,
> retval = device_register(f_dev);
> if (retval) {
> dev_err(device, "%s: device_register failed\n", __func__);
> + kfree(fw_priv->fw_id);
fw_priv->fw_id will be freed in the release function, so you don't need
to free it here.
> put_device(f_dev);
> - goto error_kfree_fw_id;
> + return retval;
> }
> *dev_p = f_dev;
> return 0;
>
> -error_kfree_fw_id:
> - kfree(fw_priv->fw_id);
> error_kfree:
> kfree(f_dev);
> kfree(fw_priv);
>
>
On Tue, 2009-07-07 at 19:01 +0200, Cornelia Huck wrote:
> On Tue, 07 Jul 2009 16:17:00 +0100,
> Catalin Marinas <[email protected]> wrote:
>
> > The patch below may fix the problem but it's only later tonight that I
> > can test it and confirm:
>
> Your patch looks fine to me (didn't test it either), just one minor nit:
I tested it and it solves this leak.
> > @@ -407,14 +407,13 @@ static int fw_register_device(struct device **dev_p, const char *fw_name,
> > retval = device_register(f_dev);
> > if (retval) {
> > dev_err(device, "%s: device_register failed\n", __func__);
> > + kfree(fw_priv->fw_id);
>
> fw_priv->fw_id will be freed in the release function, so you don't need
> to free it here.
OK, thanks.
There is one more leak in this area which I couldn't figure out where it
should be freed:
unreferenced object 0xc353e530 (size 512):
comm "cat", pid 3130, jiffies 4294903232
backtrace:
[<c01e6f6a>] create_object+0xfa/0x250
[<c01e753d>] kmemleak_alloc+0x5d/0x70
[<c01e223d>] __kmalloc+0x10d/0x210
[<c03b2d2f>] firmware_data_write+0x1df/0x270
[<c024163a>] write+0x13a/0x1b0
[<c01eae1c>] vfs_write+0x9c/0x190
[<c01eafcd>] sys_write+0x3d/0x70
[<c010319c>] sysenter_do_call+0x12/0x38
[<ffffffff>] 0xffffffff
Any idea? It looks like this is the kmalloc() in fw_realloc_buffer()
(inlined in firmware_data_write).
--
Catalin
2009/7/7 Catalin Marinas <[email protected]>:
> Hi,
>
> I get a couple kmemleak reports like below which I think happen on the
> failure path (-ENOENT) of a request_firmware() call:
>
> unreferenced object 0xc355fdb0 (size 256):
> ?comm "NetworkManager", pid 2606, jiffies 4294902882
> ?backtrace:
> ? ?[<c01e0c3a>] create_object+0xfa/0x250
> ? ?[<c01e1e7d>] kmemleak_alloc+0x5d/0x70
> ? ?[<c01dac1b>] kmem_cache_alloc+0x14b/0x190
> ? ?[<c03a0c4c>] _request_firmware+0x11c/0x530
> ? ?[<c03a1102>] request_firmware+0x12/0x20
> ? ?[<f95f6591>] iwl_mac_start+0xa1/0x850 [iwlagn]
> ? ?[<f8fb08c1>] ieee80211_open+0x2e1/0x860 [mac80211]
> ? ?[<c048459a>] dev_open+0xba/0x100
> ? ?[<c0483ab9>] dev_change_flags+0x139/0x1d0
> ? ?[<c048d392>] do_setlink+0x282/0x410
> ? ?[<c048ea81>] rtnl_setlink+0xf1/0x130
> ? ?[<c048e285>] rtnetlink_rcv_msg+0x165/0x200
> ? ?[<c049fac6>] netlink_rcv_skb+0x76/0xa0
> ? ?[<c048e10e>] rtnetlink_rcv+0x1e/0x30
> ? ?[<c049f7fb>] netlink_unicast+0x23b/0x250
> ? ?[<c04a02db>] netlink_sendmsg+0x1db/0x2d0
>
> The f_dev in _request_firmware() is allocated via the fw_setup_device()
> and fw_register_device() calls and its class set to firmware_class (the
> class release function is fw_dev_release).
>
> Commit 6acf70f078ca replaced the kfree(dev) in fw_dev_release() with a
> put_device() call but my understanding is that the release function is
> called via put_device -> kobject_put -> kref_put -> koject_release etc.
> and it should call kfree since it's the last to see this device
> structure alive.
>
> Because of that, the _request_firmware() function on its -ENOENT error
> path only calls device_unregister(f_dev) which would eventually call
> fw_dev_release() but there is no kfree (the subsequent put_device call
> would just make the kref negative).
>
> The patch below may fix the problem but it's only later tonight that I
> can test it and confirm:
>
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index ddeb819..12e6e64 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -357,7 +357,7 @@ static void fw_dev_release(struct device *dev)
> ? ? ? ?kfree(fw_priv->pages);
> ? ? ? ?kfree(fw_priv->fw_id);
> ? ? ? ?kfree(fw_priv);
> - ? ? ? put_device(dev);
> + ? ? ? kfree(dev);
Ackd-by: Ming Lei <[email protected]>
Thanks.
>
> ? ? ? ?module_put(THIS_MODULE);
> ?}
> @@ -407,14 +407,13 @@ static int fw_register_device(struct device **dev_p, const char *fw_name,
> ? ? ? ?retval = device_register(f_dev);
> ? ? ? ?if (retval) {
> ? ? ? ? ? ? ? ?dev_err(device, "%s: device_register failed\n", __func__);
> + ? ? ? ? ? ? ? kfree(fw_priv->fw_id);
> ? ? ? ? ? ? ? ?put_device(f_dev);
> - ? ? ? ? ? ? ? goto error_kfree_fw_id;
> + ? ? ? ? ? ? ? return retval;
> ? ? ? ?}
> ? ? ? ?*dev_p = f_dev;
> ? ? ? ?return 0;
>
> -error_kfree_fw_id:
> - ? ? ? kfree(fw_priv->fw_id);
> ?error_kfree:
> ? ? ? ?kfree(f_dev);
> ? ? ? ?kfree(fw_priv);
>
>
> --
> Catalin
>
>
--
Lei Ming
2009/7/8 Catalin Marinas <[email protected]>:
> On Tue, 2009-07-07 at 19:01 +0200, Cornelia Huck wrote:
>
> There is one more leak in this area which I couldn't figure out where it
> should be freed:
>
> unreferenced object 0xc353e530 (size 512):
> ?comm "cat", pid 3130, jiffies 4294903232
> ?backtrace:
> ? ?[<c01e6f6a>] create_object+0xfa/0x250
> ? ?[<c01e753d>] kmemleak_alloc+0x5d/0x70
> ? ?[<c01e223d>] __kmalloc+0x10d/0x210
> ? ?[<c03b2d2f>] firmware_data_write+0x1df/0x270
> ? ?[<c024163a>] write+0x13a/0x1b0
> ? ?[<c01eae1c>] vfs_write+0x9c/0x190
> ? ?[<c01eafcd>] sys_write+0x3d/0x70
> ? ?[<c010319c>] sysenter_do_call+0x12/0x38
> ? ?[<ffffffff>] 0xffffffff
>
> Any idea? It looks like this is the kmalloc() in fw_realloc_buffer()
> (inlined in firmware_data_write).
I guess the leak is introduced in commit :
commit 6e03a201bbe8137487f340d26aa662110e324b20
firmware: speed up request_firmware(), v3
The attachment patch may fix the leak, please test and verify it.
Thanks.
--
Lei Ming
On Wed, 2009-07-08 at 12:38 +0800, Ming Lei wrote:
> 2009/7/8 Catalin Marinas <[email protected]>:
> > On Tue, 2009-07-07 at 19:01 +0200, Cornelia Huck wrote:
> >
> > There is one more leak in this area which I couldn't figure out where it
> > should be freed:
> >
> > unreferenced object 0xc353e530 (size 512):
> > comm "cat", pid 3130, jiffies 4294903232
> > backtrace:
> > [<c01e6f6a>] create_object+0xfa/0x250
> > [<c01e753d>] kmemleak_alloc+0x5d/0x70
> > [<c01e223d>] __kmalloc+0x10d/0x210
> > [<c03b2d2f>] firmware_data_write+0x1df/0x270
> > [<c024163a>] write+0x13a/0x1b0
> > [<c01eae1c>] vfs_write+0x9c/0x190
> > [<c01eafcd>] sys_write+0x3d/0x70
> > [<c010319c>] sysenter_do_call+0x12/0x38
> > [<ffffffff>] 0xffffffff
> >
> > Any idea? It looks like this is the kmalloc() in fw_realloc_buffer()
> > (inlined in firmware_data_write).
>
> I guess the leak is introduced in commit :
> commit 6e03a201bbe8137487f340d26aa662110e324b20
> firmware: speed up request_firmware(), v3
>
> The attachment patch may fix the leak, please test and verify it.
> Thanks.
I think you need to stop it from clearing fw_priv->nr_pages too.
With that change, it looks correct. Thanks.
Acked-by: David Woodhouse <[email protected]>
--
David Woodhouse Open Source Technology Centre
[email protected] Intel Corporation
2009/7/8 David Woodhouse <[email protected]>:
> On Wed, 2009-07-08 at 12:38 +0800, Ming Lei wrote:
>> 2009/7/8 Catalin Marinas <[email protected]>:
>> > On Tue, 2009-07-07 at 19:01 +0200, Cornelia Huck wrote:
>> >
>> > There is one more leak in this area which I couldn't figure out where it
>> > should be freed:
>> >
>> > unreferenced object 0xc353e530 (size 512):
>> > ?comm "cat", pid 3130, jiffies 4294903232
>> > ?backtrace:
>> > ? ?[<c01e6f6a>] create_object+0xfa/0x250
>> > ? ?[<c01e753d>] kmemleak_alloc+0x5d/0x70
>> > ? ?[<c01e223d>] __kmalloc+0x10d/0x210
>> > ? ?[<c03b2d2f>] firmware_data_write+0x1df/0x270
>> > ? ?[<c024163a>] write+0x13a/0x1b0
>> > ? ?[<c01eae1c>] vfs_write+0x9c/0x190
>> > ? ?[<c01eafcd>] sys_write+0x3d/0x70
>> > ? ?[<c010319c>] sysenter_do_call+0x12/0x38
>> > ? ?[<ffffffff>] 0xffffffff
>> >
>> > Any idea? It looks like this is the kmalloc() in fw_realloc_buffer()
>> > (inlined in firmware_data_write).
>>
>> I guess the leak is introduced in commit :
>> ? ? ? ? ? ? ?commit ? 6e03a201bbe8137487f340d26aa662110e324b20
>> ? ? ? ? ? ? ?firmware: speed up request_firmware(), v3
>>
>> The attachment patch may fix the leak, please test and verify it.
>> Thanks.
>
> I think you need to stop it from clearing fw_priv->nr_pages too.
> With that change, it looks correct. Thanks.
IMHO, No.
If nr_pages is not cleaned, pages pointed by pages[] will be freed
by fw_dev_release, but they should be freed by vfree()
in release_firmware. Right?
>
> Acked-by: David Woodhouse <[email protected]>
--
Lei Ming
On Wed, 2009-07-08 at 16:42 +0800, Ming Lei wrote:
>
> IMHO, No.
> If nr_pages is not cleaned, pages pointed by pages[] will be freed
> by fw_dev_release, but they should be freed by vfree()
> in release_firmware. Right?
Yes, sorry -- you're right, and the original patch is fine.
--
David Woodhouse Open Source Technology Centre
[email protected] Intel Corporation
On Tue, Jul 07, 2009 at 04:17:00PM +0100, Catalin Marinas wrote:
> Hi,
>
> I get a couple kmemleak reports like below which I think happen on the
> failure path (-ENOENT) of a request_firmware() call:
<snip>
Hm, can you send me the final version of this patch, it seemed to go
through a few different versions, with a signed-off-by line?
thanks,
greg k-h
Greg KH <[email protected]> wrote:
> On Tue, Jul 07, 2009 at 04:17:00PM +0100, Catalin Marinas wrote:
>> I get a couple kmemleak reports like below which I think happen on the
>> failure path (-ENOENT) of a request_firmware() call:
>
> Hm, can you send me the final version of this patch, it seemed to go
> through a few different versions, with a signed-off-by line?
It was merged as commit 0f2f2221b4ad8.
--
Catalin
On Fri, Jul 10, 2009 at 11:49:35PM +0100, Catalin Marinas wrote:
> Greg KH <[email protected]> wrote:
> > On Tue, Jul 07, 2009 at 04:17:00PM +0100, Catalin Marinas wrote:
> >> I get a couple kmemleak reports like below which I think happen on the
> >> failure path (-ENOENT) of a request_firmware() call:
> >
> > Hm, can you send me the final version of this patch, it seemed to go
> > through a few different versions, with a signed-off-by line?
>
> It was merged as commit 0f2f2221b4ad8.
Ah, nice, I don't have to worry about it then, thanks.
greg k-h