[PATCH bpf-next v2] bpftool: Preserve unsigned enum values in data dumps

[PATCH bpf-next v2] bpftool: Preserve unsigned enum values in data dumps

[Hungyu Lin]

btf_dumper_enum() always loads enum data as signed, compares it with
the signed enumerator field, and uses signed numeric output. Unsigned
values with the high bit set can therefore be printed as negative
numbers. Narrow unsigned values also fail to match their named
enumerators: an 8-bit value of 0xff is loaded as -1, while its
enumerator is stored as 255.

btf_dumper_enum64() likewise uses signed numeric output when no named
enumerator matches. For example, 0x8000000000000001 is printed as
-9223372036854775807 instead of 9223372036854775809.

Respect the enum's signedness when loading ordinary enum data,
comparing it with enumerators, and choosing the numeric writer for
both enum kinds. Keep the signed arm of the 32-bit conversions at
64 bits to avoid unsigned promotion of negative values.

Fixes: ca7e6e45d108 ("bpftool: Print the enum's name instead of value")
Fixes: 58a53978fdf6 ("bpftool: Add btf enum64 support")
Suggested-by: Alexei Starovoitov <ast@kernel.org>
Link: https://lore.kernel.org/bpf/DMJE4YGAEGPX.2PIN4TJJ0AV0N@gmail.com/
Signed-off-by: Hungyu Lin <dennylin0707@gmail.com>
---
Changes in v2:
- Handle ordinary enum signedness in data loads, name matching and numeric
  output, as suggested by Alexei.
- Add the Fixes tag for the ordinary enum name lookup.

For legacy BTF that predates enum signedness, kflag is always 0,
including for signed enums. A one-byte enum with NEG = -1 holding
0xff now prints 255 instead of "NEG", while U255 = 255 now prints
"U255" instead of -1. This change follows the current BTF kflag
semantics, consistent with libbpf's data dumper.

 tools/bpf/bpftool/btf_dumper.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/tools/bpf/bpftool/btf_dumper.c b/tools/bpf/bpftool/btf_dumper.c
index 3267163119dc..09dd428aa12d 100644
--- a/tools/bpf/bpftool/btf_dumper.c
+++ b/tools/bpf/bpftool/btf_dumper.c
@@ -149,7 +149,8 @@ static int btf_dumper_enum(const struct btf_dumper *d,
 			    const void *data)
 {
 	const struct btf_enum *enums = btf_enum(t);
-	__s64 value;
+	bool is_signed = btf_kflag(t);
+	__s64 value, val;
 	__u32 i;
 
 	switch (t->size) {
@@ -157,20 +158,21 @@ static int btf_dumper_enum(const struct btf_dumper *d,
 		value = *(__s64 *)data;
 		break;
 	case 4:
-		value = *(__s32 *)data;
+		value = is_signed ? (__s64)*(__s32 *)data : *(__u32 *)data;
 		break;
 	case 2:
-		value = *(__s16 *)data;
+		value = is_signed ? *(__s16 *)data : *(__u16 *)data;
 		break;
 	case 1:
-		value = *(__s8 *)data;
+		value = is_signed ? *(__s8 *)data : *(__u8 *)data;
 		break;
 	default:
 		return -EINVAL;
 	}
 
 	for (i = 0; i < btf_vlen(t); i++) {
-		if (value == enums[i].val) {
+		val = is_signed ? (__s64)enums[i].val : (__u32)enums[i].val;
+		if (value == val) {
 			jsonw_string(d->jw,
 				     btf__name_by_offset(d->btf,
 							 enums[i].name_off));
@@ -178,7 +180,10 @@ static int btf_dumper_enum(const struct btf_dumper *d,
 		}
 	}
 
-	jsonw_int(d->jw, value);
+	if (is_signed)
+		jsonw_int(d->jw, value);
+	else
+		jsonw_uint(d->jw, value);
 	return 0;
 }
 
@@ -204,7 +209,10 @@ static int btf_dumper_enum64(const struct btf_dumper *d,
 		}
 	}
 
-	jsonw_int(d->jw, value);
+	if (btf_kflag(t))
+		jsonw_int(d->jw, value);
+	else
+		jsonw_uint(d->jw, value);
 	return 0;
 }
 

base-commit: eaae576c34d7e001767b6845a5950b0b8db3e68a
-- 
2.43.0

Re: [PATCH bpf-next v2] bpftool: Preserve unsigned enum values in data dumps

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 3399 bytes --]

> bpftool: Preserve unsigned enum values in data dumps

TL;DR: bpftool enum dump may regress on BTF without kind_flag (older
kernels/LLVM): small negative enumerators like -1 in 1- or 2-byte enums may
print as numbers instead of names; not a crash.

> diff --git a/tools/bpf/bpftool/btf_dumper.c b/tools/bpf/bpftool/btf_dumper.c
> --- a/tools/bpf/bpftool/btf_dumper.c
> +++ b/tools/bpf/bpftool/btf_dumper.c
> @@ -149,7 +149,8 @@ static int btf_dumper_enum(const struct btf_dumper *d,
>  			    const void *data)
>  {
>  	const struct btf_enum *enums = btf_enum(t);
> -	__s64 value;
> +	bool is_signed = btf_kflag(t);
> +	__s64 value, val;
>  	__u32 i;
>
>  	switch (t->size) {

[ ... ]

> @@ -157,20 +158,21 @@ static int btf_dumper_enum(const struct btf_dumper *d,
>  		value = *(__s64 *)data;
>  		break;
>  	case 4:
> -		value = *(__s32 *)data;
> +		value = is_signed ? (__s64)*(__s32 *)data : *(__u32 *)data;
>  		break;
>  	case 2:
> -		value = *(__s16 *)data;
> +		value = is_signed ? *(__s16 *)data : *(__u16 *)data;
>  		break;
>  	case 1:
> -		value = *(__s8 *)data;
> +		value = is_signed ? *(__s8 *)data : *(__u8 *)data;
>  		break;
>  	default:
>  		return -EINVAL;
>  	}
>
>  	for (i = 0; i < btf_vlen(t); i++) {
> -		if (value == enums[i].val) {
> +		val = is_signed ? (__s64)enums[i].val : (__u32)enums[i].val;
> +		if (value == val) {
>  			jsonw_string(d->jw,
>  				     btf__name_by_offset(d->btf,
>  							 enums[i].name_off));

Does treating a clear kind_flag as "unsigned" break name matching for BTF
that carries no signedness information?

A BTF_KIND_ENUM with kflag == 0 and a negative enumerator is common in the
BTF that bpftool reads:

  - kernels before 6.0 reject kind_flag on enums in btf_enum_check_meta()
  - libbpf's bpf_object__sanitize_btf() clears the flag when the kernel
    lacks enum64 support
  - LLVM before 15 never sets the bit

bpftool map dump reads this BTF back from the kernel, so the path is
do_dump_btf() -> btf_dumper_type() -> btf_dumper_do_type() ->
btf_dumper_enum().

For example, with a one byte enum:

  enum __attribute__((packed)) e { NEG = -1, POS = 1 };

where NEG is stored in btf_enum.val as the __s32 -1, kflag is 0, and the map
value byte is 0xff:

  before: value = *(__s8 *)data = -1; enums[i].val = -1, so it matches
          and prints "NEG"

  after:  is_signed = false; value = *(__u8 *)data = 255;
          val = (__u32)enums[i].val = 4294967295, so there is no match
          and it prints 255

For four byte enums the name still matches because both sides are zero
extended the same way.  A negative value that is not an enumerator (for
example -22) is now printed as 4294967274 rather than -22.

Would comparing at the enum's own width in the !is_signed case work for both
encodings and still fix the reported unsigned case?  For instance, comparing
the data against the enumerator truncated to t->size bytes, where
(__u8)255 == 255 and (__u8)-1 == 255 both match a data byte of 0xff.

libbpf's btf_dump_enum_data() makes the same kflag assumption, so this isn't
a crash, but it is a visible change from the earlier bpftool output on older
kernels that bpftool still supports.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/41256964100