Add parse_integer() (replacement for simple_strto*())

PATCH In review Alexey Dobriyan 2 participants 23 messages May 2, 2015 – May 6, 2015

Subsystems (13)

Memory management VFS Library code exec and binfmt +9 more subsystems

lore

Share & tools

lore

Alexey Dobriyan

author

2015-05-02 00:47 UTC #0 lore raw

PATCH 1/10 Add parse_integer() (replacement for simple_strto*())

kstrto*() and kstrto*_from_user() family of functions were added to help with parsing one integer written as string to proc/sysfs/debugfs files. But they have a limitation: string passed must end with \0 or \n\0. There are enough places where kstrto*() functions can't be used because of this limitation. Trivial example: major:minor "%u:%u".

Currently the only way to parse everything is simple_strto*() functions. But they are suboptimal:
* they do not detect overflow (can be fixed, but no one bothered since ~0.99.11),
* there are only 4 of them -- long and "long long" versions,
This leads to silent truncation in the most simple case:

	val = strtoul(s, NULL, 0);

* half of the people think that "char **endp" argument is necessary and add unnecessary variable.

OpenBSD people, fed up with how complex correct integer parsing is, added strtonum(3) to fixup for deficiencies of libc-style integer parsing:
http://www.openbsd.org/cgi-bin/man.cgi/OpenBSD-current/man3/strtonum.3?query=strtonum&arch=i386

It'd be OK to copy that but it relies on "errno" and fixed strings as error reporting channel which I think is not OK for kernel. strtonum() also doesn't report number of characted consumed.

What to do?

Enter parse_integer().

	int parse_integer(const char *s, unsigned int base, T *val);

Rationale:

* parse_integer() is exactly 1 (one) interface not 4 or many,one for each type.

* parse_integer() reports -E errors reliably in a canonical kernel way:

	rv = parse_integer(str, 10, &val);
	if (rv < 0)
		return rv;

* parse_integer() writes result only if there were no errors, at least one digit has to be consumed,

* parse_integer doesn't mix error reporting channel and value channel, it does mix error and number-of-character-consumed channel, though.

* parse_integer() reports number of characters consumed, makes parsing multiple values easy:

	rv = parse_integer(str, 0, &val1);
	if (rv < 0)
		return rv;
	str += rv;
	if (*str++ != ':')
		return -EINVAL;
	rv = parse_integer(str, 0, &val2);
	if (rv < 0)
		return rv;
	if (str[rv] != '\0')
		return -EINVAL;

There are several deficiencies in parse_integer() compared to strto*():
* can't be used in initializers:

	const T x = strtoul();

* can't be used with bitfields,
* can't be used in expressions:

	x = strtol() * 1024;
	x = y = strtol() * 1024;
	x += strtol();

* currently there is no support for _Bool, and at least one place where simple_strtoul() is directly assigned to _Bool variable. It is trivial to add, but not clear if it should only accept "0" and "1", because, say, module param code accepts 0, 1, y, n, Y and N (which I personally think is stupid).

In defense of parse_integer() all I can say, is that using strtol() in expression or initializer promotes no error checking and thus probably should not be encouraged in C, language with no built-in error checking anyway.

The amount of "x = y = strtol()" expressions in kernel is very small. The amount of direct usage in expression is not small, but can be counted as an acceptable loss.

Signed-off-by Alexey Dobriyan <[email protected]>

---

 include/linux/kernel.h        |    7 +
 include/linux/parse-integer.h |   79 ++++++++++++++++
 lib/Makefile                  |    1 
 lib/kstrtox.c                 |   76 ++++-----------
 lib/kstrtox.h                 |    1 
 lib/parse-integer.c           |  203 ++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 310 insertions(+), 57 deletions(-)

include/linux/kernel.h +5 -2

@@ -10,6 +10,7 @@
 #include <linux/bitops.h>
 #include <linux/log2.h>
 #include <linux/typecheck.h>
+#include <linux/parse-integer.h>
 #include <linux/printk.h>
 #include <linux/dynamic_debug.h>
 #include <asm/byteorder.h>

@@ -387,8 +388,10 @@ static inline int __must_check kstrtos32_from_user(const char __user *s, size_t
 	return kstrtoint_from_user(s, count, base, res);
 }
 
-/* Obsolete, do not use.  Use kstrto<foo> instead */
-
+/*
+ * Obsolete, do not use.
+ * Use parse_integer(), kstrto*(), kstrto*_from_user(), sscanf().
+ */
 extern unsigned long simple_strtoul(const char *,char **,unsigned int);
 extern long simple_strtol(const char *,char **,unsigned int);
 extern unsigned long long simple_strtoull(const char *,char **,unsigned int);

include/linux/parse-integer.h new file +79 -0

79 lines not on this page · raw message

lib/Makefile +1 -0

@@ -32,6 +32,7 @@ obj-$(CONFIG_TEST_STRING_HELPERS) += test-string_helpers.o
 obj-y += hexdump.o
 obj-$(CONFIG_TEST_HEXDUMP) += test-hexdump.o
 obj-y += kstrtox.o
+obj-y += parse-integer.o
 obj-$(CONFIG_TEST_BPF) += test_bpf.o
 obj-$(CONFIG_TEST_FIRMWARE) += test_firmware.o
 obj-$(CONFIG_TEST_KASAN) += test_kasan.o

lib/kstrtox.c +22 ~~-54~~

103 lines not on this page · raw message

lib/kstrtox.h +0 -1

@@ -2,7 +2,6 @@
 #define _LIB_KSTRTOX_H
 
 #define KSTRTOX_OVERFLOW	(1U << 31)
-const char *_parse_integer_fixup_radix(const char *s, unsigned int *base);
 unsigned int _parse_integer(const char *s, unsigned int base, unsigned long long *res);
 
 #endif

lib/parse-integer.c new file +203 -0

203 lines not on this page · raw message

Alexey Dobriyan

author

2015-05-02 00:48 UTC #1 lore raw

PATCH 2/10 parse_integer: rewrite kstrto*()

Rewrite kstrto*() functions through parse_integer().

_kstrtoul() and _kstrtol() are removed because parse_integer() can dispatch based on sizeof(long) saving function call.

Also move function definitions and comment one instance. Remove redundant boilerplate comments from elsewhere.

High bit base hack suggested by Andrew M.

Signed-off-by Alexey Dobriyan <[email protected]>

---

 include/linux/kernel.h        |  124 -----------------------
 include/linux/parse-integer.h |  111 +++++++++++++++++++++
 lib/kstrtox.c                 |  222 ------------------------------------------
 lib/parse-integer.c           |   38 ++++++-
 4 files changed, 145 insertions(+), 350 deletions(-)

include/linux/kernel.h +0 ~~-124~~

@@ -252,130 +252,6 @@ void do_exit(long error_code)
 void complete_and_exit(struct completion *, long)
 	__noreturn;
 
-/* Internal, do not use. */
-int __must_check _kstrtoul(const char *s, unsigned int base, unsigned long *res);
-int __must_check _kstrtol(const char *s, unsigned int base, long *res);
-
-int __must_check kstrtoull(const char *s, unsigned int base, unsigned long long *res);
-int __must_check kstrtoll(const char *s, unsigned int base, long long *res);
-
-/**
- * kstrtoul - convert a string to an unsigned long
- * @s: The start of the string. The string must be null-terminated, and may also
- *  include a single newline before its terminating null. The first character
- *  may also be a plus sign, but not a minus sign.
- * @base: The number base to use. The maximum supported base is 16. If base is
- *  given as 0, then the base of the string is automatically detected with the
- *  conventional semantics - If it begins with 0x the number will be parsed as a
- *  hexadecimal (case insensitive), if it otherwise begins with 0, it will be
- *  parsed as an octal number. Otherwise it will be parsed as a decimal.
- * @res: Where to write the result of the conversion on success.
- *
- * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the obsolete simple_strtoull. Return code must
- * be checked.
-*/
-static inline int __must_check kstrtoul(const char *s, unsigned int base, unsigned long *res)
-{
-	/*
-	 * We want to shortcut function call, but
-	 * __builtin_types_compatible_p(unsigned long, unsigned long long) = 0.
-	 */
-	if (sizeof(unsigned long) == sizeof(unsigned long long) &&
-	    __alignof__(unsigned long) == __alignof__(unsigned long long))
-		return kstrtoull(s, base, (unsigned long long *)res);
-	else
-		return _kstrtoul(s, base, res);
-}
-
-/**
- * kstrtol - convert a string to a long
- * @s: The start of the string. The string must be null-terminated, and may also
- *  include a single newline before its terminating null. The first character
- *  may also be a plus sign or a minus sign.
- * @base: The number base to use. The maximum supported base is 16. If base is
- *  given as 0, then the base of the string is automatically detected with the
- *  conventional semantics - If it begins with 0x the number will be parsed as a
- *  hexadecimal (case insensitive), if it otherwise begins with 0, it will be
- *  parsed as an octal number. Otherwise it will be parsed as a decimal.
- * @res: Where to write the result of the conversion on success.
- *
- * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the obsolete simple_strtoull. Return code must
- * be checked.
- */
-static inline int __must_check kstrtol(const char *s, unsigned int base, long *res)
-{
-	/*
-	 * We want to shortcut function call, but
-	 * __builtin_types_compatible_p(long, long long) = 0.
-	 */
-	if (sizeof(long) == sizeof(long long) &&
-	    __alignof__(long) == __alignof__(long long))
-		return kstrtoll(s, base, (long long *)res);
-	else
-		return _kstrtol(s, base, res);
-}
-
-int __must_check kstrtouint(const char *s, unsigned int base, unsigned int *res);
-int __must_check kstrtoint(const char *s, unsigned int base, int *res);
-
-static inline int __must_check kstrtou64(const char *s, unsigned int base, u64 *res)
-{
-	return kstrtoull(s, base, res);
-}
-
-static inline int __must_check kstrtos64(const char *s, unsigned int base, s64 *res)
-{
-	return kstrtoll(s, base, res);
-}
-
-static inline int __must_check kstrtou32(const char *s, unsigned int base, u32 *res)
-{
-	return kstrtouint(s, base, res);
-}
-
-static inline int __must_check kstrtos32(const char *s, unsigned int base, s32 *res)
-{
-	return kstrtoint(s, base, res);
-}
-
-int __must_check kstrtou16(const char *s, unsigned int base, u16 *res);
-int __must_check kstrtos16(const char *s, unsigned int base, s16 *res);
-int __must_check kstrtou8(const char *s, unsigned int base, u8 *res);
-int __must_check kstrtos8(const char *s, unsigned int base, s8 *res);
-
-int __must_check kstrtoull_from_user(const char __user *s, size_t count, unsigned int base, unsigned long long *res);
-int __must_check kstrtoll_from_user(const char __user *s, size_t count, unsigned int base, long long *res);
-int __must_check kstrtoul_from_user(const char __user *s, size_t count, unsigned int base, unsigned long *res);
-int __must_check kstrtol_from_user(const char __user *s, size_t count, unsigned int base, long *res);
-int __must_check kstrtouint_from_user(const char __user *s, size_t count, unsigned int base, unsigned int *res);
-int __must_check kstrtoint_from_user(const char __user *s, size_t count, unsigned int base, int *res);
-int __must_check kstrtou16_from_user(const char __user *s, size_t count, unsigned int base, u16 *res);
-int __must_check kstrtos16_from_user(const char __user *s, size_t count, unsigned int base, s16 *res);
-int __must_check kstrtou8_from_user(const char __user *s, size_t count, unsigned int base, u8 *res);
-int __must_check kstrtos8_from_user(const char __user *s, size_t count, unsigned int base, s8 *res);
-
-static inline int __must_check kstrtou64_from_user(const char __user *s, size_t count, unsigned int base, u64 *res)
-{
-	return kstrtoull_from_user(s, count, base, res);
-}
-
-static inline int __must_check kstrtos64_from_user(const char __user *s, size_t count, unsigned int base, s64 *res)
-{
-	return kstrtoll_from_user(s, count, base, res);
-}
-
-static inline int __must_check kstrtou32_from_user(const char __user *s, size_t count, unsigned int base, u32 *res)
-{
-	return kstrtouint_from_user(s, count, base, res);
-}
-
-static inline int __must_check kstrtos32_from_user(const char __user *s, size_t count, unsigned int base, s32 *res)
-{
-	return kstrtoint_from_user(s, count, base, res);
-}
-
 /* Obsolete, do not use. Use parse_integer(), kstrto*() or kstrto*_from_user(). */
 
 extern unsigned long simple_strtoul(const char *,char **,unsigned int);

include/linux/parse-integer.h +111 -0

116 lines not on this page · raw message

lib/kstrtox.c +0 ~~-222~~

228 lines not on this page · raw message

lib/parse-integer.c +34 -4

@@ -31,7 +31,7 @@ const char *_parse_integer_fixup_radix(const char *s, unsigned int *base)
 	return s;
 }
 
-static int __parse_integer(const char *s, unsigned int base, unsigned long long *val)
+static int ___parse_integer(const char *s, unsigned int base, unsigned long long *val)
 {
 	const char *s0 = s, *sd;
 	unsigned long long acc;

@@ -63,6 +63,27 @@ static int __parse_integer(const char *s, unsigned int base, unsigned long long
 	return s - s0;
 }
 
+static int __parse_integer(const char *s, unsigned int base, unsigned long long *val)
+{
+	if (base & PARSE_INTEGER_NEWLINE) {
+		unsigned long long _val;
+		int rv;
+
+		/* Accept "integer\0" or "integer\n\0" */
+		rv = ___parse_integer(s, base & ~PARSE_INTEGER_NEWLINE, &_val);
+		if (rv < 0)
+			return rv;
+		s += rv;
+		if (*s == '\n')
+			s++;
+		if (*s)
+			return -EINVAL;
+		*val = _val;
+		return 0;
+	} else
+		return ___parse_integer(s, base, val);
+}
+
 int _parse_integer_ull(const char *s, unsigned int base, unsigned long long *val)
 {
 	int rv;

@@ -73,7 +94,10 @@ int _parse_integer_ull(const char *s, unsigned int base, unsigned long long *val
 		rv = __parse_integer(s + 1, base, val);
 		if (rv < 0)
 			return rv;
-		return rv + 1;
+		if (base & PARSE_INTEGER_NEWLINE)
+			return 0;
+		else
+			return rv + 1;
 	} else
 		return __parse_integer(s, base, val);
 }

@@ -91,7 +115,10 @@ int _parse_integer_ll(const char *s, unsigned int base, long long *val)
 		if ((long long)-tmp >= 0)
 			return -ERANGE;
 		*val = -tmp;
-		return rv + 1;
+		if (base & PARSE_INTEGER_NEWLINE)
+			return 0;
+		else
+			return rv + 1;
 	} else if (*s == '+') {
 		rv = __parse_integer(s + 1, base, &tmp);
 		if (rv < 0)

@@ -99,7 +126,10 @@ int _parse_integer_ll(const char *s, unsigned int base, long long *val)
 		if ((long long)tmp < 0)
 			return -ERANGE;
 		*val = tmp;
-		return rv + 1;
+		if (base & PARSE_INTEGER_NEWLINE)
+			return 0;
+		else
+			return rv + 1;
 	} else {
 		rv = __parse_integer(s, base, &tmp);
 		if (rv < 0)

Signature · 572 lines

2.0.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Signed-off-by: Alexey Dobriyan <[email protected]>
---

 include/linux/kernel.h        |  124 -----------------------
 include/linux/parse-integer.h |  109 ++++++++++++++++++++
 lib/kstrtox.c                 |  222 ------------------------------------------
 lib/parse-integer.c           |   38 ++++++-
 4 files changed, 143 insertions(+), 350 deletions(-)

--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -264,130 +264,6 @@ void do_exit(long error_code)
 void complete_and_exit(struct completion *, long)
 	__noreturn;
 
-/* Internal, do not use. */
-int __must_check _kstrtoul(const char *s, unsigned int base, unsigned long *res);
-int __must_check _kstrtol(const char *s, unsigned int base, long *res);
-
-int __must_check kstrtoull(const char *s, unsigned int base, unsigned long long *res);
-int __must_check kstrtoll(const char *s, unsigned int base, long long *res);
-
-/**
- * kstrtoul - convert a string to an unsigned long
- * @s: The start of the string. The string must be null-terminated, and may also
- *  include a single newline before its terminating null. The first character
- *  may also be a plus sign, but not a minus sign.
- * @base: The number base to use. The maximum supported base is 16. If base is
- *  given as 0, then the base of the string is automatically detected with the
- *  conventional semantics - If it begins with 0x the number will be parsed as a
- *  hexadecimal (case insensitive), if it otherwise begins with 0, it will be
- *  parsed as an octal number. Otherwise it will be parsed as a decimal.
- * @res: Where to write the result of the conversion on success.
- *
- * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the obsolete simple_strtoull. Return code must
- * be checked.
-*/
-static inline int __must_check kstrtoul(const char *s, unsigned int base, unsigned long *res)
-{
-	/*
-	 * We want to shortcut function call, but
-	 * __builtin_types_compatible_p(unsigned long, unsigned long long) = 0.
-	 */
-	if (sizeof(unsigned long) == sizeof(unsigned long long) &&
-	    __alignof__(unsigned long) == __alignof__(unsigned long long))
-		return kstrtoull(s, base, (unsigned long long *)res);
-	else
-		return _kstrtoul(s, base, res);
-}
-
-/**
- * kstrtol - convert a string to a long
- * @s: The start of the string. The string must be null-terminated, and may also
- *  include a single newline before its terminating null. The first character
- *  may also be a plus sign or a minus sign.
- * @base: The number base to use. The maximum supported base is 16. If base is
- *  given as 0, then the base of the string is automatically detected with the
- *  conventional semantics - If it begins with 0x the number will be parsed as a
- *  hexadecimal (case insensitive), if it otherwise begins with 0, it will be
- *  parsed as an octal number. Otherwise it will be parsed as a decimal.
- * @res: Where to write the result of the conversion on success.
- *
- * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the obsolete simple_strtoull. Return code must
- * be checked.
- */
-static inline int __must_check kstrtol(const char *s, unsigned int base, long *res)
-{
-	/*
-	 * We want to shortcut function call, but
-	 * __builtin_types_compatible_p(long, long long) = 0.
-	 */
-	if (sizeof(long) == sizeof(long long) &&
-	    __alignof__(long) == __alignof__(long long))
-		return kstrtoll(s, base, (long long *)res);
-	else
-		return _kstrtol(s, base, res);
-}
-
-int __must_check kstrtouint(const char *s, unsigned int base, unsigned int *res);
-int __must_check kstrtoint(const char *s, unsigned int base, int *res);
-
-static inline int __must_check kstrtou64(const char *s, unsigned int base, u64 *res)
-{
-	return kstrtoull(s, base, res);
-}
-
-static inline int __must_check kstrtos64(const char *s, unsigned int base, s64 *res)
-{
-	return kstrtoll(s, base, res);
-}
-
-static inline int __must_check kstrtou32(const char *s, unsigned int base, u32 *res)
-{
-	return kstrtouint(s, base, res);
-}
-
-static inline int __must_check kstrtos32(const char *s, unsigned int base, s32 *res)
-{
-	return kstrtoint(s, base, res);
-}
-
-int __must_check kstrtou16(const char *s, unsigned int base, u16 *res);
-int __must_check kstrtos16(const char *s, unsigned int base, s16 *res);
-int __must_check kstrtou8(const char *s, unsigned int base, u8 *res);
-int __must_check kstrtos8(const char *s, unsigned int base, s8 *res);
-
-int __must_check kstrtoull_from_user(const char __user *s, size_t count, unsigned int base, unsigned long long *res);
-int __must_check kstrtoll_from_user(const char __user *s, size_t count, unsigned int base, long long *res);
-int __must_check kstrtoul_from_user(const char __user *s, size_t count, unsigned int base, unsigned long *res);
-int __must_check kstrtol_from_user(const char __user *s, size_t count, unsigned int base, long *res);
-int __must_check kstrtouint_from_user(const char __user *s, size_t count, unsigned int base, unsigned int *res);
-int __must_check kstrtoint_from_user(const char __user *s, size_t count, unsigned int base, int *res);
-int __must_check kstrtou16_from_user(const char __user *s, size_t count, unsigned int base, u16 *res);
-int __must_check kstrtos16_from_user(const char __user *s, size_t count, unsigned int base, s16 *res);
-int __must_check kstrtou8_from_user(const char __user *s, size_t count, unsigned int base, u8 *res);
-int __must_check kstrtos8_from_user(const char __user *s, size_t count, unsigned int base, s8 *res);
-
-static inline int __must_check kstrtou64_from_user(const char __user *s, size_t count, unsigned int base, u64 *res)
-{
-	return kstrtoull_from_user(s, count, base, res);
-}
-
-static inline int __must_check kstrtos64_from_user(const char __user *s, size_t count, unsigned int base, s64 *res)
-{
-	return kstrtoll_from_user(s, count, base, res);
-}
-
-static inline int __must_check kstrtou32_from_user(const char __user *s, size_t count, unsigned int base, u32 *res)
-{
-	return kstrtouint_from_user(s, count, base, res);
-}
-
-static inline int __must_check kstrtos32_from_user(const char __user *s, size_t count, unsigned int base, s32 *res)
-{
-	return kstrtoint_from_user(s, count, base, res);
-}
-
 /*
  * Obsolete, do not use.
  * Use parse_integer(), kstrto*(), kstrto*_from_user(), sscanf().
--- a/include/linux/parse-integer.h
+++ b/include/linux/parse-integer.h
@@ -76,4 +76,113 @@ int _parse_integer_ll(const char *s, unsigned int base, long long *val);
 int _parse_integer_ull(const char *s, unsigned int base, unsigned long long *val);
 void _parse_integer_link_time_error(void);
 const char *_parse_integer_fixup_radix(const char *s, unsigned int *base);
+#define PARSE_INTEGER_NEWLINE 0x80000000u
+
+/*
+ * Convert integer string representation terminated by \n\0 or \0 to an integer.
+ *
+ * Return 0 on success or -E.
+ *
+ * See parse_integer().
+ */
+static inline int __must_check kstrtoull(const char *s, unsigned int base, unsigned long long *res)
+{
+	return parse_integer(s, base | PARSE_INTEGER_NEWLINE, res);
+}
+
+static inline int __must_check kstrtoll(const char *s, unsigned int base, long long *res)
+{
+	return parse_integer(s, base | PARSE_INTEGER_NEWLINE, res);
+}
+
+static inline int __must_check kstrtoul(const char *s, unsigned int base, unsigned long *res)
+{
+	return parse_integer(s, base | PARSE_INTEGER_NEWLINE, res);
+}
+
+static inline int __must_check kstrtol(const char *s, unsigned int base, long *res)
+{
+	return parse_integer(s, base | PARSE_INTEGER_NEWLINE, res);
+}
+
+static inline int __must_check kstrtouint(const char *s, unsigned int base, unsigned int *res)
+{
+	return parse_integer(s, base | PARSE_INTEGER_NEWLINE, res);
+}
+
+static inline int __must_check kstrtoint(const char *s, unsigned int base, int *res)
+{
+	return parse_integer(s, base | PARSE_INTEGER_NEWLINE, res);
+}
+
+static inline int __must_check kstrtou64(const char *s, unsigned int base, u64 *res)
+{
+	return kstrtoull(s, base, res);
+}
+
+static inline int __must_check kstrtos64(const char *s, unsigned int base, s64 *res)
+{
+	return kstrtoll(s, base, res);
+}
+
+static inline int __must_check kstrtou32(const char *s, unsigned int base, u32 *res)
+{
+	return kstrtouint(s, base, res);
+}
+
+static inline int __must_check kstrtos32(const char *s, unsigned int base, s32 *res)
+{
+	return kstrtoint(s, base, res);
+}
+
+static inline int __must_check kstrtou16(const char *s, unsigned int base, u16 *res)
+{
+	return parse_integer(s, base | PARSE_INTEGER_NEWLINE, res);
+}
+
+static inline int __must_check kstrtos16(const char *s, unsigned int base, s16 *res)
+{
+	return parse_integer(s, base | PARSE_INTEGER_NEWLINE, res);
+}
+
+static inline int __must_check kstrtou8(const char *s, unsigned int base, u8 *res)
+{
+	return parse_integer(s, base | PARSE_INTEGER_NEWLINE, res);
+}
+
+static inline int __must_check kstrtos8(const char *s, unsigned int base, s8 *res)
+{
+	return parse_integer(s, base | PARSE_INTEGER_NEWLINE, res);
+}
+
+int __must_check kstrtoull_from_user(const char __user *s, size_t count, unsigned int base, unsigned long long *res);
+int __must_check kstrtoll_from_user(const char __user *s, size_t count, unsigned int base, long long *res);
+int __must_check kstrtoul_from_user(const char __user *s, size_t count, unsigned int base, unsigned long *res);
+int __must_check kstrtol_from_user(const char __user *s, size_t count, unsigned int base, long *res);
+int __must_check kstrtouint_from_user(const char __user *s, size_t count, unsigned int base, unsigned int *res);
+int __must_check kstrtoint_from_user(const char __user *s, size_t count, unsigned int base, int *res);
+int __must_check kstrtou16_from_user(const char __user *s, size_t count, unsigned int base, u16 *res);
+int __must_check kstrtos16_from_user(const char __user *s, size_t count, unsigned int base, s16 *res);
+int __must_check kstrtou8_from_user(const char __user *s, size_t count, unsigned int base, u8 *res);
+int __must_check kstrtos8_from_user(const char __user *s, size_t count, unsigned int base, s8 *res);
+
+static inline int __must_check kstrtou64_from_user(const char __user *s, size_t count, unsigned int base, u64 *res)
+{
+	return kstrtoull_from_user(s, count, base, res);
+}
+
+static inline int __must_check kstrtos64_from_user(const char __user *s, size_t count, unsigned int base, s64 *res)
+{
+	return kstrtoll_from_user(s, count, base, res);
+}
+
+static inline int __must_check kstrtou32_from_user(const char __user *s, size_t count, unsigned int base, u32 *res)
+{
+	return kstrtouint_from_user(s, count, base, res);
+}
+
+static inline int __must_check kstrtos32_from_user(const char __user *s, size_t count, unsigned int base, s32 *res)
+{
+	return kstrtoint_from_user(s, count, base, res);
+}
 #endif
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c
@@ -67,228 +67,6 @@ unsigned int _parse_integer(const char *s, unsigned int base, unsigned long long
 	return rv;
 }
 
-/**
- * kstrtoull - convert a string to an unsigned long long
- * @s: The start of the string. The string must be null-terminated, and may also
- *  include a single newline before its terminating null. The first character
- *  may also be a plus sign, but not a minus sign.
- * @base: The number base to use. The maximum supported base is 16. If base is
- *  given as 0, then the base of the string is automatically detected with the
- *  conventional semantics - If it begins with 0x the number will be parsed as a
- *  hexadecimal (case insensitive), if it otherwise begins with 0, it will be
- *  parsed as an octal number. Otherwise it will be parsed as a decimal.
- * @res: Where to write the result of the conversion on success.
- *
- * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the obsolete simple_strtoull. Return code must
- * be checked.
- */
-int kstrtoull(const char *s, unsigned int base, unsigned long long *res)
-{
-	unsigned long long _res;
-	int rv;
-
-	rv = parse_integer(s, base, &_res);
-	if (rv < 0)
-		return rv;
-	s += rv;
-	if (*s == '\n')
-		s++;
-	if (*s)
-		return -EINVAL;
-	*res = _res;
-	return 0;
-}
-EXPORT_SYMBOL(kstrtoull);
-
-/**
- * kstrtoll - convert a string to a long long
- * @s: The start of the string. The string must be null-terminated, and may also
- *  include a single newline before its terminating null. The first character
- *  may also be a plus sign or a minus sign.
- * @base: The number base to use. The maximum supported base is 16. If base is
- *  given as 0, then the base of the string is automatically detected with the
- *  conventional semantics - If it begins with 0x the number will be parsed as a
- *  hexadecimal (case insensitive), if it otherwise begins with 0, it will be
- *  parsed as an octal number. Otherwise it will be parsed as a decimal.
- * @res: Where to write the result of the conversion on success.
- *
- * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the obsolete simple_strtoull. Return code must
- * be checked.
- */
-int kstrtoll(const char *s, unsigned int base, long long *res)
-{
-	long long _res;
-	int rv;
-
-	rv = parse_integer(s, base, &_res);
-	if (rv < 0)
-		return rv;
-	s += rv;
-	if (*s == '\n')
-		s++;
-	if (*s)
-		return -EINVAL;
-	*res = _res;
-	return 0;
-}
-EXPORT_SYMBOL(kstrtoll);
-
-/* Internal, do not use. */
-int _kstrtoul(const char *s, unsigned int base, unsigned long *res)
-{
-	unsigned long long tmp;
-	int rv;
-
-	rv = kstrtoull(s, base, &tmp);
-	if (rv < 0)
-		return rv;
-	if (tmp != (unsigned long long)(unsigned long)tmp)
-		return -ERANGE;
-	*res = tmp;
-	return 0;
-}
-EXPORT_SYMBOL(_kstrtoul);
-
-/* Internal, do not use. */
-int _kstrtol(const char *s, unsigned int base, long *res)
-{
-	long long tmp;
-	int rv;
-
-	rv = kstrtoll(s, base, &tmp);
-	if (rv < 0)
-		return rv;
-	if (tmp != (long long)(long)tmp)
-		return -ERANGE;
-	*res = tmp;
-	return 0;
-}
-EXPORT_SYMBOL(_kstrtol);
-
-/**
- * kstrtouint - convert a string to an unsigned int
- * @s: The start of the string. The string must be null-terminated, and may also
- *  include a single newline before its terminating null. The first character
- *  may also be a plus sign, but not a minus sign.
- * @base: The number base to use. The maximum supported base is 16. If base is
- *  given as 0, then the base of the string is automatically detected with the
- *  conventional semantics - If it begins with 0x the number will be parsed as a
- *  hexadecimal (case insensitive), if it otherwise begins with 0, it will be
- *  parsed as an octal number. Otherwise it will be parsed as a decimal.
- * @res: Where to write the result of the conversion on success.
- *
- * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the obsolete simple_strtoull. Return code must
- * be checked.
- */
-int kstrtouint(const char *s, unsigned int base, unsigned int *res)
-{
-	unsigned long long tmp;
-	int rv;
-
-	rv = kstrtoull(s, base, &tmp);
-	if (rv < 0)
-		return rv;
-	if (tmp != (unsigned long long)(unsigned int)tmp)
-		return -ERANGE;
-	*res = tmp;
-	return 0;
-}
-EXPORT_SYMBOL(kstrtouint);
-
-/**
- * kstrtoint - convert a string to an int
- * @s: The start of the string. The string must be null-terminated, and may also
- *  include a single newline before its terminating null. The first character
- *  may also be a plus sign or a minus sign.
- * @base: The number base to use. The maximum supported base is 16. If base is
- *  given as 0, then the base of the string is automatically detected with the
- *  conventional semantics - If it begins with 0x the number will be parsed as a
- *  hexadecimal (case insensitive), if it otherwise begins with 0, it will be
- *  parsed as an octal number. Otherwise it will be parsed as a decimal.
- * @res: Where to write the result of the conversion on success.
- *
- * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the obsolete simple_strtoull. Return code must
- * be checked.
- */
-int kstrtoint(const char *s, unsigned int base, int *res)
-{
-	long long tmp;
-	int rv;
-
-	rv = kstrtoll(s, base, &tmp);
-	if (rv < 0)
-		return rv;
-	if (tmp != (long long)(int)tmp)
-		return -ERANGE;
-	*res = tmp;
-	return 0;
-}
-EXPORT_SYMBOL(kstrtoint);
-
-int kstrtou16(const char *s, unsigned int base, u16 *res)
-{
-	unsigned long long tmp;
-	int rv;
-
-	rv = kstrtoull(s, base, &tmp);
-	if (rv < 0)
-		return rv;
-	if (tmp != (unsigned long long)(u16)tmp)
-		return -ERANGE;
-	*res = tmp;
-	return 0;
-}
-EXPORT_SYMBOL(kstrtou16);
-
-int kstrtos16(const char *s, unsigned int base, s16 *res)
-{
-	long long tmp;
-	int rv;
-
-	rv = kstrtoll(s, base, &tmp);
-	if (rv < 0)
-		return rv;
-	if (tmp != (long long)(s16)tmp)
-		return -ERANGE;
-	*res = tmp;
-	return 0;
-}
-EXPORT_SYMBOL(kstrtos16);
-
-int kstrtou8(const char *s, unsigned int base, u8 *res)
-{
-	unsigned long long tmp;
-	int rv;
-
-	rv = kstrtoull(s, base, &tmp);
-	if (rv < 0)
-		return rv;
-	if (tmp != (unsigned long long)(u8)tmp)
-		return -ERANGE;
-	*res = tmp;
-	return 0;
-}
-EXPORT_SYMBOL(kstrtou8);
-
-int kstrtos8(const char *s, unsigned int base, s8 *res)
-{
-	long long tmp;
-	int rv;
-
-	rv = kstrtoll(s, base, &tmp);
-	if (rv < 0)
-		return rv;
-	if (tmp != (long long)(s8)tmp)
-		return -ERANGE;
-	*res = tmp;
-	return 0;
-}
-EXPORT_SYMBOL(kstrtos8);
-
 #define kstrto_from_user(f, g, type)					\
 int f(const char __user *s, size_t count, unsigned int base, type *res)	\
 {									\
--- a/lib/parse-integer.c
+++ b/lib/parse-integer.c
@@ -31,7 +31,7 @@ const char *_parse_integer_fixup_radix(const char *s, unsigned int *base)
 	return s;
 }
 
-static int __parse_integer(const char *s, unsigned int base, unsigned long long *val)
+static int ___parse_integer(const char *s, unsigned int base, unsigned long long *val)
 {
 	const char *s0 = s, *sd;
 	unsigned long long acc;
@@ -63,6 +63,27 @@ static int __parse_integer(const char *s, unsigned int base, unsigned long long
 	return s - s0;
 }
 
+static int __parse_integer(const char *s, unsigned int base, unsigned long long *val)
+{
+	if (base & PARSE_INTEGER_NEWLINE) {
+		unsigned long long _val;
+		int rv;
+
+		/* Accept "integer\0" or "integer\n\0" */
+		rv = ___parse_integer(s, base & ~PARSE_INTEGER_NEWLINE, &_val);
+		if (rv < 0)
+			return rv;
+		s += rv;
+		if (*s == '\n')
+			s++;
+		if (*s)
+			return -EINVAL;
+		*val = _val;
+		return 0;
+	} else
+		return ___parse_integer(s, base, val);
+}
+
 int _parse_integer_ull(const char *s, unsigned int base, unsigned long long *val)
 {
 	int rv;
@@ -73,7 +94,10 @@ int _parse_integer_ull(const char *s, unsigned int base, unsigned long long *val
 		rv = __parse_integer(s + 1, base, val);
 		if (rv < 0)
 			return rv;
-		return rv + 1;
+		if (base & PARSE_INTEGER_NEWLINE)
+			return 0;
+		else
+			return rv + 1;
 	} else
 		return __parse_integer(s, base, val);
 }
@@ -91,7 +115,10 @@ int _parse_integer_ll(const char *s, unsigned int base, long long *val)
 		if ((long long)-tmp >= 0)
 			return -ERANGE;
 		*val = -tmp;
-		return rv + 1;
+		if (base & PARSE_INTEGER_NEWLINE)
+			return 0;
+		else
+			return rv + 1;
 	} else if (*s == '+') {
 		rv = __parse_integer(s + 1, base, &tmp);
 		if (rv < 0)
@@ -99,7 +126,10 @@ int _parse_integer_ll(const char *s, unsigned int base, long long *val)
 		if ((long long)tmp < 0)
 			return -ERANGE;
 		*val = tmp;
-		return rv + 1;
+		if (base & PARSE_INTEGER_NEWLINE)
+			return 0;
+		else
+			return rv + 1;
 	} else {
 		rv = __parse_integer(s, base, &tmp);
 		if (rv < 0)

Alexey Dobriyan

author

2015-05-02 00:50 UTC #2 lore raw

PATCH 3/10 parse_integer: convert sscanf()

In reply to Alexey Dobriyan

Remove base second guessing.

Uniformly fix too liberal acceptance in %lu/%ld cases in the next patch.

Signed-off-by Alexey Dobriyan <[email protected]>

---

 lib/vsprintf.c |   36 ++++++++++--------------------------
 1 file changed, 10 insertions(+), 26 deletions(-)

lib/vsprintf.c +10 ~~-26~~

@@ -2470,8 +2470,6 @@ EXPORT_SYMBOL_GPL(bprintf);
 int vsscanf(const char *buf, const char *fmt, va_list args)
 {
 	const char *str = buf;
-	char *next;
-	char digit;
 	int num = 0;
 	u8 qualifier;
 	unsigned int base;

@@ -2483,6 +2481,8 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 	bool is_sign;
 
 	while (*fmt) {
+		int len;
+
 		/* skip any white space in format */
 		/* white space in format matchs any amount of
 		 * white space, including none, in the input.

@@ -2611,35 +2611,22 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 		 */
 		str = skip_spaces(str);
 
-		digit = *str;
-		if (is_sign && digit == '-')
-			digit = *(str + 1);
-
-		if (!digit
-		    || (base == 16 && !isxdigit(digit))
-		    || (base == 10 && !isdigit(digit))
-		    || (base == 8 && (!isdigit(digit) || digit > '7'))
-		    || (base == 0 && !isdigit(digit)))
-			break;
-
 		if (is_sign)
-			val.s = qualifier != 'L' ?
-				simple_strtol(str, &next, base) :
-				simple_strtoll(str, &next, base);
+			len = parse_integer(str, base, &val.s);
 		else
-			val.u = qualifier != 'L' ?
-				simple_strtoul(str, &next, base) :
-				simple_strtoull(str, &next, base);
+			len = parse_integer(str, base, &val.u);
+		if (len < 0)
+			break;
 
-		if (field_width > 0 && next - str > field_width) {
+		if (field_width > 0) {
 			if (base == 0)
 				_parse_integer_fixup_radix(str, &base);
-			while (next - str > field_width) {
+			while (len > field_width) {
 				if (is_sign)
 					val.s = div_s64(val.s, base);
 				else
 					val.u = div_u64(val.u, base);
-				--next;
+				len--;
 			}
 		}

@@ -2680,10 +2667,7 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 			break;
 		}
 		num++;
-
-		if (!next)
-			break;
-		str = next;
+		str += len;
 	}
 
 	return num;

Alexey Dobriyan

author

2015-05-02 01:10 UTC #3 lore raw

PATCH 3/10 parse_integer: convert sscanf()

Rewrite kstrto*() functions through parse_integer().

_kstrtoul() and _kstrtol() are removed because parse_integer() can dispatch based on BITS_PER_LONG saving function call.

Also move function definitions and comment one instance. Remove redundant boilerplate comments from elsewhere.

High bit base hack suggested by Andrew M.

Signed-off-by Alexey Dobriyan <[email protected]>

---

I copied patch twice, lol.

 include/linux/kernel.h        |  124 -----------------------
 include/linux/parse-integer.h |  109 ++++++++++++++++++++
 lib/kstrtox.c                 |  222 ------------------------------------------
 lib/parse-integer.c           |   38 ++++++-
 4 files changed, 143 insertions(+), 350 deletions(-)

include/linux/kernel.h +0 ~~-124~~

@@ -264,130 +264,6 @@ void do_exit(long error_code)
 void complete_and_exit(struct completion *, long)
 	__noreturn;
 
-/* Internal, do not use. */
-int __must_check _kstrtoul(const char *s, unsigned int base, unsigned long *res);
-int __must_check _kstrtol(const char *s, unsigned int base, long *res);
-
-int __must_check kstrtoull(const char *s, unsigned int base, unsigned long long *res);
-int __must_check kstrtoll(const char *s, unsigned int base, long long *res);
-
-/**
- * kstrtoul - convert a string to an unsigned long
- * @s: The start of the string. The string must be null-terminated, and may also
- *  include a single newline before its terminating null. The first character
- *  may also be a plus sign, but not a minus sign.
- * @base: The number base to use. The maximum supported base is 16. If base is
- *  given as 0, then the base of the string is automatically detected with the
- *  conventional semantics - If it begins with 0x the number will be parsed as a
- *  hexadecimal (case insensitive), if it otherwise begins with 0, it will be
- *  parsed as an octal number. Otherwise it will be parsed as a decimal.
- * @res: Where to write the result of the conversion on success.
- *
- * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the obsolete simple_strtoull. Return code must
- * be checked.
-*/
-static inline int __must_check kstrtoul(const char *s, unsigned int base, unsigned long *res)
-{
-	/*
-	 * We want to shortcut function call, but
-	 * __builtin_types_compatible_p(unsigned long, unsigned long long) = 0.
-	 */
-	if (sizeof(unsigned long) == sizeof(unsigned long long) &&
-	    __alignof__(unsigned long) == __alignof__(unsigned long long))
-		return kstrtoull(s, base, (unsigned long long *)res);
-	else
-		return _kstrtoul(s, base, res);
-}
-
-/**
- * kstrtol - convert a string to a long
- * @s: The start of the string. The string must be null-terminated, and may also
- *  include a single newline before its terminating null. The first character
- *  may also be a plus sign or a minus sign.
- * @base: The number base to use. The maximum supported base is 16. If base is
- *  given as 0, then the base of the string is automatically detected with the
- *  conventional semantics - If it begins with 0x the number will be parsed as a
- *  hexadecimal (case insensitive), if it otherwise begins with 0, it will be
- *  parsed as an octal number. Otherwise it will be parsed as a decimal.
- * @res: Where to write the result of the conversion on success.
- *
- * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
- * Used as a replacement for the obsolete simple_strtoull. Return code must
- * be checked.
- */
-static inline int __must_check kstrtol(const char *s, unsigned int base, long *res)
-{
-	/*
-	 * We want to shortcut function call, but
-	 * __builtin_types_compatible_p(long, long long) = 0.
-	 */
-	if (sizeof(long) == sizeof(long long) &&
-	    __alignof__(long) == __alignof__(long long))
-		return kstrtoll(s, base, (long long *)res);
-	else
-		return _kstrtol(s, base, res);
-}
-
-int __must_check kstrtouint(const char *s, unsigned int base, unsigned int *res);
-int __must_check kstrtoint(const char *s, unsigned int base, int *res);
-
-static inline int __must_check kstrtou64(const char *s, unsigned int base, u64 *res)
-{
-	return kstrtoull(s, base, res);
-}
-
-static inline int __must_check kstrtos64(const char *s, unsigned int base, s64 *res)
-{
-	return kstrtoll(s, base, res);
-}
-
-static inline int __must_check kstrtou32(const char *s, unsigned int base, u32 *res)
-{
-	return kstrtouint(s, base, res);
-}
-
-static inline int __must_check kstrtos32(const char *s, unsigned int base, s32 *res)
-{
-	return kstrtoint(s, base, res);
-}
-
-int __must_check kstrtou16(const char *s, unsigned int base, u16 *res);
-int __must_check kstrtos16(const char *s, unsigned int base, s16 *res);
-int __must_check kstrtou8(const char *s, unsigned int base, u8 *res);
-int __must_check kstrtos8(const char *s, unsigned int base, s8 *res);
-
-int __must_check kstrtoull_from_user(const char __user *s, size_t count, unsigned int base, unsigned long long *res);
-int __must_check kstrtoll_from_user(const char __user *s, size_t count, unsigned int base, long long *res);
-int __must_check kstrtoul_from_user(const char __user *s, size_t count, unsigned int base, unsigned long *res);
-int __must_check kstrtol_from_user(const char __user *s, size_t count, unsigned int base, long *res);
-int __must_check kstrtouint_from_user(const char __user *s, size_t count, unsigned int base, unsigned int *res);
-int __must_check kstrtoint_from_user(const char __user *s, size_t count, unsigned int base, int *res);
-int __must_check kstrtou16_from_user(const char __user *s, size_t count, unsigned int base, u16 *res);
-int __must_check kstrtos16_from_user(const char __user *s, size_t count, unsigned int base, s16 *res);
-int __must_check kstrtou8_from_user(const char __user *s, size_t count, unsigned int base, u8 *res);
-int __must_check kstrtos8_from_user(const char __user *s, size_t count, unsigned int base, s8 *res);
-
-static inline int __must_check kstrtou64_from_user(const char __user *s, size_t count, unsigned int base, u64 *res)
-{
-	return kstrtoull_from_user(s, count, base, res);
-}
-
-static inline int __must_check kstrtos64_from_user(const char __user *s, size_t count, unsigned int base, s64 *res)
-{
-	return kstrtoll_from_user(s, count, base, res);
-}
-
-static inline int __must_check kstrtou32_from_user(const char __user *s, size_t count, unsigned int base, u32 *res)
-{
-	return kstrtouint_from_user(s, count, base, res);
-}
-
-static inline int __must_check kstrtos32_from_user(const char __user *s, size_t count, unsigned int base, s32 *res)
-{
-	return kstrtoint_from_user(s, count, base, res);
-}
-
 /*
  * Obsolete, do not use.
  * Use parse_integer(), kstrto*(), kstrto*_from_user(), sscanf().

include/linux/parse-integer.h +109 -0

113 lines not on this page · raw message

lib/kstrtox.c +0 ~~-222~~

228 lines not on this page · raw message

lib/parse-integer.c +34 -4

@@ -31,7 +31,7 @@ const char *_parse_integer_fixup_radix(const char *s, unsigned int *base)
 	return s;
 }
 
-static int __parse_integer(const char *s, unsigned int base, unsigned long long *val)
+static int ___parse_integer(const char *s, unsigned int base, unsigned long long *val)
 {
 	const char *s0 = s, *sd;
 	unsigned long long acc;

@@ -63,6 +63,27 @@ static int __parse_integer(const char *s, unsigned int base, unsigned long long
 	return s - s0;
 }
 
+static int __parse_integer(const char *s, unsigned int base, unsigned long long *val)
+{
+	if (base & PARSE_INTEGER_NEWLINE) {
+		unsigned long long _val;
+		int rv;
+
+		/* Accept "integer\0" or "integer\n\0" */
+		rv = ___parse_integer(s, base & ~PARSE_INTEGER_NEWLINE, &_val);
+		if (rv < 0)
+			return rv;
+		s += rv;
+		if (*s == '\n')
+			s++;
+		if (*s)
+			return -EINVAL;
+		*val = _val;
+		return 0;
+	} else
+		return ___parse_integer(s, base, val);
+}
+
 int _parse_integer_ull(const char *s, unsigned int base, unsigned long long *val)
 {
 	int rv;

@@ -73,7 +94,10 @@ int _parse_integer_ull(const char *s, unsigned int base, unsigned long long *val
 		rv = __parse_integer(s + 1, base, val);
 		if (rv < 0)
 			return rv;
-		return rv + 1;
+		if (base & PARSE_INTEGER_NEWLINE)
+			return 0;
+		else
+			return rv + 1;
 	} else
 		return __parse_integer(s, base, val);
 }

@@ -91,7 +115,10 @@ int _parse_integer_ll(const char *s, unsigned int base, long long *val)
 		if ((long long)-tmp >= 0)
 			return -ERANGE;
 		*val = -tmp;
-		return rv + 1;
+		if (base & PARSE_INTEGER_NEWLINE)
+			return 0;
+		else
+			return rv + 1;
 	} else if (*s == '+') {
 		rv = __parse_integer(s + 1, base, &tmp);
 		if (rv < 0)

@@ -99,7 +126,10 @@ int _parse_integer_ll(const char *s, unsigned int base, long long *val)
 		if ((long long)tmp < 0)
 			return -ERANGE;
 		*val = tmp;
-		return rv + 1;
+		if (base & PARSE_INTEGER_NEWLINE)
+			return 0;
+		else
+			return rv + 1;
 	} else {
 		rv = __parse_integer(s, base, &tmp);
 		if (rv < 0)

Alexey Dobriyan

author

2015-05-02 00:51 UTC #4 lore raw

PATCH 4/10 sscanf: fix overflow

In reply to Alexey Dobriyan

Fun fact:

	uint8_t val;
	sscanf("256", "%hhu", &val);

will return 1 (as it should), and make val=0 (as it should not).

Apart from correctness, patch allows to remove checks and switch to proper types in several (most?) cases:

	grep -e 'scanf.*%[0-9]\+[dioux]' -n -r .

Such checks can be incorrect too, checking for 3 digits with %3u for parsing uint8_t is not enough.

Signed-off-by Alexey Dobriyan <[email protected]>

---

 lib/vsprintf.c |   45 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 34 insertions(+), 11 deletions(-)

lib/vsprintf.c +34 ~~-11~~

@@ -2632,44 +2632,67 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
 
 		switch (qualifier) {
 		case 'H':	/* that's 'hh' in format */
-			if (is_sign)
+			if (is_sign) {
+				if (val.s != (signed char)val.s)
+					goto out;
 				*va_arg(args, signed char *) = val.s;
-			else
+			} else {
+				if (val.u != (unsigned char)val.u)
+					goto out;
 				*va_arg(args, unsigned char *) = val.u;
+			}
 			break;
 		case 'h':
-			if (is_sign)
+			if (is_sign) {
+				if (val.s != (short)val.s)
+					goto out;
 				*va_arg(args, short *) = val.s;
-			else
+			} else {
+				if (val.u != (unsigned short)val.u)
+					goto out;
 				*va_arg(args, unsigned short *) = val.u;
+			}
 			break;
 		case 'l':
-			if (is_sign)
+			if (is_sign) {
+				if (val.s != (long)val.s)
+					goto out;
 				*va_arg(args, long *) = val.s;
-			else
+			} else {
+				if (val.u != (unsigned long)val.u)
+					goto out;
 				*va_arg(args, unsigned long *) = val.u;
+			}
 			break;
 		case 'L':
-			if (is_sign)
+			if (is_sign) {
 				*va_arg(args, long long *) = val.s;
-			else
+			} else {
 				*va_arg(args, unsigned long long *) = val.u;
+			}
 			break;
 		case 'Z':
 		case 'z':
+			if (val.u != (size_t)val.u)
+				goto out;
 			*va_arg(args, size_t *) = val.u;
 			break;
 		default:
-			if (is_sign)
+			if (is_sign) {
+				if (val.s != (int)val.s)
+					goto out;
 				*va_arg(args, int *) = val.s;
-			else
+			} else {
+				if (val.u != (unsigned int)val.u)
+					goto out;
 				*va_arg(args, unsigned int *) = val.u;
+			}
 			break;
 		}
 		num++;
 		str += len;
 	}
-
+out:
 	return num;
 }
 EXPORT_SYMBOL(vsscanf);

Rasmus Villemoes

reviewer

2015-05-05 09:52 UTC #5 lore raw

Re: [PATCH 04/10] sscanf: fix overflow

Quoted 7 lines · Alexey Dobriyan

Fun fact:
	uint8_t val;
	sscanf("256", "%hhu", &val);
will return 1 (as it should), and make val=0 (as it should not).

What do you base these "should" and "should not" on? Both C99 and POSIX say that the behaviour is undefined - the kernel can obviously define its own semantics for scanf, but what do you think they should be?

If we want to correctly handle overflow, the only sane way is to make sscanf return 0 in the above case (no conversions done). This also seems to be what your patch does, but then I'm confused by your first "as it should".

Quoted 7 lines

Apart from correctness, patch allows to remove checks and switch to proper types in several (most?) cases:
	grep -e 'scanf.*%[0-9]\+[dioux]' -n -r .
Such checks can be incorrect too, checking for 3 digits with %3u for parsing uint8_t is not enough.

Yeah, and it may be too much; sscanf("0042", "%hhu", &val") should give 42, not 4. I agree that one should be able to rely on scanf doing range checking as part of matching.

Actually, I think one should go through all the callers of sscanf which use a field width with an integer conversion and see if we can get rid of it, and then rip it away from the sscanf implementation. Otherwise there's another bug which would need fixing, namely

int x;
char rest[50];
sscanf("12345678901234567890", "%3d%s", &x, rest)

should successfully return 2 (storing 123 in x), but it can't when the strategy is to convert as much as possible (which may then give an early failure due to overflow), then divide by 10 until we haven't consumed more than we're allowed.

Rasmus

Alexey Dobriyan

author

2015-05-05 11:10 UTC #6 lore raw

Re: [PATCH 04/10] sscanf: fix overflow

Quoted 7 lines · Rasmus Villemoes

Quoted 5 lines · Alexey Dobriyan
Fun fact:
      uint8_t val;
      sscanf("256", "%hhu", &val);

Quoted 6 lines

Quoted 2 lines
will return 1 (as it should), and make val=0 (as it should not).
What do you base these "should" and "should not" on? Both C99 and POSIX say that the behaviour is undefined - the kernel can obviously define its own semantics for scanf, but what do you think they should be?

POSIX can say whatever it wants, it's about common sense.

sscanf(), both kernel and libc, in this situation returns 0 when "0" character is nowhere to be found in the string! It should either return 25 or do not return anything because situation is ambiguous (read: set ERANGE).

Quoted 30 lines

If we want to correctly handle overflow, the only sane way is to make sscanf return 0 in the above case (no conversions done). This also seems to be what your patch does, but then I'm confused by your first "as it should".
Quoted 7 lines
Apart from correctness, patch allows to remove checks and switch to proper types in several (most?) cases:
      grep -e 'scanf.*%[0-9]\+[dioux]' -n -r .
Such checks can be incorrect too, checking for 3 digits with %3u for parsing uint8_t is not enough.
Yeah, and it may be too much; sscanf("0042", "%hhu", &val") should give 42, not 4. I agree that one should be able to rely on scanf doing range checking as part of matching.
Actually, I think one should go through all the callers of sscanf which use a field width with an integer conversion and see if we can get rid of it, and then rip it away from the sscanf implementation. Otherwise there's another bug which would need fixing, namely
int x;
char rest[50];
sscanf("12345678901234567890", "%3d%s", &x, rest)
should successfully return 2 (storing 123 in x), but it can't when the strategy is to convert as much as possible (which may then give an early failure due to overflow), then divide by 10 until we haven't consumed more than we're allowed.

Rasmus Villemoes

reviewer

2015-05-06 07:49 UTC #7 lore raw

Re: [PATCH 04/10] sscanf: fix overflow

Quoted 9 lines · Alexey Dobriyan

Quoted 7 lines · Rasmus Villemoes
Quoted 5 lines · Alexey Dobriyan
Fun fact:
      uint8_t val;
      sscanf("256", "%hhu", &val);

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
will return 1 (as it should), and make val=0 (as it should not).
What do you base these "should" and "should not" on? Both C99 and POSIX say that the behaviour is undefined - the kernel can obviously define its own semantics for scanf, but what do you think they should be?
POSIX can say whatever it wants,

That was sort of the point, POSIX doesn't say anything, which is why I asked what you think the semantics should be.

Quoted 5 lines

it's about common sense.
sscanf(), both kernel and libc, in this situation returns 0 when "0" character is nowhere to be found in the string! It should either return 25

Really? Do you consider it common sense that sscanf("256 123", "%hhu%d", &x, &y) can end up returning 2, putting 25 in x and 6 in y?

Quoted 1 line

or do not return anything

I agree that _that_ would be the sane thing to do, but again, I'm confused why you then said the first example should return 1.

Rasmus

Alexey Dobriyan

author

2015-05-02 00:53 UTC #8 lore raw

PATCH 5/10 parse_integer: convert lib/

In reply to Alexey Dobriyan

Convert away lib/ from deprecated simple_strto*() interfaces.

"&(int []){0}[0]" expression is anonymous variable, don't be scared. Filesystem option parser code does parsing 1.5 times: first to know boundaries of a value (args[0].from, args[0].to) and then actual parsing with match_number(). Noody knows why it does that.

match_number() needlessly allocates/duplicates memory, parsing can be done straight from original string.

Signed-off-by Alexey Dobriyan <[email protected]>

---

 lib/cmdline.c |   36 ++++++++++++++++++------------------
 lib/parser.c  |   29 ++++++++++++-----------------
 lib/swiotlb.c |    2 +-
 3 files changed, 31 insertions(+), 36 deletions(-)

lib/cmdline.c +18 ~~-18~~

@@ -27,7 +27,7 @@ static int get_range(char **str, int *pint)
 	int x, inc_counter, upper_range;
 
 	(*str)++;
-	upper_range = simple_strtol((*str), NULL, 0);
+	parse_integer(*str, 0, &upper_range);
 	inc_counter = upper_range - *pint;
 	for (x = *pint; x < upper_range; x++)
 		*pint++ = x;

@@ -51,13 +51,14 @@ static int get_range(char **str, int *pint)
 
 int get_option(char **str, int *pint)
 {
-	char *cur = *str;
+	int len;
 
-	if (!cur || !(*cur))
+	if (!str || !*str)
 		return 0;
-	*pint = simple_strtol(cur, str, 0);
-	if (cur == *str)
+	len = parse_integer(*str, 0, pint);
+	if (len < 0)
 		return 0;
+	*str += len;
 	if (**str == ',') {
 		(*str)++;
 		return 2;

@@ -126,38 +127,37 @@ EXPORT_SYMBOL(get_options);
 
 unsigned long long memparse(const char *ptr, char **retptr)
 {
-	char *endptr;	/* local pointer to end of parsed string */
+	unsigned long long val;
 
-	unsigned long long ret = simple_strtoull(ptr, &endptr, 0);
-
-	switch (*endptr) {
+	ptr += parse_integer(ptr, 0, &val);
+	switch (*ptr) {
 	case 'E':
 	case 'e':
-		ret <<= 10;
+		val <<= 10;
 	case 'P':
 	case 'p':
-		ret <<= 10;
+		val <<= 10;
 	case 'T':
 	case 't':
-		ret <<= 10;
+		val <<= 10;
 	case 'G':
 	case 'g':
-		ret <<= 10;
+		val <<= 10;
 	case 'M':
 	case 'm':
-		ret <<= 10;
+		val <<= 10;
 	case 'K':
 	case 'k':
-		ret <<= 10;
-		endptr++;
+		val <<= 10;
+		ptr++;
 	default:
 		break;
 	}
 
 	if (retptr)
-		*retptr = endptr;
+		*retptr = (char *)ptr;
 
-	return ret;
+	return val;
 }
 EXPORT_SYMBOL(memparse);

lib/parser.c +12 ~~-17~~

@@ -44,7 +44,7 @@ static int match_one(char *s, const char *p, substring_t args[])
 		p = meta + 1;
 
 		if (isdigit(*p))
-			len = simple_strtoul(p, (char **) &p, 10);
+			p += parse_integer(p, 10, (unsigned int *)&len);
 		else if (*p == '%') {
 			if (*s++ != '%')
 				return 0;

@@ -68,19 +68,21 @@ static int match_one(char *s, const char *p, substring_t args[])
 			break;
 		}
 		case 'd':
-			simple_strtol(s, &args[argc].to, 0);
+			/* anonymous variable */
+			len = parse_integer(s, 0, &(int []){0}[0]);
 			goto num;
 		case 'u':
-			simple_strtoul(s, &args[argc].to, 0);
+			len = parse_integer(s, 0, &(unsigned int []){0}[0]);
 			goto num;
 		case 'o':
-			simple_strtoul(s, &args[argc].to, 8);
+			len = parse_integer(s, 8, &(unsigned int []){0}[0]);
 			goto num;
 		case 'x':
-			simple_strtoul(s, &args[argc].to, 16);
+			len = parse_integer(s, 16, &(unsigned int []){0}[0]);
 		num:
-			if (args[argc].to == args[argc].from)
+			if (len < 0)
 				return 0;
+			args[argc].to = args[argc].from + len;
 			break;
 		default:
 			return 0;

@@ -127,10 +129,8 @@ EXPORT_SYMBOL(match_token);
  */
 static int match_number(substring_t *s, int *result, int base)
 {
-	char *endp;
 	char *buf;
 	int ret;
-	long val;
 	size_t len = s->to - s->from;
 
 	buf = kmalloc(len + 1, GFP_KERNEL);

@@ -139,16 +139,11 @@ static int match_number(substring_t *s, int *result, int base)
 	memcpy(buf, s->from, len);
 	buf[len] = '\0';
 
-	ret = 0;
-	val = simple_strtol(buf, &endp, base);
-	if (endp == buf)
-		ret = -EINVAL;
-	else if (val < (long)INT_MIN || val > (long)INT_MAX)
-		ret = -ERANGE;
-	else
-		*result = (int) val;
+	ret = parse_integer(buf, base, result);
 	kfree(buf);
-	return ret;
+	if (ret < 0)
+		return ret;
+	return 0;
 }
 
 /**

lib/swiotlb.c +1 -1

@@ -100,7 +100,7 @@ static int __init
 setup_io_tlb_npages(char *str)
 {
 	if (isdigit(*str)) {
-		io_tlb_nslabs = simple_strtoul(str, &str, 0);
+		str += parse_integer(str, 0, &io_tlb_nslabs);
 		/* avoid tail segment of size < IO_TLB_SEGSIZE */
 		io_tlb_nslabs = ALIGN(io_tlb_nslabs, IO_TLB_SEGSIZE);
 	}

Rasmus Villemoes

reviewer

2015-05-04 14:10 UTC #9 lore raw

Re: [PATCH 05/10] parse_integer: convert lib/

Quoted 2 lines · Alexey Dobriyan

match_number() needlessly allocates/duplicates memory, parsing can be done straight from original string.

I suppose that's true, but the patch doesn't seem to do anything about it? It's probably better to do in a separate cleanup anyway, but then maybe this belongs as a comment below ---.

Quoted 39 lines

Signed-off-by Alexey Dobriyan <[email protected]>

---

 lib/cmdline.c |   36 ++++++++++++++++++------------------
 lib/parser.c  |   29 ++++++++++++-----------------
 lib/swiotlb.c |    2 +-
 3 files changed, 31 insertions(+), 36 deletions(-)

lib/cmdline.c +6 -5

@@ -27,7 +27,7 @@ static int get_range(char **str, int *pint)
 	int x, inc_counter, upper_range;
 
 	(*str)++;
-	upper_range = simple_strtol((*str), NULL, 0);
+	parse_integer(*str, 0, &upper_range);
 	inc_counter = upper_range - *pint;
 	for (x = *pint; x < upper_range; x++)
 		*pint++ = x;

@@ -51,13 +51,14 @@ static int get_range(char **str, int *pint)
 
 int get_option(char **str, int *pint)
 {
-	char *cur = *str;
+	int len;
 
-	if (!cur || !(*cur))
+	if (!str || !*str)
 		return 0;
-	*pint = simple_strtol(cur, str, 0);
-	if (cur == *str)
+	len = parse_integer(*str, 0, pint);
+	if (len < 0)
 		return 0;
+	*str += len;
 	if (**str == ',') {
 		(*str)++;
 		return 2;

lib/cmdline.c EXPORT_SYMBOL(get_options); · line 132

Quoted 10 lines

lib/cmdline.c +2 -4

@@ -126,38 +127,37 @@ EXPORT_SYMBOL(get_options);
 
 unsigned long long memparse(const char *ptr, char **retptr)
 {
-	char *endptr;	/* local pointer to end of parsed string */
+	unsigned long long val;
 
-	unsigned long long ret = simple_strtoull(ptr, &endptr, 0);
-
-	switch (*endptr) {
+	ptr += parse_integer(ptr, 0, &val);

This seems wrong. simple_strtoull used to "sanitize" the return value from the (old) _parse_integer, so that endptr still points into the given string. Unconditionally adding the result from parse_integer may make ptr point far before the actual string, into who-knows-what.

Quoted 24 lines

lib/cmdline.c +6 -6

+	switch (*ptr) {
 	case 'E':
 	case 'e':
-		ret <<= 10;
+		val <<= 10;
 	case 'P':
 	case 'p':
-		ret <<= 10;
+		val <<= 10;
 	case 'T':
 	case 't':
-		ret <<= 10;
+		val <<= 10;
 	case 'G':
 	case 'g':
-		ret <<= 10;
+		val <<= 10;
 	case 'M':
 	case 'm':
-		ret <<= 10;
+		val <<= 10;
 	case 'K':
 	case 'k':
-		ret <<= 10;

lib/cmdline.c EXPORT_SYMBOL(get_options); · line 158

Quoted 10 lines

lib/cmdline.c +3 -2

-		endptr++;
+		val <<= 10;
+		ptr++;
 	default:
 		break;
 	}
 
 	if (retptr)
-		*retptr = endptr;
+		*retptr = (char *)ptr;

And here we propagate that to the caller.

lib/parser.c static int match_one(char *s, const char *p, substring_t args[]) · line 47

Quoted 13 lines

lib/cmdline.c +1 -1

-	return ret;
+	return val;
 }
 EXPORT_SYMBOL(memparse);

lib/parser.c +1 -1

@@ -44,7 +44,7 @@ static int match_one(char *s, const char *p, substring_t args[])
 		p = meta + 1;
 
 		if (isdigit(*p))
-			len = simple_strtoul(p, (char **) &p, 10);
+			p += parse_integer(p, 10, (unsigned int *)&len);

Hm, I think passing cast expressions to parse_integer should be actively discouraged. While it might work in this case, some day somebody will copy-paste this to a place where the len variable doesn't have sizeof==4.

Quoted 12 lines

lib/parser.c +2 -1

 		else if (*p == '%') {
 			if (*s++ != '%')
 				return 0;

@@ -68,19 +68,21 @@ static int match_one(char *s, const char *p, substring_t args[])
 			break;
 		}
 		case 'd':
-			simple_strtol(s, &args[argc].to, 0);
+			/* anonymous variable */
+			len = parse_integer(s, 0, &(int []){0}[0]);
 			goto num;
 		case 'u':

lib/parser.c static int match_one(char *s, const char *p, substring_t args[]) · line 81

Quoted 10 lines

lib/parser.c +3 -3

-			simple_strtoul(s, &args[argc].to, 0);
+			len = parse_integer(s, 0, &(unsigned int []){0}[0]);
 			goto num;
 		case 'o':
-			simple_strtoul(s, &args[argc].to, 8);
+			len = parse_integer(s, 8, &(unsigned int []){0}[0]);
 			goto num;
 		case 'x':
-			simple_strtoul(s, &args[argc].to, 16);
+			len = parse_integer(s, 16, &(unsigned int []){0}[0]);

I see that the commit log says "don't be scared", and the first of these even has a comment. But is there any reason to be that clever here? I see a couple of downsides:

* gcc has to initialize some stack memory to 0, since it cannot know it is only an output parameter.

* things like this usually consume an excessive amount of stack. I haven't been able to try your patches, but a simplified version of the above shows that gcc doesn't use the same stack slots for the various anonymous variables.

* It's unreadable, despite the comment and the commit log.

I suggest using the more obvious approach of declaring a union on the stack and pass the address of the appropriate member.

Rasmus

Alexey Dobriyan

author

2015-05-04 14:57 UTC #10 lore raw

Re: [PATCH 05/10] parse_integer: convert lib/

Quoted 8 lines · Rasmus Villemoes

Quoted 2 lines · Alexey Dobriyan
match_number() needlessly allocates/duplicates memory, parsing can be done straight from original string.
I suppose that's true, but the patch doesn't seem to do anything about it? It's probably better to do in a separate cleanup anyway, but then maybe this belongs as a comment below ---.

Sure, it is just "raise eyebrow" moment.

Quoted 7 lines

lib/cmdline.c +1 -1

@@ -126,38 +127,37 @@ EXPORT_SYMBOL(get_options);
 
 unsigned long long memparse(const char *ptr, char **retptr)
 {
-     char *endptr;   /* local pointer to end of parsed string */
+     unsigned long long val;

Quoted 9 lines

lib/cmdline.c EXPORT_SYMBOL(get_options); · line 132
Quoted 4 lines
lib/cmdline.c +1 -3
-     unsigned long long ret = simple_strtoull(ptr, &endptr, 0);
-
-     switch (*endptr) {
+     ptr += parse_integer(ptr, 0, &val);
This seems wrong. simple_strtoull used to "sanitize" the return value from the (old) _parse_integer, so that endptr still points into the given string. Unconditionally adding the result from parse_integer may make ptr point far before the actual string, into who-knows-what.

When converting I tried to preserve the amount of error checking done. simple_strtoull() either
a) return 0 and not advance pointer, or
b) return something and advance pointer.

Current code just ignores error case, so do I.

Quoted 5 lines

lib/parser.c +0 -0

@@ -44,7 +44,7 @@ static int match_one(char *s, const char *p, substring_t args[])
              p = meta + 1;

Quoted 8 lines

lib/parser.c static int match_one(char *s, const char *p, substring_t args[]) · line 47
Quoted 3 lines
lib/parser.c +1 -1
              if (isdigit(*p))
-                     len = simple_strtoul(p, (char **) &p, 10);
+                     p += parse_integer(p, 10, (unsigned int *)&len);
Hm, I think passing cast expressions to parse_integer should be actively discouraged. While it might work in this case, some day somebody will copy-paste this to a place where the len variable doesn't have sizeof==4.

This cast to turn on unsigned (or signed) parsing is the only nontrivial place.

All I can say is that C programmer should be diligent about types. Equivalent strtol() code has silent truncation. Equivalent code with any other real function which accepts pointer has the same problem -- direct cast.

We have to live with it, I guess.

Quoted 24 lines

Quoted 9 lines

lib/parser.c +2 -1

@@ -68,19 +68,21 @@ static int match_one(char *s, const char *p, substring_t args[])
                      break;
              }
              case 'd':
-                     simple_strtol(s, &args[argc].to, 0);
+                     /* anonymous variable */
+                     len = parse_integer(s, 0, &(int []){0}[0]);
                      goto num;
              case 'u':

lib/parser.c static int match_one(char *s, const char *p, substring_t args[]) · line 81

Quoted 10 lines

lib/parser.c +3 -3

-                     simple_strtoul(s, &args[argc].to, 0);
+                     len = parse_integer(s, 0, &(unsigned int []){0}[0]);
                      goto num;
              case 'o':
-                     simple_strtoul(s, &args[argc].to, 8);
+                     len = parse_integer(s, 8, &(unsigned int []){0}[0]);
                      goto num;
              case 'x':
-                     simple_strtoul(s, &args[argc].to, 16);
+                     len = parse_integer(s, 16, &(unsigned int []){0}[0]);

I see that the commit log says "don't be scared", and the first of these even has a comment. But is there any reason to be that clever here? I see a couple of downsides:

Quoted 2 lines

* gcc has to initialize some stack memory to 0, since it cannot know it is only an output parameter.

Yes.

Quoted 4 lines

* things like this usually consume an excessive amount of stack. I haven't been able to try your patches, but a simplified version of the above shows that gcc doesn't use the same stack slots for the various anonymous variables.

Yes.

Quoted 4 lines

* It's unreadable, despite the comment and the commit log.
I suggest using the more obvious approach of declaring a union on the stack and pass the address of the appropriate member.

Union will work.

No that it matters, it's a filesystem option parsing code which is executed rarely near the top of sys_mount(), not exactly perfomance bottleneck.

It's a shame to lose such clever hack :-(

Alexey Dobriyan

author

2015-05-02 00:55 UTC #11 lore raw

PATCH 6/10 parse_integer: convert mm/

In reply to Alexey Dobriyan

Convert mm/ directory away from deprecated simple_strto*() interface.

One thing to note about parse_integer() and seemingly useless casts -- range of accepted values depends on result type.

	int val;
	parse_integer(s, 0, &val);

will accept negative integers, while

	int val;
	parse_integer(s, 0, (unsigned int *)&val);

will accept only 0 and positive integers.

Cast is needed when result variable has to be of different type for some reason.

This is very important and hopefully [knocks wood] obvious.

Signed-off-by Alexey Dobriyan <[email protected]>

---

 mm/memcontrol.c |   19 +++++++++++--------
 mm/memtest.c    |    2 +-
 mm/page_alloc.c |    2 +-
 mm/shmem.c      |   14 ++++++++------
 4 files changed, 21 insertions(+), 16 deletions(-)

mm/memcontrol.c +11 -8

@@ -4096,20 +4096,23 @@ static ssize_t memcg_write_event_control(struct kernfs_open_file *of,
 	struct fd efile;
 	struct fd cfile;
 	const char *name;
-	char *endp;
 	int ret;
 
 	buf = strstrip(buf);
 
-	efd = simple_strtoul(buf, &endp, 10);
-	if (*endp != ' ')
+	ret = parse_integer(buf, 10, &efd);
+	if (ret < 0)
+		return ret;
+	buf += ret;
+	if (*buf++ != ' ')
 		return -EINVAL;
-	buf = endp + 1;
-
-	cfd = simple_strtoul(buf, &endp, 10);
-	if ((*endp != ' ') && (*endp != '\0'))
+	ret = parse_integer(buf, 10, &efd);
+	if (ret < 0)
+		return ret;
+	buf += ret;
+	if (*buf != ' ' && *buf != '\0')
 		return -EINVAL;
-	buf = endp + 1;
+	buf++;
 
 	event = kzalloc(sizeof(*event), GFP_KERNEL);
 	if (!event)

mm/memtest.c +1 -1

@@ -93,7 +93,7 @@ static int memtest_pattern __initdata;
 static int __init parse_memtest(char *arg)
 {
 	if (arg)
-		memtest_pattern = simple_strtoul(arg, NULL, 0);
+		parse_integer(arg, 0, (unsigned int *)&memtest_pattern);
 	else
 		memtest_pattern = ARRAY_SIZE(patterns);

mm/page_alloc.c +1 -1

@@ -6020,7 +6020,7 @@ static int __init set_hashdist(char *str)
 {
 	if (!str)
 		return 0;
-	hashdist = simple_strtoul(str, &str, 0);
+	parse_integer(str, 0, (unsigned int *)&hashdist);
 	return 1;
 }
 __setup("hashdist=", set_hashdist);

mm/shmem.c +8 -6

@@ -2742,6 +2742,7 @@ static int shmem_parse_options(char *options, struct shmem_sb_info *sbinfo,
 	struct mempolicy *mpol = NULL;
 	uid_t uid;
 	gid_t gid;
+	int rv;
 
 	while (options != NULL) {
 		this_char = options;

@@ -2795,14 +2796,15 @@ static int shmem_parse_options(char *options, struct shmem_sb_info *sbinfo,
 		} else if (!strcmp(this_char,"mode")) {
 			if (remount)
 				continue;
-			sbinfo->mode = simple_strtoul(value, &rest, 8) & 07777;
-			if (*rest)
+			rv = parse_integer(value, 8, &sbinfo->mode);
+			if (rv < 0 || value[rv])
 				goto bad_val;
+			sbinfo->mode &= 07777;
 		} else if (!strcmp(this_char,"uid")) {
 			if (remount)
 				continue;
-			uid = simple_strtoul(value, &rest, 0);
-			if (*rest)
+			rv = parse_integer(value, 0, &uid);
+			if (rv < 0 || value[rv])
 				goto bad_val;
 			sbinfo->uid = make_kuid(current_user_ns(), uid);
 			if (!uid_valid(sbinfo->uid))

@@ -2810,8 +2812,8 @@ static int shmem_parse_options(char *options, struct shmem_sb_info *sbinfo,
 		} else if (!strcmp(this_char,"gid")) {
 			if (remount)
 				continue;
-			gid = simple_strtoul(value, &rest, 0);
-			if (*rest)
+			rv = parse_integer(value, 0, &gid);
+			if (rv < 0 || value[rv])
 				goto bad_val;
 			sbinfo->gid = make_kgid(current_user_ns(), gid);
 			if (!gid_valid(sbinfo->gid))

Rasmus Villemoes

reviewer

2015-05-04 14:33 UTC #12 lore raw

Re: [PATCH 06/10] parse_integer: convert mm/

Quoted 8 lines · Alexey Dobriyan

Convert mm/ directory away from deprecated simple_strto*() interface.
One thing to note about parse_integer() and seemingly useless casts -- range of accepted values depends on result type.
	int val;
	parse_integer(s, 0, &val);

Quoted 6 lines

will accept negative integers, while
	int val;
	parse_integer(s, 0, (unsigned int *)&val);
will accept only 0 and positive integers.

... and then silently write a negative value to val if the parsed integer happens to be larger than INT_MAX.

Again, I think passing cast expressions to parse_integer should be verboten.

In these particular cases:

* memtest_pattern should just be unsigned int - it's only ever used as such anyway, and it represents a count.

* hashdist should be a boolean, but even in its current form, there's no reason to not just use parse_integer as-is. If people like to set it by passing -42 just let them.

Rasmus

Alexey Dobriyan

author

2015-05-04 15:09 UTC #13 lore raw

Re: [PATCH 06/10] parse_integer: convert mm/

Quoted 23 lines · Rasmus Villemoes

Quoted 8 lines · Alexey Dobriyan
Convert mm/ directory away from deprecated simple_strto*() interface.
One thing to note about parse_integer() and seemingly useless casts -- range of accepted values depends on result type.
      int val;
      parse_integer(s, 0, &val);
Quoted 6 lines
will accept negative integers, while
      int val;
      parse_integer(s, 0, (unsigned int *)&val);
will accept only 0 and positive integers.
... and then silently write a negative value to val if the parsed integer happens to be larger than INT_MAX.
Again, I think passing cast expressions to parse_integer should be verboten.

Quoted 8 lines

In these particular cases:
* memtest_pattern should just be unsigned int - it's only ever used as such anyway, and it represents a count.
* hashdist should be a boolean, but even in its current form, there's no reason to not just use parse_integer as-is. If people like to set it by passing -42 just let them.

In this particular code, maybe. The real use case is this:

int val = -1; //default
parse_integer(s, 0, (unsigned int *)&val); //accept unsigned only

Either you add cast here, or cast where you check that value is really unsigned after parsing:

  int val = -1;
  parse_integer(s, 0, (unsiged int *)&val);
  if (val < 0)
    return -EINVAL;

Overall number of casts is preserved.

Just grep for simple_strto* and see how much error checking people do. The aim of the patchset is to convert code, error checking is separate story. I fixed types and added error checks in obvious cases but really:

static int memtest_pattern __initdata;
static int __init parse_memtest(char *arg)

{
        if (arg)
                memtest_pattern = simple_strtoul(arg, NULL, 0);

"int" and strtoul() => developer doesn't care, neither do I.

Alexey Dobriyan

author

2015-05-02 00:57 UTC #14 lore raw

PATCH 7/10 parse_integer: convert misc fs/ code

In reply to Alexey Dobriyan

Convert random fs/ code away from simple_strto*() interfaces.

Note about "struct simple_attr" conversion:
->set_buf is unneeded because everything can be done from stack. ->get_buf is useless as well, but that's a separate patch. Mutex is not removed, as it may guard readers from writers, separate story as well.
(code has been copied to arch/powerpc/.../spufs/, don't forget!)

binfmt_misc: file offset can't really be negative, type changed.

Signed-off-by Alexey Dobriyan <[email protected]>

---

 fs/binfmt_misc.c |   12 +++++++++---
 fs/dcache.c      |    2 +-
 fs/inode.c       |    2 +-
 fs/libfs.c       |   26 ++++++++++----------------
 fs/namespace.c   |    4 ++--
 5 files changed, 23 insertions(+), 23 deletions(-)

fs/binfmt_misc.c +9 -3

@@ -47,7 +47,7 @@ enum {Enabled, Magic};
 typedef struct {
 	struct list_head list;
 	unsigned long flags;		/* type, status, etc. */
-	int offset;			/* offset of magic */
+	unsigned int offset;		/* offset of magic */
 	int size;			/* size of magic/mask */
 	char *magic;			/* magic or filename extension */
 	char *mask;			/* mask, NULL for exact match */

@@ -370,7 +370,13 @@ static Node *create_entry(const char __user *buffer, size_t count)
 		if (!s)
 			goto einval;
 		*s++ = '\0';
-		e->offset = simple_strtoul(p, &p, 10);
+		err = parse_integer(p, 10, &e->offset);
+		if (err < 0) {
+			kfree(e);
+			goto out;
+
+		}
+		p += err;
 		if (*p++)
 			goto einval;
 		pr_debug("register: offset: %#x\n", e->offset);

@@ -548,7 +554,7 @@ static void entry_status(Node *e, char *page)
 	if (!test_bit(Magic, &e->flags)) {
 		sprintf(dp, "extension .%s\n", e->magic);
 	} else {
-		dp += sprintf(dp, "offset %i\nmagic ", e->offset);
+		dp += sprintf(dp, "offset %u\nmagic ", e->offset);
 		dp = bin2hex(dp, e->magic, e->size);
 		if (e->mask) {
 			dp += sprintf(dp, "\nmask ");

fs/dcache.c +1 -1

@@ -3374,7 +3374,7 @@ static int __init set_dhash_entries(char *str)
 {
 	if (!str)
 		return 0;
-	dhash_entries = simple_strtoul(str, &str, 0);
+	parse_integer(str, 0, &dhash_entries);
 	return 1;
 }
 __setup("dhash_entries=", set_dhash_entries);

fs/inode.c +1 -1

@@ -1787,7 +1787,7 @@ static int __init set_ihash_entries(char *str)
 {
 	if (!str)
 		return 0;
-	ihash_entries = simple_strtoul(str, &str, 0);
+	parse_integer(str, 0, &ihash_entries);
 	return 1;
 }
 __setup("ihash_entries=", set_ihash_entries);

fs/libfs.c +10 ~~-16~~

@@ -752,7 +752,6 @@ struct simple_attr {
 	int (*get)(void *, u64 *);
 	int (*set)(void *, u64);
 	char get_buf[24];	/* enough to store a u64 and "\n\0" */
-	char set_buf[24];
 	void *data;
 	const char *fmt;	/* format for read operation */
 	struct mutex mutex;	/* protects access to these buffers */

@@ -830,31 +829,26 @@ ssize_t simple_attr_write(struct file *file, const char __user *buf,
 			  size_t len, loff_t *ppos)
 {
 	struct simple_attr *attr;
-	u64 val;
-	size_t size;
-	ssize_t ret;
+	s64 val;
+	int ret;
 
 	attr = file->private_data;
 	if (!attr->set)
 		return -EACCES;
 
+	ret = kstrtos64_from_user(buf, len, 0, &val);
+	if (ret < 0)
+		return ret;
+
 	ret = mutex_lock_interruptible(&attr->mutex);
 	if (ret)
 		return ret;
-
-	ret = -EFAULT;
-	size = min(sizeof(attr->set_buf) - 1, len);
-	if (copy_from_user(attr->set_buf, buf, size))
-		goto out;
-
-	attr->set_buf[size] = '\0';
-	val = simple_strtoll(attr->set_buf, NULL, 0);
 	ret = attr->set(attr->data, val);
-	if (ret == 0)
-		ret = len; /* on success, claim we got the whole input */
-out:
 	mutex_unlock(&attr->mutex);
-	return ret;
+	if (ret < 0)
+		return ret;
+	/* on success, claim we got the whole input */
+	return len;
 }
 EXPORT_SYMBOL_GPL(simple_attr_write);

fs/namespace.c +2 -2

@@ -37,7 +37,7 @@ static int __init set_mhash_entries(char *str)
 {
 	if (!str)
 		return 0;
-	mhash_entries = simple_strtoul(str, &str, 0);
+	parse_integer(str, 0, &mhash_entries);
 	return 1;
 }
 __setup("mhash_entries=", set_mhash_entries);

@@ -47,7 +47,7 @@ static int __init set_mphash_entries(char *str)
 {
 	if (!str)
 		return 0;
-	mphash_entries = simple_strtoul(str, &str, 0);
+	parse_integer(str, 0, &mphash_entries);
 	return 1;
 }
 __setup("mphash_entries=", set_mphash_entries);

Alexey Dobriyan

author

2015-05-02 00:59 UTC #15 lore raw

PATCH 8/10 fs/cachefiles/: convert to parse_integer()

In reply to Alexey Dobriyan

Convert away from deprecated simple_strto*() interfaces.

Switch "unsigned long" to "unsigned int" where possible. kstrto*() functions can't be used because of trailing "%" sign. :^)

Signed-off-by Alexey Dobriyan <[email protected]>

---

 fs/cachefiles/daemon.c |   84 ++++++++++++++++++++++++++-----------------------
 1 file changed, 45 insertions(+), 39 deletions(-)

fs/cachefiles/daemon.c +45 ~~-39~~

@@ -326,14 +326,15 @@ static int cachefiles_daemon_range_error(struct cachefiles_cache *cache,
  */
 static int cachefiles_daemon_frun(struct cachefiles_cache *cache, char *args)
 {
-	unsigned long frun;
+	unsigned int frun;
+	int rv;
 
 	_enter(",%s", args);
 
-	if (!*args)
-		return -EINVAL;
-
-	frun = simple_strtoul(args, &args, 10);
+	rv = parse_integer(args, 10, &frun);
+	if (rv < 0)
+		return rv;
+	args += rv;
 	if (args[0] != '%' || args[1] != '\0')
 		return -EINVAL;

@@ -350,14 +351,15 @@ static int cachefiles_daemon_frun(struct cachefiles_cache *cache, char *args)
  */
 static int cachefiles_daemon_fcull(struct cachefiles_cache *cache, char *args)
 {
-	unsigned long fcull;
+	unsigned int fcull;
+	int rv;
 
 	_enter(",%s", args);
 
-	if (!*args)
-		return -EINVAL;
-
-	fcull = simple_strtoul(args, &args, 10);
+	rv = parse_integer(args, 10, &fcull);
+	if (rv < 0)
+		return rv;
+	args += rv;
 	if (args[0] != '%' || args[1] != '\0')
 		return -EINVAL;

@@ -374,14 +376,15 @@ static int cachefiles_daemon_fcull(struct cachefiles_cache *cache, char *args)
  */
 static int cachefiles_daemon_fstop(struct cachefiles_cache *cache, char *args)
 {
-	unsigned long fstop;
+	unsigned int fstop;
+	int rv;
 
 	_enter(",%s", args);
 
-	if (!*args)
-		return -EINVAL;
-
-	fstop = simple_strtoul(args, &args, 10);
+	rv = parse_integer(args, 10, &fstop);
+	if (rv < 0)
+		return rv;
+	args += rv;
 	if (args[0] != '%' || args[1] != '\0')
 		return -EINVAL;

@@ -398,14 +401,15 @@ static int cachefiles_daemon_fstop(struct cachefiles_cache *cache, char *args)
  */
 static int cachefiles_daemon_brun(struct cachefiles_cache *cache, char *args)
 {
-	unsigned long brun;
+	unsigned int brun;
+	int rv;
 
 	_enter(",%s", args);
 
-	if (!*args)
-		return -EINVAL;
-
-	brun = simple_strtoul(args, &args, 10);
+	rv = parse_integer(args, 10, &brun);
+	if (rv < 0)
+		return rv;
+	args += rv;
 	if (args[0] != '%' || args[1] != '\0')
 		return -EINVAL;

@@ -422,14 +426,15 @@ static int cachefiles_daemon_brun(struct cachefiles_cache *cache, char *args)
  */
 static int cachefiles_daemon_bcull(struct cachefiles_cache *cache, char *args)
 {
-	unsigned long bcull;
+	unsigned int bcull;
+	int rv;
 
 	_enter(",%s", args);
 
-	if (!*args)
-		return -EINVAL;
-
-	bcull = simple_strtoul(args, &args, 10);
+	rv = parse_integer(args, 10, &bcull);
+	if (rv < 0)
+		return rv;
+	args += rv;
 	if (args[0] != '%' || args[1] != '\0')
 		return -EINVAL;

@@ -446,14 +451,15 @@ static int cachefiles_daemon_bcull(struct cachefiles_cache *cache, char *args)
  */
 static int cachefiles_daemon_bstop(struct cachefiles_cache *cache, char *args)
 {
-	unsigned long bstop;
+	unsigned int bstop;
+	int rv;
 
 	_enter(",%s", args);
 
-	if (!*args)
-		return -EINVAL;
-
-	bstop = simple_strtoul(args, &args, 10);
+	rv = parse_integer(args, 10, &bstop);
+	if (rv < 0)
+		return rv;
+	args += rv;
 	if (args[0] != '%' || args[1] != '\0')
 		return -EINVAL;

@@ -601,21 +607,21 @@ inval:
  */
 static int cachefiles_daemon_debug(struct cachefiles_cache *cache, char *args)
 {
-	unsigned long mask;
+	unsigned int mask;
+	int rv;
 
 	_enter(",%s", args);
 
-	mask = simple_strtoul(args, &args, 0);
-	if (args[0] != '\0')
-		goto inval;
-
+	rv = parse_integer(args, 0, &mask);
+	if (rv < 0)
+		return rv;
+	if (args[rv] != '\0') {
+		pr_err("debug command requires mask\n");
+		return -EINVAL;
+	}
 	cachefiles_debug = mask;
 	_leave(" = 0");
 	return 0;
-
-inval:
-	pr_err("debug command requires mask\n");
-	return -EINVAL;
 }
 
 /*

Alexey Dobriyan

author

2015-05-02 01:02 UTC #16 lore raw

PATCH 9/10 ocfs2: convert to parse_integer()/kstrto*()

In reply to Alexey Dobriyan

Convert away from deprecated simple_strto*() interfaces.

Autodetection of value range from type allows to remove disgusting checks:

	if ((major == LONG_MIN) || (major == LONG_MAX) ||
	    (major > (u8)-1) || (major < 1))
	        return -ERANGE;
	if ((minor == LONG_MIN) || (minor == LONG_MAX) ||
	    (minor > (u8)-1) || (minor < 0))

Poof, they're gone!

Signed-off-by Alexey Dobriyan <[email protected]>

---

 fs/ocfs2/cluster/heartbeat.c   |   54 +++++++++++++++++------------------------
 fs/ocfs2/cluster/nodemanager.c |   50 ++++++++++++++++---------------------
 fs/ocfs2/stack_user.c          |   50 ++++++++++++++++++-------------------
 3 files changed, 70 insertions(+), 84 deletions(-)

fs/ocfs2/cluster/heartbeat.c +23 ~~-31~~

@@ -1484,13 +1484,12 @@ static int o2hb_read_block_input(struct o2hb_region *reg,
 				 unsigned long *ret_bytes,
 				 unsigned int *ret_bits)
 {
-	unsigned long bytes;
-	char *p = (char *)page;
-
-	bytes = simple_strtoul(p, &p, 0);
-	if (!p || (*p && (*p != '\n')))
-		return -EINVAL;
+	unsigned int bytes;
+	int rv;
 
+	rv = kstrtouint(page, 0, &bytes);
+	if (rv < 0)
+		return rv;
 	/* Heartbeat and fs min / max block sizes are the same. */
 	if (bytes > 4096 || bytes < 512)
 		return -ERANGE;

@@ -1543,18 +1542,14 @@ static ssize_t o2hb_region_start_block_write(struct o2hb_region *reg,
 					     const char *page,
 					     size_t count)
 {
-	unsigned long long tmp;
-	char *p = (char *)page;
+	int rv;
 
 	if (reg->hr_bdev)
 		return -EINVAL;
 
-	tmp = simple_strtoull(p, &p, 0);
-	if (!p || (*p && (*p != '\n')))
-		return -EINVAL;
-
-	reg->hr_start_block = tmp;
-
+	rv = kstrtoull(page, 0, &reg->hr_start_block);
+	if (rv < 0)
+		return rv;
 	return count;
 }

@@ -1568,20 +1563,19 @@ static ssize_t o2hb_region_blocks_write(struct o2hb_region *reg,
 					const char *page,
 					size_t count)
 {
-	unsigned long tmp;
-	char *p = (char *)page;
+	unsigned int tmp;
+	int rv;
 
 	if (reg->hr_bdev)
 		return -EINVAL;
 
-	tmp = simple_strtoul(p, &p, 0);
-	if (!p || (*p && (*p != '\n')))
-		return -EINVAL;
-
+	rv = kstrtouint(page, 0, &tmp);
+	if (rv < 0)
+		return rv;
 	if (tmp > O2NM_MAX_NODES || tmp == 0)
 		return -ERANGE;
 
-	reg->hr_blocks = (unsigned int)tmp;
+	reg->hr_blocks = tmp;
 
 	return count;
 }

@@ -1717,9 +1711,8 @@ static ssize_t o2hb_region_dev_write(struct o2hb_region *reg,
 				     size_t count)
 {
 	struct task_struct *hb_task;
-	long fd;
+	int fd;
 	int sectsize;
-	char *p = (char *)page;
 	struct fd f;
 	struct inode *inode;
 	ssize_t ret = -EINVAL;

@@ -1733,10 +1726,9 @@ static ssize_t o2hb_region_dev_write(struct o2hb_region *reg,
 	if (o2nm_this_node() == O2NM_MAX_NODES)
 		goto out;
 
-	fd = simple_strtol(p, &p, 0);
-	if (!p || (*p && (*p != '\n')))
+	ret = kstrtoint(page, 0, &fd);
+	if (ret < 0)
 		goto out;
-
 	if (fd < 0 || fd >= INT_MAX)
 		goto out;

@@ -2210,12 +2202,12 @@ static ssize_t o2hb_heartbeat_group_threshold_store(struct o2hb_heartbeat_group
 						    const char *page,
 						    size_t count)
 {
-	unsigned long tmp;
-	char *p = (char *)page;
+	unsigned int tmp;
+	int rv;
 
-	tmp = simple_strtoul(p, &p, 10);
-	if (!p || (*p && (*p != '\n')))
-                return -EINVAL;
+	rv = kstrtouint(page, 10, &tmp);
+	if (rv < 0)
+		return rv;
 
 	/* this will validate ranges for us. */
 	o2hb_dead_threshold_set((unsigned int) tmp);

fs/ocfs2/cluster/nodemanager.c +22 ~~-28~~

93 lines not on this page · raw message

fs/ocfs2/stack_user.c +25 ~~-25~~

99 lines not on this page · raw message

Alexey Dobriyan

author

2015-05-02 01:03 UTC #17 lore raw

PATCH 10/10 ext2, ext3, ext4: convert to parse_integer()/kstrto*()

In reply to Alexey Dobriyan

Convert away from deprecated simple_strto*() interfaces.

Signed-off-by Alexey Dobriyan <[email protected]>

---

 fs/ext2/super.c |    6 ++++--
 fs/ext3/super.c |    7 ++++---
 fs/ext4/super.c |   15 +++++++--------
 3 files changed, 15 insertions(+), 13 deletions(-)

fs/ext2/super.c +4 -2

@@ -383,16 +383,18 @@ static unsigned long get_sb_block(void **data)
 {
 	unsigned long 	sb_block;
 	char 		*options = (char *) *data;
+	int rv;
 
 	if (!options || strncmp(options, "sb=", 3) != 0)
 		return 1;	/* Default location */
 	options += 3;
-	sb_block = simple_strtoul(options, &options, 0);
-	if (*options && *options != ',') {
+	rv = parse_integer(options, 0, &sb_block);
+	if (rv < 0 || (options[rv] && options[rv] != ',')) {
 		printk("EXT2-fs: Invalid sb specification: %s\n",
 		       (char *) *data);
 		return 1;
 	}
+	options += rv;
 	if (*options == ',')
 		options++;
 	*data = (void *) options;

fs/ext3/super.c +4 -3

@@ -902,17 +902,18 @@ static ext3_fsblk_t get_sb_block(void **data, struct super_block *sb)
 {
 	ext3_fsblk_t	sb_block;
 	char		*options = (char *) *data;
+	int rv;
 
 	if (!options || strncmp(options, "sb=", 3) != 0)
 		return 1;	/* Default location */
 	options += 3;
-	/*todo: use simple_strtoll with >32bit ext3 */
-	sb_block = simple_strtoul(options, &options, 0);
-	if (*options && *options != ',') {
+	rv = parse_integer(options, 0, &sb_block);
+	if (rv < 0 || (options[rv] && options[rv] != ',')) {
 		ext3_msg(sb, KERN_ERR, "error: invalid sb specification: %s",
 		       (char *) *data);
 		return 1;
 	}
+	options += rv;
 	if (*options == ',')
 		options++;
 	*data = (void *) options;

fs/ext4/super.c +7 -8

@@ -1210,18 +1210,19 @@ static ext4_fsblk_t get_sb_block(void **data)
 {
 	ext4_fsblk_t	sb_block;
 	char		*options = (char *) *data;
+	int rv;
 
 	if (!options || strncmp(options, "sb=", 3) != 0)
 		return 1;	/* Default location */
 
 	options += 3;
-	/* TODO: use simple_strtoll with >32bit ext4 */
-	sb_block = simple_strtoul(options, &options, 0);
-	if (*options && *options != ',') {
+	rv = parse_integer(options, 0, &sb_block);
+	if (rv < 0 || (options[rv] && options[rv] != ',')) {
 		printk(KERN_ERR "EXT4-fs: Invalid sb specification: %s\n",
 		       (char *) *data);
 		return 1;
 	}
+	options += rv;
 	if (*options == ',')
 		options++;
 	*data = (void *) options;

@@ -2491,10 +2492,10 @@ static ssize_t inode_readahead_blks_store(struct ext4_attr *a,
 					  struct ext4_sb_info *sbi,
 					  const char *buf, size_t count)
 {
-	unsigned long t;
+	unsigned int t;
 	int ret;
 
-	ret = kstrtoul(skip_spaces(buf), 0, &t);
+	ret = kstrtouint(skip_spaces(buf), 0, &t);
 	if (ret)
 		return ret;

@@ -2518,13 +2519,11 @@ static ssize_t sbi_ui_store(struct ext4_attr *a,
 			    const char *buf, size_t count)
 {
 	unsigned int *ui = (unsigned int *) (((char *) sbi) + a->u.offset);
-	unsigned long t;
 	int ret;
 
-	ret = kstrtoul(skip_spaces(buf), 0, &t);
+	ret = kstrtouint(skip_spaces(buf), 0, ui);
 	if (ret)
 		return ret;
-	*ui = t;
 	return count;
 }

Rasmus Villemoes

reviewer

2015-05-04 13:24 UTC #18 lore raw

In reply to Alexey Dobriyan

Quoted 25 lines · Alexey Dobriyan

kstrto*() and kstrto*_from_user() family of functions were added to help with parsing one integer written as string to proc/sysfs/debugfs files. But they have a limitation: string passed must end with \0 or \n\0. There are enough places where kstrto*() functions can't be used because of this limitation. Trivial example: major:minor "%u:%u".
Currently the only way to parse everything is simple_strto*() functions. But they are suboptimal:
* they do not detect overflow (can be fixed, but no one bothered since ~0.99.11),
* there are only 4 of them -- long and "long long" versions,
This leads to silent truncation in the most simple case:
	val = strtoul(s, NULL, 0);
* half of the people think that "char **endp" argument is necessary and add unnecessary variable.
OpenBSD people, fed up with how complex correct integer parsing is, added strtonum(3) to fixup for deficiencies of libc-style integer parsing:
http://www.openbsd.org/cgi-bin/man.cgi/OpenBSD-current/man3/strtonum.3?query=strtonum&arch=i386
It'd be OK to copy that but it relies on "errno" and fixed strings as error reporting channel which I think is not OK for kernel. strtonum() also doesn't report number of characted consumed.

Quoted 6 lines

What to do?
Enter parse_integer().
	int parse_integer(const char *s, unsigned int base, T *val);

I like the general idea. Few nits below (and in reply to other patches).

First: Could you tell me what tree I can commit this on top of, to see what gcc makes of it.

Quoted 11 lines

Rationale:
* parse_integer() is exactly 1 (one) interface not 4 or many,one for each type.
* parse_integer() reports -E errors reliably in a canonical kernel way:
	rv = parse_integer(str, 10, &val);
	if (rv < 0)
		return rv;

Quoted 8 lines

* parse_integer() writes result only if there were no errors, at least one digit has to be consumed,
* parse_integer doesn't mix error reporting channel and value channel, it does mix error and number-of-character-consumed channel, though.
* parse_integer() reports number of characters consumed, makes parsing multiple values easy:

I agree that these are desirable properties, and the way it should be done.

Quoted 11 lines

There are several deficiencies in parse_integer() compared to strto*():
* can't be used in initializers:
	const T x = strtoul();
* can't be used with bitfields,
* can't be used in expressions:
	x = strtol() * 1024;
	x = y = strtol() * 1024;
	x += strtol();

It's nice that you list these, but...

Quoted 3 lines

In defense of parse_integer() all I can say, is that using strtol() in expression or initializer promotes no error checking and thus probably should not be encouraged in C,

...agreed - I actually think it is a _good_ thing that the parse_integer interface makes it impossible to use in these cases.

Quoted 58 lines

include/linux/kernel.h +5 -2

@@ -10,6 +10,7 @@
 #include <linux/bitops.h>
 #include <linux/log2.h>
 #include <linux/typecheck.h>
+#include <linux/parse-integer.h>
 #include <linux/printk.h>
 #include <linux/dynamic_debug.h>
 #include <asm/byteorder.h>

@@ -387,8 +388,10 @@ static inline int __must_check kstrtos32_from_user(const char __user *s, size_t
 	return kstrtoint_from_user(s, count, base, res);
 }
 
-/* Obsolete, do not use.  Use kstrto<foo> instead */
-
+/*
+ * Obsolete, do not use.
+ * Use parse_integer(), kstrto*(), kstrto*_from_user(), sscanf().
+ */
 extern unsigned long simple_strtoul(const char *,char **,unsigned int);
 extern long simple_strtol(const char *,char **,unsigned int);
 extern unsigned long long simple_strtoull(const char *,char **,unsigned int);

include/linux/parse-integer.h new file +32 -0

@@ -0,0 +1,79 @@
+#ifndef _PARSE_INTEGER_H
+#define _PARSE_INTEGER_H
+#include <linux/compiler.h>
+#include <linux/types.h>
+
+/*
+ * int parse_integer(const char *s, unsigned int base, T *val);
+ *
+ * Convert integer string representation to an integer.
+ * Range of accepted values equals to that of type T.
+ *
+ * Conversion to unsigned integer accepts sign "+".
+ * Conversion to signed integer accepts sign "+" and sign "-".
+ *
+ * Radix 0 means autodetection: leading "0x" implies radix 16,
+ * leading "0" implies radix 8, otherwise radix is 10.
+ * Autodetection hint works after optional sign, but not before.
+ *
+ * Return number of characters parsed or -E.
+ *
+ * "T=char" case is not supported because -f{un,}signed-char can silently
+ * change range of accepted values.
+ */
+#define parse_integer(s, base, val)	\
+({					\
+	const char *_s = (s);		\
+	unsigned int _base = (base);	\
+	typeof(val) _val = (val);	\
+					\
+	__builtin_choose_expr(						\
+	__builtin_types_compatible_p(typeof(_val), signed char *),	\
+	_parse_integer_sc(_s, _base, (void *)_val),

Quoted 1 line

\

Why the (void*) cast? Isn't _val supposed to have precisely the type expected by _parse_integer_sc at this point?

include/linux/parse-integer.h line 59

Quoted 12 lines

include/linux/parse-integer.h new file +12 -0

+	__builtin_choose_expr(						\
+	__builtin_types_compatible_p(typeof(_val), long *) && sizeof(long) == 4,\
+	_parse_integer_i(_s, _base, (void *)_val),			\
+	__builtin_choose_expr(						\
+	__builtin_types_compatible_p(typeof(_val), long *) && sizeof(long) == 8,\
+	_parse_integer_ll(_s, _base, (void *)_val),			\
+	__builtin_choose_expr(						\
+	__builtin_types_compatible_p(typeof(_val), unsigned long *) && sizeof(unsigned long) == 4,\
+	_parse_integer_u(_s, _base, (void *)_val),			\
+	__builtin_choose_expr(						\
+	__builtin_types_compatible_p(typeof(_val), unsigned long *) && sizeof(unsigned long) == 8,\
+	_parse_integer_ull(_s, _base, (void *)_val),			\

Ah, I see. In these cases, one probably has to do a cast to pass a (long*) as either (int*) or (long long*) - but why not cast to the type actually expected by _parse_integer_* instead of the launder-anything (void*)?

Another thing: It may be slightly confusing that this can't be used with an array passed as val. This won't work:

long x[1];
rv = parse_integer(s, 0, x);

One could argue that one should pass &x[0] instead, but since this is a macro, gcc doesn't really give a very helpful error (I just get "error:
invalid initializer"). I think it can be fixed simply by declaring _val using typeof(&val[0]).

Quoted 17 lines

include/linux/parse-integer.h new file +17 -0

+int _parse_integer_ull(const char *s, unsigned int base, unsigned long long *val)
+{
+	int rv;
+
+	if (*s == '-') {
+		return -EINVAL;
+	} else if (*s == '+') {
+		rv = __parse_integer(s + 1, base, val);
+		if (rv < 0)
+			return rv;
+		return rv + 1;
+	} else
+		return __parse_integer(s, base, val);
+}
+EXPORT_SYMBOL(_parse_integer_ull);
+
+int _parse_integer_ll(const char *s, unsigned int base, long long *val)

include/linux/parse-integer.h

Quoted 10 lines

include/linux/parse-integer.h new file +10 -0

+{
+	unsigned long long tmp;
+	int rv;
+
+	if (*s == '-') {
+		rv = __parse_integer(s + 1, base, &tmp);
+		if (rv < 0)
+			return rv;
+		if ((long long)-tmp >= 0)
+			return -ERANGE;

Is there any reason to disallow "-0"?

Rasmus

Alexey Dobriyan

author

2015-05-04 14:33 UTC #19 lore raw

Quoted 11 lines · Rasmus Villemoes

Quoted 4 lines · Alexey Dobriyan
Enter parse_integer().
      int parse_integer(const char *s, unsigned int base, T *val);
I like the general idea. Few nits below (and in reply to other patches).
First: Could you tell me what tree I can commit this on top of, to see what gcc makes of it.

Any recent kernel should be OK, code it quite self contained. I've just applied first two patches on top 4.1-rc2. BTW the correct order is

1) [PATCH 01/10] Add parse_integer() (replacement for simple_strto*()) *** 2) [PATCH CORRECT 03/10] parse_integer: convert sscanf()
3) [PATCH 03/10] parse_integer: convert sscanf()
4) [PATCH 04/10] sscanf: fix overflow
...
10) [PATCH 10/10] ext2, ext3, ext4: convert to parse_integer()/kstrto*()

I've copied patch #2 twice, so it won't apply and resent it with subject from patch #3 to confuse everyone even more.

Quoted 27 lines

Quoted 10 lines

include/linux/parse-integer.h new file +9 -0

+#define parse_integer(s, base, val)  \
+({                                   \
+     const char *_s = (s);           \
+     unsigned int _base = (base);    \
+     typeof(val) _val = (val);       \
+                                     \
+     __builtin_choose_expr(                                          \
+     __builtin_types_compatible_p(typeof(_val), signed char *),      \
+     _parse_integer_sc(_s, _base, (void *)_val),

Why the (void*) cast? Isn't _val supposed to have precisely the type expected by _parse_integer_sc at this point?

include/linux/parse-integer.h line 59

Quoted 12 lines

include/linux/parse-integer.h new file +12 -0

+     __builtin_choose_expr(                                          \
+     __builtin_types_compatible_p(typeof(_val), long *) && sizeof(long) == 4,\
+     _parse_integer_i(_s, _base, (void *)_val),                      \
+     __builtin_choose_expr(                                          \
+     __builtin_types_compatible_p(typeof(_val), long *) && sizeof(long) == 8,\
+     _parse_integer_ll(_s, _base, (void *)_val),                     \
+     __builtin_choose_expr(                                          \
+     __builtin_types_compatible_p(typeof(_val), unsigned long *) && sizeof(unsigned long) == 4,\
+     _parse_integer_u(_s, _base, (void *)_val),                      \
+     __builtin_choose_expr(                                          \
+     __builtin_types_compatible_p(typeof(_val), unsigned long *) && sizeof(unsigned long) == 8,\
+     _parse_integer_ull(_s, _base, (void *)_val),                    \

Quoted 3 lines

Ah, I see. In these cases, one probably has to do a cast to pass a (long*) as either (int*) or (long long*) - but why not cast to the type actually expected by _parse_integer_* instead of the launder-anything (void*)?

First macro was written without casts at all naively thinking that gcc will only typecheck in chosen __builtin_choose_expr branch. But it doesn't do that, remove casts and observe million of warnings. So I shut it up with "void *". Branch is chosen base on __b_t_c_p expression and I don't think it is possible to sneak in incorrect pointer.

Quoted 9 lines

Another thing: It may be slightly confusing that this can't be used with an array passed as val. This won't work:
long x[1];
rv = parse_integer(s, 0, x);
One could argue that one should pass &x[0] instead, but since this is a macro, gcc doesn't really give a very helpful error (I just get "error:
invalid initializer"). I think it can be fixed simply by declaring _val using typeof(&val[0]).

I'd say &x[0] is way more clear that x in this case, but objection taken. kstrto*() works in exactly same situation after all.

Quoted 21 lines

lib/parse-integer.c new file +21 -0

+int _parse_integer_ull(const char *s, unsigned int base, unsigned long long *val)
+{
+     int rv;
+
+     if (*s == '-') {
+             return -EINVAL;
+     } else if (*s == '+') {
+             rv = __parse_integer(s + 1, base, val);
+             if (rv < 0)
+                     return rv;
+             return rv + 1;
+     } else
+             return __parse_integer(s, base, val);
+}
+EXPORT_SYMBOL(_parse_integer_ull);
+
+int _parse_integer_ll(const char *s, unsigned int base, long long *val)
+{
+     unsigned long long tmp;
+     int rv;
+

Quoted 8 lines

lib/parse-integer.c line 92

Quoted 6 lines

lib/parse-integer.c new file +6 -0

+     if (*s == '-') {
+             rv = __parse_integer(s + 1, base, &tmp);
+             if (rv < 0)
+                     return rv;
+             if ((long long)-tmp >= 0)
+                     return -ERANGE;

Is there any reason to disallow "-0"?

No! -0 is not accepted because code is copied from kstrtoll() which doesn't accept "-0". It is even in the testsuite:

static void __init test_kstrtoll_fail(void)
{
...
/* negative zero isn't an integer in Linux */

{"-0",  0},
{"-0",  8},
{"-0",  10},
{"-0",  16},

Frankly I don't even remember why it does that, and no one noticed until now. libc functions accept "-0".

Rasmus Villemoes

reviewer

2015-05-04 16:44 UTC #20 lore raw

[I'm merging the subthreads below]

Quoted 17 lines · Alexey Dobriyan

Quoted 3 lines · Rasmus Villemoes
First: Could you tell me what tree I can commit this on top of, to see what gcc makes of it.
Any recent kernel should be OK, code it quite self contained. I've just applied first two patches on top 4.1-rc2. BTW the correct order is
1) [PATCH 01/10] Add parse_integer() (replacement for simple_strto*()) *** 2) [PATCH CORRECT 03/10] parse_integer: convert sscanf()
3) [PATCH 03/10] parse_integer: convert sscanf()
4) [PATCH 04/10] sscanf: fix overflow
...
10) [PATCH 10/10] ext2, ext3, ext4: convert to parse_integer()/kstrto*()

Quoted 2 lines

I've copied patch #2 twice, so it won't apply and resent it with subject from patch #3 to confuse everyone even more.

You certainly successfully confused me :-)

Quoted 31 lines

Quoted 27 lines

Quoted 10 lines

include/linux/parse-integer.h new file +9 -0

+#define parse_integer(s, base, val)  \
+({                                   \
+     const char *_s = (s);           \
+     unsigned int _base = (base);    \
+     typeof(val) _val = (val);       \
+                                     \
+     __builtin_choose_expr(                                          \
+     __builtin_types_compatible_p(typeof(_val), signed char *),      \
+     _parse_integer_sc(_s, _base, (void *)_val),

Why the (void*) cast? Isn't _val supposed to have precisely the type expected by _parse_integer_sc at this point?

include/linux/parse-integer.h line 59

Quoted 12 lines

include/linux/parse-integer.h new file +12 -0

+     __builtin_choose_expr(                                          \
+     __builtin_types_compatible_p(typeof(_val), long *) && sizeof(long) == 4,\
+     _parse_integer_i(_s, _base, (void *)_val),                      \
+     __builtin_choose_expr(                                          \
+     __builtin_types_compatible_p(typeof(_val), long *) && sizeof(long) == 8,\
+     _parse_integer_ll(_s, _base, (void *)_val),                     \
+     __builtin_choose_expr(                                          \
+     __builtin_types_compatible_p(typeof(_val), unsigned long *) && sizeof(unsigned long) == 4,\
+     _parse_integer_u(_s, _base, (void *)_val),                      \
+     __builtin_choose_expr(                                          \
+     __builtin_types_compatible_p(typeof(_val), unsigned long *) && sizeof(unsigned long) == 8,\
+     _parse_integer_ull(_s, _base, (void *)_val),                    \

Quoted 3 lines

Ah, I see. In these cases, one probably has to do a cast to pass a (long*) as either (int*) or (long long*) - but why not cast to the type actually expected by _parse_integer_* instead of the launder-anything (void*)?

Quoted 5 lines

First macro was written without casts at all naively thinking that gcc will only typecheck in chosen __builtin_choose_expr branch. But it doesn't do that, remove casts and observe million of warnings. So I shut it up with "void *". Branch is chosen base on __b_t_c_p expression and I don't think it is possible to sneak in incorrect pointer.

I see. I was worried about something like the cast laundering away a const qualifier, but (const int*) is not _b_t_c with (int*). So ok for now - I'll play around with this a little and see if one can get rid of the casts anyway.

Quoted 10 lines

Quoted 2 lines
Is there any reason to disallow "-0"?
No! -0 is not accepted because code is copied from kstrtoll() which doesn't accept "-0". It is even in the testsuite:
static void __init test_kstrtoll_fail(void)
{
...
/* negative zero isn't an integer in Linux */

Quoted 7 lines

{"-0",  0},
{"-0",  8},
{"-0",  10},
{"-0",  16},
Frankly I don't even remember why it does that, and no one noticed until now. libc functions accept "-0".

I think it's odd to accept "+0" but not "-0", but that's probably just because I'm a mathematician. Am I right that you just added these test cases because of the existing behaviour of kstrtoll? I suppose that behaviour is just a historical accident.

If "-0" is not going to be accepted, I think that deserves a comment (with rationale) in the parsing code and not hidden away in the test suite.

Quoted 15 lines

Quoted 10 lines
Review
Quoted 9 lines
unsigned long long memparse(const char *ptr, char **retptr) {
(quoted diff) +2 -4
-     char *endptr;   /* local pointer to end of parsed string */
+     unsigned long long val;
 
-     unsigned long long ret = simple_strtoull(ptr, &endptr, 0);
-
-     switch (*endptr) {
+     ptr += parse_integer(ptr, 0, &val);
Quoted 4 lines
This seems wrong. simple_strtoull used to "sanitize" the return value from the (old) _parse_integer, so that endptr still points into the given string. Unconditionally adding the result from parse_integer may make ptr point far before the actual string, into who-knows-what.

Quoted 5 lines

When converting I tried to preserve the amount of error checking done. simple_strtoull() either
a) return 0 and not advance pointer, or
b) return something and advance pointer.

Are we talking about the same simple_strtoull? I see

	cp = _parse_integer_fixup_radix(cp, &base);
	rv = _parse_integer(cp, base, &result);
	/* FIXME */
	cp += (rv & ~KSTRTOX_OVERFLOW);

so cp is definitely advanced even in case of overflow. And in the case of "underflow" (no digits found), the old code does initialize *result to 0, while parse_integer by design doesn't write anything.

Quoted 1 line

Current code just ignores error case, so do I.

There's a difference between ignoring an error (which the current code does), and ignoring _the possibility_ of an error (which the new code does).

There are lots of callers of memparse(), and I don't think any of them are prepared to handle *endp ending up pointing before the passed-in string (-EINVAL == -22, -ERANGE == -34). I can easily see how that could lead to an infinite loop, maybe worse.

[on the use of anonymous variables to get right type]

Quoted 10 lines

Quoted 2 lines
I suggest using the more obvious approach of declaring a union on the stack and pass the address of the appropriate member.
Union will work.
No that it matters, it's a filesystem option parsing code which is executed rarely near the top of sys_mount(), not exactly perfomance bottleneck.
It's a shame to lose such clever hack :-(

I'm sure you can find somewhere else to apply the hack :-)

Quoted 3 lines

Just grep for simple_strto* and see how much error checking people do. The aim of the patchset is to convert code, error checking is separate story.

I think that's the wrong approach. One reason for adding this new interface is precisely to facilitate error checking. So if you do some example conversions, please do them _properly_ to show how error checking is done.

As you say, it's easy to find places where error checking isn't done by just grepping for simple_strto*. I don't want to have to add parse_integer to that list. Abusing a new interface from the beginning isn't really a recipe for happiness.

Converting uses of simple_strto*() to parse_integer() doesn't add value if semantics of calling code is exactly the same. And it adds negative value if the conversions change the semantics in a surprising way, as in the memparse case above :-(

Rasmus

Alexey Dobriyan

author

2015-05-04 19:54 UTC #21 lore raw

Quoted 23 lines · Rasmus Villemoes

[I'm merging the subthreads below]
Quoted 11 lines · Alexey Dobriyan
On Mon, May 4, 2015 at 4:24 PM, Rasmus Villemoes
Quoted 1 line
Is there any reason to disallow "-0"?
No! -0 is not accepted because code is copied from kstrtoll() which doesn't accept "-0". It is even in the testsuite:
static void __init test_kstrtoll_fail(void)
{
...
/* negative zero isn't an integer in Linux */
Quoted 7 lines
{"-0",  0},
{"-0",  8},
{"-0",  10},
{"-0",  16},
Frankly I don't even remember why it does that, and no one noticed until now. libc functions accept "-0".

Quoted 8 lines

I think it's odd to accept "+0" but not "-0", but that's probably just because I'm a mathematician. Am I right that you just added these test cases because of the existing behaviour of kstrtoll? I suppose that behaviour is just a historical accident.
If "-0" is not going to be accepted, I think that deserves a comment (with rationale) in the parsing code and not hidden away in the test suite.

Again, I honestly do not remember why "-0" was banned. Let's change it to "+0 -0" for signed case, "+0" for unsigned case.

Quoted 34 lines

Quoted 15 lines
Quoted 10 lines
Review
Quoted 9 lines
unsigned long long memparse(const char *ptr, char **retptr) {
(quoted diff) +2 -4
-     char *endptr;   /* local pointer to end of parsed string */
+     unsigned long long val;
 
-     unsigned long long ret = simple_strtoull(ptr, &endptr, 0);
-
-     switch (*endptr) {
+     ptr += parse_integer(ptr, 0, &val);
Quoted 4 lines
This seems wrong. simple_strtoull used to "sanitize" the return value from the (old) _parse_integer, so that endptr still points into the given string. Unconditionally adding the result from parse_integer may make ptr point far before the actual string, into who-knows-what.
Quoted 5 lines
When converting I tried to preserve the amount of error checking done. simple_strtoull() either
a) return 0 and not advance pointer, or
b) return something and advance pointer.
Are we talking about the same simple_strtoull? I see
	cp = _parse_integer_fixup_radix(cp, &base);
	rv = _parse_integer(cp, base, &result);
	/* FIXME */
	cp += (rv & ~KSTRTOX_OVERFLOW);
so cp is definitely advanced even in case of overflow. And in the case of "underflow" (no digits found), the old code does initialize *result to 0, while parse_integer by design doesn't write anything.
Quoted 1 line
Current code just ignores error case, so do I.

Quoted 8 lines

There's a difference between ignoring an error (which the current code does), and ignoring _the possibility_ of an error (which the new code does).
There are lots of callers of memparse(), and I don't think any of them are prepared to handle *endp ending up pointing before the passed-in string (-EINVAL == -22, -ERANGE == -34). I can easily see how that could lead to an infinite loop, maybe worse.

Yeah, possible bug could become worse, I'll add error checking, but, seriously, you're defending this :^)

case Opt_nr_inodes:
===> /* memparse() will accept a K/M/G without a digit */
===> if (!isdigit(*args[0].from))

===>			goto bad_val;
		pconfig->nr_inodes = memparse(args[0].from, &rest);
		break;

memparse() is misdesigned in the same sense strtoul() is misdesigned. Every "memparse(s, NULL)" user is a bug for example.

Rasmus Villemoes

reviewer

2015-05-04 21:48 UTC #22 lore raw

Quoted 8 lines · Alexey Dobriyan

Quoted 4 lines
There are lots of callers of memparse(), and I don't think any of them are prepared to handle *endp ending up pointing before the passed-in string (-EINVAL == -22, -ERANGE == -34). I can easily see how that could lead to an infinite loop, maybe worse.
Yeah, possible bug could become worse, I'll add error checking, but, seriously, you're defending this :^)

Quoted 7 lines

case Opt_nr_inodes:
===> /* memparse() will accept a K/M/G without a digit */
===> if (!isdigit(*args[0].from))
===>			goto bad_val;
		pconfig->nr_inodes = memparse(args[0].from, &rest);
		break;

No, I'm not defending memparse(), simple_strto* or any of their callers. I'm just trying to say that you can't change the semantics of memparse() without considering all its callers.

I don't think there's any way to "add error checking" and preserve the exact memparse() semantic - in other words, I don't think simple_strto* can actually be implemented in terms of parse_integer. But that's not a bad thing - we want to get rid of those.

Quoted 2 lines

memparse() is misdesigned in the same sense strtoul() is misdesigned. Every "memparse(s, NULL)" user is a bug for example.

Yes, memparse is misdesigned, since it doesn't have a way to indicate error. That leads me to: There's no point in adding error checking to the integer parsing part without also checking the left shifts for overflow.

I think the right approach is to rename memparse to legacy_memparse and introduce a memparse with semantics that allow error checking. One could start by introducing that under the name sane_memparse. But there are probably lots of simple_strto*() uses that are easier to eliminate.

Rasmus