[PATCH v2] bitops: Fix shift overflow in GENMASK macros

[PATCH v2] bitops: Fix shift overflow in GENMASK macros

[Maxime COQUELIN]

On some 32 bits architectures, including x86, GENMASK(31, 0) returns 0
instead of the expected ~0UL.

This is the same on some 64 bits architectures with GENMASK_ULL(63, 0).

This is due to an overflow in the shift operand, 1 << 32 for GENMASK,
1 << 64 for GENMASK_ULL.

Fixes: 10ef6b0dffe404bcc54e94cb2ca1a5b18445a66b
Cc: <stable@vger.kernel.org> #v3.13+
Reported-by: Eric Paire <eric.paire@st.com>
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>
---
 include/linux/bitops.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index be5fd38..a49d7b7 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -18,8 +18,8 @@
  * position @h. For example
  * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
  */
-#define GENMASK(h, l)		(((U32_C(1) << ((h) - (l) + 1)) - 1) << (l))
-#define GENMASK_ULL(h, l)	(((U64_C(1) << ((h) - (l) + 1)) - 1) << (l))
+#define GENMASK(h, l)     ((~0UL >> (BITS_PER_LONG - (h - l + 1))) << l)
+#define GENMASK_ULL(h, l) ((~0ULL >> (BITS_PER_LONG_LONG - (h - l + 1))) << l)
 
 extern unsigned int __sw_hweight8(unsigned int w);
 extern unsigned int __sw_hweight16(unsigned int w);
-- 
1.9.1

Re: [PATCH v2] bitops: Fix shift overflow in GENMASK macros

[Peter Zijlstra]

On Tue, Nov 04, 2014 at 11:03:57AM +0100, Maxime COQUELIN wrote:

> -#define GENMASK(h, l)		(((U32_C(1) << ((h) - (l) + 1)) - 1) << (l))
> -#define GENMASK_ULL(h, l)	(((U64_C(1) << ((h) - (l) + 1)) - 1) << (l))
> +#define GENMASK(h, l)     ((~0UL >> (BITS_PER_LONG - (h - l + 1))) << l)
> +#define GENMASK_ULL(h, l) ((~0ULL >> (BITS_PER_LONG_LONG - (h - l + 1))) << l)

OK, so you need to keep the (h) and (l) bits, macro arguments should be
wrapped in seemingly superfluous braces in order to preserve precedence
on expansion.

My bad for not explicitly doing that when suggesting the alternative.

Re: [PATCH v2] bitops: Fix shift overflow in GENMASK macros

[Maxime Coquelin]

On 11/04/2014 11:44 AM, Peter Zijlstra wrote:
> On Tue, Nov 04, 2014 at 11:03:57AM +0100, Maxime COQUELIN wrote:
>
>> -#define GENMASK(h, l)		(((U32_C(1) << ((h) - (l) + 1)) - 1) << (l))
>> -#define GENMASK_ULL(h, l)	(((U64_C(1) << ((h) - (l) + 1)) - 1) << (l))
>> +#define GENMASK(h, l)     ((~0UL >> (BITS_PER_LONG - (h - l + 1))) << l)
>> +#define GENMASK_ULL(h, l) ((~0ULL >> (BITS_PER_LONG_LONG - (h - l + 1))) << l)
> OK, so you need to keep the (h) and (l) bits, macro arguments should be
> wrapped in seemingly superfluous braces in order to preserve precedence
> on expansion.
You're right, I should have  seen this..
>
> My bad for not explicitly doing that when suggesting the alternative.
Not a problem, v3 is coming.

Maxime