Re: [PATCH v2] selftests/timers: Consolidate and fix 32-bit overflow in timespec_sub

[Carlos Llamas <cmllamas@google.com>]

On Sun, Sep 21, 2025 at 09:49:54AM +0200, Thomas Gleixner wrote:
> On Tue, Sep 16 2025 at 03:19, Wake Liu wrote:
> > The timespec_sub function, as implemented in several timer
> 
> timespec_sub()
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#function-references-in-changelogs
> 
> > selftests, is prone to integer overflow on 32-bit systems.
> >
> > The calculation `NSEC_PER_SEC * b.tv_sec` is performed using
> > 32-bit arithmetic, and the result overflows before being
> > stored in the 64-bit `ret` variable. This leads to incorrect
> > time delta calculations and test failures.
> >
> > As suggested by tglx, this patch fixes the issue by:
> 
> s/this patch fixes/fix/
> 
> 
> 
> > 1. Creating a new `static inline` helper function,
> >    `timespec_to_ns`, which safely converts a `timespec` to
> >    nanoseconds by casting `tv_sec` to `long long` before
> >    multiplying with `NSEC_PER_SEC`.
> >
> > 2. Placing the new helper and a rewritten `timespec_sub` into
> >    a common header: tools/testing/selftests/timers/helpers.h.
> >
> > 3. Removing the duplicated, buggy implementations from all
> >    timer selftests and replacing them with an #include of the
> >    new header.
> >
> > This consolidates the code and ensures the calculation is
> > correctly performed using 64-bit arithmetic across all tests.
> 
> This lacks a Signed-off-by.
> 
> > Changes in v2:
> >   - Per tglx's feedback, instead of changing NSEC_PER_SEC globally,
> >     this version consolidates the buggy timespec_sub() implementations
> >     into a new 32-bit safe inline function in a shared header.
> >   - Amended the commit message to be more descriptive.
> 
> change logs go behind the '---' separator as they are not part of the
> commit message. It's documented how to format a change log properly.
> 
> > -#define UNREASONABLE_LAT (NSEC_PER_SEC * 5) /* hopefully we resume in 5 secs */
> > +#define UNREASONABLE_LAT (NSEC_PER_SEC * 5LL) /* hopefully we resume in 5 secs */
> 
> How is this change and the pile of similar ones related to $subject and
> why are they required in the first place?
>   
> > index 000000000000..652f20247091
> > --- /dev/null
> > +++ b/tools/testing/selftests/timers/helpers.h
> > @@ -0,0 +1,31 @@
> 
> Lacks a SPDX identifier.
> 
> scripts/checkpatch.pl exists for a reason.
> 
> Thanks,
> 
>         tglx

I also just stumbled into this. However, doing a little bit of research
it seems this was introduced by commit 80fa614e2fbc ("selftests: timers:
Remove local NSEC_PER_SEC and USEC_PER_SEC defines"), which explicitly
changes the local definitions in favor of the internal kernel header,
but that doesn't seem right.

I think we should probably revert that commit instead?

--
Carlos Llamas