Bug 261781
| Summary: | vdso: Time calculation integer overflow | ||
|---|---|---|---|
| Product: | Base System | Reporter: | Jaroslaw Pelczar <jarek> |
| Component: | kern | Assignee: | Konstantin Belousov <kib> |
| Status: | Closed FIXED | ||
| Severity: | Affects Some People | CC: | emaste, kib, secteam |
| Priority: | --- | Flags: | koobs:
maintainer-feedback+
koobs: mfc-stable13? |
| Version: | CURRENT | ||
| Hardware: | arm | ||
| OS: | Any | ||
I do not understand what are you saying. If scale * delta overflows, which we detect by calculating ffsl(scale) + fls(delta), we do more complicated calculation that moves overflow to bt->sec, and clamp scale to 32bit. (In reply to Konstantin Belousov from comment #1) scale = 295147905178 ffsl(295147905178) = 2 scale_bits = 2 delta = 64100295 fls(delta) = 26 2 + 26 < 63 => but overflow occurs. I see, do you mean the following:
diff --git a/lib/libc/sys/__vdso_gettimeofday.c b/lib/libc/sys/__vdso_gettimeofday.c
index c1457a54d37e..cf1400cdf291 100644
--- a/lib/libc/sys/__vdso_gettimeofday.c
+++ b/lib/libc/sys/__vdso_gettimeofday.c
@@ -83,9 +83,9 @@ binuptime(struct bintime *bt, struct vdso_timekeep *tk, bool abs)
return (error);
scale = th->th_scale;
#ifdef _LP64
- scale_bits = ffsl(scale);
+ scale_bits = flsl(scale);
#else
- scale_bits = ffsll(scale);
+ scale_bits = flsll(scale);
#endif
if (__predict_false(scale_bits + fls(delta) > 63)) {
x = (scale >> 32) * delta;
(In reply to Konstantin Belousov from comment #3) Yes, it seems to do the trick :) I tested such fix locally for 8 hours and it looks like it works fine. A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=a1f9326607dea5ab6979935e3ca2d7402dcc7cc1 commit a1f9326607dea5ab6979935e3ca2d7402dcc7cc1 Author: Konstantin Belousov <kib@FreeBSD.org> AuthorDate: 2022-02-08 19:13:40 +0000 Commit: Konstantin Belousov <kib@FreeBSD.org> CommitDate: 2022-02-08 19:44:23 +0000 libc binuptime(): use the right function to get the most significant bit index Reported and tested by: Jaroslaw Pelczar <jarek@jpelczar.com> PR: 261781 Sponsored by: The FreeBSD Foundation MFC after: 1 week lib/libc/sys/__vdso_gettimeofday.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=d0199f27c06caac4ccfedc61e8b2d13b658ea589 commit d0199f27c06caac4ccfedc61e8b2d13b658ea589 Author: Konstantin Belousov <kib@FreeBSD.org> AuthorDate: 2022-02-08 19:13:40 +0000 Commit: Konstantin Belousov <kib@FreeBSD.org> CommitDate: 2022-02-15 00:36:51 +0000 libc binuptime(): use the right function to get the most significant bit index PR: 261781 (cherry picked from commit a1f9326607dea5ab6979935e3ca2d7402dcc7cc1) lib/libc/sys/__vdso_gettimeofday.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) |
I have been tinkering with clock subsystem for more or less tickless based approach. I am not sure whether calculation in "lib/libc/sys/__vdso_gettimeofday.c" for "binuptime" function is correct. Currently the code looks like this: scale = th->th_scale; #ifdef _LP64 scale_bits = ffsl(scale); #else scale_bits = ffsll(scale); #endif if (__predict_false(scale_bits + fls(delta) > 63)) { x = (scale >> 32) * delta; scale &= 0xffffffff; bt->sec += x >> 32; bintime_addx(bt, x << 32); } Example outputs from two time points (time is measured by ARM Generic Timer, but that's just 64bit counter masked to 32bits): th->th_boottime = 1640852968.ff886104742783f9 timecounter delta = 29015463 th->th_scale = 295147905178 th->th_offset_count = 539967626 bintime_addx(bt, scale * delta = 0x76D8EB0A9A877676) => 9.4522dbb32c111955 th->th_boottime = 1640852968.ff886104742783f9 timecounter delta = 64100295 th->th_scale = 295147905178 th->th_offset_count = 539967626 bintime_addx(bt, scale * delta) => 8.d4d7f89392515095 Multiplication of scale and delta will overflow 64 bits. I am not sure whether the scale_bits should rather be calculated by flsl/flsll to detect the overflow.