From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755613AbcI0AEE (ORCPT ); Mon, 26 Sep 2016 20:04:04 -0400 Received: from Galois.linutronix.de ([146.0.238.70]:48869 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755394AbcI0AD4 (ORCPT ); Mon, 26 Sep 2016 20:03:56 -0400 Date: Mon, 26 Sep 2016 20:01:27 -0400 (EDT) From: Thomas Gleixner To: Liav Rehana cc: john.stultz@linaro.org, linux-kernel@vger.kernel.org, eladkan@mellanox.com, noamca@mellanox.com Subject: Re: [PATCH] timekeeping: Change type of nsec variable to unsigned in its calculation. In-Reply-To: <1474868736-22724-1-git-send-email-liavr@mellanox.com> Message-ID: References: <1474868736-22724-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 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. Thanks, tglx