Re: [PATCH bpf v2 2/2] selftests/bpf: Test per-cpu initialization of a BPF_F_CPU created element

[bot+bpf-ci@kernel.org]

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

> diff --git a/tools/testing/selftests/bpf/prog_tests/percpu_alloc.c b/tools/testing/selftests/bpf/prog_tests/percpu_alloc.c
> index a72ae0b29f6e..f853f97bb882 100644
> --- a/tools/testing/selftests/bpf/prog_tests/percpu_alloc.c
> +++ b/tools/testing/selftests/bpf/prog_tests/percpu_alloc.c

[ ... ]

> @@ -350,6 +350,80 @@ static void test_lru_percpu_hash_cpu_flag(void)
>  	test_percpu_map_cpu_flag(BPF_MAP_TYPE_LRU_PERCPU_HASH);
>  }
>
> +/*
> + * A BPF_F_CPU update that creates an element must zero the value on the other
> + * cpus, rather than leave them holding whatever the recycled element last
> + * contained. max_entries is 1 so the second key can only reuse the element
> + * the first one released.
> + */
> +static void test_percpu_map_cpu_flag_create(enum bpf_map_type map_type, __u32 map_flags)
> +{
> +	LIBBPF_OPTS(bpf_map_create_opts, opts, .map_flags = map_flags);
> +	const u32 stale = 0xDEADC0DE, fresh = 0xC0FFEE;
> +	int nr_cpus, cpu, map_fd, err, key;
> +	u32 value;
> +	u64 flags;
> +
> +	nr_cpus = libbpf_num_possible_cpus();
> +	if (!ASSERT_GT(nr_cpus, 0, "libbpf_num_possible_cpus"))
> +		return;
> +
> +	if (nr_cpus < 2) {
> +		test__skip();
> +		return;
> +	}
> +
> +	map_fd = bpf_map_create(map_type, "cpu_flag_create", sizeof(key), sizeof(value), 1, &opts);
> +	if (!ASSERT_GE(map_fd, 0, "bpf_map_create"))
> +		return;
> +
> +	key = 1;
> +	value = stale;
> +	err = bpf_map_update_elem(map_fd, &key, &value, BPF_F_ALL_CPUS);
> +	if (!ASSERT_OK(err, "bpf_map_update_elem all_cpus"))
> +		goto out;
> +
> +	err = bpf_map_delete_elem(map_fd, &key);
> +	if (!ASSERT_OK(err, "bpf_map_delete_elem"))
> +		goto out;
> +
> +	key = 2;
> +	value = fresh;
> +	flags = BPF_F_CPU;
> +	err = bpf_map_update_elem(map_fd, &key, &value, flags);
> +	if (!ASSERT_OK(err, "bpf_map_update_elem specified cpu"))
> +		goto out;
> +
> +	for (cpu = 0; cpu < nr_cpus; cpu++) {
> +		value = 0;
> +		flags = (u64)cpu << 32 | BPF_F_CPU;
> +		err = bpf_map_lookup_elem_flags(map_fd, &key, &value, flags);
> +		if (!ASSERT_OK(err, "bpf_map_lookup_elem_flags specified cpu"))
> +			goto out;
> +		if (!ASSERT_EQ(value, cpu ? 0 : fresh, "value on specified cpu"))
> +			goto out;
> +	}
> +
> +out:
> +	close(map_fd);
> +}
> +
> +static void test_percpu_hash_cpu_flag_create(void)
> +{
> +	test_percpu_map_cpu_flag_create(BPF_MAP_TYPE_PERCPU_HASH, 0);
> +}
> +
> +static void test_percpu_hash_cpu_flag_create_malloc(void)
> +{
> +	test_percpu_map_cpu_flag_create(BPF_MAP_TYPE_PERCPU_HASH, BPF_F_NO_PREALLOC);
> +}

Can this test give false negatives on the BPF_F_NO_PREALLOC variant?

For preallocated maps, the comment's reasoning holds: max_entries 1 means
there's a single struct htab_elem whose per-cpu area is allocated once and
never freed. When the test deletes key 1, the element goes back to the
freelist with stale 0xDEADC0DE still in its per-cpu area. Creating key 2
gets that same element back, so if pcpu_init_value() is broken, cpu 0 will
see 0xC0FFEE but other CPUs will still see 0xDEADC0DE.

With BPF_F_NO_PREALLOC, the per-cpu area uses bpf_mem_alloc:

kernel/bpf/hashtab.c:htab_elem_free()
    bpf_mem_cache_free(&htab->pcpu_ma, l->ptr_to_pptr)

which does unit_free() and pushes to the *current* CPU's free list. The
subsequent create calls bpf_mem_cache_alloc(), which does unit_alloc() and
pops from whatever CPU the second syscall runs on.

If the thread migrated between the delete and the create, unit_alloc() gets
a prefilled zero object from __alloc_percpu_gfp() rather than the
just-freed stale one, so the test passes even on a broken kernel. Only when
both syscalls run on the same CPU does the LIFO allocator return the stale
object and trigger the assertion.

Should this arm either pin the thread with sched_setaffinity() around the
delete/create pair, or loop over all CPUs to make the result deterministic?

> +
> +static void test_lru_percpu_hash_cpu_flag_create(void)
> +{
> +	/* lru without prealloc is -ENOTSUPP, so there is no malloc variant */
> +	test_percpu_map_cpu_flag_create(BPF_MAP_TYPE_LRU_PERCPU_HASH, 0);
> +}

[ ... ]


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