[PATCH bpf-next 0/3] libbpf: btf typed data dumping fixes (__int128 usage, error propagation)
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@o...
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 | 67 +++++++++++++---------- tools/testing/selftests/bpf/prog_tests/btf_dump.c | 17 ++++++ 2 files changed, 55 insertions(+), 29 deletions(-)
__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 | 62 +++++++++++++++++++++++++++--------------------- 1 file changed, 35 insertions(+), 27 deletions(-)
diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c index accf6fe..4a25512 100644 --- a/tools/lib/bpf/btf_dump.c +++ b/tools/lib/bpf/btf_dump.c @@ -1552,28 +1552,15 @@ 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 __u64 btf_dump_bitfield_get_data(struct btf_dump *d, + const struct btf_type *t, + const void *data, + __u8 bits_offset, + __u8 bit_sz) { __u16 left_shift_bits, right_shift_bits; __u8 nr_copy_bits, nr_copy_bytes; - unsigned __int128 num = 0, ret; + __u64 num = 0, ret; const __u8 *bytes = data; int i;
@@ -1591,8 +1578,8 @@ 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;
@@ -1605,7 +1592,7 @@ static int btf_dump_bitfield_check_zero(struct btf_dump *d, __u8 bits_offset, __u8 bit_sz) { - __int128 check_num; + __u64 check_num;
check_num = btf_dump_bitfield_get_data(d, t, data, bits_offset, bit_sz); if (check_num == 0) @@ -1619,10 +1606,11 @@ static int btf_dump_bitfield_data(struct btf_dump *d, __u8 bits_offset, __u8 bit_sz) { - unsigned __int128 print_num; + __u64 print_num;
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 +1669,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); @@ -2209,7 +2217,7 @@ 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);
On Mon, Jul 19, 2021 at 2:41 PM Alan Maguire alan.maguire@oracle.com wrote:
__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
Changes look good to me, thanks. But they didn't appear in patchworks yet so I can't easily test and apply them. It might be because of patchworks delay or due to a very long CC list. Try trimming the cc list down and re-submit?
Also, while I agree that supporting 128-bit bitfields isn't important, I wonder if we should warn/error on that (instead of shifting by negative amount and reporting some garbage value), what do you think? Is there one place in the code where we can error out early if the type actually has bitfield with > 64 bits? I'd prefer to keep btf_dump_bitfield_get_data() itself non-failing though.
tools/lib/bpf/btf_dump.c | 62 +++++++++++++++++++++++++++--------------------- 1 file changed, 35 insertions(+), 27 deletions(-)
[...]
On Mon, 19 Jul 2021, Andrii Nakryiko wrote:
On Mon, Jul 19, 2021 at 2:41 PM Alan Maguire alan.maguire@oracle.com wrote:
__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
Changes look good to me, thanks. But they didn't appear in patchworks yet so I can't easily test and apply them. It might be because of patchworks delay or due to a very long CC list. Try trimming the cc list down and re-submit?
Done, looks like the v2 with the trimmed cc list made it into patchwork this time.
Also, while I agree that supporting 128-bit bitfields isn't important, I wonder if we should warn/error on that (instead of shifting by negative amount and reporting some garbage value), what do you think? Is there one place in the code where we can error out early if the type actually has bitfield with > 64 bits? I'd prefer to keep btf_dump_bitfield_get_data() itself non-failing though.
Sorry, I missed the last part and made that function fail since it's probably the easiest place to capture too-large bitfields. I renamed it to btf_dump_get_bitfield_value() to match btf_dump_get_enum_value() which as a similar function signature (return int, pass in a pointer to the value we want to retrieve).
We can't localize bitfield size checking to btf_dump_type_data_check_zero() because - depending on flags - the associated checks might not be carried out. So duplication of bitfield size checks between the zero checking and bitfield/enum bitfield display seems inevitable, and that being the case, the extra error checking required around btf_dump_get_bitfield_value() seems to be required.
I might be missing a better approach here of course; let me know what you think. Thanks again!
Alan
On Tue, Jul 20, 2021 at 2:14 AM Alan Maguire alan.maguire@oracle.com wrote:
On Mon, 19 Jul 2021, Andrii Nakryiko wrote:
On Mon, Jul 19, 2021 at 2:41 PM Alan Maguire alan.maguire@oracle.com wrote:
__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
Changes look good to me, thanks. But they didn't appear in patchworks yet so I can't easily test and apply them. It might be because of patchworks delay or due to a very long CC list. Try trimming the cc list down and re-submit?
Done, looks like the v2 with the trimmed cc list made it into patchwork this time.
v1 also made it to the list right after I wrote the email :)
Also, while I agree that supporting 128-bit bitfields isn't important, I wonder if we should warn/error on that (instead of shifting by negative amount and reporting some garbage value), what do you think? Is there one place in the code where we can error out early if the type actually has bitfield with > 64 bits? I'd prefer to keep btf_dump_bitfield_get_data() itself non-failing though.
Sorry, I missed the last part and made that function fail since it's probably the easiest place to capture too-large bitfields. I renamed it to btf_dump_get_bitfield_value() to match btf_dump_get_enum_value() which as a similar function signature (return int, pass in a pointer to the value we want to retrieve).
We can't localize bitfield size checking to btf_dump_type_data_check_zero() because - depending on flags - the associated checks might not be carried out. So duplication of bitfield size checks between the zero checking and bitfield/enum bitfield display seems inevitable, and that being the case, the extra error checking required around btf_dump_get_bitfield_value() seems to be required.
I might be missing a better approach here of course; let me know what you think. Thanks again!
Nah, that's fine. Looks good. Testing and pushing in a few minutes. Thanks.
Alan
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,
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 4a25512..ddc900b 100644 --- a/tools/lib/bpf/btf_dump.c +++ b/tools/lib/bpf/btf_dump.c @@ -2145,8 +2145,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;
linux-kselftest-mirror@lists.linaro.org
-
Alan Maguire -
Andrii Nakryiko