[PATCH v2 bpf-next 0/3] libbpf: BTF typed dump cleanups

[PATCH v2 bpf-next 0/3] libbpf: BTF typed dump cleanups

[Alan Maguire]

Fix issues with libbpf BTF typed dump code.  Patch 1 addresses handling
of unaligned data. Patch 2 fixes issues Andrii noticed when compiling
on ppc64le.  Patch 3 simplifies typed dump by getting rid of allocation
of dump data structure which tracks dump state etc.

Changes since v1:

 - Andrii suggested using a function instead of a macro for checking
   alignment of data, and pointed out that we need to consider dump
   ptr size versus native pointer size (patch 1)

Alan Maguire (3):
  libbpf: clarify/fix unaligned data issues for btf typed dump
  libbpf: fix compilation errors on ppc64le for btf dump typed data
  libbpf: btf typed dump does not need to allocate dump data

 tools/lib/bpf/btf_dump.c | 39 ++++++++++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 9 deletions(-)

-- 
1.8.3.1

[PATCH v2 bpf-next 1/3] libbpf: clarify/fix unaligned data issues for btf typed dump

[Alan Maguire]

If data is packed, data structures can store it outside of usual
boundaries.  For example a 4-byte int can be stored on a unaligned
boundary in a case like this:

struct s {
	char f1;
	int f2;
} __attribute((packed));

...the int is stored at an offset of one byte.  Some platforms have
problems dereferencing data that is not aligned with its size, and
code exists to handle most cases of this for BTF typed data display.
However pointer display was missed, and a simple function to test if
"ptr_is_aligned(data, data_sz)" would help clarify this code.

Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 tools/lib/bpf/btf_dump.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
index 929cf93..814a538 100644
--- a/tools/lib/bpf/btf_dump.c
+++ b/tools/lib/bpf/btf_dump.c
@@ -1654,6 +1654,11 @@ static int btf_dump_base_type_check_zero(struct btf_dump *d,
 	return 0;
 }
 
+static bool ptr_is_aligned(const void *data, int data_sz)
+{
+	return ((uintptr_t)data) % data_sz == 0;
+}
+
 static int btf_dump_int_data(struct btf_dump *d,
 			     const struct btf_type *t,
 			     __u32 type_id,
@@ -1672,7 +1677,7 @@ static int btf_dump_int_data(struct btf_dump *d,
 	/* handle packed int data - accesses of integers not aligned on
 	 * int boundaries can cause problems on some platforms.
 	 */
-	if (((uintptr_t)data) % sz)
+	if (!ptr_is_aligned(data, sz))
 		return btf_dump_bitfield_data(d, t, data, 0, 0);
 
 	switch (sz) {
@@ -1739,7 +1744,7 @@ static int btf_dump_float_data(struct btf_dump *d,
 	int sz = t->size;
 
 	/* handle unaligned data; copy to local union */
-	if (((uintptr_t)data) % sz) {
+	if (!ptr_is_aligned(data, sz)) {
 		memcpy(&fl, data, sz);
 		flp = &fl;
 	}
@@ -1892,12 +1897,29 @@ static int btf_dump_struct_data(struct btf_dump *d,
 	return err;
 }
 
+union ptr_data {
+	unsigned int p;
+	unsigned long lp;
+};
+
 static int btf_dump_ptr_data(struct btf_dump *d,
 			      const struct btf_type *t,
 			      __u32 id,
 			      const void *data)
 {
-	btf_dump_type_values(d, "%p", *(void **)data);
+	bool ptr_sz_matches = d->ptr_sz == sizeof(void *);
+
+	if (ptr_sz_matches && ptr_is_aligned(data, d->ptr_sz)) {
+		btf_dump_type_values(d, "%p", *(void **)data);
+	} else {
+		union ptr_data pt;
+
+		memcpy(&pt, data, d->ptr_sz);
+		if (d->ptr_sz == 4)
+			btf_dump_type_values(d, "0x%x", pt.p);
+		else
+			btf_dump_type_values(d, "0x%llx", pt.lp);
+	}
 	return 0;
 }
 
@@ -1910,7 +1932,7 @@ static int btf_dump_get_enum_value(struct btf_dump *d,
 	int sz = t->size;
 
 	/* handle unaligned enum value */
-	if (((uintptr_t)data) % sz) {
+	if (!ptr_is_aligned(data, sz)) {
 		*value = (__s64)btf_dump_bitfield_get_data(d, t, data, 0, 0);
 		return 0;
 	}
-- 
1.8.3.1

[PATCH v2 bpf-next 2/3] libbpf: fix compilation errors on ppc64le for btf dump typed data

[Alan Maguire]

Andrii reports:

"ppc64le arch doesn't like the %lld:

 In file included from btf_dump.c:22:
btf_dump.c: In function 'btf_dump_type_data_check_overflow':
libbpf_internal.h:111:22: error: format '%lld' expects argument of
type 'long long int', but argument 3 has type '__s64' {aka 'long int'}
[-Werror=format=]
  111 |  libbpf_print(level, "libbpf: " fmt, ##__VA_ARGS__); \
      |                      ^~~~~~~~~~
libbpf_internal.h:114:27: note: in expansion of macro '__pr'
  114 | #define pr_warn(fmt, ...) __pr(LIBBPF_WARN, fmt, ##__VA_ARGS__)
      |                           ^~~~
btf_dump.c:1992:3: note: in expansion of macro 'pr_warn'
 1992 |   pr_warn("unexpected size [%lld] for id [%u]\n",
      |   ^~~~~~~
btf_dump.c:1992:32: note: format string is defined here
 1992 |   pr_warn("unexpected size [%lld] for id [%u]\n",
      |                             ~~~^
      |                                |
      |                                long long int
      |                             %ld

Cast to size_t and use %zu."

Reported-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 tools/lib/bpf/btf_dump.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
index 814a538..e5fbfb8 100644
--- a/tools/lib/bpf/btf_dump.c
+++ b/tools/lib/bpf/btf_dump.c
@@ -2011,8 +2011,8 @@ static int btf_dump_type_data_check_overflow(struct btf_dump *d,
 	__s64 size = btf__resolve_size(d->btf, id);
 
 	if (size < 0 || size >= INT_MAX) {
-		pr_warn("unexpected size [%lld] for id [%u]\n",
-			size, id);
+		pr_warn("unexpected size [%zu] for id [%u]\n",
+			(size_t)size, id);
 		return -EINVAL;
 	}
 
-- 
1.8.3.1

[PATCH v2 bpf-next 3/3] libbpf: btf typed dump does not need to allocate dump data

[Alan Maguire]

By using the stack for this small structure, we avoid the need
for freeing memory in error paths.

Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 tools/lib/bpf/btf_dump.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
index e5fbfb8..bd8e005 100644
--- a/tools/lib/bpf/btf_dump.c
+++ b/tools/lib/bpf/btf_dump.c
@@ -2240,6 +2240,7 @@ int btf_dump__dump_type_data(struct btf_dump *d, __u32 id,
 			     const void *data, size_t data_sz,
 			     const struct btf_dump_type_data_opts *opts)
 {
+	struct btf_dump_data typed_dump = {};
 	const struct btf_type *t;
 	int ret;
 
@@ -2250,7 +2251,7 @@ int btf_dump__dump_type_data(struct btf_dump *d, __u32 id,
 	if (!t)
 		return libbpf_err(-ENOENT);
 
-	d->typed_dump = calloc(1, sizeof(struct btf_dump_data));
+	d->typed_dump = &typed_dump;
 	if (!d->typed_dump)
 		return libbpf_err(-ENOMEM);
 
@@ -2269,7 +2270,5 @@ int btf_dump__dump_type_data(struct btf_dump *d, __u32 id,
 
 	ret = btf_dump_dump_type_data(d, NULL, t, id, data, 0, 0);
 
-	free(d->typed_dump);
-
 	return libbpf_err(ret);
 }
-- 
1.8.3.1

Re: [PATCH v2 bpf-next 0/3] libbpf: BTF typed dump cleanups

[Andrii Nakryiko]

On Fri, Jul 16, 2021 at 3:47 PM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> Fix issues with libbpf BTF typed dump code.  Patch 1 addresses handling
> of unaligned data. Patch 2 fixes issues Andrii noticed when compiling
> on ppc64le.  Patch 3 simplifies typed dump by getting rid of allocation
> of dump data structure which tracks dump state etc.
>
> Changes since v1:
>
>  - Andrii suggested using a function instead of a macro for checking
>    alignment of data, and pointed out that we need to consider dump
>    ptr size versus native pointer size (patch 1)
>
> Alan Maguire (3):
>   libbpf: clarify/fix unaligned data issues for btf typed dump
>   libbpf: fix compilation errors on ppc64le for btf dump typed data
>   libbpf: btf typed dump does not need to allocate dump data
>
>  tools/lib/bpf/btf_dump.c | 39 ++++++++++++++++++++++++++++++---------
>  1 file changed, 30 insertions(+), 9 deletions(-)
>
> --
> 1.8.3.1
>

Thank you for the quick follow up. But see all the comments I left and
the fix ups I had to do. Just because the changes are small doesn't
mean you should get sloppy about making them. Please be a bit more
thorough in future patches.

Applied to bpf-next.

Re: [PATCH v2 bpf-next 1/3] libbpf: clarify/fix unaligned data issues for btf typed dump

[Andrii Nakryiko]

On Fri, Jul 16, 2021 at 3:47 PM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> If data is packed, data structures can store it outside of usual
> boundaries.  For example a 4-byte int can be stored on a unaligned
> boundary in a case like this:
>
> struct s {
>         char f1;
>         int f2;
> } __attribute((packed));
>
> ...the int is stored at an offset of one byte.  Some platforms have
> problems dereferencing data that is not aligned with its size, and
> code exists to handle most cases of this for BTF typed data display.
> However pointer display was missed, and a simple function to test if
> "ptr_is_aligned(data, data_sz)" would help clarify this code.
>
> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  tools/lib/bpf/btf_dump.c | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
> index 929cf93..814a538 100644
> --- a/tools/lib/bpf/btf_dump.c
> +++ b/tools/lib/bpf/btf_dump.c
> @@ -1654,6 +1654,11 @@ static int btf_dump_base_type_check_zero(struct btf_dump *d,
>         return 0;
>  }
>
> +static bool ptr_is_aligned(const void *data, int data_sz)
> +{
> +       return ((uintptr_t)data) % data_sz == 0;
> +}
> +
>  static int btf_dump_int_data(struct btf_dump *d,
>                              const struct btf_type *t,
>                              __u32 type_id,
> @@ -1672,7 +1677,7 @@ static int btf_dump_int_data(struct btf_dump *d,
>         /* handle packed int data - accesses of integers not aligned on
>          * int boundaries can cause problems on some platforms.
>          */
> -       if (((uintptr_t)data) % sz)
> +       if (!ptr_is_aligned(data, sz))
>                 return btf_dump_bitfield_data(d, t, data, 0, 0);
>
>         switch (sz) {
> @@ -1739,7 +1744,7 @@ static int btf_dump_float_data(struct btf_dump *d,
>         int sz = t->size;
>
>         /* handle unaligned data; copy to local union */
> -       if (((uintptr_t)data) % sz) {
> +       if (!ptr_is_aligned(data, sz)) {
>                 memcpy(&fl, data, sz);
>                 flp = &fl;
>         }
> @@ -1892,12 +1897,29 @@ static int btf_dump_struct_data(struct btf_dump *d,
>         return err;
>  }
>
> +union ptr_data {
> +       unsigned int p;
> +       unsigned long lp;

long can be 32-bit on 4-byte architectures, plus %llx implies long
long (or we'll get another annoying warning from the compiler)

> +};
> +
>  static int btf_dump_ptr_data(struct btf_dump *d,
>                               const struct btf_type *t,
>                               __u32 id,
>                               const void *data)
>  {
> -       btf_dump_type_values(d, "%p", *(void **)data);
> +       bool ptr_sz_matches = d->ptr_sz == sizeof(void *);

used just once and clear what it does, I inlined this, no point in
separate variable


> +
> +       if (ptr_sz_matches && ptr_is_aligned(data, d->ptr_sz)) {
> +               btf_dump_type_values(d, "%p", *(void **)data);
> +       } else {
> +               union ptr_data pt;
> +
> +               memcpy(&pt, data, d->ptr_sz);
> +               if (d->ptr_sz == 4)
> +                       btf_dump_type_values(d, "0x%x", pt.p);
> +               else
> +                       btf_dump_type_values(d, "0x%llx", pt.lp);
> +       }
>         return 0;
>  }
>
> @@ -1910,7 +1932,7 @@ static int btf_dump_get_enum_value(struct btf_dump *d,
>         int sz = t->size;
>
>         /* handle unaligned enum value */
> -       if (((uintptr_t)data) % sz) {
> +       if (!ptr_is_aligned(data, sz)) {
>                 *value = (__s64)btf_dump_bitfield_get_data(d, t, data, 0, 0);
>                 return 0;
>         }
> --
> 1.8.3.1
>

Re: [PATCH v2 bpf-next 2/3] libbpf: fix compilation errors on ppc64le for btf dump typed data

[Andrii Nakryiko]

On Fri, Jul 16, 2021 at 3:47 PM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> Andrii reports:
>
> "ppc64le arch doesn't like the %lld:
>
>  In file included from btf_dump.c:22:
> btf_dump.c: In function 'btf_dump_type_data_check_overflow':
> libbpf_internal.h:111:22: error: format '%lld' expects argument of
> type 'long long int', but argument 3 has type '__s64' {aka 'long int'}
> [-Werror=format=]
>   111 |  libbpf_print(level, "libbpf: " fmt, ##__VA_ARGS__); \
>       |                      ^~~~~~~~~~
> libbpf_internal.h:114:27: note: in expansion of macro '__pr'
>   114 | #define pr_warn(fmt, ...) __pr(LIBBPF_WARN, fmt, ##__VA_ARGS__)
>       |                           ^~~~
> btf_dump.c:1992:3: note: in expansion of macro 'pr_warn'
>  1992 |   pr_warn("unexpected size [%lld] for id [%u]\n",
>       |   ^~~~~~~
> btf_dump.c:1992:32: note: format string is defined here
>  1992 |   pr_warn("unexpected size [%lld] for id [%u]\n",
>       |                             ~~~^
>       |                                |
>       |                                long long int
>       |                             %ld
>
> Cast to size_t and use %zu."
>

Quoting me isn't a great commit message by itself, tbh. Reworded.



> Reported-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  tools/lib/bpf/btf_dump.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
> index 814a538..e5fbfb8 100644
> --- a/tools/lib/bpf/btf_dump.c
> +++ b/tools/lib/bpf/btf_dump.c
> @@ -2011,8 +2011,8 @@ static int btf_dump_type_data_check_overflow(struct btf_dump *d,
>         __s64 size = btf__resolve_size(d->btf, id);
>
>         if (size < 0 || size >= INT_MAX) {
> -               pr_warn("unexpected size [%lld] for id [%u]\n",
> -                       size, id);
> +               pr_warn("unexpected size [%zu] for id [%u]\n",
> +                       (size_t)size, id);
>                 return -EINVAL;
>         }
>
> --
> 1.8.3.1
>

Re: [PATCH v2 bpf-next 3/3] libbpf: btf typed dump does not need to allocate dump data

[Andrii Nakryiko]

On Fri, Jul 16, 2021 at 3:47 PM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> By using the stack for this small structure, we avoid the need
> for freeing memory in error paths.
>
> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  tools/lib/bpf/btf_dump.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
> index e5fbfb8..bd8e005 100644
> --- a/tools/lib/bpf/btf_dump.c
> +++ b/tools/lib/bpf/btf_dump.c
> @@ -2240,6 +2240,7 @@ int btf_dump__dump_type_data(struct btf_dump *d, __u32 id,
>                              const void *data, size_t data_sz,
>                              const struct btf_dump_type_data_opts *opts)
>  {
> +       struct btf_dump_data typed_dump = {};
>         const struct btf_type *t;
>         int ret;
>
> @@ -2250,7 +2251,7 @@ int btf_dump__dump_type_data(struct btf_dump *d, __u32 id,
>         if (!t)
>                 return libbpf_err(-ENOENT);
>
> -       d->typed_dump = calloc(1, sizeof(struct btf_dump_data));
> +       d->typed_dump = &typed_dump;
>         if (!d->typed_dump)
>                 return libbpf_err(-ENOMEM);

can't happen, removed, please pay attention to the surrounding code

>
> @@ -2269,7 +2270,5 @@ int btf_dump__dump_type_data(struct btf_dump *d, __u32 id,
>
>         ret = btf_dump_dump_type_data(d, NULL, t, id, data, 0, 0);
>
> -       free(d->typed_dump);
> -

added resetting to NULL here, so that we don't have an accidental
dangling pointers

>         return libbpf_err(ret);
>  }

> --
> 1.8.3.1
>

Re: [PATCH v2 bpf-next 0/3] libbpf: BTF typed dump cleanups

[Andrii Nakryiko]

On Fri, Jul 16, 2021 at 5:32 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Jul 16, 2021 at 3:47 PM Alan Maguire <alan.maguire@oracle.com> wrote:
> >
> > Fix issues with libbpf BTF typed dump code.  Patch 1 addresses handling
> > of unaligned data. Patch 2 fixes issues Andrii noticed when compiling
> > on ppc64le.  Patch 3 simplifies typed dump by getting rid of allocation
> > of dump data structure which tracks dump state etc.
> >
> > Changes since v1:
> >
> >  - Andrii suggested using a function instead of a macro for checking
> >    alignment of data, and pointed out that we need to consider dump
> >    ptr size versus native pointer size (patch 1)
> >
> > Alan Maguire (3):
> >   libbpf: clarify/fix unaligned data issues for btf typed dump
> >   libbpf: fix compilation errors on ppc64le for btf dump typed data
> >   libbpf: btf typed dump does not need to allocate dump data
> >
> >  tools/lib/bpf/btf_dump.c | 39 ++++++++++++++++++++++++++++++---------
> >  1 file changed, 30 insertions(+), 9 deletions(-)
> >
> > --
> > 1.8.3.1
> >
>
> Thank you for the quick follow up. But see all the comments I left and

One more thing. Please do reply to the rest of my questions and
concerns on the original patch set. You tend to just address most of
the feedback in the next revision, but I'd appreciate it if you could
reply at least with a simple "ok" or more elaborate answer where
warranted. It makes the code reviewing process a bit easier.

There are still big-endian concerns and an error propagation question
at least that you haven't addressed neither in the follow up patches
nor in an email reply. Please do so, preferably both.

> the fix ups I had to do. Just because the changes are small doesn't
> mean you should get sloppy about making them. Please be a bit more
> thorough in future patches.
>
> Applied to bpf-next.