On Tue, Nov 07, 2023 at 05:44:00PM +0100, Alexander Lobakin wrote: > From: Alexander Potapenko <[email protected]> > Date: Tue, 7 Nov 2023 17:33:56 +0100 > > > On Tue, Nov 7, 2023 at 2:23 PM Alexander Lobakin > > <[email protected]> wrote: > >> > >> From: Andy Shevchenko <[email protected]> > >> Date: Mon, 6 Nov 2023 20:23:52 +0200 > >> > >>> On Mon, Nov 06, 2023 at 05:31:34PM +0100, Alexander Lobakin wrote: > >>> > >>>>> | Reported-by: kernel test robot <[email protected]> > >>>>> | Closes: > >>>>> https://lore.kernel.org/oe-kbuild-all/[email protected]/ > >>> > >>>> Not sure how to approach this :z It was also captured on the version you > >>>> sent 2 weeks ago, so this could've been resolved already. > >>> > >>> Is it in the repository already? if so, we should revert it. > >>> Otherwise you have time to think and fix. > >> > >> Nah, neither Alex' series nor mine. And I'd say this should rather be > >> resolved in the functions Alex introduce. > >> > >> Thanks, > >> Olek > > > > Sorry, I couldn't reproduce the problem using the instructions at > > https://download.01.org/0day-ci/archive/20231017/[email protected]/reproduce > > locally, maybe that's because I only have gcc-11 and higher. > > > > But if I'm understanding correctly what's going on, then GCC will be > > reporting the same issue in the following code: > > > > ======================================================= > > #include <stddef.h> > > #include <stdbool.h> > > > > #define BITS_PER_LONG 64 > > #define unlikely(x) x > > #define UL(x) (x##UL) > > #define GENMASK(h, l) \ > > (((~UL(0)) - (UL(1) << (l)) + 1) & \ > > (~UL(0) >> (BITS_PER_LONG - 1 - (h)))) > > > > #define BIT_WORD(nr) ((nr) / BITS_PER_LONG) > > #define BITMAP_FIRST_WORD_MASK(start) (~0UL << ((start) & (BITS_PER_LONG - > > 1))) > > #define BITMAP_LAST_WORD_MASK(nbits) (~0UL >> (-(nbits) & (BITS_PER_LONG - > > 1))) > > > > inline void bitmap_write(unsigned long *map, > > unsigned long value, > > unsigned long start, unsigned long nbits) > > { > > size_t index; > > unsigned long offset; > > unsigned long space; > > unsigned long mask; > > bool fit; > > > > if (unlikely(!nbits)) > > return; > > > > mask = BITMAP_LAST_WORD_MASK(nbits); > > value &= mask; > > offset = start % BITS_PER_LONG; > > space = BITS_PER_LONG - offset; > > fit = space >= nbits; > > index = BIT_WORD(start); > > > > map[index] &= (fit ? (~(mask << offset)) : > > ~BITMAP_FIRST_WORD_MASK(start)); > > map[index] |= value << offset; > > if (fit) > > return; > > > > map[index + 1] &= BITMAP_FIRST_WORD_MASK(start + nbits); > > map[index + 1] |= (value >> space); > > } > > > > unsigned long foo(unsigned int n) { > > unsigned long bm[1] = {0}; > > bitmap_write(bm, 1, n, 2); > > return bm[0]; > > } > > ======================================================= > > (see also https://godbolt.org/z/GfGfYje53) > > > > If so, the problem is not specific to GCC 9, trunk GCC also barks on this > > code: > > > > ======================================================= > > In function 'bitmap_write', > > inlined from 'bitmap_write' at <source>:15:13, > > inlined from 'foo' at <source>:47:7: > > <source>:40:12: warning: array subscript 1 is outside array bounds of > > 'long unsigned int[1]' [-Warray-bounds=] > > 40 | map[index + 1] &= BITMAP_FIRST_WORD_MASK(start + nbits); > > | ~~~^~~~~~~~~~~ > > ======================================================= > > > > If this is true for the code in drivers/gpio/gpio-pca953x.c, > > suppressing the report for GCC 9 won't help for other versions. > > Given that this report is isolated in a single file, we probably need > > I tested it on GCC 9 using modified make.cross from lkp and it triggers > on one more file: > > drivers/thermal/intel/intel_soc_dts_iosf.c: In function 'sys_get_curr_temp': > ./include/linux/bitmap.h:601:18: error: array subscript [1, > 288230376151711744] is outside array bounds of 'long unsigned int[1]' > [-Werror=array-bounds] > > > to give the compiler some hints about the range of values passed to > > bitmap_write() rather than suppressing the optimizations. > > OPTIMIZER_HIDE_VAR() doesn't disable optimizations if I get it > correctly, rather shuts up the compiler in cases like this one. > > I've been thinking of using __member_size() from fortify-string.h, we > could probably optimize the object code even a bit more while silencing > this warning. > Adding Kees, maybe he'd like to participate in sorting this out as well.
I'm trying to find all the pieces for this code, so I might be working from the wrong version or something, but I think this is the change: https://github.com/alobakin/linux/commit/66808fb20fed014a522b868322d54daef14a6bd8 and the induced warning is correctly analyzed in this thread (i.e. GCC can't convince itself that it'll never reach the out of bounds access). Does this work? - if (fit) + if (fit || index + 1 >= __member_size(map)) return; -- Kees Cook
