[PATCH] mtd: fix memory leak

[PATCH] mtd: fix memory leak

[Mathias Krause]

Commit 4f678a58 missed two cases where the memory allocated for name
would be leaked. This commit frees the memory when register_device()
fails and on unregister_devices().

Signed-off-by: Mathias Krause <minipli@googlemail.com>
---
 drivers/mtd/devices/phram.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
index 5239328..8d28fa0 100644
--- a/drivers/mtd/devices/phram.c
+++ b/drivers/mtd/devices/phram.c
@@ -117,6 +117,7 @@ static void unregister_devices(void)
 	list_for_each_entry_safe(this, safe, &phram_list, list) {
 		del_mtd_device(&this->mtd);
 		iounmap(this->mtd.priv);
+		kfree(this->mtd.name);
 		kfree(this);
 	}
 }
@@ -275,6 +276,8 @@ static int phram_setup(const char *val, struct kernel_param *kp)
 	ret = register_device(name, start, len);
 	if (!ret)
 		pr_info("%s device: %#x at %#x\n", name, len, start);
+	else
+		kfree(name);
 
 	return ret;
 }
-- 
1.5.6.5

Re: [PATCH] mtd: fix memory leak

[Artem Bityutskiy]

Hi,

On Sun, 2011-01-30 at 10:31 +0100, Mathias Krause wrote:
> diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
> index 5239328..8d28fa0 100644
> --- a/drivers/mtd/devices/phram.c
> +++ b/drivers/mtd/devices/phram.c
> @@ -117,6 +117,7 @@ static void unregister_devices(void)
>  	list_for_each_entry_safe(this, safe, &phram_list, list) {
>  		del_mtd_device(&this->mtd);
>  		iounmap(this->mtd.priv);
> +		kfree(this->mtd.name);
>  		kfree(this);
>  	}

Since register_device() did not allocate it, unregister_devices() should
not free it. Hence, I think it is better to free(name) just after
calling unregister_devices().

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH] mtd: fix memory leak

[Mathias Krause]

On 30.01.2011 at 15:52, Artem Bityutskiy wrote:
> Hi,
> 
> On Sun, 2011-01-30 at 10:31 +0100, Mathias Krause wrote:
>> diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
>> index 5239328..8d28fa0 100644
>> --- a/drivers/mtd/devices/phram.c
>> +++ b/drivers/mtd/devices/phram.c
>> @@ -117,6 +117,7 @@ static void unregister_devices(void)
>> 	list_for_each_entry_safe(this, safe, &phram_list, list) {
>> 		del_mtd_device(&this->mtd);
>> 		iounmap(this->mtd.priv);
>> +		kfree(this->mtd.name);
>> 		kfree(this);
>> 	}
> 
> Since register_device() did not allocate it, unregister_devices() should
> not free it.

I agree with you, the internal API is a little quirky regarding that point. register_device() should strdup() the name and not just blindly use it. But since the memory for name was already allocated via kmalloc() in phram_setup() it seems a little nitpicky to copy it once again in register_device().

> Hence, I think it is better to free(name) just after
> calling unregister_devices().

This is not possible because unregister_devices() unregisters all devices, not just a single instance. Though name must be freed for every object in the list. After unregister_devices() returns the list is empty and no pointer to the memory locations do exist any more. So my patch was the straight forward fix for the memory leak.


Regards,
Mathias

Re: [PATCH] mtd: fix memory leak

[Artem Bityutskiy]

On Sun, 2011-01-30 at 18:35 +0100, Mathias Krause wrote:
> On 30.01.2011 at 15:52, Artem Bityutskiy wrote:
> > Hi,
> > 
> > On Sun, 2011-01-30 at 10:31 +0100, Mathias Krause wrote:
> >> diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c
> >> index 5239328..8d28fa0 100644
> >> --- a/drivers/mtd/devices/phram.c
> >> +++ b/drivers/mtd/devices/phram.c
> >> @@ -117,6 +117,7 @@ static void unregister_devices(void)
> >> 	list_for_each_entry_safe(this, safe, &phram_list, list) {
> >> 		del_mtd_device(&this->mtd);
> >> 		iounmap(this->mtd.priv);
> >> +		kfree(this->mtd.name);
> >> 		kfree(this);
> >> 	}
> > 
> > Since register_device() did not allocate it, unregister_devices() should
> > not free it.
> 
> I agree with you, the internal API is a little quirky regarding that point. register_device() should strdup() the name and not just blindly use it. But since the memory for name was already allocated via kmalloc() in phram_setup() it seems a little nitpicky to copy it once again in register_device().
> 
> > Hence, I think it is better to free(name) just after
> > calling unregister_devices().
> 
> This is not possible because unregister_devices() unregisters all devices, not just a single instance. Though name must be freed for every object in the list. After unregister_devices() returns the list is empty and no pointer to the memory locations do exist any more. So my patch was the straight forward fix for the memory leak.

OK, I agree, I think this patch is good enough, pushed to l2-mtd-2.6.git
tree, thanks.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)