Re: [PATCH bpf-next] libbpf: fix btf_dump's packed struct determination

[Eduard Zingerman <eddyz87@gmail.com>]

On Thu, 2022-12-15 at 10:36 -0800, Andrii Nakryiko wrote:
> Fix bug in btf_dump's logic of determining if a given struct type is
> packed or not. The notion of "natural alignment" is not needed and is
> even harmful in this case, so drop it altogether. The biggest difference
> in btf_is_struct_packed() compared to its original implementation is
> that we don't really use btf__align_of() to determine overall alignment
> of a struct type (because it could be 1 for both packed and non-packed
> struct, depending on specifci field definitions), and just use field's
> actual alignment to calculate whether any field is requiring packing or
> struct's size overall necessitates packing.
> 
> Add two simple test cases that demonstrate the difference this change
> would make.
> 
> Fixes: ea2ce1ba99aa ("libbpf: Fix BTF-to-C converter's padding logic")
> Reported-by: Eduard Zingerman <eddyz87@gmail.com>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>

Looks good!

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

> ---
>  tools/lib/bpf/btf_dump.c                      | 33 ++++---------------
>  .../bpf/progs/btf_dump_test_case_packing.c    | 19 +++++++++++
>  2 files changed, 25 insertions(+), 27 deletions(-)
> 
> diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
> index d6fd93a57f11..580985ee5545 100644
> --- a/tools/lib/bpf/btf_dump.c
> +++ b/tools/lib/bpf/btf_dump.c
> @@ -830,47 +830,26 @@ static void btf_dump_emit_type(struct btf_dump *d, __u32 id, __u32 cont_id)
>  	}
>  }
>  
> -static int btf_natural_align_of(const struct btf *btf, __u32 id)
> -{
> -	const struct btf_type *t = btf__type_by_id(btf, id);
> -	int i, align, vlen;
> -	const struct btf_member *m;
> -
> -	if (!btf_is_composite(t))
> -		return btf__align_of(btf, id);
> -
> -	align = 1;
> -	m = btf_members(t);
> -	vlen = btf_vlen(t);
> -	for (i = 0; i < vlen; i++, m++) {
> -		align = max(align, btf__align_of(btf, m->type));
> -	}
> -
> -	return align;
> -}
> -
>  static bool btf_is_struct_packed(const struct btf *btf, __u32 id,
>  				 const struct btf_type *t)
>  {
>  	const struct btf_member *m;
> -	int align, i, bit_sz;
> +	int max_align = 1, align, i, bit_sz;
>  	__u16 vlen;
>  
> -	align = btf_natural_align_of(btf, id);
> -	/* size of a non-packed struct has to be a multiple of its alignment */
> -	if (align && (t->size % align) != 0)
> -		return true;
> -
>  	m = btf_members(t);
>  	vlen = btf_vlen(t);
>  	/* all non-bitfield fields have to be naturally aligned */
>  	for (i = 0; i < vlen; i++, m++) {
> -		align = btf_natural_align_of(btf, m->type);
> +		align = btf__align_of(btf, m->type);
>  		bit_sz = btf_member_bitfield_size(t, i);
>  		if (align && bit_sz == 0 && m->offset % (8 * align) != 0)
>  			return true;
> +		max_align = max(align, max_align);
>  	}
> -
> +	/* size of a non-packed struct has to be a multiple of its alignment */
> +	if (t->size % max_align != 0)
> +		return true;
>  	/*
>  	 * if original struct was marked as packed, but its layout is
>  	 * naturally aligned, we'll detect that it's not packed
> diff --git a/tools/testing/selftests/bpf/progs/btf_dump_test_case_packing.c b/tools/testing/selftests/bpf/progs/btf_dump_test_case_packing.c
> index 5c6c62f7ed32..7998f27df7dd 100644
> --- a/tools/testing/selftests/bpf/progs/btf_dump_test_case_packing.c
> +++ b/tools/testing/selftests/bpf/progs/btf_dump_test_case_packing.c
> @@ -116,6 +116,23 @@ struct usb_host_endpoint {
>  	long: 0;
>  };
>  
> +/* ----- START-EXPECTED-OUTPUT ----- */
> +struct nested_packed_struct {
> +	int a;
> +	char b;
> +} __attribute__((packed));
> +
> +struct outer_nonpacked_struct {
> +	short a;
> +	struct nested_packed_struct b;
> +};
> +
> +struct outer_packed_struct {
> +	short a;
> +	struct nested_packed_struct b;
> +} __attribute__((packed));
> +
> +/* ------ END-EXPECTED-OUTPUT ------ */
>  
>  int f(struct {
>  	struct packed_trailing_space _1;
> @@ -128,6 +145,8 @@ int f(struct {
>  	union jump_code_union _8;
>  	struct outer_implicitly_packed_struct _9;
>  	struct usb_host_endpoint _10;
> +	struct outer_nonpacked_struct _11;
> +	struct outer_packed_struct _12;
>  } *_)
>  {
>  	return 0;