Re: [PATCH bpf-next v4 3/3] selftests/bpf: Cover enum signedness in data dumps and map fields

[bot+bpf-ci@kernel.org]

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

> selftests/bpf: Cover enum signedness in data dumps and map fields

TL;DR: The wide_unknown btf_dump test only checks for digits, so it may pass
even when libbpf prints a truncated 32-bit value; a possible crash on 32-bit
hosts is also raised.

> diff --git a/tools/testing/selftests/bpf/prog_tests/btf_dump.c b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
> --- a/tools/testing/selftests/bpf/prog_tests/btf_dump.c
> +++ b/tools/testing/selftests/bpf/prog_tests/btf_dump.c

[ ... ]

> +static void test_btf_dump_enum_values(void)
> +{

[ ... ]

> +		/*
> +		 * Wider data must not match an enumerator through its low 32 bits.
> +		 * Check numeric output without requiring an exact value.
> +		 */
> +		{ "wide_unknown", 8, 0xffffffffLL,
> +		  { .u64 = 0x1ffffffffULL }, NULL },
> +	};

[ ... ]

> +		if (cases[i].expected) {
> +			ASSERT_STREQ(str, cases[i].expected, cases[i].name);
> +		} else {
> +			ASSERT_NEQ(str[0], '\0', cases[i].name);
> +			ASSERT_EQ(strspn(str, "0123456789"), strlen(str),
> +				  cases[i].name);
> +		}

Does the wide_unknown case actually guard against the low 32 bits being
shown?

The check only requires the output to consist of digits, which also
passes when libbpf prints the truncated value.  In btf_dump_enum_data()
the fallback for an unmatched BTF_KIND_ENUM value is:

    __s64 value;
    ...
    btf_dump_type_values(d, is_signed ? "%d" : "%u", value);

The value is an __s64 but the format is %d or %u.  For an 8-byte enum
holding 0x1ffffffff, x86-64 would print "4294967295", which is the low 32
bits and equals the VALUE enumerator (0xffffffff).  That is the situation
the comment and the commit message say this case guards against, yet the
test passes.

The bpftool counterpart (enum_wide_unknown in bpftool_map_dump.c) requires
"8589934591", and passes because btf_dumper_enum() uses jsonw_uint() with
a __u64.

On 32-bit hosts the 64-bit argument takes two vararg slots, so the high
word would be consumed as the char * for the "%s" appended by
btf_dump_type_values().  Could that crash test_progs in this subtest?

Related, in existing code: the macro expands to

    is_signed ? "%d" : "%u" "%s%s"

so string literal concatenation applies only to the unsigned branch, and
signed unmatched values (and the ENUM64 "%lldLL" branch) are printed
without the delimiter and newline.  This was not introduced here; it
dates from d90ec262b35b ("libbpf: Add enum64 support for btf_dump").

Would it make sense to have the libbpf fallback print a 64-bit value (for
example separate %lld/%llu calls with a long long cast) so that this case
can require "8589934591" as the bpftool test does?


---
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/42329238242