Re: [PATCH bpf-next v2 4/4] selftests/bpf: Add tests for memory usage for arena

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Incorrect use of ASSERT_OK_PTR to check for a NULL pointer read from BPF userspace memory, breaking intended error handling.
--

--- Patch [4]: [PATCH bpf-next v2 4/4] selftests/bpf: Add tests for memory usage for arena ---
commit b8bd79585d9f7cbf2a83d5381d0df2c316e65ed7
Author: Jiayuan Chen <jiayuan.chen@linux.dev>

selftests/bpf: Add tests for memory usage for arena

This patch adds a selftest to verify memory usage tracking for BPF arenas.
It allocates and frees arena pages both from BPF and user-space and checks 
the map's memlock value via fdinfo to ensure it tracks populated pages.

> diff --git a/tools/testing/selftests/bpf/prog_tests/arena_mem_usage.c b/tools/testing/selftests/bpf/prog_tests/arena_mem_usage.c
> new file mode 100644
> index 0000000000000..14c2d1a1d673c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/arena_mem_usage.c

[ ... ]

> +void serial_test_arena_mem_usage(void)
> +{
> +	struct arena_mem_usage *skel;
> +	const long ps = PAGE_SIZE;
> +	char *base;
> +	size_t sz;
> +	int fd, i;
> +
> +	skel = arena_mem_usage__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "open_load"))
> +		return;
> +	fd = bpf_map__fd(skel->maps.arena);
> +
> +	/* Fresh arena: no data pages, and the scratch page is not counted. */
> +	ASSERT_EQ(map_memlock(fd), 0, "initial");
> +
> +	/* BPF-side allocation of 17 pages. */
> +	skel->bss->alloc_cnt = 17;
> +	if (run(skel->progs.alloc, "alloc"))
> +		goto out;
> +	/*
> +	 * A NULL ptr means bpf_arena_alloc_pages() itself failed (e.g. the host
> +	 * is under memory pressure), not a miscount -- flag it distinctly so a
> +	 * red CI run is not mistaken for a counting bug.
> +	 */
> +	if (!ASSERT_OK_PTR(skel->bss->ptr, "arena_alloc_pages"))

[Severity: Medium]
Will this check actually catch a NULL pointer when bpf_arena_alloc_pages()
fails?

When skel->bss->ptr is NULL, ASSERT_OK_PTR() evaluates libbpf_get_error(NULL),
which returns -errno. Since the userspace errno is likely 0 following a 
successful bpf_prog_test_run_opts() ioctl call, ASSERT_OK_PTR silently
evaluates to true (success).

This would cause the goto out to be skipped, and the test to proceed to
evaluate ASSERT_EQ(map_memlock(fd), 17 * ps). This subsequent check would
then fail, emitting the exact counting bug error the comment mentions trying
to avoid.

Should this use a different macro, like ASSERT_NEQ(skel->bss->ptr, NULL), to
correctly verify the userspace pointer?

> +		goto out;
> +	ASSERT_EQ(map_memlock(fd), 17 * ps, "after_alloc");

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717114117.350851-1-jiayuan.chen@linux.dev?part=4