Re: [PATCH v9 2/9] lib: vsprintf: export simple_strntoull() in a safe prototype

[Petr Mladek <pmladek@suse.com>]

On Fri 2026-03-20 16:27:27, Rodrigo Alencar via B4 Relay wrote:
> From: Rodrigo Alencar <rodrigo.alencar@analog.com>
> 
> Expose simple_strntoull(), by addressing its FIXME, i.e. its prototype is
> slightly changed so that -ERANGE or -EINVAL can be evaluated by the user.
> Flow of the function is not changed and error value is returned in the
> end. Unsafe internal wrapper is created to reduce amount of changes.
> 
> --- a/include/linux/kstrtox.h
> +++ b/include/linux/kstrtox.h
> @@ -148,4 +148,8 @@ extern long simple_strtol(const char *,char **,unsigned int);
>  extern unsigned long long simple_strtoull(const char *,char **,unsigned int);
>  extern long long simple_strtoll(const char *,char **,unsigned int);
>  
> +extern ssize_t __must_check simple_strntoull(const char *startp, const char **endp,
> +					     unsigned int base, size_t max_chars,
> +					     unsigned long long *res);

Sigh, naming is hard. I personally find it a bit confusing that the
name is too similar to the unsafe API.

IMHO, the semantic of the new API is closer to kstrtoull().
It just limits the size, so I would call it kstrntoull().

Also I would use int as the return parameter, see below.


>  #endif	/* _LINUX_KSTRTOX_H */
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 800b8ac49f53..6fb880f4013b 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -75,25 +75,66 @@ enum hash_pointers_policy {
>  };
>  static enum hash_pointers_policy hash_pointers_mode __initdata;
>  
> +/**
> + * simple_strntoull - convert a string to an unsigned long long with a character limit
> + *
> + * @startp: The start of the string
> + * @endp: A pointer to the end of the parsed string will be placed here

I would write:

  * @endp: A pointer to the end of the parsed string (output)

> + * @base: The number base to use
> + * @max_chars: The maximum number of characters to parse
> + * @res: Where to write the result of the conversion on success

Nit: I would omit "on success" *res value is set to 0 on failure.
     Instead, I would write:

  * @res: Result of the conversion (output)

> + *
> + * Returns amount of processed characters on success, -ERANGE on overflow and
> + * -EINVAL on parsing error.
> + */
>  noinline
> -static unsigned long long simple_strntoull(const char *startp, char **endp, unsigned int base, size_t max_chars)
> +ssize_t simple_strntoull(const char *startp, const char **endp,
> +			 unsigned int base, size_t max_chars,
> +			 unsigned long long *res)

It might be enoungh to use "int" for the return value. The number
of proceed characters is pretty limited by definition. And it
would be similar to vsnprintf(), kstrtoull(), ...

I guess that you wanted to match the "size_t max_chars" parameter.
It makes some sense as well.

Please, use "int" especially if we agreed to call the new API
kstrntoull().

>  {
>  	const char *cp;
> -	unsigned long long result = 0ULL;
>  	size_t prefix_chars;
>  	unsigned int rv;
> +	ssize_t ret;
>  
>  	cp = _parse_integer_fixup_radix(startp, &base);
>  	prefix_chars = cp - startp;
>  	if (prefix_chars < max_chars) {
> -		rv = _parse_integer_limit(cp, base, &result, max_chars - prefix_chars);
> -		/* FIXME */
> +		rv = _parse_integer_limit(cp, base, res, max_chars - prefix_chars);
> +		if (rv & KSTRTOX_OVERFLOW)
> +			ret = -ERANGE;
> +		else if (rv == 0)
> +			ret = -EINVAL;
> +		else
> +			ret = rv + prefix_chars;
>  		cp += (rv & ~KSTRTOX_OVERFLOW);
>  	} else {
>  		/* Field too short for prefix + digit, skip over without converting */
>  		cp = startp + max_chars;
> +		ret = -EINVAL;
> +		*res = 0ULL;
>  	}
>  
> +	if (endp)
> +		*endp = cp;
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(simple_strntoull);
> +
> +/* unsafe_strntoull ignores simple_strntoull() return value and endp const qualifier */
> +inline
> +static unsigned long long unsafe_strntoull(const char *startp, char **endp,
> +					   unsigned int base, size_t max_chars)
> +{
> +	unsigned long long result;
> +	const char *cp;
> +
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wunused-result"
> +	simple_strntoull(startp, &cp, base, max_chars, &result);
> +#pragma GCC diagnostic pop
> +
>  	if (endp)
>  		*endp = (char *)cp;

IMHO, we do not need local "cp". We could simply pass the endp
to the new simple_strntoull. Or do I miss anything?

Best Regards,
Petr