[PATCH] lib: introduce simple error-checking wrapper for memparse()

[PATCH] lib: introduce simple error-checking wrapper for memparse()

[Dmitry Antipov]

Introduce 'memvalue()' which uses 'memparse()' to parse a string with
optional memory suffix into a number and returns this number or ULLONG_MAX
if the number is negative or an unrecognized character was encountered.

Suggested-by: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 include/linux/string.h |  1 +
 lib/cmdline.c          | 21 +++++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index 1b564c36d721..c63bcff820a1 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -319,6 +319,7 @@ DEFINE_FREE(argv_free, char **, if (!IS_ERR_OR_NULL(_T)) argv_free(_T))
 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);
+extern unsigned long long memvalue(const char *ptr);
 extern bool parse_option_str(const char *str, const char *option);
 extern char *next_arg(char *args, char **param, char **val);
 
diff --git a/lib/cmdline.c b/lib/cmdline.c
index 90ed997d9570..e2455a6d17ff 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -190,6 +190,27 @@ unsigned long long memparse(const char *ptr, char **retptr)
 }
 EXPORT_SYMBOL(memparse);
 
+/**
+ *	memvalue -  Wrap memparse() with simple error detection
+ *	@ptr: Where parse begins
+ *
+ *	Unconditionally returns ULLONG_MAX for a presumably negative value.
+ *	Otherwise uses memparse() to parse a string into a number and returns
+ *	this number or ULLONG_MAX if an unrecognized character was encountered.
+ */
+
+unsigned long long memvalue(const char *ptr)
+{
+	unsigned long long ret;
+	char *end;
+
+	if (*ptr == '-')
+		return ULLONG_MAX;
+	ret = memparse(ptr, &end);
+	return *end ? ULLONG_MAX : ret;
+}
+EXPORT_SYMBOL(memvalue);
+
 /**
  *	parse_option_str - Parse a string and check an option is set or not
  *	@str: String to be parsed
-- 
2.52.0

[PATCH v2] xfs: adjust handling of a few numerical mount options

[Dmitry Antipov]

Prefer recently introduced 'memvalue()' over an ad-hoc 'suffix_kstrtoint()'
and 'suffix_kstrtoull()' to parse and basically validate the values passed
via 'logbsize', 'allocsize', and 'max_atomic_write' mount options, and
reject non-power-of-two values passed via the first and second one early
in 'xfs_fs_parse_param()' rather than in 'xfs_fs_validate_params()'.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v2: rely on 'memvalue()' as (well, IIUC) suggested by Christoph and
    handle both 'logbsize' and 'allocsize' in 'xfs_fs_parse_param()'
---
 fs/xfs/xfs_super.c | 123 ++++++++++-----------------------------------
 1 file changed, 27 insertions(+), 96 deletions(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index bc71aa9dcee8..3ee63f7b5a4a 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1319,77 +1319,6 @@ static const struct super_operations xfs_super_operations = {
 	.show_stats		= xfs_fs_show_stats,
 };
 
-static int
-suffix_kstrtoint(
-	const char	*s,
-	unsigned int	base,
-	int		*res)
-{
-	int		last, shift_left_factor = 0, _res;
-	char		*value;
-	int		ret = 0;
-
-	value = kstrdup(s, GFP_KERNEL);
-	if (!value)
-		return -ENOMEM;
-
-	last = strlen(value) - 1;
-	if (value[last] == 'K' || value[last] == 'k') {
-		shift_left_factor = 10;
-		value[last] = '\0';
-	}
-	if (value[last] == 'M' || value[last] == 'm') {
-		shift_left_factor = 20;
-		value[last] = '\0';
-	}
-	if (value[last] == 'G' || value[last] == 'g') {
-		shift_left_factor = 30;
-		value[last] = '\0';
-	}
-
-	if (kstrtoint(value, base, &_res))
-		ret = -EINVAL;
-	kfree(value);
-	*res = _res << shift_left_factor;
-	return ret;
-}
-
-static int
-suffix_kstrtoull(
-	const char		*s,
-	unsigned int		base,
-	unsigned long long	*res)
-{
-	int			last, shift_left_factor = 0;
-	unsigned long long	_res;
-	char			*value;
-	int			ret = 0;
-
-	value = kstrdup(s, GFP_KERNEL);
-	if (!value)
-		return -ENOMEM;
-
-	last = strlen(value) - 1;
-	if (value[last] == 'K' || value[last] == 'k') {
-		shift_left_factor = 10;
-		value[last] = '\0';
-	}
-	if (value[last] == 'M' || value[last] == 'm') {
-		shift_left_factor = 20;
-		value[last] = '\0';
-	}
-	if (value[last] == 'G' || value[last] == 'g') {
-		shift_left_factor = 30;
-		value[last] = '\0';
-	}
-
-	if (kstrtoull(value, base, &_res))
-		ret = -EINVAL;
-	kfree(value);
-	*res = _res << shift_left_factor;
-	return ret;
-}
-
 static inline void
 xfs_fs_warn_deprecated(
 	struct fs_context	*fc,
@@ -1427,8 +1356,8 @@ xfs_fs_parse_param(
 {
 	struct xfs_mount	*parsing_mp = fc->s_fs_info;
 	struct fs_parse_result	result;
-	int			size = 0;
 	int			opt;
+	unsigned long long	val;
 
 	BUILD_BUG_ON(XFS_QFLAGS_MNTOPTS & XFS_MOUNT_QUOTA_ALL);
 
@@ -1444,8 +1373,19 @@ xfs_fs_parse_param(
 		parsing_mp->m_logbufs = result.uint_32;
 		return 0;
 	case Opt_logbsize:
-		if (suffix_kstrtoint(param->string, 10, &parsing_mp->m_logbsize))
+		val = memvalue(param->string);
+		if (val == ULLONG_MAX)
 			return -EINVAL;
+		if (val != 0 &&
+		    (val < XLOG_MIN_RECORD_BSIZE ||
+		     val > XLOG_MAX_RECORD_BSIZE ||
+		     !is_power_of_2(val))) {
+			xfs_warn(parsing_mp,
+				 "invalid logbsize %llu: not a power-of-two in [%u..%u]",
+				 val, XLOG_MIN_RECORD_BSIZE, XLOG_MAX_RECORD_BSIZE);
+			return -EINVAL;
+		}
+		parsing_mp->m_logbsize = val;
 		return 0;
 	case Opt_logdev:
 		kfree(parsing_mp->m_logname);
@@ -1460,9 +1400,18 @@ xfs_fs_parse_param(
 			return -ENOMEM;
 		return 0;
 	case Opt_allocsize:
-		if (suffix_kstrtoint(param->string, 10, &size))
+		val = memvalue(param->string);
+		if (val == ULLONG_MAX)
+			return -EINVAL;
+		if (val < (1ULL << XFS_MIN_IO_LOG) ||
+		    val > (1ULL << XFS_MAX_IO_LOG) ||
+		    !is_power_of_2(val)) {
+			xfs_warn(parsing_mp,
+				 "invalid allocsize %llu: not a power-of-two in [%u..%u]",
+				 val, 1 << XFS_MIN_IO_LOG, 1 << XFS_MAX_IO_LOG);
 			return -EINVAL;
-		parsing_mp->m_allocsize_log = ffs(size) - 1;
+		}
+		parsing_mp->m_allocsize_log = ffs(val) - 1;
 		parsing_mp->m_features |= XFS_FEAT_ALLOCSIZE;
 		return 0;
 	case Opt_grpid:
@@ -1570,12 +1519,13 @@ xfs_fs_parse_param(
 		parsing_mp->m_features |= XFS_FEAT_NOLIFETIME;
 		return 0;
 	case Opt_max_atomic_write:
-		if (suffix_kstrtoull(param->string, 10,
-				     &parsing_mp->m_awu_max_bytes)) {
+		val = memvalue(param->string);
+		if (val == ULLONG_MAX) {
 			xfs_warn(parsing_mp,
  "max atomic write size must be positive integer");
 			return -EINVAL;
 		}
+		parsing_mp->m_awu_max_bytes = val;
 		return 0;
 	default:
 		xfs_warn(parsing_mp, "unknown mount option [%s].", param->key);
@@ -1629,25 +1579,6 @@ xfs_fs_validate_params(
 		return -EINVAL;
 	}
 
-	if (mp->m_logbsize != -1 &&
-	    mp->m_logbsize !=  0 &&
-	    (mp->m_logbsize < XLOG_MIN_RECORD_BSIZE ||
-	     mp->m_logbsize > XLOG_MAX_RECORD_BSIZE ||
-	     !is_power_of_2(mp->m_logbsize))) {
-		xfs_warn(mp,
-			"invalid logbufsize: %d [not 16k,32k,64k,128k or 256k]",
-			mp->m_logbsize);
-		return -EINVAL;
-	}
-
-	if (xfs_has_allocsize(mp) &&
-	    (mp->m_allocsize_log > XFS_MAX_IO_LOG ||
-	     mp->m_allocsize_log < XFS_MIN_IO_LOG)) {
-		xfs_warn(mp, "invalid log iosize: %d [not %d-%d]",
-			mp->m_allocsize_log, XFS_MIN_IO_LOG, XFS_MAX_IO_LOG);
-		return -EINVAL;
-	}
-
 	return 0;
 }
 
-- 
2.52.0

Re: [PATCH] lib: introduce simple error-checking wrapper for memparse()

[Andrew Morton]

On Wed,  7 Jan 2026 21:36:13 +0300 Dmitry Antipov <dmantipov@yandex.ru> wrote:

> Introduce 'memvalue()' which uses 'memparse()' to parse a string with
> optional memory suffix into a number and returns this number or ULLONG_MAX
> if the number is negative or an unrecognized character was encountered.

I'm not understanding why negative numbers get this treatment - could
you please add the reasoning to the code comment?

Presumably it's because memvalue() returns ULL, presumably because
memparse() returns ULL?  Maybe that's all wrong, and memparse() should
have returned LL - negative numbers are a bit odd, but why deny that
option.  With the new memvalue() we get to partially address that?

Re: [PATCH] lib: introduce simple error-checking wrapper for memparse()

[Andy Shevchenko]

On Wed, Jan 07, 2026 at 09:36:13PM +0300, Dmitry Antipov wrote:
> Introduce 'memvalue()' which uses 'memparse()' to parse a string with
> optional memory suffix into a number and returns this number or ULLONG_MAX
> if the number is negative or an unrecognized character was encountered.

Reading the second patch in the series I do not think this one even needed. The
problem in the original code is that

	int *res, _res;
	...
	*res = _res << something;

This is a UB for _res < 0. So, the code should never handle negative numbers to
begin with. That said the existing memparse() can be used directly.

If I missed something, it's because the commit message here is poorly written
in regard to negative number parsing.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] lib: introduce simple error-checking wrapper for memparse()

[Andy Shevchenko]

On Wed, Jan 07, 2026 at 09:22:39PM +0200, Andy Shevchenko wrote:
> On Wed, Jan 07, 2026 at 09:36:13PM +0300, Dmitry Antipov wrote:
> > Introduce 'memvalue()' which uses 'memparse()' to parse a string with
> > optional memory suffix into a number and returns this number or ULLONG_MAX
> > if the number is negative or an unrecognized character was encountered.
> 
> Reading the second patch in the series I do not think this one even needed. The
> problem in the original code is that
> 
> 	int *res, _res;
> 	...
> 	*res = _res << something;
> 
> This is a UB for _res < 0. So, the code should never handle negative numbers to
> begin with. That said the existing memparse() can be used directly.
> 
> If I missed something, it's because the commit message here is poorly written
> in regard to negative number parsing.

Also note, when do a series, make sure you have the same version for all
patches and the cover letter provided. You can use `b4` tool for that or
supply `git format-patch -v<X> --cover-letter ...`, where <X> is the desired
version number.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] lib: introduce simple error-checking wrapper for memparse()

[Kees Cook]

On Wed, Jan 07, 2026 at 09:36:13PM +0300, Dmitry Antipov wrote:
> Introduce 'memvalue()' which uses 'memparse()' to parse a string with
> optional memory suffix into a number and returns this number or ULLONG_MAX
> if the number is negative or an unrecognized character was encountered.

ULLONG_MAX is a valid address, though. I don't like this as an error
canary. How about using __must_check with 0/negative return value and
put the parsed value into a passed-by-reference variable instead? This
has the benefit of also performing type checking on the variable so that
a returned value can never be truncated accidentally:


int __must_check memvalue(const char *ptr, unsigned long long *addr);

-- 
Kees Cook

Re: [PATCH] lib: introduce simple error-checking wrapper for memparse()

[Christoph Hellwig]

On Wed, Jan 07, 2026 at 12:08:54PM -0800, Kees Cook wrote:
> On Wed, Jan 07, 2026 at 09:36:13PM +0300, Dmitry Antipov wrote:
> > Introduce 'memvalue()' which uses 'memparse()' to parse a string with
> > optional memory suffix into a number and returns this number or ULLONG_MAX
> > if the number is negative or an unrecognized character was encountered.
> 
> ULLONG_MAX is a valid address, though. I don't like this as an error
> canary. How about using __must_check with 0/negative return value and
> put the parsed value into a passed-by-reference variable instead? This
> has the benefit of also performing type checking on the variable so that
> a returned value can never be truncated accidentally:
> 
> 
> int __must_check memvalue(const char *ptr, unsigned long long *addr);

That does sound pretty nice as an API.  Should addr better be an
u64 instead of unsligned long long, though?

Re: [PATCH] lib: introduce simple error-checking wrapper for memparse()

[Dmitry Antipov]

On Wed, 2026-01-07 at 10:48 -0800, Andrew Morton wrote:

> I'm not understanding why negative numbers get this treatment - could
> you please add the reasoning to the code comment?

Hm. I suppose that memvalue() may (and hopefully will) be used to parse
and return an amount of "actually used" (for some particular task) memory,
allowing 0 with possible "use some default value" treatment in the caller.
Negative values just introduces some confusion (and possible weird effects
caused by an erroneous conversion of negative values to huge positive ones). 
If someone needs some special treatment of, say, "memsize=-32M" somewhere,
there should be a kind of an extra handling beyond memvalue(). 

> Presumably it's because memvalue() returns ULL, presumably because
> memparse() returns ULL?  Maybe that's all wrong, and memparse() should
> have returned LL - negative numbers are a bit odd, but why deny that
> option.  With the new memvalue() we get to partially address that?

I would rather try

int __must_check memvalue(const char *ptr, unsigned long long *addr);

as suggested by Kees. 

Dmitry