Re: [PATCH bpf-next 15/16] selftests/bpf: Add test for mixed arena percpu object

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests/bpf: unreachable err > 0 branch and silent test bypass in test_arena_percpu_mixed()
--

commit df1443ad7be8c02167841960847603cc924444f9
Author: Xu Kuohai <xukuohai@huawei.com>

selftests/bpf: Add test for mixed arena percpu object

This commit adds a selftest subtest and BPF program to exercise mixing
per-CPU arena variables and a regular global arena variable within the
same BPF arena map.

> diff --git a/tools/testing/selftests/bpf/prog_tests/arena_percpu.c b/tools/testing/selftests/bpf/prog_tests/arena_percpu.c
> index 27d4721c7ae9a..b450be5f2b36c 100644
> --- a/tools/testing/selftests/bpf/prog_tests/arena_percpu.c
> +++ b/tools/testing/selftests/bpf/prog_tests/arena_percpu.c

[ ... ]

> @@ -137,6 +138,99 @@ static void test_arena_percpu_only(void)
>  	restore_affinity();
>  }
>  
> +static void test_arena_percpu_mixed(void)
> +{
> +	struct arena_percpu_mixed *skel;
> +	struct bpf_map *arena, *bss_map;
> +	bool *possible = NULL, *online = NULL, *written = NULL;
> +	int possible_cpus, online_cpus, nr_written = 0, cpu, err;

[ ... ]

> +	for (cpu = 0; cpu < online_cpus; cpu++) {
> +		if (!online[cpu] || cpu >= CPU_SETSIZE || !CPU_ISSET(cpu, &affinity_save))
> +			continue;
> +
> +		err = run_on_cpu(cpu, skel->progs.arena_percpu_mixed_write, "write");
> +		if (err < 0)
> +			goto out;
> +		if (err > 0)
> +			continue;
> +		written[cpu] = true;
> +		nr_written++;
> +	}
> +	if (!ASSERT_GE(nr_written, 1, "cpus_exercised"))
> +		goto out;

[Severity: Medium]
Can the err > 0 branch ever be reached here? Looking at run_on_cpu() in
tools/testing/selftests/bpf/prog_tests/arena_percpu.c:

static int run_on_cpu(int cpu, struct bpf_program *prog, const char *name)
{
	LIBBPF_OPTS(bpf_test_run_opts, opts);
	int err;

	if (pin_to(cpu))
		return -1;

	err = bpf_prog_test_run_opts(bpf_program__fd(prog), &opts);

	if (!ASSERT_OK(err, name))
		return -1;
	if (!ASSERT_OK(opts.retval, name))
		return -1;
	return 0;
}

run_on_cpu() only returns -1 or 0, so the err > 0 check is dead code.

Furthermore, if pin_to(cpu) fails (for example, if a CPU cannot be pinned or
goes offline), run_on_cpu() returns -1 without recording an assertion
failure. In test_arena_percpu_mixed(), that triggers goto out, bypassing
ASSERT_GE(nr_written, 1, "cpus_exercised") and all subsequent arena checks
while allowing the subtest to pass silently.

Was run_on_cpu() intended to return a positive value when pin_to(cpu) fails
so the loop can skip that CPU and still enforce nr_written >= 1?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792503001.git.xukuohai@huawei.com?part=15