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: <[email protected]> #v3.13+
Reported-by: Eric Paire <[email protected]>
Suggested-by: Rasmus Villemoes <[email protected]>
Signed-off-by: Maxime Coquelin <[email protected]>
---
include/linux/bitops.h | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index be5fd38..5d858e0 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -18,8 +18,11 @@
* 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) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
+
+#define GENMASK_ULL(h, l) \
+ (((~0ULL) << (l)) & (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h))))
extern unsigned int __sw_hweight8(unsigned int w);
extern unsigned int __sw_hweight16(unsigned int w);
--
1.9.1
On Thu, 6 Nov 2014 10:54:19 +0100 Maxime COQUELIN <[email protected]> wrote:
> 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: <[email protected]> #v3.13+
> Reported-by: Eric Paire <[email protected]>
> Suggested-by: Rasmus Villemoes <[email protected]>
> Signed-off-by: Maxime Coquelin <[email protected]>
Why cc:stable? Does this bug cause some observed kernel misbehaviour?
If so, please fully describe that in the changelog. This will help
people to determine whether this patch might fix a bug they're
observing, and will help them to decide whether they should backport
this patch into their kernels.
I'm assuming that Peter will be merging this patch.
On 11/13/2014 11:09 PM, Andrew Morton wrote:
> On Thu, 6 Nov 2014 10:54:19 +0100 Maxime COQUELIN <[email protected]> wrote:
>
>> 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: <[email protected]> #v3.13+
>> Reported-by: Eric Paire <[email protected]>
>> Suggested-by: Rasmus Villemoes <[email protected]>
>> Signed-off-by: Maxime Coquelin <[email protected]>
> Why cc:stable? Does this bug cause some observed kernel misbehaviour?
> If so, please fully describe that in the changelog. This will help
> people to determine whether this patch might fix a bug they're
> observing, and will help them to decide whether they should backport
> this patch into their kernels.
We encountered some misbehavior on not upstreamed code.
Looking at all GENMASK and GENMASK_ULL occurences in v3.18-rc4,
I (only) found one possible candidate in drivers/spi/spi_xtensa-xtfpga.c:
static u32 xtfpga_spi_txrx_word(struct spi_device *spi, unsigned nsecs,
u32 v, u8 bits)
{
struct xtfpga_spi *xspi = spi_master_get_devdata(spi->master);
xspi->data = (xspi->data << bits) | (v & GENMASK(bits - 1, 0));
...
}
Max F., can xtfpga_spi_txrx_word() be called with "bits" = 32?
If yes, then GENMASK(bits - 1, 0) result would be unpredictable on some
architectures.
I don't know if Xtensa architecture is impacted though.
But even if Xtensa SPI driver is not impacted,
maybe future fixes that will be integrated into stable releases will use
GENMASK(),
and so could possibly be impacted by the bug.
Andrew, with this information, do you think we should take this patch in
stable branches?
>
> I'm assuming that Peter will be merging this patch.
Yes, Peter already added this patch in his tree.
>
Kind regards,
Maxime
Hi Maxime,
On Fri, Nov 14, 2014 at 5:07 PM, Maxime Coquelin <[email protected]> wrote:
> Looking at all GENMASK and GENMASK_ULL occurences in v3.18-rc4,
> I (only) found one possible candidate in drivers/spi/spi_xtensa-xtfpga.c:
>
> static u32 xtfpga_spi_txrx_word(struct spi_device *spi, unsigned nsecs,
> u32 v, u8 bits)
> {
> struct xtfpga_spi *xspi = spi_master_get_devdata(spi->master);
>
> xspi->data = (xspi->data << bits) | (v & GENMASK(bits - 1, 0));
> ...
> }
>
> Max F., can xtfpga_spi_txrx_word() be called with "bits" = 32?
No, bits should never be greater than 16, because spi_master sets its
bits_per_word_mask to SPI_BPW_RANGE_MASK(1, 16).
--
Thanks.
-- Max
Commit-ID: 00b4d9a14125f1e51874def2b9de6092e007412d
Gitweb: http://git.kernel.org/tip/00b4d9a14125f1e51874def2b9de6092e007412d
Author: Maxime COQUELIN <[email protected]>
AuthorDate: Thu, 6 Nov 2014 10:54:19 +0100
Committer: Ingo Molnar <[email protected]>
CommitDate: Sun, 16 Nov 2014 09:55:39 +0100
bitops: Fix shift overflow in GENMASK macros
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.
Reported-by: Eric Paire <[email protected]>
Suggested-by: Rasmus Villemoes <[email protected]>
Signed-off-by: Maxime Coquelin <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Cc: <[email protected]> # v3.13+
Cc: [email protected]
Cc: [email protected]
Cc: John Sullivan <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Paul E. McKenney <[email protected]>
Cc: Theodore Ts'o <[email protected]>
Fixes: 10ef6b0dffe4 ("bitops: Introduce a more generic BITMASK macro")
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
include/linux/bitops.h | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index be5fd38..5d858e0 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -18,8 +18,11 @@
* 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) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
+
+#define GENMASK_ULL(h, l) \
+ (((~0ULL) << (l)) & (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h))))
extern unsigned int __sw_hweight8(unsigned int w);
extern unsigned int __sw_hweight16(unsigned int w);