From: David Laight <David.Laight@ACULAB.COM>
To: "'linux-kernel@vger.kernel.org'" <linux-kernel@vger.kernel.org>,
"'Andy Shevchenko'" <andriy.shevchenko@linux.intel.com>,
'Andrew Morton' <akpm@linux-foundation.org>,
"'Matthew Wilcox (Oracle)'" <willy@infradead.org>,
'Christoph Hellwig' <hch@infradead.org>,
"'Jason A. Donenfeld'" <Jason@zx2c4.com>
Subject: [PATCH next v2 0/5] minmax: Relax type checks in min() and max().
Date: Fri, 28 Jul 2023 14:49:21 +0000 [thread overview]
Message-ID: <bde3d2dc933848bbaceeb9b7102f6f4c@AcuMS.aculab.com> (raw)
The min() (etc) functions in minmax.h require that the arguments have
exactly the same types. This was probably added after an 'accident'
where a negative value got converted to a large unsigned value.
However when the type check fails, rather than look at the types and
fix the type of a variable/constant, everyone seems to jump on min_t().
In reality min_t() ought to be rare - when something unusual is being
done, not normality.
If the wrong type is picked (and it is far too easy to pick the type
of the result instead of the larger input) then significant bits can
get discarded.
Pretty much the worst example is in the derfved clamp_val(), consider:
unsigned char x = 200u;
y = clamp_val(x, 10u, 300u);
I also suspect that many of the min_t(u16, ...) are actually wrong.
For example copy_data() in printk_ringbuffer.c contains:
data_size = min_t(u16, buf_size, len);
Here buf_size is 'unsigned int' and len 'u16', pass a 64k buffer
(can you prove that doesn't happen?) and no data is returned.
The only reason that most of the min_t() are 'fine' is that pretty
much all the value in the kernel are between 0 and INT_MAX.
Patch 1 adds min_unsigned(), this uses integer promotions to convert
both arguments to 'unsigned long long'. It can be used to compare a
signed type that is known to contain a non-negative value with an
unsigned type. The compiler typically optimises it all away.
Added first so that it can be referred to in patch 2.
Patch 2 replaces the 'same type' check with a 'same signedness' one.
This makes min(unsigned_int_var, sizeof()) be ok.
The error message is also improved and will contain the expanded
form of both arguments (useful for seeing how constants are defined).
Patch 3 just fixes some whitespace.
Patch 4 allows comparisons of 'unsigned char' and 'unsigned short'
to signed types. The integer promotion rules convert them both
to 'signed int' prior to the comparison so they can never cause
a negative value be converted to a large positive one.
Patch 5 is slightly more contentious (Linus may not like it!)
effectively adds an (int) cast to all constants between 0 and MAX_INT.
This makes min(signed_int_var, sizeof()) be ok.
With all the patches applied pretty much all the min_t() could be
replaced by min(), and most of the rest by min_unsigned().
However they all need careful inspection due to code like:
sz = min_t(unsigned char, sz - 1, LIM - 1) + 1;
which converts 0 to LIM.
v2 Fixes some issues found by the kernel build robot.
No functional changes.
David Laight (5):
Add min_unsigned(a, b) and max_unsigned(a, b)
Allow min()/max()/clamp() if the arguments have the same signedness.
Fix indentation of __cmp_once() and __clamp_once()
Allow comparisons of 'int' against 'unsigned char/short'.
Relax check to allow comparison between int and small unsigned
constants.
include/linux/minmax.h | 98 ++++++++++++++++++++++++++----------------
1 file changed, 61 insertions(+), 37 deletions(-)
--
2.17.1
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
next reply other threads:[~2023-07-28 14:49 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-28 14:49 David Laight [this message]
2023-07-28 14:50 ` [PATCH next v2 1/5] minmax: Add min_unsigned(a, b) and max_unsigned(a, b) David Laight
2023-07-28 14:51 ` [PATCH next v2 2/5] minmax: Allow min()/max()/clamp() if the arguments have the same signedness David Laight
2023-07-29 2:00 ` kernel test robot
2023-07-30 14:55 ` David Laight
2023-07-30 17:05 ` Nathan Chancellor
2023-07-30 17:33 ` David Laight
2023-07-31 21:43 ` kernel test robot
2023-08-01 8:45 ` David Laight
2023-07-28 14:52 ` [PATCH next v2 3/5] minmax: Fix indentation of __cmp_once() and __clamp_once() David Laight
2023-07-28 14:52 ` [PATCH next v2 4/5] minmax: Allow comparisons of 'int' against 'unsigned char/short' David Laight
2023-07-28 14:53 ` [PATCH next v2 5/5] minmax: Relax check to allow comparison between int and small unsigned constants David Laight
2023-07-28 21:43 ` kernel test robot
2023-07-30 16:55 ` David Laight
2023-07-31 13:26 ` David Laight
2023-08-01 16:02 ` Nick Desaulniers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=bde3d2dc933848bbaceeb9b7102f6f4c@AcuMS.aculab.com \
--to=david.laight@aculab.com \
--cc=Jason@zx2c4.com \
--cc=akpm@linux-foundation.org \
--cc=andriy.shevchenko@linux.intel.com \
--cc=hch@infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=willy@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox