| Message ID | 20090224133505.GJ21900@pengutronix.de |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 24 Feb 2009 14:35:05 +0100, Sascha Hauer <s.hauer@pengutronix.de> wrote: > On Wed, Nov 12, 2008 at 11:57:33PM +0900, Atsushi Nemoto wrote: > > The mtd partition parser returns an allocated pointer array of > > mtd_partition. The caller must free it. The array is used only for > > add_mtd_partitions(), so free it just after the call. > > This patch breaks command line parsing support. With command line > partition parsing the struct mtd_partition array is allocated, but only > once. On my board with NAND and NOR (both with command line partition > parsing) It fails badly in parse_cmdline_partitions() when the second > device gets parsed. > > The following patch fixes it, but I don't know if this is > the correct solution. Does anybody have more insights on this? Do your NAND and NOR have same mtd-id? The cmdlinepart allocates mtd_partition aray for each mtd-id. So usually another array will be returned for NAND and NOR. The physmap patch has another bug and fixes are on the way mainline: http://git.infradead.org/mtd-2.6.git?a=commit;h=e480814f138cd5d78a8efe397756ba6b6518fdb6 But this seems not enough, as you wrote. If multiple mtd have same mtd-id, bad things can happen. And more seriously, if I load physmap driver _again_ after unload, cmdlinepart will return a freed pointer on the second time. Hmm, little memory leak is less serious than crash. Now I start thinking reverting the commit 176bf2e0 will be best for 2.6.29 release. I'm not sure for long term solutions. A) make all parsers return kmalloc-ed mtd_partition array each time and fix memory leak in each driver B) make all parsers return mtd_partition array allocated only once, and fix drivers which free the mtd_partition array. David, how do you think? --- Atsushi Nemoto
On Tue, Feb 24, 2009 at 11:36:00PM +0900, Atsushi Nemoto wrote: > On Tue, 24 Feb 2009 14:35:05 +0100, Sascha Hauer <s.hauer@pengutronix.de> wrote: > > On Wed, Nov 12, 2008 at 11:57:33PM +0900, Atsushi Nemoto wrote: > > > The mtd partition parser returns an allocated pointer array of > > > mtd_partition. The caller must free it. The array is used only for > > > add_mtd_partitions(), so free it just after the call. > > > > This patch breaks command line parsing support. With command line > > partition parsing the struct mtd_partition array is allocated, but only > > once. On my board with NAND and NOR (both with command line partition > > parsing) It fails badly in parse_cmdline_partitions() when the second > > device gets parsed. > > > > The following patch fixes it, but I don't know if this is > > the correct solution. Does anybody have more insights on this? > > Do your NAND and NOR have same mtd-id? The cmdlinepart allocates > mtd_partition aray for each mtd-id. So usually another array will be > returned for NAND and NOR. Yes, it allocates one array for NAND and one for NOR. The problem starts in mtdpart_setup_real(): > struct cmdline_mtd_partition *this_mtd; > struct mtd_partition *parts; > > [...] > > /* > * parse one mtd. have it reserve memory for the > * struct cmdline_mtd_partition and the mtd-id string. > */ > parts = newpart(p + 1, /* cmdline */ > &s, /* out: updated cmdline ptr */ > &num_parts, /* out: number of parts */ > 0, /* first partition */ > (unsigned char**)&this_mtd, /* out: extra mem */ > mtd_id_len + 1 + sizeof(*this_mtd) + > sizeof(void*)-1 /*alignment*/); > Here we allocate the array containing not only the struct mtd_partition, but also the names and the struct cmdline_mtd_partition... > for(part = partitions; part; part = part->next) > { > if ((!mtd_id) || (!strcmp(part->mtd_id, mtd_id))) > { > for(i = 0, offset = 0; i < part->num_parts; i++) > { > if (part->parts[i].offset == OFFSET_CONTINUOUS) > part->parts[i].offset = offset; > else > offset = part->parts[i].offset; > if (part->parts[i].size == SIZE_REMAINING) > part->parts[i].size = master->size - offset; > if (offset + part->parts[i].size > master->size) > { > printk(KERN_WARNING ERRP > "%s: partitioning exceeds flash size, truncating\n", > part->mtd_id); > part->parts[i].size = master->size - offset; > part->num_parts = i; > } > offset += part->parts[i].size; > } > *pparts = part->parts; ...here this array is returned and kfreed in the physmap flash driver. The next time we enter this loop 'part' points to already freed memory. *pparts is not meant to be freed, cmdlinepart.c needs it for the whole kernel life. We can only return a copy of the partition array. > > The physmap patch has another bug and fixes are on the way mainline: > > http://git.infradead.org/mtd-2.6.git?a=commit;h=e480814f138cd5d78a8efe397756ba6b6518fdb6 > > But this seems not enough, as you wrote. If multiple mtd have same > mtd-id, bad things can happen. And more seriously, if I load physmap > driver _again_ after unload, cmdlinepart will return a freed pointer > on the second time. > > Hmm, little memory leak is less serious than crash. Now I start > thinking reverting the commit 176bf2e0 will be best for 2.6.29 > release. Even when reverting the commit the same problem still exists because the array then gets freed in physmap_flash_remove(). This won't hurt me though because I never use mtd drivers as modules. Sascha
On Tue, 24 Feb 2009 16:29:58 +0100, Sascha Hauer <s.hauer@pengutronix.de> wrote: > > The physmap patch has another bug and fixes are on the way mainline: > > > > http://git.infradead.org/mtd-2.6.git?a=commit;h=e480814f138cd5d78a8efe397756ba6b6518fdb6 > > > > But this seems not enough, as you wrote. If multiple mtd have same > > mtd-id, bad things can happen. And more seriously, if I load physmap > > driver _again_ after unload, cmdlinepart will return a freed pointer > > on the second time. > > > > Hmm, little memory leak is less serious than crash. Now I start > > thinking reverting the commit 176bf2e0 will be best for 2.6.29 > > release. > > Even when reverting the commit the same problem still exists because the > array then gets freed in physmap_flash_remove(). This won't hurt me > though because I never use mtd drivers as modules. If the commit reverted, kfree() in physmap_flash_remove never be called due to another bug (info->nr_parts is not set properly). But unloading the physmap module will lead crash anyway since master mtd device will be freed without deleting slave mtd devices if cmdlinepart was used. So I think either reverting the commit or applying the above fix in mtd-2.6 git tree can fix regression from 2.6.28. Both work well unless unloading the physmap module after booting with mtdparts= option. --- Atsushi Nemoto
diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c index 50a3403..14c00dd 100644 --- a/drivers/mtd/cmdlinepart.c +++ b/drivers/mtd/cmdlinepart.c @@ -335,7 +335,13 @@ static int parse_cmdline_partitions(struct mtd_info *master, } offset += part->parts[i].size; } - *pparts = part->parts; + + *pparts = kmalloc(sizeof (struct mtd_partition) * part->num_parts, GFP_KERNEL); + if (!*pparts) + return -ENOMEM; + + memcpy(*pparts, part->parts, sizeof (struct mtd_partition) * part->num_parts); + return part->num_parts; } }