Re: [PATCH] selftests/bpf: Fix map_lookup_percpu_elem on sparse CPU IDs

[bot+bpf-ci@kernel.org]

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

> diff --git a/tools/testing/selftests/bpf/prog_tests/map_lookup_percpu_elem.c b/tools/testing/selftests/bpf/prog_tests/map_lookup_percpu_elem.c
> index bfb1bf3fd427b..849f33259c003 100644
> --- a/tools/testing/selftests/bpf/prog_tests/map_lookup_percpu_elem.c
> +++ b/tools/testing/selftests/bpf/prog_tests/map_lookup_percpu_elem.c
> @@ -2,29 +2,45 @@
>  /* Copyright (c) 2022 Bytedance */
>
>  #include <test_progs.h>
> +#include "bpf/libbpf_internal.h"
>  #include "test_map_lookup_percpu_elem.skel.h"
>
>  void test_map_lookup_percpu_elem(void)
>  {
>  	struct test_map_lookup_percpu_elem *skel;
> -	__u64 key = 0, sum;
> -	int ret, i, nr_cpus = libbpf_num_possible_cpus();
> +	bool *possible = NULL;
> +	__u64 key = 0, sum = 0;
> +	int cpu, nr_cpu_ids, nr_cpus, ret, slot = 0;
>  	__u64 *buf;
>
> -	buf = malloc(nr_cpus*sizeof(__u64));
> -	if (!ASSERT_OK_PTR(buf, "malloc"))
> +	ret = parse_cpu_mask_file("/sys/devices/system/cpu/possible", &possible,
> +				  &nr_cpu_ids);
> +	if (!ASSERT_OK(ret, "parse possible CPU mask"))
>  		return;
>
> -	for (i = 0; i < nr_cpus; i++)
> -		buf[i] = i;
> -	sum = (nr_cpus - 1) * nr_cpus / 2;
> +	nr_cpus = libbpf_num_possible_cpus();
> +	if (!ASSERT_GT(nr_cpus, 0, "libbpf_num_possible_cpus"))
> +		goto free_mask;
> +
> +	buf = malloc(nr_cpus * sizeof(*buf));
> +	if (!ASSERT_OK_PTR(buf, "malloc"))
> +		goto free_mask;
> +
> +	for (cpu = 0; cpu < nr_cpu_ids; cpu++) {
> +		if (!possible[cpu])
> +			continue;
> +		buf[slot++] = cpu;
> +		sum += cpu;
> +	}

Does this buffer layout match how the kernel expects it?

The userspace side fills buf[] with one entry per possible CPU, storing
the logical CPU ID at packed index 'slot' (0, 1, 2, ...). For a mask
like 0,2-3, this creates buf = {0, 2, 3} with 3 entries total.

Looking at the kernel's update path, both percpu array and hash maps
index the user buffer by logical CPU ID:

kernel/bpf/arraymap.c:469 bpf_percpu_array_update()
    for_each_possible_cpu(cpu) {
            ptr = per_cpu_ptr(pptr, cpu);
            val = (map_flags & BPF_F_ALL_CPUS) ? value : value + size * cpu;

kernel/bpf/hashtab.c:1038 pcpu_copy_value()
    for_each_possible_cpu(cpu) {
            ...
            copy_map_value_long(&htab->map, per_cpu_ptr(pptrs, cpu),
                    value + off + size * cpu);

With the mask 0,2-3 (num_possible_cpus() = 3, nr_cpu_ids = 4), the
update walks possible CPUs {0, 2, 3} and indexes the buffer as:

  cpu 0:  *(value + 0)  = buf[0] = 0   (intended)
  cpu 2:  *(value + 16) = buf[2] = 3   (expects 2, gets 3)
  cpu 3:  *(value + 24)             (past the 24-byte buffer end)

The userspace layout is packed (3 entries), but the kernel accesses it
as sparse (indexed by CPU ID). The test sized the buffer for
num_possible_cpus() bytes, but the kernel's 'size * cpu' indexing can
exceed that when the possible mask has holes.

Should the userspace buffer either be laid out by CPU ID (sized for
nr_cpu_ids, with holes at non-possible indices), or should the kernel's
update path use packed indexing like the lookup direction does?

> +	if (!ASSERT_EQ(slot, nr_cpus, "possible CPU mask weight"))
> +		goto exit;
>
>  	skel = test_map_lookup_percpu_elem__open();
>  	if (!ASSERT_OK_PTR(skel, "test_map_lookup_percpu_elem__open"))
>  		goto exit;
>
>  	skel->rodata->my_pid = getpid();
> -	skel->rodata->nr_cpus = nr_cpus;
> +	skel->rodata->nr_cpu_ids = nr_cpu_ids;
>
>  	ret = test_map_lookup_percpu_elem__load(skel);

[ ... ]


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