On 30/11/2015 02:30, David Gibson wrote: > 1e75fa8 "time: Condense timekeeper.xtime into xtime_sec" replaced a call to > clocksource_cyc2ns() from timekeeping_get_ns() with an open-coded version > of the same logic to avoid keeping a semi-redundant struct timespec > in struct timekeeper. > > However, the commit also introduced a subtle semantic change - where > clocksource_cyc2ns() uses purely unsigned math, the new version introduces > a signed temporary, meaning that if (delta * tk->mult) has a 63-bit > overflow the following shift will still give a negative result. The > choice of 'maxsec' in __clocksource_updatefreq_scale() means this will > generally happen if there's a ~10 minute pause in examining the > clocksource. > > This can be triggered on a powerpc KVM guest by stopping it from qemu for > a bit over 10 minutes. After resuming time has jumped backwards several > minutes causing numerous problems (jiffies does not advance, msleep()s can > be extended by minutes..). It doesn't happen on x86 KVM guests, because > the guest TSC is effectively frozen while the guest is stopped, which is > not the case for the powerpc timebase. > > Obviously an unsigned (64 bit) overflow will only take twice as long as a > signed, 63-bit overflow. I don't know the time code well enough to know > if that will still cause incorrect calculations, or if a 64-bit overflow > is avoided elsewhere. > > Still, an incorrect forwards clock adjustment will cause less trouble than > time going backwards. So, this patch removes the potential for > intermediate signed overflow. > > Suggested-by: Laurent Vivier <lviv...@redhat.com> > Signed-off-by: David Gibson <da...@gibson.dropbear.id.au> > --- > kernel/time/timekeeping.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c > index d563c19..99188ee 100644 > --- a/kernel/time/timekeeping.c > +++ b/kernel/time/timekeeping.c > @@ -305,8 +305,7 @@ static inline s64 timekeeping_get_ns(struct tk_read_base > *tkr) > > delta = timekeeping_get_delta(tkr); > > - nsec = delta * tkr->mult + tkr->xtime_nsec; > - nsec >>= tkr->shift; > + nsec = (delta * tkr->mult + tkr->xtime_nsec) >> tkr->shift; > > /* If arch requires, add in get_arch_timeoffset() */ > return nsec + arch_gettimeoffset(); > Tested-by: Laurent Vivier <lviv...@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
