linux-kernel - RE: [PATCH v4 07/13] minmax: Introduce {min,max}

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 16 Jun 2023 09:08:22 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Herve Codina' <herve.codina@...tlin.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>
CC:     Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        "Rob Herring" <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>,
        "Kuninori Morimoto" <kuninori.morimoto.gx@...esas.com>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        Christophe Leroy <christophe.leroy@...roup.eu>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: RE: [PATCH v4 07/13] minmax: Introduce {min,max}_array()

From: Herve Codina
> Sent: 15 June 2023 10:35
> > ...
> >
> > > > > > > +       typeof(__array[0] + 0) __element = __array[--__len];    \
> > > > > >
> > > > > > Do we need the ' + 0' part?
> > > > >
> > > > > Yes.
> > > > >
> > > > > __array can be an array of const items and it is legitimate to get the
> > > > > minimum value from const items.
> > > > >
> > > > > typeof(__array[0]) keeps the const qualifier but we need to assign __element
> > > > > in the loop.
> > > > > One way to drop the const qualifier is to get the type from a rvalue computed
> > > > > from __array[0]. This rvalue has to have the exact same type with only the const
> > > > > dropped.
> > > > > '__array[0] + 0' was a perfect canditate.
> > > >
> > > > Seems like this also deserves a comment. But if the series is accepted
> > > > as is, it may be done as a follow up.
> > > >
> > >
> > > Finally not so simple ...
> > > I did some deeper tests and the macros need to be fixed.
> > >
> > > I hope this one (with comments added) is correct:
> > > --- 8 ---
> > > /*
> > >  * Do not check the array parameter using __must_be_array().
> > >  * In the following legit use-case where the "array" passed is a simple pointer,
> > >  * __must_be_array() will return a failure.
> > >  * --- 8< ---
> > >  * int *buff
> > >  * ...
> > >  * min = min_array(buff, nb_items);
> > >  * --- 8< ---
> > >  *
> > >  * The first typeof(&(array)[0]) is needed in order to support arrays of both
> > >  * 'int *buff' and 'int buf[N]' types.
> > >  *
> > >  * typeof(__array[0] + 0) used for __element is needed as the array can be an
> > >  * array of const items.
> > >  * In order to discard the const qualifier use an arithmetic operation (rvalue).
> >
> >
> > >  * This arithmetic operation discard the const but also can lead to an integer
> >
> > discards
> >
> > >  * promotion. For instance, a const s8 __array[0] lead to an int __element due
> >
> > leads
> >
> > >  * to the promotion.
> > >  * In this case, simple min() or max() operation fails (type mismatch).
> > >  * Use min_t() or max_t() (op_t parameter) enforcing the type in order to avoid
> > >  * the min() or max() failure.
> >
> > This part perhaps can be avoided. See below.
> >
> > >  */
> > > #define __minmax_array(op_t, array, len) ({                     \
> > >         typeof(&(array)[0]) __array = (array);                  \
> > >         typeof(len) __len = (len);                              \
> > >         typeof(__array[0] + 0) __element = __array[--__len];    \
> > >         while (__len--)                                         \
> > >                 __element = op_t(typeof(__array[0]), __element, __array[__len]); \
> >
> > But can't we instead have typeof(+(array[0])) in the definition of __element?
> > There are also other possible solutions: a) _Generic() with listed
> > const types to move them to non-const, and b) __auto_type (which is
> > supported by GCC 4.9 and clang, but not in the C11 standard).
> 
> typeof(+(array[0])) keeps the promotion.
> 
> __auto_type works with my gcc-12 but not with a gcc-5.5. Depending on the
> compiler version, it discards or keeps the const qualifier. For this reason
> I would prefer to not use it.

Just define two variables typeof(__array[0] + 0) one for an element
and one for the limit.
The just test (eg):
	if (limit > item) limit = item;
finally cast the limit back to the original type.
The promotions of char/short to signed int won't matter.
There is no need for all the type-checking in min/max.

Indeed, if min_t(type, a, b) is in anyway sane it should
expand to:
	type _a = a, _b = b;
	_a < _b ? _a : _b
without any of the checks that min() does.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)