From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752891AbcIBIwJ (ORCPT ); Fri, 2 Sep 2016 04:52:09 -0400 Received: from Galois.linutronix.de ([146.0.238.70]:53296 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752802AbcIBIwD (ORCPT ); Fri, 2 Sep 2016 04:52:03 -0400 Date: Fri, 2 Sep 2016 10:49:43 +0200 (CEST) From: Thomas Gleixner To: Liav Rehana cc: linux-kernel@vger.kernel.org, john.stultz@linaro.org, noamca@mellanox.com, eladkan@mellanox.com Subject: Re: [PATCH] Fix chance of sign extension to nsec after its msb is set during calculation. In-Reply-To: <1472728478-3788-1-git-send-email-liavr@mellanox.com> Message-ID: References: <1472728478-3788-1-git-send-email-liavr@mellanox.com> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 1 Sep 2016, Liav Rehana wrote: > From: Liav Rehana > > 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 > --- > 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. Thanks, tglx