Date: Tue, 15 Nov 2016 22:53:44 +0100 (CET)
From: Thomas Gleixner <tglx@linutronix.de>
To: John Stultz <john.stultz@linaro.org>
cc: Chris Metcalf <cmetcalf@mellanox.com>,
        Laurent Vivier <lvivier@redhat.com>,
        David Gibson <david@gibson.dropbear.id.au>,
        "Christopher S . Hall" <christopher.s.hall@intel.com>,
        lkml <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] time: Avoid signed overflow in
 timekeeping_delta_to_ns()
In-Reply-To: <CALAqxLX1iDJFbnVW3-TCjaeHdKX-HFLe4JAf7vmd4QLLLi4G3g@mail.gmail.com>
Message-ID: <alpine.DEB.2.20.1611152250080.3595@nanos>
References: <1479152569-16890-1-git-send-email-cmetcalf@mellanox.com> <CALAqxLX1iDJFbnVW3-TCjaeHdKX-HFLe4JAf7vmd4QLLLi4G3g@mail.gmail.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
Content-Length: 1819
Lines: 46

On Mon, 14 Nov 2016, John Stultz wrote:

> On Mon, Nov 14, 2016 at 11:42 AM, Chris Metcalf <cmetcalf@mellanox.com> wrote:
> > This bugfix was originally made in commit 35a4933a8959 ("time:
> > Avoid signed overflow in timekeeping_get_ns()").  When the code was
> > refactored in commit 6bd58f09e1d8 ("time: Add cycles to nanoseconds
> > translation") the signed overflow fix was lost.  Re-introduce it.
> >
> > Signed-off-by: Chris Metcalf <cmetcalf@mellanox.com>
> > ---
> > I happened to be looking for an unrelated fix, found this code,
> > realized the tip code didn't match the fixed code, and
> > backtracked to where it had gone away.
> >
> >  kernel/time/timekeeping.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> > index 37dec7e3db43..57926bc7b7f3 100644
> > --- a/kernel/time/timekeeping.c
> > +++ b/kernel/time/timekeeping.c
> > @@ -304,8 +304,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 = (delta * tkr->mult + tkr->xtime_nsec) >> tkr->shift;
> 
> Ugh.
> 
> So... I think this proves the original fix was *far* too subtle to
> maintain. So I think reintroducing it as-is doesn't protect us from
> undoing it.  If the problem is really using the signed intermediate
> nsec value, we should get rid of that.

As I told the other guy who submitted something similar: This is not really
helpful. It merily drags the overflow case out by a factor of 2.

So we really need to figure out under which circumstances this can happen
and fixup either the callsites or detect the condition right there, which
I'd like to avoid for the hotpath.

Thanks,

	tglx