Most of the inline bitmap functions are buggy if passed a compile-time
constant nbits==0. The convention is that the caller only guarantees
BITS_TO_LONGS(nbits) words can be accessed, which for nbits==0 is of
course 0. However, all the small_const_nbits() cases proceed to
dereferencing the passed src or dst pointers unconditionally.
Of course, nobody passes a literal 0 as nbits, but it could come about
from some odd CONFIG_ combination, or because the compiler is smart
enough to reduce some expression to 0, or... In any case, this patch is
just for the build-bots to chew on for various .config and arches to see
if we have any.
Since most (if not all, I'll check) of the out-of-line implementations
handle nbits==0 correctly, I'll probably just unconditionally add the
nbits>0 clause to small_const_nbits() to force the ool versions to be
used if any compile-time zero-size bitmap should turn up.
Not-really-signed-off-by: Rasmus Villemoes <[email protected]>
---
include/linux/bitmap.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index 1ee46f492267..a5879cb45687 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -196,8 +196,10 @@ extern int bitmap_print_to_pagebuf(bool list, char *buf,
#define BITMAP_FIRST_WORD_MASK(start) (~0UL << ((start) & (BITS_PER_LONG - 1)))
#define BITMAP_LAST_WORD_MASK(nbits) (~0UL >> (-(nbits) & (BITS_PER_LONG - 1)))
+int const_zero_size_bitmaps_are_buggy(void);
#define small_const_nbits(nbits) \
- (__builtin_constant_p(nbits) && (nbits) <= BITS_PER_LONG)
+ (__builtin_constant_p(nbits) && (nbits) <= BITS_PER_LONG && \
+ ((nbits) > 0 || const_zero_size_bitmaps_are_buggy()))
static inline void bitmap_zero(unsigned long *dst, unsigned int nbits)
{
--
2.16.4
Hi Rasmus,
On Wed, Aug 15, 2018 at 10:55:39AM +0200, Rasmus Villemoes wrote:
> Most of the inline bitmap functions are buggy if passed a compile-time
> constant nbits==0.
I think it's bad wording. Functions are OK. The problem is in users.
Also, run-time nbits==0 is buggy as well. On the other hand, we have a rule
in include/linux/bitmap.h:
* Note that nbits should be always a compile time evaluable constant.
* Otherwise many inlines will generate horrible code.
So runtime-defined nbits is bad thing by itself.
> The convention is that the caller only guarantees
> BITS_TO_LONGS(nbits) words can be accessed, which for nbits==0 is of
> course 0. However, all the small_const_nbits() cases proceed to
> dereferencing the passed src or dst pointers unconditionally.
>
> Of course, nobody passes a literal 0 as nbits, but it could come about
> from some odd CONFIG_ combination, or because the compiler is smart
> enough to reduce some expression to 0, or... In any case, this patch is
> just for the build-bots to chew on for various .config and arches to see
> if we have any.
>
> Since most (if not all, I'll check) of the out-of-line implementations
> handle nbits==0 correctly, I'll probably just unconditionally add the
> nbits>0 clause to small_const_nbits() to force the ool versions to be
> used if any compile-time zero-size bitmap should turn up.
>
> Not-really-signed-off-by: Rasmus Villemoes <[email protected]>
> ---
> include/linux/bitmap.h | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> index 1ee46f492267..a5879cb45687 100644
> --- a/include/linux/bitmap.h
> +++ b/include/linux/bitmap.h
> @@ -196,8 +196,10 @@ extern int bitmap_print_to_pagebuf(bool list, char *buf,
> #define BITMAP_FIRST_WORD_MASK(start) (~0UL << ((start) & (BITS_PER_LONG - 1)))
> #define BITMAP_LAST_WORD_MASK(nbits) (~0UL >> (-(nbits) & (BITS_PER_LONG - 1)))
>
> +int const_zero_size_bitmaps_are_buggy(void);
It introduces undefined symbol and uses it in pretty unusual way. I think
it worth to add comment about it.
> #define small_const_nbits(nbits) \
> - (__builtin_constant_p(nbits) && (nbits) <= BITS_PER_LONG)
> + (__builtin_constant_p(nbits) && (nbits) <= BITS_PER_LONG && \
> + ((nbits) > 0 || const_zero_size_bitmaps_are_buggy()))
Some functions in bitmap API has signed type for nbits. (This is wrong
by itself. I think I'll write patch for it.) So in fact you check here
that nbits is not negative as well. Would it be better name like
const_nonpositive_size_bitmaps_are_buggy?
Yury
>
> static inline void bitmap_zero(unsigned long *dst, unsigned int nbits)
> {
> --
> 2.16.4
On 2018-08-17 01:21, Yury Norov wrote:
> Hi Rasmus,
>
> On Wed, Aug 15, 2018 at 10:55:39AM +0200, Rasmus Villemoes wrote:
>> Most of the inline bitmap functions are buggy if passed a compile-time
>> constant nbits==0.
>
> I think it's bad wording. Functions are OK. The problem is in users.
Why shouldn't we handle a zero-size bitmap gracefully? In any case, if
you believe nbits==0 is an implicit violation of the API, all the more
reason to detect if we have such users.
> Also, run-time nbits==0 is buggy as well.
No, not according to my reading of lib/bitmap.c. Can you point out any
function in there that doesn't behave correctly when passed nbits==0?
On the other hand, we have a rule
> in include/linux/bitmap.h:
> * Note that nbits should be always a compile time evaluable constant.
> * Otherwise many inlines will generate horrible code.
> So runtime-defined nbits is bad thing by itself.
Yeah, that ship has sailed long ago. We have lots of users where for
whatever reason nbits is not constant and/or is bigger than
BITS_PER_LONG, and a function call isn't really that horrible. Probably
that piece of doc should be removed or relaxed. I have a small series
that removes another piece of wrong doc, I'll tweak this as well.
One thing that might be worth looking into is to see if we have some
places where nbits is not compile-time const, but is compile-time known
to be in 1 <= nbits <= BITS_PER_LONG, and use the inline branches in
those cases as well. Matthew already did something like that with
2c6deb01525, where we use the inline case when nbits are known to be a
multiple of 8.
>>
>> Not-really-signed-off-by: Rasmus Villemoes <[email protected]>
>> ---
>>
>> +int const_zero_size_bitmaps_are_buggy(void);
>
> It introduces undefined symbol and uses it in pretty unusual way. I think
> it worth to add comment about it.
So I thought the subject and the not-really-signed-off made it clear
enough: I don't mean for this patch to get upstream, it was only for the
buildbot to chew on and send me a build error report if we did indeed
have any such users.
The real fix(es) I'm planning to send is in
https://github.com/Villemoes/linux/tree/4.18_bitmap-zero-fix , but I'm
still waiting for the buildbot to send a report for the fake patch.
>> #define small_const_nbits(nbits) \
>> - (__builtin_constant_p(nbits) && (nbits) <= BITS_PER_LONG)
>> + (__builtin_constant_p(nbits) && (nbits) <= BITS_PER_LONG && \
>> + ((nbits) > 0 || const_zero_size_bitmaps_are_buggy()))
>
> Some functions in bitmap API has signed type for nbits. (This is wrong
> by itself. I think I'll write patch for it.)
I thought I fixed that years ago, but looking again, it seems
2fbad29917c98 failed to update bitmap_shift_right properly. There may be
other extern functions with signed nbits, but I think bitmap_shift_right
is the only inline that passes a signed nbits to small_const_nbits().
I'll include a fix in the above series.
So in fact you check here
> that nbits is not negative as well. Would it be better name like
> const_nonpositive_size_bitmaps_are_buggy?
The name of the undefined function is irrelevant per the above.
Rasmus