Messages in this thread Patch in this message | | | Date | Tue, 7 Nov 2023 10:32:06 -0800 | From | Yury Norov <> | Subject | Re: [alobakin:pfcp 11/19] include/linux/bitmap.h:642:17: warning: array subscript [1, 1024] is outside array bounds of 'long unsigned int[1]' |
| |
On Tue, Nov 07, 2023 at 06:24:04PM +0100, Alexander Lobakin wrote: > From: Alexander Lobakin <aleksander.lobakin@intel.com> > Date: Tue, 7 Nov 2023 17:44:00 +0100 > > > From: Alexander Potapenko <glider@google.com> > > Date: Tue, 7 Nov 2023 17:33:56 +0100 > > > >> On Tue, Nov 7, 2023 at 2:23 PM Alexander Lobakin > >> <aleksander.lobakin@intel.com> wrote: > > [...] > > > 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. > > This one seems to work. At least previously mad GCC 9.3.0 now sits > quietly, as if I added OPTIMIZER_HIDE_VAR() as Yury suggested.
What's wrong with OPTIMIZER_HIDE_VAR()? The problem is clearly on GCC side, namely - it doesn't realize that the map[index+1] fetch is conditional.
And moreover, it's fixed in later stable builds. I tested 12 and 13, and both are silent.
> Note that ideally @map should be marked as `POS` in both cases to help > Clang, but `POS` gets undefined at the end of fortify-string.h, so I > decided to not do that within this draft. > > Thanks, > Olek > --- > diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h > index e8031a157db5..efa0a0287d7c 100644 > --- a/include/linux/bitmap.h > +++ b/include/linux/bitmap.h > @@ -589,12 +589,14 @@ static inline unsigned long bitmap_read(const > unsigned long *map, > size_t index = BIT_WORD(start); > unsigned long offset = start % BITS_PER_LONG; > unsigned long space = BITS_PER_LONG - offset; > + const size_t map_size = __member_size(map); > unsigned long value_low, value_high; > > if (unlikely(!nbits || nbits > BITS_PER_LONG)) > return 0; > > - if (space >= nbits) > + if ((__builtin_constant_p(map_size) && map_size != SIZE_MAX && > + index + 1 >= map_size / sizeof(long)) || space >= nbits) > return (map[index] >> offset) & BITMAP_LAST_WORD_MASK(nbits);
This silences the compiler, but breaks the code logic and hides potential bugs. After the fix, the following code will become legit:
DECLARE_BITMAP(bitmap, 64);
bitmap_fill(bitmap, 64) char ret = bitmap_read(bitmap, 60, 8); // OK, return 0b00001111
Before this change, the return value would be undef: 0xXXXX1111, and it would (should) trigger Warray-bounds on compile time, because it's a compile-time boundary violation.
On runtime KASAN, UBSAN and whatever *SAN would most likely be silenced too with your fix. So no, this one doesn't seem to work.
> value_low = map[index] & BITMAP_FIRST_WORD_MASK(start); > @@ -620,6 +622,7 @@ static inline unsigned long bitmap_read(const > unsigned long *map, > static inline void bitmap_write(unsigned long *map, unsigned long value, > unsigned long start, unsigned long nbits) > { > + const size_t map_size = __member_size(map); > size_t index; > unsigned long offset; > unsigned long space; > @@ -638,7 +641,9 @@ static inline void bitmap_write(unsigned long *map, > unsigned long value, > > map[index] &= (fit ? (~(mask << offset)) : > ~BITMAP_FIRST_WORD_MASK(start)); > map[index] |= value << offset; > - if (fit) > + > + if ((__builtin_constant_p(map_size) && map_size != SIZE_MAX && > + index + 1 >= map_size / sizeof(long)) || fit) > return; > > map[index + 1] &= BITMAP_FIRST_WORD_MASK(start + nbits);
| |