From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755882AbcIKLFp (ORCPT ); Sun, 11 Sep 2016 07:05:45 -0400 Received: from mail-db5eur01on0064.outbound.protection.outlook.com ([104.47.2.64]:17984 "EHLO EUR01-DB5-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1755383AbcIKLFn (ORCPT ); Sun, 11 Sep 2016 07:05:43 -0400 From: Liav Rehana To: John Stultz , Thomas Gleixner CC: lkml , Noam Camus , Elad Kanfi Subject: RE: [PATCH] Fix chance of sign extension to nsec after its msb is set during calculation. Thread-Topic: [PATCH] Fix chance of sign extension to nsec after its msb is set during calculation. Thread-Index: AQHSBEILu9KYNHIruUeLkYYicMsnOqBl5WGAgACiSgCABt3fAIAGe80Q Date: Sun, 11 Sep 2016 06:31:44 +0000 Message-ID: References: <1472728478-3788-1-git-send-email-liavr@mellanox.com> In-Reply-To: Accept-Language: he-IL, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=liavr@mellanox.com; x-originating-ip: [193.47.165.251] x-ms-office365-filtering-correlation-id: 0af1fec9-02d4-4961-1d58-08d3da0d4ccd x-microsoft-exchange-diagnostics: 1;VI1PR0501MB2032;6:hmyTWIxKAKTuyLr56WIQiWpg1QA9T+2gsCyzA7r9Ls3kfhTcWfT+mzp/2GdLetlHZOkllF1L9ENMLIJM85gkB6bQ+7GIut2emhzQ5vwTrmV+Uy+HNtlLLPtHxa47n76D4vd7XlrmMykYmUi55HtT6hbVeJkm/wTLoLNkpxjKbH+AMNi85sXi8HpmTDR+kH3J2QhuZoLtVFBJ0MmudY7/4rmv8Y4aDnQvAEBwykeW0w35IoQlFud951FDXP7vpk6c5MHwWE+LYyfTPzv8qGj8CNaVyVYJB3z1bTL1BJzaZC+UGD1ExGkXOWxCZnnZpUZBQWBABMiG1DtCNW4S65WyAg==;5:o0E/xViD88ScOFrO3+B7GMUzvwdSWxaFPQHALCNbHTkstJSvyjAeIne+J6Sy1rQXf+56s+EeXDtK/Z22NjYDrBhR7xFPiwIFxoBSl3FpF/kX+qtrz37zgHv91P2cL1q/MlYk0MUfTE+BJEgn7Refqw==;24:++A99WWF56TXmcvns/OOmDRHl6KX9W3D4ALMrNlb4ihm3a3Y5v8+WpLEmM2ff08EL7/t2ZJxdP93o3X3N4KVaX3UaTKOXqSwt5HiOoB3+UU=;7:BvtGoZYg0gTStGAzJESADhk4sUzQRIYeGsBRuhqn1XoGA2LlORhvaLwufPaeFA/uasvvId6nwTuNM8o71eb4bStlvcM+HfvB6pOxZZMdM+w5ePQp0f6GEUPet9PH09XuUH0usIyhGudLOHXMpQVb2DTjtzyvRsT2RKGrAXZRT+0QlWic5UpC0SZGuiH9HgJYOtmt4iSXT1k9TZaYO/XBqJ6YkMHWTnmgvGKcyZBX2RIAZWkPub2jH5l7tyGtCrBB x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:VI1PR0501MB2032; x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(21532816269658); x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(6040176)(601004)(2401047)(8121501046)(5005006)(10201501046)(3002001)(6055026);SRVR:VI1PR0501MB2032;BCL:0;PCL:0;RULEID:;SRVR:VI1PR0501MB2032; x-forefront-prvs: 0062BDD52C x-forefront-antispam-report: SFV:NSPM;SFS:(10009020)(6009001)(7916002)(199003)(189002)(93886004)(586003)(3846002)(102836003)(6116002)(189998001)(305945005)(50986999)(9686002)(122556002)(5001770100001)(106116001)(33656002)(4001430100002)(74316002)(76176999)(92566002)(7846002)(54356999)(7736002)(81156014)(81166006)(3280700002)(8676002)(77096005)(87936001)(8936002)(66066001)(86362001)(19580405001)(76576001)(5660300001)(7696004)(2906002)(10400500002)(19580395003)(4326007)(5002640100001)(68736007)(105586002)(2950100001)(107886002)(106356001)(3660700001)(97736004)(101416001)(2900100001);DIR:OUT;SFP:1101;SCL:1;SRVR:VI1PR0501MB2032;H:VI1PR0501MB2461.eurprd05.prod.outlook.com;FPR:;SPF:None;PTR:InfoNoRecords;A:1;MX:1;LANG:en; spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-originalarrivaltime: 11 Sep 2016 06:31:44.5593 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0501MB2032 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id u8BB5uIW008324 >> > > 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. > > > > As a side note. John, why is that stuff unsigned at all? Shouldn't we > > use > > u64 for all of this? > >Errrr... My memory is quite foggy here. I think we just started way back when with s64 to catch cases where there were negative nsec intervals. Looking through the git logs it seems its > been that way since the beginning of the generic timekeeping logic. > >For most cases here u64 is fine. There may be a few cases where we do have valid negative nanosecond intervals, but I can't think of any off the top of my head, and those can >probably be special cased. In light of your comment for that issue, I would like to change the type of the nsec variable to u64, as it will solve the sign extension problem. For that, I intend to change the type of that variable in the functions that define it, and in the struct that uses it in kernel/time/timekeeping.c. Do you think there are other references I should change. Or do you think there is a better solution ? Please provide your opinion on this matter. Thanks, Liav.