Message ID | cfc6c0f0fd4c4724890be8a8397c2cbe@AcuMS.aculab.com |
---|---|
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:f944:0:0:0:0:0 with SMTP id q4csp4088306wrr; Fri, 25 Nov 2022 07:09:50 -0800 (PST) X-Google-Smtp-Source: AA0mqf4RLNAIyVVwYLGTWJDHc8RJeAmhFDGBA/nMuU5Old0OB9ehTgSWyz21Ofq1bWDWnlqgD8O9 X-Received: by 2002:a63:230e:0:b0:470:86e3:b93e with SMTP id j14-20020a63230e000000b0047086e3b93emr16984225pgj.337.1669388989509; Fri, 25 Nov 2022 07:09:49 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1669388989; cv=none; d=google.com; s=arc-20160816; b=cVtMfFjeY0tU93/Mp+w2SZkTfDm1W2Pg7uXuHC9GS/fqNJG4gKWOuePOBYaWxSXJSa Vz/CbjeqwzkW7JPzBzBFzM6DLMynavbN85kufoxG2cmf81ewEeCna7hZ0bZ8P7U7WSrb 0yxDoRqcs76MigFNXEkFik1yHX8LATi9m1GgCIpln5ba93ggQZ2uobY8X2UhMG1Ts5xu YPt0+qHMmRYJSqm/D/XcLAI6UPg9LQeoJIV6e7IY9KO+qr5tY6FtdBo0V8ojUHx0QZZc Yd0FizCzzwyguewxPliyJNIMBGN3QAH8sptfpwqJu0D6Q/pSH3QSSX0OmognexVfmTfz sHhA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:content-language :mime-version:accept-language:message-id:date:thread-index :thread-topic:subject:cc:to:from; bh=xOtZOjm+AUH+W5xUchVR60vW/dmbcuC9ZaZmt/0pLfw=; b=fxepGMgUrtoQxa8INeZUliUZygnu/Oz3shp/GlAO1dGGO6/87f6EPsvNeXyEUKCHIm wbG7VGqk/aZFASqeOY9ui+immG9RLxrquD2FFWeuIO9070yDMoz42iM8zlSbPFD9NmZD IOZlshx8q1tbR9jgV5zCVnKgldsY9kLUkRiwjshucaAADtfI+tzIzvRRggCKkHuF9DgC f0VhjC4XbcwDgEe67XZaGOOQPpouL0wM3RvsGn80aXDVi0ozLzGMoivhmClzTkpXHMkZ 5izIT1ENKKwW7X61WqS80QTFlrWPWWGD4Y+3YBeNvqkojA/F8g+9jukgPDqYe9Li9H/4 3FCw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=aculab.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id k13-20020aa79d0d000000b00563ab8e5e83si3837445pfp.370.2022.11.25.07.09.32; Fri, 25 Nov 2022 07:09:49 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=aculab.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230033AbiKYPAz convert rfc822-to-8bit (ORCPT <rfc822;zxc52fgh@gmail.com> + 99 others); Fri, 25 Nov 2022 10:00:55 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55366 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229657AbiKYPAv (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 25 Nov 2022 10:00:51 -0500 Received: from eu-smtp-delivery-151.mimecast.com (eu-smtp-delivery-151.mimecast.com [185.58.86.151]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3009332060 for <linux-kernel@vger.kernel.org>; Fri, 25 Nov 2022 07:00:49 -0800 (PST) Received: from AcuMS.aculab.com (156.67.243.121 [156.67.243.121]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384) id uk-mta-233-_fQABpPHMNKhxMHqvcLVOg-1; Fri, 25 Nov 2022 15:00:44 +0000 X-MC-Unique: _fQABpPHMNKhxMHqvcLVOg-1 Received: from AcuMS.Aculab.com (10.202.163.4) by AcuMS.aculab.com (10.202.163.4) with Microsoft SMTP Server (TLS) id 15.0.1497.42; Fri, 25 Nov 2022 15:00:41 +0000 Received: from AcuMS.Aculab.com ([::1]) by AcuMS.aculab.com ([::1]) with mapi id 15.00.1497.044; Fri, 25 Nov 2022 15:00:41 +0000 From: David Laight <David.Laight@ACULAB.COM> To: LKML <linux-kernel@vger.kernel.org>, Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Andrew Morton <akpm@linux-foundation.org> CC: Steven Rostedt <rostedt@goodmis.org>, 'Joe Perches' <joe@perches.com>, Linus Torvalds <torvalds@linux-foundation.org> Subject: [PATCH 0/1] Slightly relax the type checking done by min() and max(). Thread-Topic: [PATCH 0/1] Slightly relax the type checking done by min() and max(). Thread-Index: AdkA3hAUrkOWi1xYRg2SAA6P778SAw== Date: Fri, 25 Nov 2022 15:00:40 +0000 Message-ID: <cfc6c0f0fd4c4724890be8a8397c2cbe@AcuMS.aculab.com> Accept-Language: en-GB, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.202.205.107] MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: aculab.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1750481229445703799?= X-GMAIL-MSGID: =?utf-8?q?1750481229445703799?= |
Series |
Slightly relax the type checking done by min() and max().
|
|
Message
David Laight
Nov. 25, 2022, 3 p.m. UTC
The min() and max() defines include a type check to avoid the unexpected behaviour when a negative value is compared against and unsigned value. However a lot of code hits this check and uses min_t() to avoid the error. Many of these are just plain wrong. Those casting to u8 or u16 are particularly suspect, eg: drivers/usb/misc/usb251xb.c:528: hub->max_current_sp = min_t(u8, property_u32 / 2000, 50); This patch does two changes: - Replace typeof(x) with typeof((x) + 0) to promote char/short to int. - Add an (int) cast to constants between 0 and MAX_INT so the compiler doesn't promote the 'other side' of the comparison to an unsinged type. If this is done the type test is arranged to always succeed. The following can also be done (with some lateral thought): - Allow all comparisons where both types are signed. - Allow all comparisons where both types are unsigned. - Allow comparisons where the larger type is signed. In addition most of the min_t() calls are there to compare a signed type (that holds a non-negative value) with an unsigned value. The definition: #define min_unsigned(x,y) min((x) + 0u + 0ull, (y) + 0u + 0ull) will do an unsigned comparision without 'accidentally' masking off any non-zero high bits. With those extra changes there can be a 'duck shoot' on min_t(). David Laight (1): Slightly relax the type checking done by min() and max(). include/linux/minmax.h | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-)
Comments
On Fri, Nov 25, 2022 at 03:00:40PM +0000, David Laight wrote: > The min() and max() defines include a type check to avoid the unexpected > behaviour when a negative value is compared against and unsigned value. > However a lot of code hits this check and uses min_t() to avoid the error. > Many of these are just plain wrong. > > Those casting to u8 or u16 are particularly suspect, eg: > drivers/usb/misc/usb251xb.c:528: > hub->max_current_sp = min_t(u8, property_u32 / 2000, 50); > > This patch does two changes: > - Replace typeof(x) with typeof((x) + 0) to promote char/short to int. > - Add an (int) cast to constants between 0 and MAX_INT so the compiler > doesn't promote the 'other side' of the comparison to an unsinged type. > If this is done the type test is arranged to always succeed. > > The following can also be done (with some lateral thought): > - Allow all comparisons where both types are signed. > - Allow all comparisons where both types are unsigned. > - Allow comparisons where the larger type is signed. > > In addition most of the min_t() calls are there to compare a signed type > (that holds a non-negative value) with an unsigned value. > The definition: > #define min_unsigned(x,y) min((x) + 0u + 0ull, (y) + 0u + 0ull) > will do an unsigned comparision without 'accidentally' masking off > any non-zero high bits. > > With those extra changes there can be a 'duck shoot' on min_t(). You have an issue with your email setup, i.e. you send two independent messages (not a chain). It probably shows that either you don't use `git send-email` for sending patch, or you missed --thread option to it.
On Fri, Nov 25, 2022 at 03:00:40PM +0000, David Laight wrote: > The min() and max() defines include a type check to avoid the unexpected > behaviour when a negative value is compared against and unsigned value. > However a lot of code hits this check and uses min_t() to avoid the error. > Many of these are just plain wrong. > > Those casting to u8 or u16 are particularly suspect, eg: > drivers/usb/misc/usb251xb.c:528: > hub->max_current_sp = min_t(u8, property_u32 / 2000, 50); I don't buy this. What's exactly wrong with this code?
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Sent: 25 November 2022 15:18 ... > You have an issue with your email setup, i.e. you send two independent messages > (not a chain). It probably shows that either you don't use `git send-email` for > sending patch, or you missed --thread option to it. It is technically impossible for me to send (or view) threaded emails. I have to send them using outlook. The only way to avoid mangling the text is to cut&paste from wordpad. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
From: Andy Shevchenko > Sent: 25 November 2022 15:21 > > On Fri, Nov 25, 2022 at 03:00:40PM +0000, David Laight wrote: > > The min() and max() defines include a type check to avoid the unexpected > > behaviour when a negative value is compared against and unsigned value. > > However a lot of code hits this check and uses min_t() to avoid the error. > > Many of these are just plain wrong. > > > > Those casting to u8 or u16 are particularly suspect, eg: > > drivers/usb/misc/usb251xb.c:528: > > hub->max_current_sp = min_t(u8, property_u32 / 2000, 50); > > I don't buy this. What's exactly wrong with this code? Consider what happens if propery_u32 is 512000. The returned value is 0 not 50. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Fri, Nov 25, 2022 at 03:27:07PM +0000, David Laight wrote: > From: Andy Shevchenko > > Sent: 25 November 2022 15:21 > > On Fri, Nov 25, 2022 at 03:00:40PM +0000, David Laight wrote: > > > The min() and max() defines include a type check to avoid the unexpected > > > behaviour when a negative value is compared against and unsigned value. > > > However a lot of code hits this check and uses min_t() to avoid the error. > > > Many of these are just plain wrong. > > > > > > Those casting to u8 or u16 are particularly suspect, eg: > > > drivers/usb/misc/usb251xb.c:528: > > > hub->max_current_sp = min_t(u8, property_u32 / 2000, 50); > > > > I don't buy this. What's exactly wrong with this code? > > Consider what happens if propery_u32 is 512000. > The returned value is 0 not 50. I considered that and there are two things to consider on your side: 1) it's coming from device property; 2) device property is validated using YAML schema. On top of that, the wrong property is on the user. We have a lot of stuff that user may put wrongly, but it's user's choice. Any better example, please?
From: 'Andy Shevchenko' > Sent: 25 November 2022 15:58 > > On Fri, Nov 25, 2022 at 03:27:07PM +0000, David Laight wrote: > > From: Andy Shevchenko > > > Sent: 25 November 2022 15:21 > > > On Fri, Nov 25, 2022 at 03:00:40PM +0000, David Laight wrote: > > > > The min() and max() defines include a type check to avoid the unexpected > > > > behaviour when a negative value is compared against and unsigned value. > > > > However a lot of code hits this check and uses min_t() to avoid the error. > > > > Many of these are just plain wrong. > > > > > > > > Those casting to u8 or u16 are particularly suspect, eg: > > > > drivers/usb/misc/usb251xb.c:528: > > > > hub->max_current_sp = min_t(u8, property_u32 / 2000, 50); > > > > > > I don't buy this. What's exactly wrong with this code? > > > > Consider what happens if propery_u32 is 512000. > > The returned value is 0 not 50. > > I considered that and there are two things to consider on your side: > 1) it's coming from device property; > 2) device property is validated using YAML schema. > > On top of that, the wrong property is on the user. We have a lot of stuff that > user may put wrongly, but it's user's choice. > > Any better example, please? How about: data_size = min_t(u16, buf_size, len); https://elixir.bootlin.com/linux/v6.1-rc6/source/kernel/printk/printk_ringbuffer.c#L1738 Now, maybe, you could claim that buf_size > 64k never happens. But the correct cast here is u32 to match buf_size. len (being u16) will be promoted to int before the compare. Just search the kernel for "min_t(u8," or "min_t(u16," while some might be ok, I really wouldn't want to verify each case. If you look hard enough there are also some: u32_var = min_t(u32, u32_val, u64_val); where the intent is to limit values that might be invalid for u32. I did try compiling a kernel with min_t() defined to be min() (no casts) but there were too many false positives without allowing all unsigned v unsigned compares. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Fri, Nov 25, 2022 at 04:14:58PM +0000, David Laight wrote: > From: 'Andy Shevchenko' > > Sent: 25 November 2022 15:58 > > On Fri, Nov 25, 2022 at 03:27:07PM +0000, David Laight wrote: ... > > Any better example, please? > > How about: Better, indeed. > data_size = min_t(u16, buf_size, len); > > https://elixir.bootlin.com/linux/v6.1-rc6/source/kernel/printk/printk_ringbuffer.c#L1738 > > Now, maybe, you could claim that buf_size > 64k never happens. > But the correct cast here is u32 to match buf_size. > len (being u16) will be promoted to int before the compare. > > Just search the kernel for "min_t(u8," or "min_t(u16," while some might > be ok, I really wouldn't want to verify each case. > > If you look hard enough there are also some: > u32_var = min_t(u32, u32_val, u64_val); > where the intent is to limit values that might be invalid for u32. Wouldn't be better to actually issue a warning if the desired type is shorter than one of the min_t() arguments? Then you go through all cases and fix them accordingly. Blindly relaxing the rules is not an option in my opinion.
From: 'Andy Shevchenko' > Sent: 25 November 2022 17:48 > > On Fri, Nov 25, 2022 at 04:14:58PM +0000, David Laight wrote: > > From: 'Andy Shevchenko' > > > Sent: 25 November 2022 15:58 > > > On Fri, Nov 25, 2022 at 03:27:07PM +0000, David Laight wrote: > > ... > > > > Any better example, please? > > > > How about: > > Better, indeed. > > > data_size = min_t(u16, buf_size, len); > > > > https://elixir.bootlin.com/linux/v6.1-rc6/source/kernel/printk/printk_ringbuffer.c#L1738 > > > > Now, maybe, you could claim that buf_size > 64k never happens. > > But the correct cast here is u32 to match buf_size. > > len (being u16) will be promoted to int before the compare. > > > > Just search the kernel for "min_t(u8," or "min_t(u16," while some might > > be ok, I really wouldn't want to verify each case. > > > > If you look hard enough there are also some: > > u32_var = min_t(u32, u32_val, u64_val); > > where the intent is to limit values that might be invalid for u32. > > Wouldn't be better to actually issue a warning if the desired type is shorter > than one of the min_t() arguments? > > Then you go through all cases and fix them accordingly. That is an option, but that is separate from this change. > Blindly relaxing the rules is not an option in my opinion. The point is I'm not really relaxing the rules. If anything I'm actually tightening them by allowing min() to be used in more cases - so letting the buggy min_t() casts be removed at some point in the future. The purpose of the type check is to error code like: int x = -4; x = min(x, 100u); where integer promotion changes the comparison 'x < 100u' to the unexpected '(unsigned int)x < 100u' so x is set to 100. However is also errors the opposite: unsigned int x = 4; x = min(x, 100); But, in this case, the compiler just converts 100 to 100u and everything is fine. It also errors the perfectly valid (and reasonable looking): unsigned char x = 4; x = min(x, 100u); because 100u is 'unsigned int'. Indeed, 'x' gets converted to 'signed int' for the comparison. When the (usually) RHS is a compile-time constant that is known to be positive (and less than 2^31) I'm just changing the test to: if (variable < (int)constant) If 'variable' is a signed type this always generates the required signed compare. If 'variable is 'unsigned char' or 'unsigned short' then it gets promoted to signed int and a signed compare of positive values is done. If variable is 'unsigned int', 'unsigned long' or 'unsigned long long' then the RHS is converted to unsigned - but keeps its value since it is known to be positive. In all cases the outcome is exactly what is required. No change of putting in the wrong cast. There are a few other common cases that are valid but currently generate an error: min(s8_var, int_expr) min(u8_var, int_expr) min(u8_var, unsigned_int_expr) These generate an error regardless of whether the sizes match: min(int_expr, long_expr) min(unsigned_int_expr, unsigned_long_expr) min(unsigned_long_expr, unsigned_long_long_expr) This is less common but should also be allowed: min(long_long_int_expr, unsigned_int_expr) (ie when the signed type is larger than the unsigned one) I have a plan for how to allow all those as well. Checking typeof((x) + 0) against typeof((y) + 0) allows for the promotion of char/short to signed int - but not any of the other comparisons that never implicitly convert a signed value to unsigned (and hence negative values to large positive ones). David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Fri, 25 Nov 2022 19:47:14 +0000 David Laight <David.Laight@ACULAB.COM> wrote: > > Blindly relaxing the rules is not an option in my opinion. > > The point is I'm not really relaxing the rules. > If anything I'm actually tightening them by allowing min() to > be used in more cases - so letting the buggy min_t() casts be > removed at some point in the future. That sounds very virtuous. It would be helpful to see a few "convert min_t to min" patches to see this proposal in action.
From: Andrew Morton > Sent: 26 November 2022 00:02 > > On Fri, 25 Nov 2022 19:47:14 +0000 David Laight <David.Laight@ACULAB.COM> wrote: > > > > Blindly relaxing the rules is not an option in my opinion. > > > > The point is I'm not really relaxing the rules. > > If anything I'm actually tightening them by allowing min() to > > be used in more cases - so letting the buggy min_t() casts be > > removed at some point in the future. > > That sounds very virtuous. Indeed. > It would be helpful to see a few "convert min_t to min" patches > to see this proposal in action. I did try building a kernel with '#define min_t min' and then picking up the pieces. Doable but there are still too many false positives. That is when I found some of the 'broken' uses of min_t(). I'd rather get this patch accepted and then build on top of it. Then it should be possible to work out which min_t() actually need to stay - I suspect very few. It might even be worth converting min_t(type,x,y) to min_t_type(x,y) and then converting the min_t_u[0-9]*(x,y) to #define min_unsigned(x,y) min((x) + 0u + 0ull, (y) + 0u + 0ull) I think that efficiently converts both values to an unsigned type without masking the value. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)