Re: [PATCH bpf v3] selftests/bpf: Check returned keys in map batch validators

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The patch introduces new uses of the deprecated `CHECK()` macro for assertions instead of migrating to the required `ASSERT_*()` macros.
--

commit ea80e2126710dbf0e0f3a1f8861acf3e34b2207d
Author: Tianyi Chen <hi@tychen.cc>

selftests/bpf: Check returned keys in map batch validators

This patch updates the hash, array and LPM trie batch validators to check
returned keys against the populated set, and indexes the visited array by
key. It aims to detect missing entries while preserving unordered results
and per-CPU value validation.

> diff --git a/tools/testing/selftests/bpf/map_tests/array_map_batch_ops.c b/tools/testing/selftests/bpf/map_tests/array_map_batch_ops.c
> index b595556315bc3..76d3800a82af9 100644
> --- a/tools/testing/selftests/bpf/map_tests/array_map_batch_ops.c
> +++ b/tools/testing/selftests/bpf/map_tests/array_map_batch_ops.c
> @@ -45,6 +45,9 @@ static void map_batch_verify(int *visited, __u32 max_entries, int *keys,
>  
>  	memset(visited, 0, max_entries * sizeof(*visited));
>  	for (i = 0; i < max_entries; i++) {
> +		CHECK(keys[i] < 0 || keys[i] >= max_entries, "key checking",
> +		      "error: i %d key %d out of range\n", i, keys[i]);
> +

[Severity: Low]
Does this new assertion use a deprecated macro?

According to the BPF selftest subsystem guidelines, the CHECK() macro is
deprecated and should be avoided in new tests or patches. Should we use the
modern ASSERT_*() family of macros (like ASSERT_GE and ASSERT_LT) here
instead?

[ ... ]
> diff --git a/tools/testing/selftests/bpf/map_tests/htab_map_batch_ops.c b/tools/testing/selftests/bpf/map_tests/htab_map_batch_ops.c
> index 5da493b94ae25..430949f9691d7 100644
> --- a/tools/testing/selftests/bpf/map_tests/htab_map_batch_ops.c
> +++ b/tools/testing/selftests/bpf/map_tests/htab_map_batch_ops.c
> @@ -50,6 +50,8 @@ static void map_batch_verify(int *visited, __u32 max_entries,
>  
>  	memset(visited, 0, max_entries * sizeof(*visited));
>  	for (i = 0; i < max_entries; i++) {
> +		CHECK(keys[i] < 1 || keys[i] > max_entries, "key checking",
> +		      "error: i %d key %d out of range\n", i, keys[i]);

[Severity: Low]
Could this assertion also be replaced with ASSERT_GE() and ASSERT_LE() to
follow current selftests standards?

[ ... ]
> diff --git a/tools/testing/selftests/bpf/map_tests/lpm_trie_map_batch_ops.c b/tools/testing/selftests/bpf/map_tests/lpm_trie_map_batch_ops.c
> index fe3e19f962446..3b51670b3cd4b 100644
> --- a/tools/testing/selftests/bpf/map_tests/lpm_trie_map_batch_ops.c
> +++ b/tools/testing/selftests/bpf/map_tests/lpm_trie_map_batch_ops.c
> @@ -44,18 +44,19 @@ static void map_batch_update(int map_fd, __u32 max_entries,
>  static void map_batch_verify(int *visited, __u32 max_entries,
>  			     struct test_lpm_key *keys, int *values)
>  {
> -	char buff[16] = { 0 };
> -	int lower_byte = 0;
> -	__u32 i;
> +	__u32 i, ipv4, key;
>  
>  	memset(visited, 0, max_entries * sizeof(*visited));
>  	for (i = 0; i < max_entries; i++) {
> -		inet_ntop(AF_INET, &keys[i].ipv4, buff, 32);
> -		CHECK(sscanf(buff, "192.168.1.%d", &lower_byte) == EOF,
> -		      "sscanf()", "error: i %d\n", i);
> -		CHECK(lower_byte != values[i], "key/value checking",
> -		      "error: i %d key %s value %d\n", i, buff, values[i]);
> -		visited[i] = 1;
> +		ipv4 = ntohl(keys[i].ipv4.s_addr);
> +		key = ipv4 & 0xff;
> +		/* Expected keys are 192.168.1.1..max_entries with a /32 prefix. */
> +		CHECK(keys[i].prefix != 32 || (ipv4 & 0xffffff00) != 0xc0a80100 ||
> +		      key == 0 || key > max_entries, "key checking",
> +		      "error: i %u prefix %u ipv4 %#x\n", i, keys[i].prefix, ipv4);

[Severity: Low]
Even though the surrounding code previously used CHECK() in this function, the
BPF guidelines indicate that we should avoid introducing new uses of it. Can we
upgrade these new assertions to use the modern ASSERT_*() macros instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911025036.190487-1-diannaaav@gmail.com?part=1