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

[Guenter Roeck]

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)