On Mon, Nov 09, 2020 at 03:41:53PM +0100, Arnd Bergmann wrote:

quoted

On Mon, Nov 9, 2020 at 2:41 PM William Breathitt Gray
[off-list ref] wrote:

quoted

On Mon, Nov 09, 2020 at 06:04:11PM +0530, Syed Nayyar Waris wrote:

One of my concerns is that we're incurring the latency two additional
conditional checks just to suppress a compiler warning about a case that
wouldn't occur in the actual use of bitmap_set_value(). I'm hoping
there's a way for us to suppress these warnings without adding onto the
latency of this function; given that bitmap_set_value() is intended to
be used in loops, conditionals here could significantly increase latency
in drivers.

At least for this caller, the size check would be a compile-time
constant that can be eliminated.

quoted

I wonder if array_index_nospec() might have the side effect of
suppressing these warnings for us. For example, would this work:

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;
        size_t index = BIT_WORD(start);

        value &= GENMASK(nbits - 1, 0);

        if (space >= nbits) {
                index = array_index_nospec(index, index + 1);

                map[index] &= ~(GENMASK(nbits - 1, 0) << offset);
                map[index] |= value << offset;
        } else {
                index = array_index_nospec(index, index + 2);

                map[index + 0] &= ~BITMAP_FIRST_WORD_MASK(start);
                map[index + 0] |= value << offset;
                map[index + 1] &= ~BITMAP_LAST_WORD_MASK(start + nbits);
                map[index + 1] |= value >> space;
        }
}

Or is this going to produce the same warning because we're not using an
explicit check against the map array size?

https://godbolt.org/z/fxnsG9

It still warns about the 'map[index + 1]' access: from all I can tell,
gcc mainly complains because it cannot rule out that 'space < nbits',
and then it knows the size of 'DECLARE_BITMAP(old, 64)' and finds
that if 'index + 0' is correct, then 'index + 1' overflows that array.

      Arnd

Hi Arnd,

As suggested by William, sharing another solution to suppress the 
compiler warning. Please let me know your views on the below fix. Thanks.

If its alright, I shall submit a (new) v13 patchset soon. Let me know.

@@ -1,5 +1,5 @@
 static inline void bitmap_set_value(unsigned long *map,
-                                    unsigned long value,
+                                    unsigned long value, const size_t length,
                                     unsigned long start, unsigned long nbits)
 {
         const size_t index = BIT_WORD(start);

@@ -15,6 +15,10 @@ static inline void bitmap_set_value(unsigned long *map,
         } else {
                 map[index + 0] &= ~BITMAP_FIRST_WORD_MASK(start);
                 map[index + 0] |= value << offset;
+
+               if (index + 1 >= length)
+                       __builtin_unreachable();
+
                 map[index + 1] &= ~BITMAP_LAST_WORD_MASK(start + nbits);
                 map[index + 1] |= value >> space;
         }