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

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

[Qu Wenruo]

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

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

[Qu Wenruo]

[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 <wqu@suse.com>
---
 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

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

[David Disseldorp]

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

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

[Qu Wenruo]

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
>

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

[Qu Wenruo]

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 <wqu@suse.com>
---
 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

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

[kernel test robot]

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/202312261423.zqIlU2hn-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20231226/202312261423.zqIlU2hn-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312261423.zqIlU2hn-lkp@intel.com/

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

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

[Qu Wenruo]

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/202312261423.zqIlU2hn-lkp@intel.com/config)
> compiler: m68k-linux-gcc (GCC) 13.2.0
> reproduce: (https://download.01.org/0day-ci/archive/20231226/202312261423.zqIlU2hn-lkp@intel.com/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 <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202312261423.zqIlU2hn-lkp@intel.com/
>
> 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
>

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

[Yujie Liu]

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/202312261423.zqIlU2hn-lkp@intel.com/config)
> > compiler: m68k-linux-gcc (GCC) 13.2.0
> > reproduce: (https://download.01.org/0day-ci/archive/20231226/202312261423.zqIlU2hn-lkp@intel.com/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 <lkp@intel.com>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202312261423.zqIlU2hn-lkp@intel.com/
> > 
> > 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
> > 
> 

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

[Qu Wenruo]

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/202312261423.zqIlU2hn-lkp@intel.com/config)
>>> compiler: m68k-linux-gcc (GCC) 13.2.0
>>> reproduce: (https://download.01.org/0day-ci/archive/20231226/202312261423.zqIlU2hn-lkp@intel.com/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 <lkp@intel.com>
>>> | Closes: https://lore.kernel.org/oe-kbuild-all/202312261423.zqIlU2hn-lkp@intel.com/
>>>
>>> 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
>>>
>>
>

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

[Qu Wenruo]

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 <wqu@suse.com>
---
 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

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

[kernel test robot]

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/202312261215.eFf9J1xV-lkp@intel.com/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/202312261215.eFf9J1xV-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312261215.eFf9J1xV-lkp@intel.com/

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

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

[kernel test robot]

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/202312261319.pYgA0ivL-lkp@intel.com/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/202312261319.pYgA0ivL-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312261319.pYgA0ivL-lkp@intel.com/

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

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

[David Disseldorp]

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 <wqu@suse.com>
> ---
>  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

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

[Qu Wenruo]

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 <wqu@suse.com>
>> ---
>>   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
>

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

[Andy Shevchenko]

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