[PATCH bpf-next v2 0/4] btf_dump fixes for s390

[PATCH bpf-next v2 0/4] btf_dump fixes for s390

[Ilya Leoshkevich]

v1: https://lore.kernel.org/bpf/20211012023218.399568-1-iii@linux.ibm.com/
v1 -> v2:
- Remove redundant local variables, use t->size directly instead.
- Add btf__align_of() patch.
Pending questions:
- Can things like defined(__i386__) break cross-compilation?
- Why exactly do we need both cpu_number and cpu_profile_flip? If we do,
  is there a suitable replacement for cpu_number in common code?

---

Hi,

This series along with [1] and [2] fixes all the failures in the
btf_dump testsuite currently present on s390, in particular:

* [1] fixes intermittent build bug causing "failed to encode tag ..."
  * error messages.
* [2] fixes missing VAR entries on s390.
* Patch 1 disables Intel-specific code in a testcase.
* Patch 2 fixes an endianness-related bug.
* Patch 3 fixes an alignment-related bug.
* Patch 4 improves overly pessimistic alignment handling.

[1] https://lore.kernel.org/bpf/20211012022521.399302-1-iii@linux.ibm.com/
[2] https://lore.kernel.org/bpf/20211012022637.399365-1-iii@linux.ibm.com/

Best regards,
Ilya

Ilya Leoshkevich (4):
  selftests/bpf: Use cpu_number only on arches that have it
  libbpf: Fix dumping big-endian bitfields
  libbpf: Fix dumping non-aligned __int128
  libbpf: Fix ptr_is_aligned() usages

 tools/lib/bpf/btf_dump.c                      | 34 +++++++++----------
 .../selftests/bpf/prog_tests/btf_dump.c       |  2 ++
 2 files changed, 19 insertions(+), 17 deletions(-)

-- 
2.31.1

[PATCH bpf-next v2 1/4] selftests/bpf: Use cpu_number only on arches that have it

[Ilya Leoshkevich]

cpu_number exists only on Intel and aarch64, so skip the test involing
it on other arches. An alternative would be to replace it with an
exported non-ifdefed primitive-typed percpu variable from the common
code, but there appears to be none.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/bpf/prog_tests/btf_dump.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/btf_dump.c b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
index 87f9df653e4e..12f457b6786d 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf_dump.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
@@ -778,8 +778,10 @@ static void test_btf_dump_struct_data(struct btf *btf, struct btf_dump *d,
 static void test_btf_dump_var_data(struct btf *btf, struct btf_dump *d,
 				   char *str)
 {
+#if defined(__i386__) || defined(__x86_64__) || defined(__aarch64__)
 	TEST_BTF_DUMP_VAR(btf, d, NULL, str, "cpu_number", int, BTF_F_COMPACT,
 			  "int cpu_number = (int)100", 100);
+#endif
 	TEST_BTF_DUMP_VAR(btf, d, NULL, str, "cpu_profile_flip", int, BTF_F_COMPACT,
 			  "static int cpu_profile_flip = (int)2", 2);
 }
-- 
2.31.1

[PATCH bpf-next v2 2/4] libbpf: Fix dumping big-endian bitfields

[Ilya Leoshkevich]

On big-endian arches not only bytes, but also bits are numbered in
reverse order (see e.g. S/390 ELF ABI Supplement, but this is also true
for other big-endian arches as well).

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/lib/bpf/btf_dump.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
index ad6df97295ae..614719c73bd7 100644
--- a/tools/lib/bpf/btf_dump.c
+++ b/tools/lib/bpf/btf_dump.c
@@ -1562,29 +1562,28 @@ static int btf_dump_get_bitfield_value(struct btf_dump *d,
 				       __u64 *value)
 {
 	__u16 left_shift_bits, right_shift_bits;
-	__u8 nr_copy_bits, nr_copy_bytes;
 	const __u8 *bytes = data;
-	int sz = t->size;
+	__u8 nr_copy_bits;
 	__u64 num = 0;
 	int i;
 
 	/* Maximum supported bitfield size is 64 bits */
-	if (sz > 8) {
-		pr_warn("unexpected bitfield size %d\n", sz);
+	if (t->size > 8) {
+		pr_warn("unexpected bitfield size %d\n", t->size);
 		return -EINVAL;
 	}
 
 	/* Bitfield value retrieval is done in two steps; first relevant bytes are
 	 * stored in num, then we left/right shift num to eliminate irrelevant bits.
 	 */
-	nr_copy_bits = bit_sz + bits_offset;
-	nr_copy_bytes = t->size;
 #if __BYTE_ORDER == __LITTLE_ENDIAN
-	for (i = nr_copy_bytes - 1; i >= 0; i--)
+	for (i = t->size - 1; i >= 0; i--)
 		num = num * 256 + bytes[i];
+	nr_copy_bits = bit_sz + bits_offset;
 #elif __BYTE_ORDER == __BIG_ENDIAN
-	for (i = 0; i < nr_copy_bytes; i++)
+	for (i = 0; i < t->size; i++)
 		num = num * 256 + bytes[i];
+	nr_copy_bits = t->size * 8 - bits_offset;
 #else
 # error "Unrecognized __BYTE_ORDER__"
 #endif
-- 
2.31.1

[PATCH bpf-next v2 3/4] libbpf: Fix dumping non-aligned __int128

[Ilya Leoshkevich]

Non-aligned integers are dumped as bields, which is supported for at
most 64-bit integers. Fix by using the same trick as
btf_dump_float_data(): copy non-aligned values to the local buffer.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/lib/bpf/btf_dump.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
index 614719c73bd7..25ce60828e8d 100644
--- a/tools/lib/bpf/btf_dump.c
+++ b/tools/lib/bpf/btf_dump.c
@@ -1670,9 +1670,10 @@ static int btf_dump_int_data(struct btf_dump *d,
 {
 	__u8 encoding = btf_int_encoding(t);
 	bool sign = encoding & BTF_INT_SIGNED;
+	char buf[16] __aligned(16);
 	int sz = t->size;
 
-	if (sz == 0) {
+	if (sz == 0 || sz > sizeof(buf)) {
 		pr_warn("unexpected size %d for id [%u]\n", sz, type_id);
 		return -EINVAL;
 	}
@@ -1680,8 +1681,10 @@ 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 (!ptr_is_aligned(data, sz))
-		return btf_dump_bitfield_data(d, t, data, 0, 0);
+	if (!ptr_is_aligned(data, sz)) {
+		memcpy(buf, data, sz);
+		data = buf;
+	}
 
 	switch (sz) {
 	case 16: {
-- 
2.31.1

[PATCH bpf-next v2 4/4] libbpf: Fix ptr_is_aligned() usages

[Ilya Leoshkevich]

Currently ptr_is_aligned() takes size, and not alignment, as a
parameter, which may be overly pessimistic e.g. for __i128 on s390,
which must be only 8-byte aligned. Fix by using btf__align_of() where
possible - one notable exception is ptr_sz, for which there is no
corresponding type.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/lib/bpf/btf_dump.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
index 25ce60828e8d..da345520892f 100644
--- a/tools/lib/bpf/btf_dump.c
+++ b/tools/lib/bpf/btf_dump.c
@@ -1657,9 +1657,9 @@ 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)
+static bool ptr_is_aligned(const void *data, int alignment)
 {
-	return ((uintptr_t)data) % data_sz == 0;
+	return ((uintptr_t)data) % alignment == 0;
 }
 
 static int btf_dump_int_data(struct btf_dump *d,
@@ -1681,7 +1681,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 (!ptr_is_aligned(data, sz)) {
+	if (!ptr_is_aligned(data, btf__align_of(d->btf, type_id))) {
 		memcpy(buf, data, sz);
 		data = buf;
 	}
@@ -1770,7 +1770,7 @@ static int btf_dump_float_data(struct btf_dump *d,
 	int sz = t->size;
 
 	/* handle unaligned data; copy to local union */
-	if (!ptr_is_aligned(data, sz)) {
+	if (!ptr_is_aligned(data, btf__align_of(d->btf, type_id))) {
 		memcpy(&fl, data, sz);
 		flp = &fl;
 	}
@@ -1953,10 +1953,8 @@ static int btf_dump_get_enum_value(struct btf_dump *d,
 				   __u32 id,
 				   __s64 *value)
 {
-	int sz = t->size;
-
 	/* handle unaligned enum value */
-	if (!ptr_is_aligned(data, sz)) {
+	if (!ptr_is_aligned(data, btf__align_of(d->btf, id))) {
 		__u64 val;
 		int err;
 
-- 
2.31.1

Re: [PATCH bpf-next v2 4/4] libbpf: Fix ptr_is_aligned() usages

[Andrii Nakryiko]

On Wed, Oct 13, 2021 at 9:09 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> Currently ptr_is_aligned() takes size, and not alignment, as a
> parameter, which may be overly pessimistic e.g. for __i128 on s390,
> which must be only 8-byte aligned. Fix by using btf__align_of() where
> possible - one notable exception is ptr_sz, for which there is no
> corresponding type.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  tools/lib/bpf/btf_dump.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
> index 25ce60828e8d..da345520892f 100644
> --- a/tools/lib/bpf/btf_dump.c
> +++ b/tools/lib/bpf/btf_dump.c
> @@ -1657,9 +1657,9 @@ 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)
> +static bool ptr_is_aligned(const void *data, int alignment)
>  {
> -       return ((uintptr_t)data) % data_sz == 0;
> +       return ((uintptr_t)data) % alignment == 0;

btf__align_of() can return 0 on error and this will be div by 0. I
think the better approach would be for ptr_is_aligned to accept struct
btf *btf and __u32 type_id, call btf__align_of() based on btf and type
id, handle 0 case pessimistically (assume not aligned).

>  }
>
>  static int btf_dump_int_data(struct btf_dump *d,
> @@ -1681,7 +1681,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 (!ptr_is_aligned(data, sz)) {
> +       if (!ptr_is_aligned(data, btf__align_of(d->btf, type_id))) {
>                 memcpy(buf, data, sz);
>                 data = buf;
>         }
> @@ -1770,7 +1770,7 @@ static int btf_dump_float_data(struct btf_dump *d,
>         int sz = t->size;
>
>         /* handle unaligned data; copy to local union */
> -       if (!ptr_is_aligned(data, sz)) {
> +       if (!ptr_is_aligned(data, btf__align_of(d->btf, type_id))) {
>                 memcpy(&fl, data, sz);
>                 flp = &fl;
>         }
> @@ -1953,10 +1953,8 @@ static int btf_dump_get_enum_value(struct btf_dump *d,
>                                    __u32 id,
>                                    __s64 *value)
>  {
> -       int sz = t->size;
> -
>         /* handle unaligned enum value */
> -       if (!ptr_is_aligned(data, sz)) {
> +       if (!ptr_is_aligned(data, btf__align_of(d->btf, id))) {
>                 __u64 val;
>                 int err;
>
> --
> 2.31.1
>

Re: [PATCH bpf-next v2 0/4] btf_dump fixes for s390

[Andrii Nakryiko]

On Wed, Oct 13, 2021 at 9:09 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> v1: https://lore.kernel.org/bpf/20211012023218.399568-1-iii@linux.ibm.com/
> v1 -> v2:
> - Remove redundant local variables, use t->size directly instead.
> - Add btf__align_of() patch.
> Pending questions:
> - Can things like defined(__i386__) break cross-compilation?
> - Why exactly do we need both cpu_number and cpu_profile_flip? If we do,
>   is there a suitable replacement for cpu_number in common code?
>
> ---
>
> Hi,
>
> This series along with [1] and [2] fixes all the failures in the
> btf_dump testsuite currently present on s390, in particular:
>
> * [1] fixes intermittent build bug causing "failed to encode tag ..."
>   * error messages.
> * [2] fixes missing VAR entries on s390.
> * Patch 1 disables Intel-specific code in a testcase.
> * Patch 2 fixes an endianness-related bug.
> * Patch 3 fixes an alignment-related bug.
> * Patch 4 improves overly pessimistic alignment handling.
>
> [1] https://lore.kernel.org/bpf/20211012022521.399302-1-iii@linux.ibm.com/
> [2] https://lore.kernel.org/bpf/20211012022637.399365-1-iii@linux.ibm.com/
>
> Best regards,
> Ilya
>
> Ilya Leoshkevich (4):
>   selftests/bpf: Use cpu_number only on arches that have it
>   libbpf: Fix dumping big-endian bitfields
>   libbpf: Fix dumping non-aligned __int128
>   libbpf: Fix ptr_is_aligned() usages
>
>  tools/lib/bpf/btf_dump.c                      | 34 +++++++++----------
>  .../selftests/bpf/prog_tests/btf_dump.c       |  2 ++
>  2 files changed, 19 insertions(+), 17 deletions(-)
>
> --
> 2.31.1
>

I've dropped patch 4 for now, applied the first three to bpf-next, thanks.

Re: [PATCH bpf-next v2 4/4] libbpf: Fix ptr_is_aligned() usages

[Ilya Leoshkevich]

On Wed, 2021-10-20 at 11:44 -0700, Andrii Nakryiko wrote:
> On Wed, Oct 13, 2021 at 9:09 AM Ilya Leoshkevich <iii@linux.ibm.com>
> wrote:
> > 
> > Currently ptr_is_aligned() takes size, and not alignment, as a
> > parameter, which may be overly pessimistic e.g. for __i128 on s390,
> > which must be only 8-byte aligned. Fix by using btf__align_of()
> > where
> > possible - one notable exception is ptr_sz, for which there is no
> > corresponding type.
> > 
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >  tools/lib/bpf/btf_dump.c | 12 +++++-------
> >  1 file changed, 5 insertions(+), 7 deletions(-)
> > 
> > diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
> > index 25ce60828e8d..da345520892f 100644
> > --- a/tools/lib/bpf/btf_dump.c
> > +++ b/tools/lib/bpf/btf_dump.c
> > @@ -1657,9 +1657,9 @@ 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)
> > +static bool ptr_is_aligned(const void *data, int alignment)
> >  {
> > -       return ((uintptr_t)data) % data_sz == 0;
> > +       return ((uintptr_t)data) % alignment == 0;
> 
> btf__align_of() can return 0 on error and this will be div by 0. I
> think the better approach would be for ptr_is_aligned to accept
> struct
> btf *btf and __u32 type_id, call btf__align_of() based on btf and
> type
> id, handle 0 case pessimistically (assume not aligned).

I thought about this, but it won't cover the ptr_sz case. Maybe we
just need two functions - I'll give it a try tomorrow.

Re: [PATCH bpf-next v2 4/4] libbpf: Fix ptr_is_aligned() usages

[Andrii Nakryiko]

On Wed, Oct 20, 2021 at 4:05 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> On Wed, 2021-10-20 at 11:44 -0700, Andrii Nakryiko wrote:
> > On Wed, Oct 13, 2021 at 9:09 AM Ilya Leoshkevich <iii@linux.ibm.com>
> > wrote:
> > >
> > > Currently ptr_is_aligned() takes size, and not alignment, as a
> > > parameter, which may be overly pessimistic e.g. for __i128 on s390,
> > > which must be only 8-byte aligned. Fix by using btf__align_of()
> > > where
> > > possible - one notable exception is ptr_sz, for which there is no
> > > corresponding type.
> > >
> > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > > ---
> > >  tools/lib/bpf/btf_dump.c | 12 +++++-------
> > >  1 file changed, 5 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
> > > index 25ce60828e8d..da345520892f 100644
> > > --- a/tools/lib/bpf/btf_dump.c
> > > +++ b/tools/lib/bpf/btf_dump.c
> > > @@ -1657,9 +1657,9 @@ 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)
> > > +static bool ptr_is_aligned(const void *data, int alignment)
> > >  {
> > > -       return ((uintptr_t)data) % data_sz == 0;
> > > +       return ((uintptr_t)data) % alignment == 0;
> >
> > btf__align_of() can return 0 on error and this will be div by 0. I
> > think the better approach would be for ptr_is_aligned to accept
> > struct
> > btf *btf and __u32 type_id, call btf__align_of() based on btf and
> > type
> > id, handle 0 case pessimistically (assume not aligned).
>
> I thought about this, but it won't cover the ptr_sz case. Maybe we
> just need two functions - I'll give it a try tomorrow.
>

Sorry, what's the ptr_sz case? Is this about btf_ptr_sz() helper somehow?

Re: [PATCH bpf-next v2 4/4] libbpf: Fix ptr_is_aligned() usages

[Ilya Leoshkevich]

On Wed, 2021-10-20 at 16:10 -0700, Andrii Nakryiko wrote:
> On Wed, Oct 20, 2021 at 4:05 PM Ilya Leoshkevich <iii@linux.ibm.com>
> wrote:
> > 
> > On Wed, 2021-10-20 at 11:44 -0700, Andrii Nakryiko wrote:
> > > On Wed, Oct 13, 2021 at 9:09 AM Ilya Leoshkevich
> > > <iii@linux.ibm.com>
> > > wrote:
> > > > 
> > > > Currently ptr_is_aligned() takes size, and not alignment, as a
> > > > parameter, which may be overly pessimistic e.g. for __i128 on
> > > > s390,
> > > > which must be only 8-byte aligned. Fix by using btf__align_of()
> > > > where
> > > > possible - one notable exception is ptr_sz, for which there is no
> > > > corresponding type.
> > > > 
> > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > > > ---
> > > >  tools/lib/bpf/btf_dump.c | 12 +++++-------
> > > >  1 file changed, 5 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
> > > > index 25ce60828e8d..da345520892f 100644
> > > > --- a/tools/lib/bpf/btf_dump.c
> > > > +++ b/tools/lib/bpf/btf_dump.c
> > > > @@ -1657,9 +1657,9 @@ 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)
> > > > +static bool ptr_is_aligned(const void *data, int alignment)
> > > >  {
> > > > -       return ((uintptr_t)data) % data_sz == 0;
> > > > +       return ((uintptr_t)data) % alignment == 0;
> > > 
> > > btf__align_of() can return 0 on error and this will be div by 0. I
> > > think the better approach would be for ptr_is_aligned to accept
> > > struct
> > > btf *btf and __u32 type_id, call btf__align_of() based on btf and
> > > type
> > > id, handle 0 case pessimistically (assume not aligned).
> > 
> > I thought about this, but it won't cover the ptr_sz case. Maybe we
> > just need two functions - I'll give it a try tomorrow.
> > 
> 
> Sorry, what's the ptr_sz case? Is this about btf_ptr_sz() helper
> somehow?

Almost, I'm referring to this check:

static int btf_dump_ptr_data(struct btf_dump *d,
			      const struct btf_type *t,
			      __u32 id,
			      const void *data)
{
	if (ptr_is_aligned(data, d->ptr_sz) && d->ptr_sz ==
sizeof(void *)) {
...