2013-10-07 03:22:19

by Larry Finger

[permalink] [raw]
Subject: [PATCH V2] memstick: Fix memory leak in memstick_check() error path

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 <[email protected]>
Cc: Alex Dubov <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Larry Finger <[email protected]>
---

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().

Larry
---
drivers/memstick/core/memstick.c | 2 ++
1 file changed, 2 insertion(+)

Index: wireless-testing-save/drivers/memstick/core/memstick.c
===================================================================
--- wireless-testing-save.orig/drivers/memstick/core/memstick.c
+++ wireless-testing-save/drivers/memstick/core/memstick.c
@@ -195,6 +195,7 @@ static void memstick_free_card(struct de
{
struct memstick_dev *card = container_of(dev, struct memstick_dev,
dev);
+ kfree(card->dev.kobj.name);
kfree(card);
}

@@ -415,6 +416,7 @@ static struct memstick_dev *memstick_all
return card;
err_out:
host->card = old_card;
+ kfree(card->dev.kobj.name);
kfree(card);
return NULL;
}


2013-10-08 02:20:10

by Greg Kroah-Hartman

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

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 <[email protected]>
> Cc: Alex Dubov <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Signed-off-by: Larry Finger <[email protected]>
> ---
>
> 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

2013-10-08 03:12:07

by Larry Finger

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

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 <[email protected]>
>> Cc: Alex Dubov <[email protected]>
>> Cc: Greg Kroah-Hartman <[email protected]>
>> Signed-off-by: Larry Finger <[email protected]>
>> ---
>>
>> 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