Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1174178 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] lib/bitmap.c: return -EINVAL for grouping errors in __bitmap_parselist Pan Xinhui <xinhuix.pan@intel.com> - 2015-06-30 03:40 +0200
    [PATCH V3] lib/bitmap.c: rewrite __bitmap_parse && __bitmap_parselist Pan Xinhui <xinhuix.pan@intel.com> - 2015-06-30 12:10 +0200

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web