2023-12-23 09:59:05

by Qu Wenruo

Subject: [PATCH 0/3] kstrtox: introduce memparse_safe()

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

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 (3):
kstrtox: introduce a safer version of memparse()
kstrtox: add unit tests for memparse_safe()
btrfs: migrate to the newer memparse_safe() helper

fs/btrfs/ioctl.c | 8 +-
fs/btrfs/super.c | 8 ++
fs/btrfs/sysfs.c | 14 ++-
include/linux/kernel.h | 8 +-
include/linux/kstrtox.h | 15 +++
lib/cmdline.c | 5 +-
lib/kstrtox.c | 96 ++++++++++++++++++
lib/test-kstrtox.c | 217 ++++++++++++++++++++++++++++++++++++++++
8 files changed, 364 insertions(+), 7 deletions(-)

--
2.43.0

2023-12-23 09:59:08

by Qu Wenruo

Subject: [PATCH 1/3] kstrtox: introduce a safer version of memparse()

[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]>
---
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 d586e6af5e5a..38c772097301 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

2023-12-23 09:59:27

by Qu Wenruo

Subject: [PATCH 2/3] kstrtox: add unit tests for memparse_safe()

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.

* bad cases without any number but stray suffix
Should be rejected with -EINVAL

Signed-off-by: Qu Wenruo <[email protected]>
---
lib/test-kstrtox.c | 217 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 217 insertions(+)

diff --git a/lib/test-kstrtox.c b/lib/test-kstrtox.c
index f355f67169b6..094beab3dfac 100644
--- a/lib/test-kstrtox.c
+++ b/lib/test-kstrtox.c
@@ -268,6 +268,219 @@ 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)
+
+/* 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},
+
+ /*
+ * 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 = (char *)ULL_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 != (char *)ULL_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;
+ };
+ 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},
+
+ /*
+ * Finally one suffix then tailing chars, to test the @retptr
+ * behavior.
+ */
+ {"68k ", 69632, 3},
+ {"8MS", 8388608, 2},
+ {"0xaeGis", 0x2b80000000, 5},
+ {"0xaTx", 0xa0000000000, 4},
+ {"3E8", 0x3000000000000000, 2},
+ };
+ unsigned int i;
+
+ for_each_test(i, tests) {
+ const struct memparse_test_ok *t = &tests[i];
+ unsigned long long tmp;
+ char *retptr;
+ int ret;
+
+ ret = memparse_safe(t->str, MEMPARSE_TEST_SUFFIX, &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 +923,10 @@ 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

2023-12-23 09:59:46

by Qu Wenruo

Subject: [PATCH 3/3] btrfs: migrate to the newer memparse_safe() helper

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]>
---
fs/btrfs/ioctl.c | 8 ++++++--
fs/btrfs/super.c | 8 ++++++++
fs/btrfs/sysfs.c | 14 +++++++++++---
3 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 4e50b62db2a8..8bfd4b4ccf02 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1175,8 +1175,12 @@ static noinline int btrfs_ioctl_resize(struct file *file,
mod = 1;
sizestr++;
}
- new_size = memparse(sizestr, &retptr);
- if (*retptr != '\0' || new_size == 0) {
+
+ ret = memparse_safe(sizestr, MEMPARSE_SUFFIXES_DEFAULT,
+ &new_size, &retptr);
+ if (ret < 0)
+ goto out_finish;
+ if (*retptr != '\0') {
ret = -EINVAL;
goto out_finish;
}
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 3a677b808f0f..2bb6ea525e89 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -400,6 +400,14 @@ 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:
+ int ret;
+
+ 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;
+ }
ctx->max_inline = memparse(param->string, NULL);
break;
case Opt_acl:
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

2023-12-26 04:55:18

by kernel test robot

Subject: Re: [PATCH 3/3] btrfs: migrate to the newer memparse_safe() helper

Hi Qu,

kernel test robot noticed the following build warnings:

[auto build test WARNING on kdave/for-next]
[also build test WARNING on next-20231222]
[cannot apply to akpm-mm/mm-nonmm-unstable akpm-mm/mm-everything linus/master v6.7-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Qu-Wenruo/kstrtox-introduce-a-safer-version-of-memparse/20231225-151921
base: https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
patch link: https://lore.kernel.org/r/6dfa53ded887caa2269c1beeaedcff086342339a.1703324146.git.wqu%40suse.com
patch subject: [PATCH 3/3] btrfs: migrate to the newer memparse_safe() helper
config: powerpc-allmodconfig (https://download.01.org/0day-ci/archive/20231226/[email protected]/config)
compiler: clang version 18.0.0git (https://github.com/llvm/llvm-project d3ef86708241a3bee902615c190dead1638c4e09)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231226/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All warnings (new ones prefixed by >>):

>> fs/btrfs/super.c:403:3: warning: label followed by a declaration is a C23 extension [-Wc23-extensions]
403 | int ret;
| ^
1 warning generated.


vim +403 fs/btrfs/super.c

261
262 static int btrfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
263 {
264 struct btrfs_fs_context *ctx = fc->fs_private;
265 struct fs_parse_result result;
266 int opt;
267
268 opt = fs_parse(fc, btrfs_fs_parameters, param, &result);
269 if (opt < 0)
270 return opt;
271
272 switch (opt) {
273 case Opt_degraded:
274 btrfs_set_opt(ctx->mount_opt, DEGRADED);
275 break;
276 case Opt_subvol_empty:
277 /*
278 * This exists because we used to allow it on accident, so we're
279 * keeping it to maintain ABI. See 37becec95ac3 ("Btrfs: allow
280 * empty subvol= again").
281 */
282 break;
283 case Opt_subvol:
284 kfree(ctx->subvol_name);
285 ctx->subvol_name = kstrdup(param->string, GFP_KERNEL);
286 if (!ctx->subvol_name)
287 return -ENOMEM;
288 break;
289 case Opt_subvolid:
290 ctx->subvol_objectid = result.uint_64;
291
292 /* subvolid=0 means give me the original fs_tree. */
293 if (!ctx->subvol_objectid)
294 ctx->subvol_objectid = BTRFS_FS_TREE_OBJECTID;
295 break;
296 case Opt_device: {
297 struct btrfs_device *device;
298 blk_mode_t mode = sb_open_mode(fc->sb_flags);
299
300 mutex_lock(&uuid_mutex);
301 device = btrfs_scan_one_device(param->string, mode, false);
302 mutex_unlock(&uuid_mutex);
303 if (IS_ERR(device))
304 return PTR_ERR(device);
305 break;
306 }
307 case Opt_datasum:
308 if (result.negated) {
309 btrfs_set_opt(ctx->mount_opt, NODATASUM);
310 } else {
311 btrfs_clear_opt(ctx->mount_opt, NODATACOW);
312 btrfs_clear_opt(ctx->mount_opt, NODATASUM);
313 }
314 break;
315 case Opt_datacow:
316 if (result.negated) {
317 btrfs_clear_opt(ctx->mount_opt, COMPRESS);
318 btrfs_clear_opt(ctx->mount_opt, FORCE_COMPRESS);
319 btrfs_set_opt(ctx->mount_opt, NODATACOW);
320 btrfs_set_opt(ctx->mount_opt, NODATASUM);
321 } else {
322 btrfs_clear_opt(ctx->mount_opt, NODATACOW);
323 }
324 break;
325 case Opt_compress_force:
326 case Opt_compress_force_type:
327 btrfs_set_opt(ctx->mount_opt, FORCE_COMPRESS);
328 fallthrough;
329 case Opt_compress:
330 case Opt_compress_type:
331 if (opt == Opt_compress || opt == Opt_compress_force) {
332 ctx->compress_type = BTRFS_COMPRESS_ZLIB;
333 ctx->compress_level = BTRFS_ZLIB_DEFAULT_LEVEL;
334 btrfs_set_opt(ctx->mount_opt, COMPRESS);
335 btrfs_clear_opt(ctx->mount_opt, NODATACOW);
336 btrfs_clear_opt(ctx->mount_opt, NODATASUM);
337 } else if (strncmp(param->string, "zlib", 4) == 0) {
338 ctx->compress_type = BTRFS_COMPRESS_ZLIB;
339 ctx->compress_level =
340 btrfs_compress_str2level(BTRFS_COMPRESS_ZLIB,
341 param->string + 4);
342 btrfs_set_opt(ctx->mount_opt, COMPRESS);
343 btrfs_clear_opt(ctx->mount_opt, NODATACOW);
344 btrfs_clear_opt(ctx->mount_opt, NODATASUM);
345 } else if (strncmp(param->string, "lzo", 3) == 0) {
346 ctx->compress_type = BTRFS_COMPRESS_LZO;
347 ctx->compress_level = 0;
348 btrfs_set_opt(ctx->mount_opt, COMPRESS);
349 btrfs_clear_opt(ctx->mount_opt, NODATACOW);
350 btrfs_clear_opt(ctx->mount_opt, NODATASUM);
351 } else if (strncmp(param->string, "zstd", 4) == 0) {
352 ctx->compress_type = BTRFS_COMPRESS_ZSTD;
353 ctx->compress_level =
354 btrfs_compress_str2level(BTRFS_COMPRESS_ZSTD,
355 param->string + 4);
356 btrfs_set_opt(ctx->mount_opt, COMPRESS);
357 btrfs_clear_opt(ctx->mount_opt, NODATACOW);
358 btrfs_clear_opt(ctx->mount_opt, NODATASUM);
359 } else if (strncmp(param->string, "no", 2) == 0) {
360 ctx->compress_level = 0;
361 ctx->compress_type = 0;
362 btrfs_clear_opt(ctx->mount_opt, COMPRESS);
363 btrfs_clear_opt(ctx->mount_opt, FORCE_COMPRESS);
364 } else {
365 btrfs_err(NULL, "unrecognized compression value %s",
366 param->string);
367 return -EINVAL;
368 }
369 break;
370 case Opt_ssd:
371 if (result.negated) {
372 btrfs_set_opt(ctx->mount_opt, NOSSD);
373 btrfs_clear_opt(ctx->mount_opt, SSD);
374 btrfs_clear_opt(ctx->mount_opt, SSD_SPREAD);
375 } else {
376 btrfs_set_opt(ctx->mount_opt, SSD);
377 btrfs_clear_opt(ctx->mount_opt, NOSSD);
378 }
379 break;
380 case Opt_ssd_spread:
381 if (result.negated) {
382 btrfs_clear_opt(ctx->mount_opt, SSD_SPREAD);
383 } else {
384 btrfs_set_opt(ctx->mount_opt, SSD);
385 btrfs_set_opt(ctx->mount_opt, SSD_SPREAD);
386 btrfs_clear_opt(ctx->mount_opt, NOSSD);
387 }
388 break;
389 case Opt_barrier:
390 if (result.negated)
391 btrfs_set_opt(ctx->mount_opt, NOBARRIER);
392 else
393 btrfs_clear_opt(ctx->mount_opt, NOBARRIER);
394 break;
395 case Opt_thread_pool:
396 if (result.uint_32 == 0) {
397 btrfs_err(NULL, "invalid value 0 for thread_pool");
398 return -EINVAL;
399 }
400 ctx->thread_pool_size = result.uint_32;
401 break;
402 case Opt_max_inline:
> 403 int ret;
404
405 ret = memparse_safe(param->string, MEMPARSE_SUFFIXES_DEFAULT,
406 &ctx->max_inline, NULL);
407 if (ret < 0) {
408 btrfs_err(NULL, "invalid string \"%s\"", param->string);
409 return ret;
410 }
411 ctx->max_inline = memparse(param->string, NULL);
412 break;
413 case Opt_acl:
414 if (result.negated) {
415 fc->sb_flags &= ~SB_POSIXACL;
416 } else {
417 #ifdef CONFIG_BTRFS_FS_POSIX_ACL
418 fc->sb_flags |= SB_POSIXACL;
419 #else
420 btrfs_err(NULL, "support for ACL not compiled in");
421 return -EINVAL;
422 #endif
423 }
424 /*
425 * VFS limits the ability to toggle ACL on and off via remount,
426 * despite every file system allowing this. This seems to be
427 * an oversight since we all do, but it'll fail if we're
428 * remounting. So don't set the mask here, we'll check it in
429 * btrfs_reconfigure and do the toggling ourselves.
430 */
431 if (fc->purpose != FS_CONTEXT_FOR_RECONFIGURE)
432 fc->sb_flags_mask |= SB_POSIXACL;
433 break;
434 case Opt_treelog:
435 if (result.negated)
436 btrfs_set_opt(ctx->mount_opt, NOTREELOG);
437 else
438 btrfs_clear_opt(ctx->mount_opt, NOTREELOG);
439 break;
440 case Opt_nologreplay:
441 btrfs_warn(NULL,
442 "'nologreplay' is deprecated, use 'rescue=nologreplay' instead");
443 btrfs_set_opt(ctx->mount_opt, NOLOGREPLAY);
444 break;
445 case Opt_flushoncommit:
446 if (result.negated)
447 btrfs_clear_opt(ctx->mount_opt, FLUSHONCOMMIT);
448 else
449 btrfs_set_opt(ctx->mount_opt, FLUSHONCOMMIT);
450 break;
451 case Opt_ratio:
452 ctx->metadata_ratio = result.uint_32;
453 break;
454 case Opt_discard:
455 if (result.negated) {
456 btrfs_clear_opt(ctx->mount_opt, DISCARD_SYNC);
457 btrfs_clear_opt(ctx->mount_opt, DISCARD_ASYNC);
458 btrfs_set_opt(ctx->mount_opt, NODISCARD);
459 } else {
460 btrfs_set_opt(ctx->mount_opt, DISCARD_SYNC);
461 btrfs_clear_opt(ctx->mount_opt, DISCARD_ASYNC);
462 }
463 break;
464 case Opt_discard_mode:
465 switch (result.uint_32) {
466 case Opt_discard_sync:
467 btrfs_clear_opt(ctx->mount_opt, DISCARD_ASYNC);
468 btrfs_set_opt(ctx->mount_opt, DISCARD_SYNC);
469 break;
470 case Opt_discard_async:
471 btrfs_clear_opt(ctx->mount_opt, DISCARD_SYNC);
472 btrfs_set_opt(ctx->mount_opt, DISCARD_ASYNC);
473 break;
474 default:
475 btrfs_err(NULL, "unrecognized discard mode value %s",
476 param->key);
477 return -EINVAL;
478 }
479 btrfs_clear_opt(ctx->mount_opt, NODISCARD);
480 break;
481 case Opt_space_cache:
482 if (result.negated) {
483 btrfs_set_opt(ctx->mount_opt, NOSPACECACHE);
484 btrfs_clear_opt(ctx->mount_opt, SPACE_CACHE);
485 btrfs_clear_opt(ctx->mount_opt, FREE_SPACE_TREE);
486 } else {
487 btrfs_clear_opt(ctx->mount_opt, FREE_SPACE_TREE);
488 btrfs_set_opt(ctx->mount_opt, SPACE_CACHE);
489 }
490 break;
491 case Opt_space_cache_version:
492 switch (result.uint_32) {
493 case Opt_space_cache_v1:
494 btrfs_set_opt(ctx->mount_opt, SPACE_CACHE);
495 btrfs_clear_opt(ctx->mount_opt, FREE_SPACE_TREE);
496 break;
497 case Opt_space_cache_v2:
498 btrfs_clear_opt(ctx->mount_opt, SPACE_CACHE);
499 btrfs_set_opt(ctx->mount_opt, FREE_SPACE_TREE);
500 break;
501 default:
502 btrfs_err(NULL, "unrecognized space_cache value %s",
503 param->key);
504 return -EINVAL;
505 }
506 break;
507 case Opt_rescan_uuid_tree:
508 btrfs_set_opt(ctx->mount_opt, RESCAN_UUID_TREE);
509 break;
510 case Opt_clear_cache:
511 btrfs_set_opt(ctx->mount_opt, CLEAR_CACHE);
512 break;
513 case Opt_user_subvol_rm_allowed:
514 btrfs_set_opt(ctx->mount_opt, USER_SUBVOL_RM_ALLOWED);
515 break;
516 case Opt_enospc_debug:
517 if (result.negated)
518 btrfs_clear_opt(ctx->mount_opt, ENOSPC_DEBUG);
519 else
520 btrfs_set_opt(ctx->mount_opt, ENOSPC_DEBUG);
521 break;
522 case Opt_defrag:
523 if (result.negated)
524 btrfs_clear_opt(ctx->mount_opt, AUTO_DEFRAG);
525 else
526 btrfs_set_opt(ctx->mount_opt, AUTO_DEFRAG);
527 break;
528 case Opt_usebackuproot:
529 btrfs_warn(NULL,
530 "'usebackuproot' is deprecated, use 'rescue=usebackuproot' instead");
531 btrfs_set_opt(ctx->mount_opt, USEBACKUPROOT);
532
533 /* If we're loading the backup roots we can't trust the space cache. */
534 btrfs_set_opt(ctx->mount_opt, CLEAR_CACHE);
535 break;
536 case Opt_skip_balance:
537 btrfs_set_opt(ctx->mount_opt, SKIP_BALANCE);
538 break;
539 case Opt_fatal_errors:
540 switch (result.uint_32) {
541 case Opt_fatal_errors_panic:
542 btrfs_set_opt(ctx->mount_opt, PANIC_ON_FATAL_ERROR);
543 break;
544 case Opt_fatal_errors_bug:
545 btrfs_clear_opt(ctx->mount_opt, PANIC_ON_FATAL_ERROR);
546 break;
547 default:
548 btrfs_err(NULL, "unrecognized fatal_errors value %s",
549 param->key);
550 return -EINVAL;
551 }
552 break;
553 case Opt_commit_interval:
554 ctx->commit_interval = result.uint_32;
555 if (ctx->commit_interval == 0)
556 ctx->commit_interval = BTRFS_DEFAULT_COMMIT_INTERVAL;
557 break;
558 case Opt_rescue:
559 switch (result.uint_32) {
560 case Opt_rescue_usebackuproot:
561 btrfs_set_opt(ctx->mount_opt, USEBACKUPROOT);
562 break;
563 case Opt_rescue_nologreplay:
564 btrfs_set_opt(ctx->mount_opt, NOLOGREPLAY);
565 break;
566 case Opt_rescue_ignorebadroots:
567 btrfs_set_opt(ctx->mount_opt, IGNOREBADROOTS);
568 break;
569 case Opt_rescue_ignoredatacsums:
570 btrfs_set_opt(ctx->mount_opt, IGNOREDATACSUMS);
571 break;
572 case Opt_rescue_parameter_all:
573 btrfs_set_opt(ctx->mount_opt, IGNOREDATACSUMS);
574 btrfs_set_opt(ctx->mount_opt, IGNOREBADROOTS);
575 btrfs_set_opt(ctx->mount_opt, NOLOGREPLAY);
576 break;
577 default:
578 btrfs_info(NULL, "unrecognized rescue option '%s'",
579 param->key);
580 return -EINVAL;
581 }
582 break;
583 #ifdef CONFIG_BTRFS_DEBUG
584 case Opt_fragment:
585 switch (result.uint_32) {
586 case Opt_fragment_parameter_all:
587 btrfs_set_opt(ctx->mount_opt, FRAGMENT_DATA);
588 btrfs_set_opt(ctx->mount_opt, FRAGMENT_METADATA);
589 break;
590 case Opt_fragment_parameter_metadata:
591 btrfs_set_opt(ctx->mount_opt, FRAGMENT_METADATA);
592 break;
593 case Opt_fragment_parameter_data:
594 btrfs_set_opt(ctx->mount_opt, FRAGMENT_DATA);
595 break;
596 default:
597 btrfs_info(NULL, "unrecognized fragment option '%s'",
598 param->key);
599 return -EINVAL;
600 }
601 break;
602 #endif
603 #ifdef CONFIG_BTRFS_FS_REF_VERIFY
604 case Opt_ref_verify:
605 btrfs_set_opt(ctx->mount_opt, REF_VERIFY);
606 break;
607 #endif
608 default:
609 btrfs_err(NULL, "unrecognized mount option '%s'", param->key);
610 return -EINVAL;
611 }
612
613 return 0;
614 }
615

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

2023-12-26 06:07:04

by kernel test robot

Subject: Re: [PATCH 3/3] btrfs: migrate to the newer memparse_safe() helper

Hi Qu,

kernel test robot noticed the following build errors:

[auto build test ERROR on kdave/for-next]
[also build test ERROR on next-20231222]
[cannot apply to akpm-mm/mm-nonmm-unstable akpm-mm/mm-everything linus/master v6.7-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Qu-Wenruo/kstrtox-introduce-a-safer-version-of-memparse/20231225-151921
base: https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
patch link: https://lore.kernel.org/r/6dfa53ded887caa2269c1beeaedcff086342339a.1703324146.git.wqu%40suse.com
patch subject: [PATCH 3/3] btrfs: migrate to the newer memparse_safe() helper
config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20231226/[email protected]/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231226/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All errors (new ones prefixed by >>):

>> fs/btrfs/super.c:403:3: error: expected expression
int ret;
^
>> fs/btrfs/super.c:405:3: error: use of undeclared identifier 'ret'
ret = memparse_safe(param->string, MEMPARSE_SUFFIXES_DEFAULT,
^
fs/btrfs/super.c:407:7: error: use of undeclared identifier 'ret'
if (ret < 0) {
^
fs/btrfs/super.c:409:11: error: use of undeclared identifier 'ret'
return ret;
^
4 errors generated.


vim +403 fs/btrfs/super.c

261
262 static int btrfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
263 {
264 struct btrfs_fs_context *ctx = fc->fs_private;
265 struct fs_parse_result result;
266 int opt;
267
268 opt = fs_parse(fc, btrfs_fs_parameters, param, &result);
269 if (opt < 0)
270 return opt;
271
272 switch (opt) {
273 case Opt_degraded:
274 btrfs_set_opt(ctx->mount_opt, DEGRADED);
275 break;
276 case Opt_subvol_empty:
277 /*
278 * This exists because we used to allow it on accident, so we're
279 * keeping it to maintain ABI. See 37becec95ac3 ("Btrfs: allow
280 * empty subvol= again").
281 */
282 break;
283 case Opt_subvol:
284 kfree(ctx->subvol_name);
285 ctx->subvol_name = kstrdup(param->string, GFP_KERNEL);
286 if (!ctx->subvol_name)
287 return -ENOMEM;
288 break;
289 case Opt_subvolid:
290 ctx->subvol_objectid = result.uint_64;
291
292 /* subvolid=0 means give me the original fs_tree. */
293 if (!ctx->subvol_objectid)
294 ctx->subvol_objectid = BTRFS_FS_TREE_OBJECTID;
295 break;
296 case Opt_device: {
297 struct btrfs_device *device;
298 blk_mode_t mode = sb_open_mode(fc->sb_flags);
299
300 mutex_lock(&uuid_mutex);
301 device = btrfs_scan_one_device(param->string, mode, false);
302 mutex_unlock(&uuid_mutex);
303 if (IS_ERR(device))
304 return PTR_ERR(device);
305 break;
306 }
307 case Opt_datasum:
308 if (result.negated) {
309 btrfs_set_opt(ctx->mount_opt, NODATASUM);
310 } else {
311 btrfs_clear_opt(ctx->mount_opt, NODATACOW);
312 btrfs_clear_opt(ctx->mount_opt, NODATASUM);
313 }
314 break;
315 case Opt_datacow:
316 if (result.negated) {
317 btrfs_clear_opt(ctx->mount_opt, COMPRESS);
318 btrfs_clear_opt(ctx->mount_opt, FORCE_COMPRESS);
319 btrfs_set_opt(ctx->mount_opt, NODATACOW);
320 btrfs_set_opt(ctx->mount_opt, NODATASUM);
321 } else {
322 btrfs_clear_opt(ctx->mount_opt, NODATACOW);
323 }
324 break;
325 case Opt_compress_force:
326 case Opt_compress_force_type:
327 btrfs_set_opt(ctx->mount_opt, FORCE_COMPRESS);
328 fallthrough;
329 case Opt_compress:
330 case Opt_compress_type:
331 if (opt == Opt_compress || opt == Opt_compress_force) {
332 ctx->compress_type = BTRFS_COMPRESS_ZLIB;
333 ctx->compress_level = BTRFS_ZLIB_DEFAULT_LEVEL;
334 btrfs_set_opt(ctx->mount_opt, COMPRESS);
335 btrfs_clear_opt(ctx->mount_opt, NODATACOW);
336 btrfs_clear_opt(ctx->mount_opt, NODATASUM);
337 } else if (strncmp(param->string, "zlib", 4) == 0) {
338 ctx->compress_type = BTRFS_COMPRESS_ZLIB;
339 ctx->compress_level =
340 btrfs_compress_str2level(BTRFS_COMPRESS_ZLIB,
341 param->string + 4);
342 btrfs_set_opt(ctx->mount_opt, COMPRESS);
343 btrfs_clear_opt(ctx->mount_opt, NODATACOW);
344 btrfs_clear_opt(ctx->mount_opt, NODATASUM);
345 } else if (strncmp(param->string, "lzo", 3) == 0) {
346 ctx->compress_type = BTRFS_COMPRESS_LZO;
347 ctx->compress_level = 0;
348 btrfs_set_opt(ctx->mount_opt, COMPRESS);
349 btrfs_clear_opt(ctx->mount_opt, NODATACOW);
350 btrfs_clear_opt(ctx->mount_opt, NODATASUM);
351 } else if (strncmp(param->string, "zstd", 4) == 0) {
352 ctx->compress_type = BTRFS_COMPRESS_ZSTD;
353 ctx->compress_level =
354 btrfs_compress_str2level(BTRFS_COMPRESS_ZSTD,
355 param->string + 4);
356 btrfs_set_opt(ctx->mount_opt, COMPRESS);
357 btrfs_clear_opt(ctx->mount_opt, NODATACOW);
358 btrfs_clear_opt(ctx->mount_opt, NODATASUM);
359 } else if (strncmp(param->string, "no", 2) == 0) {
360 ctx->compress_level = 0;
361 ctx->compress_type = 0;
362 btrfs_clear_opt(ctx->mount_opt, COMPRESS);
363 btrfs_clear_opt(ctx->mount_opt, FORCE_COMPRESS);
364 } else {
365 btrfs_err(NULL, "unrecognized compression value %s",
366 param->string);
367 return -EINVAL;
368 }
369 break;
370 case Opt_ssd:
371 if (result.negated) {
372 btrfs_set_opt(ctx->mount_opt, NOSSD);
373 btrfs_clear_opt(ctx->mount_opt, SSD);
374 btrfs_clear_opt(ctx->mount_opt, SSD_SPREAD);
375 } else {
376 btrfs_set_opt(ctx->mount_opt, SSD);
377 btrfs_clear_opt(ctx->mount_opt, NOSSD);
378 }
379 break;
380 case Opt_ssd_spread:
381 if (result.negated) {
382 btrfs_clear_opt(ctx->mount_opt, SSD_SPREAD);
383 } else {
384 btrfs_set_opt(ctx->mount_opt, SSD);
385 btrfs_set_opt(ctx->mount_opt, SSD_SPREAD);
386 btrfs_clear_opt(ctx->mount_opt, NOSSD);
387 }
388 break;
389 case Opt_barrier:
390 if (result.negated)
391 btrfs_set_opt(ctx->mount_opt, NOBARRIER);
392 else
393 btrfs_clear_opt(ctx->mount_opt, NOBARRIER);
394 break;
395 case Opt_thread_pool:
396 if (result.uint_32 == 0) {
397 btrfs_err(NULL, "invalid value 0 for thread_pool");
398 return -EINVAL;
399 }
400 ctx->thread_pool_size = result.uint_32;
401 break;
402 case Opt_max_inline:
> 403 int ret;
404
> 405 ret = memparse_safe(param->string, MEMPARSE_SUFFIXES_DEFAULT,
406 &ctx->max_inline, NULL);
407 if (ret < 0) {
408 btrfs_err(NULL, "invalid string \"%s\"", param->string);
409 return ret;
410 }
411 ctx->max_inline = memparse(param->string, NULL);
412 break;
413 case Opt_acl:
414 if (result.negated) {
415 fc->sb_flags &= ~SB_POSIXACL;
416 } else {
417 #ifdef CONFIG_BTRFS_FS_POSIX_ACL
418 fc->sb_flags |= SB_POSIXACL;
419 #else
420 btrfs_err(NULL, "support for ACL not compiled in");
421 return -EINVAL;
422 #endif
423 }
424 /*
425 * VFS limits the ability to toggle ACL on and off via remount,
426 * despite every file system allowing this. This seems to be
427 * an oversight since we all do, but it'll fail if we're
428 * remounting. So don't set the mask here, we'll check it in
429 * btrfs_reconfigure and do the toggling ourselves.
430 */
431 if (fc->purpose != FS_CONTEXT_FOR_RECONFIGURE)
432 fc->sb_flags_mask |= SB_POSIXACL;
433 break;
434 case Opt_treelog:
435 if (result.negated)
436 btrfs_set_opt(ctx->mount_opt, NOTREELOG);
437 else
438 btrfs_clear_opt(ctx->mount_opt, NOTREELOG);
439 break;
440 case Opt_nologreplay:
441 btrfs_warn(NULL,
442 "'nologreplay' is deprecated, use 'rescue=nologreplay' instead");
443 btrfs_set_opt(ctx->mount_opt, NOLOGREPLAY);
444 break;
445 case Opt_flushoncommit:
446 if (result.negated)
447 btrfs_clear_opt(ctx->mount_opt, FLUSHONCOMMIT);
448 else
449 btrfs_set_opt(ctx->mount_opt, FLUSHONCOMMIT);
450 break;
451 case Opt_ratio:
452 ctx->metadata_ratio = result.uint_32;
453 break;
454 case Opt_discard:
455 if (result.negated) {
456 btrfs_clear_opt(ctx->mount_opt, DISCARD_SYNC);
457 btrfs_clear_opt(ctx->mount_opt, DISCARD_ASYNC);
458 btrfs_set_opt(ctx->mount_opt, NODISCARD);
459 } else {
460 btrfs_set_opt(ctx->mount_opt, DISCARD_SYNC);
461 btrfs_clear_opt(ctx->mount_opt, DISCARD_ASYNC);
462 }
463 break;
464 case Opt_discard_mode:
465 switch (result.uint_32) {
466 case Opt_discard_sync:
467 btrfs_clear_opt(ctx->mount_opt, DISCARD_ASYNC);
468 btrfs_set_opt(ctx->mount_opt, DISCARD_SYNC);
469 break;
470 case Opt_discard_async:
471 btrfs_clear_opt(ctx->mount_opt, DISCARD_SYNC);
472 btrfs_set_opt(ctx->mount_opt, DISCARD_ASYNC);
473 break;
474 default:
475 btrfs_err(NULL, "unrecognized discard mode value %s",
476 param->key);
477 return -EINVAL;
478 }
479 btrfs_clear_opt(ctx->mount_opt, NODISCARD);
480 break;
481 case Opt_space_cache:
482 if (result.negated) {
483 btrfs_set_opt(ctx->mount_opt, NOSPACECACHE);
484 btrfs_clear_opt(ctx->mount_opt, SPACE_CACHE);
485 btrfs_clear_opt(ctx->mount_opt, FREE_SPACE_TREE);
486 } else {
487 btrfs_clear_opt(ctx->mount_opt, FREE_SPACE_TREE);
488 btrfs_set_opt(ctx->mount_opt, SPACE_CACHE);
489 }
490 break;
491 case Opt_space_cache_version:
492 switch (result.uint_32) {
493 case Opt_space_cache_v1:
494 btrfs_set_opt(ctx->mount_opt, SPACE_CACHE);
495 btrfs_clear_opt(ctx->mount_opt, FREE_SPACE_TREE);
496 break;
497 case Opt_space_cache_v2:
498 btrfs_clear_opt(ctx->mount_opt, SPACE_CACHE);
499 btrfs_set_opt(ctx->mount_opt, FREE_SPACE_TREE);
500 break;
501 default:
502 btrfs_err(NULL, "unrecognized space_cache value %s",
503 param->key);
504 return -EINVAL;
505 }
506 break;
507 case Opt_rescan_uuid_tree:
508 btrfs_set_opt(ctx->mount_opt, RESCAN_UUID_TREE);
509 break;
510 case Opt_clear_cache:
511 btrfs_set_opt(ctx->mount_opt, CLEAR_CACHE);
512 break;
513 case Opt_user_subvol_rm_allowed:
514 btrfs_set_opt(ctx->mount_opt, USER_SUBVOL_RM_ALLOWED);
515 break;
516 case Opt_enospc_debug:
517 if (result.negated)
518 btrfs_clear_opt(ctx->mount_opt, ENOSPC_DEBUG);
519 else
520 btrfs_set_opt(ctx->mount_opt, ENOSPC_DEBUG);
521 break;
522 case Opt_defrag:
523 if (result.negated)
524 btrfs_clear_opt(ctx->mount_opt, AUTO_DEFRAG);
525 else
526 btrfs_set_opt(ctx->mount_opt, AUTO_DEFRAG);
527 break;
528 case Opt_usebackuproot:
529 btrfs_warn(NULL,
530 "'usebackuproot' is deprecated, use 'rescue=usebackuproot' instead");
531 btrfs_set_opt(ctx->mount_opt, USEBACKUPROOT);
532
533 /* If we're loading the backup roots we can't trust the space cache. */
534 btrfs_set_opt(ctx->mount_opt, CLEAR_CACHE);
535 break;
536 case Opt_skip_balance:
537 btrfs_set_opt(ctx->mount_opt, SKIP_BALANCE);
538 break;
539 case Opt_fatal_errors:
540 switch (result.uint_32) {
541 case Opt_fatal_errors_panic:
542 btrfs_set_opt(ctx->mount_opt, PANIC_ON_FATAL_ERROR);
543 break;
544 case Opt_fatal_errors_bug:
545 btrfs_clear_opt(ctx->mount_opt, PANIC_ON_FATAL_ERROR);
546 break;
547 default:
548 btrfs_err(NULL, "unrecognized fatal_errors value %s",
549 param->key);
550 return -EINVAL;
551 }
552 break;
553 case Opt_commit_interval:
554 ctx->commit_interval = result.uint_32;
555 if (ctx->commit_interval == 0)
556 ctx->commit_interval = BTRFS_DEFAULT_COMMIT_INTERVAL;
557 break;
558 case Opt_rescue:
559 switch (result.uint_32) {
560 case Opt_rescue_usebackuproot:
561 btrfs_set_opt(ctx->mount_opt, USEBACKUPROOT);
562 break;
563 case Opt_rescue_nologreplay:
564 btrfs_set_opt(ctx->mount_opt, NOLOGREPLAY);
565 break;
566 case Opt_rescue_ignorebadroots:
567 btrfs_set_opt(ctx->mount_opt, IGNOREBADROOTS);
568 break;
569 case Opt_rescue_ignoredatacsums:
570 btrfs_set_opt(ctx->mount_opt, IGNOREDATACSUMS);
571 break;
572 case Opt_rescue_parameter_all:
573 btrfs_set_opt(ctx->mount_opt, IGNOREDATACSUMS);
574 btrfs_set_opt(ctx->mount_opt, IGNOREBADROOTS);
575 btrfs_set_opt(ctx->mount_opt, NOLOGREPLAY);
576 break;
577 default:
578 btrfs_info(NULL, "unrecognized rescue option '%s'",
579 param->key);
580 return -EINVAL;
581 }
582 break;
583 #ifdef CONFIG_BTRFS_DEBUG
584 case Opt_fragment:
585 switch (result.uint_32) {
586 case Opt_fragment_parameter_all:
587 btrfs_set_opt(ctx->mount_opt, FRAGMENT_DATA);
588 btrfs_set_opt(ctx->mount_opt, FRAGMENT_METADATA);
589 break;
590 case Opt_fragment_parameter_metadata:
591 btrfs_set_opt(ctx->mount_opt, FRAGMENT_METADATA);
592 break;
593 case Opt_fragment_parameter_data:
594 btrfs_set_opt(ctx->mount_opt, FRAGMENT_DATA);
595 break;
596 default:
597 btrfs_info(NULL, "unrecognized fragment option '%s'",
598 param->key);
599 return -EINVAL;
600 }
601 break;
602 #endif
603 #ifdef CONFIG_BTRFS_FS_REF_VERIFY
604 case Opt_ref_verify:
605 btrfs_set_opt(ctx->mount_opt, REF_VERIFY);
606 break;
607 #endif
608 default:
609 btrfs_err(NULL, "unrecognized mount option '%s'", param->key);
610 return -EINVAL;
611 }
612
613 return 0;
614 }
615

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

2023-12-26 06:38:04

by kernel test robot

Subject: Re: [PATCH 2/3] kstrtox: add unit tests for memparse_safe()

Hi Qu,

kernel test robot noticed the following build warnings:

[auto build test WARNING on kdave/for-next]
[also build test WARNING on akpm-mm/mm-everything linus/master v6.7-rc7 next-20231222]
[cannot apply to akpm-mm/mm-nonmm-unstable]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Qu-Wenruo/kstrtox-introduce-a-safer-version-of-memparse/20231225-151921
base: https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
patch link: https://lore.kernel.org/r/56ea15d8b430f4fe3f8e55509ad0bc72b1d9356f.1703324146.git.wqu%40suse.com
patch subject: [PATCH 2/3] kstrtox: add unit tests for memparse_safe()
config: m68k-randconfig-r133-20231226 (https://download.01.org/0day-ci/archive/20231226/[email protected]/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20231226/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

sparse warnings: (new ones prefixed by >>)
>> lib/test-kstrtox.c:339:40: sparse: sparse: cast truncates bits from constant value (efefefef7a7a7a7a becomes 7a7a7a7a)
lib/test-kstrtox.c:351:39: sparse: sparse: cast truncates bits from constant value (efefefef7a7a7a7a becomes 7a7a7a7a)

vim +339 lib/test-kstrtox.c

275
276 /* Want to include "E" suffix for full coverage. */
277 #define MEMPARSE_TEST_SUFFIX (MEMPARSE_SUFFIX_K | MEMPARSE_SUFFIX_M |\
278 MEMPARSE_SUFFIX_G | MEMPARSE_SUFFIX_T |\
279 MEMPARSE_SUFFIX_P | MEMPARSE_SUFFIX_E)
280
281 static void __init test_memparse_safe_fail(void)
282 {
283 struct memparse_test_fail {
284 const char *str;
285 /* Expected error number, either -EINVAL or -ERANGE. */
286 unsigned int expected_ret;
287 };
288 static const struct memparse_test_fail tests[] __initconst = {
289 /* No valid string can be found at all. */
290 {"", -EINVAL},
291 {"\n", -EINVAL},
292 {"\n0", -EINVAL},
293 {"+", -EINVAL},
294 {"-", -EINVAL},
295
296 /*
297 * No support for any leading "+-" chars, even followed by a valid
298 * number.
299 */
300 {"-0", -EINVAL},
301 {"+0", -EINVAL},
302 {"-1", -EINVAL},
303 {"+1", -EINVAL},
304
305 /* Stray suffix would also be rejected. */
306 {"K", -EINVAL},
307 {"P", -EINVAL},
308
309 /* Overflow in the string itself*/
310 {"18446744073709551616", -ERANGE},
311 {"02000000000000000000000", -ERANGE},
312 {"0x10000000000000000", -ERANGE},
313
314 /*
315 * Good string but would overflow with suffix.
316 *
317 * Note, for "E" suffix, one should not use with hex, or "0x1E"
318 * would be treated as 0x1e (30 in decimal), not 0x1 and "E" suffix.
319 * Another reason "E" suffix is cursed.
320 */
321 {"16E", -ERANGE},
322 {"020E", -ERANGE},
323 {"16384P", -ERANGE},
324 {"040000P", -ERANGE},
325 {"16777216T", -ERANGE},
326 {"0100000000T", -ERANGE},
327 {"17179869184G", -ERANGE},
328 {"0200000000000G", -ERANGE},
329 {"17592186044416M", -ERANGE},
330 {"0400000000000000M", -ERANGE},
331 {"18014398509481984K", -ERANGE},
332 {"01000000000000000000K", -ERANGE},
333 };
334 unsigned int i;
335
336 for_each_test(i, tests) {
337 const struct memparse_test_fail *t = &tests[i];
338 unsigned long long tmp = ULL_PATTERN;
> 339 char *retptr = (char *)ULL_PATTERN;
340 int ret;
341
342 ret = memparse_safe(t->str, MEMPARSE_TEST_SUFFIX, &tmp, &retptr);
343 if (ret != t->expected_ret) {
344 WARN(1, "str '%s', expected ret %d got %d\n", t->str,
345 t->expected_ret, ret);
346 continue;
347 }
348 if (tmp != ULL_PATTERN)
349 WARN(1, "str '%s' failed as expected, but result got modified",
350 t->str);
351 if (retptr != (char *)ULL_PATTERN)
352 WARN(1, "str '%s' failed as expected, but pointer got modified",
353 t->str);
354 }
355 }
356

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

2023-12-26 07:40:23

by Qu Wenruo

Subject: Re: [PATCH 2/3] kstrtox: add unit tests for memparse_safe()

On 2023/12/26 17:06, kernel test robot wrote:
> Hi Qu,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on kdave/for-next]
> [also build test WARNING on akpm-mm/mm-everything linus/master v6.7-rc7 next-20231222]
> [cannot apply to akpm-mm/mm-nonmm-unstable]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Qu-Wenruo/kstrtox-introduce-a-safer-version-of-memparse/20231225-151921
> base: https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
> patch link: https://lore.kernel.org/r/56ea15d8b430f4fe3f8e55509ad0bc72b1d9356f.1703324146.git.wqu%40suse.com
> patch subject: [PATCH 2/3] kstrtox: add unit tests for memparse_safe()
> config: m68k-randconfig-r133-20231226 (https://download.01.org/0day-ci/archive/20231226/[email protected]/config)
> compiler: m68k-linux-gcc (GCC) 13.2.0
> reproduce: (https://download.01.org/0day-ci/archive/20231226/[email protected]/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <[email protected]>
> | Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
>
> sparse warnings: (new ones prefixed by >>)
>>> lib/test-kstrtox.c:339:40: sparse: sparse: cast truncates bits from constant value (efefefef7a7a7a7a becomes 7a7a7a7a)
> lib/test-kstrtox.c:351:39: sparse: sparse: cast truncates bits from constant value (efefefef7a7a7a7a becomes 7a7a7a7a)

Any way to suppress the warning? As long as the constant value (u64) is
checked against the same truncated value (u32), the result should be fine.

I really want to make sure the pointer is not incorrectly updated in the
failure case.

Thanks,
Qu
>
> vim +339 lib/test-kstrtox.c
>
> 275
> 276 /* Want to include "E" suffix for full coverage. */
> 277 #define MEMPARSE_TEST_SUFFIX (MEMPARSE_SUFFIX_K | MEMPARSE_SUFFIX_M |\
> 278 MEMPARSE_SUFFIX_G | MEMPARSE_SUFFIX_T |\
> 279 MEMPARSE_SUFFIX_P | MEMPARSE_SUFFIX_E)
> 280
> 281 static void __init test_memparse_safe_fail(void)
> 282 {
> 283 struct memparse_test_fail {
> 284 const char *str;
> 285 /* Expected error number, either -EINVAL or -ERANGE. */
> 286 unsigned int expected_ret;
> 287 };
> 288 static const struct memparse_test_fail tests[] __initconst = {
> 289 /* No valid string can be found at all. */
> 290 {"", -EINVAL},
> 291 {"\n", -EINVAL},
> 292 {"\n0", -EINVAL},
> 293 {"+", -EINVAL},
> 294 {"-", -EINVAL},
> 295
> 296 /*
> 297 * No support for any leading "+-" chars, even followed by a valid
> 298 * number.
> 299 */
> 300 {"-0", -EINVAL},
> 301 {"+0", -EINVAL},
> 302 {"-1", -EINVAL},
> 303 {"+1", -EINVAL},
> 304
> 305 /* Stray suffix would also be rejected. */
> 306 {"K", -EINVAL},
> 307 {"P", -EINVAL},
> 308
> 309 /* Overflow in the string itself*/
> 310 {"18446744073709551616", -ERANGE},
> 311 {"02000000000000000000000", -ERANGE},
> 312 {"0x10000000000000000", -ERANGE},
> 313
> 314 /*
> 315 * Good string but would overflow with suffix.
> 316 *
> 317 * Note, for "E" suffix, one should not use with hex, or "0x1E"
> 318 * would be treated as 0x1e (30 in decimal), not 0x1 and "E" suffix.
> 319 * Another reason "E" suffix is cursed.
> 320 */
> 321 {"16E", -ERANGE},
> 322 {"020E", -ERANGE},
> 323 {"16384P", -ERANGE},
> 324 {"040000P", -ERANGE},
> 325 {"16777216T", -ERANGE},
> 326 {"0100000000T", -ERANGE},
> 327 {"17179869184G", -ERANGE},
> 328 {"0200000000000G", -ERANGE},
> 329 {"17592186044416M", -ERANGE},
> 330 {"0400000000000000M", -ERANGE},
> 331 {"18014398509481984K", -ERANGE},
> 332 {"01000000000000000000K", -ERANGE},
> 333 };
> 334 unsigned int i;
> 335
> 336 for_each_test(i, tests) {
> 337 const struct memparse_test_fail *t = &tests[i];
> 338 unsigned long long tmp = ULL_PATTERN;
> > 339 char *retptr = (char *)ULL_PATTERN;
> 340 int ret;
> 341
> 342 ret = memparse_safe(t->str, MEMPARSE_TEST_SUFFIX, &tmp, &retptr);
> 343 if (ret != t->expected_ret) {
> 344 WARN(1, "str '%s', expected ret %d got %d\n", t->str,
> 345 t->expected_ret, ret);
> 346 continue;
> 347 }
> 348 if (tmp != ULL_PATTERN)
> 349 WARN(1, "str '%s' failed as expected, but result got modified",
> 350 t->str);
> 351 if (retptr != (char *)ULL_PATTERN)
> 352 WARN(1, "str '%s' failed as expected, but pointer got modified",
> 353 t->str);
> 354 }
> 355 }
> 356
>

2023-12-27 06:35:03

by David Disseldorp

Subject: Re: [PATCH 3/3] btrfs: migrate to the newer memparse_safe() helper

On Sat, 23 Dec 2023 20:28:07 +1030, Qu Wenruo wrote:

> 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]>
> ---
> fs/btrfs/ioctl.c | 8 ++++++--
> fs/btrfs/super.c | 8 ++++++++
> fs/btrfs/sysfs.c | 14 +++++++++++---
> 3 files changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 4e50b62db2a8..8bfd4b4ccf02 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1175,8 +1175,12 @@ static noinline int btrfs_ioctl_resize(struct file *file,
> mod = 1;
> sizestr++;
> }
> - new_size = memparse(sizestr, &retptr);
> - if (*retptr != '\0' || new_size == 0) {
> +
> + ret = memparse_safe(sizestr, MEMPARSE_SUFFIXES_DEFAULT,
> + &new_size, &retptr);
> + if (ret < 0)
> + goto out_finish;
> + if (*retptr != '\0') {

Was dropping the -EINVAL return for new_size=0 intentional?

> ret = -EINVAL;
> goto out_finish;
> }
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 3a677b808f0f..2bb6ea525e89 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -400,6 +400,14 @@ 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:
> + int ret;
> +
> + 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;
> + }
> ctx->max_inline = memparse(param->string, NULL);

Looks like you overlooked removal of the old memparse() call above.

Cheers, David

2023-12-27 08:29:03

by Qu Wenruo

Subject: Re: [PATCH 3/3] btrfs: migrate to the newer memparse_safe() helper

On 2023/12/27 16:57, David Disseldorp wrote:
> On Sat, 23 Dec 2023 20:28:07 +1030, Qu Wenruo wrote:
>
>> 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]>
>> ---
>> fs/btrfs/ioctl.c | 8 ++++++--
>> fs/btrfs/super.c | 8 ++++++++
>> fs/btrfs/sysfs.c | 14 +++++++++++---
>> 3 files changed, 25 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 4e50b62db2a8..8bfd4b4ccf02 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -1175,8 +1175,12 @@ static noinline int btrfs_ioctl_resize(struct file *file,
>> mod = 1;
>> sizestr++;
>> }
>> - new_size = memparse(sizestr, &retptr);
>> - if (*retptr != '\0' || new_size == 0) {
>> +
>> + ret = memparse_safe(sizestr, MEMPARSE_SUFFIXES_DEFAULT,
>> + &new_size, &retptr);
>> + if (ret < 0)
>> + goto out_finish;
>> + if (*retptr != '\0') {
>
> Was dropping the -EINVAL return for new_size=0 intentional?

Oh, that's unintentional. Although we would reject the invalid string, a
dedicated "0" can still be parsed.
In that case we should still return -EINVAL.

I just got it confused with the old behavior for invalid string (where 0
is returned and @retptr is not advanced).
>
>> ret = -EINVAL;
>> goto out_finish;
>> }
>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>> index 3a677b808f0f..2bb6ea525e89 100644
>> --- a/fs/btrfs/super.c
>> +++ b/fs/btrfs/super.c
>> @@ -400,6 +400,14 @@ 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:
>> + int ret;
>> +
>> + 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;
>> + }
>> ctx->max_inline = memparse(param->string, NULL);
>
> Looks like you overlooked removal of the old memparse() call above.

My bad, I forgot to remove the old line.

Furthermore, the declaration of "ret" inside case block is not allowed,
I'll fix it anyway.

Thanks,
Qu
>
> Cheers, David
>

2023-12-27 13:35:04

by David Disseldorp

Subject: Re: [PATCH 1/3] kstrtox: introduce a safer version of memparse()

On Sat, 23 Dec 2023 20:28:05 +1030, Qu Wenruo wrote:

> + s = _parse_integer_fixup_radix(s, &base);
> + rv = _parse_integer(s, base, &value);
> + if (rv & KSTRTOX_OVERFLOW)
> + return -ERANGE;
> + if (rv == 0)
> + return -EINVAL;

I was playing around with your unit tests and noticed that "0xG" didn't
reach the expected rv == 0 -> -EINVAL above. It seems that
_parse_integer_fixup_radix() should handle 0x<non hex> differently, or
at least step past any autodetected '0' octal prefix.

Cheers, David

2023-12-27 15:00:58

by Alexey Dobriyan

Subject: Re: [PATCH 0/3] kstrtox: introduce memparse_safe()

> fs/btrfs/ioctl.c | 8 +-
> fs/btrfs/super.c | 8 ++
> fs/btrfs/sysfs.c | 14 ++-
> include/linux/kernel.h | 8 +-
> include/linux/kstrtox.h | 15 +++
> lib/cmdline.c | 5 +-
> lib/kstrtox.c | 96 ++++++++++++++++++
> lib/test-kstrtox.c | 217 ++++++++++++++++++++++++++++++++++++++++
> 8 files changed, 364 insertions(+), 7 deletions(-)

Please keep memparse stuff out of kstrtox.c .
kstrtox.c is for kstrto*() functions.

2023-12-27 16:12:21

by Andy Shevchenko

Subject: Re: [PATCH 0/3] kstrtox: introduce memparse_safe()

On Sat, Dec 23, 2023 at 08:28:04PM +1030, Qu Wenruo wrote:
> 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 other 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.
>
> With the new helper, migrate btrfs to the interface, and since the
> @retptr behavior is the same, we won't cause any behavior change.

Thank you for the prompt update, I will be off till end of January,
and in any case this is material for v6.9+, so I will look at this
afterwards.

--
With Best Regards,
Andy Shevchenko

2023-12-27 20:30:02

by Qu Wenruo

Subject: Re: [PATCH 1/3] kstrtox: introduce a safer version of memparse()

On 2023/12/27 23:56, David Disseldorp wrote:
> On Sat, 23 Dec 2023 20:28:05 +1030, Qu Wenruo wrote:
>
>> + s = _parse_integer_fixup_radix(s, &base);
>> + rv = _parse_integer(s, base, &value);
>> + if (rv & KSTRTOX_OVERFLOW)
>> + return -ERANGE;
>> + if (rv == 0)
>> + return -EINVAL;
>
> I was playing around with your unit tests and noticed that "0xG" didn't
> reach the expected rv == 0 -> -EINVAL above. It seems that
> _parse_integer_fixup_radix() should handle 0x<non hex> differently, or
> at least step past any autodetected '0' octal prefix.

Yes, that's also the problem I hit, but I'm not 100% sure if changing
_parse_integer_fixup_radix() is safe for other call sites thus I didn't
put such test case here.

My initial failure cases includes things like "0x" which would still
return 0 is already a warning sign.

Thanks,
Qu
>
> Cheers, David
>

2024-01-02 01:38:37

by Yujie Liu

Subject: Re: [PATCH 2/3] kstrtox: add unit tests for memparse_safe()

On Tue, Dec 26, 2023 at 06:07:40PM +1030, Qu Wenruo wrote:
> On 2023/12/26 17:06, kernel test robot wrote:
> > Hi Qu,
> >
> > kernel test robot noticed the following build warnings:
> >
> > [auto build test WARNING on kdave/for-next]
> > [also build test WARNING on akpm-mm/mm-everything linus/master v6.7-rc7 next-20231222]
> > [cannot apply to akpm-mm/mm-nonmm-unstable]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> >
> > url: https://github.com/intel-lab-lkp/linux/commits/Qu-Wenruo/kstrtox-introduce-a-safer-version-of-memparse/20231225-151921
> > base: https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
> > patch link: https://lore.kernel.org/r/56ea15d8b430f4fe3f8e55509ad0bc72b1d9356f.1703324146.git.wqu%40suse.com
> > patch subject: [PATCH 2/3] kstrtox: add unit tests for memparse_safe()
> > config: m68k-randconfig-r133-20231226 (https://download.01.org/0day-ci/archive/20231226/[email protected]/config)
> > compiler: m68k-linux-gcc (GCC) 13.2.0
> > reproduce: (https://download.01.org/0day-ci/archive/20231226/[email protected]/reproduce)
> >
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <[email protected]>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> >
> > sparse warnings: (new ones prefixed by >>)
> > > > lib/test-kstrtox.c:339:40: sparse: sparse: cast truncates bits from constant value (efefefef7a7a7a7a becomes 7a7a7a7a)
> > lib/test-kstrtox.c:351:39: sparse: sparse: cast truncates bits from constant value (efefefef7a7a7a7a becomes 7a7a7a7a)
>
> Any way to suppress the warning? As long as the constant value (u64) is
> checked against the same truncated value (u32), the result should be fine.

Hi Qu, we've suppressed this warning in the bot for the specific file
lib/test-kstrtox.c, while keep it enabled for the rest. If there are
similar warnings in other files that could be false positives, we will
also suppress them later.

Thanks,
Yujie

>
> I really want to make sure the pointer is not incorrectly updated in the
> failure case.
>
> Thanks,
> Qu
> >
> > vim +339 lib/test-kstrtox.c
> >
> > 275
> > 276 /* Want to include "E" suffix for full coverage. */
> > 277 #define MEMPARSE_TEST_SUFFIX (MEMPARSE_SUFFIX_K | MEMPARSE_SUFFIX_M |\
> > 278 MEMPARSE_SUFFIX_G | MEMPARSE_SUFFIX_T |\
> > 279 MEMPARSE_SUFFIX_P | MEMPARSE_SUFFIX_E)
> > 280
> > 281 static void __init test_memparse_safe_fail(void)
> > 282 {
> > 283 struct memparse_test_fail {
> > 284 const char *str;
> > 285 /* Expected error number, either -EINVAL or -ERANGE. */
> > 286 unsigned int expected_ret;
> > 287 };
> > 288 static const struct memparse_test_fail tests[] __initconst = {
> > 289 /* No valid string can be found at all. */
> > 290 {"", -EINVAL},
> > 291 {"\n", -EINVAL},
> > 292 {"\n0", -EINVAL},
> > 293 {"+", -EINVAL},
> > 294 {"-", -EINVAL},
> > 295
> > 296 /*
> > 297 * No support for any leading "+-" chars, even followed by a valid
> > 298 * number.
> > 299 */
> > 300 {"-0", -EINVAL},
> > 301 {"+0", -EINVAL},
> > 302 {"-1", -EINVAL},
> > 303 {"+1", -EINVAL},
> > 304
> > 305 /* Stray suffix would also be rejected. */
> > 306 {"K", -EINVAL},
> > 307 {"P", -EINVAL},
> > 308
> > 309 /* Overflow in the string itself*/
> > 310 {"18446744073709551616", -ERANGE},
> > 311 {"02000000000000000000000", -ERANGE},
> > 312 {"0x10000000000000000", -ERANGE},
> > 313
> > 314 /*
> > 315 * Good string but would overflow with suffix.
> > 316 *
> > 317 * Note, for "E" suffix, one should not use with hex, or "0x1E"
> > 318 * would be treated as 0x1e (30 in decimal), not 0x1 and "E" suffix.
> > 319 * Another reason "E" suffix is cursed.
> > 320 */
> > 321 {"16E", -ERANGE},
> > 322 {"020E", -ERANGE},
> > 323 {"16384P", -ERANGE},
> > 324 {"040000P", -ERANGE},
> > 325 {"16777216T", -ERANGE},
> > 326 {"0100000000T", -ERANGE},
> > 327 {"17179869184G", -ERANGE},
> > 328 {"0200000000000G", -ERANGE},
> > 329 {"17592186044416M", -ERANGE},
> > 330 {"0400000000000000M", -ERANGE},
> > 331 {"18014398509481984K", -ERANGE},
> > 332 {"01000000000000000000K", -ERANGE},
> > 333 };
> > 334 unsigned int i;
> > 335
> > 336 for_each_test(i, tests) {
> > 337 const struct memparse_test_fail *t = &tests[i];
> > 338 unsigned long long tmp = ULL_PATTERN;
> > > 339 char *retptr = (char *)ULL_PATTERN;
> > 340 int ret;
> > 341
> > 342 ret = memparse_safe(t->str, MEMPARSE_TEST_SUFFIX, &tmp, &retptr);
> > 343 if (ret != t->expected_ret) {
> > 344 WARN(1, "str '%s', expected ret %d got %d\n", t->str,
> > 345 t->expected_ret, ret);
> > 346 continue;
> > 347 }
> > 348 if (tmp != ULL_PATTERN)
> > 349 WARN(1, "str '%s' failed as expected, but result got modified",
> > 350 t->str);
> > 351 if (retptr != (char *)ULL_PATTERN)
> > 352 WARN(1, "str '%s' failed as expected, but pointer got modified",
> > 353 t->str);
> > 354 }
> > 355 }
> > 356
> >
>

2024-01-02 02:03:51

by Qu Wenruo

Subject: Re: [PATCH 2/3] kstrtox: add unit tests for memparse_safe()

On 2024/1/2 12:03, Yujie Liu wrote:
> On Tue, Dec 26, 2023 at 06:07:40PM +1030, Qu Wenruo wrote:
>> On 2023/12/26 17:06, kernel test robot wrote:
>>> Hi Qu,
>>>
>>> kernel test robot noticed the following build warnings:
>>>
>>> [auto build test WARNING on kdave/for-next]
>>> [also build test WARNING on akpm-mm/mm-everything linus/master v6.7-rc7 next-20231222]
>>> [cannot apply to akpm-mm/mm-nonmm-unstable]
>>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>>> And when submitting patch, we suggest to use '--base' as documented in
>>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>>>
>>> url: https://github.com/intel-lab-lkp/linux/commits/Qu-Wenruo/kstrtox-introduce-a-safer-version-of-memparse/20231225-151921
>>> base: https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
>>> patch link: https://lore.kernel.org/r/56ea15d8b430f4fe3f8e55509ad0bc72b1d9356f.1703324146.git.wqu%40suse.com
>>> patch subject: [PATCH 2/3] kstrtox: add unit tests for memparse_safe()
>>> config: m68k-randconfig-r133-20231226 (https://download.01.org/0day-ci/archive/20231226/[email protected]/config)
>>> compiler: m68k-linux-gcc (GCC) 13.2.0
>>> reproduce: (https://download.01.org/0day-ci/archive/20231226/[email protected]/reproduce)
>>>
>>> If you fix the issue in a separate patch/commit (i.e. not just a new version of
>>> the same patch/commit), kindly add following tags
>>> | Reported-by: kernel test robot <[email protected]>
>>> | Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
>>>
>>> sparse warnings: (new ones prefixed by >>)
>>>>> lib/test-kstrtox.c:339:40: sparse: sparse: cast truncates bits from constant value (efefefef7a7a7a7a becomes 7a7a7a7a)
>>> lib/test-kstrtox.c:351:39: sparse: sparse: cast truncates bits from constant value (efefefef7a7a7a7a becomes 7a7a7a7a)
>>
>> Any way to suppress the warning? As long as the constant value (u64) is
>> checked against the same truncated value (u32), the result should be fine.
>
> Hi Qu, we've suppressed this warning in the bot for the specific file
> lib/test-kstrtox.c, while keep it enabled for the rest. If there are
> similar warnings in other files that could be false positives, we will
> also suppress them later.

I'll update the pattern depending on the ULL size to avoid the warning.

Thanks,
Qu
>
> Thanks,
> Yujie
>
>>
>> I really want to make sure the pointer is not incorrectly updated in the
>> failure case.
>>
>> Thanks,
>> Qu
>>>
>>> vim +339 lib/test-kstrtox.c
>>>
>>> 275
>>> 276 /* Want to include "E" suffix for full coverage. */
>>> 277 #define MEMPARSE_TEST_SUFFIX (MEMPARSE_SUFFIX_K | MEMPARSE_SUFFIX_M |\
>>> 278 MEMPARSE_SUFFIX_G | MEMPARSE_SUFFIX_T |\
>>> 279 MEMPARSE_SUFFIX_P | MEMPARSE_SUFFIX_E)
>>> 280
>>> 281 static void __init test_memparse_safe_fail(void)
>>> 282 {
>>> 283 struct memparse_test_fail {
>>> 284 const char *str;
>>> 285 /* Expected error number, either -EINVAL or -ERANGE. */
>>> 286 unsigned int expected_ret;
>>> 287 };
>>> 288 static const struct memparse_test_fail tests[] __initconst = {
>>> 289 /* No valid string can be found at all. */
>>> 290 {"", -EINVAL},
>>> 291 {"\n", -EINVAL},
>>> 292 {"\n0", -EINVAL},
>>> 293 {"+", -EINVAL},
>>> 294 {"-", -EINVAL},
>>> 295
>>> 296 /*
>>> 297 * No support for any leading "+-" chars, even followed by a valid
>>> 298 * number.
>>> 299 */
>>> 300 {"-0", -EINVAL},
>>> 301 {"+0", -EINVAL},
>>> 302 {"-1", -EINVAL},
>>> 303 {"+1", -EINVAL},
>>> 304
>>> 305 /* Stray suffix would also be rejected. */
>>> 306 {"K", -EINVAL},
>>> 307 {"P", -EINVAL},
>>> 308
>>> 309 /* Overflow in the string itself*/
>>> 310 {"18446744073709551616", -ERANGE},
>>> 311 {"02000000000000000000000", -ERANGE},
>>> 312 {"0x10000000000000000", -ERANGE},
>>> 313
>>> 314 /*
>>> 315 * Good string but would overflow with suffix.
>>> 316 *
>>> 317 * Note, for "E" suffix, one should not use with hex, or "0x1E"
>>> 318 * would be treated as 0x1e (30 in decimal), not 0x1 and "E" suffix.
>>> 319 * Another reason "E" suffix is cursed.
>>> 320 */
>>> 321 {"16E", -ERANGE},
>>> 322 {"020E", -ERANGE},
>>> 323 {"16384P", -ERANGE},
>>> 324 {"040000P", -ERANGE},
>>> 325 {"16777216T", -ERANGE},
>>> 326 {"0100000000T", -ERANGE},
>>> 327 {"17179869184G", -ERANGE},
>>> 328 {"0200000000000G", -ERANGE},
>>> 329 {"17592186044416M", -ERANGE},
>>> 330 {"0400000000000000M", -ERANGE},
>>> 331 {"18014398509481984K", -ERANGE},
>>> 332 {"01000000000000000000K", -ERANGE},
>>> 333 };
>>> 334 unsigned int i;
>>> 335
>>> 336 for_each_test(i, tests) {
>>> 337 const struct memparse_test_fail *t = &tests[i];
>>> 338 unsigned long long tmp = ULL_PATTERN;
>>> > 339 char *retptr = (char *)ULL_PATTERN;
>>> 340 int ret;
>>> 341
>>> 342 ret = memparse_safe(t->str, MEMPARSE_TEST_SUFFIX, &tmp, &retptr);
>>> 343 if (ret != t->expected_ret) {
>>> 344 WARN(1, "str '%s', expected ret %d got %d\n", t->str,
>>> 345 t->expected_ret, ret);
>>> 346 continue;
>>> 347 }
>>> 348 if (tmp != ULL_PATTERN)
>>> 349 WARN(1, "str '%s' failed as expected, but result got modified",
>>> 350 t->str);
>>> 351 if (retptr != (char *)ULL_PATTERN)
>>> 352 WARN(1, "str '%s' failed as expected, but pointer got modified",
>>> 353 t->str);
>>> 354 }
>>> 355 }
>>> 356
>>>
>>
>