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@xxxxxxxxxxxx>
To: Catalin Marinas <catalin.marinas@xxxxxxx> 
Cc: Alex Dubov <oakad@xxxxxxxxx>; Linux Kernel Mailing List <linux-kernel@xxxxxxxxxxxxxxx>; Kay Sievers <kay.sievers@xxxxxxxx>; Greg Kroah-Hartman <gregkh@xxxxxxx> 
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@xxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/