Re: [PATCH] timekeeping: Change type of nsec variable to unsigned in its calculation.

[David Gibson]

On Tue, Nov 29, 2016 at 03:22:17PM +0100, Thomas Gleixner wrote:
> On Fri, 18 Nov 2016, John Stultz wrote:
> > From: Liav Rehana <liavr@xxxxxxxxxxxx>
> > 
> > During the calculation of the nsec variable in the inline function
> > timekeeping_delta_to_ns, it may undergo a sign extension if its msb
> > is set just before the shift. The sign extension may, in some cases,
> > gain it a value near the maximum value of the 64-bit range. This is
> > bad when it is later used in a division function, such as
> > __iter_div_u64_rem, where the amount of loops it will go through to
> > calculate the division will be too large. One can encounter such a
> > problem, for example, when trying to connect through ftp from an
> > outside host to the operation system. When the OS is too overloaded,
> > delta will get a high enough value for the msb of the sum
> > delta * tkr->mult + tkr->xtime_nsec to be set, and so after the
> > shift the nsec variable will gain a value similar to
> > 0xffffffffff000000. Using a variable with such a value in the
> > inline function __iter_div_u64_rem will take too long, making the
> > ftp connection attempt seem to get stuck.
> > The following commit fixes that chance of sign extension, while
> > maintaining the type of the nsec variable as signed for other
> > functions that use this variable, for possible legit negative
> > time intervals.
> >
> > Thomas/Ingo: This is for tip:timers/urgent.
> 
> Certainly not! My objections against this still stand. See:
> 
> http://lkml.kernel.org/r/alpine.DEB.2.20.1609261956160.4915@nanos
> 
> http://lkml.kernel.org/r/alpine.DEB.2.20.1609270929170.4891@nanos
> 
> If we have legitimate use cases with a negative delta, then this patch
> breaks them no matter what. See the basic C course section in the second
> link.

So, fwiw, when I first wrote a variant on this, I wasn't trying to fix
every case - just to make the consequences less bad if something goes
wrong.  An overflow here can still mess up timekeeping, it's true, but
time going backwards tends to cause things to go horribly, horribly
wrong - which was why I spotted this in the first place.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Attachment: signature.asc
Description: PGP signature