Re: [PATCH] time: Avoid possible NTP adjustment mult overflow

[John Stultz]

On Mon, Nov 24, 2014 at 5:16 PM, pang.xunlei <pang.xunlei@xxxxxxxxxx> wrote:
> Ideally, __clocksource_updatefreq_scale, selects the largest shift
> value possible for a clocksource. This results in the mult memember
> of struct clocksource being particularly large, although not so large
> that NTP would adjust the clock to cause it to overflow.
>
> That said, nothing actually prohibits an overflow from occurring, its
> just that it "shouldn't" occur.
>
> So while very unlikely, and so far never observed, the value of
> (cs->mult+cs->maxadj) may have a chance to reach very near 0xFFFFFFFF,
> so there is a possibility it may overflow when doing NTP positive
> adjustment
>
> See the following detail: When NTP slewes the clock, kernel goes
> through update_wall_time()->...->timekeeping_apply_adjustment():
>                  tk->tkr.mult += mult_adj;
>
> Since there is no guard against it, its possible tk->tkr.mult may
> overflow during this operation.
>
> This patch avoids any possible mult overflow by judging the overflow
> case before adding mult_adj to mult, also adds the WARNING message
> when capturing such case.
>
> Signed-off-by: pang.xunlei <pang.xunlei@xxxxxxxxxx>
> ---
> Fix the problem in the former patch catched by Fengguang's 0day robot:
> [time] WARNING: CPU: 0 PID: 1 at kernel/time/timekeeping.c:1337 update_wall_time()
>
>  kernel/time/timekeeping.c |    6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index 0aef92a..9a1482e 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -1390,6 +1390,12 @@ static __always_inline void timekeeping_apply_adjustment(struct timekeeper *tk,
>          *
>          * XXX - TODO: Doc ntp_error calculation.
>          */
> +       if (mult_adj > 0 && (tk->tkr.mult + mult_adj < mult_adj)) {
> +               /* NTP adjustment caused clocksource mult overflow */
> +               WARN_ON_ONCE(1);
> +               return;
> +       }
> +
>         tk->tkr.mult += mult_adj;
>         tk->xtime_interval += interval;
>         tk->tkr.xtime_nsec -= offset;

So the original problematic patch is already queued in -tip, so next
time its best to just provide the additional fix on-top of the
problematic one (instead of sending a entirely new patch).

I've got a fix implementing the same, which went through testing
today, and I'll send it to Thomas in a moment.

thanks
-john
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/