On Mon, Jul 06, 2020 at 11:57:32PM +0200, Mark Kettenis wrote:
> > Date: Mon, 6 Jul 2020 16:40:35 -0500
> > From: Scott Cheloha <[email protected]>
> >
> > On Fri, Jul 03, 2020 at 10:52:15AM +0200, Otto Moerbeek wrote:
> > > On Thu, Jul 02, 2020 at 08:27:58PM -0500, Scott Cheloha wrote:
> > >
> > > > Hi,
> > > >
> > > > When we recompute the scaling factor during tc_windup() there is an
> > > > opportunity for arithmetic overflow/underflow when we add the NTP
> > > > adjustment into the scale:
> > > >
> > > > 649 scale = (u_int64_t)1 << 63;
> > > > 650 scale += \
> > > > 651 ((th->th_adjustment + th->th_counter->tc_freq_adj)
> > > > / 1024) * 2199;
> > > > 652 scale /= th->th_counter->tc_frequency;
> > > > 653 th->th_scale = scale * 2;
> > > >
> > > > At lines 650 and 651, you will overflow/underflow if
> > > > th->th_counter->tc_freq_adj is sufficiently positive/negative.
> > > >
> > > > I don't like the idea of checking for that overflow during
> > > > tc_windup(). We can pick a reasonable adjustment range and check for
> > > > it during adjfreq(2) and that should be good enough.
> > > >
> > > > My strawman proposal is a range of -500000000 to 500000000 parts per
> > > > billion. We could push the limits a bit, but half a billion seems
> > > > like a nice round number to me.
> > > >
> > > > On a perfect clock, this means you can effect a 0.5x slowdown or a
> > > > 1.5x speedup via adjfreq(2), but no slower/faster.
> > > >
> > > > I don't *think* ntpd(8) would ever reach such extreme adjustments
> > > > through its algorithm. I don't think this will break anyone's working
> > > > setup.
> > > >
> > > > (Maybe I'm wrong, though. otto@?)
> > >
> > > Right, ntpd is pretty conversative and won't do big adjustments.
> >
> > So, what is the right way to describe these limits?
> >
> > "Parts per billion"? Something else?
>
> The traditional way to express clock drift in the context of NTP is
> "parts per million" or "ppm" for short. There are good reasons to
> avoid using "billion" in documentation as a very similar word is used
> in Germanic languages for 10^12 where in English you mean 10^9.
Huh. So from what I've gathered:
German English ISO
Million Million 10^6
Milliarde Billion 10^9
Billion Trillion 10^12
Billiarde Quadrillion 10^15
Trillion Quintillion 10^18
Potentially confusing.
The "German Connection" to NTP in particular eludes me, though.
> Stripping three zeroes also makes it easier to read. [...]
Yes, it always does. It looks nicer as "N ppm" in the mandoc(1)
output, too.
--
otto@, from what you said I take it you're OK with the new limits so
I'm going commit it as follows in a day or two.
Index: lib/libc/sys/adjfreq.2
===================================================================
RCS file: /cvs/src/lib/libc/sys/adjfreq.2,v
retrieving revision 1.7
diff -u -p -r1.7 adjfreq.2
--- lib/libc/sys/adjfreq.2 10 Sep 2015 17:55:21 -0000 1.7
+++ lib/libc/sys/adjfreq.2 6 Jul 2020 23:00:24 -0000
@@ -60,6 +60,9 @@ The
.Fa freq
argument is non-null and the process's effective user ID is not that
of the superuser.
+.It Bq Er EINVAL
+.Fa freq
+is less than -500000 ppm or greater than 500000 ppm.
.El
.Sh SEE ALSO
.Xr date 1 ,
Index: sys/kern/kern_time.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_time.c,v
retrieving revision 1.131
diff -u -p -r1.131 kern_time.c
--- sys/kern/kern_time.c 22 Jun 2020 18:25:57 -0000 1.131
+++ sys/kern/kern_time.c 6 Jul 2020 23:00:24 -0000
@@ -391,6 +391,9 @@ sys_settimeofday(struct proc *p, void *v
return (0);
}
+#define ADJFREQ_MAX (500000000LL << 32)
+#define ADJFREQ_MIN (-500000000LL << 32)
+
int
sys_adjfreq(struct proc *p, void *v, register_t *retval)
{
@@ -408,6 +411,8 @@ sys_adjfreq(struct proc *p, void *v, reg
return (error);
if ((error = copyin(freq, &f, sizeof(f))))
return (error);
+ if (f < ADJFREQ_MIN || f > ADJFREQ_MAX)
+ return (EINVAL);
}
rw_enter(&tc_lock, (freq == NULL) ? RW_READ : RW_WRITE);
