> 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.
Stripping three zeroes also makes it easier to read.
I don't think the fact that this is expressed in the code as "parts
per billion" is an issue.
Cheers,
Mark
> 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 21:39:37 -0000
> @@ -60,6 +60,10 @@ 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 -500000000 parts-per-billion or greater than 500000000
> +parts-per-billion.
> .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 21:39:38 -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);
>
>
