From: Colin King <hidden> Date: 2021-05-13 08:52:36
From: Colin Ian King <redacted>
The left shift of the u32 integer v is evaluated using 32 bit
arithmetic and then assigned to a u64 integer. There are cases
where v will currently overflow on the shift. Avoid this by
casting it to unsigned long (same type as map[]) before shifting
it.
Addresses-Coverity: ("Unintentional integer overflow")
Fixes: 02b3f84d9080 ("gpio: xilinx: Switch to use bitmap APIs")
Signed-off-by: Colin Ian King <redacted>
---
drivers/gpio/gpio-xilinx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: David Laight <hidden> Date: 2021-05-13 09:10:23
From: Colin King
quoted hunk
Sent: 13 May 2021 09:52
From: Colin Ian King <redacted>
The left shift of the u32 integer v is evaluated using 32 bit
arithmetic and then assigned to a u64 integer. There are cases
where v will currently overflow on the shift. Avoid this by
casting it to unsigned long (same type as map[]) before shifting
it.
Addresses-Coverity: ("Unintentional integer overflow")
Fixes: 02b3f84d9080 ("gpio: xilinx: Switch to use bitmap APIs")
Signed-off-by: Colin Ian King <redacted>
---
drivers/gpio/gpio-xilinx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
@@ -99,7 +99,7 @@ static inline void xgpio_set_value32(unsigned long *map, int bit, u32 v)constunsignedlongoffset=(bit%BITS_PER_LONG)&BIT(5);map[index]&=~(0xFFFFFFFFul<<offset);-map[index]|=v<<offset;+map[index]|=(unsignedlong)v<<offset;}
That code looks dubious on 32bit architectures.
I don't have 02b3f84d9080 in any of my source trees.
But that patch may itself be very dubious.
Since the hardware requires explicit bits be set, relying
on the bitmap functions seems pointless and possibly wrong.
Clearly they cause additional problems because they use long[]
and here the code needs u32[].
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
From: Dan Carpenter <hidden> Date: 2021-05-14 05:38:52
On Thu, May 13, 2021 at 09:52:27AM +0100, Colin King wrote:
quoted hunk
From: Colin Ian King <redacted>
The left shift of the u32 integer v is evaluated using 32 bit
arithmetic and then assigned to a u64 integer. There are cases
where v will currently overflow on the shift. Avoid this by
casting it to unsigned long (same type as map[]) before shifting
it.
Addresses-Coverity: ("Unintentional integer overflow")
Fixes: 02b3f84d9080 ("gpio: xilinx: Switch to use bitmap APIs")
Signed-off-by: Colin Ian King <redacted>
---
drivers/gpio/gpio-xilinx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
@@ -99,7 +99,7 @@ static inline void xgpio_set_value32(unsigned long *map, int bit, u32 v)constunsignedlongoffset=(bit%BITS_PER_LONG)&BIT(5);map[index]&=~(0xFFFFFFFFul<<offset);-map[index]|=v<<offset;+map[index]|=(unsignedlong)v<<offset;
Doing a shift by BIT(5) is super weird. It looks like a double shift
bug and should probably trigger a static checker warning. It's like
when people do BIT(BIT(5)).
It would be more readable to write it as:
int shift = (bit % BITS_PER_LONG) ? 32 : 0;
regards,
dan carpenter
From: Andy Shevchenko <hidden> Date: 2021-05-17 07:03:35
On Thu, May 13, 2021 at 12:12 PM Colin King [off-list ref] wrote:
From: Colin Ian King <redacted>
The left shift of the u32 integer v is evaluated using 32 bit
arithmetic and then assigned to a u64 integer. There are cases
where v will currently overflow on the shift. Avoid this by
casting it to unsigned long (same type as map[]) before shifting
it.
Addresses-Coverity: ("Unintentional integer overflow")
Fixes: 02b3f84d9080 ("gpio: xilinx: Switch to use bitmap APIs")
No, it is a false positive,
const unsigned long offset = (bit % BITS_PER_LONG) & BIT(5);
See above, offset is 0 when BITS_PER_LONG == 32 and 32 when it's equal to 64.
That code looks dubious on 32bit architectures.
I don't have 02b3f84d9080 in any of my source trees.
Can you please be more specific on which code is dubious on 32-bit
arches and why?
But that patch may itself be very dubious.
Since the hardware requires explicit bits be set, relying
on the bitmap functions seems pointless and possibly wrong.
Clearly they cause additional problems because they use long[]
and here the code needs u32[].
Not the first place in the kernel with such a trick.
It looks like a double shift
bug and should probably trigger a static checker warning. It's like
when people do BIT(BIT(5)).
It would be more readable to write it as:
int shift = (bit % BITS_PER_LONG) ? 32 : 0;
Usually this code is in a kinda fast path. Have you checked if the
compiler generates the same or better code when you are using ternary?
--
With Best Regards,
Andy Shevchenko
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: 2021-05-17 07:33:05
On Mon, May 17, 2021 at 10:03:15AM +0300, Andy Shevchenko wrote:
On Thu, May 13, 2021 at 12:12 PM Colin King [off-list ref] wrote:
quoted
From: Colin Ian King <redacted>
The left shift of the u32 integer v is evaluated using 32 bit
arithmetic and then assigned to a u64 integer. There are cases
where v will currently overflow on the shift. Avoid this by
casting it to unsigned long (same type as map[]) before shifting
it.
Addresses-Coverity: ("Unintentional integer overflow")
Fixes: 02b3f84d9080 ("gpio: xilinx: Switch to use bitmap APIs")
No, it is a false positive,
quoted
const unsigned long offset = (bit % BITS_PER_LONG) & BIT(5);
See above, offset is 0 when BITS_PER_LONG == 32 and 32 when it's equal to 64.
Not the first place in the kernel with such a trick.
quoted
It looks like a double shift
bug and should probably trigger a static checker warning. It's like
when people do BIT(BIT(5)).
It would be more readable to write it as:
int shift = (bit % BITS_PER_LONG) ? 32 : 0;
Usually this code is in a kinda fast path. Have you checked if the
compiler generates the same or better code when you are using ternary?
I wrote a little benchmark to see which was faster and they're the same
as far as I can see.
regards,
dan carpenter
static inline __attribute__((__gnu_inline__)) unsigned long xgpio_set_value_orig(unsigned long *map, int bit, u32 v)
{
int shift = (bit % 64) & ((((1UL))) << (5));
return v << shift;
}
static inline __attribute__((__gnu_inline__)) unsigned long xgpio_set_value_new(unsigned long *map, int bit, u32 v)
{
int shift = (bit % 64) ? 32 : 0;
return v << shift;
}
int main(void)
{
int i;
for (i = 0; i < INT_MAX; i++)
xgpio_set_value_orig(NULL, i, 0);
// for (i = 0; i < INT_MAX; i++)
// xgpio_set_value_new(NULL, i, 0);
return 0;
}
Not the first place in the kernel with such a trick.
quoted
It looks like a double shift
bug and should probably trigger a static checker warning. It's like
when people do BIT(BIT(5)).
It would be more readable to write it as:
int shift = (bit % BITS_PER_LONG) ? 32 : 0;
Usually this code is in a kinda fast path. Have you checked if the
compiler generates the same or better code when you are using ternary?
I wrote a little benchmark to see which was faster and they're the same
as far as I can see.
Thanks for checking.
Besides the fact that offset should be 0 for 32-bit always and if compiler can
proof that...
The test below doesn't take into account the exact trick is used for offset
(i.e. implicit dependency between BITS_PER_LONG, size of unsigned long, and
using 5th bit out of value). I don't know if compiler can properly optimize
the ternary in this case (but it looks like it should generate the same code).
That said, I would rather to see the diff between assembly of the exact
function before and after your proposal.
static inline __attribute__((__gnu_inline__)) unsigned long xgpio_set_value_orig(unsigned long *map, int bit, u32 v)
{
int shift = (bit % 64) & ((((1UL))) << (5));
return v << shift;
}
static inline __attribute__((__gnu_inline__)) unsigned long xgpio_set_value_new(unsigned long *map, int bit, u32 v)
{
int shift = (bit % 64) ? 32 : 0;
return v << shift;
}
int main(void)
{
int i;
for (i = 0; i < INT_MAX; i++)
xgpio_set_value_orig(NULL, i, 0);
// for (i = 0; i < INT_MAX; i++)
// xgpio_set_value_new(NULL, i, 0);
return 0;
}