[Fwd: Fwd: [PATCH] ntpd not adjusting clock after a step backward in time]

Denys Vlasenko vda.linux at googlemail.com
Wed Jan 8 16:17:22 UTC 2014


On Wed, Jan 8, 2014 at 1:14 PM, John Spencer
<maillist-busybox at barfooze.de> wrote:
> forwarding this on behalf of Maxim Naumov who registered to the ML but is
> unable to send anything to the list despite having received the "welcome"
> mail. denys, maybe you can find out what's wrong with his registration ?

I'm looking into it... First, I need to reset admin password on the list :(


> ---------- Forwarded message ----------
> From: Maxim Naumov <muxx.dev at gmail.com>
> Date: 20 December 2013 13:29
> Subject: [PATCH] ntpd not adjusting clock after a step backward in time
> To: busybox at busybox.net
>
>
> hi everyone
>
> I found a problem where busybox ntpd will ignore datapoints after a step
> backwards is performed.
>
> to reproduce:
> 1. set system clock 1 year (or other large amount) ahead (in the future)
> 2. start ntpd -p <server>
>
> the system clock will be set to NTP time correctly, but kernel clock
> frequency will not be updated for the duration of the jump, i.e. if the
> system clock was 1 year ahead, ntpd will not correct for 1 year.
>
> this is because immediately after the step, set_new_values() resets
> G.last_update_recv_time with the original receive time of the packet (which
> is in future). therefore successive datapoints are ignored as too old.
>
> the fix is not to use recv_time for set_new_values() but to use
> G.last_update_recv_time already updated by step_time() called previously.
>
> here's the patch:
>
> ---
> diff --git a/networking/ntpd.c b/networking/ntpd.c
> index ed83415..32d0dcb 100644
> --- a/networking/ntpd.c
> +++ b/networking/ntpd.c
> @@ -1447,12 +1447,14 @@ update_local_clock(peer_t *p)
>
>  #if USING_INITIAL_FREQ_ESTIMATION
>                 if (G.discipline_state == STATE_NSET) {
> -                       set_new_values(STATE_FREQ, /*offset:*/ 0,
> recv_time);
> +                       /* G.last_update_recv_time already updated */
> +                       set_new_values(STATE_FREQ, /*offset:*/ 0,
> G.last_update_recv_time);

No, G.last_update_recv_time is *not* already updated here.
Its _previous_ value is just corrected by step_time(),
but here we want to record a _new_ one.

>                         return 1; /* "ok to increase poll interval" */
>                 }
>  #endif
>                 abs_offset = offset = 0;
> -               set_new_values(STATE_SYNC, offset, recv_time);
> +               /* G.last_update_recv_time already updated */
> +               set_new_values(STATE_SYNC, offset, G.last_update_recv_time);
>
>         } else { /* abs_offset <= STEP_THRESHOLD */

I'd rather do

recv_time += offset;

right after step_time(offset) is called. This way. code is more obvious.

Can you test that it works?


More information about the busybox mailing list