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

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 <[email protected]>

---
  lib/bitmap.c | 7 +++++--
  1 file changed, 5 insertions(+), 2 deletions(-)

lib/bitmap.c +2 -1

index 64c0926..995fca2 100644
@@ -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) {

lib/bitmap.c +0 -1

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

1.9.1

Quoted 1 line

Sometimes the input from user may cause an unexpected result.

Could you please provide specific example?

Quoted 5 lines

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

Signed-off-by Pan Xinhui <[email protected]>

Hello, Pan.

(Adding Alexey Klimov, Rasmus Villemoes)

Quoted 26 lines

---
   lib/bitmap.c | 7 +++++--
   1 file changed, 5 insertions(+), 2 deletions(-)

lib/bitmap.c +2 -1

index 64c0926..995fca2 100644
@@ -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) {

lib/bitmap.c static int __bitmap_parselist(const char *buf, unsigned int buflen, · line 559

Quoted 5 lines

lib/bitmap.c +1 -1

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

lib/bitmap.c static int __bitmap_parselist(const char *buf, unsigned int buflen, · line 562

Quoted 3 lines

lib/bitmap.c +2 -0

           }
+        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...

Quoted 5 lines

lib/bitmap.c +0 -1

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

1.9.1

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 <[email protected]>

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

lib/bitmap.c +134 -104

index 64c0926..f2095e1d 100644
@@ -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)

Subject: 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 <[email protected]>

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

lib/bitmap.c +2 -0

index 64c0926..f2095e1d 100644
@@ -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);

lib/bitmap.c +123 -105

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

Quoted 1 line

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

Quoted 40 lines

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 <[email protected]>

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

lib/bitmap.c +2 -0

index 64c0926..f2095e1d 100644
@@ -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);
 

lib/bitmap.c again: · line 343

Quoted 9 lines

lib/bitmap.c +8 -0

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

Quoted 269 lines

lib/bitmap.c +123 -105

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

hi, Yury
thanks for your nice reply.

Quoted 4 lines · Yury Norov

Quoted 2 lines

Sometimes the input from user may cause an unexpected result.

Quoted 2 lines

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.

Quoted 53 lines

Quoted 7 lines

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

Signed-off-by Pan Xinhui <[email protected]>

Hello, Pan.

(Adding Alexey Klimov, Rasmus Villemoes)

Quoted 29 lines

---
   lib/bitmap.c | 7 +++++--
   1 file changed, 5 insertions(+), 2 deletions(-)

lib/bitmap.c +1 -1

index 64c0926..995fca2 100644
@@ -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;

lib/bitmap.c +1 -0

@@ -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) {

lib/bitmap.c static int __bitmap_parselist(const char *buf, · line 560

Quoted 6 lines

lib/bitmap.c +1 -1

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

Quoted 6 lines

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

Quoted 18 lines

lib/bitmap.c static int __bitmap_parselist(const char *buf, unsigned int buflen, · line 562

Quoted 3 lines

lib/bitmap.c +2 -0

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

Quoted 2 lines

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

Quoted 5 lines

Quoted 5 lines

lib/bitmap.c +0 -0

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

if (b >= nmaskbits)
--
1.9.1

2015-07-01 4:37 GMT+03:00 Pan Xinhui <[email protected]>:

Quoted 8 lines

hi, Yury
thanks for your nice reply.

Quoted 4 lines · Yury Norov

Quoted 2 lines

Sometimes the input from user may cause an unexpected result.

Quoted 8 lines

Quoted 2 lines

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.

Quoted 98 lines

Quoted 53 lines

Quoted 7 lines

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

Signed-off-by Pan Xinhui <[email protected]>

Hello, Pan.

(Adding Alexey Klimov, Rasmus Villemoes)

Quoted 29 lines

---
    lib/bitmap.c | 7 +++++--
    1 file changed, 5 insertions(+), 2 deletions(-)

lib/bitmap.c +1 -1

index 64c0926..995fca2 100644
@@ -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;

lib/bitmap.c +1 -0

@@ -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) {

lib/bitmap.c static int __bitmap_parselist(const char *buf, · line 560

Quoted 6 lines

lib/bitmap.c +1 -1

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

Quoted 6 lines

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

Quoted 18 lines

lib/bitmap.c static int __bitmap_parselist(const char *buf, unsigned int buflen, · line 562

Quoted 3 lines

lib/bitmap.c +2 -0

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

Quoted 2 lines

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

Quoted 5 lines

Quoted 5 lines

lib/bitmap.c +0 -0

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

if (b >= nmaskbits)
--
1.9.1