[PATCH 0/2] pahole: BTF alignment inference improvements

[PATCH 0/2] pahole: BTF alignment inference improvements

[Antoine Tenart]

Hello,

While generating C code using pahole we noticed alignment issues in rare
cases: when having variable size and 0-length arrays, and when a hole
follows a bitfield. It's worth noticing alignment is inferred when using
BTF as a source as the format does not contain such information. As such
the logic is tricky.

The first patch fixes alignment inference for zero-length and variable
length arrays. The second patch fixes alignment when a hole follows a
bitfield.

I generated a full vmlinux.h using `pahole --compile` before and after
the changes, the differences looked reasonable to me. I've also ran the
project's tests using GH actions.

Thanks,
Antoine

Antoine Tenart (2):
  btf_loader: infer alignment for zero-length arrays
  btf_loader: fix smallest offset in case of bitfields

 btf_loader.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

-- 
2.49.0

[PATCH 1/2] btf_loader: infer alignment for zero-length arrays

[Antoine Tenart]

Zero sized arrays do not go through the bitfield fixup logic, which in
turn makes them to skip the alignment infer logic. The result is
zero-length arrays being incorrectly aligned when an explicit statement
is needed. E.g. this happens for variable length arrays at end of
structures, or for 0-length ones.

Before this patch we can see:

  struct skb_ext {
	refcount_t                 refcnt;               /*     0     4 */
	u8                         offset[4];            /*     4     4 */
	u8                         chunks;               /*     8     1 */

	/* XXX 7 bytes hole, try to pack */

	char                       data[];               /*    16     0 */

	/* size: 16, cachelines: 1, members: 4 */
	/* sum members: 9, holes: 1, sum holes: 7 */
	/* last cacheline: 16 bytes */
  } __attribute__((__aligned__(8)));

  offsetof(struct skb_ext, data) returns 9.

After this patch we get:

  struct skb_ext {
	refcount_t                 refcnt;               /*     0     4 */
	u8                         offset[4];            /*     4     4 */
	u8                         chunks;               /*     8     1 */

	/* XXX 7 bytes hole, try to pack */

	char                       data[] __attribute__((__aligned__(8))); /*    16     0 */

	/* size: 16, cachelines: 1, members: 4 */
	/* sum members: 9, holes: 1, sum holes: 7 */
	/* forced alignments: 1, forced holes: 1, sum forced holes: 7 */
	/* last cacheline: 16 bytes */
  } __attribute__((__aligned__(8)));

  offsetof(struct skb_ext, data) returns 16.

Signed-off-by: Antoine Tenart <atenart@kernel.org>
---
 btf_loader.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/btf_loader.c b/btf_loader.c
index f4f9f65289b5..76771afedd95 100644
--- a/btf_loader.c
+++ b/btf_loader.c
@@ -648,6 +648,10 @@ static int class__fixup_btf_bitfields(const struct conf_load *conf, struct tag *
 		/* if BTF data is incorrect and has size == 0, skip field,
 		 * instead of crashing */
 		if (pos->byte_size == 0) {
+			pos->alignment = class__infer_alignment(conf,
+								pos->byte_offset,
+								tag__natural_alignment(type, cu),
+								smallest_offset);
 			continue;
 		}
 
-- 
2.49.0

[PATCH 2/2] btf_loader: fix smallest offset in case of bitfields

[Antoine Tenart]

In case the member is a bitfield, using pos->byte_size can be misleading
as it can be larger than the actual size. In such case we could miss an
actual hole in the struct and not infer the alignment as expected.

Before this patch we can see:

  $ pahole nf_tables.btf -C nft_rule_dp
  struct nft_rule_dp {
  	u64                        is_last:1;            /*     0: 0  8 */
  	u64                        dlen:12;              /*     0: 1  8 */
  	u64                        handle:42;            /*     0:13  8 */

  	/* XXX 9 bits hole, try to pack */

  	unsigned char              data[];               /*     8     0 */

  	/* size: 8, cachelines: 1, members: 4 */
  	/* sum bitfield members: 55 bits, bit holes: 1, sum bit holes: 9 bits */
  	/* last cacheline: 8 bytes */
  };

  offsetof(struct nft_rule_dp, data) returns 7.

Where `data` won't be aligned as expected, as the first member fits on
the byte left after the bitfield.

After this patch we get:

  $ pahole nf_tables.btf -C nft_rule_dp
  struct nft_rule_dp {
  	u64                        is_last:1;            /*     0: 0  8 */
  	u64                        dlen:12;              /*     0: 1  8 */
  	u64                        handle:42;            /*     0:13  8 */

  	/* XXX 9 bits hole, try to pack */

  	unsigned char              data[] __attribute__((__aligned__(2))); /*     8     0 */

  	/* size: 8, cachelines: 1, members: 4 */
  	/* sum bitfield members: 55 bits, bit holes: 1, sum bit holes: 9 bits */
  	/* forced alignments: 1 */
  	/* last cacheline: 8 bytes */
  };

  offsetof(struct nft_rule_dp, data) returns 8.

Thanks to Paolo Valerio for helping, including narrowing down where the
issue was (BTF decoder).

Signed-off-by: Antoine Tenart <atenart@kernel.org>
---
 btf_loader.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/btf_loader.c b/btf_loader.c
index 76771afedd95..3beb5bba5f94 100644
--- a/btf_loader.c
+++ b/btf_loader.c
@@ -676,7 +676,10 @@ static int class__fixup_btf_bitfields(const struct conf_load *conf, struct tag *
 							pos->byte_offset,
 							tag__natural_alignment(type, cu),
 							smallest_offset);
-		smallest_offset = pos->byte_offset + pos->byte_size;
+		smallest_offset = pos->byte_offset;
+		smallest_offset += pos->bitfield_size ?
+			(pos->bitfield_offset + pos->bitfield_size + 7) / 8 :
+			pos->byte_size;
 	}
 
 	tag_type->alignment = class__infer_alignment(conf,
-- 
2.49.0

Re: [PATCH 1/2] btf_loader: infer alignment for zero-length arrays

[Alan Maguire]

On 25/06/2025 16:23, Antoine Tenart wrote:
> Zero sized arrays do not go through the bitfield fixup logic, which in
> turn makes them to skip the alignment infer logic. The result is
> zero-length arrays being incorrectly aligned when an explicit statement
> is needed. E.g. this happens for variable length arrays at end of
> structures, or for 0-length ones.
> 
> Before this patch we can see:
> 
>   struct skb_ext {
> 	refcount_t                 refcnt;               /*     0     4 */
> 	u8                         offset[4];            /*     4     4 */
> 	u8                         chunks;               /*     8     1 */
> 
> 	/* XXX 7 bytes hole, try to pack */
> 
> 	char                       data[];               /*    16     0 */
> 
> 	/* size: 16, cachelines: 1, members: 4 */
> 	/* sum members: 9, holes: 1, sum holes: 7 */
> 	/* last cacheline: 16 bytes */
>   } __attribute__((__aligned__(8)));
> 
>   offsetof(struct skb_ext, data) returns 9.
> 
> After this patch we get:
> 
>   struct skb_ext {
> 	refcount_t                 refcnt;               /*     0     4 */
> 	u8                         offset[4];            /*     4     4 */
> 	u8                         chunks;               /*     8     1 */
> 
> 	/* XXX 7 bytes hole, try to pack */
> 
> 	char                       data[] __attribute__((__aligned__(8))); /*    16     0 */
> 
> 	/* size: 16, cachelines: 1, members: 4 */
> 	/* sum members: 9, holes: 1, sum holes: 7 */
> 	/* forced alignments: 1, forced holes: 1, sum forced holes: 7 */
> 	/* last cacheline: 16 bytes */
>   } __attribute__((__aligned__(8)));
> 
>   offsetof(struct skb_ext, data) returns 16.
> 
> Signed-off-by: Antoine Tenart <atenart@kernel.org>

Reviewed-by: Alan Maguire <alan.maguire@oracle.com>

with a small suggestion below..

> ---
>  btf_loader.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/btf_loader.c b/btf_loader.c
> index f4f9f65289b5..76771afedd95 100644
> --- a/btf_loader.c
> +++ b/btf_loader.c
> @@ -648,6 +648,10 @@ static int class__fixup_btf_bitfields(const struct conf_load *conf, struct tag *
>  		/* if BTF data is incorrect and has size == 0, skip field,
>  		 * instead of crashing */

nit: could we update the comment here to reflect the change, adding
something like "however we still need to infer alignment for cases like
zero/variable-length arays".


>  		if (pos->byte_size == 0) {
> +			pos->alignment = class__infer_alignment(conf,
> +								pos->byte_offset,
> +								tag__natural_alignment(type, cu),
> +								smallest_offset);
>  			continue;
>  		}
>  

Re: [PATCH 2/2] btf_loader: fix smallest offset in case of bitfields

[Alan Maguire]

On 25/06/2025 16:23, Antoine Tenart wrote:
> In case the member is a bitfield, using pos->byte_size can be misleading
> as it can be larger than the actual size. In such case we could miss an
> actual hole in the struct and not infer the alignment as expected.
> 
> Before this patch we can see:
> 
>   $ pahole nf_tables.btf -C nft_rule_dp
>   struct nft_rule_dp {
>   	u64                        is_last:1;            /*     0: 0  8 */
>   	u64                        dlen:12;              /*     0: 1  8 */
>   	u64                        handle:42;            /*     0:13  8 */
> 
>   	/* XXX 9 bits hole, try to pack */
> 
>   	unsigned char              data[];               /*     8     0 */
> 
>   	/* size: 8, cachelines: 1, members: 4 */
>   	/* sum bitfield members: 55 bits, bit holes: 1, sum bit holes: 9 bits */
>   	/* last cacheline: 8 bytes */
>   };
> 
>   offsetof(struct nft_rule_dp, data) returns 7.
> 
> Where `data` won't be aligned as expected, as the first member fits on
> the byte left after the bitfield.
> 
> After this patch we get:
> 
>   $ pahole nf_tables.btf -C nft_rule_dp
>   struct nft_rule_dp {
>   	u64                        is_last:1;            /*     0: 0  8 */
>   	u64                        dlen:12;              /*     0: 1  8 */
>   	u64                        handle:42;            /*     0:13  8 */
> 
>   	/* XXX 9 bits hole, try to pack */
> 
>   	unsigned char              data[] __attribute__((__aligned__(2))); /*     8     0 */
> 
>   	/* size: 8, cachelines: 1, members: 4 */
>   	/* sum bitfield members: 55 bits, bit holes: 1, sum bit holes: 9 bits */
>   	/* forced alignments: 1 */
>   	/* last cacheline: 8 bytes */
>   };
> 
>   offsetof(struct nft_rule_dp, data) returns 8.
> 
> Thanks to Paolo Valerio for helping, including narrowing down where the
> issue was (BTF decoder).
> 
> Signed-off-by: Antoine Tenart <atenart@kernel.org>

Reviewed-by: Alan Maguire <alan.maguire@oracle.com>

> ---
>  btf_loader.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/btf_loader.c b/btf_loader.c
> index 76771afedd95..3beb5bba5f94 100644
> --- a/btf_loader.c
> +++ b/btf_loader.c
> @@ -676,7 +676,10 @@ static int class__fixup_btf_bitfields(const struct conf_load *conf, struct tag *
>  							pos->byte_offset,
>  							tag__natural_alignment(type, cu),
>  							smallest_offset);
> -		smallest_offset = pos->byte_offset + pos->byte_size;
> +		smallest_offset = pos->byte_offset;
> +		smallest_offset += pos->bitfield_size ?
> +			(pos->bitfield_offset + pos->bitfield_size + 7) / 8 :
> +			pos->byte_size;


would be good to have a comment here explaining the situation too; it's
quite a subtle problem so anything that helps explain things would be
great. Thanks for tracking this down!

>  	}
>  
>  	tag_type->alignment = class__infer_alignment(conf,

Re: [PATCH 1/2] btf_loader: infer alignment for zero-length arrays

[Antoine Tenart]

On Fri, Jul 04, 2025 at 05:34:47PM +0100, Alan Maguire wrote:
> On 25/06/2025 16:23, Antoine Tenart wrote:
> > Zero sized arrays do not go through the bitfield fixup logic, which in
> > turn makes them to skip the alignment infer logic. The result is
> > zero-length arrays being incorrectly aligned when an explicit statement
> > is needed. E.g. this happens for variable length arrays at end of
> > structures, or for 0-length ones.
> > 
> > Before this patch we can see:
> > 
> >   struct skb_ext {
> > 	refcount_t                 refcnt;               /*     0     4 */
> > 	u8                         offset[4];            /*     4     4 */
> > 	u8                         chunks;               /*     8     1 */
> > 
> > 	/* XXX 7 bytes hole, try to pack */
> > 
> > 	char                       data[];               /*    16     0 */
> > 
> > 	/* size: 16, cachelines: 1, members: 4 */
> > 	/* sum members: 9, holes: 1, sum holes: 7 */
> > 	/* last cacheline: 16 bytes */
> >   } __attribute__((__aligned__(8)));
> > 
> >   offsetof(struct skb_ext, data) returns 9.
> > 
> > After this patch we get:
> > 
> >   struct skb_ext {
> > 	refcount_t                 refcnt;               /*     0     4 */
> > 	u8                         offset[4];            /*     4     4 */
> > 	u8                         chunks;               /*     8     1 */
> > 
> > 	/* XXX 7 bytes hole, try to pack */
> > 
> > 	char                       data[] __attribute__((__aligned__(8))); /*    16     0 */
> > 
> > 	/* size: 16, cachelines: 1, members: 4 */
> > 	/* sum members: 9, holes: 1, sum holes: 7 */
> > 	/* forced alignments: 1, forced holes: 1, sum forced holes: 7 */
> > 	/* last cacheline: 16 bytes */
> >   } __attribute__((__aligned__(8)));
> > 
> >   offsetof(struct skb_ext, data) returns 16.
> > 
> > Signed-off-by: Antoine Tenart <atenart@kernel.org>
> 
> Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
> 
> with a small suggestion below..
> 
> > ---
> >  btf_loader.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/btf_loader.c b/btf_loader.c
> > index f4f9f65289b5..76771afedd95 100644
> > --- a/btf_loader.c
> > +++ b/btf_loader.c
> > @@ -648,6 +648,10 @@ static int class__fixup_btf_bitfields(const struct conf_load *conf, struct tag *
> >  		/* if BTF data is incorrect and has size == 0, skip field,
> >  		 * instead of crashing */
> 
> nit: could we update the comment here to reflect the change, adding
> something like "however we still need to infer alignment for cases like
> zero/variable-length arays".

Makes sense, I'll improve the comment in v2.

Thanks!

> 
> 
> >  		if (pos->byte_size == 0) {
> > +			pos->alignment = class__infer_alignment(conf,
> > +								pos->byte_offset,
> > +								tag__natural_alignment(type, cu),
> > +								smallest_offset);
> >  			continue;
> >  		}
> >  
> 

Re: [PATCH 2/2] btf_loader: fix smallest offset in case of bitfields

[Antoine Tenart]

On Fri, Jul 04, 2025 at 05:40:36PM +0100, Alan Maguire wrote:
> On 25/06/2025 16:23, Antoine Tenart wrote:
> > In case the member is a bitfield, using pos->byte_size can be misleading
> > as it can be larger than the actual size. In such case we could miss an
> > actual hole in the struct and not infer the alignment as expected.
> > 
> > Before this patch we can see:
> > 
> >   $ pahole nf_tables.btf -C nft_rule_dp
> >   struct nft_rule_dp {
> >   	u64                        is_last:1;            /*     0: 0  8 */
> >   	u64                        dlen:12;              /*     0: 1  8 */
> >   	u64                        handle:42;            /*     0:13  8 */
> > 
> >   	/* XXX 9 bits hole, try to pack */
> > 
> >   	unsigned char              data[];               /*     8     0 */
> > 
> >   	/* size: 8, cachelines: 1, members: 4 */
> >   	/* sum bitfield members: 55 bits, bit holes: 1, sum bit holes: 9 bits */
> >   	/* last cacheline: 8 bytes */
> >   };
> > 
> >   offsetof(struct nft_rule_dp, data) returns 7.
> > 
> > Where `data` won't be aligned as expected, as the first member fits on
> > the byte left after the bitfield.
> > 
> > After this patch we get:
> > 
> >   $ pahole nf_tables.btf -C nft_rule_dp
> >   struct nft_rule_dp {
> >   	u64                        is_last:1;            /*     0: 0  8 */
> >   	u64                        dlen:12;              /*     0: 1  8 */
> >   	u64                        handle:42;            /*     0:13  8 */
> > 
> >   	/* XXX 9 bits hole, try to pack */
> > 
> >   	unsigned char              data[] __attribute__((__aligned__(2))); /*     8     0 */
> > 
> >   	/* size: 8, cachelines: 1, members: 4 */
> >   	/* sum bitfield members: 55 bits, bit holes: 1, sum bit holes: 9 bits */
> >   	/* forced alignments: 1 */
> >   	/* last cacheline: 8 bytes */
> >   };
> > 
> >   offsetof(struct nft_rule_dp, data) returns 8.
> > 
> > Thanks to Paolo Valerio for helping, including narrowing down where the
> > issue was (BTF decoder).
> > 
> > Signed-off-by: Antoine Tenart <atenart@kernel.org>
> 
> Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
> 
> > ---
> >  btf_loader.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/btf_loader.c b/btf_loader.c
> > index 76771afedd95..3beb5bba5f94 100644
> > --- a/btf_loader.c
> > +++ b/btf_loader.c
> > @@ -676,7 +676,10 @@ static int class__fixup_btf_bitfields(const struct conf_load *conf, struct tag *
> >  							pos->byte_offset,
> >  							tag__natural_alignment(type, cu),
> >  							smallest_offset);
> > -		smallest_offset = pos->byte_offset + pos->byte_size;
> > +		smallest_offset = pos->byte_offset;
> > +		smallest_offset += pos->bitfield_size ?
> > +			(pos->bitfield_offset + pos->bitfield_size + 7) / 8 :
> > +			pos->byte_size;
> 
> 
> would be good to have a comment here explaining the situation too; it's
> quite a subtle problem so anything that helps explain things would be
> great. Thanks for tracking this down!

Will do.

Thanks!