RE: [PATCH] Fix chance of sign extension to nsec after its msb is set during calculation.

[Thomas Gleixner]

On Sun, 4 Sep 2016, Liav Rehana wrote:
> >> The root of the problem is that in case the multiplication of delta 
> >> and
> >> tkr->mult in the line that I've changed is too big that the MSB of the
> >> result is set, then the shift will cause an unwanted sign extension.
> 
> > I completely understand that, but as I said before:
> 
> > > > This typecast is just a baindaid. What happens if you double the 
> > > > suspend time?  The multiplication will simply overflow. So the 
> > > > proper fix is to sanity check delta and do multiple conversions if 
> > > > delta is big enough.  Preferrably this happens somewhere at the call 
> > > > site and not in this hotpath function.
> 
> > > That sign extension will be avoided completely if the variable nsec 
> > > was unsigned (u64 instead of s64), so I think the correct solution for 
> > > this is to change the type of nsec to u64.
> 
> > That's a different story and its not a solution for the general problem of
> 
> >        delta * mult >= (1 << 31) or delta * mult >= (1 << 32)
> 
> The case that delta * mult >= 1 << 31 is not a problem by itself, but it causes
> an unwanted sign extension since the type of nsec is signed. That sign
> extension is what causes the loop to take too long, and not the overflow.
> I understand that the typecast is not a general solution, so as I've said, I
> think that changing the type of nsec to u64 instead of s64 will be a good and
> general solution, as it will indeed solve the problem of the unwanted sign
> extension.
> 
> To summarize: a sign extension occurs if the nsec variable is signed, and so
> I ask if you think it will be a good solution to change its type to unsigned.

Do you actually read what I write? I asked John before:

> John, why is that stuff signed at all? Shouldn't we use u64 for all of this?

So to summarize: 

   - Yes, we can use u64 if there is nothing which I missed, but John will
     have the last word on this

   - No, making it u64 does not solve the general problem. It just papers
     over the problem you observe. And we don't add 'paper over' fixes,
     period.

Thanks,

	tglx