Patchwork mtd: mtdpart: break it as soon as we parse out the partitions

login
register
mail settings
Submitter Huang Shijie
Date Aug. 18, 2012, 5 p.m.
Message ID <1345309236-25806-1-git-send-email-shijie8@gmail.com>
Download mbox | patch
Permalink /patch/178425/
State New
Headers show

Comments

Huang Shijie - Aug. 18, 2012, 5 p.m.
We may cause a memory leak when the @types has more then one parser.

Take the `default_mtd_part_types` for example. The default_mtd_part_types has
two parsers now: `cmdlinepart` and `ofpart`.

Assume the following case:
The kernel command line sets the partitions like:
	#gpmi-nand:20m(boot),20m(kernel),1g(rootfs),-(user)
But the devicetree file(such as arch/arm/boot/dts/imx28-evk.dts) also sets
the same partitions as the kernel command line does.

In the current code, the partitions parsed out by the `ofpart` will
overwrite the @pparts which has already set by the `cmdlinepart` parser,
and the the partitions parsed out by the `cmdlinepart` is missed.
A memory leak occurs.

So we should break the code as soon as we parse out the partitions,
In actually, this patch makes a priority order between the parsers.
If one parser has already parsed out the partitions successfully,
it's no need to use another parser anymore.

Signed-off-by: Huang Shijie <shijie8@gmail.com>
---
 drivers/mtd/mtdpart.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
Artem Bityutskiy - Aug. 24, 2012, 3:54 p.m.
On Sat, 2012-08-18 at 13:00 -0400, Huang Shijie wrote:
> We may cause a memory leak when the @types has more then one parser.
> 
> Take the `default_mtd_part_types` for example. The default_mtd_part_types has
> two parsers now: `cmdlinepart` and `ofpart`.
> 
> Assume the following case:
> The kernel command line sets the partitions like:
> 	#gpmi-nand:20m(boot),20m(kernel),1g(rootfs),-(user)
> But the devicetree file(such as arch/arm/boot/dts/imx28-evk.dts) also sets
> the same partitions as the kernel command line does.
> 
> In the current code, the partitions parsed out by the `ofpart` will
> overwrite the @pparts which has already set by the `cmdlinepart` parser,
> and the the partitions parsed out by the `cmdlinepart` is missed.
> A memory leak occurs.
> 
> So we should break the code as soon as we parse out the partitions,
> In actually, this patch makes a priority order between the parsers.
> If one parser has already parsed out the partitions successfully,
> it's no need to use another parser anymore.
> 
> Signed-off-by: Huang Shijie <shijie8@gmail.com>

Pushed to l2-mtd.git with "Cc: stable@vger.kernel.org" - do you agree
with that?

Thanks!
Artem Bityutskiy - Aug. 24, 2012, 3:57 p.m.
On Fri, 2012-08-24 at 18:54 +0300, Artem Bityutskiy wrote:
> Pushed to l2-mtd.git with "Cc: stable@vger.kernel.org" - do you agree
> with that?

Note, I've actually pushed v2 of this patch.
Huang Shijie - Aug. 24, 2012, 4:20 p.m.
On Fri, Aug 24, 2012 at 11:54 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Sat, 2012-08-18 at 13:00 -0400, Huang Shijie wrote:
>> We may cause a memory leak when the @types has more then one parser.
>>
>> Take the `default_mtd_part_types` for example. The default_mtd_part_types has
>> two parsers now: `cmdlinepart` and `ofpart`.
>>
>> Assume the following case:
>> The kernel command line sets the partitions like:
>>       #gpmi-nand:20m(boot),20m(kernel),1g(rootfs),-(user)
>> But the devicetree file(such as arch/arm/boot/dts/imx28-evk.dts) also sets
>> the same partitions as the kernel command line does.
>>
>> In the current code, the partitions parsed out by the `ofpart` will
>> overwrite the @pparts which has already set by the `cmdlinepart` parser,
>> and the the partitions parsed out by the `cmdlinepart` is missed.
>> A memory leak occurs.
>>
>> So we should break the code as soon as we parse out the partitions,
>> In actually, this patch makes a priority order between the parsers.
>> If one parser has already parsed out the partitions successfully,
>> it's no need to use another parser anymore.
>>
>> Signed-off-by: Huang Shijie <shijie8@gmail.com>
>
> Pushed to l2-mtd.git with "Cc: stable@vger.kernel.org" - do you agree
> with that?
>
thanks a lot. I do agree to cc to stable.

Huang Shijie

Patch

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index d518e4d..5999856 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -711,6 +711,8 @@  static const char *default_mtd_part_types[] = {
  * partition parsers, specified in @types. However, if @types is %NULL, then
  * the default list of parsers is used. The default list contains only the
  * "cmdlinepart" and "ofpart" parsers ATM.
+ * Note: If there are more one parsers in @types, the kernel only takes the
+ * partitions parsed out by the first parser.
  *
  * This function may return:
  * o a negative error code in case of failure
@@ -735,11 +737,12 @@  int parse_mtd_partitions(struct mtd_info *master, const char **types,
 		if (!parser)
 			continue;
 		ret = (*parser->parse_fn)(master, pparts, data);
+		put_partition_parser(parser);
 		if (ret > 0) {
 			printk(KERN_NOTICE "%d %s partitions found on MTD device %s\n",
 			       ret, parser->name, master->name);
+			break;
 		}
-		put_partition_parser(parser);
 	}
 	return ret;
 }