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

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

[Wake Liu]

The timespec_sub function, as implemented in several timer
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:

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.

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.
---
 .../selftests/timers/alarmtimer-suspend.c     | 15 +++------
 tools/testing/selftests/timers/helpers.h      | 31 +++++++++++++++++++
 tools/testing/selftests/timers/nanosleep.c    |  2 +-
 tools/testing/selftests/timers/nsleep-lat.c   | 12 ++-----
 .../testing/selftests/timers/valid-adjtimex.c |  8 ++---
 5 files changed, 43 insertions(+), 25 deletions(-)
 create mode 100644 tools/testing/selftests/timers/helpers.h

diff --git a/tools/testing/selftests/timers/alarmtimer-suspend.c b/tools/testing/selftests/timers/alarmtimer-suspend.c
index a9ef76ea6051..e85ab182abe5 100644
--- a/tools/testing/selftests/timers/alarmtimer-suspend.c
+++ b/tools/testing/selftests/timers/alarmtimer-suspend.c
@@ -31,8 +31,9 @@
 #include <include/vdso/time64.h>
 #include <errno.h>
 #include "../kselftest.h"
+#include "helpers.h"
 
-#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 */
 
 #define SUSPEND_SECS 15
 int alarmcount;
@@ -70,14 +71,6 @@ char *clockstring(int clockid)
 }
 
 
-long long timespec_sub(struct timespec a, struct timespec b)
-{
-	long long ret = NSEC_PER_SEC * b.tv_sec + b.tv_nsec;
-
-	ret -= NSEC_PER_SEC * a.tv_sec + a.tv_nsec;
-	return ret;
-}
-
 int final_ret;
 
 void sigalarm(int signo)
@@ -88,8 +81,8 @@ void sigalarm(int signo)
 	clock_gettime(alarm_clock_id, &ts);
 	alarmcount++;
 
-	delta_ns = timespec_sub(start_time, ts);
-	delta_ns -= NSEC_PER_SEC * SUSPEND_SECS * alarmcount;
+	delta_ns = timespec_sub(ts, start_time);
+	delta_ns -= (long long)NSEC_PER_SEC * SUSPEND_SECS * alarmcount;
 
 	printf("ALARM(%i): %ld:%ld latency: %lld ns ", alarmcount, ts.tv_sec,
 							ts.tv_nsec, delta_ns);
diff --git a/tools/testing/selftests/timers/helpers.h b/tools/testing/selftests/timers/helpers.h
new file mode 100644
index 000000000000..652f20247091
--- /dev/null
+++ b/tools/testing/selftests/timers/helpers.h
@@ -0,0 +1,31 @@
+#ifndef SELFTESTS_TIMERS_HELPERS_H
+#define SELFTESTS_TIMERS_HELPERS_H
+
+#include <time.h>
+
+#ifndef NSEC_PER_SEC
+#define NSEC_PER_SEC 1000000000L
+#endif
+
+/*
+ * timespec_to_ns - Convert timespec to nanoseconds
+ */
+static inline long long timespec_to_ns(const struct timespec *ts)
+{
+	return ((long long) ts->tv_sec * NSEC_PER_SEC) + ts->tv_nsec;
+}
+
+/*
+ * timespec_sub - Subtract two timespec values
+ *
+ * @a: first timespec
+ * @b: second timespec
+ *
+ * Returns a - b in nanoseconds.
+ */
+static inline long long timespec_sub(struct timespec a, struct timespec b)
+{
+	return timespec_to_ns(&a) - timespec_to_ns(&b);
+}
+
+#endif /* SELFTESTS_TIMERS_HELPERS_H */
diff --git a/tools/testing/selftests/timers/nanosleep.c b/tools/testing/selftests/timers/nanosleep.c
index 252c6308c569..41c33629d5f0 100644
--- a/tools/testing/selftests/timers/nanosleep.c
+++ b/tools/testing/selftests/timers/nanosleep.c
@@ -138,7 +138,7 @@ int main(int argc, char **argv)
 		fflush(stdout);
 
 		length = 10;
-		while (length <= (NSEC_PER_SEC * 10)) {
+		while (length <= (NSEC_PER_SEC * 10LL)) {
 			ret = nanosleep_test(clockid, length);
 			if (ret == UNSUPPORTED) {
 				ksft_test_result_skip("%-31s\n", clockstring(clockid));
diff --git a/tools/testing/selftests/timers/nsleep-lat.c b/tools/testing/selftests/timers/nsleep-lat.c
index de23dc0c9f97..c888a77aab7a 100644
--- a/tools/testing/selftests/timers/nsleep-lat.c
+++ b/tools/testing/selftests/timers/nsleep-lat.c
@@ -26,6 +26,7 @@
 #include <signal.h>
 #include <include/vdso/time64.h>
 #include "../kselftest.h"
+#include "helpers.h"
 
 #define UNRESONABLE_LATENCY 40000000 /* 40ms in nanosecs */
 
@@ -74,14 +75,6 @@ struct timespec timespec_add(struct timespec ts, unsigned long long ns)
 }
 
 
-long long timespec_sub(struct timespec a, struct timespec b)
-{
-	long long ret = NSEC_PER_SEC * b.tv_sec + b.tv_nsec;
-
-	ret -= NSEC_PER_SEC * a.tv_sec + a.tv_nsec;
-	return ret;
-}
-
 int nanosleep_lat_test(int clockid, long long ns)
 {
 	struct timespec start, end, target;
@@ -146,7 +139,7 @@ int main(int argc, char **argv)
 			continue;
 
 		length = 10;
-		while (length <= (NSEC_PER_SEC * 10)) {
+		while (length <= (NSEC_PER_SEC * 10LL)) {
 			ret = nanosleep_lat_test(clockid, length);
 			if (ret)
 				break;
@@ -164,3 +157,4 @@ int main(int argc, char **argv)
 
 	ksft_finished();
 }
+
diff --git a/tools/testing/selftests/timers/valid-adjtimex.c b/tools/testing/selftests/timers/valid-adjtimex.c
index 6b7801055ad1..a61d4b4739a2 100644
--- a/tools/testing/selftests/timers/valid-adjtimex.c
+++ b/tools/testing/selftests/timers/valid-adjtimex.c
@@ -260,16 +260,16 @@ int validate_set_offset(void)
 	if (set_offset(-NSEC_PER_SEC - 1, 1))
 		return -1;
 
-	if (set_offset(5 * NSEC_PER_SEC, 1))
+	if (set_offset(5LL * NSEC_PER_SEC, 1))
 		return -1;
 
-	if (set_offset(-5 * NSEC_PER_SEC, 1))
+	if (set_offset(-5LL * NSEC_PER_SEC, 1))
 		return -1;
 
-	if (set_offset(5 * NSEC_PER_SEC + NSEC_PER_SEC / 2, 1))
+	if (set_offset(5LL * NSEC_PER_SEC + NSEC_PER_SEC / 2, 1))
 		return -1;
 
-	if (set_offset(-5 * NSEC_PER_SEC - NSEC_PER_SEC / 2, 1))
+	if (set_offset(-5LL * NSEC_PER_SEC - NSEC_PER_SEC / 2, 1))
 		return -1;
 
 	if (set_offset(USEC_PER_SEC - 1, 0))
-- 
2.51.0.384.g4c02a37b29-goog

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

[Thomas Gleixner]

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

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

[Carlos Llamas]

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