Re: [PATCH 17/21] libmpathutil: use union for bitfield

[Benjamin Marzinski <bmarzins@redhat.com>]

On Wed, Dec 17, 2025 at 10:21:09PM +0100, Martin Wilck wrote:
> The macro BITFIELD used a cast of a pointer to a different anonymous struct
> to (struct bitfield *). This violates strict aliasing rules, because the
> two structs aren't compatible types in the sense of the language
> specification. With -O2, gcc 15.1 generates code that makes th pgcmp()
> function fail, because the compiler doesn't initialize bf->len aka
> __storage_for__bf.len to 64.
> 
> The problem will show through error messages like this:
> 
> multipathd[1974208]: is_bit_set_in_bitfield: bitfield overflow: 1 >= 0
> 
> Fix this by using a union
> 
> Fixes: 9a2f173 ("libmpathutil: change STATIC_BITFIELD to BITFIELD")
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmpathutil/util.c       |  8 ++++----
>  libmpathutil/util.h       | 41 +++++++++++++++++++++++----------------
>  libmultipath/configure.c  |  4 ++--
>  libmultipath/pgpolicies.c |  2 +-
>  tests/util.c              | 10 +++++-----
>  5 files changed, 36 insertions(+), 29 deletions(-)
> 
> diff --git a/libmpathutil/util.c b/libmpathutil/util.c
> index 37412c6..5e45750 100644
> --- a/libmpathutil/util.c
> +++ b/libmpathutil/util.c
> @@ -337,15 +337,15 @@ void cleanup_fclose(void *p)
>  		fclose(p);
>  }
>  
> -void cleanup_bitfield(struct bitfield **p)
> +void cleanup_bitfield(union bitfield **p)
>  {
>  	free(*p);
>  }
>  
> -struct bitfield *alloc_bitfield(unsigned int maxbit)
> +union bitfield *alloc_bitfield(unsigned int maxbit)
>  {
>  	unsigned int n;
> -	struct bitfield *bf;
> +	union bitfield *bf;
>  
>  	if (maxbit == 0) {
>  		errno = EINVAL;
> @@ -353,7 +353,7 @@ struct bitfield *alloc_bitfield(unsigned int maxbit)
>  	}
>  
>  	n = (maxbit - 1) / bits_per_slot + 1;
> -	bf = calloc(1, sizeof(struct bitfield) + n * sizeof(bitfield_t));
> +	bf = calloc(1, sizeof(union bitfield) + (n - 1) * sizeof(bitfield_t));
>  	if (bf)
>  		bf->len = maxbit;
>  	return bf;
> diff --git a/libmpathutil/util.h b/libmpathutil/util.h
> index 3799fd4..1ec6a46 100644
> --- a/libmpathutil/util.h
> +++ b/libmpathutil/util.h
> @@ -86,29 +86,36 @@ typedef unsigned int bitfield_t;
>  #endif
>  #define bits_per_slot (sizeof(bitfield_t) * CHAR_BIT)
>  
> -struct bitfield {
> -	unsigned int len;
> -	bitfield_t bits[];
> +union bitfield {
> +	struct {
> +		unsigned int len;
> +		bitfield_t bits[];
> +	};
> +	/* for initialization in the BITFIELD macro */
> +	struct {
> +		unsigned int __len;
> +		bitfield_t __bits[1];
> +	};
>  };
>  
> -#define BITFIELD(name, length)					\
> -	struct {							\
> -		unsigned int len;					\
> -		bitfield_t bits[((length) - 1) / bits_per_slot + 1];	\
> -	} __storage_for__ ## name = {					\
> -		.len = (length),					\
> -		.bits = { 0, },						\
> +/*
> + * gcc 4.9.2 (Debian Jessie) raises an error if the initializer for
> + * .__len comes first. Thus put .__bits first.
> + */
> +#define BITFIELD(name, length)						\
> +	union bitfield __storage_for__ ## name = {			\
> +		.__bits = { 0 },					\
> +		.__len = (length),					\
>  	}; \

It looks like this only allocates one bitfield_t, regardless of length.
Am I missing something here?

-Ben

> -	struct bitfield *name = (struct bitfield *)& __storage_for__ ## name
> +	union bitfield *name = & __storage_for__ ## name
>  
> -struct bitfield *alloc_bitfield(unsigned int maxbit);
> +union bitfield *alloc_bitfield(unsigned int maxbit);
>  
>  void log_bitfield_overflow__(const char *f, unsigned int bit, unsigned int len);
>  #define log_bitfield_overflow(bit, len) \
>  	log_bitfield_overflow__(__func__, bit, len)
>  
> -static inline bool is_bit_set_in_bitfield(unsigned int bit,
> -				       const struct bitfield *bf)
> +static inline bool is_bit_set_in_bitfield(unsigned int bit, const union bitfield *bf)
>  {
>  	if (bit >= bf->len) {
>  		log_bitfield_overflow(bit, bf->len);
> @@ -118,7 +125,7 @@ static inline bool is_bit_set_in_bitfield(unsigned int bit,
>  		  (1ULL << (bit % bits_per_slot)));
>  }
>  
> -static inline void set_bit_in_bitfield(unsigned int bit, struct bitfield *bf)
> +static inline void set_bit_in_bitfield(unsigned int bit, union bitfield *bf)
>  {
>  	if (bit >= bf->len) {
>  		log_bitfield_overflow(bit, bf->len);
> @@ -127,7 +134,7 @@ static inline void set_bit_in_bitfield(unsigned int bit, struct bitfield *bf)
>  	bf->bits[bit / bits_per_slot] |= (1ULL << (bit % bits_per_slot));
>  }
>  
> -static inline void clear_bit_in_bitfield(unsigned int bit, struct bitfield *bf)
> +static inline void clear_bit_in_bitfield(unsigned int bit, union bitfield *bf)
>  {
>  	if (bit >= bf->len) {
>  		log_bitfield_overflow(bit, bf->len);
> @@ -146,5 +153,5 @@ static inline void clear_bit_in_bitfield(unsigned int bit, struct bitfield *bf)
>  void cleanup_charp(char **p);
>  void cleanup_ucharp(unsigned char **p);
>  void cleanup_udev_device(struct udev_device **udd);
> -void cleanup_bitfield(struct bitfield **p);
> +void cleanup_bitfield(union bitfield **p);
>  #endif /* UTIL_H_INCLUDED */
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index a8e18a2..41b6a9f 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -440,7 +440,7 @@ static int pgcmp(struct multipath *mpp, struct multipath *cmpp)
>  	int i, j;
>  	struct pathgroup *pgp, *cpgp;
>  	BITFIELD(bf, bits_per_slot);
> -	struct bitfield *bf__  __attribute__((cleanup(cleanup_bitfield))) = NULL;
> +	union bitfield *bf__ __attribute__((cleanup(cleanup_bitfield))) = NULL;
>  
>  	if (VECTOR_SIZE(mpp->pg) != VECTOR_SIZE(cmpp->pg))
>  		return 1;
> @@ -1049,7 +1049,7 @@ int coalesce_paths (struct vectors *vecs, vector mpvec, char *refwwid,
>  	vector newmp;
>  	struct config *conf = NULL;
>  	int allow_queueing;
> -	struct bitfield *size_mismatch_seen;
> +	union bitfield *size_mismatch_seen;
>  	struct multipath * cmpp;
>  
>  	/* ignore refwwid if it's empty */
> diff --git a/libmultipath/pgpolicies.c b/libmultipath/pgpolicies.c
> index 2e1a543..57b7485 100644
> --- a/libmultipath/pgpolicies.c
> +++ b/libmultipath/pgpolicies.c
> @@ -201,7 +201,7 @@ int group_by_match(struct multipath * mp, vector paths,
>  		   bool (*path_match_fn)(struct path *, struct path *))
>  {
>  	int i, j;
> -	struct bitfield *bitmap;
> +	union bitfield *bitmap;
>  	struct path * pp;
>  	struct pathgroup * pgp;
>  	struct path * pp2;
> diff --git a/tests/util.c b/tests/util.c
> index da192ba..67d8148 100644
> --- a/tests/util.c
> +++ b/tests/util.c
> @@ -198,7 +198,7 @@ static uint64_t maybe_swap(uint64_t v)
>  
>  static void test_bitmask_1(void **state)
>  {
> -	struct bitfield *bf;
> +	union bitfield *bf;
>  	uint64_t *arr;
>  	int i, j, k, m, b;
>  
> @@ -240,7 +240,7 @@ static void test_bitmask_1(void **state)
>  
>  static void test_bitmask_2(void **state)
>  {
> -	struct bitfield *bf;
> +	union bitfield *bf;
>  	uint64_t *arr;
>  	int i, j, k, m, b;
>  
> @@ -307,7 +307,7 @@ static void test_bitmask_2(void **state)
>   */
>  static void test_bitmask_len_0(void **state)
>  {
> -	struct bitfield *bf;
> +	union bitfield *bf;
>  
>  	bf = alloc_bitfield(0);
>  	assert_null(bf);
> @@ -329,7 +329,7 @@ static unsigned int maybe_swap_idx(unsigned int i)
>  
>  static void _test_bitmask_small(unsigned int n)
>  {
> -	struct bitfield *bf;
> +	union bitfield *bf;
>  	uint32_t *arr;
>  	unsigned int size = maybe_swap_idx((n - 1) / 32) + 1, i;
>  
> @@ -378,7 +378,7 @@ static void _test_bitmask_small(unsigned int n)
>  
>  static void _test_bitmask_small_2(unsigned int n)
>  {
> -	struct bitfield *bf;
> +	union bitfield *bf;
>  	uint32_t *arr;
>  	unsigned int size = maybe_swap_idx((n - 1) / 32) + 1, i;
>  
> -- 
> 2.52.0