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

[bot+bpf-ci@kernel.org]

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

> selftests/bpf: Add test for arena percpu bounds

TL;DR: Nits only in the arena percpu bounds selftest: a redundant insns[2].off
= 0 assignment, and the repeated bpf_prog_load and log-check code could share
a helper.

> diff --git a/tools/testing/selftests/bpf/prog_tests/arena_percpu.c b/tools/testing/selftests/bpf/prog_tests/arena_percpu.c
> --- a/tools/testing/selftests/bpf/prog_tests/arena_percpu.c
> +++ b/tools/testing/selftests/bpf/prog_tests/arena_percpu.c

[ ... ]

> +/* check out of range pseudo instructions and accesses */
> +static void test_arena_percpu_percpu_bounds(void)
> +{

[ ... ]

> +	/* a reference at the start of the chunk loads */
> +	insns[1].imm = page_sz;
> +	prog_fd = bpf_prog_load(BPF_PROG_TYPE_RAW_TRACEPOINT,
> +				"arena_percpu_ref", "GPL", insns,
> +				ARRAY_SIZE(insns), &prog_opts);
> +	if (!ASSERT_GE(prog_fd, 0, "in_range_load"))
> +		goto out;
> +	close(prog_fd);
> +
> +	/* the first u64 below the chunk and one past its end are rejected */
> +	bad_offs[0] = page_sz - 8;
> +	bad_offs[1] = page_sz + page_sz;
> +
> +	for (i = 0; i < ARRAY_SIZE(bad_offs); i++) {
> +		insns[1].imm = bad_offs[i];
> +		prog_fd = bpf_prog_load(BPF_PROG_TYPE_RAW_TRACEPOINT,
> +					"arena_percpu_ref", "GPL", insns,
> +					ARRAY_SIZE(insns), &prog_opts);
> +		if (!ASSERT_LT(prog_fd, 0, "out_of_range_load"))
> +			close(prog_fd);
> +		snprintf(expected, sizeof(expected),
> +			 "per-cpu arena reference at offset %u "
> +			 "outside cpu 0's chunk", bad_offs[i]);
> +		ASSERT_HAS_SUBSTR(log_buf, expected, "out_of_range_log");
> +	}
> +
> +	/* the last u64 of the chunk is in range and loads */
> +	insns[1].imm = page_sz + page_sz - 8;
> +	insns[2].off = 0;

This isn't a bug, but is this insns[2].off = 0 assignment needed?  The field
is still 0 from the initializer at this point.

> +	prog_fd = bpf_prog_load(BPF_PROG_TYPE_RAW_TRACEPOINT,
> +				"arena_percpu_ref", "GPL", insns,
> +				ARRAY_SIZE(insns), &prog_opts);
> +	if (!ASSERT_GE(prog_fd, 0, "last_u64_load"))
> +		goto out;
> +	close(prog_fd);
> +
> +	/* one u64 past the chunk end is rejected */
> +	insns[2].off = 8;
> +	prog_fd = bpf_prog_load(BPF_PROG_TYPE_RAW_TRACEPOINT,
> +				"arena_percpu_ref", "GPL", insns,
> +				ARRAY_SIZE(insns), &prog_opts);
> +	if (!ASSERT_LT(prog_fd, 0, "out_of_range_access"))
> +		close(prog_fd);
> +	snprintf(expected, sizeof(expected),
> +		 "per-cpu arena access at offset %d size 8 "
> +		 "outside cpu 0's chunk", (int)page_sz);
> +	ASSERT_HAS_SUBSTR(log_buf, expected, "out_of_range_access_log");
> +
> +	/* an access below the chunk start is rejected */
> +	insns[1].imm = page_sz;
> +	insns[2].off = -8;
> +	prog_fd = bpf_prog_load(BPF_PROG_TYPE_RAW_TRACEPOINT,
> +				"arena_percpu_ref", "GPL", insns,
> +				ARRAY_SIZE(insns), &prog_opts);
> +	if (!ASSERT_LT(prog_fd, 0, "below_chunk_access"))
> +		close(prog_fd);
> +	snprintf(expected, sizeof(expected),
> +		 "per-cpu arena access at offset -8 size 8 "
> +		 "outside cpu 0's chunk");
> +	ASSERT_HAS_SUBSTR(log_buf, expected, "below_chunk_access_log");

This isn't a bug, but could the five identical bpf_prog_load() calls and the
repeated reject/log checks share a small helper?

Only insns[1].imm and insns[2].off differ between the cases, and the
ASSERT_LT + close, snprintf into expected, ASSERT_HAS_SUBSTR sequence is
repeated three times.

A helper taking the reference offset and the access offset, and returning
the prog fd or checking the expected log, would make each case a single
line, like run_on_cpu() and check_percpu_value() do elsewhere in this file.

[ ... ]


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