In '_parse_integer_limit()', adjust native integer arithmetic
with near-to-overflow branch where 'check_mul_overflow()' and
'check_add_overflow()' are used to check whether an intermediate
result goes out of range, and denote such a case with ULLONG_MAX,
thus making the function more similar to standard C library's
'strtoull()'. Adjust comment to kernel-doc style as well.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v5: minor brace style adjustment
v4: restore plain integer arithmetic and use check_xxx_overflow()
on near-to-overflow branch only
v3: adjust commit message and comments as suggested by Andy
v2: initial version to join the series
---
lib/kstrtox.c | 39 ++++++++++++++++++++++++++-------------
1 file changed, 26 insertions(+), 13 deletions(-)
diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index bdde40cd69d7..8691f85cf2ce 100644
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c
@@ -39,20 +39,26 @@ const char *_parse_integer_fixup_radix(const char *s, unsigned int *base)
return s;
}
-/*
- * Convert non-negative integer string representation in explicitly given radix
- * to an integer. A maximum of max_chars characters will be converted.
+/**
+ * _parse_integer_limit - Convert integer string representation to an integer
+ * @s: Integer string representation
+ * @base: Radix
+ * @p: Where to store result
+ * @max_chars: Maximum amount of characters to convert
+ *
+ * Convert non-negative integer string representation in explicitly given
+ * radix to an integer. If overflow occurs, value at @p is set to ULLONG_MAX.
*
- * Return number of characters consumed maybe or-ed with overflow bit.
- * If overflow occurs, result integer (incorrect) is still returned.
+ * This function is the workhorse of other string conversion functions and it
+ * is discouraged to use it explicitly. Consider kstrto*() family instead.
*
- * Don't you dare use this function.
+ * Return: Number of characters consumed, maybe ORed with overflow bit
*/
noinline
unsigned int _parse_integer_limit(const char *s, unsigned int base, unsigned long long *p,
size_t max_chars)
{
- unsigned long long res;
+ unsigned long long tmp, res;
unsigned int rv;
res = 0;
@@ -72,14 +78,21 @@ unsigned int _parse_integer_limit(const char *s, unsigned int base, unsigned lon
if (val >= base)
break;
/*
- * Check for overflow only if we are within range of
- * it in the max base we support (16)
+ * Accumulate result if no overflow detected.
+ * Otherwise just consume valid characters.
*/
- if (unlikely(res & (~0ull << 60))) {
- if (res > div_u64(ULLONG_MAX - val, base))
- rv |= KSTRTOX_OVERFLOW;
+ if (likely(res != ULLONG_MAX)) {
+ if (unlikely(res & (~0ull << 60))) {
+ /* We're close to possible overflow. */
+ if (check_mul_overflow(res, base, &tmp) ||
+ check_add_overflow(tmp, val, &res)) {
+ res = ULLONG_MAX;
+ rv |= KSTRTOX_OVERFLOW;
+ }
+ } else {
+ res = res * base + val;
+ }
}
- res = res * base + val;
rv++;
s++;
}
--
2.52.0On Wed, 4 Feb 2026 16:57:13 +0300
Dmitry Antipov <dmantipov@yandex.ru> wrote:
> In '_parse_integer_limit()', adjust native integer arithmetic
> with near-to-overflow branch where 'check_mul_overflow()' and
> 'check_add_overflow()' are used to check whether an intermediate
> result goes out of range, and denote such a case with ULLONG_MAX,
> thus making the function more similar to standard C library's
> 'strtoull()'. Adjust comment to kernel-doc style as well.
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
> v5: minor brace style adjustment
> v4: restore plain integer arithmetic and use check_xxx_overflow()
> on near-to-overflow branch only
> v3: adjust commit message and comments as suggested by Andy
> v2: initial version to join the series
> ---
> lib/kstrtox.c | 39 ++++++++++++++++++++++++++-------------
> 1 file changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
> index bdde40cd69d7..8691f85cf2ce 100644
> --- a/lib/kstrtox.c
> +++ b/lib/kstrtox.c
> @@ -39,20 +39,26 @@ const char *_parse_integer_fixup_radix(const char *s, unsigned int *base)
> return s;
> }
>
> -/*
> - * Convert non-negative integer string representation in explicitly given radix
> - * to an integer. A maximum of max_chars characters will be converted.
> +/**
> + * _parse_integer_limit - Convert integer string representation to an integer
> + * @s: Integer string representation
> + * @base: Radix
> + * @p: Where to store result
> + * @max_chars: Maximum amount of characters to convert
> + *
> + * Convert non-negative integer string representation in explicitly given
> + * radix to an integer. If overflow occurs, value at @p is set to ULLONG_MAX.
> *
> - * Return number of characters consumed maybe or-ed with overflow bit.
> - * If overflow occurs, result integer (incorrect) is still returned.
> + * This function is the workhorse of other string conversion functions and it
> + * is discouraged to use it explicitly. Consider kstrto*() family instead.
> *
> - * Don't you dare use this function.
> + * Return: Number of characters consumed, maybe ORed with overflow bit
> */
> noinline
> unsigned int _parse_integer_limit(const char *s, unsigned int base, unsigned long long *p,
> size_t max_chars)
> {
> - unsigned long long res;
> + unsigned long long tmp, res;
> unsigned int rv;
>
> res = 0;
> @@ -72,14 +78,21 @@ unsigned int _parse_integer_limit(const char *s, unsigned int base, unsigned lon
> if (val >= base)
> break;
> /*
> - * Check for overflow only if we are within range of
> - * it in the max base we support (16)
> + * Accumulate result if no overflow detected.
> + * Otherwise just consume valid characters.
> */
> - if (unlikely(res & (~0ull << 60))) {
> - if (res > div_u64(ULLONG_MAX - val, base))
> - rv |= KSTRTOX_OVERFLOW;
> + if (likely(res != ULLONG_MAX)) {
> + if (unlikely(res & (~0ull << 60))) {
Aren't those two checks in the wrong order?
The likely/unlikely really don't make that much difference
you want the main test first.
In any case what is the first check for?
I think it just stops 0xffffffffffffffff0 being treated as an error.
If you are trying to skip the rest of the digits after an overflow
you need to check 'rv'.
Although I wonder whether strtoul() (etc) should stop 'eating' input
when the value would overflow and return a pointer to the digit that
caused the error.
Code looking at the terminating character wont be expecting a digit
and will treat it as a syntax error - which is what you are trying to do.
That is a much easier API to use, and a 'drop-in' for existing code.
David
> + /* We're close to possible overflow. */
> + if (check_mul_overflow(res, base, &tmp) ||
> + check_add_overflow(tmp, val, &res)) {
> + res = ULLONG_MAX;
> + rv |= KSTRTOX_OVERFLOW;
> + }
> + } else {
> + res = res * base + val;
> + }
> }
> - res = res * base + val;
> rv++;
> s++;
> }
On Thu, Feb 05, 2026 at 10:15:37PM +0000, David Laight wrote: > On Wed, 4 Feb 2026 16:57:13 +0300 > Dmitry Antipov <dmantipov@yandex.ru> wrote: ... > Although I wonder whether strtoul() (etc) should stop 'eating' input > when the value would overflow Definitely no stop condition. The idea behind simple_strto*() in the kernel is that they will help to parse combined strings (several fields in one *constant* string), not eating the extra "valid" characters (digits) will be a disaster in a couple of aspects. > and return a pointer to the digit that caused the error. No. > Code looking at the terminating character wont be expecting a digit > and will treat it as a syntax error - which is what you are trying to do. > > That is a much easier API to use, and a 'drop-in' for existing code. Maybe, but problematic from the usage point of view as I described above. -- With Best Regards, Andy Shevchenko
On Fri, 2026-02-06 at 09:42 +0200, Andy Shevchenko wrote: > > Code looking at the terminating character wont be expecting a digit > > and will treat it as a syntax error - which is what you are trying to do. > > > > That is a much easier API to use, and a 'drop-in' for existing code. > > Maybe, but problematic from the usage point of view as I described above. Note that was an idea behihd memvalue(), see https://lists.openwall.net/linux-hardening/2026/01/07/23. IOW since "mem=64K" is expected to be used more often than, say, "mem=64K@0xaaaaa" or "mem=64K,sync", it may be useful to have a wrapper which is enough to parse "64K" but treat everything else as error. Dmitry
On Fri, Feb 06, 2026 at 12:53:50PM +0300, Dmitry Antipov wrote: > On Fri, 2026-02-06 at 09:42 +0200, Andy Shevchenko wrote: > > > > Code looking at the terminating character wont be expecting a digit > > > and will treat it as a syntax error - which is what you are trying to do. > > > > > > That is a much easier API to use, and a 'drop-in' for existing code. > > > > Maybe, but problematic from the usage point of view as I described above. > > Note that was an idea behihd memvalue(), see https://lists.openwall.net/linux-hardening/2026/01/07/23. > IOW since "mem=64K" is expected to be used more often than, say, "mem=64K@0xaaaaa" or "mem=64K,sync", > it may be useful to have a wrapper which is enough to parse "64K" but treat everything else as error. It will make it less useful. -- With Best Regards, Andy Shevchenko
On Wed, Feb 04, 2026 at 04:57:13PM +0300, Dmitry Antipov wrote:
> In '_parse_integer_limit()', adjust native integer arithmetic
> with near-to-overflow branch where 'check_mul_overflow()' and
> 'check_add_overflow()' are used to check whether an intermediate
> result goes out of range, and denote such a case with ULLONG_MAX,
> thus making the function more similar to standard C library's
> 'strtoull()'. Adjust comment to kernel-doc style as well.
...
> /*
> - * Check for overflow only if we are within range of
> - * it in the max base we support (16)
> + * Accumulate result if no overflow detected.
> + * Otherwise just consume valid characters.
> */
> - if (unlikely(res & (~0ull << 60))) {
> - if (res > div_u64(ULLONG_MAX - val, base))
> - rv |= KSTRTOX_OVERFLOW;
> + if (likely(res != ULLONG_MAX)) {
> + if (unlikely(res & (~0ull << 60))) {
> + /* We're close to possible overflow. */
> + if (check_mul_overflow(res, base, &tmp) ||
> + check_add_overflow(tmp, val, &res)) {
> + res = ULLONG_MAX;
> + rv |= KSTRTOX_OVERFLOW;
> + }
> + } else {
> + res = res * base + val;
> + }
> }
> - res = res * base + val;
> rv++;
> s++;
In case you would need a v6, we can leave some of the lines untouched if we
switch to for-loop instead of while, but it might make the for-loop quite long.
I'm okay with the current version, up to you to experiment and choose.
--
With Best Regards,
Andy Shevchenko
On Wed, 2026-02-04 at 16:31 +0200, Andy Shevchenko wrote:
> In case you would need a v6, we can leave some of the lines untouched if we
> switch to for-loop instead of while, but it might make the for-loop quite long.
Hmm...the for-loop might be:
for (res = 0, rv = 0; max_chars--; rv++, s++) {
...
}
and it makes _parse_integer_limit() a few lines shorter.
Dmitry
On Thu, Feb 05, 2026 at 12:04:14PM +0300, Dmitry Antipov wrote:
> On Wed, 2026-02-04 at 16:31 +0200, Andy Shevchenko wrote:
>
> > In case you would need a v6, we can leave some of the lines untouched if we
> > switch to for-loop instead of while, but it might make the for-loop quite long.
>
> Hmm...the for-loop might be:
>
> for (res = 0, rv = 0; max_chars--; rv++, s++) {
res = 0 should be left outside, it's not part of the for-loop iterators.
> ...
> }
>
> and it makes _parse_integer_limit() a few lines shorter.
Yes, but more disruption on the code, so there are pros and cons,
but if you decide to go with it in v6, I won't object.
--
With Best Regards,
Andy Shevchenko
© 2016 - 2026 Red Hat, Inc.