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.
-Otto
>
> Just so we're all clear that the math is sound, here's the result at
> the upper limit of the input range. Note that adjtime(2) is capped at
> 5000PPM in ntp_update_second(), hence its value here.
>
> int64_t th_adjustment = (5000 * 1000) << 32; /* 21474836480000000 */
> int64_t tc_freq_adj = 500000000LL << 32; /* 2147483648000000000
> */
>
>
> scale = (u_int64_t)1 << 63 /* 9223372036854775808
> */
> scale += (th_adjustment + tc_freq_adj) / 1024 * 2199;
> /* += (2168958484480000000) / 1024 * 2199; */
> /* += 4657753620480000000; */
>
> 9223372036854775808 + 4657753620480000000 = 13881125657334775808,
> which less than 18446744073709551616, so we don't have overflow.
>
> At the negative end of the input range, i.e.
>
> int64_t th_adjustment = (-5000 * 1000) << 32;
> int64_t tc_freq_adj = -500000000LL << 32;
>
> you have 9223372036854775808 - 4657753620480000000 = 4565618416374775808,
> so no underflow either.
>
> Thoughts?
>
> What is the best way to express this range in the documentation? Do I
> say "parts per billion", or something else?
>
> 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 3 Jul 2020 00:57:49 -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);
> 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 3 Jul 2020 00:57:49 -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 ,
