Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1475875

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

Show all headers | View raw

Hi Thomas,

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.

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.

Do you agree ?
If not, what is the reason for it being of s64 type in the first place ?
Is there any place where we want the nsec variable to contain a negative value ?
Moreover, how can we cope with the sign extension problems in this case ?

Thanks,
Liav.

-----Original Message-----
From: Thomas Gleixner [mailto:tglx@linutronix.de] 
Sent: Friday, September 02, 2016 9:31 PM
To: Liav Rehana <liavr@mellanox.com>
Cc: linux-kernel@vger.kernel.org; john.stultz@linaro.org; Noam Camus <noamca@mellanox.com>; Elad Kanfi <eladkan@mellanox.com>
Subject: Re: [PATCH] Fix chance of sign extension to nsec after its msb is set during calculation.

On Fri, 2 Sep 2016, Thomas Gleixner wrote:
> On Thu, 1 Sep 2016, Liav Rehana wrote:
> > From: Liav Rehana <liavr@mellanox.com>
> > 
> > During the calculation of the nsec variable, "delta * tkr->mult" may 
> > cause overflow to the msb, if the suspended time is too long.
> > In that case, we need to guarantee that the variable will not go 
> > through a sign extension during its shift, and thus it will result 
> > in a much higher value - close to the larget value of 64 bits.
> > The following commit fixes this problem, which causes the following bug:
> > Trying to connect through ftp to the os after a long enough 
> > suspended time will cause the nsec variable to get a much higher 
> > value after its shift because of sign extension, and thus the loop 
> > that follows some instructions afterwards, implemented in the inline 
> > function __iter_div_u64_rem, will take too long.
> > 
> > Signed-off-by: Liav Rehana <liavr@mellanox.com>
> > ---
> >  kernel/time/timekeeping.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c 
> > index 479d25c..ddf56a5 100644
> > --- a/kernel/time/timekeeping.c
> > +++ b/kernel/time/timekeeping.c
> > @@ -305,7 +305,7 @@ static inline s64 timekeeping_delta_to_ns(struct tk_read_base *tkr,
> >  	s64 nsec;
> >  
> >  	nsec = delta * tkr->mult + tkr->xtime_nsec;
> > -	nsec >>= tkr->shift;
> > +	nsec = ((u64) nsec) >> tkr->shift;
> 
> 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.

As a side note. John, why is that stuff unsigned at all? Shouldn't we use
u64 for all of this?

Thanks,

	tglx

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

Re: [PATCH] Fix chance of sign extension to nsec after its msb is  set during calculation. Thomas Gleixner <tglx@linutronix.de> - 2016-09-02 11:00 +0200
  Re: [PATCH] Fix chance of sign extension to nsec after its msb is  set during calculation. Thomas Gleixner <tglx@linutronix.de> - 2016-09-02 20:40 +0200
    Re: [PATCH] Fix chance of sign extension to nsec after its msb is  set during calculation. Thomas Gleixner <tglx@linutronix.de> - 2016-09-02 20:50 +0200
    RE: [PATCH] Fix chance of sign extension to nsec after its msb is set  during calculation. Liav Rehana <liavr@mellanox.com> - 2016-09-04 09:20 +0200
      RE: [PATCH] Fix chance of sign extension to nsec after its msb is  set during calculation. Thomas Gleixner <tglx@linutronix.de> - 2016-09-04 11:10 +0200
        RE: [PATCH] Fix chance of sign extension to nsec after its msb is  set during calculation. Thomas Gleixner <tglx@linutronix.de> - 2016-09-04 12:50 +0200
        RE: [PATCH] Fix chance of sign extension to nsec after its msb is set  during calculation. Liav Rehana <liavr@mellanox.com> - 2016-09-05 00:50 +0200
    Re: [PATCH] Fix chance of sign extension to nsec after its msb is set  during calculation. John Stultz <john.stultz@linaro.org> - 2016-09-07 05:30 +0200

csiph-web