On Thu, Oct 29, 2020 at 11:44:47PM +0100, Arnd Bergmann wrote: > On Sun, Oct 18, 2020 at 11:44 PM Syed Nayyar Waris wrote: > > > > This patch reimplements the xgpio_set_multiple() function in > > drivers/gpio/gpio-xilinx.c to use the new generic functions: > > bitmap_get_value() and bitmap_set_value(). The code is now simpler > > to read and understand. Moreover, instead of looping for each bit > > in xgpio_set_multiple() function, now we can check each channel at > > a time and save cycles. > > This now causes -Wtype-limits warnings in linux-next with gcc-10: Hi Arnd, What version of gcc-10 are you running? I'm having trouble generating these warnings so I suspect I'm using a different version than you. Regardless I can see your concern about the code, and I think I have a solution. > > > + u32 *const state = chip->gpio_state; > > + unsigned int *const width = chip->gpio_width; > > + > > + DECLARE_BITMAP(old, 64); > > + DECLARE_BITMAP(new, 64); > > + DECLARE_BITMAP(changed, 64); > > + > > + spin_lock_irqsave(&chip->gpio_lock[0], flags); > > + spin_lock(&chip->gpio_lock[1]); > > + > > + bitmap_set_value(old, state[0], 0, width[0]); > > + bitmap_set_value(old, state[1], width[0], width[1]); > > In file included from ../include/linux/cpumask.h:12, > from ../arch/x86/include/asm/cpumask.h:5, > from ../arch/x86/include/asm/msr.h:11, > from ../arch/x86/include/asm/processor.h:22, > from ../arch/x86/include/asm/timex.h:5, > from ../include/linux/timex.h:65, > from ../include/linux/time32.h:13, > from ../include/linux/time.h:73, > from ../include/linux/stat.h:19, > from ../include/linux/module.h:13, > from ../drivers/gpio/gpio-xilinx.c:11: > ../include/linux/bitmap.h:639:18: warning: array subscript [1, > 67108864] is outside array bounds of 'long unsigned int[1]' > [-Warray-bounds] > 639 | map[index + 1] |= value >> space; > | ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~ > In file included from ../include/linux/kasan-checks.h:5, > from ../include/asm-generic/rwonce.h:26, > from ./arch/x86/include/generated/asm/rwonce.h:1, > from ../include/linux/compiler.h:246, > from ../include/linux/build_bug.h:5, > from ../include/linux/bits.h:22, > from ../include/linux/bitops.h:6, > from ../drivers/gpio/gpio-xilinx.c:8: > ../drivers/gpio/gpio-xilinx.c:144:17: note: while referencing 'old' > 144 | DECLARE_BITMAP(old, 64); > | ^~~ > ../include/linux/types.h:11:16: note: in definition of macro 'DECLARE_BITMAP' > 11 | unsigned long name[BITS_TO_LONGS(bits)] > | ^~~~ > In file included from ../include/linux/cpumask.h:12, > from ../arch/x86/include/asm/cpumask.h:5, > from ../arch/x86/include/asm/msr.h:11, > from ../arch/x86/include/asm/processor.h:22, > from ../arch/x86/include/asm/timex.h:5, > from ../include/linux/timex.h:65, > from ../include/linux/time32.h:13, > from ../include/linux/time.h:73, > from ../include/linux/stat.h:19, > from ../include/linux/module.h:13, > from ../drivers/gpio/gpio-xilinx.c:11: > > The compiler clearly tries to do range-checking here and notices > that the index into the fixed-length array on the stack is not correctly > bounded. It seems this would happen whenever width[0] + width[1] > is larger than 64. > > I have just submitted patches for all other -Wtype-limits warnings > and would like to enable this option by default. Can you try to find > a way to make this code safer? I would expect that you need a > variant of bitmap_set_value() that takes an explicit ceiling here, > and checks the stand and nbits values against that. > > Arnd Let me first verify that I understand the problem correctly. The issue is the possibility of a stack smash in bitmap_set_value() when the value of start + nbits is larger than the length of the map bitmap memory region. This is because index (or index + 1) could be outside the range of the bitmap memory region passed in as map. Is my understanding correct here? In xgpio_set_multiple(), the variables width[0] and width[1] serve as possible start and nbits values for the bitmap_set_value() calls. Because width[0] and width[1] are unsigned int variables, GCC considers the possibility that the value of width[0]/width[1] might exceed the length of the bitmap memory region named old and thus result in a stack smash. I don't know if invalid width values are actually possible for the Xilinx gpio device, but let's err on the side of safety and assume this is actually a possibility. We should verify that the combined value of gpio_width[0] + gpio_width[1] does not exceed 64 bits; we can add a check for this in xgpio_probe() when we grab the gpio_width values. However, we're still left with the GCC warnings because GCC is not smart enough to know that we've already checked the boundary and width[0] and width[1] are valid values. I suspect we can avoid this warning is we refactor bitmap_set_value() to increment map seperately and then set it: static inline void bitmap_set_value(unsigned long *map, unsigned long value, unsigned long start, unsigned long nbits) { const unsigned long offset = start % BITS_PER_LONG; const unsigned long ceiling = round_up(start + 1, BITS_PER_LONG); const unsigned long space = ceiling - start; map += BIT_WORD(start); value &= GENMASK(nbits - 1, 0); if (space >= nbits) { *map &= ~(GENMASK(nbits - 1, 0) << offset); *map |= value << offset; } else { *map &= ~BITMAP_FIRST_WORD_MASK(start); *map |= value << offset; map++; *map &= ~BITMAP_LAST_WORD_MASK(start + nbits); *map |= value >> space; } } This avoids adding a costly conditional check inside bitmap_set_value() when almost all bitmap_set_value() calls will have static arguments with well-defined and obvious boundaries. Do you think this would be an acceptable solution to resolve your GCC warnings? Sincerely, William Breathitt Gray