diff mbox series

[14/24] init: clear root_wait on all invalid root= strings

Message ID 20230523074535.249802-15-hch@lst.de (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series [01/24] driver core: return bool from driver_probe_done | expand

Commit Message

Christoph Hellwig May 23, 2023, 7:45 a.m. UTC
Instead of only clearing root_wait in devt_from_partuuid when the UUID
format was invalid, do that in parse_root_device for all strings that
failed to parse.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 init/do_mounts.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

Comments

Guenter Roeck June 21, 2023, 9:07 p.m. UTC | #1
Hi,

On Tue, May 23, 2023 at 09:45:25AM +0200, Christoph Hellwig wrote:
> Instead of only clearing root_wait in devt_from_partuuid when the UUID
> format was invalid, do that in parse_root_device for all strings that
> failed to parse.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

In linux-next, almost all of my boot tests from usb drives as well
as a few others fail with "Disabling rootwait; root= is invalid."
in the log. Bisect points to this patch.

It can not easily be reverted, but the following change fixes the problem.

--- a/init/do_mounts.c
+++ b/init/do_mounts.c
@@ -413,10 +413,12 @@ static dev_t __init parse_root_device(char *root_device_name)
 
        error = early_lookup_bdev(root_device_name, &dev);
        if (error) {
+#if 0
                if (error == -EINVAL && root_wait) {
                        pr_err("Disabling rootwait; root= is invalid.\n");
                        root_wait = 0;
                }
+#endif
                return 0;
        }
        return dev;

Debugging shows that early_lookup_bdev() indeed returns -EINVAL.
Looking into it further, it turns out that devt_from_devname() returns
-EINVAL for root devices such as
	root=/dev/sda
if the device is not found, making it impossible to rootwait for such
a device (this might for example be a raw USB drive without partitions,
or any qemu drive with format=raw).

Guenter

---
# bad: [15e71592dbae49a674429c618a10401d7f992ac3] Add linux-next specific files for 20230621
# good: [45a3e24f65e90a047bef86f927ebdc4c710edaa1] Linux 6.4-rc7
git bisect start 'HEAD' 'v6.4-rc7'
# good: [e867e67cd55ae460c860ffd896c7fc96add2821c] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git
git bisect good e867e67cd55ae460c860ffd896c7fc96add2821c
# bad: [0ab4015a11182e2a19c3dd52db85418f370cef39] Merge branch 'for-next' of git://git.kernel.dk/linux-block.git
git bisect bad 0ab4015a11182e2a19c3dd52db85418f370cef39
# good: [901bdf5ea1a836400ee69aa32b04e9c209271ec7] Merge tag 'amd-drm-next-6.5-2023-06-09' of https://gitlab.freedesktop.org/agd5f/linux into drm-next
git bisect good 901bdf5ea1a836400ee69aa32b04e9c209271ec7
# good: [07164956fbc26eff280f3a044a489460ae36413c] Merge branch 'for-next' of https://git.kernel.org/pub/scm/linux/kernel/git/ieee1394/linux1394.git
git bisect good 07164956fbc26eff280f3a044a489460ae36413c
# good: [3067e020d361ed346957eb5e253911f7a3e18f59] add snd_soc_{of_}get_dlc()
git bisect good 3067e020d361ed346957eb5e253911f7a3e18f59
# bad: [0dbbd269fb6a8799f312dfc9b1ae1244a144cfc6] Merge branch 'for-6.5/block' into for-next
git bisect bad 0dbbd269fb6a8799f312dfc9b1ae1244a144cfc6
# good: [6c500000af037f74b66dd01b565c8ee1b501cc1b] block: mark bio_add_folio as __must_check
git bisect good 6c500000af037f74b66dd01b565c8ee1b501cc1b
# bad: [1a0ddd56e545b743af510b5a1b8dbdfe7d35cd3b] pktcdvd: replace sscanf() by kstrtoul()
git bisect bad 1a0ddd56e545b743af510b5a1b8dbdfe7d35cd3b
# good: [e3102722ffe77094ba9e7e46380792b3dd8a7abd] init: rename mount_block_root to mount_root_generic
git bisect good e3102722ffe77094ba9e7e46380792b3dd8a7abd
# bad: [d4a28d7defe79006e59293a4b43d518ba8483fb0] dm: remove dm_get_dev_t
git bisect bad d4a28d7defe79006e59293a4b43d518ba8483fb0
# good: [c0c1a7dcb6f5db4500e6574294674213bc24940c] init: move the nfs/cifs/ram special cases out of name_to_dev_t
git bisect good c0c1a7dcb6f5db4500e6574294674213bc24940c
# bad: [702f3189e454b3c3c2f3c99dbf30acf41aab707c] block: move the code to do early boot lookup of block devices to block/
git bisect bad 702f3189e454b3c3c2f3c99dbf30acf41aab707c
# bad: [079caa35f7863cd9958b4555ae873ea4d352a502] init: clear root_wait on all invalid root= strings
git bisect bad 079caa35f7863cd9958b4555ae873ea4d352a502
# good: [cf056a43121559d3642419917d405c3237ded90a] init: improve the name_to_dev_t interface
git bisect good cf056a43121559d3642419917d405c3237ded90a
# first bad commit: [079caa35f7863cd9958b4555ae873ea4d352a502] init: clear root_wait on all invalid root= strings
Christoph Hellwig June 22, 2023, 3:51 a.m. UTC | #2
On Wed, Jun 21, 2023 at 02:07:13PM -0700, Guenter Roeck wrote:
> On Tue, May 23, 2023 at 09:45:25AM +0200, Christoph Hellwig wrote:
> > Instead of only clearing root_wait in devt_from_partuuid when the UUID
> > format was invalid, do that in parse_root_device for all strings that
> > failed to parse.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> In linux-next, almost all of my boot tests from usb drives as well
> as a few others fail with "Disabling rootwait; root= is invalid."
> in the log. Bisect points to this patch.

Can you send such a log, and the command line you've used?
Guenter Roeck June 22, 2023, 4:28 a.m. UTC | #3
On 6/21/23 20:51, Christoph Hellwig wrote:
> On Wed, Jun 21, 2023 at 02:07:13PM -0700, Guenter Roeck wrote:
>> On Tue, May 23, 2023 at 09:45:25AM +0200, Christoph Hellwig wrote:
>>> Instead of only clearing root_wait in devt_from_partuuid when the UUID
>>> format was invalid, do that in parse_root_device for all strings that
>>> failed to parse.
>>>
>>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>>
>> In linux-next, almost all of my boot tests from usb drives as well
>> as a few others fail with "Disabling rootwait; root= is invalid."
>> in the log. Bisect points to this patch.
> 
> Can you send such a log, and the command line you've used?
> 


There are lots of logs at https://kerneltests.org/builders, in the 'next'
column of qemu tests. One example is
https://kerneltests.org/builders/qemu-arm-v7-next/builds/511/steps/qemubuildcommand/logs/stdio

Sample command line:

qemu-system-arm -M mcimx7d-sabre \
      -kernel arch/arm/boot/zImage -no-reboot -snapshot \
      -usb -device usb-storage,drive=d0,bus=usb-bus.1 \
      -drive file=rootfs-armv7a.ext2,if=none,id=d0,format=raw \
      -m 256 -nic user -display none \
      --append "root=/dev/sda rootwait earlycon=ec_imx6q,mmio,0x30860000,115200n8 console=ttymxc0,115200" \
      -dtb arch/arm/boot/dts/imx7d-sdb.dtb \
      -nographic -monitor null -serial stdio

This is with a modified imx_v6_v7_defconfig and root file system from
https://github.com/groeck/linux-build-test/blob/master/rootfs/arm-v7/rootfs-armv7a.ext2.gz

The -EINVAL return value is from

	if (p < s + 2 || !isdigit(p[-2]) || p[-1] != 'p')

in devt_from_devname().

Guenter
Christoph Hellwig June 22, 2023, 6 a.m. UTC | #4
Hi Guenter,

can you try this patch?

diff --git a/block/early-lookup.c b/block/early-lookup.c
index a5be3c68ed079c..66e4514d671179 100644
--- a/block/early-lookup.c
+++ b/block/early-lookup.c
@@ -174,7 +174,7 @@ static int __init devt_from_devname(const char *name, dev_t *devt)
 	while (p > s && isdigit(p[-1]))
 		p--;
 	if (p == s || !*p || *p == '0')
-		return -EINVAL;
+		return -ENODEV;
 
 	/* try disk name without <part number> */
 	part = simple_strtoul(p, NULL, 10);
Guenter Roeck June 22, 2023, 1:54 p.m. UTC | #5
On 6/21/23 23:00, Christoph Hellwig wrote:
> Hi Guenter,
> 
> can you try this patch?
> 
> diff --git a/block/early-lookup.c b/block/early-lookup.c
> index a5be3c68ed079c..66e4514d671179 100644
> --- a/block/early-lookup.c
> +++ b/block/early-lookup.c
> @@ -174,7 +174,7 @@ static int __init devt_from_devname(const char *name, dev_t *devt)
>   	while (p > s && isdigit(p[-1]))
>   		p--;
>   	if (p == s || !*p || *p == '0')
> -		return -EINVAL;
> +		return -ENODEV;
>   
>   	/* try disk name without <part number> */
>   	part = simple_strtoul(p, NULL, 10);

Not completely. Tests with root=/dev/sda still fail.

"name" passed to devt_from_devname() is "sda".

        for (p = s; *p; p++) {
                 if (*p == '/')
                         *p = '!';
         }

advances 'p' to the end of the string.

         while (p > s && isdigit(p[-1]))
		p--;

moves it back to point to the first digit (if there is one).

         if (p == s || !*p || *p == '0')
		return -EINVAL;

then fails because *p is 0. In other words, the function only accepts
drive names with digits at the end (and the first digit must not be '0').

I don't recall how I hit the other condition earlier. I have various
"/dev/mmcblkX" in my tests, where X can be any number including 0.
Maybe those fail randomly as well.

Overall I am not sure though what an "invalid" devicename is supposed
to be in this context. I have "sda", "sr0", "vda", "mtdblkX",
"nvme0n1", "mmcblkX", and "hda". Why would any of those not be eligible
for "rootwait" ?

In practice, everything not ending with a digit, or ending with
'0', fails the first test. Everything ending with a digit > 0
fails the second test. But "humptydump3p4" passes all those tests.

Guenter

---
#include <stdio.h>
#include <string.h>
#include <ctype.h>
#include <stdlib.h>

#define EINVAL1	1
#define EINVAL2	2
#define EINVAL3	3
#define ENODEV	4

static int devt_from_devname(const char *name)
{
         int part;
         char s[32];
         char *p;

         if (strlen(name) > 31)
                 return EINVAL1;

         strcpy(s, name);
         for (p = s; *p; p++) {
                 if (*p == '/')
                         *p = '!';
         }

         /*
          * Try non-existent, but valid partition, which may only exist after
          * opening the device, like partitioned md devices.
          */
         while (p > s && isdigit(p[-1]))
                 p--;
         if (p == s || !*p || *p == '0') {
                 return EINVAL2;
         }

         /* try disk name without <part number> */
         part = strtoul(p, NULL, 10);
         *p = '\0';

         /* try disk name without p<part number> */
         if (p < s + 2 || !isdigit(p[-2]) || p[-1] != 'p') {
                 return EINVAL3;
         }
         return ENODEV;
}

char *devnames[] = {
     "sda",
     "sda1",
     "mmcblk0",
     "mmcblk1",
     "mtdblk0",
     "mtdblk1",
     "vda",
     "hda",
     "nvme0n1",
     "sr0",
     "sr1",
     "humptydump3p4",
     NULL
};

int main(int argc, char **argv)
{
	char *str;
	int i;

	for (i = 0, str = devnames[0]; str; str = devnames[++i]) {
	    printf("%s: %d\n", str, devt_from_devname(str));
	}
}
Christoph Hellwig June 22, 2023, 2:40 p.m. UTC | #6
On Thu, Jun 22, 2023 at 06:54:41AM -0700, Guenter Roeck wrote:
> On 6/21/23 23:00, Christoph Hellwig wrote:
>> Hi Guenter,
>>
>> can you try this patch?
>>
>> diff --git a/block/early-lookup.c b/block/early-lookup.c
>> index a5be3c68ed079c..66e4514d671179 100644
>> --- a/block/early-lookup.c
>> +++ b/block/early-lookup.c
>> @@ -174,7 +174,7 @@ static int __init devt_from_devname(const char *name, dev_t *devt)
>>   	while (p > s && isdigit(p[-1]))
>>   		p--;
>>   	if (p == s || !*p || *p == '0')
>> -		return -EINVAL;
>> +		return -ENODEV;
>>     	/* try disk name without <part number> */
>>   	part = simple_strtoul(p, NULL, 10);
>
> Not completely. Tests with root=/dev/sda still fail.
>
> "name" passed to devt_from_devname() is "sda".
>
>        for (p = s; *p; p++) {
>                 if (*p == '/')
>                         *p = '!';
>         }
>
> advances 'p' to the end of the string.
>
>         while (p > s && isdigit(p[-1]))
> 		p--;
>
> moves it back to point to the first digit (if there is one).
>
>         if (p == s || !*p || *p == '0')
> 		return -EINVAL;
>
> then fails because *p is 0. In other words, the function only accepts
> drive names with digits at the end (and the first digit must not be '0').
>
> I don't recall how I hit the other condition earlier. I have various
> "/dev/mmcblkX" in my tests, where X can be any number including 0.
> Maybe those fail randomly as well.
>
> Overall I am not sure though what an "invalid" devicename is supposed
> to be in this context. I have "sda", "sr0", "vda", "mtdblkX",
> "nvme0n1", "mmcblkX", and "hda". Why would any of those not be eligible
> for "rootwait" ?
>
> In practice, everything not ending with a digit, or ending with
> '0', fails the first test. Everything ending with a digit > 0
> fails the second test. But "humptydump3p4" passes all those tests.


Yeah.  I guess I should give up on the idea of error out in this
particular parser.  The idea sounded good, but I guess it doesn't
work.  So we'll probably want his fix:


diff --git a/block/early-lookup.c b/block/early-lookup.c
index a5be3c68ed079c..9e2d5a19de1b3b 100644
--- a/block/early-lookup.c
+++ b/block/early-lookup.c
@@ -174,7 +174,7 @@ static int __init devt_from_devname(const char *name, dev_t *devt)
 	while (p > s && isdigit(p[-1]))
 		p--;
 	if (p == s || !*p || *p == '0')
-		return -EINVAL;
+		return -ENODEV;
 
 	/* try disk name without <part number> */
 	part = simple_strtoul(p, NULL, 10);
@@ -185,7 +185,7 @@ static int __init devt_from_devname(const char *name, dev_t *devt)
 
 	/* try disk name without p<part number> */
 	if (p < s + 2 || !isdigit(p[-2]) || p[-1] != 'p')
-		return -EINVAL;
+		return -ENODEV;
 	p[-1] = '\0';
 	*devt = blk_lookup_devt(s, part);
 	if (*devt)
Guenter Roeck June 22, 2023, 2:57 p.m. UTC | #7
On 6/22/23 07:40, Christoph Hellwig wrote:
> On Thu, Jun 22, 2023 at 06:54:41AM -0700, Guenter Roeck wrote:
>> On 6/21/23 23:00, Christoph Hellwig wrote:
>>> Hi Guenter,
>>>
>>> can you try this patch?
>>>
>>> diff --git a/block/early-lookup.c b/block/early-lookup.c
>>> index a5be3c68ed079c..66e4514d671179 100644
>>> --- a/block/early-lookup.c
>>> +++ b/block/early-lookup.c
>>> @@ -174,7 +174,7 @@ static int __init devt_from_devname(const char *name, dev_t *devt)
>>>    	while (p > s && isdigit(p[-1]))
>>>    		p--;
>>>    	if (p == s || !*p || *p == '0')
>>> -		return -EINVAL;
>>> +		return -ENODEV;
>>>      	/* try disk name without <part number> */
>>>    	part = simple_strtoul(p, NULL, 10);
>>
>> Not completely. Tests with root=/dev/sda still fail.
>>
>> "name" passed to devt_from_devname() is "sda".
>>
>>         for (p = s; *p; p++) {
>>                  if (*p == '/')
>>                          *p = '!';
>>          }
>>
>> advances 'p' to the end of the string.
>>
>>          while (p > s && isdigit(p[-1]))
>> 		p--;
>>
>> moves it back to point to the first digit (if there is one).
>>
>>          if (p == s || !*p || *p == '0')
>> 		return -EINVAL;
>>
>> then fails because *p is 0. In other words, the function only accepts
>> drive names with digits at the end (and the first digit must not be '0').
>>
>> I don't recall how I hit the other condition earlier. I have various
>> "/dev/mmcblkX" in my tests, where X can be any number including 0.
>> Maybe those fail randomly as well.
>>
>> Overall I am not sure though what an "invalid" devicename is supposed
>> to be in this context. I have "sda", "sr0", "vda", "mtdblkX",
>> "nvme0n1", "mmcblkX", and "hda". Why would any of those not be eligible
>> for "rootwait" ?
>>
>> In practice, everything not ending with a digit, or ending with
>> '0', fails the first test. Everything ending with a digit > 0
>> fails the second test. But "humptydump3p4" passes all those tests.
> 
> 
> Yeah.  I guess I should give up on the idea of error out in this
> particular parser.  The idea sounded good, but I guess it doesn't
> work.  So we'll probably want his fix:
> 

Yes, that fixes the problem for me.

Guenter

> 
> diff --git a/block/early-lookup.c b/block/early-lookup.c
> index a5be3c68ed079c..9e2d5a19de1b3b 100644
> --- a/block/early-lookup.c
> +++ b/block/early-lookup.c
> @@ -174,7 +174,7 @@ static int __init devt_from_devname(const char *name, dev_t *devt)
>   	while (p > s && isdigit(p[-1]))
>   		p--;
>   	if (p == s || !*p || *p == '0')
> -		return -EINVAL;
> +		return -ENODEV;
>   
>   	/* try disk name without <part number> */
>   	part = simple_strtoul(p, NULL, 10);
> @@ -185,7 +185,7 @@ static int __init devt_from_devname(const char *name, dev_t *devt)
>   
>   	/* try disk name without p<part number> */
>   	if (p < s + 2 || !isdigit(p[-2]) || p[-1] != 'p')
> -		return -EINVAL;
> +		return -ENODEV;
>   	p[-1] = '\0';
>   	*devt = blk_lookup_devt(s, part);
>   	if (*devt)
diff mbox series

Patch

diff --git a/init/do_mounts.c b/init/do_mounts.c
index f1953aeb321978..0b36a5f39ee8e2 100644
--- a/init/do_mounts.c
+++ b/init/do_mounts.c
@@ -112,14 +112,14 @@  static int devt_from_partuuid(const char *uuid_str, dev_t *devt)
 
 		/* Explicitly fail on poor PARTUUID syntax. */
 		if (sscanf(slash + 1, "PARTNROFF=%d%c", &offset, &c) != 1)
-			goto clear_root_wait;
+			goto out_invalid;
 		cmp.len = slash - uuid_str;
 	} else {
 		cmp.len = strlen(uuid_str);
 	}
 
 	if (!cmp.len)
-		goto clear_root_wait;
+		goto out_invalid;
 
 	dev = class_find_device(&block_class, NULL, &cmp, &match_dev_by_uuid);
 	if (!dev)
@@ -139,12 +139,9 @@  static int devt_from_partuuid(const char *uuid_str, dev_t *devt)
 	put_device(dev);
 	return 0;
 
-clear_root_wait:
+out_invalid:
 	pr_err("VFS: PARTUUID= is invalid.\n"
 	       "Expected PARTUUID=<valid-uuid-id>[/PARTNROFF=%%d]\n");
-	if (root_wait)
-		pr_err("Disabling rootwait; root= is invalid.\n");
-	root_wait = 0;
 	return -EINVAL;
 }
 
@@ -611,6 +608,7 @@  static void __init wait_for_root(char *root_device_name)
 
 static dev_t __init parse_root_device(char *root_device_name)
 {
+	int error;
 	dev_t dev;
 
 	if (!strncmp(root_device_name, "mtd", 3) ||
@@ -623,8 +621,14 @@  static dev_t __init parse_root_device(char *root_device_name)
 	if (strcmp(root_device_name, "/dev/ram") == 0)
 		return Root_RAM0;
 
-	if (early_lookup_bdev(root_device_name, &dev))
+	error = early_lookup_bdev(root_device_name, &dev);
+	if (error) {
+		if (error == -EINVAL && root_wait) {
+			pr_err("Disabling rootwait; root= is invalid.\n");
+			root_wait = 0;
+		}
 		return 0;
+	}
 	return dev;
 }