[PATCH v3 1/2] lib: introduce simple error-checking wrapper for memparse()

[PATCH v3 1/2] 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 non-negative number. If parsing
has succeeded, returns 0 and stores the result at the location
specified by the second argument. Otherwise returns -EINVAL and
leaves the location untouched.

Suggested-by: Christoph Hellwig <hch@infradead.org>
Suggested-by: Kees Cook <kees@kernel.org>
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v3: adjust as suggested by Kees and bump version to match the series
---
 include/linux/string.h |  1 +
 lib/cmdline.c          | 26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index 1b564c36d721..470c7051c58b 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 int __must_check memvalue(const char *ptr, unsigned long long *valptr);
 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..cf81c6363f6c 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -190,6 +190,32 @@ unsigned long long memparse(const char *ptr, char **retptr)
 }
 EXPORT_SYMBOL(memparse);
 
+/**
+ *	memvalue -  Wrap memparse() with simple error detection
+ *	@ptr: Where parse begins
+ *	@valptr: Where to store result
+ *
+ *	Unconditionally returns -EINVAL for a presumably negative value.
+ *	Otherwise uses memparse() to parse a string into a number stored
+ *	at @valptr and returns 0 or -EINVAL if an unrecognized character
+ *	was encountered. For a non-zero return value, memory at @valptr
+ *	is left untouched.
+ */
+int __must_check memvalue(const char *ptr, unsigned long long *valptr)
+{
+	unsigned long long ret;
+	char *end;
+
+	if (*ptr == '-')
+		return -EINVAL;
+	ret = memparse(ptr, &end);
+	if (*end)
+		return -EINVAL;
+	*valptr = ret;
+	return 0;
+}
+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 v3 2/2] 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>
---
v3: adjust to match new 'memvalue()' syntax
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 | 127 +++++++++++----------------------------------
 1 file changed, 29 insertions(+), 98 deletions(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index bc71aa9dcee8..4707cd3acf73 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;
+	int			opt, ret;
+	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))
+		ret = memvalue(param->string, &val);
+		if (ret)
+			return ret;
+		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))
+		ret = memvalue(param->string, &val);
+		if (ret)
+			return ret;
+		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)) {
+		ret = memvalue(param->string, &val);
+		if (ret) {
 			xfs_warn(parsing_mp,
  "max atomic write size must be positive integer");
-			return -EINVAL;
+			return ret;
 		}
+		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 v3 1/2] lib: introduce simple error-checking wrapper for memparse()

[Kees Cook]

On Thu, Jan 08, 2026 at 07:52:15PM +0300, Dmitry Antipov wrote:
> Introduce 'memvalue()' which uses 'memparse()' to parse a string
> with optional memory suffix into a non-negative number. If parsing
> has succeeded, returns 0 and stores the result at the location
> specified by the second argument. Otherwise returns -EINVAL and
> leaves the location untouched.
> 
> Suggested-by: Christoph Hellwig <hch@infradead.org>
> Suggested-by: Kees Cook <kees@kernel.org>
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>

LGTM, thanks!

Reviewed-by: Kees Cook <kees@kernel.org>

-- 
Kees Cook

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

[Andrew Morton]

On Thu, 8 Jan 2026 09:06:49 -0800 Kees Cook <kees@kernel.org> wrote:

> On Thu, Jan 08, 2026 at 07:52:15PM +0300, Dmitry Antipov wrote:
> > Introduce 'memvalue()' which uses 'memparse()' to parse a string
> > with optional memory suffix into a non-negative number. If parsing
> > has succeeded, returns 0 and stores the result at the location
> > specified by the second argument. Otherwise returns -EINVAL and
> > leaves the location untouched.
> > 
> > Suggested-by: Christoph Hellwig <hch@infradead.org>
> > Suggested-by: Kees Cook <kees@kernel.org>
> > Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> 
> LGTM, thanks!
> 
> Reviewed-by: Kees Cook <kees@kernel.org>

Thanks, I'll add both these to mm.git's mm-nonmm-unstable branch for
testing.

If XFS people would prefer to take [2/2] via the xfs tree then please
lmk and I'll send it over when [1/2] is upstreamed.  Or we can take
both patches via the xfs tree.  Or something.  Sending out an acked-by:
would be simplest!

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

[Kees Cook]

On Thu, Jan 08, 2026 at 09:42:42AM -0800, Andrew Morton wrote:
> On Thu, 8 Jan 2026 09:06:49 -0800 Kees Cook <kees@kernel.org> wrote:
> 
> > On Thu, Jan 08, 2026 at 07:52:15PM +0300, Dmitry Antipov wrote:
> > > Introduce 'memvalue()' which uses 'memparse()' to parse a string
> > > with optional memory suffix into a non-negative number. If parsing
> > > has succeeded, returns 0 and stores the result at the location
> > > specified by the second argument. Otherwise returns -EINVAL and
> > > leaves the location untouched.
> > > 
> > > Suggested-by: Christoph Hellwig <hch@infradead.org>
> > > Suggested-by: Kees Cook <kees@kernel.org>
> > > Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> > 
> > LGTM, thanks!
> > 
> > Reviewed-by: Kees Cook <kees@kernel.org>
> 
> Thanks, I'll add both these to mm.git's mm-nonmm-unstable branch for
> testing.
> 
> If XFS people would prefer to take [2/2] via the xfs tree then please
> lmk and I'll send it over when [1/2] is upstreamed.  Or we can take
> both patches via the xfs tree.  Or something.  Sending out an acked-by:
> would be simplest!

I assumed this would go via xfs tree, but I'm happy to do whatever.

-- 
Kees Cook

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

[Carlos Maiolino]

On Thu, Jan 08, 2026 at 09:55:27AM -0800, Kees Cook wrote:
> On Thu, Jan 08, 2026 at 09:42:42AM -0800, Andrew Morton wrote:
> > On Thu, 8 Jan 2026 09:06:49 -0800 Kees Cook <kees@kernel.org> wrote:
> > 
> > > On Thu, Jan 08, 2026 at 07:52:15PM +0300, Dmitry Antipov wrote:
> > > > Introduce 'memvalue()' which uses 'memparse()' to parse a string
> > > > with optional memory suffix into a non-negative number. If parsing
> > > > has succeeded, returns 0 and stores the result at the location
> > > > specified by the second argument. Otherwise returns -EINVAL and
> > > > leaves the location untouched.
> > > > 
> > > > Suggested-by: Christoph Hellwig <hch@infradead.org>
> > > > Suggested-by: Kees Cook <kees@kernel.org>
> > > > Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> > > 
> > > LGTM, thanks!
> > > 
> > > Reviewed-by: Kees Cook <kees@kernel.org>
> > 
> > Thanks, I'll add both these to mm.git's mm-nonmm-unstable branch for
> > testing.
> > 
> > If XFS people would prefer to take [2/2] via the xfs tree then please
> > lmk and I'll send it over when [1/2] is upstreamed.  Or we can take
> > both patches via the xfs tree.  Or something.  Sending out an acked-by:
> > would be simplest!
> 
> I assumed this would go via xfs tree, but I'm happy to do whatever.

Particularly I find it much simpler to have those inter-dependent
patches to go through in a single series via a single tree, instead of
breaking them apart and create merge dependencies.

As long as xfs list is in the loop I particularly don't mind. Thanks for
pulling it Andrew.

Carlos

> 
> -- 
> Kees Cook
> 

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

[Andy Shevchenko]

On Thu, Jan 08, 2026 at 07:52:15PM +0300, Dmitry Antipov wrote:
> Introduce 'memvalue()' which uses 'memparse()' to parse a string
> with optional memory suffix into a non-negative number. If parsing
> has succeeded, returns 0 and stores the result at the location
> specified by the second argument. Otherwise returns -EINVAL and
> leaves the location untouched.

...

> +/**
> + *	memvalue -  Wrap memparse() with simple error detection
> + *	@ptr: Where parse begins
> + *	@valptr: Where to store result
> + *
> + *	Unconditionally returns -EINVAL for a presumably negative value.
> + *	Otherwise uses memparse() to parse a string into a number stored
> + *	at @valptr and returns 0 or -EINVAL if an unrecognized character
> + *	was encountered. For a non-zero return value, memory at @valptr
> + *	is left untouched.
> + */

There are two problems with this kernel-doc:
1) inherited one with strange indentation;
2) missing Return section (run kernel-doc validator with -Wreturn, for example).

-- 
With Best Regards,
Andy Shevchenko

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

[Dmitry Antipov]

On Thu, 2026-01-08 at 22:05 +0200, Andy Shevchenko wrote:

> 1) inherited one with strange indentation;

Hm...where? AFAICS everything is properly indented with TABs.

> 2) missing Return section (run kernel-doc validator with -Wreturn,
> for example).

Good point. Should checkpatch.pl call kernel-doc (always or perhaps
if requested using command-line option)?

OTOH 1) lib/cmdline.c violates kernel-doc -Wreturn almost everywhere
:-( and 2) IIUC this patch is already queued by Andrew. I would
prefer to fix kernel-doc glitches immediately after memvalue() and
its first real use case (presumably XFS) both reaches an upstream.

Dmitry

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

[Andy Shevchenko]

On Fri, Jan 09, 2026 at 02:41:55PM +0300, Dmitry Antipov wrote:
> On Thu, 2026-01-08 at 22:05 +0200, Andy Shevchenko wrote:
> 
> > 1) inherited one with strange indentation;
> 
> Hm...where? AFAICS everything is properly indented with TABs.

Should be with one space, and not one tab.

> > 2) missing Return section (run kernel-doc validator with -Wreturn,
> > for example).
> 
> Good point. Should checkpatch.pl call kernel-doc (always or perhaps
> if requested using command-line option)?
> 
> OTOH 1) lib/cmdline.c violates kernel-doc -Wreturn almost everywhere
> :-( and 2) IIUC this patch is already queued by Andrew. I would
> prefer to fix kernel-doc glitches immediately after memvalue() and
> its first real use case (presumably XFS) both reaches an upstream.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Jan 09, 2026 at 02:41:55PM +0300, Dmitry Antipov wrote:
> On Thu, 2026-01-08 at 22:05 +0200, Andy Shevchenko wrote:

...

> > 2) missing Return section (run kernel-doc validator with -Wreturn,
> > for example).
> 
> Good point. Should checkpatch.pl call kernel-doc (always or perhaps
> if requested using command-line option)?
> 
> OTOH 1) lib/cmdline.c violates kernel-doc -Wreturn almost everywhere
> :-( and 2) IIUC this patch is already queued by Andrew.

Andrew's workflow allows folding / squashing patches.

> I would
> prefer to fix kernel-doc glitches immediately after memvalue() and
> its first real use case (presumably XFS) both reaches an upstream.

Logically we should fix existing problems first and then do not add more
(technical debt).


-- 
With Best Regards,
Andy Shevchenko

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

[Andrew Morton]

On Fri, 9 Jan 2026 19:11:31 +0200 Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Fri, Jan 09, 2026 at 02:41:55PM +0300, Dmitry Antipov wrote:
> > On Thu, 2026-01-08 at 22:05 +0200, Andy Shevchenko wrote:
> 
> ...
> 
> > > 2) missing Return section (run kernel-doc validator with -Wreturn,
> > > for example).
> > 
> > Good point. Should checkpatch.pl call kernel-doc (always or perhaps
> > if requested using command-line option)?
> > 
> > OTOH 1) lib/cmdline.c violates kernel-doc -Wreturn almost everywhere
> > :-( and 2) IIUC this patch is already queued by Andrew.
> 
> Andrew's workflow allows folding / squashing patches.

And replacing and dropping and reordering...

Please, just do whatever you feel most appropriate.  I'll cope ;)

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

[Andy Shevchenko]

On Thu, Jan 08, 2026 at 07:52:15PM +0300, Dmitry Antipov wrote:
> Introduce 'memvalue()' which uses 'memparse()' to parse a string
> with optional memory suffix into a non-negative number. If parsing
> has succeeded, returns 0 and stores the result at the location
> specified by the second argument. Otherwise returns -EINVAL and
> leaves the location untouched.

...

> +int __must_check memvalue(const char *ptr, unsigned long long *valptr)
> +{
> +	unsigned long long ret;
> +	char *end;
> +
> +	if (*ptr == '-')
> +		return -EINVAL;

Hmm... Why not -ERANGE (IIRC this what kstrto*() returns when it doesn't match
the given range).

> +	ret = memparse(ptr, &end);
> +	if (*end)
> +		return -EINVAL;
> +	*valptr = ret;
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko

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

[Dmitry Antipov]

On Thu, 2026-01-08 at 22:10 +0200, Andy Shevchenko wrote:

> Hmm... Why not -ERANGE (IIRC this what kstrto*() returns when it doesn't match
> the given range).

Well, I've always treated -ERANGE closer to formal math, i.e. "return -ERANGE
if X is not in [A:B)", rather than using it to denote something which makes
no practical sense in some particular context, like negative amount of memory
or negative string length.

Dmitry

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

[Andy Shevchenko]

On Fri, Jan 09, 2026 at 02:05:41PM +0300, Dmitry Antipov wrote:
> On Thu, 2026-01-08 at 22:10 +0200, Andy Shevchenko wrote:
> 
> > Hmm... Why not -ERANGE (IIRC this what kstrto*() returns when it doesn't match
> > the given range).
> 
> Well, I've always treated -ERANGE closer to formal math, i.e. "return -ERANGE
> if X is not in [A:B)", rather than using it to denote something which makes
> no practical sense in some particular context, like negative amount of memory
> or negative string length.

Well, I'm not talking about your preferences, I'm talking about the change you
made. First of all,

	        if (rv & KSTRTOX_OVERFLOW)
                return -ERANGE;

is in the original kstrtox implementation that suggests that the code is
already established for the purpose. Second, the bigger issue, the two
semantically different error paths return the same error code when it can
be easily avoided.

-- 
With Best Regards,
Andy Shevchenko

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

[Andrew Morton]

On Thu,  8 Jan 2026 19:52:15 +0300 Dmitry Antipov <dmantipov@yandex.ru> wrote:

> Introduce 'memvalue()' which uses 'memparse()' to parse a string
> with optional memory suffix into a non-negative number. If parsing
> has succeeded, returns 0 and stores the result at the location
> specified by the second argument. Otherwise returns -EINVAL and
> leaves the location untouched.

Where do we stand with this patchset now?  I saw a lot of discussion but
not a lot of clarity.  Thanks.

Unrelated:

> --- 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 int __must_check memvalue(const char *ptr, unsigned long long *valptr);

Sensible.

>  EXPORT_SYMBOL(memparse);
> +EXPORT_SYMBOL(memvalue);

memparse is used in many places.  Seems inappropriate that these things
are implemented in lib/cmdline.c?

[PATCH v4 1/3] 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 non-negative number. If parsing
has succeeded, returns 0 and stores the result at the location
specified by the second argument. Otherwise returns -EINVAL and
leaves the location untouched.

Suggested-by: Christoph Hellwig <hch@infradead.org>
Suggested-by: Kees Cook <kees@kernel.org>
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v4: adjust documentation to match kernel-doc -Wreturn's style
v3: adjust as suggested by Kees and bump version to match the series
---
 include/linux/string.h |  1 +
 lib/cmdline.c          | 26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index 1b564c36d721..470c7051c58b 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 int __must_check memvalue(const char *ptr, unsigned long long *valptr);
 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..28048d05eb35 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -190,6 +190,32 @@ unsigned long long memparse(const char *ptr, char **retptr)
 }
 EXPORT_SYMBOL(memparse);
 
+/**
+ *	memvalue -  Wrap memparse() with simple error detection
+ *	@ptr: Where parse begins
+ *	@valptr: Where to store result
+ *
+ *	Uses memparse() to parse a string into a number stored at
+ *	@valptr, leaving memory at @valptr untouched in case of error.
+ *
+ *	Return: -EINVAL for a presumably negative value or if an
+ *	unrecognized character was encountered, and 0 otherwise.
+ */
+int __must_check memvalue(const char *ptr, unsigned long long *valptr)
+{
+	unsigned long long ret;
+	char *end;
+
+	if (*ptr == '-')
+		return -EINVAL;
+	ret = memparse(ptr, &end);
+	if (*end)
+		return -EINVAL;
+	*valptr = ret;
+	return 0;
+}
+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 v4 2/3] lib: fix a few comments to match kernel-doc -Wreturn style

[Dmitry Antipov]

Fix 'get_option()', 'memparse()' and 'parse_option_str()' comments
to match the commonly used style as suggested by kernel-doc -Wreturn.

Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v4: initial version to join the series
---
 lib/cmdline.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lib/cmdline.c b/lib/cmdline.c
index 28048d05eb35..6922ddd90e7c 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -43,7 +43,7 @@ static int get_range(char **str, int *pint, int n)
  *	When @pint is NULL the function can be used as a validator of
  *	the current option in the string.
  *
- *	Return values:
+ *	Return:
  *	0 - no int in string
  *	1 - int found, no subsequent comma
  *	2 - int found including a subsequent comma
@@ -145,6 +145,9 @@ EXPORT_SYMBOL(get_options);
  *
  *	Parses a string into a number.  The number stored at @ptr is
  *	potentially suffixed with K, M, G, T, P, E.
+ *
+ *	Return: The value as recognized by simple_strtoull() multiplied
+ *	by the value as specified by suffix, if any.
  */
 
 unsigned long long memparse(const char *ptr, char **retptr)
@@ -224,7 +227,7 @@ EXPORT_SYMBOL(memvalue);
  *	This function parses a string containing a comma-separated list of
  *	strings like a=b,c.
  *
- *	Return true if there's such option in the string, or return false.
+ *	Return: True if there's such option in the string or false otherwise.
  */
 bool parse_option_str(const char *str, const char *option)
 {
-- 
2.52.0

Re: [PATCH v4 2/3] lib: fix a few comments to match kernel-doc -Wreturn style

[Andy Shevchenko]

On Tue, Jan 20, 2026 at 05:12:28PM +0300, Dmitry Antipov wrote:
> Fix 'get_option()', 'memparse()' and 'parse_option_str()' comments
> to match the commonly used style as suggested by kernel-doc -Wreturn.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

Thanks for doing this change, I think it should go first for the consistency's
sake.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 3/3] 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>
---
v4: bump version to match the series
v3: adjust to match new 'memvalue()' syntax
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 | 127 +++++++++++----------------------------------
 1 file changed, 29 insertions(+), 98 deletions(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index bc71aa9dcee8..4707cd3acf73 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;
+	int			opt, ret;
+	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))
+		ret = memvalue(param->string, &val);
+		if (ret)
+			return ret;
+		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))
+		ret = memvalue(param->string, &val);
+		if (ret)
+			return ret;
+		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)) {
+		ret = memvalue(param->string, &val);
+		if (ret) {
 			xfs_warn(parsing_mp,
  "max atomic write size must be positive integer");
-			return -EINVAL;
+			return ret;
 		}
+		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 v4 3/3] xfs: adjust handling of a few numerical mount options

[Andy Shevchenko]

On Tue, Jan 20, 2026 at 05:12:29PM +0300, Dmitry Antipov wrote:
> 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()'.

...

> -	if (kstrtoint(value, base, &_res))
> -		ret = -EINVAL;
> -	kfree(value);
> -	*res = _res << shift_left_factor;
> -	return ret;

_res is int, if negative the above is UB in accordance with C standard.
So, if ever this code runs to the shifting left negative numbers it goes
to a slippery slope (I think it works as intended, but...).

That said, I assume this code was never designed to get a negative value
to the _res.

With all this, I do not see the point of having a new API.
Also, where are the test cases for it?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 3/3] xfs: adjust handling of a few numerical mount options

[Dmitry Antipov]

On Tue, 2026-01-20 at 16:59 +0200, Andy Shevchenko wrote:

> With all this, I do not see the point of having a new API.
> Also, where are the test cases for it?

If there is no point, why worrying about tests?
Also, do you always communicate with the people
just like they're your (well-) paid personnel?

Dmitry

Re: [PATCH v4 3/3] xfs: adjust handling of a few numerical mount options

[Andy Shevchenko]

On Tue, Jan 20, 2026 at 07:57:50PM +0300, Dmitry Antipov wrote:
> On Tue, 2026-01-20 at 16:59 +0200, Andy Shevchenko wrote:
> 
> > With all this, I do not see the point of having a new API.
> > Also, where are the test cases for it?

> If there is no point, why worrying about tests?

I don't know yet if there is a point or not, I provided my view.
I think you know better than me the code in question. It might
be that I'm mistaken, and if so the good justification in the
(currently absent) cover letter may well help with that.

> Also, do you always communicate with the people
> just like they're your (well-) paid personnel?

What do you mean? Test cases is the requirement for the new APIs
added to the lib/. It's really should be regular practice for
the code development independently on the project. If you think
frustrated by this, I can tell you that I was more than once in
the past in the same situation until I learnt it very well and
now when I submit anything to the lib I always add test cases.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 3/3] xfs: adjust handling of a few numerical mount options

[Darrick J. Wong]

On Tue, Jan 20, 2026 at 11:49:35PM +0200, Andy Shevchenko wrote:
> On Tue, Jan 20, 2026 at 07:57:50PM +0300, Dmitry Antipov wrote:
> > On Tue, 2026-01-20 at 16:59 +0200, Andy Shevchenko wrote:
> > 
> > > With all this, I do not see the point of having a new API.
> > > Also, where are the test cases for it?
> 
> > If there is no point, why worrying about tests?
> 
> I don't know yet if there is a point or not, I provided my view.
> I think you know better than me the code in question. It might
> be that I'm mistaken, and if so the good justification in the
> (currently absent) cover letter may well help with that.
> 
> > Also, do you always communicate with the people
> > just like they're your (well-) paid personnel?
> 
> What do you mean? Test cases is the requirement for the new APIs
> added to the lib/. It's really should be regular practice for
> the code development independently on the project. If you think
> frustrated by this, I can tell you that I was more than once in
> the past in the same situation until I learnt it very well and
> now when I submit anything to the lib I always add test cases.

Yes.  Common code needs to have a rigorous self test suite, because I
see no point in replacing inadequately tested bespoke parsing code with
inadequately tested common parsing code.

--D

> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 
> 

Re: [PATCH v4 3/3] xfs: adjust handling of a few numerical mount options

[Dmitry Antipov]

On Tue, 2026-01-20 at 14:55 -0800, Darrick J. Wong wrote:

> Yes.  Common code needs to have a rigorous self test suite, because I
> see no point in replacing inadequately tested bespoke parsing code with
> inadequately tested common parsing code.

Nothing to disagree but:

1) My experience clearly shows that it takes a few patch submission
iterations and a bunch of e-mails just to notice that the tests are
mandatory for lib/ stuff. If it is really a requirement, it is worth
to be mentioned somewhere under Documentation/process at least.

2) I've traced memparse() back to 2006 at least, and (if I didn't miss
something) there is no actual tests for it since them. And it's hard to
see a point in testing memvalue() prior to testing its actual workhorse.

Dmitry

Re: [PATCH v4 3/3] xfs: adjust handling of a few numerical mount options

[Andy Shevchenko]

On Wed, Jan 21, 2026 at 08:21:42AM +0300, Dmitry Antipov wrote:
> On Tue, 2026-01-20 at 14:55 -0800, Darrick J. Wong wrote:
> 
> > Yes.  Common code needs to have a rigorous self test suite, because I
> > see no point in replacing inadequately tested bespoke parsing code with
> > inadequately tested common parsing code.
> 
> Nothing to disagree but:
> 
> 1) My experience clearly shows that it takes a few patch submission
> iterations and a bunch of e-mails just to notice that the tests are
> mandatory for lib/ stuff. If it is really a requirement, it is worth
> to be mentioned somewhere under Documentation/process at least.

Feel free to submit an update! :-)

Sorry that I mentioned it one or two versions later than I should have.

> 2) I've traced memparse() back to 2006 at least, and (if I didn't miss
> something) there is no actual tests for it since them. And it's hard to
> see a point in testing memvalue() prior to testing its actual workhorse.

Yes, the historical code needs test cases. I added a few for get_option*()
for example before touching that code. So you're welcome to start test
cases for memparse(), I will appreciate that!

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Jan 20, 2026 at 05:12:27PM +0300, Dmitry Antipov wrote:
> Introduce 'memvalue()' which uses 'memparse()' to parse a string
> with optional memory suffix into a non-negative number. If parsing
> has succeeded, returns 0 and stores the result at the location
> specified by the second argument. Otherwise returns -EINVAL and
> leaves the location untouched.

...

> +/**
> + *	memvalue -  Wrap memparse() with simple error detection
> + *	@ptr: Where parse begins
> + *	@valptr: Where to store result
> + *
> + *	Uses memparse() to parse a string into a number stored at
> + *	@valptr, leaving memory at @valptr untouched in case of error.
> + *
> + *	Return: -EINVAL for a presumably negative value or if an
> + *	unrecognized character was encountered, and 0 otherwise.
> + */
> +int __must_check memvalue(const char *ptr, unsigned long long *valptr)
> +{
> +	unsigned long long ret;
> +	char *end;
> +
> +	if (*ptr == '-')
> +		return -EINVAL;
> +	ret = memparse(ptr, &end);
> +	if (*end)
> +		return -EINVAL;
> +	*valptr = ret;
> +	return 0;
> +}

My questions seem left unsettled:
- why -EINVAL in the first place and not -ERANGE in the first place;
- why do we need this patch _at all_ based on the how callers are
doing now (w.o. this change), i.o.w. why the memparse() can't be
used directly.


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Jan 20, 2026 at 04:45:39PM +0200, Andy Shevchenko wrote:
> On Tue, Jan 20, 2026 at 05:12:27PM +0300, Dmitry Antipov wrote:
> > Introduce 'memvalue()' which uses 'memparse()' to parse a string
> > with optional memory suffix into a non-negative number. If parsing
> > has succeeded, returns 0 and stores the result at the location
> > specified by the second argument. Otherwise returns -EINVAL and
> > leaves the location untouched.

Also this misses the cover letter to explain the motivation, changelog, etc.

-- 
With Best Regards,
Andy Shevchenko