Re: [PATCH bpf-next 3/3] selftests/bpf: Add tests to verify no unintended eviction when updating lru hash maps

[bot+bpf-ci@kernel.org]

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

> diff --git a/tools/testing/selftests/bpf/prog_tests/htab_update.c b/tools/testing/selftests/bpf/prog_tests/htab_update.c
> index d0b405eb2..bd29a915b 100644
> --- a/tools/testing/selftests/bpf/prog_tests/htab_update.c
> +++ b/tools/testing/selftests/bpf/prog_tests/htab_update.c

[ ... ]

> +static void test_lru_hash_map_update_elem(enum bpf_map_type map_type)
> +{
> +	int err, map_fd, i, key, nr_cpus, max_entries = 128;
> +	u64 *values, value = 0xDEADC0DE;
> +
> +	nr_cpus = libbpf_num_possible_cpus();
> +	if (!ASSERT_GT(nr_cpus, 0, "libbpf_num_possible_cpus"))
> +		return;
> +
> +	values = calloc(nr_cpus, sizeof(u64));

[ ... ]

> +	err = bpf_map_lookup_elem(map_fd, &key, values);
> +	if (!ASSERT_OK(err, "bpf_map_lookup_elem"))
> +		goto out;
> +	if (!ASSERT_EQ(*values, 0, "bpf_map_lookup_elem value"))
> +		goto out;
> +
> +	for (key = 1; key < max_entries; key++) {
> +		err = bpf_map_lookup_elem(map_fd, &key, values);
> +		if (!ASSERT_OK(err, "bpf_map_lookup_elem"))
> +			goto out;
> +		if (!ASSERT_EQ(*values, value, "bpf_map_lookup_elem value"))
> +			goto out;
> +	}

When test_lru_hash_map_update_elem() is called with
BPF_MAP_TYPE_LRU_PERCPU_HASH via test_update_lru_percpu_hash_map(),
should all CPU values be checked rather than just *values?

For percpu maps, bpf_map_lookup_elem() returns an array with one value
per CPU. The test allocates space for nr_cpus values but only verifies
the first element. This means values for CPUs 1 through nr_cpus-1 are
not validated.

Looking at tools/testing/selftests/bpf/prog_tests/lookup_and_delete.c,
the pattern for percpu maps is to loop through all CPUs:

    for (i = 0; i < nr_cpus; i++) {
        val = value[i];
        /* check val */
    }

For BPF_MAP_TYPE_LRU_HASH this is correct, but for
BPF_MAP_TYPE_LRU_PERCPU_HASH the test may pass even if non-primary
CPUs have incorrect values.

> +
> +out:
> +	close(map_fd);
> +	free(values);
> +}
> +
> +static void test_update_lru_hash_map(void)
> +{
> +	test_lru_hash_map_update_elem(BPF_MAP_TYPE_LRU_HASH);
> +}
> +
> +static void test_update_lru_percpu_hash_map(void)
> +{
> +	test_lru_hash_map_update_elem(BPF_MAP_TYPE_LRU_PERCPU_HASH);
> +}

[ ... ]


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