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 <yno...@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
