On Fri, May 24, 2024, at 05:00, Andrew Morton wrote:
> ------------------------------------------------------
> From: Yury Norov <yury.norov@xxxxxxxxx>
> Subject: gcc: disable '-Warray-bounds' for gcc-9
> Date: Wed, 22 May 2024 15:58:30 -0700
>
> '-Warray-bounds' is already disabled for gcc-10+. Now that we've merged
> bitmap_{read,write), I see the following error when building the kernel
> with gcc-9.4 (Ubuntu 20.04.4 LTS) for x86_64 allmodconfig:
>
> drivers/pinctrl/pinctrl-cy8c95x0.c: In function
> `cy8c95x0_read_regs_mask.isra.0':
> include/linux/bitmap.h:756:18: error: array subscript [1,
> 288230376151711744] is outside array bounds of `long unsigned int[1]'
> [-Werror=array-bounds]
> 756 | value_high = map[index + 1] & BITMAP_LAST_WORD_MASK(start +
> nbits);
> | ~~~^~~~~~~~~~~
>
> The immediate reason is that the commit b44759705f7d ("bitmap: make
> bitmap_{get,set}_value8() use bitmap_{read,write}()") switched the
> bitmap_get_value8() to an alias of bitmap_read(); the same for 'set'.
>
> Now; the code that triggers Warray-bounds, calls the function like this:
>
> #define MAX_BANK 8
> #define BANK_SZ 8
> #define MAX_LINE (MAX_BANK * BANK_SZ)
> DECLARE_BITMAP(tval, MAX_LINE); // 64-bit map: unsigned long tval[1]
>
> read_val |= bitmap_get_value8(tval, i * BANK_SZ) & ~bits;
>
> bitmap_read() is implemented such that it may conditionally dereference a
> pointer beyond the boundary like this:
>
> unsigned long offset = start % BITS_PER_LONG;
> unsigned long space = BITS_PER_LONG - offset;
>
> if (space >= nbits)
> return (map[index] >> offset) & BITMAP_LAST_WORD_MASK(nbits);
>
> value_low = map[index] & BITMAP_FIRST_WORD_MASK(start);
> value_high = map[index + 1] & BITMAP_LAST_WORD_MASK(start + nbits);
> return (value_low >> offset) | (value_high << space);
>
> In case of bitmap_get_value8(), it's impossible to violate the boundary
> because 'space >= nbits' is never the true for byte-aligned 8-bit access.
> So, this is clearly a false-positive.
>
> The same type of false-positives break my allmodconfig build in many
> places. gcc-8, is clear, however.
I'm not too happy about this one, I think this is mixing up
a couple of independent issues, and makes it harder to
ever enable the warning again.
The bitmap.h code looks suspicious to me, and if gcc is
unable to analyze this as a false positive, it probably
also can't optimize it correctly, so it may be better
to either not have this as an inline function at all,
or find an implementation that gcc can optimize better.
In the meantime, I would suggest reverting b44759705f7d
("bitmap: make bitmap_{get,set}_value8() use
bitmap_{read,write}()"), until the implementation is
improved to work without a warning.
The other problem I see is that the warning is
disabled globally even when building with W=123,
and I think we should change it to always warn at
least with W=1 regardless of the compiler version.
It's also likely that the other false-positive
warnings only happen when sanitizers are enabled,
so we could turn it on by default without sanitizers
and move it to W=1 with sanitizers.
Arnd
