| Message ID | 1296379908-10150-1-git-send-email-minipli@googlemail.com |
|---|---|
| State | Accepted |
| Commit | f17f12ce9dd6ec0a8e1f415ecdbaebfce0207464 |
| Headers | show |
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().
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
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.
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; }
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(-)