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

[bot+bpf-ci]

> 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