[PATCH 0/4] rework bitmap_parselist

[PATCH 0/4] rework bitmap_parselist

[Yuri Norov]

bitmap_parselist evolved from a pretty simple idea for long and now lacks
for refactoring. It is not structured, has nested loops and a set of
opaque-named variables. All this leads to extremely hard understanding
of the code. Once during the optimization of it I missed a scenario which
leads to kernel hangup. Tetsuo Handa spotted this and found it simpler to
rewrite the code instead fixing it. (Though, that attempt had some flaws.)
https://lkml.org/lkml/2018/4/1/93

Things are more complicated than they may be because bitmap_parselist()
is part of user interface, and its behavior should not change.

In this patchset __bitmap_parselist() is split to a set of smaller
functions doing only one thing.

Yury Norov (4):
  bitmap_parselist: don't calculate length of the input string
  bitmap_parselist: move part of logic to helpers
  bitmap_parselist: rework input string parser
  test_bitmap: add testcases for bitmap_parselist

 lib/bitmap.c      | 294 ++++++++++++++++++++++++++++++----------------
 lib/test_bitmap.c |  18 ++-
 2 files changed, 208 insertions(+), 104 deletions(-)

-- 
2.17.1

[PATCH 1/4] bitmap_parselist: don't calculate length of the input string

[Yuri Norov]

bitmap_parselist() calculates length of the input string before passing
it to the __bitmap_parselist(). But the end-of-line condition is checked
for every character in __bitmap_parselist() anyway. So doing it in wrapper
is a simple waste of time.

Signed-off-by: Yury Norov <ynorov@marvell.com>
---
 lib/bitmap.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/lib/bitmap.c b/lib/bitmap.c
index eead55aa7170..ad43ba397c58 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -614,10 +614,7 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
 
 int bitmap_parselist(const char *bp, unsigned long *maskp, int nmaskbits)
 {
-	char *nl  = strchrnul(bp, '\n');
-	int len = nl - bp;
-
-	return __bitmap_parselist(bp, len, 0, maskp, nmaskbits);
+	return __bitmap_parselist(bp, UINT_MAX, 0, maskp, nmaskbits);
 }
 EXPORT_SYMBOL(bitmap_parselist);
 
-- 
2.17.1

[PATCH 2/4] bitmap_parselist: move part of logic to helpers

[Yuri Norov]

Move region checking and setting functionality of __bitmap_parselist()
to helpers.

Signed-off-by: Yury Norov <ynorov@marvell.com>
---
 lib/bitmap.c | 64 +++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 53 insertions(+), 11 deletions(-)

diff --git a/lib/bitmap.c b/lib/bitmap.c
index ad43ba397c58..a60fd9723677 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -477,6 +477,42 @@ int bitmap_print_to_pagebuf(bool list, char *buf, const unsigned long *maskp,
 }
 EXPORT_SYMBOL(bitmap_print_to_pagebuf);
 
+/*
+ * Region 9-38:4/10 describes the following bitmap structure:
+ * 0	   9  12    18			38
+ * .........****......****......****......
+ *	    ^  ^     ^			 ^
+ *      start  off   grlen	       end
+ */
+struct region {
+	unsigned int start;
+	unsigned int off;
+	unsigned int grlen;
+	unsigned int end;
+};
+
+static int bitmap_set_region(const struct region *r,
+				unsigned long *bitmap, int nbits)
+{
+	unsigned int start;
+
+	if (r->end >= nbits)
+		return -ERANGE;
+
+	for (start = r->start; start <= r->end; start += r->grlen)
+		bitmap_set(bitmap, start, min(r->end - start + 1, r->off));
+
+	return 0;
+}
+
+static int bitmap_check_region(const struct region *r)
+{
+	if (r->start > r->end || r->grlen == 0 || r->off > r->grlen)
+		return -EINVAL;
+
+	return 0;
+}
+
 /**
  * __bitmap_parselist - convert list format ASCII string to bitmap
  * @buf: read nul-terminated user string from this buffer
@@ -507,10 +543,11 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
 		int nmaskbits)
 {
 	unsigned int a, b, old_a, old_b;
-	unsigned int group_size, used_size, off;
+	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;
+	int at_start, in_range, in_partial_range, ret;
+	struct region r;
 
 	totaldigits = c = 0;
 	old_a = old_b = 0;
@@ -599,15 +636,20 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
 		/* if no digit is after '-', it's wrong*/
 		if (at_start && in_range)
 			return -EINVAL;
-		if (!(a <= b) || group_size == 0 || !(used_size <= group_size))
-			return -EINVAL;
-		if (b >= nmaskbits)
-			return -ERANGE;
-		while (a <= b) {
-			off = min(b - a + 1, used_size);
-			bitmap_set(maskp, a, off);
-			a += group_size;
-		}
+
+		r.start = a;
+		r.off = used_size;
+		r.grlen = group_size;
+		r.end = b;
+
+		ret = bitmap_check_region(&r);
+		if (ret)
+			return ret;
+
+		ret = bitmap_set_region(&r, maskp, nmaskbits);
+		if (ret)
+			return ret;
+
 	} while (buflen && c == ',');
 	return 0;
 }
-- 
2.17.1

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

[Yuri Norov]

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 <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

[PATCH 4/4] test_bitmap: add testcases for bitmap_parselist

[Yuri Norov]

Add tests for non-number character, empty regions, integer overflow.

Signed-off-by: Yury Norov <ynorov@marvell.com>
---
 lib/test_bitmap.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
index 6cd7d0740005..7580dd6ac599 100644
--- a/lib/test_bitmap.c
+++ b/lib/test_bitmap.c
@@ -224,7 +224,8 @@ static const unsigned long exp[] __initconst = {
 	BITMAP_FROM_U64(0xffffffff),
 	BITMAP_FROM_U64(0xfffffffe),
 	BITMAP_FROM_U64(0x3333333311111111ULL),
-	BITMAP_FROM_U64(0xffffffff77777777ULL)
+	BITMAP_FROM_U64(0xffffffff77777777ULL),
+	BITMAP_FROM_U64(0),
 };
 
 static const unsigned long exp2[] __initconst = {
@@ -247,19 +248,34 @@ static const struct test_bitmap_parselist parselist_tests[] __initconst = {
 	{0, "1-31:4/4",			&exp[9 * step], 32, 0},
 	{0, "0-31:1/4,32-63:2/4",	&exp[10 * step], 64, 0},
 	{0, "0-31:3/4,32-63:4/4",	&exp[11 * step], 64, 0},
+	{0, "  ,,  0-31:3/4  ,, 32-63:4/4  ,,  ",	&exp[11 * step], 64, 0},
 
 	{0, "0-31:1/4,32-63:2/4,64-95:3/4,96-127:4/4",	exp2, 128, 0},
 
 	{0, "0-2047:128/256", NULL, 2048, PARSE_TIME},
 
+	{0, "",				&exp[12], 8, 0},
+	{0, "\n",			&exp[12], 8, 0},
+	{0, ",,  ,,  , ,  ,",		&exp[12], 8, 0},
+	{0, " ,  ,,  , ,   ",		&exp[12], 8, 0},
+	{0, " ,  ,,  , ,   \n",		&exp[12], 8, 0},
+
 	{-EINVAL, "-1",	NULL, 8, 0},
 	{-EINVAL, "-0",	NULL, 8, 0},
 	{-EINVAL, "10-1", NULL, 8, 0},
 	{-EINVAL, "0-31:", NULL, 8, 0},
 	{-EINVAL, "0-31:0", NULL, 8, 0},
+	{-EINVAL, "0-31:0/", NULL, 8, 0},
 	{-EINVAL, "0-31:0/0", NULL, 8, 0},
 	{-EINVAL, "0-31:1/0", NULL, 8, 0},
 	{-EINVAL, "0-31:10/1", NULL, 8, 0},
+	{-EOVERFLOW, "0-98765432123456789:10/1", NULL, 8, 0},
+
+	{-EINVAL, "a-31", NULL, 8, 0},
+	{-EINVAL, "0-a1", NULL, 8, 0},
+	{-EINVAL, "a-31:10/1", NULL, 8, 0},
+	{-EINVAL, "0-31:a/1", NULL, 8, 0},
+	{-EINVAL, "0-\n", NULL, 8, 0},
 };
 
 static void __init test_bitmap_parselist(void)
-- 
2.17.1

Re: [PATCH 4/4] test_bitmap: add testcases for bitmap_parselist

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2004 bytes --]

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
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

>> lib//bitmap.c:519:24: warning: incorrect type in argument 1 (different address spaces)
   lib//bitmap.c:519:24:    expected void const volatile [noderef] <asn:1> *
   lib//bitmap.c:519:24:    got char const *str
   lib//bitmap.c:735:53: warning: incorrect type in argument 2 (different address spaces)
   lib//bitmap.c:735:53:    expected char const *const end
   lib//bitmap.c:735:53:    got char const [noderef] <asn:1> *
   include/linux/slab.h:332:43: warning: dubious: x & !y

vim +519 lib//bitmap.c

87ac23b8 Yuri Norov 2018-12-23  515  
32eb0e6b Yuri Norov 2018-12-23  516  static long get_char(char *c, const char *str, int is_user)
32eb0e6b Yuri Norov 2018-12-23  517  {
32eb0e6b Yuri Norov 2018-12-23  518  	if (unlikely(is_user))
32eb0e6b Yuri Norov 2018-12-23 @519  		return __get_user(*c, str);
32eb0e6b Yuri Norov 2018-12-23  520  
32eb0e6b Yuri Norov 2018-12-23  521  	*c = *str;
32eb0e6b Yuri Norov 2018-12-23  522  	return 0;
32eb0e6b Yuri Norov 2018-12-23  523  }
32eb0e6b Yuri Norov 2018-12-23  524  

:::::: The code at line 519 was first introduced by commit
:::::: 32eb0e6b5e705132d4efef268c65730a2f2b9c41 bitmap_parselist: rework input string parser

:::::: TO: Yuri Norov <ynorov@marvell.com>
:::::: CC: 0day robot <lkp@intel.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 66645 bytes --]

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 17446 bytes --]

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 '
   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: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: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: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: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 '

vim +680 lib/bitmap.c

32eb0e6b5 Yuri Norov            2018-12-23  649  
5aaba3631 Sudeep Holla          2014-09-30  650  /**
4b060420a Mike Travis           2011-05-24  651   * __bitmap_parselist - convert list format ASCII string to bitmap
b0825ee3a Randy Dunlap          2011-06-15  652   * @buf: read nul-terminated user string from this buffer
4b060420a Mike Travis           2011-05-24  653   * @buflen: buffer size in bytes.  If string is smaller than this
4b060420a Mike Travis           2011-05-24  654   *    then it must be terminated with a \0.
4b060420a Mike Travis           2011-05-24  655   * @is_user: location of buffer, 0 indicates kernel space
6e1907ffd Randy Dunlap          2006-06-25  656   * @maskp: write resulting mask here
^1da177e4 Linus Torvalds        2005-04-16  657   * @nmaskbits: number of bits in mask to be written
^1da177e4 Linus Torvalds        2005-04-16  658   *
^1da177e4 Linus Torvalds        2005-04-16  659   * Input format is a comma-separated list of decimal numbers and
^1da177e4 Linus Torvalds        2005-04-16  660   * ranges.  Consecutively set bits are shown as two hyphen-separated
^1da177e4 Linus Torvalds        2005-04-16  661   * decimal numbers, the smallest and largest bit numbers set in
^1da177e4 Linus Torvalds        2005-04-16  662   * the range.
2d13e6ca4 Noam Camus            2016-10-11  663   * Optionally each range can be postfixed to denote that only parts of it
2d13e6ca4 Noam Camus            2016-10-11  664   * should be set. The range will divided to groups of specific size.
2d13e6ca4 Noam Camus            2016-10-11  665   * From each group will be used only defined amount of bits.
2d13e6ca4 Noam Camus            2016-10-11  666   * Syntax: range:used_size/group_size
2d13e6ca4 Noam Camus            2016-10-11  667   * Example: 0-1023:2/256 ==> 0,1,256,257,512,513,768,769
^1da177e4 Linus Torvalds        2005-04-16  668   *
40bf19a8d Mauro Carvalho Chehab 2017-03-30  669   * Returns: 0 on success, -errno on invalid input strings. Error values:
40bf19a8d Mauro Carvalho Chehab 2017-03-30  670   *
32eb0e6b5 Yuri Norov            2018-12-23  671   *   - ``-EINVAL``: wrong region format
40bf19a8d Mauro Carvalho Chehab 2017-03-30  672   *   - ``-EINVAL``: invalid character in string
40bf19a8d Mauro Carvalho Chehab 2017-03-30  673   *   - ``-ERANGE``: bit number specified too large for mask
32eb0e6b5 Yuri Norov            2018-12-23  674   *   - ``-EOVERFLOW``: integer overflow in the input parameters
^1da177e4 Linus Torvalds        2005-04-16  675   */
32eb0e6b5 Yuri Norov            2018-12-23  676  static int __bitmap_parselist(const char *buf, const char *const end,
4b060420a Mike Travis           2011-05-24  677  		int is_user, unsigned long *maskp,
4b060420a Mike Travis           2011-05-24  678  		int nmaskbits)
^1da177e4 Linus Torvalds        2005-04-16 @679  {
87ac23b87 Yuri Norov            2018-12-23 @680  	struct region r;
32eb0e6b5 Yuri Norov            2018-12-23  681  	long ret;
^1da177e4 Linus Torvalds        2005-04-16  682  
^1da177e4 Linus Torvalds        2005-04-16  683  	bitmap_zero(maskp, nmaskbits);
4b060420a Mike Travis           2011-05-24  684  
32eb0e6b5 Yuri Norov            2018-12-23  685  	while (buf && buf < end) {
32eb0e6b5 Yuri Norov            2018-12-23  686  		buf = bitmap_find_region(buf, end, is_user);
32eb0e6b5 Yuri Norov            2018-12-23  687  		if (buf == NULL)
32eb0e6b5 Yuri Norov            2018-12-23  688  			return 0;
87ac23b87 Yuri Norov            2018-12-23  689  
32eb0e6b5 Yuri Norov            2018-12-23  690  		buf = bitmap_parse_region(&r, buf, end, is_user);
32eb0e6b5 Yuri Norov            2018-12-23  691  		if (IS_ERR(buf))
32eb0e6b5 Yuri Norov            2018-12-23  692  			return (long)buf;
87ac23b87 Yuri Norov            2018-12-23  693  
87ac23b87 Yuri Norov            2018-12-23  694  		ret = bitmap_check_region(&r);
87ac23b87 Yuri Norov            2018-12-23  695  		if (ret)
87ac23b87 Yuri Norov            2018-12-23  696  			return ret;
87ac23b87 Yuri Norov            2018-12-23  697  
87ac23b87 Yuri Norov            2018-12-23  698  		ret = bitmap_set_region(&r, maskp, nmaskbits);
87ac23b87 Yuri Norov            2018-12-23  699  		if (ret)
87ac23b87 Yuri Norov            2018-12-23  700  			return ret;
32eb0e6b5 Yuri Norov            2018-12-23  701  	}
87ac23b87 Yuri Norov            2018-12-23  702  
^1da177e4 Linus Torvalds        2005-04-16  703  	return 0;
^1da177e4 Linus Torvalds        2005-04-16  704  }
4b060420a Mike Travis           2011-05-24  705  

:::::: The code at line 680 was first introduced by commit
:::::: 87ac23b8755f8cbef41f1befaee43d76bbfb2cc9 bitmap_parselist: move part of logic to helpers

:::::: TO: Yuri Norov <ynorov@marvell.com>
:::::: CC: 0day robot <lkp@intel.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 6598 bytes --]

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

[Andy Shevchenko]

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

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

[Yuri Norov]

On Wed, Jan 09, 2019 at 06:01:46PM +0200, Andy Shevchenko wrote:
> 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

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 <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;

This cast looks unneeded.

> > +}
> > +
> >  /**
> >   * __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
> > 
> 

Thanks,
Yury