Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755961AbdIGUm6 (ORCPT ); Thu, 7 Sep 2017 16:42:58 -0400 Received: from mx0b-00190b01.pphosted.com ([67.231.157.127]:57150 "EHLO mx0b-00190b01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751456AbdIGUm5 (ORCPT ); Thu, 7 Sep 2017 16:42:57 -0400 From: "Lubashev, Igor" To: "Pai, Vishwanath" , Linus Torvalds CC: Ingo Molnar , "Hunt, Joshua" , "Pablo Neira Ayuso" , Borislav Petkov , "Andy Lutomirski" , the arch/x86 maintainers , Linux Kernel Mailing List , Brian Gerst , Andrew Cooper , Juergen Gross , Boris Ostrovsky , Kees Cook , Andrew Morton , "David S. Miller" , Arnd Bergmann Subject: RE: xt_hashlimig build error (was Re: [RFC 01/17] x86/asm/64: Remove the restore_c_regs_and_iret label) Thread-Topic: xt_hashlimig build error (was Re: [RFC 01/17] x86/asm/64: Remove the restore_c_regs_and_iret label) Thread-Index: AQHTKAHsqsAZeD1z90u0fsP9pGKvNaKqEQ0AgAAFCgCAABotgP//rb1A Date: Thu, 7 Sep 2017 20:41:38 +0000 Message-ID: References: <69b38985-e094-ddcc-6f7e-d6e5cc2c657e@akamai.com> <6667f710-68f3-b97e-b0eb-d9879476831e@akamai.com> In-Reply-To: <6667f710-68f3-b97e-b0eb-d9879476831e@akamai.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [172.19.37.12] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-09-07_12:,, signatures=0 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000 definitions=main-1709070307 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-09-07_12:,, signatures=0 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1011 lowpriorityscore=0 impostorscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000 definitions=main-1709070306 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 nfs id v87Kh7dd026399 Content-Length: 3224 Lines: 76 Since user is u64, it is best to have a predictable return value for all possible values of user. So maybe: static u64 user2rate_bytes(u64 user) { u64 r; r = user ? U32_MAX / (u32) min(user, U32_MAX) : U32_MAX; r = (r - 1) << XT_HASHLIMIT_BYTE_SHIFT; return r; } -----Original Message----- From: Vishwanath Pai [mailto:vpai@akamai.com] Sent: Thursday, September 07, 2017 4:17 PM To: Linus Torvalds Cc: Ingo Molnar ; Lubashev, Igor ; Hunt, Joshua ; Pablo Neira Ayuso ; Borislav Petkov ; Andy Lutomirski ; the arch/x86 maintainers ; Linux Kernel Mailing List ; Brian Gerst ; Andrew Cooper ; Juergen Gross ; Boris Ostrovsky ; Kees Cook ; Andrew Morton ; David S. Miller ; Arnd Bergmann Subject: Re: xt_hashlimig build error (was Re: [RFC 01/17] x86/asm/64: Remove the restore_c_regs_and_iret label) On 09/07/2017 02:43 PM, Linus Torvalds wrote: > Note: that patch has *exactly* the issue I was talking about above. > > Doing that > > if (user > 0xFFFFFFFFULL) > return 0; > > is different from the old code, which used to result in a zero in the > divide, and then > > r = (r - 1) << 4; > > would cause it to return a large value. > > So the patch in question doesn't just fix the build error, it > completely changes the semantics of the function too. > > I *think* the new behavior is likely what you want, but these kinds of > things should be _described_. > > Also, even with the patch, we have garbage: > > 0xFFFFFFFFULL / (u32)user > > why is that sub-expression pointlessly doing a 64-bit divide with a > 32-bit number? The compiler is hopefully smart enough to point things > out, but that "ULL" really is _wrong_ there, and could cause a stupid > compiler to still do a 64-bit divide (although hopefully the simpler > version that is 64/32). > > So please clarify both the correct behavior _and_ the actual typing of > the divide, ok? > > Linus The value of 'user' is sent from userspace, which is the return value of this function: static uint64_t bytes_to_cost(uint32_t bytes) { uint32_t r = bytes >> XT_HASHLIMIT_BYTE_SHIFT; return UINT32_MAX / (r+1); } What user2rate_bytes() is trying to do is the opposite of above. The size of 'user' is 64bit for a different reason altogether, but in this case it is guaranteed to be always < U32_MAX. And hence using 64bit divide is completely pointless (which I now realize). Writing U32INT_MAX as 0xFFFFFFFFULL was a mistake on my part. I could have avoided all of this by using built-in constants instead of trying to define them myself. I will rewrite the function as below and send out another patch: static u64 user2rate_bytes(u64 user) { u64 r; r = user ? U32_MAX / (u32) user : U32_MAX; r = (r - 1) << XT_HASHLIMIT_BYTE_SHIFT; return r; } -Vishwanath