Re: [PATCH v3 0/3] Add compile time sanity check of GENMASK inputs

From: Rikard Falkeborn <rikard.falkeborn@gmail.com>
To: Rikard Falkeborn <rikard.falkeborn@gmail.com>
Cc: akpm@linux-foundation.org, joe@perches.com,
	johannes@sipsolutions.net, linux-kernel@vger.kernel.org,
	yamada.masahiro@socionext.com,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	Kees Cook <keescook@chromium.org>,
	x86@kernel.org
Subject: Re: [PATCH v3 0/3] Add compile time sanity check of GENMASK inputs
Date: Sat, 5 Oct 2019 21:46:27 +0200	[thread overview]
Message-ID: <20191005194627.GA1817543@rikard> (raw)
In-Reply-To: <20190811184938.1796-1-rikard.falkeborn@gmail.com>

On Sun, Aug 11, 2019 at 08:49:35PM +0200, Rikard Falkeborn wrote:
> Hello,
> 
> A new attempt to try to add build time validity checks of GENMASK (and
> GENMASK_ULL) inputs. There main differences from v2:
> 
> Remove a define of BUILD_BUG_ON in x86/boot to avoid a compiler warning
> about redefining BUILD_BUG_ON. Instead, use the common one from
> include/.
> 
> Drop patch 2 in v2 where GENMASK arguments where made more verbose.
> 
> Add a cast in the BUILD_BUG_ON_ZERO macro change the type to int to
> avoid the somewhat clumpsy casts of BUILD_BUG_ON_ZERO. The second patch
> in this series adds such a cast to BUILD_BUG_ON_ZERO, which makes it
> possible to avoid casts when using BUILD_BUG_ON_ZERO in patch 3.
> 
> I have checked all users of BUILD_BUG_ON_ZERO and I did not find a case
> where adding a cast to int would affect existing users but I'd feel much
> more comfortable if someone else double (or tripple) checked (there are
> ~80 instances plus ~10 copies in tools). Perhaps I should have CC:d
> maintainers of files using BUILD_BUG_ON_ZERO?
> 
> Finally, use __builtin_constant_p instead of __is_constexpr. This avoids
> pulling in kernel.h in bits.h.
> 
> Joe Perches sent a patch series to fix existing misuses, currently there
> are five such misuses (which patches pending) left in Linus tree and two
> (with patches pending) in linux-next. Those patches should fix all
> "simple" misuses of GENMASK (cases where the arguments are numerical
> constants). Pushing v2 to linux-next also revealed an arm-specific
> misuse where GENMASK was used in another macro (and also broke the
> arm-builds). There is a patch to fix that by Nathan Chancellor (not in
> linux-next yet). Those patches should be merged before the last patch of
> this series to avoid breaking builds.
> 

Ping.

The current status is that patch 1 has been merged into Linus tree
through the x86 tree. The patches mentioned about actually fixing the
existing misuses have all been merged except two of Joe Perches patches
[0], [1], but those patches fixes misuse in unused macros, and will not
affect anyones build.

I have testbuilt the two remaining patches rebased on top of Linus tree
and on linux-next for aarch64 and x86 without seeing any problems (when
v2 of the series went into linux-next, those were the only archs where
any problems were spotted (build warning on x86, build error on
aarch64)).

[0] https://lore.kernel.org/lkml/cddd7ad7e9f81dec1e86c106f04229d21fc21920.1562734889.git.joe@perches.com/
[1] https://lore.kernel.org/lkml/d149d2851f9aa2425c927cb8e311e20c4b83e186.1562734889.git.joe@perches.com/

Rikard