Messages in this thread Patch in this message | | | From | "Ulrich Windl" <> | Date | Mon, 15 Sep 1997 09:08:46 +0200 | Subject | Two bugs in kernel time code |
| |
The patch below is actually just an extract from my new PPSkit-0.3.0 that I'll announce a bit later.
There's a very old bug in the kernel that rarely occurs (who's using NTP?): The code to update the CMOS clock does not consider the hour field. I had no clever idea to fix it, so I simply added a message to log the problem (well, a part of it).
In adjtimex() several parameters were not checked, making it possibly to pass invalid values to the kernel. Also the plain old adjtim() was dependent on the time_status & STA_PLL, which should be completely independent of adjtimex() (ntp_adjtime).
Especially it was allowed to change the read-only bits of time_status...
The function now even returns TIME_ERROR if the flags in time_state indicate an error condition.
It should go into 2.1 as well I guess...
Ulrich
diff -r -u --new-file /usr/src/linux-2.0.31-pre9/arch/i386/kernel/time.c /usr/src/linux/arch/i386/kernel/time.c --- /usr/src/linux-2.0.31-pre9/arch/i386/kernel/time.c Sun Nov 10 18:40:53 1996 +++ /usr/src/linux/arch/i386/kernel/time.c Fri Sep 12 21:35:53 1997 @@ -282,6 +286,9 @@ * nowtime is written into the registers of the CMOS clock, it will * jump to the next second precisely 500 ms later. Check the Motorola * MC146818A or Dallas DS12887 data sheet for details. + * + * BUG: This routine does not handle hour overflow properly; it just + * sets the minutes. Usually you'll only notice that after reboot! */ static int set_rtc_mmss(unsigned long nowtime) { @@ -318,8 +325,12 @@ } CMOS_WRITE(real_seconds,RTC_SECONDS); CMOS_WRITE(real_minutes,RTC_MINUTES); - } else + } else { + printk(KERN_WARNING + "set_rtc_mmss: can't update from %d to %d\n", + cmos_minutes, real_minutes); retval = -1; + } /* The following flags have to be released exactly in this order, * otherwise the DS12887 (popular MC146818A clone with integrated diff -r -u --new-file /usr/src/linux-2.0.31-pre9/include/linux/timex.h /usr/src/linux/include/linux/timex.h --- /usr/src/linux-2.0.31-pre9/include/linux/timex.h Sat May 4 18:39:22 1996 +++ /usr/src/linux/include/linux/timex.h Tue Sep 9 21:17:46 1997 @@ -38,6 +38,8 @@ * Derived linux/timex.h * 1995-08-13 Torsten Duwe * kernel PLL updated to 1994-12-13 specs (rfc-1589) + * 1997-08-30 Ulrich Windl + * Added new constant NTP_PHASE_LIMIT */ #ifndef _LINUX_TIMEX_H #define _LINUX_TIMEX_H @@ -95,6 +97,7 @@ #define MAXTIME (200L << PPS_AVG) /* max PPS error (jitter) (200 us) */ #define MINSEC 16L /* min interval between updates (s) */ #define MAXSEC 1200L /* max interval between updates (s) */ +#define NTP_PHASE_LIMIT (MAXPHASE << 5) /* beyond max. dispersion */ /* * The following defines are used only if a pulse-per-second (PPS) diff -r -u --new-file /usr/src/linux-2.0.31-pre9/kernel/time.c /usr/src/linux/kernel/time.c --- /usr/src/linux-2.0.31-pre9/kernel/time.c Tue Sep 9 19:09:49 1997 +++ /usr/src/linux/kernel/time.c Sat Sep 13 19:48:48 1997 @@ -195,14 +494,14 @@ asmlinkage int sys_adjtimex(struct timex *txc_p) { long ltemp, mtemp, save_adjust; - int error; + int error = 0; /* Local copy of parameter */ struct timex txc; error = verify_area(VERIFY_WRITE, txc_p, sizeof(struct timex)); if (error) - return error; + return error; /* do not write results */ /* Copy the user data space into the kernel copy * structure. But bear in mind that the structures @@ -216,112 +515,139 @@ /* Now we validate the data before disabling interrupts */ - if (txc.modes != ADJ_OFFSET_SINGLESHOT && (txc.modes & ADJ_OFFSET)) - /* adjustment Offset limited to +- .512 seconds */ - if (txc.offset <= - MAXPHASE || txc.offset >= MAXPHASE ) - return -EINVAL; - - /* if the quartz is off by more than 10% something is VERY wrong ! */ - if (txc.modes & ADJ_TICK) - if (txc.tick < 900000/HZ || txc.tick > 1100000/HZ) - return -EINVAL; + /* adjustment Offset limited to +- .512 seconds */ + if (txc.offset <= - MAXPHASE || txc.offset >= MAXPHASE ) + return -EINVAL; cli(); - /* Save for later - semantics of adjtime is to return old value */ + /* Save for later - semantics of adjtime() is to return old value */ save_adjust = time_adjust; /* If there are input parameters, then process them */ if (txc.modes) { - if (time_state == TIME_BAD) - time_state = TIME_OK; - - if (txc.modes & ADJ_STATUS) - time_status = txc.status; + if (time_state == TIME_ERROR) + time_state = TIME_OK; /* reset error -- why? */ - if (txc.modes & ADJ_FREQUENCY) + if (txc.modes & ADJ_STATUS) /* only set allowed bits */ + time_status = (txc.status & ~STA_RONLY) | + (time_status & STA_RONLY); + + if (txc.modes & ADJ_FREQUENCY) { /* p. 22 */ + if (txc.freq > MAXFREQ || txc.freq < -MAXFREQ) { + error = -EINVAL; + goto leave; + } time_freq = txc.freq; + } - if (txc.modes & ADJ_MAXERROR) + if (txc.modes & ADJ_MAXERROR) { + if (txc.maxerror < 0 || txc.maxerror >= NTP_PHASE_LIMIT) { + error = -EINVAL; + goto leave; + } time_maxerror = txc.maxerror; + } - if (txc.modes & ADJ_ESTERROR) + if (txc.modes & ADJ_ESTERROR) { + if (txc.esterror < 0 || txc.esterror >= NTP_PHASE_LIMIT) { + error = -EINVAL; + goto leave; + } time_esterror = txc.esterror; + } - if (txc.modes & ADJ_TIMECONST) + if (txc.modes & ADJ_TIMECONST) { /* p. 24 */ + if (txc.constant < 0 || txc.constant > MAXTC) { + error = -EINVAL; + goto leave; + } time_constant = txc.constant; + } - if (txc.modes & ADJ_OFFSET) - if ((txc.modes == ADJ_OFFSET_SINGLESHOT) - || !(time_status & STA_PLL)) - { - time_adjust = txc.offset; - } - else if ((time_status & STA_PLL)||(time_status & STA_PPSTIME)) - { - ltemp = (time_status & STA_PPSTIME && - time_status & STA_PPSSIGNAL) ? - pps_offset : txc.offset; - - /* - * Scale the phase adjustment and - * clamp to the operating range. - */ - if (ltemp > MAXPHASE) - time_offset = MAXPHASE << SHIFT_UPDATE; - else if (ltemp < -MAXPHASE) - time_offset = -(MAXPHASE << SHIFT_UPDATE); - else - time_offset = ltemp << SHIFT_UPDATE; - - /* - * Select whether the frequency is to be controlled and in which - * mode (PLL or FLL). Clamp to the operating range. Ugly - * multiply/divide should be replaced someday. - */ - - if (time_status & STA_FREQHOLD || time_reftime == 0) + if (txc.modes & ADJ_OFFSET) { /* values checked earlier */ + if (txc.modes == ADJ_OFFSET_SINGLESHOT) { + /* adjtime() is independent from ntp_adjtime() */ + time_adjust = txc.offset; + } + else if ( time_status & (STA_PLL | STA_PPSTIME) ) { + ltemp = (time_status & (STA_PPSTIME | STA_PPSSIGNAL)) == + (STA_PPSTIME | STA_PPSSIGNAL) ? + pps_offset : txc.offset; + + /* + * Scale the phase adjustment and + * clamp to the operating range. + */ + if (ltemp > MAXPHASE) + time_offset = MAXPHASE << SHIFT_UPDATE; + else if (ltemp < -MAXPHASE) + time_offset = -(MAXPHASE << SHIFT_UPDATE); + else + time_offset = ltemp << SHIFT_UPDATE; + + /* + * Select whether the frequency is to be controlled + * and in which mode (PLL or FLL). Clamp to the operating + * range. Ugly multiply/divide should be replaced someday. + */ + + if (time_status & STA_FREQHOLD || time_reftime == 0) + time_reftime = xtime.tv_sec; + mtemp = xtime.tv_sec - time_reftime; time_reftime = xtime.tv_sec; - mtemp = xtime.tv_sec - time_reftime; - time_reftime = xtime.tv_sec; - if (time_status & STA_FLL) - { - if (mtemp >= MINSEC) - { - ltemp = ((time_offset / mtemp) << (SHIFT_USEC - - SHIFT_UPDATE)); - if (ltemp < 0) - time_freq -= -ltemp >> SHIFT_KH; - else - time_freq += ltemp >> SHIFT_KH; - } - } - else - { - if (mtemp < MAXSEC) - { - ltemp *= mtemp; - if (ltemp < 0) - time_freq -= -ltemp >> (time_constant + - time_constant + SHIFT_KF - - SHIFT_USEC); - else - time_freq += ltemp >> (time_constant + - time_constant + SHIFT_KF - - SHIFT_USEC); - } + if (time_status & STA_FLL) { + if (mtemp >= MINSEC) { + ltemp = (time_offset / mtemp) << (SHIFT_USEC - + SHIFT_UPDATE); + if (ltemp < 0) + time_freq -= -ltemp >> SHIFT_KH; + else + time_freq += ltemp >> SHIFT_KH; + } else /* calibration interval too short (p. 12) */ + time_state = TIME_ERROR; + } else { /* PLL mode */ + if (mtemp < MAXSEC) { + ltemp *= mtemp; + if (ltemp < 0) + time_freq -= -ltemp >> (time_constant + + time_constant + + SHIFT_KF - SHIFT_USEC); + else + time_freq += ltemp >> (time_constant + + time_constant + + SHIFT_KF - SHIFT_USEC); + } else /* calibration interval too long (p. 12) */ + time_state = TIME_ERROR; } - if (time_freq > time_tolerance) - time_freq = time_tolerance; - else if (time_freq < -time_tolerance) - time_freq = -time_tolerance; + if (time_freq > time_tolerance) + time_freq = time_tolerance; + else if (time_freq < -time_tolerance) + time_freq = -time_tolerance; } /* STA_PLL || STA_PPSTIME */ - if (txc.modes & ADJ_TICK) - tick = txc.tick; - - } + } /* txc.modes & ADJ_OFFSET */ + if (txc.modes & ADJ_TICK) { + /* if the quartz is off by more than 10% something is + VERY wrong ! */ + if (txc.tick < 900000/HZ || txc.tick > 1100000/HZ) { + error = -EINVAL; + goto leave; + } + tick = txc.tick; + } + } /* txc.modes */ +leave: if (((time_status & STA_PPSSIGNAL) == 0 + && (time_status & (STA_PPSFREQ|STA_PPSTIME)) != 0) + /* p. 24, (b) */ + || ((time_status + & (STA_PPSTIME|STA_PPSJITTER)) == (STA_PPSTIME|STA_PPSJITTER)) + /* p. 24, (c) */ + || ((time_status & STA_PPSFREQ) != 0 + && (time_status & (STA_PPSWANDER|STA_PPSERROR)) != 0)) + /* p. 24, (d) */ + time_state = TIME_ERROR; txc.offset = save_adjust; txc.freq = time_freq; txc.maxerror = time_maxerror; @@ -344,5 +670,5 @@ sti(); memcpy_tofs(txc_p, &txc, sizeof(struct timex)); - return time_state; + return error ? error :time_state; }
| |