Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Rikard Falkeborn <rikard.falkeborn@gmail.com>
To:     rikard.falkeborn@gmail.com
Cc:     akpm@linux-foundation.org, joe@perches.com,
        johannes@sipsolutions.net, linux-kernel@vger.kernel.org,
        yamada.masahiro@socionext.com
Subject: [PATCH v2 2/2] linux/bits.h: Add compile time sanity check of GENMASK inputs
Date:   Fri,  2 Aug 2019 01:03:58 +0200
Message-Id: <20190801230358.4193-2-rikard.falkeborn@gmail.com>
In-Reply-To: <20190801230358.4193-1-rikard.falkeborn@gmail.com>
References: <20190731190309.19909-1-rikard.falkeborn@gmail.com>
 <20190801230358.4193-1-rikard.falkeborn@gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.

Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
---
Changes in v2:
  - Add comment about why inputs are not checked when used in asm file
  - Use UL(0) instead of 0
  - Extract mask creation in a separate macro to improve readability
  - Use high and low instead of h and l (part of this was extracted to a
    separate patch)
  - Updated commit message

Joe Perches sent a series to fix the existing misuses of GENMASK() that
needs to be merged before this to avoid build failures. Currently, 6 of
the patches are not in Linus tree, and 2 are not in linux-next.


 include/linux/bits.h | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/include/linux/bits.h b/include/linux/bits.h
index d4466aa42a9c..955e9e43c4a5 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -18,12 +18,30 @@
  * position @high. For example
  * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
  */
-#define GENMASK(high, low) \
+#ifndef __ASSEMBLY__
+#include <linux/build_bug.h>
+#define GENMASK_INPUT_CHECK(high, low) \
+	((unsigned long)BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
+		__is_constexpr(high) && __is_constexpr(low), \
+		(low) > (high), UL(0))))
+#else
+/*
+ * BUILD_BUG_ON_ZERO and __is_constexpr() are not available in h files
+ * included from asm files, disable the input check if that is the case.
+ */
+#define GENMASK_INPUT_CHECK(high, low) UL(0)
+#endif
+
+#define __GENMASK(high, low) \
 	(((~UL(0)) - (UL(1) << (low)) + 1) & \
 	 (~UL(0) >> (BITS_PER_LONG - 1 - (high))))
+#define GENMASK(high, low) \
+	(GENMASK_INPUT_CHECK(high, low) + __GENMASK(high, low))
 
-#define GENMASK_ULL(high, low) \
+#define __GENMASK_ULL(high, low) \
 	(((~ULL(0)) - (ULL(1) << (low)) + 1) & \
 	 (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (high))))
+#define GENMASK_ULL(high, low) \
+	(GENMASK_INPUT_CHECK(high, low) + __GENMASK_ULL(high, low))
 
 #endif	/* __LINUX_BITS_H */
-- 
2.22.0