Messages in this thread Patch in this message | | | Date | Wed, 9 Jan 2019 18:01:46 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH 3/4] bitmap_parselist: rework input string parser |
| |
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
| |