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

[Liav Rehana]

-----Original Message-----
From: Thomas Gleixner [mailto:tglx@xxxxxxxxxxxxx] 
Sent: Tuesday, September 27, 2016 3:01 AM
To: Liav Rehana <liavr@xxxxxxxxxxxx>
Cc: john.stultz@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Elad Kanfi <eladkan@xxxxxxxxxxxx>; Noam Camus <noamca@xxxxxxxxxxxx>
Subject: Re: [PATCH] timekeeping: Change type of nsec variable to unsigned in its calculation.

On Mon, 26 Sep 2016, Liav Rehana wrote:
>> 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.
>> The following commit fixes that chance of sign extension,

>Again. This does not fix anything, it papers over the underlying problem that the calling code hands in a delta which is big enough to overflow the multiplication into the negative space. >You just extend the range of deltas which are handled gracefully, but that does not fix the underlying problem as we still can run into the multiplication overflow. It won't cause the >result to be negative, but it will be crap nevertheless.

>> while maintaining the type of the nsec variable as signed for other 
>> functions that use this variable, for possible legit negative time 
>> intervals.

>What is this maintaining? The type of this variable changes to u64 and other functions cannot use this variable at all because it's local to that function. This sentence makes no sense at >all.

About your first note, I understand that it doesn't fix the overflow chance, but I'm not quite sure what can be done about that. If there was a code in the calling function that detects
such event, what do you think can be done about it ? Would you just print a warning and lower delta as to not overflow after the multiplication ? If not, how do you think the problem I ran into can be fixed, if not by eliminating the possibility for sign extension the way I did ?

About the other note, I understood from you and John that there are some cases where negative time intervals are valid. What I meant by 'maintaining the type of the nsec variable as signed' was, that for the other functions that call the function I've changed, they do define a variable named nsec, and they define it as signed. In their implementation they assign him a value that is returned from the function I've changed. While the nsec variable is unsigned now in the function that calculates it, it can still return a value with an MSB that is set, which will be handled as negative in the caller function, where it was defined as signed. In that case, the change I added just removes the possibility of a sign extension, but the nsec variable will still be viewed as negative on the caller functions where it was supposed to return a negative value in the function I've changed.