Re: [PATCH v2 2/2] linux/bits.h: Add compile time sanity check of GENMASK inputs

From: Guenter Roeck <linux@roeck-us.net>
To: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Rikard Falkeborn <rikard.falkeborn@gmail.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Joe Perches <joe@perches.com>,
	Johannes Berg <johannes@sipsolutions.net>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 2/2] linux/bits.h: Add compile time sanity check of GENMASK inputs
Date: Wed, 7 Aug 2019 09:52:33 -0700	[thread overview]
Message-ID: <d1bd1c05-3e92-1a7f-ebd3-3b26981a6f8e@roeck-us.net> (raw)
In-Reply-To: <CAK7LNATGuO0D0a-sTvWw5Pzkn5C7jrLiS=rCwiRsEqaS86KbuQ@mail.gmail.com>

On 8/7/19 7:55 AM, Masahiro Yamada wrote:
> On Wed, Aug 7, 2019 at 11:27 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On Fri, Aug 02, 2019 at 01:03:58AM +0200, Rikard Falkeborn wrote:
>>> GENMASK() and GENMASK_ULL() are supposed to be called with the high bit
>>> as the first argument and the low bit as the second argument. Mixing
>>> them will return a mask with zero bits set.
>>>
>>> Recent commits show getting this wrong is not uncommon, see e.g.
>>> commit aa4c0c9091b0 ("net: stmmac: Fix misuses of GENMASK macro") and
>>> commit 9bdd7bb3a844 ("clocksource/drivers/npcm: Fix misuse of GENMASK
>>> macro").
>>>
>>> To prevent such mistakes from appearing again, add compile time sanity
>>> checking to the arguments of GENMASK() and GENMASK_ULL(). If both the
>>> arguments are known at compile time, and the low bit is higher than the
>>> high bit, break the build to detect the mistake immediately.
>>>
>>> Since GENMASK() is used in declarations, BUILD_BUG_ON_ZERO() must be
>>> used instead of BUILD_BUG_ON(), and __is_constexpr() must be used instead
>>> of __builtin_constant_p().
>>>
>>> If successful, BUILD_BUG_OR_ZERO() returns 0 of type size_t. To avoid
>>> problems with implicit conversions, cast the result of BUILD_BUG_OR_ZERO
>>> to unsigned long.
>>>
>>> Since both BUILD_BUG_ON_ZERO() and __is_constexpr() only uses sizeof()
>>> on the arguments passed to them, neither of them evaluate the expression
>>> unless it is a VLA. Therefore, GENMASK(1, x++) still behaves as
>>> expected.
>>>
>>> Commit 95b980d62d52 ("linux/bits.h: make BIT(), GENMASK(), and friends
>>> available in assembly") made the macros in linux/bits.h available in
>>> assembly. Since neither BUILD_BUG_OR_ZERO() or __is_constexpr() are asm
>>> compatible, disable the checks if the file is included in an asm file.
>>>
>>
>> Who is going to fix the fallout ? For example, arm64:defconfig no longer
>> compiles with this patch applied.
>>
>> It seems to me that the benefit of catching misuses of GENMASK is much
>> less than the fallout from no longer compiling kernels, since those
>> kernels won't get any test coverage at all anymore.
> 
> 
> We cannot apply this until we fix all errors.
> 
> I do not understand why Andrew picked up this so soon.
> 

The same was done with the fallthrough warning in mainline, which still results
in all "sh" builds failing there (and in -next, obviously). I don't understand
the logic either, but maybe it is the new normal.

Guenter