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

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

[Andrii Nakryiko]

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>
---
 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;
-- 
2.30.2

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

[Eduard Zingerman]

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;

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Thu, 15 Dec 2022 10:36:05 -0800 you 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.
> 
> [...]

Here is the summary with links:
  - [bpf-next] libbpf: fix btf_dump's packed struct determination
    https://git.kernel.org/bpf/bpf-next/c/4fb877aaa179

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html