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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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
- you remove lines here which you added in the same series.

Second one shows ordering issue of logical changes.

> 
> Signed-off-by: Yury Norov <ynorov@marvell.com>
> ---
>  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;
> +}
> +
>  /**
>   * __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
> 

-- 
With Best Regards,
Andy Shevchenko