From: Yury Norov
> Sent: 20 April 2022 23:25
>
> Manipulating 64-bit arrays with bitmap functions is potentially dangerous
> because on 32-bit BE machines the order of halfwords doesn't match. Another
> issue is that compiler may throw a warning about out-of-boundary access.
>
> This patch adds bitmap_{from,to}_arr64 functions in addition to existing
> bitmap_{from,to}_arr32.
>
> Signed-off-by: Yury Norov <[email protected]>
> ---
> include/linux/bitmap.h | 23 +++++++++++++++++----
> lib/bitmap.c | 47 ++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 66 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> index 10d805c2893c..f78c534fb814 100644
> --- a/include/linux/bitmap.h
> +++ b/include/linux/bitmap.h
> @@ -292,6 +292,24 @@ void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap,
> (const unsigned long *) (bitmap), (nbits))
> #endif
>
> +/*
> + * On 64-bit systems bitmaps are represented as u64 arrays internally. On LE32
> + * machines the order of hi and lo parts of nubmers match the bitmap structure.
> + * In both cases conversion is not needed when copying data from/to arrays of
> + * u64.
> + */
> +#if (BITS_PER_LONG == 32) && defined(__BIG_ENDIAN)
I think I'd change the condition to (inverting it):
#if (BITS_PER_LONG == 64) || defined(__LITTLE_ENDIAN)
since that is the condition when the layout matches.
> +void bitmap_from_arr64(unsigned long *bitmap, const u64 *buf, unsigned int nbits);
> +void bitmap_to_arr64(u64 *buf, const unsigned long *bitmap, unsigned int nbits);
> +#else
> +#define bitmap_from_arr64(bitmap, buf, nbits) \
> + bitmap_copy_clear_tail((unsigned long *) (bitmap), \
> + (const unsigned long *) (buf), (nbits))
> +#define bitmap_to_arr64(buf, bitmap, nbits) \
> + bitmap_copy_clear_tail((unsigned long *) (buf), \
> + (const unsigned long *) (bitmap), (nbits))
> +#endif
> +
> static inline int bitmap_and(unsigned long *dst, const unsigned long *src1,
> const unsigned long *src2, unsigned int nbits)
> {
> @@ -596,10 +614,7 @@ static inline void bitmap_next_set_region(unsigned long *bitmap,
> */
> static inline void bitmap_from_u64(unsigned long *dst, u64 mask)
> {
> - dst[0] = mask & ULONG_MAX;
> -
> - if (sizeof(mask) > sizeof(unsigned long))
> - dst[1] = mask >> 32;
> + bitmap_from_arr64(dst, &mask, 64);
> }
I'd leave this alone.
>
> /**
> diff --git a/lib/bitmap.c b/lib/bitmap.c
> index d9a4480af5b9..aea9493f4216 100644
> --- a/lib/bitmap.c
> +++ b/lib/bitmap.c
> @@ -1533,5 +1533,52 @@ void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap, unsigned int nbits)
> buf[halfwords - 1] &= (u32) (UINT_MAX >> ((-nbits) & 31));
> }
> EXPORT_SYMBOL(bitmap_to_arr32);
> +#endif
> +
> +#if (BITS_PER_LONG == 32) && defined(__BIG_ENDIAN)
> +/**
> + * bitmap_from_arr64 - copy the contents of u64 array of bits to bitmap
> + * @bitmap: array of unsigned longs, the destination bitmap
> + * @buf: array of u64 (in host byte order), the source bitmap
> + * @nbits: number of bits in @bitmap
> + */
> +void bitmap_from_arr64(unsigned long *bitmap, const u64 *buf, unsigned int nbits)
> +{
> + while (nbits > 0) {
This looks like a for look to me...
> + u64 val = *buf++;
> +
> + *bitmap++ = (unsigned long)val;
> + if (nbits > 32)
> + *bitmap++ = (unsigned long)(val >> 32);
No need for either cast.
> + nbits -= 64;
> + }
>
> + /* Clear tail bits in last word beyond nbits. */
> + if (nbits % BITS_PER_LONG)
> + bitmap[-1] &= BITMAP_LAST_WORD_MASK(nbits);
> +}
> +EXPORT_SYMBOL(bitmap_from_arr64);
> +
> +/**
> + * bitmap_to_arr64 - copy the contents of bitmap to a u64 array of bits
> + * @buf: array of u64 (in host byte order), the dest bitmap
> + * @bitmap: array of unsigned longs, the source bitmap
> + * @nbits: number of bits in @bitmap
> + */
> +void bitmap_to_arr64(u64 *buf, const unsigned long *bitmap, unsigned int nbits)
> +{
> + unsigned long *end = bitmap + BITS_TO_LONGS(nbits);
> +
> + while (bitmap < end) {
Another for loop...
> + *buf = *bitmap++;
> + if (bitmap < end)
> + *buf |= *bitmap++ << 32;
That is UB.
Did you even compile this??
David
> + buf++;
> + }
> +
> + /* Clear tail bits in last element of array beyond nbits. */
> + if (nbits % 64)
> + buf[-1] &= GENMASK_ULL(nbits, 0);
> +}
> +EXPORT_SYMBOL(bitmap_to_arr64);
> #endif
> --
> 2.32.0
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)