Re: [PATCH bpf-next v3 2/2] selftests/bpf: Add overflow test for BTF dump zero-scan

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] selftests/bpf: wrong expected_val in test_btf_dump_struct_data() overflow test
- [Low] selftests/bpf: multi-line comment style in test_btf_dump_struct_data()
--

commit c037995f7dcc94c6cb7bb8863c0b417b37425882
Author: Gu Bin <gubin@cmss.chinamobile.com>

selftests/bpf: Add overflow test for BTF dump zero-scan

This commit adds a TEST_BTF_DUMP_DATA_OVER() test case in
test_btf_dump_struct_data() that dumps an all-zero union bpf_iter_link_info
with a data_sz one byte short of the type size to verify that the zero-scan
returns -E2BIG instead of reading past data_end.

> diff --git a/tools/testing/selftests/bpf/prog_tests/btf_dump.c b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
> index dbef014f75acd..f6cff6cc56c4f 100644
> --- a/tools/testing/selftests/bpf/prog_tests/btf_dump.c
> +++ b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
> @@ -1161,6 +1161,13 @@ static void test_btf_dump_struct_data(struct btf *btf, struct btf_dump *d,
>  			   "(union bpf_iter_link_info){.cgroup = (struct){.cgroup_fd = (__u32)1,},.task = (struct){.pid = (__u32)1,},}",
>  			   { .cgroup = { .cgroup_fd = 1 }});
>  
> +	/* overflow test for all-zero union: with a data_sz one byte short
> +	 * of the type size, the zero-scan must not read past data_end and
> +	 * must return -E2BIG.
> +	 */

[Severity: Low]
This isn't a bug, but in BPF code multi-line comments place the opening /*
on its own line with the comment text starting on the next line. Could this
comment be formatted to match the BPF style?

> +	TEST_BTF_DUMP_DATA_OVER(btf, d, "union", str, union bpf_iter_link_info,
> +				sizeof(union bpf_iter_link_info) - 1, "", {});

[Severity: Medium]
Will this test fail the expected string check in btf_dump_data()?

When btf_dump__dump_type_data() enters btf_dump_dump_type_data() at
d->typed_dump->depth == 0 for a union:

tools/lib/bpf/btf_dump.c:btf_dump_dump_type_data() {
	size = btf_dump_type_data_check_overflow(d, t, id, data, bits_offset, bit_sz);
	if (size < 0)
		return size;
	err = btf_dump_type_data_check_zero(d, t, id, data, bits_offset, bit_sz);
	if (err) {
		...
	}
	btf_dump_data_pfx(d);

	if (!d->typed_dump->skip_names) {
		if (fname && strlen(fname) > 0)
			btf_dump_printf(d, ".%s = ", fname);
		btf_dump_emit_type_cast(d, id, true);
	}
}

At depth == 0, btf_dump_type_data_check_overflow() only checks base types and
returns the union size (16), and btf_dump_type_data_check_zero() immediately
returns 0. Then btf_dump_emit_type_cast() and btf_dump_struct_data() emit
"(union bpf_iter_link_info){\n" into str before iterating over the union
members at depth == 1, where btf_dump_type_data_check_zero() on the .cgroup
member detects the overflow and returns -E2BIG.

In btf_dump_data(), after verifying that btf_dump__dump_type_data() returns
-E2BIG, it checks:

	if (!ASSERT_STREQ(str, expected_val, "ensure expected/actual match"))
		return -EFAULT;

Because str contains "(union bpf_iter_link_info){\n" while expected_val is
passed as "", shouldn't expected_val be "(union bpf_iter_link_info){\n" here
(similar to the atomic_t overflow test in test_btf_dump_int_data())?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103032619.1378-1-gubin@cmss.chinamobile.com?part=2