Possible memory leak in request_firmware()

Possible memory leak in request_firmware()

[Catalin Marinas]

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

Re: Possible memory leak in request_firmware()

[Cornelia Huck]

On Tue, 07 Jul 2009 16:17:00 +0100,
Catalin Marinas <catalin.marinas@arm.com> 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);
> 
> 

Re: Possible memory leak in request_firmware()

[Catalin Marinas]

On Tue, 2009-07-07 at 19:01 +0200, Cornelia Huck wrote:
> On Tue, 07 Jul 2009 16:17:00 +0100,
> Catalin Marinas <catalin.marinas@arm.com> 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

Re: Possible memory leak in request_firmware()

[Ming Lei]

2009/7/7 Catalin Marinas <catalin.marinas@arm.com>:
> 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 <tom.leiming@gmail.com>

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

Re: Possible memory leak in request_firmware()

[Ming Lei]

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

2009/7/8 Catalin Marinas <catalin.marinas@arm.com>:
> 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

[-- Attachment #2: firmware_class_mem_leak.patch --]
[-- Type: text/x-patch, Size: 441 bytes --]

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 2da4803..271dc6b 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -180,7 +180,6 @@ static ssize_t firmware_loading_store(struct device *dev,
 				goto err;
 			}
 			/* Pages will be freed by vfree() */
-			fw_priv->pages = NULL;
 			fw_priv->page_array_size = 0;
 			fw_priv->nr_pages = 0;
 			complete(&fw_priv->completion);

Re: Possible memory leak in request_firmware()

[David Woodhouse]

On Wed, 2009-07-08 at 12:38 +0800, Ming Lei wrote:
> 2009/7/8 Catalin Marinas <catalin.marinas@arm.com>:
> > 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 <David.Woodhouse@intel.com>

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: Possible memory leak in request_firmware()

[Ming Lei]

2009/7/8 David Woodhouse <dwmw2@infradead.org>:
> On Wed, 2009-07-08 at 12:38 +0800, Ming Lei wrote:
>> 2009/7/8 Catalin Marinas <catalin.marinas@arm.com>:
>> > 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 <David.Woodhouse@intel.com>




-- 
Lei Ming

Re: Possible memory leak in request_firmware()

[David Woodhouse]

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
David.Woodhouse@intel.com                              Intel Corporation

Re: Possible memory leak in request_firmware()

[Greg KH]

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

Re: Possible memory leak in request_firmware()

[Catalin Marinas]

Greg KH <greg@kroah.com> 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

Re: Possible memory leak in request_firmware()

[Greg KH]

On Fri, Jul 10, 2009 at 11:49:35PM +0100, Catalin Marinas wrote:
> Greg KH <greg@kroah.com> 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