linux-kernel - Re: mtd, nand, omap2: parse cmdline partition fail

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAH6sp9OqYt76FGbOWKxSovvbx8+TjGCKbAvy-V7t5N6FDuO3pQ@mail.gmail.com>
Date:	Thu, 10 Dec 2015 08:13:15 +0100
From:	Frans Klaver <fransklaver@...il.com>
To:	Brian Norris <computersforpeace@...il.com>
Cc:	Heiko Schocher <hs@...x.de>, David Woodhouse <dwmw2@...radead.org>,
	Boris BREZILLON <boris.brezillon@...e-electrons.com>,
	Pekon Gupta <pekon@...-sem.com>, Roger Quadros <rogerq@...com>,
	Nicholas Mc Guire <hofrat@...dl.org>,
	linux-mtd@...ts.infradead.org,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Stefano Babic <sbabic@...x.de>,
	"Stahl Martin (Helbling Technik)" <Martin.Stahl@...bling.ch>
Subject: Re: mtd, nand, omap2: parse cmdline partition fail

On Thu, Dec 10, 2015 at 12:19 AM, Brian Norris
<computersforpeace@...il.com> wrote:
> On Fri, Dec 04, 2015 at 09:42:06AM +0100, Heiko Schocher wrote:
>> It seems to me add_mtd_device() gets only called for the mtd partitions
>> parsed from the cmdline ...
>
> That's true, when CONFIG_MTD_PARTITIONED_MASTER=n. (I'm really thinking
> we should accelerate the adoption of PARTITIONED_MASTER... maybe set it
> to default =y?)
>
> But even with CONFIG_MTD_PARTITIONED_MASTER=y we still have a problem.
>
> [...]
>
>> >The fact that this produces different names for you is slightly
>> >surprising to me, unless mtd->name is already set to something by the
>> >time it reaches add_mtd_device(). Or I overlooked something, which is
>> >entirely plausible as well.
>> >
>> >So effectively this should be the same as doing:
>> >
>> >   mtd->dev.parent = &pdev->dev;
>> >   mtd->name = dev_name(mtd->dev.parent);
>
> Yes, except for one thing (and this is the key): the mtd->name only gets
> set *after* the partitions are parsed, using the method from commit
> 807f16d4db95 ("mtd: core: set some defaults when dev.parent is set"). So
> the parsers (including cmdlinepart) get run with a blank (NULL) name,
> and you can't detect any partitions, since the name match will never
> work.

Right, that was something we overlooked earlier.


> I have a hack of a patch below (untested) that would hopefully work
> (based on current l2-mtd.git). I could port this to a base on 4.4-rc1 if
> it works OK, so we can get the regression fixed in this cycle.

That would be nice.


>> >>But wondering, if there are two or more identical nand chips in the
>> >>system, they will have the same mtd->name ... which seems buggy to me...
>> >
>> >Agree.
>>
>> Good, so we must fix it ;-)
>
> Yeah, that's a bad problem. Most people only plan for one chip and then
> realize they need 2 later. nand_base should probably support something
> to do this easily. Unfortunately, making a change like that could cause
> some problems with cmdline naming across kernel versions, if we start
> changing the convention for some drivers...(do we consider the MTD name
> part of the ABI?)

I would expect a name to be just a name; something parsable by humans.
By definition that cannot be an ABI. On the other hand, maybe it has
grown to become part of the ABI.


> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 89d811e7b04a..185dc36c687f 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -592,6 +592,15 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>         struct mtd_partitions parsed;
>         int ret;
>
> +       if (mtd->dev.parent) {
> +               if (!mtd->owner && mtd->dev.parent->driver)
> +                       mtd->owner = mtd->dev.parent->driver->owner;
> +               if (!mtd->name)
> +                       mtd->name = dev_name(mtd->dev.parent);
> +       } else {
> +               pr_debug("mtd device won't show a device symlink in sysfs\n");
> +       }
> +
>         memset(&parsed, 0, sizeof(parsed));
>
>         ret = parse_mtd_partitions(mtd, types, &parsed, parser_data);

This was the first thing I thought of when this issue was brought up.
If you do this, do you still need the chunk of code you copied from in
add_mtd_device()? Since we fill in these things on the master, I
wouldn't think we do.

Thanks,
Frans
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/