[PATCH v3 0/5] lib: rework bitmap_parselist and tests

[PATCH v3 0/5] lib: rework bitmap_parselist and tests

[Yury Norov]

bitmap_parselist has been evolved from a pretty simple idea for long and
now lacks for refactoring. It is not structured, has nested loops and a
set of opaque-named variables.

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

In this patchset
 - bitmap_parselist_user() made a wrapper on bitmap_parselist();
 - bitmap_parselist() reworked (patch 2);
 - time measurement in test_bitmap_parselist switched to ktime_get
   (patch 3);
 - new tests introduced (patch 4), and
 - bitmap_parselist_user() testing enabled with the same testset as
   bitmap_parselist() (patch 5).

v1: https://lkml.org/lkml/2018/12/23/50
v2: https://www.spinics.net/lists/kernel/msg3048976.html
v3: Implementation of an approach with copying the data to
    kernel space in bitmap_parselist_user() instead of parsing
    user data byte by byte. For me, it looks better than v2.

Yury Norov (5):
  lib: make bitmap_parselist_user() a wrapper on bitmap_parselist()
  lib: rework bitmap_parselist
  lib/test_bitmap: switch test_bitmap_parselist to ktime_get()
  lib/test_bitmap: add testcases for bitmap_parselist
  lib/test_bitmap: add tests for bitmap_parselist_user

 lib/bitmap.c      | 285 ++++++++++++++++++++++++++--------------------
 lib/test_bitmap.c |  67 ++++++++---
 2 files changed, 218 insertions(+), 134 deletions(-)

-- 
2.17.1

[PATCH 1/5] lib: make bitmap_parselist_user() a wrapper on bitmap_parselist()

[Yury Norov]

Currently we parse user data byte after byte which leads to
overcomplification of parsing algorithm. The only user of
bitmap_parselist_user() is not performance-critical, and so we
can copy user data to kernel buffer and simply call
bitmap_parselist(). This rework lets us unify and simplify
bitmap_parselist() and bitmap_parselist_user(), which is done
in the following patch.

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

diff --git a/lib/bitmap.c b/lib/bitmap.c
index 98872e9025da..ce9ea804d06d 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -632,19 +632,30 @@ EXPORT_SYMBOL(bitmap_parselist);
  * @nmaskbits: size of bitmap, in bits.
  *
  * Wrapper for bitmap_parselist(), providing it with user buffer.
- *
- * We cannot have this as an inline function in bitmap.h because it needs
- * linux/uaccess.h to get the access_ok() declaration and this causes
- * cyclic dependencies.
  */
 int bitmap_parselist_user(const char __user *ubuf,
 			unsigned int ulen, unsigned long *maskp,
 			int nmaskbits)
 {
-	if (!access_ok(ubuf, ulen))
-		return -EFAULT;
-	return __bitmap_parselist((const char __force *)ubuf,
-					ulen, 1, maskp, nmaskbits);
+	char *buf;
+	int ret;
+
+	buf = kmalloc(ulen + 1, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	buf[ulen] = 0;
+
+	ret = copy_from_user(buf, ubuf, ulen);
+	if (ret)
+		goto out;
+
+	ret = bitmap_parselist(buf, maskp, nmaskbits);
+
+out:
+	kfree(buf);
+
+	return ret;
 }
 EXPORT_SYMBOL(bitmap_parselist_user);
 
-- 
2.17.1

[PATCH 2/5] lib: rework bitmap_parselist

[Yury Norov]

Remove __bitmap_parselist helper and split the function to logical
parts.

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

diff --git a/lib/bitmap.c b/lib/bitmap.c
index ce9ea804d06d..0c08f18aed88 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -477,12 +477,133 @@ int bitmap_print_to_pagebuf(bool list, char *buf, const unsigned long *maskp,
 }
 EXPORT_SYMBOL(bitmap_print_to_pagebuf);
 
+/*
+ * Region 9-38:4/10 describes the following bitmap structure:
+ * 0	   9  12    18			38
+ * .........****......****......****......
+ *	    ^  ^     ^			 ^
+ *      start  off   group_len	       end
+ */
+struct region {
+	unsigned int start;
+	unsigned int off;
+	unsigned int group_len;
+	unsigned int end;
+};
+
+static int bitmap_set_region(const struct region *r,
+				unsigned long *bitmap, int nbits)
+{
+	unsigned int start;
+
+	if (r->end >= nbits)
+		return -ERANGE;
+
+	for (start = r->start; start <= r->end; start += r->group_len)
+		bitmap_set(bitmap, start, min(r->end - start + 1, r->off));
+
+	return 0;
+}
+
+static int bitmap_check_region(const struct region *r)
+{
+	if (r->start > r->end || r->group_len == 0 || r->off > r->group_len)
+		return -EINVAL;
+
+	return 0;
+}
+
+static const char *bitmap_getnum(const char *str, unsigned int *num)
+{
+	unsigned int n = 0, _num = 0;
+
+	if (!isdigit(*str))
+		return ERR_PTR(-EINVAL);
+
+	for (; isdigit(*str); str++) {
+		_num = _num * 10 + (*str - '0');
+		if (_num < n)
+			return ERR_PTR(-EOVERFLOW);
+
+		n = _num;
+	}
+
+	*num = _num;
+
+	return str;
+}
+
+static inline bool end_of_str(char c)
+{
+	return c == '\0' || c == '\n';
+}
+
+static inline bool __end_of_region(char c)
+{
+	return isspace(c) || c == ',';
+}
+
+static inline bool end_of_region(char c)
+{
+	return __end_of_region(c) || end_of_str(c);
+}
+
+/*
+ * The format allows commas and whitespases at the beginning
+ * of the region.
+ */
+static const char *bitmap_find_region(const char *str)
+{
+	while (__end_of_region(*str))
+		str++;
+
+	return end_of_str(*str) ? NULL : str;
+}
+
+static const char *bitmap_parse_region(const char *str, struct region *r)
+{
+	str = bitmap_getnum(str, &r->start);
+	if (IS_ERR(str))
+		return str;
+
+	if (end_of_region(*str))
+		goto no_end;
+
+	if (*str != '-')
+		return ERR_PTR(-EINVAL);
+
+	str = bitmap_getnum(str + 1, &r->end);
+	if (IS_ERR(str))
+		return str;
+
+	if (end_of_region(*str))
+		goto no_pattern;
+
+	if (*str != ':')
+		return ERR_PTR(-EINVAL);
+
+	str = bitmap_getnum(str + 1, &r->off);
+	if (IS_ERR(str))
+		return str;
+
+	if (*str != '/')
+		return ERR_PTR(-EINVAL);
+
+	return bitmap_getnum(str + 1, &r->group_len);
+
+no_end:
+	r->end = r->start;
+no_pattern:
+	r->off = r->end + 1;
+	r->group_len = r->end + 1;
+
+	return end_of_str(*str) ? NULL : str;
+}
+
 /**
- * __bitmap_parselist - convert list format ASCII string to bitmap
- * @buf: read nul-terminated user string from this buffer
- * @buflen: buffer size in bytes.  If string is smaller than this
- *    then it must be terminated with a \0.
- * @is_user: location of buffer, 0 indicates kernel space
+ * bitmap_parselist - convert list format ASCII string to bitmap
+ * @buf: read user string from this buffer; must be terminated
+ * with a \0 or \n.
  * @maskp: write resulting mask here
  * @nmaskbits: number of bits in mask to be written
  *
@@ -498,127 +619,38 @@ EXPORT_SYMBOL(bitmap_print_to_pagebuf);
  *
  * Returns: 0 on success, -errno on invalid input strings. Error values:
  *
- *   - ``-EINVAL``: second number in range smaller than first
+ *   - ``-EINVAL``: wrong region format
  *   - ``-EINVAL``: invalid character in string
  *   - ``-ERANGE``: bit number specified too large for mask
+ *   - ``-EOVERFLOW``: integer overflow in the input parameters
  */
-static int __bitmap_parselist(const char *buf, unsigned int buflen,
-		int is_user, unsigned long *maskp,
-		int nmaskbits)
+int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits)
 {
-	unsigned int a, b, old_a, old_b;
-	unsigned int group_size, used_size, off;
-	int c, old_c, totaldigits, ndigits;
-	const char __user __force *ubuf = (const char __user __force *)buf;
-	int at_start, in_range, in_partial_range;
+	struct region r;
+	long ret;
 
-	totaldigits = c = 0;
-	old_a = old_b = 0;
-	group_size = used_size = 0;
 	bitmap_zero(maskp, nmaskbits);
-	do {
-		at_start = 1;
-		in_range = 0;
-		in_partial_range = 0;
-		a = b = 0;
-		ndigits = totaldigits;
 
-		/* Get the next cpu# or a range of cpu#'s */
-		while (buflen) {
-			old_c = c;
-			if (is_user) {
-				if (__get_user(c, ubuf++))
-					return -EFAULT;
-			} else
-				c = *buf++;
-			buflen--;
-			if (isspace(c))
-				continue;
-
-			/* A '\0' or a ',' signal the end of a cpu# or range */
-			if (c == '\0' || c == ',')
-				break;
-			/*
-			* whitespaces between digits are not allowed,
-			* but it's ok if whitespaces are on head or tail.
-			* when old_c is whilespace,
-			* if totaldigits == ndigits, whitespace is on head.
-			* if whitespace is on tail, it should not run here.
-			* as c was ',' or '\0',
-			* the last code line has broken the current loop.
-			*/
-			if ((totaldigits != ndigits) && isspace(old_c))
-				return -EINVAL;
-
-			if (c == '/') {
-				used_size = a;
-				at_start = 1;
-				in_range = 0;
-				a = b = 0;
-				continue;
-			}
+	while (buf) {
+		buf = bitmap_find_region(buf);
+		if (buf == NULL)
+			return 0;
 
-			if (c == ':') {
-				old_a = a;
-				old_b = b;
-				at_start = 1;
-				in_range = 0;
-				in_partial_range = 1;
-				a = b = 0;
-				continue;
-			}
+		buf = bitmap_parse_region(buf, &r);
+		if (IS_ERR(buf))
+			return PTR_ERR(buf);
 
-			if (c == '-') {
-				if (at_start || in_range)
-					return -EINVAL;
-				b = 0;
-				in_range = 1;
-				at_start = 1;
-				continue;
-			}
+		ret = bitmap_check_region(&r);
+		if (ret)
+			return ret;
 
-			if (!isdigit(c))
-				return -EINVAL;
+		ret = bitmap_set_region(&r, maskp, nmaskbits);
+		if (ret)
+			return ret;
+	}
 
-			b = b * 10 + (c - '0');
-			if (!in_range)
-				a = b;
-			at_start = 0;
-			totaldigits++;
-		}
-		if (ndigits == totaldigits)
-			continue;
-		if (in_partial_range) {
-			group_size = a;
-			a = old_a;
-			b = old_b;
-			old_a = old_b = 0;
-		} else {
-			used_size = group_size = b - a + 1;
-		}
-		/* if no digit is after '-', it's wrong*/
-		if (at_start && in_range)
-			return -EINVAL;
-		if (!(a <= b) || group_size == 0 || !(used_size <= group_size))
-			return -EINVAL;
-		if (b >= nmaskbits)
-			return -ERANGE;
-		while (a <= b) {
-			off = min(b - a + 1, used_size);
-			bitmap_set(maskp, a, off);
-			a += group_size;
-		}
-	} while (buflen && c == ',');
 	return 0;
 }
-
-int bitmap_parselist(const char *bp, unsigned long *maskp, int nmaskbits)
-{
-	char *nl  = strchrnul(bp, '\n');
-	int len = nl - bp;
-
-	return __bitmap_parselist(bp, len, 0, maskp, nmaskbits);
-}
 EXPORT_SYMBOL(bitmap_parselist);
 
 
-- 
2.17.1

[PATCH 3/5] lib/test_bitmap: switch test_bitmap_parselist to ktime_get()

[Yury Norov]

test_bitmap_parselist currently uses get_cycles which is not
implemented on some platforms, so use ktime_get() instead.

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

diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
index 6cd7d0740005..b06e0fd3811b 100644
--- a/lib/test_bitmap.c
+++ b/lib/test_bitmap.c
@@ -266,15 +266,15 @@ static void __init test_bitmap_parselist(void)
 {
 	int i;
 	int err;
-	cycles_t cycles;
+	ktime_t time;
 	DECLARE_BITMAP(bmap, 2048);
 
 	for (i = 0; i < ARRAY_SIZE(parselist_tests); i++) {
 #define ptest parselist_tests[i]
 
-		cycles = get_cycles();
+		time = ktime_get();
 		err = bitmap_parselist(ptest.in, bmap, ptest.nbits);
-		cycles = get_cycles() - cycles;
+		time = ktime_get() - time;
 
 		if (err != ptest.errno) {
 			pr_err("test %d: input is %s, errno is %d, expected %d\n",
@@ -291,8 +291,7 @@ static void __init test_bitmap_parselist(void)
 
 		if (ptest.flags & PARSE_TIME)
 			pr_err("test %d: input is '%s' OK, Time: %llu\n",
-					i, ptest.in,
-					(unsigned long long)cycles);
+					i, ptest.in, time);
 	}
 }
 
-- 
2.17.1

Re: [PATCH v3 0/5] lib: rework bitmap_parselist and tests

[Yury Norov]

On Wed, Apr 03, 2019 at 07:45:35AM +0300, Yury Norov wrote:
> bitmap_parselist has been evolved from a pretty simple idea for long and
> now lacks for refactoring. It is not structured, has nested loops and a
> set of opaque-named variables.
> 
> Things are more complicated because bitmap_parselist() is a part of user
> interface, and its behavior should not change.
> 
> In this patchset
>  - bitmap_parselist_user() made a wrapper on bitmap_parselist();
>  - bitmap_parselist() reworked (patch 2);
>  - time measurement in test_bitmap_parselist switched to ktime_get
>    (patch 3);
>  - new tests introduced (patch 4), and
>  - bitmap_parselist_user() testing enabled with the same testset as
>    bitmap_parselist() (patch 5).
> 
> v1: https://lkml.org/lkml/2018/12/23/50
> v2: https://www.spinics.net/lists/kernel/msg3048976.html
> v3: Implementation of an approach with copying the data to
>     kernel space in bitmap_parselist_user() instead of parsing
>     user data byte by byte. For me, it looks better than v2.

Hi all,

I'm sorry for noise. My mail server works unstable, and it would be
better to submit patches 4 and 5 a bit later. They are actually the
same as in v2.

Yury

Re: [PATCH 1/5] lib: make bitmap_parselist_user() a wrapper on bitmap_parselist()

[Andy Shevchenko]

On Wed, Apr 03, 2019 at 07:45:36AM +0300, Yury Norov wrote:
> Currently we parse user data byte after byte which leads to
> overcomplification of parsing algorithm. The only user of
> bitmap_parselist_user() is not performance-critical, and so we
> can copy user data to kernel buffer and simply call
> bitmap_parselist(). This rework lets us unify and simplify
> bitmap_parselist() and bitmap_parselist_user(), which is done
> in the following patch.

> +	buf = kmalloc(ulen + 1, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	buf[ulen] = 0;
> +
> +	ret = copy_from_user(buf, ubuf, ulen);
> +	if (ret)
> +		goto out;

Why not memdup_user() ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/5] lib: make bitmap_parselist_user() a wrapper on bitmap_parselist()

[Andy Shevchenko]

On Wed, Apr 03, 2019 at 02:15:22PM +0300, Andy Shevchenko wrote:
> On Wed, Apr 03, 2019 at 07:45:36AM +0300, Yury Norov wrote:
> > Currently we parse user data byte after byte which leads to
> > overcomplification of parsing algorithm. The only user of
> > bitmap_parselist_user() is not performance-critical, and so we
> > can copy user data to kernel buffer and simply call
> > bitmap_parselist(). This rework lets us unify and simplify
> > bitmap_parselist() and bitmap_parselist_user(), which is done
> > in the following patch.
> 
> > +	buf = kmalloc(ulen + 1, GFP_KERNEL);
> > +	if (!buf)
> > +		return -ENOMEM;
> > +
> > +	buf[ulen] = 0;
> > +
> > +	ret = copy_from_user(buf, ubuf, ulen);
> > +	if (ret)
> > +		goto out;
> 
> Why not memdup_user() ?

Even more precisely (for strings) strndup_user().

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/5] lib: rework bitmap_parselist

[Andy Shevchenko]

On Wed, Apr 03, 2019 at 07:45:37AM +0300, Yury Norov wrote:
> Remove __bitmap_parselist helper and split the function to logical
> parts.

>  /**
> - * __bitmap_parselist - convert list format ASCII string to bitmap
> - * @buf: read nul-terminated user string from this buffer
> - * @buflen: buffer size in bytes.  If string is smaller than this
> - *    then it must be terminated with a \0.
> - * @is_user: location of buffer, 0 indicates kernel space
> + * bitmap_parselist - convert list format ASCII string to bitmap

> + * @buf: read user string from this buffer; must be terminated
> + * with a \0 or \n.

It's still good idea to put line continuation with deeper level of
indentation.

>   * @maskp: write resulting mask here
>   * @nmaskbits: number of bits in mask to be written
>   *
> @@ -498,127 +619,38 @@ EXPORT_SYMBOL(bitmap_print_to_pagebuf);
>   *
>   * Returns: 0 on success, -errno on invalid input strings. Error values:
>   *
> - *   - ``-EINVAL``: second number in range smaller than first
> + *   - ``-EINVAL``: wrong region format
>   *   - ``-EINVAL``: invalid character in string
>   *   - ``-ERANGE``: bit number specified too large for mask
> + *   - ``-EOVERFLOW``: integer overflow in the input parameters
>   */

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 0/5] lib: rework bitmap_parselist and tests

[Andy Shevchenko]

On Wed, Apr 03, 2019 at 07:54:27AM +0300, Yury Norov wrote:
> On Wed, Apr 03, 2019 at 07:45:35AM +0300, Yury Norov wrote:
> > bitmap_parselist has been evolved from a pretty simple idea for long and
> > now lacks for refactoring. It is not structured, has nested loops and a
> > set of opaque-named variables.
> > 
> > Things are more complicated because bitmap_parselist() is a part of user
> > interface, and its behavior should not change.
> > 
> > In this patchset
> >  - bitmap_parselist_user() made a wrapper on bitmap_parselist();
> >  - bitmap_parselist() reworked (patch 2);
> >  - time measurement in test_bitmap_parselist switched to ktime_get
> >    (patch 3);
> >  - new tests introduced (patch 4), and
> >  - bitmap_parselist_user() testing enabled with the same testset as
> >    bitmap_parselist() (patch 5).
> > 
> > v1: https://lkml.org/lkml/2018/12/23/50
> > v2: https://www.spinics.net/lists/kernel/msg3048976.html
> > v3: Implementation of an approach with copying the data to
> >     kernel space in bitmap_parselist_user() instead of parsing
> >     user data byte by byte. For me, it looks better than v2.

> I'm sorry for noise. My mail server works unstable, and it would be
> better to submit patches 4 and 5 a bit later. They are actually the
> same as in v2.

May you send then v4 with my (and others if any) comments being addressed?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 0/5] lib: rework bitmap_parselist and tests

[Andy Shevchenko]

On Wed, Apr 03, 2019 at 07:45:35AM +0300, Yury Norov wrote:
> bitmap_parselist has been evolved from a pretty simple idea for long and
> now lacks for refactoring. It is not structured, has nested loops and a
> set of opaque-named variables.
> 
> Things are more complicated because bitmap_parselist() is a part of user
> interface, and its behavior should not change.
> 
> In this patchset
>  - bitmap_parselist_user() made a wrapper on bitmap_parselist();
>  - bitmap_parselist() reworked (patch 2);
>  - time measurement in test_bitmap_parselist switched to ktime_get
>    (patch 3);
>  - new tests introduced (patch 4), and
>  - bitmap_parselist_user() testing enabled with the same testset as
>    bitmap_parselist() (patch 5).

Thanks for an update. Result looks good, though few comments per individual patches.

> 
> v1: https://lkml.org/lkml/2018/12/23/50
> v2: https://www.spinics.net/lists/kernel/msg3048976.html
> v3: Implementation of an approach with copying the data to
>     kernel space in bitmap_parselist_user() instead of parsing
>     user data byte by byte. For me, it looks better than v2.
> 
> Yury Norov (5):
>   lib: make bitmap_parselist_user() a wrapper on bitmap_parselist()
>   lib: rework bitmap_parselist
>   lib/test_bitmap: switch test_bitmap_parselist to ktime_get()
>   lib/test_bitmap: add testcases for bitmap_parselist
>   lib/test_bitmap: add tests for bitmap_parselist_user
> 
>  lib/bitmap.c      | 285 ++++++++++++++++++++++++++--------------------
>  lib/test_bitmap.c |  67 ++++++++---
>  2 files changed, 218 insertions(+), 134 deletions(-)
> 
> -- 
> 2.17.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/5] lib: make bitmap_parselist_user() a wrapper on bitmap_parselist()

[Rasmus Villemoes]

On 03/04/2019 13.17, Andy Shevchenko wrote:
> On Wed, Apr 03, 2019 at 02:15:22PM +0300, Andy Shevchenko wrote:
>> On Wed, Apr 03, 2019 at 07:45:36AM +0300, Yury Norov wrote:
>>> Currently we parse user data byte after byte which leads to
>>> overcomplification of parsing algorithm. The only user of
>>> bitmap_parselist_user() is not performance-critical, and so we
>>> can copy user data to kernel buffer and simply call
>>> bitmap_parselist(). This rework lets us unify and simplify
>>> bitmap_parselist() and bitmap_parselist_user(), which is done
>>> in the following patch.
>>
>>> +	buf = kmalloc(ulen + 1, GFP_KERNEL);
>>> +	if (!buf)
>>> +		return -ENOMEM;
>>> +
>>> +	buf[ulen] = 0;
>>> +
>>> +	ret = copy_from_user(buf, ubuf, ulen);
>>> +	if (ret)
>>> +		goto out;
>>
>> Why not memdup_user() ?
> 
> Even more precisely (for strings) strndup_user().
> 

But the user buffer is not nul-terminated, i.e. it's not a string. What
you want is memdup_user_nul() - take a length-delimited user buffer and
turn it into a nul-terminated string in kernel memory. And yes, please
use that.

Rasmus

Re: [PATCH 1/5] lib: make bitmap_parselist_user() a wrapper on bitmap_parselist()

[Andy Shevchenko]

On Wed, Apr 03, 2019 at 02:17:39PM +0200, Rasmus Villemoes wrote:
> On 03/04/2019 13.17, Andy Shevchenko wrote:
> > On Wed, Apr 03, 2019 at 02:15:22PM +0300, Andy Shevchenko wrote:
> >> On Wed, Apr 03, 2019 at 07:45:36AM +0300, Yury Norov wrote:
> >>> Currently we parse user data byte after byte which leads to
> >>> overcomplification of parsing algorithm. The only user of
> >>> bitmap_parselist_user() is not performance-critical, and so we
> >>> can copy user data to kernel buffer and simply call
> >>> bitmap_parselist(). This rework lets us unify and simplify
> >>> bitmap_parselist() and bitmap_parselist_user(), which is done
> >>> in the following patch.
> >>
> >>> +	buf = kmalloc(ulen + 1, GFP_KERNEL);
> >>> +	if (!buf)
> >>> +		return -ENOMEM;
> >>> +
> >>> +	buf[ulen] = 0;
> >>> +
> >>> +	ret = copy_from_user(buf, ubuf, ulen);
> >>> +	if (ret)
> >>> +		goto out;
> >>
> >> Why not memdup_user() ?
> > 
> > Even more precisely (for strings) strndup_user().
> > 
> 
> But the user buffer is not nul-terminated, i.e. it's not a string. 

Missed that. I stand corrected.

> What
> you want is memdup_user_nul() - take a length-delimited user buffer and
> turn it into a nul-terminated string in kernel memory.

Good, we have a helper.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/5] lib: make bitmap_parselist_user() a wrapper on bitmap_parselist()

[Andy Shevchenko]

On Mon, Apr 15, 2019 at 11:37:57PM -0700, Yury Norov wrote:
> From: Yury Norov <ynorov@marvell.com>
> 
> Currently we parse user data byte after byte which leads to
> overcomplification of parsing algorithm. The only user of
> bitmap_parselist_user() is not performance-critical, and so we
> can duplicate user data to kernel buffer and simply call
> bitmap_parselist(). This rework lets us unify and simplify
> bitmap_parselist() and bitmap_parselist_user(), which is done
> in the following patch.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> 
> Signed-off-by: Yury Norov <ynorov@marvell.com>
> ---
>  lib/bitmap.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/bitmap.c b/lib/bitmap.c
> index 3f3b8051f342..c63ddd06a5da 100644
> --- a/lib/bitmap.c
> +++ b/lib/bitmap.c
> @@ -632,19 +632,22 @@ EXPORT_SYMBOL(bitmap_parselist);
>   * @nmaskbits: size of bitmap, in bits.
>   *
>   * Wrapper for bitmap_parselist(), providing it with user buffer.
> - *
> - * We cannot have this as an inline function in bitmap.h because it needs
> - * linux/uaccess.h to get the access_ok() declaration and this causes
> - * cyclic dependencies.
>   */
>  int bitmap_parselist_user(const char __user *ubuf,
>  			unsigned int ulen, unsigned long *maskp,
>  			int nmaskbits)
>  {
> -	if (!access_ok(ubuf, ulen))
> -		return -EFAULT;
> -	return __bitmap_parselist((const char __force *)ubuf,
> -					ulen, 1, maskp, nmaskbits);
> +	char *buf;
> +	int ret;
> +
> +	buf = memdup_user_nul(ubuf, ulen);
> +	if (IS_ERR(buf))
> +		return PTR_ERR(buf);
> +
> +	ret = bitmap_parselist(buf, maskp, nmaskbits);
> +
> +	kfree(buf);
> +	return ret;
>  }
>  EXPORT_SYMBOL(bitmap_parselist_user);
>  
> -- 
> 2.17.1
> 

-- 
With Best Regards,
Andy Shevchenko

[PATCH 1/5] lib: make bitmap_parselist_user() a wrapper on bitmap_parselist()

[Yury Norov]

From: Yury Norov <ynorov@marvell.com>

Currently we parse user data byte after byte which leads to
overcomplification of parsing algorithm. The only user of
bitmap_parselist_user() is not performance-critical, and so we
can duplicate user data to kernel buffer and simply call
bitmap_parselist(). This rework lets us unify and simplify
bitmap_parselist() and bitmap_parselist_user(), which is done
in the following patch.

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

diff --git a/lib/bitmap.c b/lib/bitmap.c
index 3f3b8051f342..c63ddd06a5da 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -632,19 +632,22 @@ EXPORT_SYMBOL(bitmap_parselist);
  * @nmaskbits: size of bitmap, in bits.
  *
  * Wrapper for bitmap_parselist(), providing it with user buffer.
- *
- * We cannot have this as an inline function in bitmap.h because it needs
- * linux/uaccess.h to get the access_ok() declaration and this causes
- * cyclic dependencies.
  */
 int bitmap_parselist_user(const char __user *ubuf,
 			unsigned int ulen, unsigned long *maskp,
 			int nmaskbits)
 {
-	if (!access_ok(ubuf, ulen))
-		return -EFAULT;
-	return __bitmap_parselist((const char __force *)ubuf,
-					ulen, 1, maskp, nmaskbits);
+	char *buf;
+	int ret;
+
+	buf = memdup_user_nul(ubuf, ulen);
+	if (IS_ERR(buf))
+		return PTR_ERR(buf);
+
+	ret = bitmap_parselist(buf, maskp, nmaskbits);
+
+	kfree(buf);
+	return ret;
 }
 EXPORT_SYMBOL(bitmap_parselist_user);
 
-- 
2.17.1

Re: [PATCH 1/5] lib: make bitmap_parselist_user() a wrapper on bitmap_parselist()

[Andy Shevchenko]

On Fri, Apr 05, 2019 at 08:32:07PM +0300, Yury Norov wrote:
> Currently we parse user data byte after byte which leads to
> overcomplification of parsing algorithm. The only user of
> bitmap_parselist_user() is not performance-critical, and so we
> can duplicate user data to kernel buffer and simply call
> bitmap_parselist(). This rework lets us unify and simplify
> bitmap_parselist() and bitmap_parselist_user(), which is done
> in the following patch.
> 

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Yury Norov <ynorov@marvell.com>
> ---
>  lib/bitmap.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/bitmap.c b/lib/bitmap.c
> index 98872e9025da..79ed5538617b 100644
> --- a/lib/bitmap.c
> +++ b/lib/bitmap.c
> @@ -632,19 +632,22 @@ EXPORT_SYMBOL(bitmap_parselist);
>   * @nmaskbits: size of bitmap, in bits.
>   *
>   * Wrapper for bitmap_parselist(), providing it with user buffer.
> - *
> - * We cannot have this as an inline function in bitmap.h because it needs
> - * linux/uaccess.h to get the access_ok() declaration and this causes
> - * cyclic dependencies.
>   */
>  int bitmap_parselist_user(const char __user *ubuf,
>  			unsigned int ulen, unsigned long *maskp,
>  			int nmaskbits)
>  {
> -	if (!access_ok(ubuf, ulen))
> -		return -EFAULT;
> -	return __bitmap_parselist((const char __force *)ubuf,
> -					ulen, 1, maskp, nmaskbits);
> +	char *buf;
> +	int ret;
> +
> +	buf = memdup_user_nul(ubuf, ulen);
> +	if (IS_ERR(buf))
> +		return PTR_ERR(buf);
> +
> +	ret = bitmap_parselist(buf, maskp, nmaskbits);
> +
> +	kfree(buf);
> +	return ret;
>  }
>  EXPORT_SYMBOL(bitmap_parselist_user);
>  
> -- 
> 2.17.1
> 

-- 
With Best Regards,
Andy Shevchenko

[PATCH 1/5] lib: make bitmap_parselist_user() a wrapper on bitmap_parselist()

[Yury Norov]

Currently we parse user data byte after byte which leads to
overcomplification of parsing algorithm. The only user of
bitmap_parselist_user() is not performance-critical, and so we
can duplicate user data to kernel buffer and simply call
bitmap_parselist(). This rework lets us unify and simplify
bitmap_parselist() and bitmap_parselist_user(), which is done
in the following patch.

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

diff --git a/lib/bitmap.c b/lib/bitmap.c
index 98872e9025da..79ed5538617b 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -632,19 +632,22 @@ EXPORT_SYMBOL(bitmap_parselist);
  * @nmaskbits: size of bitmap, in bits.
  *
  * Wrapper for bitmap_parselist(), providing it with user buffer.
- *
- * We cannot have this as an inline function in bitmap.h because it needs
- * linux/uaccess.h to get the access_ok() declaration and this causes
- * cyclic dependencies.
  */
 int bitmap_parselist_user(const char __user *ubuf,
 			unsigned int ulen, unsigned long *maskp,
 			int nmaskbits)
 {
-	if (!access_ok(ubuf, ulen))
-		return -EFAULT;
-	return __bitmap_parselist((const char __force *)ubuf,
-					ulen, 1, maskp, nmaskbits);
+	char *buf;
+	int ret;
+
+	buf = memdup_user_nul(ubuf, ulen);
+	if (IS_ERR(buf))
+		return PTR_ERR(buf);
+
+	ret = bitmap_parselist(buf, maskp, nmaskbits);
+
+	kfree(buf);
+	return ret;
 }
 EXPORT_SYMBOL(bitmap_parselist_user);
 
-- 
2.17.1