mtd: parser: cmdline: Support MTD names containing one or more colons

From: Boris Brezillon <[email protected]>

Looks like some drivers define MTD names with a colon in it, thus making mtdpart= parsing impossible. Let's fix the parser to gracefully handle that case: the last ':' in a partition definition sequence is considered instead of the first one.

---
 drivers/mtd/parsers/cmdlinepart.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

index c86f2db8c882..0625b25620ca 100644
@@ -218,12 +218,29 @@ static int mtdpart_setup_real(char *s)

		struct cmdline_mtd_partition *this_mtd;
		struct mtd_partition *parts;
		int mtd_id_len, num_parts;

- char *p, *mtd_id;
+ char *p, *mtd_id, *semicol;
+
+ /*
+ * Replace the first ';' by a NULL char so strrchr can work
+ * properly.
+ */
+ semicol = strchr(s, ';');
+ if (semicol)
+ *semicol = '\0';

		mtd_id = s;

- /* fetch <mtd-id> */
- p = strchr(s, ':');
+ /*
+ * fetch <mtd-id>. We use strrchr to ignore all ':' that could
+ * be present in the MTD name, only the last one is interpreted
+ * as an <mtd-id>/<part-definition> separator.
+ */
+ p = strrchr(s, ':');
+
+ /* Restore the ';' now. */
+ if (semicol)
+ *semicol = ';';
+

		if (!p) {
			pr_err("no mtd-id\n");
			return -EINVAL;

This change broke OpenWrt booting on some IPQ40xx devices. Here for example the parts of the cmdline which the u-boot on an OpenMesh A62 sets automatically:

    root=31:11 mtdparts=spi0.0:256k(0:SBL1),128k(0:MIBIB),384k(0:QSEE),64k(0:CDT),64k(0:DDRPARAMS),64k(0:APPSBLENV),512k(0:APPSBL),64k(0:ART),64k(custom),64k(0:KEYS),0x002b0000(kernel),0x00c80000(rootfs),15552k(inactive) rootfsname=rootfs rootwait

This is then parsed by newpart as: KEYS),0x002b0000(kernel),0x00c80000(rootfs),15552k(inactive)

And of course, this results in:

    mtd: partition has size 0
    [...]
    /dev/root: Can't open blockdev
    VFS: Cannot open root device "31:11" or unknown-block(31,11): error -6
    Please append a correct "root=" boot option; here are the available partitions:
    1f00           32768 mtdblock0
    (driver?)
    Kernel panic - not syncing: VFS: Unable to mount root fs on unknown-block(31,11)
    CPU1: stopping
    CPU: 1 PID: 0 Comm: 

This affects OpenWrt since the commit d6a9a92e3217 ("kernel: bump 5.4 to 5.4.69") because this change was backported to Linux v5.4.69. Reverting this change fixes the problem for me.

And if I see it correctly, this is also affecting the kernel (4.14) for the OpenWrt 18.06.x + 19.07.x branch because it can also be found in there as part of the v4.14.200 release.

Another workaround is to replace the first "(" with a NULL too. See the attached patch for the one which I used to fix the OpenWrt bootup.

Kind regards,
Sven

From: Sven Eckelmann <[email protected]>
Date: Sun, 22 Nov 2020 00:48:33 +0100
Subject: [PATCH RFC] mtd: parser: cmdline: Fix parsing of part-names with colons

Some devices (especially QCA ones) are already using hardcoded partition names with colons in it. The OpenMesh A62 for example provides following mtd relevant information via cmdline:

root=31:11 mtdparts=spi0.0:256k(0:SBL1),128k(0:MIBIB),384k(0:QSEE),64k(0:CDT),64k(0:DDRPARAMS),64k(0:APPSBLENV),512k(0:APPSBL),64k(0:ART),64k(custom),64k(0:KEYS),0x002b0000(kernel),0x00c80000(rootfs),15552k(inactive) rootfsname=rootfs rootwait

The change to split only on the last colon between mtd-id and partitions will cause newpart to see following string for the first partition:

KEYS),0x002b0000(kernel),0x00c80000(rootfs),15552k(inactive)

Such a partition list cannot be parsed and thus the device fails to boot.

Avoid this behavior by making sure that the start of the first part-name ("(") will also be the last byte the colon search algorithm is using for its mtd-id search.

index 0625b25620ca766318ea4646a6e3761ff4d3a4cc..22881ea4c132ea5a5ba7aebd025d91bf1cd023af 100644
@@ -218,7 +218,7 @@ static int mtdpart_setup_real(char *s)
 		struct cmdline_mtd_partition *this_mtd;
 		struct mtd_partition *parts;
 		int mtd_id_len, num_parts;
-		char *p, *mtd_id, *semicol;
+		char *p, *mtd_id, *semicol, *open_parenth;
 
 		/*
 		 * Replace the first ';' by a NULL char so strrchr can work

@@ -228,6 +228,13 @@ static int mtdpart_setup_real(char *s)
 		if (semicol)
 			*semicol = '\0';
 
+		/* make sure that part-names with ":" will not be handled as
+		 * part of the mtd-id with an ":"
+		 */
+		open_parenth = strchr(s, '(');
+		if (open_parenth)
+			*open_parenth = '\0';
+
 		mtd_id = s;
 
 		/*

@@ -237,6 +244,10 @@ static int mtdpart_setup_real(char *s)
 		 */
 		p = strrchr(s, ':');
 
+		/* Restore the '(' now. */
+		if (open_parenth)
+			*open_parenth = '(';
+
 		/* Restore the ';' now. */
 		if (semicol)
 			*semicol = ';';

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

I'm a bit worried about how tricky this starts to get. I'm inclined to go back to an earlier implementation which used a character that had not yet been used (iirc I used [] around the PCI ID in a very early version). What if we used, e.g, a single ! and searched for that? It need not be !; pick a character. Just something not already in use, as the ambiguity around which ':' delimits the device has become an issue, as you show.

Almost nothing in the original patch would change, save the character being searched for. By using a character we'd never used, we'd avoid breaking existing usage.

Comments?

What? Doesn't make any sense to me. The mtdparts shown in the the commit message is as it is. I cannot simply change it because it is in the control of the bootloader - not the linux kernel or me. So I can also not introduce a different character like ! for separating things.

KInd regards,
Sven

Ah ha, I see how different my world is from yours :-)

My world is linuxboot, which means the command line is always under control of linux, as linux is my bootloader. So this kind of thing is very easy for me to change.

Let me go back to the initial problem and hope I can make better sense.

The original problem was that ':' was used to separate the device from the partitions. A problem came along in that PCI device specifiers have ':', and the original parsing code broke. The patch that caused you trouble fixed that by using the right-most ':' as the separator for device and partitions.

What none of the people involved in the original patch knew was that there would be other ':' in use. Sorry!

But you are right, my idea is a complete non-starter, don't know what I was thinking.

So it seems your patch, if it works, is the way to go? I can't think of anything better that lets us preserve current behavior and support PCI device specifiers?

Ron

I am still not sure because I still didn't get what you actually wanted to change. I first thought that you wanted to change

    mtdparts=spi0.0:256k(0:SBL1)

to

    mtdparts=spi0.0!256k(0:SBL1)

which wouldn't work for me when ":" is not supported anymore. And it would break a lot of already working installations.

But maybe I completely misread it. Maybe you wanted to introduce an optional(!!!) stop marker like !

    mtdparts=spi0.0!:256k(0:SBL1)

to inform the parser that it doesn't have to search for : before the !. While this could work for me, I am not qualified enough to say which character is not yet used and can be utilized.

But the note about [ and ] at least makes sense to me (if it is optional):

    mtdparts=[spi0.0]:256k(0:SBL1)

But I am not sure if this will be a problem for people which already adopted PCI IDs inside the mtdparts without [ and ].

At least this is a workaround [1] which can be pushed to all the stable kernels which broke with the "Support MTD names containing one or more colons" patch. And the one which OpenWrt adopted now to get the devices booting again. It is only waiting for a Tested-by from you.

I am not that deep in this topic. So I am not sure what else could be done.

Kind regards,
Sven

[1] https://patchwork.ozlabs.org/project/linux-mtd/patch/[email protected]/

Thanks, Sven, for your patience, I will indeed try to test this next week.

Any test results?

Kind regards,
Sven

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

I pinged the person again. Hope to hear today. Sorry for delay.