Messages in this thread | | | Date | Tue, 25 Jul 2023 13:38:24 +0300 | From | 'Andy Shevchenko' <> | Subject | Re: [PATCH next 0/5] minmax: Relax type checks in min() and max(). |
| |
On Tue, Jul 25, 2023 at 10:00:40AM +0000, David Laight wrote: > 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.
I don't know how you made this series, but it has no thread. You need to use --thread when forming the patch series.
-- With Best Regards, Andy Shevchenko
| |