Re: adjfreq(2): limit adjustment to prevent overflow during tc_windup()
10^12 was the old definition of billion in the UK also, although in the last few decades it has become rare and 10^9 is now the norm. https://en.wikipedia.org/wiki/Long_and_short_scales has quite a bit about it. On Tue, 7 Jul 2020 at 00:27, Scott Cheloha wrote: > 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 > > > > > > 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 -5 to 5 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 Trillion10^12 > Billiarde Quadrillion 10^15 > TrillionQuintillion 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 - 1.7 > +++ lib/libc/sys/adjfreq.2 6 Jul 2020 23:00:24 - > @@ -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 -50 ppm or greater than 50 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.c22 Jun 2020 18:25:57 - 1.131 > +++ sys/kern/kern_time.c6 Jul 2020 23:00:24 - > @@ -391,6 +391,9 @@ sys_settimeofday(struct proc *p, void *v > return (0); > } > > +#define ADJFREQ_MAX (5LL << 32) > +#define ADJFREQ_MIN (-5LL << 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); > >
Re: adjfreq(2): limit adjustment to prevent overflow during tc_windup()
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 > > > > 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 -5 to 5 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 Trillion10^12 Billiarde Quadrillion 10^15 TrillionQuintillion 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 - 1.7 +++ lib/libc/sys/adjfreq.2 6 Jul 2020 23:00:24 - @@ -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 -50 ppm or greater than 50 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.c22 Jun 2020 18:25:57 - 1.131 +++ sys/kern/kern_time.c6 Jul 2020 23:00:24 - @@ -391,6 +391,9 @@ sys_settimeofday(struct proc *p, void *v return (0); } +#define ADJFREQ_MAX (5LL << 32) +#define ADJFREQ_MIN (-5LL << 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);
Re: adjfreq(2): limit adjustment to prevent overflow during tc_windup()
> Date: Mon, 6 Jul 2020 16:40:35 -0500 > From: Scott Cheloha > > 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 -5 to 5 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.210 Sep 2015 17:55:21 - 1.7 > +++ lib/libc/sys/adjfreq.26 Jul 2020 21:39:37 - > @@ -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 -5 parts-per-billion or greater than 5 > +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 - 1.131 > +++ sys/kern/kern_time.c 6 Jul 2020 21:39:38 - > @@ -391,6 +391,9 @@ sys_settimeofday(struct proc *p, void *v > return (0); > } > > +#define ADJFREQ_MAX (5LL << 32) > +#define ADJFREQ_MIN (-5LL << 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); > >
Re: adjfreq(2): limit adjustment to prevent overflow during tc_windup()
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 -5 to 5 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? 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 - 1.7 +++ lib/libc/sys/adjfreq.2 6 Jul 2020 21:39:37 - @@ -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 -5 parts-per-billion or greater than 5 +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.c22 Jun 2020 18:25:57 - 1.131 +++ sys/kern/kern_time.c6 Jul 2020 21:39:38 - @@ -391,6 +391,9 @@ sys_settimeofday(struct proc *p, void *v return (0); } +#define ADJFREQ_MAX (5LL << 32) +#define ADJFREQ_MIN (-5LL << 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);
Re: adjfreq(2): limit adjustment to prevent overflow during tc_windup()
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 -5 to 5 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;/* 2147483648000 */ > int64_t tc_freq_adj = 5LL << 32;/* 21474836480 > */ > > > scale = (u_int64_t)1 << 63 /* 9223372036854775808 > */ > scale += (th_adjustment + tc_freq_adj) / 1024 * 2199; > /*+= (216895848448000) / 1024 * 2199; */ > /*+= 465775362048000; */ > > 9223372036854775808 + 465775362048000 = 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 = -5LL << 32; > > you have 9223372036854775808 - 465775362048000 = 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 - 1.131 > +++ sys/kern/kern_time.c 3 Jul 2020 00:57:49 - > @@ -391,6 +391,9 @@ sys_settimeofday(struct proc *p, void *v > return (0); > } > > +#define ADJFREQ_MAX (5LL << 32) > +#define ADJFREQ_MIN (-5LL << 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.210 Sep 2015 17:55:21 - 1.7 > +++ lib/libc/sys/adjfreq.23 Jul 2020 00:57:49 - > @@ -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 -5 parts-per-billion or greater than 5 > +parts-per-billion. > .El > .Sh SEE ALSO > .Xr date 1 ,
adjfreq(2): limit adjustment to prevent overflow during tc_windup()
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 -5 to 5 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@?) 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;/* 2147483648000 */ int64_t tc_freq_adj = 5LL << 32;/* 21474836480 */ scale = (u_int64_t)1 << 63 /* 9223372036854775808 */ scale += (th_adjustment + tc_freq_adj) / 1024 * 2199; /*+= (216895848448000) / 1024 * 2199; */ /*+= 465775362048000; */ 9223372036854775808 + 465775362048000 = 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 = -5LL << 32; you have 9223372036854775808 - 465775362048000 = 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.c22 Jun 2020 18:25:57 - 1.131 +++ sys/kern/kern_time.c3 Jul 2020 00:57:49 - @@ -391,6 +391,9 @@ sys_settimeofday(struct proc *p, void *v return (0); } +#define ADJFREQ_MAX (5LL << 32) +#define ADJFREQ_MIN (-5LL << 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 - 1.7 +++ lib/libc/sys/adjfreq.2 3 Jul 2020 00:57:49 - @@ -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 -5 parts-per-billion or greater than 5 +parts-per-billion. .El .Sh SEE ALSO .Xr date 1 ,