Re: [PATCH v12 02/11] lib: kstrtox: add kstrtoudec64() and kstrtodec64()

[Andy Shevchenko]

On Tue, May 12, 2026 at 6:11 PM Rodrigo Alencar
<455.rodrigo.alencar@xxxxxxxxx> wrote:
> On 26/05/12 05:43PM, Andy Shevchenko wrote:
> > On Tue, May 12, 2026 at 03:12:24PM +0100, Rodrigo Alencar wrote:
> > > On 26/05/12 04:48PM, Andy Shevchenko wrote:
> > > > On Tue, May 12, 2026 at 02:21:14PM +0100, Rodrigo Alencar wrote:
> > > > > On 26/05/12 04:12PM, Andy Shevchenko wrote:
> > > > > > On Tue, May 12, 2026 at 12:39:53PM +0100, Jonathan Cameron wrote:
> > > > > > > On Sun, 10 May 2026 13:42:20 +0100
> > > > > > > Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@xxxxxxxxxx> wrote:
> > > > > > >
> > > > > > > > Add helpers that parses decimal numbers into 64-bit number, i.e., decimal
> > > > > > > > point numbers with pre-defined scale are parsed into a 64-bit value (fixed
> > > > > > > > precision). After the decimal point, digits beyond the specified scale
> > > > > > > > are ignored.
> > > > > > >
> > > > > > > Whilst Rodrigo has already replied to say there will be another version
> > > > > > > I'd like to request final feedback from those who were involved in the parser
> > > > > > > discussions.
> > > > > > >
> > > > > > > They got very involved and I'm far from an expert in the right way to do
> > > > > > > this stuff.
> > > > > > >
> > > > > > > I don't think David Laight was +CC so I've added that.
> > > > > > > David, Andy - I think you two were most involved in that discussion:
> > > > > > > Any objections to the end result?
> > > > > >
> > > > > > I already said a few times about the naming. I do not like the kstrto*()
> > > > > > be semantically different on how they treat the input. Second point is
> > > > > > to avoid code duplication, but this one is less of a concern since the
> > > > > > new code is in the library close to the other potentially duplicate code
> > > > > > piece and hence can be addressed later.
> > > > >
> > > > > I suppose I reached into kstrtodec64() and kstrtoudec64() because it aligns
> > > > > with your expectations for kstrto*() semantics, no? Those include:
> > > > >  - overflow check;
> > > > >  - extensive input validation;
> > > > >  - optional '\n' in the end;
> > > > >  - mandatory nul-termination.
> > > > >
> > > > > am I missing anything?
> > > >
> > > > When we add scale we basically make that not true. Moreover the code in this
> > > > patch makes scale == number_of_characters which I think a bit fragile, however
> > > > it's about the fractional part when the amount of digits is equal to scale.
> > >
> > > That is not really the case. It is being set as a limit, so it does check for
> > > truncation and zero-padding.
> >
> > I do not see it happens in _parse_integer_limit(). It doesn't try to parse more
> > characters than it's requested in max_chars. It doesn't check if there are more
> > character nor their converted values.
> >
> > > > To make this work as expected we need to add an additional call like
> > > > kstrtoull() (and perhaps drop that \n and NUL-terminator checks) and see
> > > > if that overflows or not. Since it's a fractional part it must have less
> > > > than 20 (decimal) digits there, so we check the rv (or how many digits
> > > > were parsed successfully) and compare to 20. If it's more, we got too many
> > > > decimal digits.
> > >
> > > For overflow it checks the KSTRTOX_OVERFLOW flag and leverages check_mul_overflow()
> > > and check_add_overflow() when combining fractional and integer parts. The amount
> > > of characters is not really important there. The scale cannot be bigger than 19 and
> > > that makes sure that int_pow() does not overflow. The code uses _parse_integer_limit()
> > > due to the nature of input and to avoid 64-bit division, kstrtoull() at any point
> > > (parsing integer or fractional parts) does not make much sense.
> >
> > Under 'like kstrotoull()' I meant something that repeats needed functionality.
> > I believe it's parse_integer() (without limit).
>
> I think we are going in circles here and we could look at the code instead:
> - integer parsing with _parse_integer()
>         - overflow check and validation of the return value
> - fractional parsing with _parse_integer_limit()
>         - overflow check and validation of the return value

No, this is not fully true. That's what my whole point is about. The
max_chars parameter limits the input check, then it skips an arbitrary
number of digits and only *then* it checks for \n and \0. What will be
the result of the
0.00000000000000000000000000000000423 in your case? Whatever scale you
gave it will return 0 without checking on how many digits were
supplied. All the same for 0.9999999999999999999999999999999000423. My
point is that we should limit this by 19 digits.

On top of that, what about -0.9(19 times) ? the fraction should be u64
in this case and it's fine. The sign applies to the combined value.

>         - extra scaling and truncation happening outside if needed.

Right, but the given input may be way too long and still needs more validation.

> - check for input termination
> - combination of integer and fractional parts with check_mul_overflow() and check_add_overflow()
>
> > > > Maybe I'm missing these checks already performed?
> > > >
> > > > > > Having the test cases is a big benefit, and that part I like the most.



-- 
With Best Regards,
Andy Shevchenko