[PATCH v2 bpf-next 0/3] libbpf: btf typed data dumping fixes (__int128 usage, error propagation)

[PATCH v2 bpf-next 0/3] libbpf: btf typed data dumping fixes (__int128 usage, error propagation)

[Alan Maguire]

This series aims to resolve further issues with the BTF typed data
dumping interfaces in libbpf.

Compilation failures with use of __int128 on 32-bit platforms were
reported [1].  As a result, the use of __int128 in libbpf typed data
dumping is replaced with __u64 usage for bitfield manipulations.
In the case of 128-bit integer values, they are simply split into
two 64-bit hex values for display (patch 1).

Tests are added for __int128 display in patch 2, using conditional
compilation to avoid problems with a lack of __int128 support.

Patch 3 resolves an issue Andrii noted about error propagation
when handling enum data display.

More followup work is required to ensure multi-dimensional char array
display works correctly.

[1] https://lore.kernel.org/bpf/1626362126-27775-1-git-send-email-alan.maguire@oracle.com/T/#mc2cb023acfd6c3cd0b661e385787b76bb757430d

Changes since v1:

 - added error handling for bitfield size > 64 bits by changing function
   signature for bitfield retrieval to return an int error value and to set
   bitfield value via a __u64 * argument (Andrii)

Alan Maguire (3):
  libbpf: avoid use of __int128 in typed dump display
  selftests/bpf: add __int128-specific tests for typed data dump
  libbpf: propagate errors when retrieving enum value for typed data
    display

 tools/lib/bpf/btf_dump.c                          | 103 ++++++++++++++--------
 tools/testing/selftests/bpf/prog_tests/btf_dump.c |  17 ++++
 2 files changed, 85 insertions(+), 35 deletions(-)

-- 
1.8.3.1

[PATCH v2 bpf-next 1/3] libbpf: avoid use of __int128 in typed dump display

[Alan Maguire]

__int128 is not supported for some 32-bit platforms (arm and i386).
__int128 was used in carrying out computations on bitfields which
aid display, but the same calculations could be done with __u64
with the small effect of not supporting 128-bit bitfields.

With these changes, a big-endian issue with casting 128-bit integers
to 64-bit for enum bitfields is solved also, as we now use 64-bit
integers for bitfield calculations.

Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 tools/lib/bpf/btf_dump.c | 98 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 65 insertions(+), 33 deletions(-)

diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
index accf6fe..d52e546 100644
--- a/tools/lib/bpf/btf_dump.c
+++ b/tools/lib/bpf/btf_dump.c
@@ -1552,31 +1552,26 @@ static int btf_dump_unsupported_data(struct btf_dump *d,
 	return -ENOTSUP;
 }
 
-static void btf_dump_int128(struct btf_dump *d,
-			    const struct btf_type *t,
-			    const void *data)
-{
-	__int128 num = *(__int128 *)data;
-
-	if ((num >> 64) == 0)
-		btf_dump_type_values(d, "0x%llx", (long long)num);
-	else
-		btf_dump_type_values(d, "0x%llx%016llx", (long long)num >> 32,
-				     (long long)num);
-}
-
-static unsigned __int128 btf_dump_bitfield_get_data(struct btf_dump *d,
-						    const struct btf_type *t,
-						    const void *data,
-						    __u8 bits_offset,
-						    __u8 bit_sz)
+static int btf_dump_get_bitfield_value(struct btf_dump *d,
+				       const struct btf_type *t,
+				       const void *data,
+				       __u8 bits_offset,
+				       __u8 bit_sz,
+				       __u64 *value)
 {
 	__u16 left_shift_bits, right_shift_bits;
 	__u8 nr_copy_bits, nr_copy_bytes;
-	unsigned __int128 num = 0, ret;
 	const __u8 *bytes = data;
+	int sz = t->size;
+	__u64 num = 0;
 	int i;
 
+	/* Maximum supported bitfield size is 64 bits */
+	if (sz > 8) {
+		pr_warn("unexpected bitfield size %d\n", sz);
+		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.
 	 */
@@ -1591,12 +1586,12 @@ static unsigned __int128 btf_dump_bitfield_get_data(struct btf_dump *d,
 #else
 # error "Unrecognized __BYTE_ORDER__"
 #endif
-	left_shift_bits = 128 - nr_copy_bits;
-	right_shift_bits = 128 - bit_sz;
+	left_shift_bits = 64 - nr_copy_bits;
+	right_shift_bits = 64 - bit_sz;
 
-	ret = (num << left_shift_bits) >> right_shift_bits;
+	*value = (num << left_shift_bits) >> right_shift_bits;
 
-	return ret;
+	return 0;
 }
 
 static int btf_dump_bitfield_check_zero(struct btf_dump *d,
@@ -1605,9 +1600,12 @@ static int btf_dump_bitfield_check_zero(struct btf_dump *d,
 					__u8 bits_offset,
 					__u8 bit_sz)
 {
-	__int128 check_num;
+	__u64 check_num;
+	int err;
 
-	check_num = btf_dump_bitfield_get_data(d, t, data, bits_offset, bit_sz);
+	err = btf_dump_get_bitfield_value(d, t, data, bits_offset, bit_sz, &check_num);
+	if (err)
+		return err;
 	if (check_num == 0)
 		return -ENODATA;
 	return 0;
@@ -1619,10 +1617,14 @@ static int btf_dump_bitfield_data(struct btf_dump *d,
 				  __u8 bits_offset,
 				  __u8 bit_sz)
 {
-	unsigned __int128 print_num;
+	__u64 print_num;
+	int err;
+
+	err = btf_dump_get_bitfield_value(d, t, data, bits_offset, bit_sz, &print_num);
+	if (err)
+		return err;
 
-	print_num = btf_dump_bitfield_get_data(d, t, data, bits_offset, bit_sz);
-	btf_dump_int128(d, t, &print_num);
+	btf_dump_type_values(d, "0x%llx", (unsigned long long)print_num);
 
 	return 0;
 }
@@ -1681,9 +1683,29 @@ static int btf_dump_int_data(struct btf_dump *d,
 		return btf_dump_bitfield_data(d, t, data, 0, 0);
 
 	switch (sz) {
-	case 16:
-		btf_dump_int128(d, t, data);
+	case 16: {
+		const __u64 *ints = data;
+		__u64 lsi, msi;
+
+		/* avoid use of __int128 as some 32-bit platforms do not
+		 * support it.
+		 */
+#if __BYTE_ORDER == __LITTLE_ENDIAN
+		lsi = ints[0];
+		msi = ints[1];
+#elif __BYTE_ORDER == __BIG_ENDIAN
+		lsi = ints[1];
+		msi = ints[0];
+#else
+# error "Unrecognized __BYTE_ORDER__"
+#endif
+		if (msi == 0)
+			btf_dump_type_values(d, "0x%llx", (unsigned long long)lsi);
+		else
+			btf_dump_type_values(d, "0x%llx%016llx", (unsigned long long)msi,
+					     (unsigned long long)lsi);
 		break;
+	}
 	case 8:
 		if (sign)
 			btf_dump_type_values(d, "%lld", *(long long *)data);
@@ -1931,9 +1953,16 @@ static int btf_dump_get_enum_value(struct btf_dump *d,
 
 	/* handle unaligned enum value */
 	if (!ptr_is_aligned(data, sz)) {
-		*value = (__s64)btf_dump_bitfield_get_data(d, t, data, 0, 0);
+		__u64 val;
+		int err;
+
+		err = btf_dump_get_bitfield_value(d, t, data, 0, 0, &val);
+		if (err)
+			return err;
+		*value = (__s64)val;
 		return 0;
 	}
+
 	switch (t->size) {
 	case 8:
 		*value = *(__s64 *)data;
@@ -2209,10 +2238,13 @@ static int btf_dump_dump_type_data(struct btf_dump *d,
 	case BTF_KIND_ENUM:
 		/* handle bitfield and int enum values */
 		if (bit_sz) {
-			unsigned __int128 print_num;
+			__u64 print_num;
 			__s64 enum_val;
 
-			print_num = btf_dump_bitfield_get_data(d, t, data, bits_offset, bit_sz);
+			err = btf_dump_get_bitfield_value(d, t, data, bits_offset, bit_sz,
+							  &print_num);
+			if (err)
+				break;
 			enum_val = (__s64)print_num;
 			err = btf_dump_enum_data(d, t, id, &enum_val);
 		} else
-- 
1.8.3.1

[PATCH v2 bpf-next 2/3] selftests/bpf: add __int128-specific tests for typed data dump

[Alan Maguire]

Add tests for __int128 display for platforms that support it.
__int128s are dumped as hex values.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 tools/testing/selftests/bpf/prog_tests/btf_dump.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/btf_dump.c b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
index 0b4ba53..52ccf0c 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf_dump.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
@@ -327,6 +327,14 @@ static int btf_dump_data(struct btf *btf, struct btf_dump *d,
 static void test_btf_dump_int_data(struct btf *btf, struct btf_dump *d,
 				   char *str)
 {
+#ifdef __SIZEOF_INT128__
+	__int128 i = 0xffffffffffffffff;
+
+	/* this dance is required because we cannot directly initialize
+	 * a 128-bit value to anything larger than a 64-bit value.
+	 */
+	i = (i << 64) | (i - 1);
+#endif
 	/* simple int */
 	TEST_BTF_DUMP_DATA_C(btf, d, NULL, str, int, BTF_F_COMPACT, 1234);
 	TEST_BTF_DUMP_DATA(btf, d, NULL, str, int, BTF_F_COMPACT | BTF_F_NONAME,
@@ -348,6 +356,15 @@ static void test_btf_dump_int_data(struct btf *btf, struct btf_dump *d,
 	TEST_BTF_DUMP_DATA(btf, d, NULL, str, int, 0, "(int)-4567", -4567);
 
 	TEST_BTF_DUMP_DATA_OVER(btf, d, NULL, str, int, sizeof(int)-1, "", 1);
+
+#ifdef __SIZEOF_INT128__
+	TEST_BTF_DUMP_DATA(btf, d, NULL, str, __int128, BTF_F_COMPACT,
+			   "(__int128)0xffffffffffffffff",
+			   0xffffffffffffffff);
+	ASSERT_OK(btf_dump_data(btf, d, "__int128", NULL, 0, &i, 16, str,
+				"(__int128)0xfffffffffffffffffffffffffffffffe"),
+		  "dump __int128");
+#endif
 }
 
 static void test_btf_dump_float_data(struct btf *btf, struct btf_dump *d,
-- 
1.8.3.1

[PATCH v2 bpf-next 3/3] libbpf: propagate errors when retrieving enum value for typed data display

[Alan Maguire]

When retrieving the enum value associated with typed data during
"is data zero?" checking in btf_dump_type_data_check_zero(), the
return value of btf_dump_get_enum_value() is not passed to the caller
if the function returns a non-zero (error) value.  Currently, 0
is returned if the function returns an error.  We should instead
propagate the error to the caller.

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

diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
index d52e546..e4b483f 100644
--- a/tools/lib/bpf/btf_dump.c
+++ b/tools/lib/bpf/btf_dump.c
@@ -2166,8 +2166,9 @@ static int btf_dump_type_data_check_zero(struct btf_dump *d,
 		return -ENODATA;
 	}
 	case BTF_KIND_ENUM:
-		if (btf_dump_get_enum_value(d, t, data, id, &value))
-			return 0;
+		err = btf_dump_get_enum_value(d, t, data, id, &value);
+		if (err)
+			return err;
 		if (value == 0)
 			return -ENODATA;
 		return 0;
-- 
1.8.3.1

Re: [PATCH v2 bpf-next 0/3] libbpf: btf typed data dumping fixes (__int128 usage, error propagation)

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (refs/heads/master):

On Tue, 20 Jul 2021 09:49:50 +0100 you wrote:
> This series aims to resolve further issues with the BTF typed data
> dumping interfaces in libbpf.
> 
> Compilation failures with use of __int128 on 32-bit platforms were
> reported [1].  As a result, the use of __int128 in libbpf typed data
> dumping is replaced with __u64 usage for bitfield manipulations.
> In the case of 128-bit integer values, they are simply split into
> two 64-bit hex values for display (patch 1).
> 
> [...]

Here is the summary with links:
  - [v2,bpf-next,1/3] libbpf: avoid use of __int128 in typed dump display
    https://git.kernel.org/bpf/bpf-next/c/a1d3cc3c5eca
  - [v2,bpf-next,2/3] selftests/bpf: add __int128-specific tests for typed data dump
    https://git.kernel.org/bpf/bpf-next/c/a17553dde294
  - [v2,bpf-next,3/3] libbpf: propagate errors when retrieving enum value for typed data display
    https://git.kernel.org/bpf/bpf-next/c/720c29fca9fb

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