[PATCH] lib/bitmap.c: return -EINVAL for grouping errors in __bitmap_parselist

[PATCH] lib/bitmap.c: return -EINVAL for grouping errors in __bitmap_parselist

[Pan Xinhui]

Sometimes the input from user may cause an unexpected result.

just like __bitmap_parse, we return -EINVAL if there is no avaiable digit in each
parsing procedures.

Signed-off-by: Pan Xinhui <xinhuix.pan@intel.com>
---
  lib/bitmap.c | 7 +++++--
  1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lib/bitmap.c b/lib/bitmap.c
index 64c0926..995fca2 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -504,7 +504,7 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
  		int nmaskbits)
  {
  	unsigned a, b;
-	int c, old_c, totaldigits;
+	int c, old_c, totaldigits, ndigits;
  	const char __user __force *ubuf = (const char __user __force *)buf;
  	int exp_digit, in_range;
  
@@ -514,6 +514,7 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
  		exp_digit = 1;
  		in_range = 0;
  		a = b = 0;
+		ndigits = 0;
  
  		/* Get the next cpu# or a range of cpu#'s */
  		while (buflen) {
@@ -555,8 +556,10 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
  			if (!in_range)
  				a = b;
  			exp_digit = 0;
-			totaldigits++;
+			ndigits++; totaldigits++;
  		}
+		if (ndigits == 0)
+			return -EINVAL;
  		if (!(a <= b))
  			return -EINVAL;
  		if (b >= nmaskbits)
-- 
1.9.1

Re: [PATCH] lib/bitmap.c: return -EINVAL for grouping errors in __bitmap_parselist

[Yury Norov]

> Sometimes the input from user may cause an unexpected result.

Could you please provide specific example?

>
> just like __bitmap_parse, we return -EINVAL if there is no avaiable digit in each
> parsing procedures.
>
> Signed-off-by: Pan Xinhui <xinhuix.pan@intel.com>

Hello, Pan.

(Adding Alexey Klimov, Rasmus Villemoes)

> ---
>   lib/bitmap.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/lib/bitmap.c b/lib/bitmap.c
> index 64c0926..995fca2 100644
> --- a/lib/bitmap.c
> +++ b/lib/bitmap.c
> @@ -504,7 +504,7 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
>           int nmaskbits)
>   {
>       unsigned a, b;
> -    int c, old_c, totaldigits;
> +    int c, old_c, totaldigits, ndigits;
>       const char __user __force *ubuf = (const char __user __force *)buf;
>       int exp_digit, in_range;
>
> @@ -514,6 +514,7 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
>           exp_digit = 1;
>           in_range = 0;
>           a = b = 0;
> +        ndigits = 0;
>
>           /* Get the next cpu# or a range of cpu#'s */
>           while (buflen) {
> @@ -555,8 +556,10 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
>               if (!in_range)
>                   a = b;
>               exp_digit = 0;
> -            totaldigits++;
> +            ndigits++; totaldigits++;

I'm not happy with joining two statements to a single line.
Maybe sometimes it's OK for loop iterators like

    while (a[i][j]) {
        i++; j++;
    }

But here it looks nasty. Anyway, it's minor.

>           }
> +        if (ndigits == 0)
> +            return -EINVAL;

You can avoid in-loop incrementation of ndigits if you'll
save current totaldigits to ndigits before loop, and check
ndigits against totaldigits after the loop:

    ndigits = totaldigits;
    while (...) {
         ...
        totaldigits++;
    }

    if (ndigits == totaldigits)
        return -EINVAL;

Maybe it's a good point to rework initial __bitmap_parse() similar way...

>           if (!(a <= b))
>               return -EINVAL;
>           if (b >= nmaskbits)
> --
> 1.9.1

Re: [PATCH] lib/bitmap.c: return -EINVAL for grouping errors in __bitmap_parselist

[Pan Xinhui]

hi, Yury
	thanks for your nice reply.

On 2015年06月29日 21:39, Yury Norov wrote:
>> Sometimes the input from user may cause an unexpected result.
>
> Could you please provide specific example?
>
I wrote some scripts to do some tests about irqs.
echo "1-3," > /proc/irq/<xxx>/smp_affinity_list
this command ends with ',' by mistake.
actually __bitmap_parselist() will report "0-3" for the final result which is wrong.

>>
>> just like __bitmap_parse, we return -EINVAL if there is no avaiable digit in each
>> parsing procedures.
>>
>> Signed-off-by: Pan Xinhui <xinhuix.pan@intel.com>
>
> Hello, Pan.
>
> (Adding Alexey Klimov, Rasmus Villemoes)
>
>> ---
>>    lib/bitmap.c | 7 +++++--
>>    1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/bitmap.c b/lib/bitmap.c
>> index 64c0926..995fca2 100644
>> --- a/lib/bitmap.c
>> +++ b/lib/bitmap.c
>> @@ -504,7 +504,7 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
>>            int nmaskbits)
>>    {
>>        unsigned a, b;
>> -    int c, old_c, totaldigits;
>> +    int c, old_c, totaldigits, ndigits;
>>        const char __user __force *ubuf = (const char __user __force *)buf;
>>        int exp_digit, in_range;
>>
>> @@ -514,6 +514,7 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
>>            exp_digit = 1;
>>            in_range = 0;
>>            a = b = 0;
>> +        ndigits = 0;
>>
>>            /* Get the next cpu# or a range of cpu#'s */
>>            while (buflen) {
>> @@ -555,8 +556,10 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
>>                if (!in_range)
>>                    a = b;
>>                exp_digit = 0;
>> -            totaldigits++;
>> +            ndigits++; totaldigits++;
>
> I'm not happy with joining two statements to a single line.
> Maybe sometimes it's OK for loop iterators like
>
>      while (a[i][j]) {
>          i++; j++;
>      }
>
> But here it looks nasty. Anyway, it's minor.
>

thanks for pointing out my mistake about the code style :)

>>            }
>> +        if (ndigits == 0)
>> +            return -EINVAL;
>
> You can avoid in-loop incrementation of ndigits if you'll
> save current totaldigits to ndigits before loop, and check
> ndigits against totaldigits after the loop:
>
>      ndigits = totaldigits;
>      while (...) {
>           ...
>          totaldigits++;
>      }
>
>      if (ndigits == totaldigits)
>          return -EINVAL;
>
> Maybe it's a good point to rework initial __bitmap_parse() similar way...
>

your advice is a good idea, thanks.
I am also thinking if we can rewrite them into one function for common codes.

thanks for your reply again :)

thanks
xinhui

>>            if (!(a <= b))
>>                return -EINVAL;
>>            if (b >= nmaskbits)
>> --
>> 1.9.1

[PATCH] lib/bitmap.c: rewrite __bitmap_parse && __bitmap_parselist

[Pan Xinhui]

add __bitmap_parse_common to match any contents and return expected result.

as __bitmap_parse_common need NULL-terminated string, we alloc a new buf.

this patch also fix some parse issues in __bitmap_parselist.
now it can handle grouping errors with input like " ", ",", etc.

Signed-off-by: xinhuix.pan <xinhuix.pan@intel.com>
---
  lib/bitmap.c | 232 ++++++++++++++++++++++++++++++++---------------------------
  1 file changed, 128 insertions(+), 104 deletions(-)

diff --git a/lib/bitmap.c b/lib/bitmap.c
index 64c0926..bc53c4f 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -16,6 +16,8 @@
  #include <asm/page.h>
  #include <asm/uaccess.h>
  
+#include <linux/parser.h>
+#include <linux/slab.h>
  /*
   * bitmaps provide an array of bits, implemented using an an
   * array of unsigned longs.  The number of valid bits in a
@@ -331,6 +333,58 @@ again:
  EXPORT_SYMBOL(bitmap_find_next_zero_area_off);
  
  /*
+ * __bitmap_parse_common - parse expected number from buf
+ * Return 0 on success.
+ * there two patterns.
+ * if buf's contents did not match any of them, reutrn equivalent error.
+ * Notice buf's contents may be changed.
+ */
+static int __bitmap_parse_common(char *buf, unsigned int buflen,
+		 unsigned long *a, unsigned long *b)
+{
+	int ret;
+	int token;
+	const match_table_t table = {
+		{
+			.token = 1,
+			.pattern = "%u",
+		},
+		{
+			.token = 2,
+			.pattern = "%u-%u",
+		},
+		{
+			.token = 0,
+			.pattern = NULL,
+		}
+	};
+	substring_t substr[MAX_OPT_ARGS];
+
+	if (!buflen || !a)
+		return -EINVAL;
+
+	token = match_token((char *)buf, table, substr);
+	switch (token) {
+	case 1:
+		*substr[0].to = '\0';
+		ret = kstrtoul(substr[0].from, 0, a);
+		if (b)
+			*b = *a;
+		break;
+	case 2:
+		*substr[0].to = '\0';
+		*substr[1].to = '\0';
+		ret = kstrtoul(substr[0].from, 0, a);
+		ret |= b ? kstrtoul(substr[1].from, 0, b) : -EINVAL;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+	return ret;
+}
+
+/*
   * Bitmap printing & parsing functions: first version by Nadia Yvette Chambers,
   * second version by Paul Jackson, third by Joe Korty.
   */
@@ -359,57 +413,44 @@ int __bitmap_parse(const char *buf, unsigned int buflen,
  		int is_user, unsigned long *maskp,
  		int nmaskbits)
  {
-	int c, old_c, totaldigits, ndigits, nchunks, nbits;
+	int nchunks, nbits, ret;
+	unsigned long a;
  	u32 chunk;
  	const char __user __force *ubuf = (const char __user __force *)buf;
+	char *kbuf, *endp;
+
+	if (!buflen)
+		return -EINVAL;
+	kbuf = kmalloc(buflen + 1, GFP_KERNEL);
+	if (!kbuf)
+		return -ENOMEM;
+	if (is_user) {
+		if (copy_from_user(kbuf, ubuf, buflen) != 0) {
+			kfree(kbuf);
+			return -EFAULT;
+		}
+	} else
+		memcpy(kbuf, buf, buflen);
+	kbuf[buflen] = '\0';
+	buf = strim(kbuf);
  
  	bitmap_zero(maskp, nmaskbits);
  
-	nchunks = nbits = totaldigits = c = 0;
+	nchunks = nbits = 0;
  	do {
-		chunk = ndigits = 0;
-
-		/* Get the next chunk of the bitmap */
-		while (buflen) {
-			old_c = c;
-			if (is_user) {
-				if (__get_user(c, ubuf++))
-					return -EFAULT;
-			}
-			else
-				c = *buf++;
-			buflen--;
-			if (isspace(c))
-				continue;
-
-			/*
-			 * If the last character was a space and the current
-			 * character isn't '\0', we've got embedded whitespace.
-			 * This is a no-no, so throw an error.
-			 */
-			if (totaldigits && c && isspace(old_c))
-				return -EINVAL;
-
-			/* A '\0' or a ',' signal the end of the chunk */
-			if (c == '\0' || c == ',')
-				break;
-
-			if (!isxdigit(c))
-				return -EINVAL;
-
-			/*
-			 * Make sure there are at least 4 free bits in 'chunk'.
-			 * If not, this hexdigit will overflow 'chunk', so
-			 * throw an error.
-			 */
-			if (chunk & ~((1UL << (CHUNKSZ - 4)) - 1))
-				return -EOVERFLOW;
-
-			chunk = (chunk << 4) | hex_to_bin(c);
-			ndigits++; totaldigits++;
+		endp = strchr(buf, ',');
+		if (endp)
+			*endp = '\0';
+		ret = __bitmap_parse_common((char *)buf, strlen(buf), &a, NULL);
+		if (ret)
+			break;
+		buf = endp + 1;
+
+		if (unlikely(a > U32_MAX)) {
+			ret = -ERANGE;
+			break;
  		}
-		if (ndigits == 0)
-			return -EINVAL;
+		chunk = (u32)a;
  		if (nchunks == 0 && chunk == 0)
  			continue;
  
@@ -417,11 +458,13 @@ int __bitmap_parse(const char *buf, unsigned int buflen,
  		*maskp |= chunk;
  		nchunks++;
  		nbits += (nchunks == 1) ? nbits_to_hold_value(chunk) : CHUNKSZ;
-		if (nbits > nmaskbits)
-			return -EOVERFLOW;
-	} while (buflen && c == ',');
-
-	return 0;
+		if (nbits > nmaskbits) {
+			ret = -EOVERFLOW;
+			break;
+		}
+	} while (endp);
+	kfree(kbuf);
+	return ret;
  }
  EXPORT_SYMBOL(__bitmap_parse);
  
@@ -503,70 +546,51 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
  		int is_user, unsigned long *maskp,
  		int nmaskbits)
  {
-	unsigned a, b;
-	int c, old_c, totaldigits;
+	unsigned long a, b;
+	int ret = 0;
  	const char __user __force *ubuf = (const char __user __force *)buf;
-	int exp_digit, in_range;
+	char *kbuf, *endp;
+
+	if (!buflen)
+		return -EINVAL;
+	kbuf = kmalloc(buflen + 1, GFP_KERNEL);
+	if (!kbuf)
+		return -ENOMEM;
+	if (is_user) {
+		if (copy_from_user(kbuf, ubuf, buflen) != 0) {
+			kfree(kbuf);
+			return -EFAULT;
+		}
+	} else
+		memcpy(kbuf, buf, buflen);
+	kbuf[buflen] = '\0';
+	buf = strim(kbuf);
  
-	totaldigits = c = 0;
  	bitmap_zero(maskp, nmaskbits);
  	do {
-		exp_digit = 1;
-		in_range = 0;
-		a = b = 0;
-
-		/* 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;
-
-			/*
-			 * If the last character was a space and the current
-			 * character isn't '\0', we've got embedded whitespace.
-			 * This is a no-no, so throw an error.
-			 */
-			if (totaldigits && c && isspace(old_c))
-				return -EINVAL;
-
-			/* A '\0' or a ',' signal the end of a cpu# or range */
-			if (c == '\0' || c == ',')
-				break;
-
-			if (c == '-') {
-				if (exp_digit || in_range)
-					return -EINVAL;
-				b = 0;
-				in_range = 1;
-				exp_digit = 1;
-				continue;
-			}
-
-			if (!isdigit(c))
-				return -EINVAL;
-
-			b = b * 10 + (c - '0');
-			if (!in_range)
-				a = b;
-			exp_digit = 0;
-			totaldigits++;
+		endp = strchr(buf, ',');
+		if (endp)
+			*endp = '\0';
+		ret = __bitmap_parse_common((char *)buf, strlen(buf), &a, &b);
+		if (ret)
+			break;
+		buf = endp + 1;
+
+		if (!(a <= b)) {
+			ret = -EINVAL;
+			break;
+		}
+		if (b >= nmaskbits) {
+			ret = -ERANGE;
+			break;
  		}
-		if (!(a <= b))
-			return -EINVAL;
-		if (b >= nmaskbits)
-			return -ERANGE;
  		while (a <= b) {
  			set_bit(a, maskp);
  			a++;
  		}
-	} while (buflen && c == ',');
-	return 0;
+	} while (endp);
+	kfree(kbuf);
+	return ret;
  }
  
  int bitmap_parselist(const char *bp, unsigned long *maskp, int nmaskbits)
-- 
1.9.1

[PATCH V2] lib/bitmap.c: rewrite __bitmap_parse && __bitmap_parselist

[Pan Xinhui]

add __bitmap_parse_common to match any contents and return expected reslut.

as __bitmap_parse_common need NULL-terminated string, we alloc a new buf.

this patch also fix some parse issues in __bitmap_parselist.
now it can handle grouping errors with input like " ", ",", etc.

Signed-off-by: Pan Xinhui <xinhuix.pan@intel.com>
---
  lib/bitmap.c | 234 +++++++++++++++++++++++++++++++++--------------------------
  1 file changed, 130 insertions(+), 104 deletions(-)
---
change log
V2:
__bitmap_parse_common need *base* to parse correct result

V1:
add __bitmap_parse_common and rewrite __bitmap_parse && __bitmap_parselist
---

diff --git a/lib/bitmap.c b/lib/bitmap.c
index 64c0926..dde19bf 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -16,6 +16,8 @@
  #include <asm/page.h>
  #include <asm/uaccess.h>
  
+#include <linux/parser.h>
+#include <linux/slab.h>
  /*
   * bitmaps provide an array of bits, implemented using an an
   * array of unsigned longs.  The number of valid bits in a
@@ -331,6 +333,58 @@ again:
  EXPORT_SYMBOL(bitmap_find_next_zero_area_off);
  
  /*
+ * __bitmap_parse_common - parse expected number from buf
+ * Return 0 on success.
+ * there two patterns.
+ * if buf's contents did not match any of them, reutrn equivalent error.
+ * Notice buf's contents may be changed.
+ */
+static int __bitmap_parse_common(char *buf, unsigned int buflen,
+		 unsigned long *a, unsigned long *b, int base)
+{
+	int ret;
+	int token;
+	const match_table_t table = {
+		{
+			.token = 1,
+			.pattern = "%x",
+		},
+		{
+			.token = 2,
+			.pattern = "%x-%x",
+		},
+		{
+			.token = 0,
+			.pattern = NULL,
+		}
+	};
+	substring_t substr[MAX_OPT_ARGS];
+
+	if (!buflen || !a)
+		return -EINVAL;
+
+	token = match_token((char *)buf, table, substr);
+	switch (token) {
+	case 1:
+		*substr[0].to = '\0';
+		ret = kstrtoul(substr[0].from, base, a);
+		if (b)
+			*b = *a;
+		break;
+	case 2:
+		*substr[0].to = '\0';
+		*substr[1].to = '\0';
+		ret = kstrtoul(substr[0].from, base, a);
+		ret |= b ? kstrtoul(substr[1].from, base, b) : -EINVAL;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+	return ret;
+}
+
+/*
   * Bitmap printing & parsing functions: first version by Nadia Yvette Chambers,
   * second version by Paul Jackson, third by Joe Korty.
   */
@@ -359,57 +413,45 @@ int __bitmap_parse(const char *buf, unsigned int buflen,
  		int is_user, unsigned long *maskp,
  		int nmaskbits)
  {
-	int c, old_c, totaldigits, ndigits, nchunks, nbits;
+	int nchunks, nbits, ret;
+	unsigned long a;
  	u32 chunk;
  	const char __user __force *ubuf = (const char __user __force *)buf;
+	char *kbuf, *endp;
+
+	if (!buflen)
+		return -EINVAL;
+	kbuf = kmalloc(buflen + 1, GFP_KERNEL);
+	if (!kbuf)
+		return -ENOMEM;
+	if (is_user) {
+		if (copy_from_user(kbuf, ubuf, buflen) != 0) {
+			kfree(kbuf);
+			return -EFAULT;
+		}
+	} else
+		memcpy(kbuf, buf, buflen);
+	kbuf[buflen] = '\0';
+	buf = strim(kbuf);
  
  	bitmap_zero(maskp, nmaskbits);
  
-	nchunks = nbits = totaldigits = c = 0;
+	nchunks = nbits = 0;
  	do {
-		chunk = ndigits = 0;
-
-		/* Get the next chunk of the bitmap */
-		while (buflen) {
-			old_c = c;
-			if (is_user) {
-				if (__get_user(c, ubuf++))
-					return -EFAULT;
-			}
-			else
-				c = *buf++;
-			buflen--;
-			if (isspace(c))
-				continue;
-
-			/*
-			 * If the last character was a space and the current
-			 * character isn't '\0', we've got embedded whitespace.
-			 * This is a no-no, so throw an error.
-			 */
-			if (totaldigits && c && isspace(old_c))
-				return -EINVAL;
-
-			/* A '\0' or a ',' signal the end of the chunk */
-			if (c == '\0' || c == ',')
-				break;
-
-			if (!isxdigit(c))
-				return -EINVAL;
-
-			/*
-			 * Make sure there are at least 4 free bits in 'chunk'.
-			 * If not, this hexdigit will overflow 'chunk', so
-			 * throw an error.
-			 */
-			if (chunk & ~((1UL << (CHUNKSZ - 4)) - 1))
-				return -EOVERFLOW;
-
-			chunk = (chunk << 4) | hex_to_bin(c);
-			ndigits++; totaldigits++;
+		endp = strchr(buf, ',');
+		if (endp)
+			*endp = '\0';
+		ret = __bitmap_parse_common((char *)buf, strlen(buf),
+								&a, NULL, 16);
+		if (ret)
+			break;
+		buf = endp + 1;
+
+		if (unlikely(a > U32_MAX)) {
+			ret = -ERANGE;
+			break;
  		}
-		if (ndigits == 0)
-			return -EINVAL;
+		chunk = (u32)a;
  		if (nchunks == 0 && chunk == 0)
  			continue;
  
@@ -417,11 +459,13 @@ int __bitmap_parse(const char *buf, unsigned int buflen,
  		*maskp |= chunk;
  		nchunks++;
  		nbits += (nchunks == 1) ? nbits_to_hold_value(chunk) : CHUNKSZ;
-		if (nbits > nmaskbits)
-			return -EOVERFLOW;
-	} while (buflen && c == ',');
-
-	return 0;
+		if (nbits > nmaskbits) {
+			ret = -EOVERFLOW;
+			break;
+		}
+	} while (endp);
+	kfree(kbuf);
+	return ret;
  }
  EXPORT_SYMBOL(__bitmap_parse);
  
@@ -503,70 +547,52 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
  		int is_user, unsigned long *maskp,
  		int nmaskbits)
  {
-	unsigned a, b;
-	int c, old_c, totaldigits;
+	unsigned long a, b;
+	int ret = 0;
  	const char __user __force *ubuf = (const char __user __force *)buf;
-	int exp_digit, in_range;
+	char *kbuf, *endp;
+
+	if (!buflen)
+		return -EINVAL;
+	kbuf = kmalloc(buflen + 1, GFP_KERNEL);
+	if (!kbuf)
+		return -ENOMEM;
+	if (is_user) {
+		if (copy_from_user(kbuf, ubuf, buflen) != 0) {
+			kfree(kbuf);
+			return -EFAULT;
+		}
+	} else
+		memcpy(kbuf, buf, buflen);
+	kbuf[buflen] = '\0';
+	buf = strim(kbuf);
  
-	totaldigits = c = 0;
  	bitmap_zero(maskp, nmaskbits);
  	do {
-		exp_digit = 1;
-		in_range = 0;
-		a = b = 0;
-
-		/* 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;
-
-			/*
-			 * If the last character was a space and the current
-			 * character isn't '\0', we've got embedded whitespace.
-			 * This is a no-no, so throw an error.
-			 */
-			if (totaldigits && c && isspace(old_c))
-				return -EINVAL;
-
-			/* A '\0' or a ',' signal the end of a cpu# or range */
-			if (c == '\0' || c == ',')
-				break;
-
-			if (c == '-') {
-				if (exp_digit || in_range)
-					return -EINVAL;
-				b = 0;
-				in_range = 1;
-				exp_digit = 1;
-				continue;
-			}
-
-			if (!isdigit(c))
-				return -EINVAL;
-
-			b = b * 10 + (c - '0');
-			if (!in_range)
-				a = b;
-			exp_digit = 0;
-			totaldigits++;
+		endp = strchr(buf, ',');
+		if (endp)
+			*endp = '\0';
+		ret = __bitmap_parse_common((char *)buf, strlen(buf),
+								&a, &b, 0);
+		if (ret)
+			break;
+		buf = endp + 1;
+
+		if (!(a <= b)) {
+			ret = -EINVAL;
+			break;
+		}
+		if (b >= nmaskbits) {
+			ret = -ERANGE;
+			break;
  		}
-		if (!(a <= b))
-			return -EINVAL;
-		if (b >= nmaskbits)
-			return -ERANGE;
  		while (a <= b) {
  			set_bit(a, maskp);
  			a++;
  		}
-	} while (buflen && c == ',');
-	return 0;
+	} while (endp);
+	kfree(kbuf);
+	return ret;
  }
  
  int bitmap_parselist(const char *bp, unsigned long *maskp, int nmaskbits)
-- 
1.9.1

[PATCH V3] lib/bitmap.c: rewrite __bitmap_parse && __bitmap_parselist

[Pan Xinhui]

add __bitmap_parse_common to match any contents and return expected reslut.

as __bitmap_parse_common need NULL-terminated string, we alloc a new buf.

this patch also fix an unexpected parse result issue in __bitmap_parselist.

Signed-off-by: Pan Xinhui <xinhuix.pan@intel.com>
---
  lib/bitmap.c | 238 +++++++++++++++++++++++++++++++++--------------------------
  1 file changed, 134 insertions(+), 104 deletions(-)
---
change log
v3:
__bitmap_parselist now allow some extra input, like ",2, 3,,,,  5-8", at least one digit inside.
input like " " ", " is still not allowed.

V2:
__bitmap_parse_common need *base* to parse correct result

V1:
add __bitmap_parse_common and rewrite __bitmap_parse && __bitmap_parselist
---

diff --git a/lib/bitmap.c b/lib/bitmap.c
index 64c0926..f2095e1d 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -16,6 +16,8 @@
  #include <asm/page.h>
  #include <asm/uaccess.h>
  
+#include <linux/parser.h>
+#include <linux/slab.h>
  /*
   * bitmaps provide an array of bits, implemented using an an
   * array of unsigned longs.  The number of valid bits in a
@@ -331,6 +333,58 @@ again:
  EXPORT_SYMBOL(bitmap_find_next_zero_area_off);
  
  /*
+ * __bitmap_parse_common - parse expected number from buf
+ * Return 0 on success.
+ * there two patterns.
+ * if buf's contents did not match any of them, reutrn equivalent error.
+ * Notice buf's contents may be changed.
+ */
+static int __bitmap_parse_common(char *buf, unsigned int buflen,
+		 unsigned long *a, unsigned long *b, int base)
+{
+	int ret;
+	int token;
+	const match_table_t table = {
+		{
+			.token = 1,
+			.pattern = "%x",
+		},
+		{
+			.token = 2,
+			.pattern = "%x-%x",
+		},
+		{
+			.token = 0,
+			.pattern = NULL,
+		}
+	};
+	substring_t substr[MAX_OPT_ARGS];
+
+	if (!buflen || !a)
+		return -EINVAL;
+
+	token = match_token((char *)buf, table, substr);
+	switch (token) {
+	case 1:
+		*substr[0].to = '\0';
+		ret = kstrtoul(substr[0].from, base, a);
+		if (b)
+			*b = *a;
+		break;
+	case 2:
+		*substr[0].to = '\0';
+		*substr[1].to = '\0';
+		ret = kstrtoul(substr[0].from, base, a);
+		ret |= b ? kstrtoul(substr[1].from, base, b) : -EINVAL;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+	return ret;
+}
+
+/*
   * Bitmap printing & parsing functions: first version by Nadia Yvette Chambers,
   * second version by Paul Jackson, third by Joe Korty.
   */
@@ -359,57 +413,45 @@ int __bitmap_parse(const char *buf, unsigned int buflen,
  		int is_user, unsigned long *maskp,
  		int nmaskbits)
  {
-	int c, old_c, totaldigits, ndigits, nchunks, nbits;
+	int nchunks, nbits, ret;
+	unsigned long a;
  	u32 chunk;
  	const char __user __force *ubuf = (const char __user __force *)buf;
+	char *kbuf, *endp;
+
+	if (!buflen)
+		return -EINVAL;
+	kbuf = kmalloc(buflen + 1, GFP_KERNEL);
+	if (!kbuf)
+		return -ENOMEM;
+	if (is_user) {
+		if (copy_from_user(kbuf, ubuf, buflen) != 0) {
+			kfree(kbuf);
+			return -EFAULT;
+		}
+	} else
+		memcpy(kbuf, buf, buflen);
+	kbuf[buflen] = '\0';
+	buf = strim(kbuf);
  
  	bitmap_zero(maskp, nmaskbits);
  
-	nchunks = nbits = totaldigits = c = 0;
+	nchunks = nbits = 0;
  	do {
-		chunk = ndigits = 0;
-
-		/* Get the next chunk of the bitmap */
-		while (buflen) {
-			old_c = c;
-			if (is_user) {
-				if (__get_user(c, ubuf++))
-					return -EFAULT;
-			}
-			else
-				c = *buf++;
-			buflen--;
-			if (isspace(c))
-				continue;
-
-			/*
-			 * If the last character was a space and the current
-			 * character isn't '\0', we've got embedded whitespace.
-			 * This is a no-no, so throw an error.
-			 */
-			if (totaldigits && c && isspace(old_c))
-				return -EINVAL;
-
-			/* A '\0' or a ',' signal the end of the chunk */
-			if (c == '\0' || c == ',')
-				break;
-
-			if (!isxdigit(c))
-				return -EINVAL;
-
-			/*
-			 * Make sure there are at least 4 free bits in 'chunk'.
-			 * If not, this hexdigit will overflow 'chunk', so
-			 * throw an error.
-			 */
-			if (chunk & ~((1UL << (CHUNKSZ - 4)) - 1))
-				return -EOVERFLOW;
-
-			chunk = (chunk << 4) | hex_to_bin(c);
-			ndigits++; totaldigits++;
+		endp = strchr(buf, ',');
+		if (endp)
+			*endp = '\0';
+		ret = __bitmap_parse_common((char *)buf, strlen(buf),
+								&a, NULL, 16);
+		if (ret)
+			break;
+		buf = endp + 1;
+
+		if (unlikely(a > U32_MAX)) {
+			ret = -ERANGE;
+			break;
  		}
-		if (ndigits == 0)
-			return -EINVAL;
+		chunk = (u32)a;
  		if (nchunks == 0 && chunk == 0)
  			continue;
  
@@ -417,11 +459,13 @@ int __bitmap_parse(const char *buf, unsigned int buflen,
  		*maskp |= chunk;
  		nchunks++;
  		nbits += (nchunks == 1) ? nbits_to_hold_value(chunk) : CHUNKSZ;
-		if (nbits > nmaskbits)
-			return -EOVERFLOW;
-	} while (buflen && c == ',');
-
-	return 0;
+		if (nbits > nmaskbits) {
+			ret = -EOVERFLOW;
+			break;
+		}
+	} while (endp);
+	kfree(kbuf);
+	return ret;
  }
  EXPORT_SYMBOL(__bitmap_parse);
  
@@ -503,70 +547,56 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
  		int is_user, unsigned long *maskp,
  		int nmaskbits)
  {
-	unsigned a, b;
-	int c, old_c, totaldigits;
+	unsigned long a, b;
+	int ret = -EINVAL;
  	const char __user __force *ubuf = (const char __user __force *)buf;
-	int exp_digit, in_range;
+	char *kbuf, *endp, *ibuf;
+
+	if (!buflen)
+		return -EINVAL;
+	ibuf = kbuf = kmalloc(buflen + 1, GFP_KERNEL);
+	if (!kbuf)
+		return -ENOMEM;
+	if (is_user) {
+		if (copy_from_user(kbuf, ubuf, buflen) != 0) {
+			kfree(kbuf);
+			return -EFAULT;
+		}
+	} else
+		memcpy(kbuf, buf, buflen);
+	kbuf[buflen] = '\0';
  
-	totaldigits = c = 0;
  	bitmap_zero(maskp, nmaskbits);
  	do {
-		exp_digit = 1;
-		in_range = 0;
-		a = b = 0;
-
-		/* 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;
-
-			/*
-			 * If the last character was a space and the current
-			 * character isn't '\0', we've got embedded whitespace.
-			 * This is a no-no, so throw an error.
-			 */
-			if (totaldigits && c && isspace(old_c))
-				return -EINVAL;
-
-			/* A '\0' or a ',' signal the end of a cpu# or range */
-			if (c == '\0' || c == ',')
-				break;
-
-			if (c == '-') {
-				if (exp_digit || in_range)
-					return -EINVAL;
-				b = 0;
-				in_range = 1;
-				exp_digit = 1;
-				continue;
-			}
-
-			if (!isdigit(c))
-				return -EINVAL;
-
-			b = b * 10 + (c - '0');
-			if (!in_range)
-				a = b;
-			exp_digit = 0;
-			totaldigits++;
+		endp = strchr(ibuf, ',');
+		if (endp)
+			*endp = '\0';
+		ibuf = strim(ibuf);
+		if (*ibuf == 0) {
+			ibuf = endp + 1;
+			continue;
+		}
+		ret = __bitmap_parse_common(ibuf, strlen(ibuf),
+								&a, &b, 0);
+		if (ret)
+			break;
+		ibuf = endp + 1;
+
+		if (!(a <= b)) {
+			ret = -EINVAL;
+			break;
+		}
+		if (b >= nmaskbits) {
+			ret = -ERANGE;
+			break;
  		}
-		if (!(a <= b))
-			return -EINVAL;
-		if (b >= nmaskbits)
-			return -ERANGE;
  		while (a <= b) {
  			set_bit(a, maskp);
  			a++;
  		}
-	} while (buflen && c == ',');
-	return 0;
+	} while (endp);
+	kfree(kbuf);
+	return ret;
  }
  
  int bitmap_parselist(const char *bp, unsigned long *maskp, int nmaskbits)
-- 
1.9.1

Re: [PATCH V3] lib/bitmap.c: rewrite __bitmap_parse && __bitmap_parselist

[Pan Xinhui]

hi, all
	after a deep think, allocing a new buf is ugly.
new patch "lib/bitmap.c: fix some parsing issues and code style" is sent out.

any comments is welcome. :)

thanks
xinhui

On 2015年06月30日 18:02, Pan Xinhui wrote:
>
> add __bitmap_parse_common to match any contents and return expected reslut.
>
> as __bitmap_parse_common need NULL-terminated string, we alloc a new buf.
>
> this patch also fix an unexpected parse result issue in __bitmap_parselist.
>
> Signed-off-by: Pan Xinhui <xinhuix.pan@intel.com>
> ---
>   lib/bitmap.c | 238 +++++++++++++++++++++++++++++++++--------------------------
>   1 file changed, 134 insertions(+), 104 deletions(-)
> ---
> change log
> v3:
> __bitmap_parselist now allow some extra input, like ",2, 3,,,,  5-8", at least one digit inside.
> input like " " ", " is still not allowed.
>
> V2:
> __bitmap_parse_common need *base* to parse correct result
>
> V1:
> add __bitmap_parse_common and rewrite __bitmap_parse && __bitmap_parselist
> ---
>
> diff --git a/lib/bitmap.c b/lib/bitmap.c
> index 64c0926..f2095e1d 100644
> --- a/lib/bitmap.c
> +++ b/lib/bitmap.c
> @@ -16,6 +16,8 @@
>   #include <asm/page.h>
>   #include <asm/uaccess.h>
>
> +#include <linux/parser.h>
> +#include <linux/slab.h>
>   /*
>    * bitmaps provide an array of bits, implemented using an an
>    * array of unsigned longs.  The number of valid bits in a
> @@ -331,6 +333,58 @@ again:
>   EXPORT_SYMBOL(bitmap_find_next_zero_area_off);
>
>   /*
> + * __bitmap_parse_common - parse expected number from buf
> + * Return 0 on success.
> + * there two patterns.
> + * if buf's contents did not match any of them, reutrn equivalent error.
> + * Notice buf's contents may be changed.
> + */
> +static int __bitmap_parse_common(char *buf, unsigned int buflen,
> +         unsigned long *a, unsigned long *b, int base)
> +{
> +    int ret;
> +    int token;
> +    const match_table_t table = {
> +        {
> +            .token = 1,
> +            .pattern = "%x",
> +        },
> +        {
> +            .token = 2,
> +            .pattern = "%x-%x",
> +        },
> +        {
> +            .token = 0,
> +            .pattern = NULL,
> +        }
> +    };
> +    substring_t substr[MAX_OPT_ARGS];
> +
> +    if (!buflen || !a)
> +        return -EINVAL;
> +
> +    token = match_token((char *)buf, table, substr);
> +    switch (token) {
> +    case 1:
> +        *substr[0].to = '\0';
> +        ret = kstrtoul(substr[0].from, base, a);
> +        if (b)
> +            *b = *a;
> +        break;
> +    case 2:
> +        *substr[0].to = '\0';
> +        *substr[1].to = '\0';
> +        ret = kstrtoul(substr[0].from, base, a);
> +        ret |= b ? kstrtoul(substr[1].from, base, b) : -EINVAL;
> +        break;
> +    default:
> +        ret = -EINVAL;
> +        break;
> +    }
> +    return ret;
> +}
> +
> +/*
>    * Bitmap printing & parsing functions: first version by Nadia Yvette Chambers,
>    * second version by Paul Jackson, third by Joe Korty.
>    */
> @@ -359,57 +413,45 @@ int __bitmap_parse(const char *buf, unsigned int buflen,
>           int is_user, unsigned long *maskp,
>           int nmaskbits)
>   {
> -    int c, old_c, totaldigits, ndigits, nchunks, nbits;
> +    int nchunks, nbits, ret;
> +    unsigned long a;
>       u32 chunk;
>       const char __user __force *ubuf = (const char __user __force *)buf;
> +    char *kbuf, *endp;
> +
> +    if (!buflen)
> +        return -EINVAL;
> +    kbuf = kmalloc(buflen + 1, GFP_KERNEL);
> +    if (!kbuf)
> +        return -ENOMEM;
> +    if (is_user) {
> +        if (copy_from_user(kbuf, ubuf, buflen) != 0) {
> +            kfree(kbuf);
> +            return -EFAULT;
> +        }
> +    } else
> +        memcpy(kbuf, buf, buflen);
> +    kbuf[buflen] = '\0';
> +    buf = strim(kbuf);
>
>       bitmap_zero(maskp, nmaskbits);
>
> -    nchunks = nbits = totaldigits = c = 0;
> +    nchunks = nbits = 0;
>       do {
> -        chunk = ndigits = 0;
> -
> -        /* Get the next chunk of the bitmap */
> -        while (buflen) {
> -            old_c = c;
> -            if (is_user) {
> -                if (__get_user(c, ubuf++))
> -                    return -EFAULT;
> -            }
> -            else
> -                c = *buf++;
> -            buflen--;
> -            if (isspace(c))
> -                continue;
> -
> -            /*
> -             * If the last character was a space and the current
> -             * character isn't '\0', we've got embedded whitespace.
> -             * This is a no-no, so throw an error.
> -             */
> -            if (totaldigits && c && isspace(old_c))
> -                return -EINVAL;
> -
> -            /* A '\0' or a ',' signal the end of the chunk */
> -            if (c == '\0' || c == ',')
> -                break;
> -
> -            if (!isxdigit(c))
> -                return -EINVAL;
> -
> -            /*
> -             * Make sure there are at least 4 free bits in 'chunk'.
> -             * If not, this hexdigit will overflow 'chunk', so
> -             * throw an error.
> -             */
> -            if (chunk & ~((1UL << (CHUNKSZ - 4)) - 1))
> -                return -EOVERFLOW;
> -
> -            chunk = (chunk << 4) | hex_to_bin(c);
> -            ndigits++; totaldigits++;
> +        endp = strchr(buf, ',');
> +        if (endp)
> +            *endp = '\0';
> +        ret = __bitmap_parse_common((char *)buf, strlen(buf),
> +                                &a, NULL, 16);
> +        if (ret)
> +            break;
> +        buf = endp + 1;
> +
> +        if (unlikely(a > U32_MAX)) {
> +            ret = -ERANGE;
> +            break;
>           }
> -        if (ndigits == 0)
> -            return -EINVAL;
> +        chunk = (u32)a;
>           if (nchunks == 0 && chunk == 0)
>               continue;
>
> @@ -417,11 +459,13 @@ int __bitmap_parse(const char *buf, unsigned int buflen,
>           *maskp |= chunk;
>           nchunks++;
>           nbits += (nchunks == 1) ? nbits_to_hold_value(chunk) : CHUNKSZ;
> -        if (nbits > nmaskbits)
> -            return -EOVERFLOW;
> -    } while (buflen && c == ',');
> -
> -    return 0;
> +        if (nbits > nmaskbits) {
> +            ret = -EOVERFLOW;
> +            break;
> +        }
> +    } while (endp);
> +    kfree(kbuf);
> +    return ret;
>   }
>   EXPORT_SYMBOL(__bitmap_parse);
>
> @@ -503,70 +547,56 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
>           int is_user, unsigned long *maskp,
>           int nmaskbits)
>   {
> -    unsigned a, b;
> -    int c, old_c, totaldigits;
> +    unsigned long a, b;
> +    int ret = -EINVAL;
>       const char __user __force *ubuf = (const char __user __force *)buf;
> -    int exp_digit, in_range;
> +    char *kbuf, *endp, *ibuf;
> +
> +    if (!buflen)
> +        return -EINVAL;
> +    ibuf = kbuf = kmalloc(buflen + 1, GFP_KERNEL);
> +    if (!kbuf)
> +        return -ENOMEM;
> +    if (is_user) {
> +        if (copy_from_user(kbuf, ubuf, buflen) != 0) {
> +            kfree(kbuf);
> +            return -EFAULT;
> +        }
> +    } else
> +        memcpy(kbuf, buf, buflen);
> +    kbuf[buflen] = '\0';
>
> -    totaldigits = c = 0;
>       bitmap_zero(maskp, nmaskbits);
>       do {
> -        exp_digit = 1;
> -        in_range = 0;
> -        a = b = 0;
> -
> -        /* 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;
> -
> -            /*
> -             * If the last character was a space and the current
> -             * character isn't '\0', we've got embedded whitespace.
> -             * This is a no-no, so throw an error.
> -             */
> -            if (totaldigits && c && isspace(old_c))
> -                return -EINVAL;
> -
> -            /* A '\0' or a ',' signal the end of a cpu# or range */
> -            if (c == '\0' || c == ',')
> -                break;
> -
> -            if (c == '-') {
> -                if (exp_digit || in_range)
> -                    return -EINVAL;
> -                b = 0;
> -                in_range = 1;
> -                exp_digit = 1;
> -                continue;
> -            }
> -
> -            if (!isdigit(c))
> -                return -EINVAL;
> -
> -            b = b * 10 + (c - '0');
> -            if (!in_range)
> -                a = b;
> -            exp_digit = 0;
> -            totaldigits++;
> +        endp = strchr(ibuf, ',');
> +        if (endp)
> +            *endp = '\0';
> +        ibuf = strim(ibuf);
> +        if (*ibuf == 0) {
> +            ibuf = endp + 1;
> +            continue;
> +        }
> +        ret = __bitmap_parse_common(ibuf, strlen(ibuf),
> +                                &a, &b, 0);
> +        if (ret)
> +            break;
> +        ibuf = endp + 1;
> +
> +        if (!(a <= b)) {
> +            ret = -EINVAL;
> +            break;
> +        }
> +        if (b >= nmaskbits) {
> +            ret = -ERANGE;
> +            break;
>           }
> -        if (!(a <= b))
> -            return -EINVAL;
> -        if (b >= nmaskbits)
> -            return -ERANGE;
>           while (a <= b) {
>               set_bit(a, maskp);
>               a++;
>           }
> -    } while (buflen && c == ',');
> -    return 0;
> +    } while (endp);
> +    kfree(kbuf);
> +    return ret;
>   }
>
>   int bitmap_parselist(const char *bp, unsigned long *maskp, int nmaskbits)

Re: [PATCH V3] lib/bitmap.c: rewrite __bitmap_parse && __bitmap_parselist

[Yury]

 > Subject: lib/bitmap.c: rewrite __bitmap_parse && __bitmap_parselist
     scripts/checkpatch.pl 
lib_bitmap.c:-rewrite-__bitmap_parse-__bitmap_parselist.patch
     total: 134 errors, 1 warnings, 284 lines checked

     NOTE: whitespace errors detected, you may wish to use 
scripts/cleanpatch or
           scripts/cleanfile

Most of them are about DOS line endings, but it prevents me to apply 
your patch:
     patch -p1 < 
lib_bitmap.c:-rewrite-__bitmap_parse-__bitmap_parselist.patch
     (Stripping trailing CRs from patch; use --binary to disable.)
     patching file lib/bitmap.c
     Hunk #1 FAILED at 16.
     Hunk #2 FAILED at 331.
     Hunk #3 FAILED at 359.
     Hunk #4 FAILED at 417.
     Hunk #5 FAILED at 503.
     5 out of 5 hunks FAILED -- saving rejects to file lib/bitmap.c.rej
 >
 > add __bitmap_parse_common to match any contents and return expected 
reslut.
 >
 > as __bitmap_parse_common need NULL-terminated string, we alloc a new buf.
 >
 > this patch also fix an unexpected parse result issue in 
__bitmap_parselist.
 >
 > Signed-off-by: Pan Xinhui <xinhuix.pan@intel.com>
 > ---
 >   lib/bitmap.c | 238 
+++++++++++++++++++++++++++++++++--------------------------
 >   1 file changed, 134 insertions(+), 104 deletions(-)
 > ---
 > change log
 > v3:
 > __bitmap_parselist now allow some extra input, like ",2, 3,,,, 5-8", 
at least one digit inside.
 > input like " " ", " is still not allowed.
 >
 > V2:
 > __bitmap_parse_common need *base* to parse correct result
 >
 > V1:
 > add __bitmap_parse_common and rewrite __bitmap_parse && 
__bitmap_parselist
 > ---
 >
 > diff --git a/lib/bitmap.c b/lib/bitmap.c
 > index 64c0926..f2095e1d 100644
 > --- a/lib/bitmap.c
 > +++ b/lib/bitmap.c
 > @@ -16,6 +16,8 @@
 >   #include <asm/page.h>
 >   #include <asm/uaccess.h>
 >
 > +#include <linux/parser.h>
 > +#include <linux/slab.h>
 >   /*
 >    * bitmaps provide an array of bits, implemented using an an
 >    * array of unsigned longs.  The number of valid bits in a
 > @@ -331,6 +333,58 @@ again:
 >   EXPORT_SYMBOL(bitmap_find_next_zero_area_off);
 >
 >   /*
 > + * __bitmap_parse_common - parse expected number from buf
 > + * Return 0 on success.
 > + * there two patterns.
 > + * if buf's contents did not match any of them, reutrn equivalent error.
 > + * Notice buf's contents may be changed.
 > + */
 > +static int __bitmap_parse_common(char *buf, unsigned int buflen,
 > +         unsigned long *a, unsigned long *b, int base)

It looks weird, and I don't like in your version too much:
Name is bad.
There's nothing about bitmap. You just parsing a string for two 
patterns: %d, and %d-%d.

You do your work twice (at least): first you detect digits in 
match_token, then - in kstrtoul.
You allocate kbuf unconditionally, no matter you need it or not.
You do more than one thing in __bitmap_parse_common (you search number 
and region).
You modify initial string.

Let's consider a more straight interface:

     /*
      * I don't know why this function is not written yet.
      * Maybe it's something ideological...
      */
     void set_bits(unsigned long *bitmap, unsigned long start, unsigned 
long len);

     /*
      * Takes care of all user whitespaces and commas,
      * Return endp, or error if parse fails, or null if string reached 
the end.
      */
     char *parse_range(const char *buf, unsigned long *start, unsigned 
long *len);

than pattern usage would be:

     while (str = parse_range(str, &start, &len)) {
         if (IS_ERROR(str))
             return ...;
         if (start + len >= nbits)
             return ...;

         set_bits(bitmap, start, len);
     }

 > +{
 > +    int ret;
 > +    int token;
 > +    const match_table_t table = {
 > +        {
 > +            .token = 1,
 > +            .pattern = "%x",
 > +        },
 > +        {
 > +            .token = 2,
 > +            .pattern = "%x-%x",
 > +        },
 > +        {
 > +            .token = 0,
 > +            .pattern = NULL,
 > +        }
 > +    };
 > +    substring_t substr[MAX_OPT_ARGS];
 > +
 > +    if (!buflen || !a)
 > +        return -EINVAL;
 > +    token = match_token((char *)buf, table, substr);
 > +    switch (token) {
 > +    case 1:
 > +        *substr[0].to = '\0';
 > +        ret = kstrtoul(substr[0].from, base, a);
 > +        if (b)
 > +            *b = *a;
 > +        break;
 > +    case 2:
 > +        *substr[0].to = '\0';
 > +        *substr[1].to = '\0';
 > +        ret = kstrtoul(substr[0].from, base, a);
 > +        ret |= b ? kstrtoul(substr[1].from, base, b) : -EINVAL;
 > +        break;
 > +    default:
 > +        ret = -EINVAL;
 > +        break;
 > +    }
 > +    return ret;
 > +}
 > +
 > +/*
 >    * Bitmap printing & parsing functions: first version by Nadia 
Yvette Chambers,
 >    * second version by Paul Jackson, third by Joe Korty.
 >    */
 > @@ -359,57 +413,45 @@ int __bitmap_parse(const char *buf, unsigned 
int buflen,
 >           int is_user, unsigned long *maskp,
 >           int nmaskbits)
 >   {
 > -    int c, old_c, totaldigits, ndigits, nchunks, nbits;
 > +    int nchunks, nbits, ret;
 > +    unsigned long a;
 >       u32 chunk;
 >       const char __user __force *ubuf = (const char __user __force *)buf;
 > +    char *kbuf, *endp;
 > +
 > +    if (!buflen)
 > +        return -EINVAL;
 > +    kbuf = kmalloc(buflen + 1, GFP_KERNEL);
 > +    if (!kbuf)
 > +        return -ENOMEM;
 > +    if (is_user) {
 > +        if (copy_from_user(kbuf, ubuf, buflen) != 0) {
 > +            kfree(kbuf);
 > +            return -EFAULT;
 > +        }
 > +    } else
 > +        memcpy(kbuf, buf, buflen);
 > +    kbuf[buflen] = '\0';
 > +    buf = strim(kbuf);
 >
 >       bitmap_zero(maskp, nmaskbits);
 >
 > -    nchunks = nbits = totaldigits = c = 0;
 > +    nchunks = nbits = 0;
 >       do {
 > -        chunk = ndigits = 0;
 > -
 > -        /* Get the next chunk of the bitmap */
 > -        while (buflen) {
 > -            old_c = c;
 > -            if (is_user) {
 > -                if (__get_user(c, ubuf++))
 > -                    return -EFAULT;
 > -            }
 > -            else
 > -                c = *buf++;
 > -            buflen--;
 > -            if (isspace(c))
 > -                continue;
 > -
 > -            /*
 > -             * If the last character was a space and the current
 > -             * character isn't '\0', we've got embedded whitespace.
 > -             * This is a no-no, so throw an error.
 > -             */
 > -            if (totaldigits && c && isspace(old_c))
 > -                return -EINVAL;
 > -
 > -            /* A '\0' or a ',' signal the end of the chunk */
 > -            if (c == '\0' || c == ',')
 > -                break;
 > -
 > -            if (!isxdigit(c))
 > -                return -EINVAL;
 > -
 > -            /*
 > -             * Make sure there are at least 4 free bits in 'chunk'.
 > -             * If not, this hexdigit will overflow 'chunk', so
 > -             * throw an error.
 > -             */
 > -            if (chunk & ~((1UL << (CHUNKSZ - 4)) - 1))
 > -                return -EOVERFLOW;
 > -
 > -            chunk = (chunk << 4) | hex_to_bin(c);
 > -            ndigits++; totaldigits++;
 > +        endp = strchr(buf, ',');
 > +        if (endp)
 > +            *endp = '\0';
 > +        ret = __bitmap_parse_common((char *)buf, strlen(buf),
 > +                                &a, NULL, 16);
 > +        if (ret)
 > +            break;
 > +        buf = endp + 1;
 > +
 > +        if (unlikely(a > U32_MAX)) {
 > +            ret = -ERANGE;
 > +            break;
 >           }
 > -        if (ndigits == 0)
 > -            return -EINVAL;
 > +        chunk = (u32)a;
 >           if (nchunks == 0 && chunk == 0)
 >               continue;
 >
 > @@ -417,11 +459,13 @@ int __bitmap_parse(const char *buf, unsigned 
int buflen,
 >           *maskp |= chunk;
 >           nchunks++;
 >           nbits += (nchunks == 1) ? nbits_to_hold_value(chunk) : CHUNKSZ;
 > -        if (nbits > nmaskbits)
 > -            return -EOVERFLOW;
 > -    } while (buflen && c == ',');
 > -
 > -    return 0;
 > +        if (nbits > nmaskbits) {
 > +            ret = -EOVERFLOW;
 > +            break;
 > +        }
 > +    } while (endp);
 > +    kfree(kbuf);
 > +    return ret;
 >   }
 >   EXPORT_SYMBOL(__bitmap_parse);
 >
 > @@ -503,70 +547,56 @@ static int __bitmap_parselist(const char *buf, 
unsigned int buflen,
 >           int is_user, unsigned long *maskp,
 >           int nmaskbits)
 >   {
 > -    unsigned a, b;
 > -    int c, old_c, totaldigits;
 > +    unsigned long a, b;
 > +    int ret = -EINVAL;
 >       const char __user __force *ubuf = (const char __user __force *)buf;
 > -    int exp_digit, in_range;
 > +    char *kbuf, *endp, *ibuf;
 > +
 > +    if (!buflen)
 > +        return -EINVAL;
 > +    ibuf = kbuf = kmalloc(buflen + 1, GFP_KERNEL);
 > +    if (!kbuf)
 > +        return -ENOMEM;
 > +    if (is_user) {
 > +        if (copy_from_user(kbuf, ubuf, buflen) != 0) {
 > +            kfree(kbuf);
 > +            return -EFAULT;
 > +        }
 > +    } else
 > +        memcpy(kbuf, buf, buflen);
 > +    kbuf[buflen] = '\0';
 >
 > -    totaldigits = c = 0;
 >       bitmap_zero(maskp, nmaskbits);
 >       do {
 > -        exp_digit = 1;
 > -        in_range = 0;
 > -        a = b = 0;
 > -
 > -        /* 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;
 > -
 > -            /*
 > -             * If the last character was a space and the current
 > -             * character isn't '\0', we've got embedded whitespace.
 > -             * This is a no-no, so throw an error.
 > -             */
 > -            if (totaldigits && c && isspace(old_c))
 > -                return -EINVAL;
 > -
 > -            /* A '\0' or a ',' signal the end of a cpu# or range */
 > -            if (c == '\0' || c == ',')
 > -                break;
 > -
 > -            if (c == '-') {
 > -                if (exp_digit || in_range)
 > -                    return -EINVAL;
 > -                b = 0;
 > -                in_range = 1;
 > -                exp_digit = 1;
 > -                continue;
 > -            }
 > -
 > -            if (!isdigit(c))
 > -                return -EINVAL;
 > -
 > -            b = b * 10 + (c - '0');
 > -            if (!in_range)
 > -                a = b;
 > -            exp_digit = 0;
 > -            totaldigits++;
 > +        endp = strchr(ibuf, ',');
 > +        if (endp)
 > +            *endp = '\0';
 > +        ibuf = strim(ibuf);
 > +        if (*ibuf == 0) {
 > +            ibuf = endp + 1;
 > +            continue;
 > +        }
 > +        ret = __bitmap_parse_common(ibuf, strlen(ibuf),
 > +                                &a, &b, 0);
 > +        if (ret)
 > +            break;
 > +        ibuf = endp + 1;
 > +
 > +        if (!(a <= b)) {
 > +            ret = -EINVAL;
 > +            break;
 > +        }
 > +        if (b >= nmaskbits) {
 > +            ret = -ERANGE;
 > +            break;
 >           }
 > -        if (!(a <= b))
 > -            return -EINVAL;
 > -        if (b >= nmaskbits)
 > -            return -ERANGE;
 >           while (a <= b) {
 >               set_bit(a, maskp);
 >               a++;
 >           }
 > -    } while (buflen && c == ',');
 > -    return 0;
 > +    } while (endp);
 > +    kfree(kbuf);
 > +    return ret;
 >   }
 >
 >   int bitmap_parselist(const char *bp, unsigned long *maskp, int 
nmaskbits)
 > --
 > 1.9.1

Re: [PATCH V3] lib/bitmap.c: rewrite __bitmap_parse && __bitmap_parselist

[Pan Xinhui]

hi, Yury
	
On 2015年07月01日 17:16, Yury wrote:
>> Subject: lib/bitmap.c: rewrite __bitmap_parse && __bitmap_parselist
>     scripts/checkpatch.pl lib_bitmap.c:-rewrite-__bitmap_parse-__bitmap_parselist.patch
>     total: 134 errors, 1 warnings, 284 lines checked
> 
>     NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or
>           scripts/cleanfile
> 
> Most of them are about DOS line endings, but it prevents me to apply your patch:
>     patch -p1 < lib_bitmap.c:-rewrite-__bitmap_parse-__bitmap_parselist.patch
>     (Stripping trailing CRs from patch; use --binary to disable.)
>     patching file lib/bitmap.c
>     Hunk #1 FAILED at 16.
>     Hunk #2 FAILED at 331.
>     Hunk #3 FAILED at 359.
>     Hunk #4 FAILED at 417.
>     Hunk #5 FAILED at 503.
>     5 out of 5 hunks FAILED -- saving rejects to file lib/bitmap.c.rej

sorry for that I did not configure thunderbird correctly.
tab becomes spaces unexpectedly. :(

>>
>> add __bitmap_parse_common to match any contents and return expected reslut.
>>
>> as __bitmap_parse_common need NULL-terminated string, we alloc a new buf.
>>
>> this patch also fix an unexpected parse result issue in __bitmap_parselist.
>>
>> Signed-off-by: Pan Xinhui <xinhuix.pan@intel.com>
>> ---
>>   lib/bitmap.c | 238 +++++++++++++++++++++++++++++++++--------------------------
>>   1 file changed, 134 insertions(+), 104 deletions(-)
>> ---
>> change log
>> v3:
>> __bitmap_parselist now allow some extra input, like ",2, 3,,,, 5-8", at least one digit inside.
>> input like " " ", " is still not allowed.
>>
>> V2:
>> __bitmap_parse_common need *base* to parse correct result
>>
>> V1:
>> add __bitmap_parse_common and rewrite __bitmap_parse && __bitmap_parselist
>> ---
>>
>> diff --git a/lib/bitmap.c b/lib/bitmap.c
>> index 64c0926..f2095e1d 100644
>> --- a/lib/bitmap.c
>> +++ b/lib/bitmap.c
>> @@ -16,6 +16,8 @@
>>   #include <asm/page.h>
>>   #include <asm/uaccess.h>
>>
>> +#include <linux/parser.h>
>> +#include <linux/slab.h>
>>   /*
>>    * bitmaps provide an array of bits, implemented using an an
>>    * array of unsigned longs.  The number of valid bits in a
>> @@ -331,6 +333,58 @@ again:
>>   EXPORT_SYMBOL(bitmap_find_next_zero_area_off);
>>
>>   /*
>> + * __bitmap_parse_common - parse expected number from buf
>> + * Return 0 on success.
>> + * there two patterns.
>> + * if buf's contents did not match any of them, reutrn equivalent error.
>> + * Notice buf's contents may be changed.
>> + */
>> +static int __bitmap_parse_common(char *buf, unsigned int buflen,
>> +         unsigned long *a, unsigned long *b, int base)
> 
> It looks weird, and I don't like in your version too much:
> Name is bad.
> There's nothing about bitmap. You just parsing a string for two patterns: %d, and %d-%d.
> 
yes, this is a bad name.

> You do your work twice (at least): first you detect digits in match_token, then - in kstrtoul.
> You allocate kbuf unconditionally, no matter you need it or not.
> You do more than one thing in __bitmap_parse_common (you search number and region).
> You modify initial string.
> 
I will try to find a better way. thanks for pointing out the bad codes design.

> Let's consider a more straight interface:
> 
>     /*
>      * I don't know why this function is not written yet.
>      * Maybe it's something ideological...
>      */
>     void set_bits(unsigned long *bitmap, unsigned long start, unsigned long len);
> 
>     /*
>      * Takes care of all user whitespaces and commas,
>      * Return endp, or error if parse fails, or null if string reached the end.
>      */
>     char *parse_range(const char *buf, unsigned long *start, unsigned long *len);
> 
> than pattern usage would be:
> 
>     while (str = parse_range(str, &start, &len)) {
>         if (IS_ERROR(str))
>             return ...;
>         if (start + len >= nbits)
>             return ...;
> 
>         set_bits(bitmap, start, len);
>     }
> 

agree!
a minor change.
int parse_range(const char *buf, char *endp, unsigned long *start, unsigned long *len)
seems better, I prefer ret value as error codes only.

thanks for your advices. :)
I will rewrite them.

thanks,
xinhui

>> +{
>> +    int ret;
>> +    int token;
>> +    const match_table_t table = {
>> +        {
>> +            .token = 1,
>> +            .pattern = "%x",
>> +        },
>> +        {
>> +            .token = 2,
>> +            .pattern = "%x-%x",
>> +        },
>> +        {
>> +            .token = 0,
>> +            .pattern = NULL,
>> +        }
>> +    };
>> +    substring_t substr[MAX_OPT_ARGS];
>> +
>> +    if (!buflen || !a)
>> +        return -EINVAL;
>> +    token = match_token((char *)buf, table, substr);
>> +    switch (token) {
>> +    case 1:
>> +        *substr[0].to = '\0';
>> +        ret = kstrtoul(substr[0].from, base, a);
>> +        if (b)
>> +            *b = *a;
>> +        break;
>> +    case 2:
>> +        *substr[0].to = '\0';
>> +        *substr[1].to = '\0';
>> +        ret = kstrtoul(substr[0].from, base, a);
>> +        ret |= b ? kstrtoul(substr[1].from, base, b) : -EINVAL;
>> +        break;
>> +    default:
>> +        ret = -EINVAL;
>> +        break;
>> +    }
>> +    return ret;
>> +}
>> +
>> +/*
>>    * Bitmap printing & parsing functions: first version by Nadia Yvette Chambers,
>>    * second version by Paul Jackson, third by Joe Korty.
>>    */
>> @@ -359,57 +413,45 @@ int __bitmap_parse(const char *buf, unsigned int buflen,
>>           int is_user, unsigned long *maskp,
>>           int nmaskbits)
>>   {
>> -    int c, old_c, totaldigits, ndigits, nchunks, nbits;
>> +    int nchunks, nbits, ret;
>> +    unsigned long a;
>>       u32 chunk;
>>       const char __user __force *ubuf = (const char __user __force *)buf;
>> +    char *kbuf, *endp;
>> +
>> +    if (!buflen)
>> +        return -EINVAL;
>> +    kbuf = kmalloc(buflen + 1, GFP_KERNEL);
>> +    if (!kbuf)
>> +        return -ENOMEM;
>> +    if (is_user) {
>> +        if (copy_from_user(kbuf, ubuf, buflen) != 0) {
>> +            kfree(kbuf);
>> +            return -EFAULT;
>> +        }
>> +    } else
>> +        memcpy(kbuf, buf, buflen);
>> +    kbuf[buflen] = '\0';
>> +    buf = strim(kbuf);
>>
>>       bitmap_zero(maskp, nmaskbits);
>>
>> -    nchunks = nbits = totaldigits = c = 0;
>> +    nchunks = nbits = 0;
>>       do {
>> -        chunk = ndigits = 0;
>> -
>> -        /* Get the next chunk of the bitmap */
>> -        while (buflen) {
>> -            old_c = c;
>> -            if (is_user) {
>> -                if (__get_user(c, ubuf++))
>> -                    return -EFAULT;
>> -            }
>> -            else
>> -                c = *buf++;
>> -            buflen--;
>> -            if (isspace(c))
>> -                continue;
>> -
>> -            /*
>> -             * If the last character was a space and the current
>> -             * character isn't '\0', we've got embedded whitespace.
>> -             * This is a no-no, so throw an error.
>> -             */
>> -            if (totaldigits && c && isspace(old_c))
>> -                return -EINVAL;
>> -
>> -            /* A '\0' or a ',' signal the end of the chunk */
>> -            if (c == '\0' || c == ',')
>> -                break;
>> -
>> -            if (!isxdigit(c))
>> -                return -EINVAL;
>> -
>> -            /*
>> -             * Make sure there are at least 4 free bits in 'chunk'.
>> -             * If not, this hexdigit will overflow 'chunk', so
>> -             * throw an error.
>> -             */
>> -            if (chunk & ~((1UL << (CHUNKSZ - 4)) - 1))
>> -                return -EOVERFLOW;
>> -
>> -            chunk = (chunk << 4) | hex_to_bin(c);
>> -            ndigits++; totaldigits++;
>> +        endp = strchr(buf, ',');
>> +        if (endp)
>> +            *endp = '\0';
>> +        ret = __bitmap_parse_common((char *)buf, strlen(buf),
>> +                                &a, NULL, 16);
>> +        if (ret)
>> +            break;
>> +        buf = endp + 1;
>> +
>> +        if (unlikely(a > U32_MAX)) {
>> +            ret = -ERANGE;
>> +            break;
>>           }
>> -        if (ndigits == 0)
>> -            return -EINVAL;
>> +        chunk = (u32)a;
>>           if (nchunks == 0 && chunk == 0)
>>               continue;
>>
>> @@ -417,11 +459,13 @@ int __bitmap_parse(const char *buf, unsigned int buflen,
>>           *maskp |= chunk;
>>           nchunks++;
>>           nbits += (nchunks == 1) ? nbits_to_hold_value(chunk) : CHUNKSZ;
>> -        if (nbits > nmaskbits)
>> -            return -EOVERFLOW;
>> -    } while (buflen && c == ',');
>> -
>> -    return 0;
>> +        if (nbits > nmaskbits) {
>> +            ret = -EOVERFLOW;
>> +            break;
>> +        }
>> +    } while (endp);
>> +    kfree(kbuf);
>> +    return ret;
>>   }
>>   EXPORT_SYMBOL(__bitmap_parse);
>>
>> @@ -503,70 +547,56 @@ static int __bitmap_parselist(const char *buf, unsigned int buflen,
>>           int is_user, unsigned long *maskp,
>>           int nmaskbits)
>>   {
>> -    unsigned a, b;
>> -    int c, old_c, totaldigits;
>> +    unsigned long a, b;
>> +    int ret = -EINVAL;
>>       const char __user __force *ubuf = (const char __user __force *)buf;
>> -    int exp_digit, in_range;
>> +    char *kbuf, *endp, *ibuf;
>> +
>> +    if (!buflen)
>> +        return -EINVAL;
>> +    ibuf = kbuf = kmalloc(buflen + 1, GFP_KERNEL);
>> +    if (!kbuf)
>> +        return -ENOMEM;
>> +    if (is_user) {
>> +        if (copy_from_user(kbuf, ubuf, buflen) != 0) {
>> +            kfree(kbuf);
>> +            return -EFAULT;
>> +        }
>> +    } else
>> +        memcpy(kbuf, buf, buflen);
>> +    kbuf[buflen] = '\0';
>>
>> -    totaldigits = c = 0;
>>       bitmap_zero(maskp, nmaskbits);
>>       do {
>> -        exp_digit = 1;
>> -        in_range = 0;
>> -        a = b = 0;
>> -
>> -        /* 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;
>> -
>> -            /*
>> -             * If the last character was a space and the current
>> -             * character isn't '\0', we've got embedded whitespace.
>> -             * This is a no-no, so throw an error.
>> -             */
>> -            if (totaldigits && c && isspace(old_c))
>> -                return -EINVAL;
>> -
>> -            /* A '\0' or a ',' signal the end of a cpu# or range */
>> -            if (c == '\0' || c == ',')
>> -                break;
>> -
>> -            if (c == '-') {
>> -                if (exp_digit || in_range)
>> -                    return -EINVAL;
>> -                b = 0;
>> -                in_range = 1;
>> -                exp_digit = 1;
>> -                continue;
>> -            }
>> -
>> -            if (!isdigit(c))
>> -                return -EINVAL;
>> -
>> -            b = b * 10 + (c - '0');
>> -            if (!in_range)
>> -                a = b;
>> -            exp_digit = 0;
>> -            totaldigits++;
>> +        endp = strchr(ibuf, ',');
>> +        if (endp)
>> +            *endp = '\0';
>> +        ibuf = strim(ibuf);
>> +        if (*ibuf == 0) {
>> +            ibuf = endp + 1;
>> +            continue;
>> +        }
>> +        ret = __bitmap_parse_common(ibuf, strlen(ibuf),
>> +                                &a, &b, 0);
>> +        if (ret)
>> +            break;
>> +        ibuf = endp + 1;
>> +
>> +        if (!(a <= b)) {
>> +            ret = -EINVAL;
>> +            break;
>> +        }
>> +        if (b >= nmaskbits) {
>> +            ret = -ERANGE;
>> +            break;
>>           }
>> -        if (!(a <= b))
>> -            return -EINVAL;
>> -        if (b >= nmaskbits)
>> -            return -ERANGE;
>>           while (a <= b) {
>>               set_bit(a, maskp);
>>               a++;
>>           }
>> -    } while (buflen && c == ',');
>> -    return 0;
>> +    } while (endp);
>> +    kfree(kbuf);
>> +    return ret;
>>   }
>>
>>   int bitmap_parselist(const char *bp, unsigned long *maskp, int nmaskbits)
>> --
>> 1.9.1
> 

Re: [PATCH] lib/bitmap.c: return -EINVAL for grouping errors in __bitmap_parselist

[Yury Norov]

2015-07-01 4:37 GMT+03:00 Pan Xinhui <xinhuix.pan@intel.com>:
> hi, Yury
>         thanks for your nice reply.
>
> On 2015年06月29日 21:39, Yury Norov wrote:
>>>
>>> Sometimes the input from user may cause an unexpected result.
>>
>>
>> Could you please provide specific example?
>>
> I wrote some scripts to do some tests about irqs.
> echo "1-3," > /proc/irq/<xxx>/smp_affinity_list
> this command ends with ',' by mistake.
> actually __bitmap_parselist() will report "0-3" for the final result which
> is wrong.
>

Hmm...
I don't think this is wrong passing echo "1-3,".
With or without a comma, the final result must be the same.
More flexible format is useful for hard scripts (for your one).
It's not too difficult to imagine a script producing a line:
         "1-24,  ,   ,,, ,  12-64, 92,92,92,,,"
And I don't think we should reject user with this once the range is valid.
Even more, to spend a time writing some additional code for it, and make
user spend his time as well.

I just tried
          cd /home/yury///./././/work
and it works perfectly well for me, and it's fine.

The true problem is that a and b variables
goes zero after comma, and EOL after comma just takes it:
 514     do {
 ...
 517         a = b = 0;                                           //
<--- comma makes it 0 here
 ...
 520         while (buflen) {
 ...
 539             /* A '\0' or a ',' signal the end of a cpu# or range */
 540             if (c == '\0' || c == ',')                     //
<---here we just break after '\0'
 541                 break;
 559         }
 ...
 565             while (a <= b) {
 566                 set_bit(a, maskp);                   // <--- and
here we set unneeded 0 bit.
 567                 a++;
 568             }

So currently, "1-3,\0" is the same as "1-3,0,\0". And this is definitely wrong.

>
>>>
>>> just like __bitmap_parse, we return -EINVAL if there is no avaiable digit
>>> in each
>>> parsing procedures.
>>>
>>> Signed-off-by: Pan Xinhui <xinhuix.pan@intel.com>
>>
>>
>> Hello, Pan.
>>
>> (Adding Alexey Klimov, Rasmus Villemoes)
>>
>>> ---
>>>    lib/bitmap.c | 7 +++++--
>>>    1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/bitmap.c b/lib/bitmap.c
>>> index 64c0926..995fca2 100644
>>> --- a/lib/bitmap.c
>>> +++ b/lib/bitmap.c
>>> @@ -504,7 +504,7 @@ static int __bitmap_parselist(const char *buf,
>>> unsigned int buflen,
>>>            int nmaskbits)
>>>    {
>>>        unsigned a, b;
>>> -    int c, old_c, totaldigits;
>>> +    int c, old_c, totaldigits, ndigits;
>>>        const char __user __force *ubuf = (const char __user __force
>>> *)buf;
>>>        int exp_digit, in_range;
>>>
>>> @@ -514,6 +514,7 @@ static int __bitmap_parselist(const char *buf,
>>> unsigned int buflen,
>>>            exp_digit = 1;
>>>            in_range = 0;
>>>            a = b = 0;
>>> +        ndigits = 0;
>>>
>>>            /* Get the next cpu# or a range of cpu#'s */
>>>            while (buflen) {
>>> @@ -555,8 +556,10 @@ static int __bitmap_parselist(const char *buf,
>>> unsigned int buflen,
>>>                if (!in_range)
>>>                    a = b;
>>>                exp_digit = 0;
>>> -            totaldigits++;
>>> +            ndigits++; totaldigits++;
>>
>>
>> I'm not happy with joining two statements to a single line.
>> Maybe sometimes it's OK for loop iterators like
>>
>>      while (a[i][j]) {
>>          i++; j++;
>>      }
>>
>> But here it looks nasty. Anyway, it's minor.
>>
>
> thanks for pointing out my mistake about the code style :)
>
>>>            }
>>> +        if (ndigits == 0)
>>> +            return -EINVAL;
>>
>>
>> You can avoid in-loop incrementation of ndigits if you'll
>> save current totaldigits to ndigits before loop, and check
>> ndigits against totaldigits after the loop:
>>
>>      ndigits = totaldigits;
>>      while (...) {
>>           ...
>>          totaldigits++;
>>      }
>>
>>      if (ndigits == totaldigits)
>>          return -EINVAL;
>>
>> Maybe it's a good point to rework initial __bitmap_parse() similar way...
>>
>
> your advice is a good idea, thanks.
> I am also thinking if we can rewrite them into one function for common
> codes.
>
> thanks for your reply again :)
>
> thanks
> xinhui
>
>
>>>            if (!(a <= b))
>>>                return -EINVAL;
>>>            if (b >= nmaskbits)
>>> --
>>> 1.9.1

Re: [PATCH] lib/bitmap.c: return -EINVAL for grouping errors in __bitmap_parselist

[Pan Xinhui]

hi, Yury

On 2015年06月30日 16:32, Yury Norov wrote:
> 2015-07-01 4:37 GMT+03:00 Pan Xinhui <xinhuix.pan@intel.com>:
>> hi, Yury
>>          thanks for your nice reply.
>>
>> On 2015年06月29日 21:39, Yury Norov wrote:
>>>>
>>>> Sometimes the input from user may cause an unexpected result.
>>>
>>>
>>> Could you please provide specific example?
>>>
>> I wrote some scripts to do some tests about irqs.
>> echo "1-3," > /proc/irq/<xxx>/smp_affinity_list
>> this command ends with ',' by mistake.
>> actually __bitmap_parselist() will report "0-3" for the final result which
>> is wrong.
>>
>
> Hmm...
> I don't think this is wrong passing echo "1-3,".
> With or without a comma, the final result must be the same.
> More flexible format is useful for hard scripts (for your one).
> It's not too difficult to imagine a script producing a line:
>           "1-24,  ,   ,,, ,  12-64, 92,92,92,,,"
> And I don't think we should reject user with this once the range is valid.
> Even more, to spend a time writing some additional code for it, and make
> user spend his time as well.
>
> I just tried
>            cd /home/yury///./././/work
> and it works perfectly well for me, and it's fine.
>
> The true problem is that a and b variables
> goes zero after comma, and EOL after comma just takes it:
>   514     do {
>   ...
>   517         a = b = 0;                                           //
> <--- comma makes it 0 here
>   ...
>   520         while (buflen) {
>   ...
>   539             /* A '\0' or a ',' signal the end of a cpu# or range */
>   540             if (c == '\0' || c == ',')                     //
> <---here we just break after '\0'
>   541                 break;
>   559         }
>   ...
>   565             while (a <= b) {
>   566                 set_bit(a, maskp);                   // <--- and
> here we set unneeded 0 bit.
>   567                 a++;
>   568             }
>
> So currently, "1-3,\0" is the same as "1-3,0,\0". And this is definitely wrong.
>
yes, you are right.
current codes did not check if there is any digit between ',' or '\0'.
I has sent out patch V2, which rewrite two functions.
could you help have a code review if you have free time? thanks for your nice reply :)

thanks,
xinhui

>>
>>>>
>>>> just like __bitmap_parse, we return -EINVAL if there is no avaiable digit
>>>> in each
>>>> parsing procedures.
>>>>
>>>> Signed-off-by: Pan Xinhui <xinhuix.pan@intel.com>
>>>
>>>
>>> Hello, Pan.
>>>
>>> (Adding Alexey Klimov, Rasmus Villemoes)
>>>
>>>> ---
>>>>     lib/bitmap.c | 7 +++++--
>>>>     1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/lib/bitmap.c b/lib/bitmap.c
>>>> index 64c0926..995fca2 100644
>>>> --- a/lib/bitmap.c
>>>> +++ b/lib/bitmap.c
>>>> @@ -504,7 +504,7 @@ static int __bitmap_parselist(const char *buf,
>>>> unsigned int buflen,
>>>>             int nmaskbits)
>>>>     {
>>>>         unsigned a, b;
>>>> -    int c, old_c, totaldigits;
>>>> +    int c, old_c, totaldigits, ndigits;
>>>>         const char __user __force *ubuf = (const char __user __force
>>>> *)buf;
>>>>         int exp_digit, in_range;
>>>>
>>>> @@ -514,6 +514,7 @@ static int __bitmap_parselist(const char *buf,
>>>> unsigned int buflen,
>>>>             exp_digit = 1;
>>>>             in_range = 0;
>>>>             a = b = 0;
>>>> +        ndigits = 0;
>>>>
>>>>             /* Get the next cpu# or a range of cpu#'s */
>>>>             while (buflen) {
>>>> @@ -555,8 +556,10 @@ static int __bitmap_parselist(const char *buf,
>>>> unsigned int buflen,
>>>>                 if (!in_range)
>>>>                     a = b;
>>>>                 exp_digit = 0;
>>>> -            totaldigits++;
>>>> +            ndigits++; totaldigits++;
>>>
>>>
>>> I'm not happy with joining two statements to a single line.
>>> Maybe sometimes it's OK for loop iterators like
>>>
>>>       while (a[i][j]) {
>>>           i++; j++;
>>>       }
>>>
>>> But here it looks nasty. Anyway, it's minor.
>>>
>>
>> thanks for pointing out my mistake about the code style :)
>>
>>>>             }
>>>> +        if (ndigits == 0)
>>>> +            return -EINVAL;
>>>
>>>
>>> You can avoid in-loop incrementation of ndigits if you'll
>>> save current totaldigits to ndigits before loop, and check
>>> ndigits against totaldigits after the loop:
>>>
>>>       ndigits = totaldigits;
>>>       while (...) {
>>>            ...
>>>           totaldigits++;
>>>       }
>>>
>>>       if (ndigits == totaldigits)
>>>           return -EINVAL;
>>>
>>> Maybe it's a good point to rework initial __bitmap_parse() similar way...
>>>
>>
>> your advice is a good idea, thanks.
>> I am also thinking if we can rewrite them into one function for common
>> codes.
>>
>> thanks for your reply again :)
>>
>> thanks
>> xinhui
>>
>>
>>>>             if (!(a <= b))
>>>>                 return -EINVAL;
>>>>             if (b >= nmaskbits)
>>>> --
>>>> 1.9.1