Re: [PATCH 3/4] bitmap_parselist: rework input string parser
On Wed, Jan 09, 2019 at 06:01:46PM +0200, Andy Shevchenko wrote:
> On Sun, Dec 23, 2018 at 09:44:55AM +, 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
> > ---
> > 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(, _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(, 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(>start, str, end, is_user);
> > + if (IS_ERR(str))
> > + return str;
> > +
> > + ret = get_char(, 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(>end, str + 1, end, is_user);
> > + if (IS_ERR(str))
> > + return str;
> > +
> > + ret = get_char(, 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(>off, str + 1, end, is_user);
> > + if (IS_ERR(str))
> > + return str;
> > +
> > + ret = get_char(, str, is_user);
> > + if (ret)
> > + return (char *)ret;
> > +
> > + if (c != '/')
> > +
Re: [PATCH 3/4] bitmap_parselist: rework input string parser
On Sun, Dec 23, 2018 at 09:44:55AM +, 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
> ---
> 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(, _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(, 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(>start, str, end, is_user);
> + if (IS_ERR(str))
> + return str;
> +
> + ret = get_char(, 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(>end, str + 1, end, is_user);
> + if (IS_ERR(str))
> + return str;
> +
> + ret = get_char(, 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(>off, str + 1, end, is_user);
> + if (IS_ERR(str))
> + return str;
> +
> + ret = get_char(, str, is_user);
> + if (ret)
> + return (char *)ret;
> +
> + if (c != '/')
> + return (char *)-EINVAL;
> +
> + str = bitmap_getnum(>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
>
Re: [PATCH 3/4] bitmap_parselist: rework input string parser
Hi Yuri, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on linus/master] [also build test WARNING on v4.20-rc7 next-20181221] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Yuri-Norov/rework-bitmap_parselist/20181223-175529 reproduce: make htmldocs All warnings (new ones prefixed by >>): WARNING: convert(1) not found, for SVG to PDF conversion install ImageMagick (https://www.imagemagick.org) lib/bitmap.c:679: warning: Excess function parameter 'buflen' description in '__bitmap_parselist' lib/bitmap.c:680: warning: Excess function parameter 'buflen' description in '__bitmap_parselist' >> lib/bitmap.c:680: warning: Function parameter or member 'end' not described >> in '__bitmap_parselist' lib/bitmap.c:680: warning: Excess function parameter 'buflen' description in '__bitmap_parselist' include/linux/rcutree.h:1: warning: no structured comments found kernel/rcu/tree.c:684: warning: Excess function parameter 'irq' description in 'rcu_nmi_exit' include/linux/srcu.h:175: warning: Function parameter or member 'p' not described in 'srcu_dereference_notrace' include/linux/srcu.h:175: warning: Function parameter or member 'sp' not described in 'srcu_dereference_notrace' include/linux/gfp.h:1: warning: no structured comments found include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats ' include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats ' include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats ' include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats ' include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats ' include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats ' include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats ' include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats ' include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats ' include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats ' include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats ' include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats ' include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats ' include/net/cfg80211.h:4439: warning: Function parameter or member 'wext.ibss' not described in 'wireless_dev' include/net/cfg80211.h:4439: warning: Function parameter or member 'wext.connect' not described in 'wireless_dev' include/net/cfg80211.h:4439: warning: Function parameter or member 'wext.keys' not described in 'wireless_dev' include/net/cfg80211.h:4439: warning: Function parameter or member 'wext.ie' not described in 'wireless_dev' include/net/cfg80211.h:4439: warning: Function parameter or member 'wext.ie_len' not described in 'wireless_dev' include/net/cfg80211.h:4439: warning: Function parameter or member 'wext.bssid' not described in 'wireless_dev' include/net/cfg80211.h:4439: warning: Function parameter or member 'wext.ssid' not described in 'wireless_dev' include/net/cfg80211.h:4439: warning: Function parameter or member 'wext.default_key' not described in 'wireless_dev' include/net/cfg80211.h:4439: warning: Function parameter or member 'wext.default_mgmt_key' not described in 'wireless_dev' include/net/cfg80211.h:4439: warning: Function parameter or member 'wext.prev_bssid_valid' not described in 'wireless_dev' include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats ' include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats ' include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats ' include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats ' include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats ' include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats ' include/net/cfg80211.h:2838: warning: cannot understand function prototype: 'struct cfg80211_ftm_responder_stats '
[PATCH 3/4] bitmap_parselist: rework input string parser
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.
Signed-off-by: Yury Norov
---
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(, _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(, 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(>start, str, end, is_user);
+ if (IS_ERR(str))
+ return str;
+
+ ret = get_char(, 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(>end, str + 1, end, is_user);
+ if (IS_ERR(str))
+ return str;
+
+ ret = get_char(, 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(>off, str + 1, end, is_user);
+ if (IS_ERR(str))
+ return str;
+
+ ret = get_char(, str, is_user);
+ if (ret)
+ return (char *)ret;
+
+ if (c != '/')
+ return (char *)-EINVAL;
+
+ str = bitmap_getnum(>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;
-
