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@xxxxxxxxx>
> Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
Looks good!
Acked-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
> ---
> 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;
