lkml.org 
[lkml]   [2019]   [Mar]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 3/6] bitmap_parselist: rework input string parser
On Tue, Mar 26, 2019 at 12:07:45AM +0300, Yury 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.

> +static long get_char(char *c, const char *str, int is_user)
> +{
> + if (unlikely(is_user))

Can is_user be boolean?

Can we name it from_user or is_from_user?

> + return __get_user(*c, (const char __force __user *)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 ERR_PTR(ret);
> +
> + if (!isdigit(c)) {
> + if (_str == str)
> + return ERR_PTR(-EINVAL);
> +
> + return _str;
> + }
> +
> + *num = *num * 10 + (c - '0');
> + if (*num < n)
> + return ERR_PTR(-EOVERFLOW);
> +
> + n = *num;
> + }

Can't we do other way around, i.e. move the loop body to a helper and do
something like this:

if (is_from_user) {
for (...) {
__get_user(...);
helper1();
helper2();
}
} else {
for (...) {
*c = *str;
helper1();
helper2()
}
}

Each branch can be optimized more I think.

> +
> + 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 ERR_PTR(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 <= end ? str : NULL;
> +}
> +
> +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 ERR_PTR(-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 ERR_PTR(ret);
> +
> + if (c == 0 || c == '\n') {
> + str = end + 1;
> + goto no_pattern;
> + }
> +
> + if (isspace(c) || c == ',')
> + goto no_pattern;
> +
> + if (c != ':')
> + return ERR_PTR(-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 ERR_PTR(-EINVAL);
> +

> + str = bitmap_getnum(&r->grlen, str + 1, end, is_user);
> +
> + return str;

return bitmap_getnum(...);

> +
> +no_end:
> + r->end = r->start;
> +no_pattern:
> + r->off = r->end + 1;
> + r->grlen = r->end + 1;
> +
> + return str;
> +}
> +

So, all above depends to what memory we access kernel / user space.
Perhaps we can get copy of memory of a given size and then parse it in kernel space always?

--
With Best Regards,
Andy Shevchenko


\
 
 \ /
  Last update: 2019-03-26 11:11    [W:0.129 / U:0.764 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site