[PATCH] memstick: Fix memory leak in memstick_check() error path

[PATCH] memstick: Fix memory leak in memstick_check() error path

[Larry Finger]

With kernel 3.12-rc3, kemcheck reports the following leak:

> unreferenced object 0xffff8800ae85c190 (size 16):
>   comm "kworker/u4:3", pid 685, jiffies 4294916336 (age 2831.760s)
>   hex dump (first 16 bytes):
>     6d 65 6d 73 74 69 63 6b 30 00 00 00 00 00 00 00  memstick0.......
>   backtrace:
>     [<ffffffff8146a0d1>] kmemleak_alloc+0x21/0x50
>     [<ffffffff81160720>] __kmalloc_track_caller+0x160/0x2f0
>     [<ffffffff81237b9b>] kvasprintf+0x5b/0x90
>     [<ffffffff8122c0c1>] kobject_set_name_vargs+0x21/0x60
>     [<ffffffff812e7f5c>] dev_set_name+0x3c/0x40
>     [<ffffffffa02bf918>] memstick_check+0xb8/0x340 [memstick]
>     [<ffffffff81069862>] process_one_work+0x1d2/0x670
>     [<ffffffff8106a88a>] worker_thread+0x11a/0x370
>     [<ffffffff81072ea6>] kthread+0xd6/0xe0
>     [<ffffffff81478bbc>] ret_from_fork+0x7c/0xb0

This problem was introduced by commit 0252c3b "memstick: struct device -
replace bus_id with dev_name(), dev_set_name() where the name is not freed
in the error path.

Thanks to Catalin Marinas for suggesting the fix.

Cc: Kay Sievers <kay.sievers@vrfy.org>
Cc: Alex Dubov <oakad@yahoo.com>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
---
 drivers/memstick/core/memstick.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
index ffcb10a..0c73a45 100644
--- a/drivers/memstick/core/memstick.c
+++ b/drivers/memstick/core/memstick.c
@@ -415,6 +415,7 @@ static struct memstick_dev *memstick_alloc_card(struct memstick_host *host)
 	return card;
 err_out:
 	host->card = old_card;
+	kfree(card->dev.kobj.name);
 	kfree(card);
 	return NULL;
 }
-- 
1.8.1.4

Re: [PATCH] memstick: Fix memory leak in memstick_check() error path

[Catalin Marinas]

On 3 October 2013 22:13, Larry Finger <Larry.Finger@lwfinger.net> wrote:
> With kernel 3.12-rc3, kemcheck reports the following leak:

That would be "kmemleak" rather than "kmemcheck" ;)

>> unreferenced object 0xffff8800ae85c190 (size 16):
>>   comm "kworker/u4:3", pid 685, jiffies 4294916336 (age 2831.760s)
>>   hex dump (first 16 bytes):
>>     6d 65 6d 73 74 69 63 6b 30 00 00 00 00 00 00 00  memstick0.......
>>   backtrace:
>>     [<ffffffff8146a0d1>] kmemleak_alloc+0x21/0x50
>>     [<ffffffff81160720>] __kmalloc_track_caller+0x160/0x2f0
>>     [<ffffffff81237b9b>] kvasprintf+0x5b/0x90
>>     [<ffffffff8122c0c1>] kobject_set_name_vargs+0x21/0x60
>>     [<ffffffff812e7f5c>] dev_set_name+0x3c/0x40
>>     [<ffffffffa02bf918>] memstick_check+0xb8/0x340 [memstick]
>>     [<ffffffff81069862>] process_one_work+0x1d2/0x670
>>     [<ffffffff8106a88a>] worker_thread+0x11a/0x370
>>     [<ffffffff81072ea6>] kthread+0xd6/0xe0
>>     [<ffffffff81478bbc>] ret_from_fork+0x7c/0xb0
>
> This problem was introduced by commit 0252c3b "memstick: struct device -
> replace bus_id with dev_name(), dev_set_name() where the name is not freed
> in the error path.
>
> Thanks to Catalin Marinas for suggesting the fix.
>
> Cc: Kay Sievers <kay.sievers@vrfy.org>
> Cc: Alex Dubov <oakad@yahoo.com>
> Cc: Greg Kroah-Hartman <gregkh@suse.de>
> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
> ---
>  drivers/memstick/core/memstick.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
> index ffcb10a..0c73a45 100644
> --- a/drivers/memstick/core/memstick.c
> +++ b/drivers/memstick/core/memstick.c
> @@ -415,6 +415,7 @@ static struct memstick_dev *memstick_alloc_card(struct memstick_host *host)
>         return card;
>  err_out:
>         host->card = old_card;
> +       kfree(card->dev.kobj.name);

It looks weird to go into dev.kobj internals here for freeing the
name. There is also memstick_free_card() which doesn't seem to do
anything about the name freeing.

Should memstick_alloc_card() do a device_initialise(&card->dev) and in
memstick_free_card() (or the error path) do a put_device(&card->dev)?
This should take care of kobj.name as well via kobject_put().

Catalin

Re: [PATCH] memstick: Fix memory leak in memstick_check() error path

[Larry Finger]

On 10/04/2013 03:54 AM, Catalin Marinas wrote:
> On 3 October 2013 22:13, Larry Finger <Larry.Finger@lwfinger.net> wrote:
>> diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
>> index ffcb10a..0c73a45 100644
>> --- a/drivers/memstick/core/memstick.c
>> +++ b/drivers/memstick/core/memstick.c
>> @@ -415,6 +415,7 @@ static struct memstick_dev *memstick_alloc_card(struct memstick_host *host)
>>          return card;
>>   err_out:
>>          host->card = old_card;
>> +       kfree(card->dev.kobj.name);
>
> It looks weird to go into dev.kobj internals here for freeing the
> name. There is also memstick_free_card() which doesn't seem to do
> anything about the name freeing.
>
> Should memstick_alloc_card() do a device_initialise(&card->dev) and in
> memstick_free_card() (or the error path) do a put_device(&card->dev)?
> This should take care of kobj.name as well via kobject_put().

I tried several code changes that included adding a device_initialize() call, 
but all of them oopsed even when I followed the examples in other drivers. 
Adding a put_device() without the device_initialize() did not oops, but it still 
leaked the name.

We could avoid going into the dev.kobj internals if a device_free_name() routine 
existed as a companion to dev_set_name().

Larry

Re: [PATCH] memstick: Fix memory leak in memstick_check() error path

[Alex Dubov]

Hi,

In the good old times, when this driver was first written, device name used to be a fixed
size array (of 32 chars, if I'm not mistaken) in the kobj struct, so there was no need to
free it explicitly.

Since than, somebody changed the name field to become a loose pointer, but it's not
obvious how it is supposed to be handled these days.




________________________________
From: Larry Finger <Larry.Finger@lwfinger.net>
To: Catalin Marinas <catalin.marinas@arm.com> 
Cc: Alex Dubov <oakad@yahoo.com>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; Kay Sievers <kay.sievers@vrfy.org>; Greg Kroah-Hartman <gregkh@suse.de> 
Sent: Monday, 7 October 2013 11:02 AM
Subject: Re: [PATCH] memstick: Fix memory leak in memstick_check() error path


On 10/04/2013 03:54 AM, Catalin Marinas wrote:
> On 3 October 2013 22:13, Larry Finger <Larry.Finger@lwfinger.net> wrote:
>> diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
>> index ffcb10a..0c73a45 100644
>> --- a/drivers/memstick/core/memstick.c
>> +++ b/drivers/memstick/core/memstick.c
>> @@ -415,6 +415,7 @@ static struct memstick_dev *memstick_alloc_card(struct memstick_host *host)
>>          return card;
>>   err_out:
>>          host->card = old_card;
>> +       kfree(card->dev.kobj.name);
>
> It looks weird to go into dev.kobj internals here for freeing the
> name. There is also memstick_free_card() which doesn't seem to do
> anything about the name freeing.
>
> Should memstick_alloc_card() do a device_initialise(&card->dev) and in
> memstick_free_card() (or the error path) do a put_device(&card->dev)?
> This should take care of kobj.name as well via kobject_put().

I tried several code changes that included adding a device_initialize() call, 
but all of them oopsed even when I followed the examples in other drivers. 
Adding a put_device() without the device_initialize() did not oops, but it still 
leaked the name.

We could avoid going into the dev.kobj internals if a device_free_name() routine 
existed as a companion to dev_set_name().


Larry

Re: [PATCH] memstick: Fix memory leak in memstick_check() error path

[Larry Finger]

On 10/06/2013 08:57 PM, Alex Dubov wrote:
> Hi,
>
> In the good old times, when this driver was first written, device name used to be a fixed
> size array (of 32 chars, if I'm not mistaken) in the kobj struct, so there was no need to
> free it explicitly.
>
> Since than, somebody changed the name field to become a loose pointer, but it's not
> obvious how it is supposed to be handled these days.

It has been some time since it was changed. In commit af5ca3f by Kay Sievers and 
merged on Dec 20, 2007, "const char *k_name" was changed to "const char *name". 
I did not go any further back.

I'll submit V2 of my patch for further comment.

Larry

Re: [PATCH] memstick: Fix memory leak in memstick_check() error path

[Catalin Marinas]

On Mon, Oct 07, 2013 at 01:02:43AM +0100, Larry Finger wrote:
> On 10/04/2013 03:54 AM, Catalin Marinas wrote:
> > On 3 October 2013 22:13, Larry Finger <Larry.Finger@lwfinger.net> wrote:
> >> diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
> >> index ffcb10a..0c73a45 100644
> >> --- a/drivers/memstick/core/memstick.c
> >> +++ b/drivers/memstick/core/memstick.c
> >> @@ -415,6 +415,7 @@ static struct memstick_dev *memstick_alloc_card(struct memstick_host *host)
> >>          return card;
> >>   err_out:
> >>          host->card = old_card;
> >> +       kfree(card->dev.kobj.name);
> >
> > It looks weird to go into dev.kobj internals here for freeing the
> > name. There is also memstick_free_card() which doesn't seem to do
> > anything about the name freeing.
> >
> > Should memstick_alloc_card() do a device_initialise(&card->dev) and in
> > memstick_free_card() (or the error path) do a put_device(&card->dev)?
> > This should take care of kobj.name as well via kobject_put().
> 
> I tried several code changes that included adding a device_initialize() call, 
> but all of them oopsed even when I followed the examples in other drivers. 
> Adding a put_device() without the device_initialize() did not oops, but it still 
> leaked the name.
> 
> We could avoid going into the dev.kobj internals if a device_free_name() routine 
> existed as a companion to dev_set_name().

device_free_name() wouldn't be a bad idea.

-- 
Catalin

Re: [PATCH V2] memstick: Fix memory leak in memstick_check() error path

[Greg Kroah-Hartman]

On Sun, Oct 06, 2013 at 10:21:51PM -0500, Larry Finger wrote:
> With kernel 3.12-rc3, kmemleak reports the following leak:
> 
> > unreferenced object 0xffff8800ae85c190 (size 16):
> >   comm "kworker/u4:3", pid 685, jiffies 4294916336 (age 2831.760s)
> >   hex dump (first 16 bytes):
> >     6d 65 6d 73 74 69 63 6b 30 00 00 00 00 00 00 00  memstick0.......
> >   backtrace:
> >     [<ffffffff8146a0d1>] kmemleak_alloc+0x21/0x50
> >     [<ffffffff81160720>] __kmalloc_track_caller+0x160/0x2f0
> >     [<ffffffff81237b9b>] kvasprintf+0x5b/0x90
> >     [<ffffffff8122c0c1>] kobject_set_name_vargs+0x21/0x60
> >     [<ffffffff812e7f5c>] dev_set_name+0x3c/0x40
> >     [<ffffffffa02bf918>] memstick_check+0xb8/0x340 [memstick]
> >     [<ffffffff81069862>] process_one_work+0x1d2/0x670
> >     [<ffffffff8106a88a>] worker_thread+0x11a/0x370
> >     [<ffffffff81072ea6>] kthread+0xd6/0xe0
> >     [<ffffffff81478bbc>] ret_from_fork+0x7c/0xb0
> 
> This problem was introduced by commit 0252c3b "memstick: struct device -
> replace bus_id with dev_name(), dev_set_name()" where the name is not freed
> in the error path. The name is also leaked in memstick_free_card(). 
> 
> Thanks to Catalin Marinas for suggesting the fix.
> 
> Cc: Kay Sievers <kay.sievers@vrfy.org>
> Cc: Alex Dubov <oakad@yahoo.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
> ---
> 
> V2 fixes the typos in the commit message, and frees the name in
> memstick_free_card() as well as the error path in memstick_check().

Looking back at this, to try to figure out why the kmemleak report shows
up, shows that this is a mess.  Why would we be erroring out _before_ we
try to register the struct device with the driver core, yet we had
already initialized the struct device structure?  Only set up the
structure right before sending it to driver core, don't delay in
allocation, only problems can happen (like here.)

To fix this up, will take some major work, which I can't do, sorry, but
this patch will not work either.

greg k-h

Re: [PATCH V2] memstick: Fix memory leak in memstick_check() error path

[Larry Finger]

On 10/07/2013 09:13 PM, Greg Kroah-Hartman wrote:
> On Sun, Oct 06, 2013 at 10:21:51PM -0500, Larry Finger wrote:
>> With kernel 3.12-rc3, kmemleak reports the following leak:
>>
>>> unreferenced object 0xffff8800ae85c190 (size 16):
>>>    comm "kworker/u4:3", pid 685, jiffies 4294916336 (age 2831.760s)
>>>    hex dump (first 16 bytes):
>>>      6d 65 6d 73 74 69 63 6b 30 00 00 00 00 00 00 00  memstick0.......
>>>    backtrace:
>>>      [<ffffffff8146a0d1>] kmemleak_alloc+0x21/0x50
>>>      [<ffffffff81160720>] __kmalloc_track_caller+0x160/0x2f0
>>>      [<ffffffff81237b9b>] kvasprintf+0x5b/0x90
>>>      [<ffffffff8122c0c1>] kobject_set_name_vargs+0x21/0x60
>>>      [<ffffffff812e7f5c>] dev_set_name+0x3c/0x40
>>>      [<ffffffffa02bf918>] memstick_check+0xb8/0x340 [memstick]
>>>      [<ffffffff81069862>] process_one_work+0x1d2/0x670
>>>      [<ffffffff8106a88a>] worker_thread+0x11a/0x370
>>>      [<ffffffff81072ea6>] kthread+0xd6/0xe0
>>>      [<ffffffff81478bbc>] ret_from_fork+0x7c/0xb0
>>
>> This problem was introduced by commit 0252c3b "memstick: struct device -
>> replace bus_id with dev_name(), dev_set_name()" where the name is not freed
>> in the error path. The name is also leaked in memstick_free_card().
>>
>> Thanks to Catalin Marinas for suggesting the fix.
>>
>> Cc: Kay Sievers <kay.sievers@vrfy.org>
>> Cc: Alex Dubov <oakad@yahoo.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
>> ---
>>
>> V2 fixes the typos in the commit message, and frees the name in
>> memstick_free_card() as well as the error path in memstick_check().
>
> Looking back at this, to try to figure out why the kmemleak report shows
> up, shows that this is a mess.  Why would we be erroring out _before_ we
> try to register the struct device with the driver core, yet we had
> already initialized the struct device structure?  Only set up the
> structure right before sending it to driver core, don't delay in
> allocation, only problems can happen (like here.)
>
> To fix this up, will take some major work, which I can't do, sorry, but
> this patch will not work either.

Thanks for the analysis. My interest is getting rid of memory leaks from other 
sources so that the leaks from my drivers are more obvious.

Perhaps Alex will pick this up and do the rewrite.

Please drop both patches.

Larry