[CHANGELOG]
v3:
- Fix the 32bit pointer pattern in the test case
The old pointer pattern for 32 bit systems is in fact 40 bits,
which would still lead to sparse warning.
The newer pattern is using UINTPTR_MAX to trim the pattern, then
converted to a pointer, which should not cause any trimmed bits and
make sparse happy.
v2:
- Make _parse_integer_fixup_radix() to always treat "0x" as hex
This is to make sure invalid strings like "0x" or "0xG" to fail
as expected for memparse_safe().
Or they would only parse the first 0, then leaving "x" for caller
to handle.
- Update the test case to include above failure cases
This including:
* "0x", just hex prefix without any suffix/follow up chars
* "0xK", just hex prefix and a stray suffix
* "0xY", hex prefix with an invalid char
- Fix a bug in btrfs' conversion to memparse_safe()
Where I forgot to delete the old memparse() line.
- Fix a compiler warning on m68K
On that platform, a pointer (32 bits) is smaller than unsigned long long
(64 bits), which can cause static checker to warn.
Function memparse() lacks error handling:
- If no valid number string at all
In that case @retptr would just be updated and return value would be
zero.
- No overflown detection
This applies to both the number string part, and the suffixes part.
And since we have no way to indicate errors, we can get weird results
like:
"25E" -> 10376293541461622784 (9E)
This is due to the fact that for "E" suffix, there is only 4 bits
left, and 25 with 60 bits left shift would lead to overflow.
(And decision to support for that "E" suffix is already cursed)
So here we introduce a safer version of it: memparse_safe(), and mark
the original one deprecated.
Unfortunately I didn't find a good way to mark it deprecated, as with
recent -Werror changes, '__deprecated' marco does not seem to warn
anymore.
The new helper has the following advantages:
- Better overflow and invalid string detection
The overflow detection is for both the numberic part, and with the
suffix. Thus above "25E" would be rejected correctly.
The invalid string part means if there is no valid number starts at
the buffer, we return -EINVAL.
- Allow caller to select the suffixes, and saner default ones
The new default one would be "KMGTP", without the cursed and overflow
prone "E".
Some older code like setup_elfcorehdr() would benefit from it, if the
code really wants to only allow "KMG" suffixes.
- Keep the old @retptr behavior
So the existing callers can easily migrate to the new one, without the
need to do extra strsep() work.
- Finally test cases
The test case would cover more things than the existing kstrtox
tests:
* The @retptr behavior
Either for bad cases, which @retptr should not be touched,
or for good cases, the @retptr is properly advanced,
* Return value verification
Make sure we distinguish -EINVAL and -ERANGE correctly.
* Valid string with suffix, but disable the corresponding suffix
To make sure we still got the numeric string parsed, and can
still pass the disabled suffix to the caller.
With the new helper, migrate btrfs to the interface, and since the
@retptr behavior is the same, we won't cause any behavior change.
Qu Wenruo (4):
kstrtox: always skip the leading "0x" even if no more valid chars
kstrtox: introduce a safer version of memparse()
kstrtox: add unit tests for memparse_safe()
btrfs: migrate to the newer memparse_safe() helper
arch/x86/boot/string.c | 2 +-
fs/btrfs/ioctl.c | 6 +-
fs/btrfs/super.c | 9 +-
fs/btrfs/sysfs.c | 14 ++-
include/linux/kernel.h | 8 +-
include/linux/kstrtox.h | 15 +++
lib/cmdline.c | 5 +-
lib/kstrtox.c | 98 +++++++++++++++-
lib/test-kstrtox.c | 244 ++++++++++++++++++++++++++++++++++++++++
9 files changed, 392 insertions(+), 9 deletions(-)
--
2.43.0
Currently the invalid string "0x" (only hex prefix, no valid chars
followed) would make _parse_integer_fixup_radix() to treat it as octal.
This is due to the fact that the function would only auto-detect hex if
and only if there is any valid hex char after "0x".
Or it would only go octal instead.
Thankfully this won't affect our unit test, as "0x" would still be
treated as failure (-EINVAL) anyway:
- Old code treats "0x" as '0' with tailing 'x'
Thus return -EINVAL due to the tailing 'x'.
- New code treats "0x" as "0x" suffix with nothing following up
Thus return -EINVAL due to no valid string.
But for the incoming memparse_safe(), the remaining string would still
be consumed by the caller, and we need to properly treat "0x" as an
invalid string.
So this patch would make _parse_integer_fixup_radix() to forcefully go
hex no matter if there is any valid char following.
And there is a also copy of _parse_integer_fixup_radix() inside
arch/x86/boot/string.c, to keep the code consistent this patch would
also modify that copy.
Thankfully for that copy in arch/x86/boot/string.c, it's only doing
kstrtoll(), thus there would be no behavior change, just as explained
above.
Signed-off-by: Qu Wenruo <[email protected]>
Reviewed-by: David Disseldorp <[email protected]>
---
arch/x86/boot/string.c | 2 +-
lib/kstrtox.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/boot/string.c b/arch/x86/boot/string.c
index 1c8541ae3b3a..49750ef697bb 100644
--- a/arch/x86/boot/string.c
+++ b/arch/x86/boot/string.c
@@ -233,7 +233,7 @@ static const char *_parse_integer_fixup_radix(const char *s, unsigned int *base)
{
if (*base == 0) {
if (s[0] == '0') {
- if (_tolower(s[1]) == 'x' && isxdigit(s[2]))
+ if (_tolower(s[1]) == 'x')
*base = 16;
else
*base = 8;
diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index d586e6af5e5a..41c9a499bbf3 100644
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c
@@ -27,7 +27,7 @@ const char *_parse_integer_fixup_radix(const char *s, unsigned int *base)
{
if (*base == 0) {
if (s[0] == '0') {
- if (_tolower(s[1]) == 'x' && isxdigit(s[2]))
+ if (_tolower(s[1]) == 'x')
*base = 16;
else
*base = 8;
--
2.43.0
[BUGS]
Function memparse() lacks error handling:
- If no valid number string at all
In that case @retptr would just be updated and return value would be
zero.
- No overflown detection
This applies to both the number string part, and the suffixes part.
And since we have no way to indicate errors, we can get weird results
like:
"25E" -> 10376293541461622784 (9E)
This is due to the fact that for "E" suffix, there is only 4 bits
left, and 25 with 60 bits left shift would lead to overflow.
[CAUSE]
The root cause is already mentioned in the comments of the function, the
usage of simple_strtoull() is the source of evil.
Furthermore the function prototype is no good either, just returning an
unsigned long long gives us no way to indicate an error.
[FIX]
Due to the prototype limits, we can not have a drop-in replacement for
memparse().
This patch can only help by introduce a new helper, memparse_safe(), and
mark the old memparse() deprecated.
The new memparse_safe() has the following improvement:
- Invalid string detection
If no number string can be detected at all, -EINVAL would be returned.
- Better overflow detection
Both the string part and the extra left shift would have overflow
detection.
Any overflow would result -ERANGE.
- Safer default suffix selection
The helper allows the caller to choose the suffixes that they want to
use.
But only "KMGTP" are recommended by default since the "E" leaves only
4 bits before overflow.
For those callers really know what they are doing, they can still
manually to include all suffixes.
Due to the prototype change, callers should migrate to the new one and
change their code and add extra error handling.
Signed-off-by: Qu Wenruo <[email protected]>
Reviewed-by: David Disseldorp <[email protected]>
---
include/linux/kernel.h | 8 +++-
include/linux/kstrtox.h | 15 +++++++
lib/cmdline.c | 5 ++-
lib/kstrtox.c | 96 +++++++++++++++++++++++++++++++++++++++++
4 files changed, 122 insertions(+), 2 deletions(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index d9ad21058eed..b1b6da60ea43 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -201,7 +201,13 @@ void do_exit(long error_code) __noreturn;
extern int get_option(char **str, int *pint);
extern char *get_options(const char *str, int nints, int *ints);
-extern unsigned long long memparse(const char *ptr, char **retptr);
+
+/*
+ * DEPRECATED, lack of any kind of error handling.
+ *
+ * Use memparse_safe() from lib/kstrtox.c instead.
+ */
+extern __deprecated unsigned long long memparse(const char *ptr, char **retptr);
extern bool parse_option_str(const char *str, const char *option);
extern char *next_arg(char *args, char **param, char **val);
diff --git a/include/linux/kstrtox.h b/include/linux/kstrtox.h
index 7fcf29a4e0de..53a1e059dd31 100644
--- a/include/linux/kstrtox.h
+++ b/include/linux/kstrtox.h
@@ -9,6 +9,21 @@
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);
+enum memparse_suffix {
+ MEMPARSE_SUFFIX_K = 1 << 0,
+ MEMPARSE_SUFFIX_M = 1 << 1,
+ MEMPARSE_SUFFIX_G = 1 << 2,
+ MEMPARSE_SUFFIX_T = 1 << 3,
+ MEMPARSE_SUFFIX_P = 1 << 4,
+ MEMPARSE_SUFFIX_E = 1 << 5,
+};
+
+#define MEMPARSE_SUFFIXES_DEFAULT (MEMPARSE_SUFFIX_K | MEMPARSE_SUFFIX_M |\
+ MEMPARSE_SUFFIX_G | MEMPARSE_SUFFIX_T |\
+ MEMPARSE_SUFFIX_P)
+
+int __must_check memparse_safe(const char *s, enum memparse_suffix suffixes,
+ unsigned long long *res, char **retptr);
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);
diff --git a/lib/cmdline.c b/lib/cmdline.c
index 90ed997d9570..d379157de349 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -139,12 +139,15 @@ char *get_options(const char *str, int nints, int *ints)
EXPORT_SYMBOL(get_options);
/**
- * memparse - parse a string with mem suffixes into a number
+ * memparse - DEPRECATED, parse a string with mem suffixes into a number
* @ptr: Where parse begins
* @retptr: (output) Optional pointer to next char after parse completes
*
+ * There is no way to handle errors, and no overflown detection and string
+ * sanity checks.
* Parses a string into a number. The number stored at @ptr is
* potentially suffixed with K, M, G, T, P, E.
+ *
*/
unsigned long long memparse(const char *ptr, char **retptr)
diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index 41c9a499bbf3..a1e4279f52b3 100644
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c
@@ -113,6 +113,102 @@ static int _kstrtoull(const char *s, unsigned int base, unsigned long long *res)
return 0;
}
+/**
+ * memparse_safe - convert a string to an unsigned long long, safer version of
+ * memparse()
+ *
+ * @s: The start of the string. Must be null-terminated.
+ * The base would be determined automatically, if it starts with
+ * "0x" the base would be 16, if it starts with "0" the base
+ * would be 8, otherwise the base would be 10.
+ * After a valid number string, there can be at most one
+ * case-insensive suffix character, specified by the @suffixes
+ * parameter.
+ *
+ * @suffixes: The suffixes which should be parsed. Use logical ORed
+ * memparse_suffix enum to indicate the supported suffixes.
+ * The suffixes are case-insensive, all 2 ^ 10 based.
+ * Supported ones are "KMGPTE".
+ * NOTE: If one suffix out of the supported one is hit, it would
+ * end the parse normally, with @retptr pointed to the unsupported
+ * suffix.
+ *
+ * @res: Where to write the result.
+ *
+ * @retptr: (output) Optional pointer to the next char after parse completes.
+ *
+ * Return 0 if any valid numberic string can be parsed, and @retptr updated.
+ * Return -INVALID if no valid number string can be found.
+ * Return -ERANGE if the number overflows.
+ * For minus return values, @retptr would not be updated.
+ */
+noinline int memparse_safe(const char *s, enum memparse_suffix suffixes,
+ unsigned long long *res, char **retptr)
+{
+ unsigned long long value;
+ unsigned int rv;
+ int shift = 0;
+ int base = 0;
+
+ s = _parse_integer_fixup_radix(s, &base);
+ rv = _parse_integer(s, base, &value);
+ if (rv & KSTRTOX_OVERFLOW)
+ return -ERANGE;
+ if (rv == 0)
+ return -EINVAL;
+
+ s += rv;
+ switch (*s) {
+ case 'K':
+ case 'k':
+ if (!(suffixes & MEMPARSE_SUFFIX_K))
+ break;
+ shift = 10;
+ break;
+ case 'M':
+ case 'm':
+ if (!(suffixes & MEMPARSE_SUFFIX_M))
+ break;
+ shift = 20;
+ break;
+ case 'G':
+ case 'g':
+ if (!(suffixes & MEMPARSE_SUFFIX_G))
+ break;
+ shift = 30;
+ break;
+ case 'T':
+ case 't':
+ if (!(suffixes & MEMPARSE_SUFFIX_T))
+ break;
+ shift = 40;
+ break;
+ case 'P':
+ case 'p':
+ if (!(suffixes & MEMPARSE_SUFFIX_P))
+ break;
+ shift = 50;
+ break;
+ case 'E':
+ case 'e':
+ if (!(suffixes & MEMPARSE_SUFFIX_E))
+ break;
+ shift = 60;
+ break;
+ }
+ if (shift) {
+ s++;
+ if (value >> (64 - shift))
+ return -ERANGE;
+ value <<= shift;
+ }
+ *res = value;
+ if (retptr)
+ *retptr = (char *)s;
+ return 0;
+}
+EXPORT_SYMBOL(memparse_safe);
+
/**
* kstrtoull - convert a string to an unsigned long long
* @s: The start of the string. The string must be null-terminated, and may also
--
2.43.0
The new tests cases for memparse_safe() include:
- The existing test cases for kstrtoull()
Including all the 3 bases (8, 10, 16), and all the ok and failure
cases.
Although there are something we need to verify specific for
memparse_safe():
* @retptr and @value are not modified for failure cases
* return value are correct for failure cases
* @retptr is correct for the good cases
- New test cases
Not only testing the result value, but also the @retptr, including:
* good cases with extra tailing chars, but without valid prefix
The @retptr should point to the first char after a valid string.
3 cases for all the 3 bases.
* good cases with extra tailing chars, with valid prefix
5 cases for all the suffixes.
* good cases with valid but disabled suffixes
The same 5 cases as the previous one, just disable the corresponding
suffixes.
* bad cases without any number but stray suffix
Should be rejected with -EINVAL
Signed-off-by: Qu Wenruo <[email protected]>
Reviewed-by: David Disseldorp <[email protected]>
---
lib/test-kstrtox.c | 244 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 244 insertions(+)
diff --git a/lib/test-kstrtox.c b/lib/test-kstrtox.c
index f355f67169b6..8f52b53c1374 100644
--- a/lib/test-kstrtox.c
+++ b/lib/test-kstrtox.c
@@ -268,6 +268,247 @@ static void __init test_kstrtoll_ok(void)
TEST_OK(kstrtoll, long long, "%lld", test_ll_ok);
}
+/*
+ * The special pattern to make sure the result is not modified for error cases.
+ */
+#define ULL_PATTERN (0xefefefef7a7a7a7aULL)
+#define POINTER_PATTERN ((void *)(uintptr_t)(ULL_PATTERN & UINTPTR_MAX))
+
+/* Want to include "E" suffix for full coverage. */
+#define MEMPARSE_TEST_SUFFIX (MEMPARSE_SUFFIX_K | MEMPARSE_SUFFIX_M |\
+ MEMPARSE_SUFFIX_G | MEMPARSE_SUFFIX_T |\
+ MEMPARSE_SUFFIX_P | MEMPARSE_SUFFIX_E)
+
+static void __init test_memparse_safe_fail(void)
+{
+ struct memparse_test_fail {
+ const char *str;
+ /* Expected error number, either -EINVAL or -ERANGE. */
+ unsigned int expected_ret;
+ };
+ static const struct memparse_test_fail tests[] __initconst = {
+ /* No valid string can be found at all. */
+ {"", -EINVAL},
+ {"\n", -EINVAL},
+ {"\n0", -EINVAL},
+ {"+", -EINVAL},
+ {"-", -EINVAL},
+
+ /* Only hex prefix, but no valid string. */
+ {"0x", -EINVAL},
+ {"0X", -EINVAL},
+
+ /* Only hex prefix, with suffix but still no valid string. */
+ {"0xK", -EINVAL},
+ {"0xM", -EINVAL},
+ {"0xG", -EINVAL},
+
+ /* Only hex prefix, with invalid chars. */
+ {"0xH", -EINVAL},
+ {"0xy", -EINVAL},
+
+ /*
+ * No support for any leading "+-" chars, even followed by a valid
+ * number.
+ */
+ {"-0", -EINVAL},
+ {"+0", -EINVAL},
+ {"-1", -EINVAL},
+ {"+1", -EINVAL},
+
+ /* Stray suffix would also be rejected. */
+ {"K", -EINVAL},
+ {"P", -EINVAL},
+
+ /* Overflow in the string itself*/
+ {"18446744073709551616", -ERANGE},
+ {"02000000000000000000000", -ERANGE},
+ {"0x10000000000000000", -ERANGE},
+
+ /*
+ * Good string but would overflow with suffix.
+ *
+ * Note, for "E" suffix, one should not use with hex, or "0x1E"
+ * would be treated as 0x1e (30 in decimal), not 0x1 and "E" suffix.
+ * Another reason "E" suffix is cursed.
+ */
+ {"16E", -ERANGE},
+ {"020E", -ERANGE},
+ {"16384P", -ERANGE},
+ {"040000P", -ERANGE},
+ {"16777216T", -ERANGE},
+ {"0100000000T", -ERANGE},
+ {"17179869184G", -ERANGE},
+ {"0200000000000G", -ERANGE},
+ {"17592186044416M", -ERANGE},
+ {"0400000000000000M", -ERANGE},
+ {"18014398509481984K", -ERANGE},
+ {"01000000000000000000K", -ERANGE},
+ };
+ unsigned int i;
+
+ for_each_test(i, tests) {
+ const struct memparse_test_fail *t = &tests[i];
+ unsigned long long tmp = ULL_PATTERN;
+ char *retptr = POINTER_PATTERN;
+ int ret;
+
+ ret = memparse_safe(t->str, MEMPARSE_TEST_SUFFIX, &tmp, &retptr);
+ if (ret != t->expected_ret) {
+ WARN(1, "str '%s', expected ret %d got %d\n", t->str,
+ t->expected_ret, ret);
+ continue;
+ }
+ if (tmp != ULL_PATTERN)
+ WARN(1, "str '%s' failed as expected, but result got modified",
+ t->str);
+ if (retptr != POINTER_PATTERN)
+ WARN(1, "str '%s' failed as expected, but pointer got modified",
+ t->str);
+ }
+}
+
+static void __init test_memparse_safe_ok(void)
+{
+ struct memparse_test_ok {
+ const char *str;
+ unsigned long long expected_value;
+ /* How many bytes the @retptr pointer should be moved forward. */
+ unsigned int retptr_off;
+
+ /* If 0, falls back to MEMPARSE_TEST_SUFFIX. */
+ enum memparse_suffix suffixes;
+ };
+ static DEFINE_TEST_OK(struct memparse_test_ok, tests) = {
+ /*
+ * The same pattern of kstrtoull, just with extra @retptr
+ * verification.
+ */
+ {"0", 0ULL, 1},
+ {"1", 1ULL, 1},
+ {"127", 127ULL, 3},
+ {"128", 128ULL, 3},
+ {"129", 129ULL, 3},
+ {"255", 255ULL, 3},
+ {"256", 256ULL, 3},
+ {"257", 257ULL, 3},
+ {"32767", 32767ULL, 5},
+ {"32768", 32768ULL, 5},
+ {"32769", 32769ULL, 5},
+ {"65535", 65535ULL, 5},
+ {"65536", 65536ULL, 5},
+ {"65537", 65537ULL, 5},
+ {"2147483647", 2147483647ULL, 10},
+ {"2147483648", 2147483648ULL, 10},
+ {"2147483649", 2147483649ULL, 10},
+ {"4294967295", 4294967295ULL, 10},
+ {"4294967296", 4294967296ULL, 10},
+ {"4294967297", 4294967297ULL, 10},
+ {"9223372036854775807", 9223372036854775807ULL, 19},
+ {"9223372036854775808", 9223372036854775808ULL, 19},
+ {"9223372036854775809", 9223372036854775809ULL, 19},
+ {"18446744073709551614", 18446744073709551614ULL, 20},
+ {"18446744073709551615", 18446744073709551615ULL, 20},
+
+ {"00", 00ULL, 2},
+ {"01", 01ULL, 2},
+ {"0177", 0177ULL, 4},
+ {"0200", 0200ULL, 4},
+ {"0201", 0201ULL, 4},
+ {"0377", 0377ULL, 4},
+ {"0400", 0400ULL, 4},
+ {"0401", 0401ULL, 4},
+ {"077777", 077777ULL, 6},
+ {"0100000", 0100000ULL, 7},
+ {"0100001", 0100001ULL, 7},
+ {"0177777", 0177777ULL, 7},
+ {"0200000", 0200000ULL, 7},
+ {"0200001", 0200001ULL, 7},
+ {"017777777777", 017777777777ULL, 12},
+ {"020000000000", 020000000000ULL, 12},
+ {"020000000001", 020000000001ULL, 12},
+ {"037777777777", 037777777777ULL, 12},
+ {"040000000000", 040000000000ULL, 12},
+ {"040000000001", 040000000001ULL, 12},
+ {"0777777777777777777777", 0777777777777777777777ULL, 22},
+ {"01000000000000000000000", 01000000000000000000000ULL, 23},
+ {"01000000000000000000001", 01000000000000000000001ULL, 23},
+ {"01777777777777777777776", 01777777777777777777776ULL, 23},
+ {"01777777777777777777777", 01777777777777777777777ULL, 23},
+
+ {"0x0", 0x0ULL, 3},
+ {"0x1", 0x1ULL, 3},
+ {"0x7f", 0x7fULL, 4},
+ {"0x80", 0x80ULL, 4},
+ {"0x81", 0x81ULL, 4},
+ {"0xff", 0xffULL, 4},
+ {"0x100", 0x100ULL, 5},
+ {"0x101", 0x101ULL, 5},
+ {"0x7fff", 0x7fffULL, 6},
+ {"0x8000", 0x8000ULL, 6},
+ {"0x8001", 0x8001ULL, 6},
+ {"0xffff", 0xffffULL, 6},
+ {"0x10000", 0x10000ULL, 7},
+ {"0x10001", 0x10001ULL, 7},
+ {"0x7fffffff", 0x7fffffffULL, 10},
+ {"0x80000000", 0x80000000ULL, 10},
+ {"0x80000001", 0x80000001ULL, 10},
+ {"0xffffffff", 0xffffffffULL, 10},
+ {"0x100000000", 0x100000000ULL, 11},
+ {"0x100000001", 0x100000001ULL, 11},
+ {"0x7fffffffffffffff", 0x7fffffffffffffffULL, 18},
+ {"0x8000000000000000", 0x8000000000000000ULL, 18},
+ {"0x8000000000000001", 0x8000000000000001ULL, 18},
+ {"0xfffffffffffffffe", 0xfffffffffffffffeULL, 18},
+ {"0xffffffffffffffff", 0xffffffffffffffffULL, 18},
+
+ /* Now with extra non-suffix chars to test @retptr update. */
+ {"1q84", 1, 1},
+ {"02o45", 2, 2},
+ {"0xffvii", 0xff, 4},
+
+ /*
+ * Valid suffix then tailing chars, to test the @retptr
+ * behavior.
+ */
+ {"68k ", 69632, 3},
+ {"8MS", 8388608, 2},
+ {"0xaeGis", 0x2b80000000, 5},
+ {"0xaTx", 0xa0000000000, 4},
+ {"3E8", 0x3000000000000000, 2},
+
+ /* Valid suffixes but not enabled. */
+ {"68k ", 68, 2, MEMPARSE_SUFFIX_M},
+ {"8MS", 8, 1, MEMPARSE_SUFFIX_K},
+ {"0xaeGis", 0xae, 4, MEMPARSE_SUFFIX_K},
+ {"0xaTx", 0xa, 3, MEMPARSE_SUFFIX_K},
+ {"3E8", 3, 1, MEMPARSE_SUFFIX_K},
+ };
+ unsigned int i;
+
+ for_each_test(i, tests) {
+ const struct memparse_test_ok *t = &tests[i];
+ unsigned long long tmp;
+ char *retptr;
+ int ret;
+ enum memparse_suffix suffixes = MEMPARSE_TEST_SUFFIX;
+
+ if (t->suffixes)
+ suffixes = t->suffixes;
+
+ ret = memparse_safe(t->str, suffixes, &tmp, &retptr);
+ if (ret != 0) {
+ WARN(1, "str '%s', expected ret 0 got %d\n", t->str, ret);
+ continue;
+ }
+ if (tmp != t->expected_value)
+ WARN(1, "str '%s' incorrect result, expected %llu got %llu",
+ t->str, t->expected_value, tmp);
+ if (retptr != t->str + t->retptr_off)
+ WARN(1, "str '%s' incorrect endptr, expected %u got %zu",
+ t->str, t->retptr_off, retptr - t->str);
+ }
+}
static void __init test_kstrtoll_fail(void)
{
static DEFINE_TEST_FAIL(test_ll_fail) = {
@@ -710,6 +951,9 @@ static int __init test_kstrtox_init(void)
test_kstrtoll_ok();
test_kstrtoll_fail();
+ test_memparse_safe_ok();
+ test_memparse_safe_fail();
+
test_kstrtou64_ok();
test_kstrtou64_fail();
test_kstrtos64_ok();
--
2.43.0
The new helper has better error report and correct overflow detection,
furthermore the old @retptr behavior is also kept, thus there should be
no behavior change.
Signed-off-by: Qu Wenruo <[email protected]>
Reviewed-by: David Disseldorp <[email protected]>
---
fs/btrfs/ioctl.c | 6 +++++-
fs/btrfs/super.c | 9 ++++++++-
fs/btrfs/sysfs.c | 14 +++++++++++---
3 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 4e50b62db2a8..cb63f50a2078 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1175,7 +1175,11 @@ static noinline int btrfs_ioctl_resize(struct file *file,
mod = 1;
sizestr++;
}
- new_size = memparse(sizestr, &retptr);
+
+ ret = memparse_safe(sizestr, MEMPARSE_SUFFIXES_DEFAULT,
+ &new_size, &retptr);
+ if (ret < 0)
+ goto out_finish;
if (*retptr != '\0' || new_size == 0) {
ret = -EINVAL;
goto out_finish;
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 3a677b808f0f..0f29fd692e0f 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -263,6 +263,8 @@ static int btrfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
{
struct btrfs_fs_context *ctx = fc->fs_private;
struct fs_parse_result result;
+ /* Only for memparse_safe() caller. */
+ int ret;
int opt;
opt = fs_parse(fc, btrfs_fs_parameters, param, &result);
@@ -400,7 +402,12 @@ static int btrfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
ctx->thread_pool_size = result.uint_32;
break;
case Opt_max_inline:
- ctx->max_inline = memparse(param->string, NULL);
+ ret = memparse_safe(param->string, MEMPARSE_SUFFIXES_DEFAULT,
+ &ctx->max_inline, NULL);
+ if (ret < 0) {
+ btrfs_err(NULL, "invalid string \"%s\"", param->string);
+ return ret;
+ }
break;
case Opt_acl:
if (result.negated) {
diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 84c05246ffd8..6846572496a6 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -762,6 +762,7 @@ static ssize_t btrfs_chunk_size_store(struct kobject *kobj,
struct btrfs_fs_info *fs_info = to_fs_info(get_btrfs_kobj(kobj));
char *retptr;
u64 val;
+ int ret;
if (!capable(CAP_SYS_ADMIN))
return -EPERM;
@@ -776,7 +777,10 @@ static ssize_t btrfs_chunk_size_store(struct kobject *kobj,
if (space_info->flags & BTRFS_BLOCK_GROUP_SYSTEM)
return -EPERM;
- val = memparse(buf, &retptr);
+ ret = memparse_safe(buf, MEMPARSE_SUFFIXES_DEFAULT, &val, &retptr);
+ if (ret < 0)
+ return ret;
+
/* There could be trailing '\n', also catch any typos after the value */
retptr = skip_spaces(retptr);
if (*retptr != 0 || val == 0)
@@ -1779,10 +1783,14 @@ static ssize_t btrfs_devinfo_scrub_speed_max_store(struct kobject *kobj,
{
struct btrfs_device *device = container_of(kobj, struct btrfs_device,
devid_kobj);
- char *endptr;
unsigned long long limit;
+ char *endptr;
+ int ret;
+
+ ret = memparse_safe(buf, MEMPARSE_SUFFIXES_DEFAULT, &limit, &endptr);
+ if (ret < 0)
+ return ret;
- limit = memparse(buf, &endptr);
/* There could be trailing '\n', also catch any typos after the value. */
endptr = skip_spaces(endptr);
if (*endptr != 0)
--
2.43.0
Hi,
On 1/3/24 15:27, Qu Wenruo wrote:
> [BUGS]
> Function memparse() lacks error handling:
>
> - If no valid number string at all
> In that case @retptr would just be updated and return value would be
> zero.
>
> - No overflown detection
> This applies to both the number string part, and the suffixes part.
> And since we have no way to indicate errors, we can get weird results
> like:
>
> "25E" -> 10376293541461622784 (9E)
>
> This is due to the fact that for "E" suffix, there is only 4 bits
> left, and 25 with 60 bits left shift would lead to overflow.
>
> [CAUSE]
> The root cause is already mentioned in the comments of the function, the
> usage of simple_strtoull() is the source of evil.
> Furthermore the function prototype is no good either, just returning an
> unsigned long long gives us no way to indicate an error.
>
> [FIX]
> Due to the prototype limits, we can not have a drop-in replacement for
> memparse().
>
> This patch can only help by introduce a new helper, memparse_safe(), and
> mark the old memparse() deprecated.
>
> The new memparse_safe() has the following improvement:
>
> - Invalid string detection
> If no number string can be detected at all, -EINVAL would be returned.
>
> - Better overflow detection
> Both the string part and the extra left shift would have overflow
> detection.
> Any overflow would result -ERANGE.
>
> - Safer default suffix selection
> The helper allows the caller to choose the suffixes that they want to
> use.
> But only "KMGTP" are recommended by default since the "E" leaves only
> 4 bits before overflow.
> For those callers really know what they are doing, they can still
> manually to include all suffixes.
>
> Due to the prototype change, callers should migrate to the new one and
> change their code and add extra error handling.
>
> Signed-off-by: Qu Wenruo <[email protected]>
> Reviewed-by: David Disseldorp <[email protected]>
> ---
> include/linux/kernel.h | 8 +++-
> include/linux/kstrtox.h | 15 +++++++
> lib/cmdline.c | 5 ++-
> lib/kstrtox.c | 96 +++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 122 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index d9ad21058eed..b1b6da60ea43 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -201,7 +201,13 @@ void do_exit(long error_code) __noreturn;
>
> extern int get_option(char **str, int *pint);
> extern char *get_options(const char *str, int nints, int *ints);
> -extern unsigned long long memparse(const char *ptr, char **retptr);
> +
> +/*
> + * DEPRECATED, lack of any kind of error handling.
> + *
> + * Use memparse_safe() from lib/kstrtox.c instead.
> + */
> +extern __deprecated unsigned long long memparse(const char *ptr, char **retptr);
> extern bool parse_option_str(const char *str, const char *option);
> extern char *next_arg(char *args, char **param, char **val);
>
> diff --git a/include/linux/kstrtox.h b/include/linux/kstrtox.h
> index 7fcf29a4e0de..53a1e059dd31 100644
> --- a/include/linux/kstrtox.h
> +++ b/include/linux/kstrtox.h
> @@ -9,6 +9,21 @@
> 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);
>
> +enum memparse_suffix {
> + MEMPARSE_SUFFIX_K = 1 << 0,
> + MEMPARSE_SUFFIX_M = 1 << 1,
> + MEMPARSE_SUFFIX_G = 1 << 2,
> + MEMPARSE_SUFFIX_T = 1 << 3,
> + MEMPARSE_SUFFIX_P = 1 << 4,
> + MEMPARSE_SUFFIX_E = 1 << 5,
> +};
> +
> +#define MEMPARSE_SUFFIXES_DEFAULT (MEMPARSE_SUFFIX_K | MEMPARSE_SUFFIX_M |\
> + MEMPARSE_SUFFIX_G | MEMPARSE_SUFFIX_T |\
> + MEMPARSE_SUFFIX_P)
> +
> +int __must_check memparse_safe(const char *s, enum memparse_suffix suffixes,
> + unsigned long long *res, char **retptr);
> 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);
>
> diff --git a/lib/cmdline.c b/lib/cmdline.c
> index 90ed997d9570..d379157de349 100644
> --- a/lib/cmdline.c
> +++ b/lib/cmdline.c
> @@ -139,12 +139,15 @@ char *get_options(const char *str, int nints, int *ints)
> EXPORT_SYMBOL(get_options);
>
> /**
> - * memparse - parse a string with mem suffixes into a number
> + * memparse - DEPRECATED, parse a string with mem suffixes into a number
> * @ptr: Where parse begins
> * @retptr: (output) Optional pointer to next char after parse completes
> *
> + * There is no way to handle errors, and no overflown detection and string
> + * sanity checks.
> * Parses a string into a number. The number stored at @ptr is
> * potentially suffixed with K, M, G, T, P, E.
> + *
Extra line is not needed.
> */
>
> unsigned long long memparse(const char *ptr, char **retptr)
> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
> index 41c9a499bbf3..a1e4279f52b3 100644
> --- a/lib/kstrtox.c
> +++ b/lib/kstrtox.c
> @@ -113,6 +113,102 @@ static int _kstrtoull(const char *s, unsigned int base, unsigned long long *res)
> return 0;
> }
>
> +/**
> + * memparse_safe - convert a string to an unsigned long long, safer version of
> + * memparse()
> + *
> + * @s: The start of the string. Must be null-terminated.
> + * The base would be determined automatically, if it starts with
> + * "0x" the base would be 16, if it starts with "0" the base
> + * would be 8, otherwise the base would be 10.
> + * After a valid number string, there can be at most one
> + * case-insensive suffix character, specified by the @suffixes
case-insensitive
> + * parameter.
> + *
> + * @suffixes: The suffixes which should be parsed. Use logical ORed
> + * memparse_suffix enum to indicate the supported suffixes.
> + * The suffixes are case-insensive, all 2 ^ 10 based.
case-insensitive
> + * Supported ones are "KMGPTE".
> + * NOTE: If one suffix out of the supported one is hit, it would
ones
> + * end the parse normally, with @retptr pointed to the unsupported
> + * suffix.
Could you explain (or give an example) of "to the unsupported suffix"?
This isn't clear IMO.
> + *
> + * @res: Where to write the result.
> + *
> + * @retptr: (output) Optional pointer to the next char after parse completes.
> + *
> + * Return 0 if any valid numberic string can be parsed, and @retptr updated.
> + * Return -INVALID if no valid number string can be found.
> + * Return -ERANGE if the number overflows.
> + * For minus return values, @retptr would not be updated.
* Returns:
* * %0 if any valid numeric string can be parsed, and @retptr is updated.
* * %-EINVAL if no valid number string can be found.
* * %-ERANGE if the number overflows.
* * For negative return values, @retptr is not updated.
For *ALL* of the comments, I request/suggest that you change the "would be" or
"would not be" to "is" or "is not" or whatever present tense words make the
most sense.
> + */
> +noinline int memparse_safe(const char *s, enum memparse_suffix suffixes,
> + unsigned long long *res, char **retptr)
> +{
> + unsigned long long value;
> + unsigned int rv;
> + int shift = 0;
> + int base = 0;
> +
> + s = _parse_integer_fixup_radix(s, &base);
> + rv = _parse_integer(s, base, &value);
> + if (rv & KSTRTOX_OVERFLOW)
> + return -ERANGE;
> + if (rv == 0)
> + return -EINVAL;
> +
> + s += rv;
> + switch (*s) {
> + case 'K':
> + case 'k':
> + if (!(suffixes & MEMPARSE_SUFFIX_K))
> + break;
> + shift = 10;
> + break;
> + case 'M':
> + case 'm':
> + if (!(suffixes & MEMPARSE_SUFFIX_M))
> + break;
> + shift = 20;
> + break;
> + case 'G':
> + case 'g':
> + if (!(suffixes & MEMPARSE_SUFFIX_G))
> + break;
> + shift = 30;
> + break;
> + case 'T':
> + case 't':
> + if (!(suffixes & MEMPARSE_SUFFIX_T))
> + break;
> + shift = 40;
> + break;
> + case 'P':
> + case 'p':
> + if (!(suffixes & MEMPARSE_SUFFIX_P))
> + break;
> + shift = 50;
> + break;
> + case 'E':
> + case 'e':
> + if (!(suffixes & MEMPARSE_SUFFIX_E))
> + break;
> + shift = 60;
> + break;
> + }
> + if (shift) {
> + s++;
> + if (value >> (64 - shift))
> + return -ERANGE;
> + value <<= shift;
> + }
> + *res = value;
> + if (retptr)
> + *retptr = (char *)s;
> + return 0;
> +}
> +EXPORT_SYMBOL(memparse_safe);
> +
> /**
> * kstrtoull - convert a string to an unsigned long long
> * @s: The start of the string. The string must be null-terminated, and may also
Thanks.
--
#Randy
On 2024/1/4 17:00, Randy Dunlap wrote:
> Hi,
>
[...]
>> + * parameter.
>> + *
>> + * @suffixes: The suffixes which should be parsed. Use logical ORed
>> + * memparse_suffix enum to indicate the supported suffixes.
>> + * The suffixes are case-insensive, all 2 ^ 10 based.
>
> case-insensitive
>
>> + * Supported ones are "KMGPTE".
>> + * NOTE: If one suffix out of the supported one is hit, it would
>
> ones
>
>> + * end the parse normally, with @retptr pointed to the unsupported
>> + * suffix.
>
> Could you explain (or give an example) of "to the unsupported suffix"?
> This isn't clear IMO.
Oh, my bad, that sentence itself is not correct.
What I really want to say is:
If one suffix (one of "KMGPTE") is hit but that suffix is not
specified in the @suffxies parameter, it would end the parse normally,
with @retptr pointed to the (unsupported) suffix.
The example would be the "68k " case in the ok cases in the next patch.
We have two different cases for the same "68k" string, with different
@suffixes and different results:
"68k ", KMGPTE -> 68 * 1024, @retptr at " ".
"68k ", M -> 68, @retptr at 'k'.
I don't have a better expression here unfortunately, maybe the special
case is not even worthy explaining?
>
>> + *
>> + * @res: Where to write the result.
>> + *
>> + * @retptr: (output) Optional pointer to the next char after parse completes.
>> + *
>> + * Return 0 if any valid numberic string can be parsed, and @retptr updated.
>> + * Return -INVALID if no valid number string can be found.
>> + * Return -ERANGE if the number overflows.
>> + * For minus return values, @retptr would not be updated.
>
> * Returns:
> * * %0 if any valid numeric string can be parsed, and @retptr is updated.
> * * %-EINVAL if no valid number string can be found.
> * * %-ERANGE if the number overflows.
> * * For negative return values, @retptr is not updated.
>
>
> For *ALL* of the comments, I request/suggest that you change the "would be" or
> "would not be" to "is" or "is not" or whatever present tense words make the
> most sense.
No problem.
Thanks,
Qu
>
>
>> + */
>> +noinline int memparse_safe(const char *s, enum memparse_suffix suffixes,
>> + unsigned long long *res, char **retptr)
>> +{
>> + unsigned long long value;
>> + unsigned int rv;
>> + int shift = 0;
>> + int base = 0;
>> +
>> + s = _parse_integer_fixup_radix(s, &base);
>> + rv = _parse_integer(s, base, &value);
>> + if (rv & KSTRTOX_OVERFLOW)
>> + return -ERANGE;
>> + if (rv == 0)
>> + return -EINVAL;
>> +
>> + s += rv;
>> + switch (*s) {
>> + case 'K':
>> + case 'k':
>> + if (!(suffixes & MEMPARSE_SUFFIX_K))
>> + break;
>> + shift = 10;
>> + break;
>> + case 'M':
>> + case 'm':
>> + if (!(suffixes & MEMPARSE_SUFFIX_M))
>> + break;
>> + shift = 20;
>> + break;
>> + case 'G':
>> + case 'g':
>> + if (!(suffixes & MEMPARSE_SUFFIX_G))
>> + break;
>> + shift = 30;
>> + break;
>> + case 'T':
>> + case 't':
>> + if (!(suffixes & MEMPARSE_SUFFIX_T))
>> + break;
>> + shift = 40;
>> + break;
>> + case 'P':
>> + case 'p':
>> + if (!(suffixes & MEMPARSE_SUFFIX_P))
>> + break;
>> + shift = 50;
>> + break;
>> + case 'E':
>> + case 'e':
>> + if (!(suffixes & MEMPARSE_SUFFIX_E))
>> + break;
>> + shift = 60;
>> + break;
>> + }
>> + if (shift) {
>> + s++;
>> + if (value >> (64 - shift))
>> + return -ERANGE;
>> + value <<= shift;
>> + }
>> + *res = value;
>> + if (retptr)
>> + *retptr = (char *)s;
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(memparse_safe);
>> +
>> /**
>> * kstrtoull - convert a string to an unsigned long long
>> * @s: The start of the string. The string must be null-terminated, and may also
>
> Thanks.
On 2024/1/4 17:20, Randy Dunlap wrote:
>
>
> On 1/3/24 22:42, Qu Wenruo wrote:
>>
>>
>> On 2024/1/4 17:00, Randy Dunlap wrote:
>>> Hi,
>>>
>> [...]
>>>> + * parameter.
>>>> + *
>>>> + * @suffixes: The suffixes which should be parsed. Use logical ORed
>>>> + * memparse_suffix enum to indicate the supported suffixes.
>>>> + * The suffixes are case-insensive, all 2 ^ 10 based.
>>>
>>> case-insensitive
>>>
>>>> + * Supported ones are "KMGPTE".
>>>> + * NOTE: If one suffix out of the supported one is hit, it would
>>>
>>> ones
>>>
>>>> + * end the parse normally, with @retptr pointed to the unsupported
>>>> + * suffix.
>>>
>>> Could you explain (or give an example) of "to the unsupported suffix"?
>>> This isn't clear IMO.
>>
>> Oh, my bad, that sentence itself is not correct.
>>
>> What I really want to say is:
>>
>> If one suffix (one of "KMGPTE") is hit but that suffix is not
>> specified in the @suffxies parameter, it would end the parse normally,
>> with @retptr pointed to the (unsupported) suffix.
>
> (corrected ^^^)
>
>> The example would be the "68k " case in the ok cases in the next patch.
>> We have two different cases for the same "68k" string, with different
>> @suffixes and different results:
>>
>> "68k ", KMGPTE -> 68 * 1024, @retptr at " ".
>> "68k ", M -> 68, @retptr at 'k'.
>>
>> I don't have a better expression here unfortunately, maybe the special
>> case is not even worthy explaining?
>>
>
> No, the corrected paragraph above is good. (s/would end/ends/)
>
> Except for one thing: the examples here terminate on a space character,
> but the function kernel-doc says "null-terminated":
Oh, all the strings used here and in the test cases are all "\0" ended.
Just to make it a little more readable (and easier to type), the tailing
"\0" is not explicitly showed.
That extra ' ' in the "68k \0" case is just to make sure the @retptr is
properly updated.
For all the other existing ok cases for memparse_safe() (those without
extra tailing chars), the @retptr points to the tailing NUL '\0'.
Thanks,
Qu
>
> +/**
> + * memparse_safe - convert a string to an unsigned long long, safer version of
> + * memparse()
> + *
> + * @s: The start of the string. Must be null-terminated.
>
>
>
>
>>>
>>>> + *
>>>> + * @res: Where to write the result.
>>>> + *
>>>> + * @retptr: (output) Optional pointer to the next char after parse completes.
>>>> + *
>>>> + * Return 0 if any valid numberic string can be parsed, and @retptr updated.
>>>> + * Return -INVALID if no valid number string can be found.
>>>> + * Return -ERANGE if the number overflows.
>>>> + * For minus return values, @retptr would not be updated.
>>>
>>> * Returns:
>>> * * %0 if any valid numeric string can be parsed, and @retptr is updated.
>>> * * %-EINVAL if no valid number string can be found.
>>> * * %-ERANGE if the number overflows.
>>> * * For negative return values, @retptr is not updated.
>>>
>>>
>>> For *ALL* of the comments, I request/suggest that you change the "would be" or
>>> "would not be" to "is" or "is not" or whatever present tense words make the
>>> most sense.
>>
>> No problem.
>
>
On 1/3/24 22:42, Qu Wenruo wrote:
>
>
> On 2024/1/4 17:00, Randy Dunlap wrote:
>> Hi,
>>
> [...]
>>> + * parameter.
>>> + *
>>> + * @suffixes: The suffixes which should be parsed. Use logical ORed
>>> + * memparse_suffix enum to indicate the supported suffixes.
>>> + * The suffixes are case-insensive, all 2 ^ 10 based.
>>
>> case-insensitive
>>
>>> + * Supported ones are "KMGPTE".
>>> + * NOTE: If one suffix out of the supported one is hit, it would
>>
>> ones
>>
>>> + * end the parse normally, with @retptr pointed to the unsupported
>>> + * suffix.
>>
>> Could you explain (or give an example) of "to the unsupported suffix"?
>> This isn't clear IMO.
>
> Oh, my bad, that sentence itself is not correct.
>
> What I really want to say is:
>
> If one suffix (one of "KMGPTE") is hit but that suffix is not
> specified in the @suffxies parameter, it would end the parse normally,
> with @retptr pointed to the (unsupported) suffix.
(corrected ^^^)
> The example would be the "68k " case in the ok cases in the next patch.
> We have two different cases for the same "68k" string, with different
> @suffixes and different results:
>
> "68k ", KMGPTE -> 68 * 1024, @retptr at " ".
> "68k ", M -> 68, @retptr at 'k'.
>
> I don't have a better expression here unfortunately, maybe the special
> case is not even worthy explaining?
>
No, the corrected paragraph above is good. (s/would end/ends/)
Except for one thing: the examples here terminate on a space character,
but the function kernel-doc says "null-terminated":
+/**
+ * memparse_safe - convert a string to an unsigned long long, safer version of
+ * memparse()
+ *
+ * @s: The start of the string. Must be null-terminated.
>>
>>> + *
>>> + * @res: Where to write the result.
>>> + *
>>> + * @retptr: (output) Optional pointer to the next char after parse completes.
>>> + *
>>> + * Return 0 if any valid numberic string can be parsed, and @retptr updated.
>>> + * Return -INVALID if no valid number string can be found.
>>> + * Return -ERANGE if the number overflows.
>>> + * For minus return values, @retptr would not be updated.
>>
>> * Returns:
>> * * %0 if any valid numeric string can be parsed, and @retptr is updated.
>> * * %-EINVAL if no valid number string can be found.
>> * * %-ERANGE if the number overflows.
>> * * For negative return values, @retptr is not updated.
>>
>>
>> For *ALL* of the comments, I request/suggest that you change the "would be" or
>> "would not be" to "is" or "is not" or whatever present tense words make the
>> most sense.
>
> No problem.
--
#Randy
On Thu, Jan 04, 2024 at 09:57:47AM +1030, Qu Wenruo wrote:
> [CHANGELOG]
> v3:
> - Fix the 32bit pointer pattern in the test case
> The old pointer pattern for 32 bit systems is in fact 40 bits,
> which would still lead to sparse warning.
> The newer pattern is using UINTPTR_MAX to trim the pattern, then
> converted to a pointer, which should not cause any trimmed bits and
> make sparse happy.
Having test cases is quite good, thanks!
But as I understood what Alexey wanted, is not using the kstrtox files for this.
You can introduce it in the cmdline.c, correct? Just include local "kstrtox.h".
I'm on leave till end of the month, I'll look at this later.
--
With Best Regards,
Andy Shevchenko
On 2024/1/7 01:04, Andy Shevchenko wrote:
> On Thu, Jan 04, 2024 at 09:57:47AM +1030, Qu Wenruo wrote:
>> [CHANGELOG]
>> v3:
>> - Fix the 32bit pointer pattern in the test case
>> The old pointer pattern for 32 bit systems is in fact 40 bits,
>> which would still lead to sparse warning.
>> The newer pattern is using UINTPTR_MAX to trim the pattern, then
>> converted to a pointer, which should not cause any trimmed bits and
>> make sparse happy.
>
> Having test cases is quite good, thanks!
> But as I understood what Alexey wanted, is not using the kstrtox files for this.
> You can introduce it in the cmdline.c, correct? Just include local "kstrtox.h".
Not really possible, all the needed parsing helpers are internal inside
kstrtox.c.
Furthermore, this also means memparse() can not be enhanced due to:
- Lack of ways to return errors
- Unable to call the parsing helpers inside cmdline.c
Thanks,
Qu
>
> I'm on leave till end of the month, I'll look at this later.
>
On Sun, Jan 07, 2024 at 07:28:27AM +1030, Qu Wenruo wrote:
> On 2024/1/7 01:04, Andy Shevchenko wrote:
> > On Thu, Jan 04, 2024 at 09:57:47AM +1030, Qu Wenruo wrote:
..
> > Having test cases is quite good, thanks!
> > But as I understood what Alexey wanted, is not using the kstrtox files for this.
> > You can introduce it in the cmdline.c, correct? Just include local "kstrtox.h".
>
> Not really possible, all the needed parsing helpers are internal inside
> kstrtox.c.
I'm not sure I follow. The functions are available to other library (built-in)
modules.
> Furthermore, this also means memparse() can not be enhanced due to:
>
> - Lack of ways to return errors
What does this mean?
> - Unable to call the parsing helpers inside cmdline.c
??? (see above)
--
With Best Regards,
Andy Shevchenko
On 2024/1/15 00:12, Andy Shevchenko wrote:
> On Sun, Jan 07, 2024 at 07:28:27AM +1030, Qu Wenruo wrote:
>> On 2024/1/7 01:04, Andy Shevchenko wrote:
>>> On Thu, Jan 04, 2024 at 09:57:47AM +1030, Qu Wenruo wrote:
>
> ...
>
>>> Having test cases is quite good, thanks!
>>> But as I understood what Alexey wanted, is not using the kstrtox files for this.
>>> You can introduce it in the cmdline.c, correct? Just include local "kstrtox.h".
>>
>> Not really possible, all the needed parsing helpers are internal inside
>> kstrtox.c.
>
> I'm not sure I follow. The functions are available to other library (built-in)
> modules.
Did I miss something?
Firstly neither _parse_integer_fixup_radix() nor _parse_integer_limit()
is exported to modules. (No EXPORT_SYMBOL() call on them).
Secondly _parse_integer_fixup_radix() and _parse_integer_limit have "_"
prefix, and is only declared in "lib/kstrtox.h", which means they are
designed only for internal usage.
If putting memparse_safe() into cmdline.c, at least we would need to
include local header "kstrtox.h", and I'm not sure if this is any better
than putting memparse_safe() into kstrtox.[ch].
Finally, I just tried putting memparse_safe() into cmdline.c, and it
failed at linkage stage, even if that offending file has no call to
memparse_safe():
ld: arch/x86/boot/compressed/kaslr.o: in function `memparse_safe':
kaslr.c:(.text+0xbb1): undefined reference to `_parse_integer_fixup_radix'
ld: kaslr.c:(.text+0xbc5): undefined reference to `_parse_integer'
ld: arch/x86/boot/compressed/vmlinux: hidden symbol `_parse_integer'
isn't defined
I can try again but I'm not sure if it's possible to move
memparse_safe() to cmdline.[ch].
>
>> Furthermore, this also means memparse() can not be enhanced due to:
>>
>> - Lack of ways to return errors
>
> What does this mean?
If you want to keep the prototype of memparse() (aka, a drop-in
enhancement), then there is no good way to indicate the errors like
overflow at all.
>
>> - Unable to call the parsing helpers inside cmdline.c
>
> ??? (see above)
>
See above.
Thanks,
Qu
On Mon, Jan 15, 2024 at 06:31:05AM +1030, Qu Wenruo wrote:
> On 2024/1/15 00:12, Andy Shevchenko wrote:
> > On Sun, Jan 07, 2024 at 07:28:27AM +1030, Qu Wenruo wrote:
> > > On 2024/1/7 01:04, Andy Shevchenko wrote:
> > > > On Thu, Jan 04, 2024 at 09:57:47AM +1030, Qu Wenruo wrote:
..
> > > > Having test cases is quite good, thanks!
> > > > But as I understood what Alexey wanted, is not using the kstrtox files for this.
> > > > You can introduce it in the cmdline.c, correct? Just include local "kstrtox.h".
> > >
> > > Not really possible, all the needed parsing helpers are internal inside
> > > kstrtox.c.
> >
> > I'm not sure I follow. The functions are available to other library (built-in)
> > modules.
>
> Did I miss something?
>
> Firstly neither _parse_integer_fixup_radix() nor _parse_integer_limit() is
> exported to modules. (No EXPORT_SYMBOL() call on them).
Should they?
> Secondly _parse_integer_fixup_radix() and _parse_integer_limit have "_"
> prefix, and is only declared in "lib/kstrtox.h", which means they are
> designed only for internal usage.
> If putting memparse_safe() into cmdline.c, at least we would need to include
> local header "kstrtox.h", and I'm not sure if this is any better than
> putting memparse_safe() into kstrtox.[ch].
Yes, this is much better.
> Finally, I just tried putting memparse_safe() into cmdline.c, and it failed
> at linkage stage, even if that offending file has no call to
> memparse_safe():
>
> ld: arch/x86/boot/compressed/kaslr.o: in function `memparse_safe':
> kaslr.c:(.text+0xbb1): undefined reference to `_parse_integer_fixup_radix'
> ld: kaslr.c:(.text+0xbc5): undefined reference to `_parse_integer'
> ld: arch/x86/boot/compressed/vmlinux: hidden symbol `_parse_integer' isn't
> defined
I don't understand. Implement memparse_safe() in the C file
(i.e. lib/cmdline.c) and if it's needed to be exported, export it.
> I can try again but I'm not sure if it's possible to move memparse_safe() to
> cmdline.[ch].
Please do. I do not see design issues with proposed approach.
..
> > > Furthermore, this also means memparse() can not be enhanced due to:
> > >
> > > - Lack of ways to return errors
> >
> > What does this mean?
>
> If you want to keep the prototype of memparse() (aka, a drop-in
> enhancement), then there is no good way to indicate the errors like overflow
> at all.
No need to replace one by another in the implementation side, having two is
okay, while moving affected code to the better one one-by-one. Then we may
remove the old one. And then, if there is a wish, we may rename it back.
> > > - Unable to call the parsing helpers inside cmdline.c
> >
> > ??? (see above)
> >
> See above.
Got it, but please try again. I do believe it's possible to avoid touching
kstrotox.c.
--
With Best Regards,
Andy Shevchenko