Re: [PATCH 3/4] bitmap_parselist: rework input string parser

[Yuri Norov]

On Wed, Jan 09, 2019 at 06:01:46PM +0200, Andy Shevchenko wrote:
> On Sun, Dec 23, 2018 at 09:44:55AM +0000, Yuri Norov wrote:
> > The requirement for this rework is to keep the __bitmap_parselist()
> > copy-less and single-pass but make it more readable and maintainable by
> > splitting into logical parts and removing explicit nested cycles and
> > opaque local variables.
> > 
> > __bitmap_parselist() can parse userspace inputs and therefore we cannot
> > use simple_strtoul() to parse numbers.
> 
> I see two issues with this patch:
> - you are using IS_ERR() but ain't utilizing PTR_ERR(), ERR_PTR() ones

OK. Will use them in v2.

> - you remove lines here which you added in the same series.
>
> Second one shows ordering issue of logical changes.

Do you mean this chunk?

> > -		r.start = a;
> > -		r.off = used_size;
> > -		r.grlen = group_size;
> > -		r.end = b;

It's because I split refactoring into 2 parts for the sake of
readability. Patch #2 may be applied independently if #3 will
be considered inappropriate, that's why I ordered it prior to
#3, and it caused the need for removing that lines. I don't
think it's too ugly though, and I'd prefer to keep 2 and 3
separated with the cost of this little mess.

Reversing the order looks tricky at the first glance, but I'm
OK to join #2 and #3 if it will be considered worthy. Would
work both ways.

> > Signed-off-by: Yury Norov <ynorov@xxxxxxxxxxx>
> > ---
> >  lib/bitmap.c | 247 ++++++++++++++++++++++++++++++---------------------
> >  1 file changed, 148 insertions(+), 99 deletions(-)
> > 
> > diff --git a/lib/bitmap.c b/lib/bitmap.c
> > index a60fd9723677..edc7068c28a6 100644
> > --- a/lib/bitmap.c
> > +++ b/lib/bitmap.c
> > @@ -513,6 +513,140 @@ static int bitmap_check_region(const struct region *r)
> >  	return 0;
> >  }
> >  
> > +static long get_char(char *c, const char *str, int is_user)
> > +{
> > +	if (unlikely(is_user))
> > +		return __get_user(*c, str);
> > +
> > +	*c = *str;
> > +	return 0;
> > +}
> > +
> > +static const char *bitmap_getnum(unsigned int *num, const char *str,
> > +			    const char *const end, int is_user)
> > +{
> > +	unsigned int n = 0;
> > +	const char *_str;
> > +	long ret;
> > +	char c;
> > +
> > +	for (_str = str, *num = 0; _str <= end; _str++) {
> > +		ret = get_char(&c, _str, is_user);
> > +		if (ret)
> > +			return (char *)ret;
> > +
> > +		if (!isdigit(c)) {
> > +			if (_str == str)
> > +				return (char *)-EINVAL;
> > +
> > +			goto out;
> > +		}
> > +
> > +		*num = *num * 10 + (c - '0');
> > +		if (*num < n)
> > +			return (char *)-EOVERFLOW;
> > +
> > +		n = *num;
> > +	}
> > +
> > +out:
> > +	return _str;
> > +}
> > +
> > +static const char *bitmap_find_region(const char *str,
> > +			const char *const end, int is_user)
> > +{
> > +	long ret;
> > +	char c;
> > +
> > +	for (; str < end; str++) {
> > +		ret = get_char(&c, str, is_user);
> > +		if (ret)
> > +			return (char *)ret;
> > +
> > +		/* Unexpected end of line. */
> > +		if (c == 0 || c == '\n')
> > +			return NULL;
> > +
> > +		/*
> > +		 * The format allows commas and whitespases
> > +		 * at the beginning of the region, so skip it.
> > +		 */
> > +		if (!isspace(c) && c != ',')
> > +			break;
> > +	}
> > +
> > +	return str;
> > +}
> > +
> > +static const char *bitmap_parse_region(struct region *r, const char *str,
> > +				 const char *const end, int is_user)
> > +{
> > +	long ret;
> > +	char c;
> > +
> > +	str = bitmap_getnum(&r->start, str, end, is_user);
> > +	if (IS_ERR(str))
> > +		return str;
> > +
> > +	ret = get_char(&c, str, is_user);
> > +	if (ret)
> > +		return (char *)ret;
> > +
> > +	if (c == 0 || c == '\n') {
> > +		str = end + 1;
> > +		goto no_end;
> > +	}
> > +
> > +	if (isspace(c) || c == ',')
> > +		goto no_end;
> > +
> > +	if (c != '-')
> > +		return (char *)-EINVAL;
> > +
> > +	str = bitmap_getnum(&r->end, str + 1, end, is_user);
> > +	if (IS_ERR(str))
> > +		return str;
> > +
> > +	ret = get_char(&c, str, is_user);
> > +	if (ret)
> > +		return (char *)ret;
> > +
> > +	if (c == 0 || c == '\n') {
> > +		str = end + 1;
> > +		goto no_pattern;
> > +	}
> > +
> > +	if (isspace(c) || c == ',')
> > +		goto no_pattern;
> > +
> > +	if (c != ':')
> > +		return (char *)-EINVAL;
> > +
> > +	str = bitmap_getnum(&r->off, str + 1, end, is_user);
> > +	if (IS_ERR(str))
> > +		return str;
> > +
> > +	ret = get_char(&c, str, is_user);
> > +	if (ret)
> > +		return (char *)ret;
> > +
> > +	if (c != '/')
> > +		return (char *)-EINVAL;
> > +
> > +	str = bitmap_getnum(&r->grlen, str + 1, end, is_user);
> > +
> > +	return str;
> > +
> > +no_end:
> > +	r->end = r->start;
> > +no_pattern:
> > +	r->off = r->end + 1;
> > +	r->grlen = r->end + 1;
> > +
> > +	return (char *)str;

This cast looks unneeded.

> > +}
> > +
> >  /**
> >   * __bitmap_parselist - convert list format ASCII string to bitmap
> >   * @buf: read nul-terminated user string from this buffer
> > @@ -534,113 +668,28 @@ static int bitmap_check_region(const struct region *r)
> >   *
> >   * Returns: 0 on success, -errno on invalid input strings. Error values:
> >   *
> > - *   - ``-EINVAL``: second number in range smaller than first
> > + *   - ``-EINVAL``: wrong region format
> >   *   - ``-EINVAL``: invalid character in string
> >   *   - ``-ERANGE``: bit number specified too large for mask
> > + *   - ``-EOVERFLOW``: integer overflow in the input parameters
> >   */
> > -static int __bitmap_parselist(const char *buf, unsigned int buflen,
> > +static int __bitmap_parselist(const char *buf, const char *const end,
> >  		int is_user, unsigned long *maskp,
> >  		int nmaskbits)
> >  {
> > -	unsigned int a, b, old_a, old_b;
> > -	unsigned int group_size, used_size;
> > -	int c, old_c, totaldigits, ndigits;
> > -	const char __user __force *ubuf = (const char __user __force *)buf;
> > -	int at_start, in_range, in_partial_range, ret;
> >  	struct region r;
> > +	long ret;
> >  
> > -	totaldigits = c = 0;
> > -	old_a = old_b = 0;
> > -	group_size = used_size = 0;
> >  	bitmap_zero(maskp, nmaskbits);
> > -	do {
> > -		at_start = 1;
> > -		in_range = 0;
> > -		in_partial_range = 0;
> > -		a = b = 0;
> > -		ndigits = totaldigits;
> > -
> > -		/* Get the next cpu# or a range of cpu#'s */
> > -		while (buflen) {
> > -			old_c = c;
> > -			if (is_user) {
> > -				if (__get_user(c, ubuf++))
> > -					return -EFAULT;
> > -			} else
> > -				c = *buf++;
> > -			buflen--;
> > -			if (isspace(c))
> > -				continue;
> > -
> > -			/* A '\0' or a ',' signal the end of a cpu# or range */
> > -			if (c == '\0' || c == ',')
> > -				break;
> > -			/*
> > -			* whitespaces between digits are not allowed,
> > -			* but it's ok if whitespaces are on head or tail.
> > -			* when old_c is whilespace,
> > -			* if totaldigits == ndigits, whitespace is on head.
> > -			* if whitespace is on tail, it should not run here.
> > -			* as c was ',' or '\0',
> > -			* the last code line has broken the current loop.
> > -			*/
> > -			if ((totaldigits != ndigits) && isspace(old_c))
> > -				return -EINVAL;
> > -
> > -			if (c == '/') {
> > -				used_size = a;
> > -				at_start = 1;
> > -				in_range = 0;
> > -				a = b = 0;
> > -				continue;
> > -			}
> > -
> > -			if (c == ':') {
> > -				old_a = a;
> > -				old_b = b;
> > -				at_start = 1;
> > -				in_range = 0;
> > -				in_partial_range = 1;
> > -				a = b = 0;
> > -				continue;
> > -			}
> > -
> > -			if (c == '-') {
> > -				if (at_start || in_range)
> > -					return -EINVAL;
> > -				b = 0;
> > -				in_range = 1;
> > -				at_start = 1;
> > -				continue;
> > -			}
> >  
> > -			if (!isdigit(c))
> > -				return -EINVAL;
> > -
> > -			b = b * 10 + (c - '0');
> > -			if (!in_range)
> > -				a = b;
> > -			at_start = 0;
> > -			totaldigits++;
> > -		}
> > -		if (ndigits == totaldigits)
> > -			continue;
> > -		if (in_partial_range) {
> > -			group_size = a;
> > -			a = old_a;
> > -			b = old_b;
> > -			old_a = old_b = 0;
> > -		} else {
> > -			used_size = group_size = b - a + 1;
> > -		}
> > -		/* if no digit is after '-', it's wrong*/
> > -		if (at_start && in_range)
> > -			return -EINVAL;
> > +	while (buf && buf < end) {
> > +		buf = bitmap_find_region(buf, end, is_user);
> > +		if (buf == NULL)
> > +			return 0;
> >  
> > -		r.start = a;
> > -		r.off = used_size;
> > -		r.grlen = group_size;
> > -		r.end = b;
> > +		buf = bitmap_parse_region(&r, buf, end, is_user);
> > +		if (IS_ERR(buf))
> > +			return (long)buf;
> >  
> >  		ret = bitmap_check_region(&r);
> >  		if (ret)
> > @@ -649,14 +698,14 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
> >  		ret = bitmap_set_region(&r, maskp, nmaskbits);
> >  		if (ret)
> >  			return ret;
> > +	}
> >  
> > -	} while (buflen && c == ',');
> >  	return 0;
> >  }
> >  
> >  int bitmap_parselist(const char *bp, unsigned long *maskp, int nmaskbits)
> >  {
> > -	return __bitmap_parselist(bp, UINT_MAX, 0, maskp, nmaskbits);
> > +	return __bitmap_parselist(bp, (char *)SIZE_MAX, 0, maskp, nmaskbits);
> >  }
> >  EXPORT_SYMBOL(bitmap_parselist);
> >  
> > @@ -683,7 +732,7 @@ int bitmap_parselist_user(const char __user *ubuf,
> >  	if (!access_ok(VERIFY_READ, ubuf, ulen))
> >  		return -EFAULT;
> >  	return __bitmap_parselist((const char __force *)ubuf,
> > -					ulen, 1, maskp, nmaskbits);
> > +					ubuf + ulen - 1, 1, maskp, nmaskbits);
> >  }
> >  EXPORT_SYMBOL(bitmap_parselist_user);
> >  
> > -- 
> > 2.17.1
> > 
> 

Thanks,
Yury