Re: [PATCH bpf v2 2/2] selftests/bpf: Add regression test for queue/stack map size limit

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Missing file descriptor cleanup on the test failure path for the first bpf_map_create call.
--

--- Patch [2]: [PATCH bpf v2 2/2] selftests/bpf: Add regression test for queue/stack map size limit ---
commit 25c3a9fe96d53115b495649d174f217a4729daca
Author: Yuan Chen <[email protected]>

selftests/bpf: Add regression test for queue/stack map size limit

This commit adds regression tests to ensure that queue and stack maps whose
element storage would overflow the u32 head/tail index multiplication or
wrap the u32 capacity counter are properly rejected at creation time.

> diff --git a/tools/testing/selftests/bpf/prog_tests/queue_stack_map.c b/tools/testing/selftests/bpf/prog_tests/queue_stack_map.c
> index 41441325e1794..8ab07ea777759 100644
> --- a/tools/testing/selftests/bpf/prog_tests/queue_stack_map.c
> +++ b/tools/testing/selftests/bpf/prog_tests/queue_stack_map.c
[ ... ]
> @@ -101,8 +101,53 @@ static void test_queue_stack_map_by_type(int type)
>  	bpf_object__close(obj);
>  }
>  
> +static void test_queue_stack_map_alloc_check(void)
> +{
> +	LIBBPF_OPTS(bpf_map_create_opts, opts);
> +	const __u32 big_value = 1 << 20; /* 1MB */
> +	int fd, saved_errno;
> +
> +	/*
> +	 * Regression test for the u32 index overflow in queue/stack maps:
> +	 * a map whose element storage (max_entries * value_size) exceeds
> +	 * U32_MAX bytes must be rejected at creation time, otherwise the
> +	 * u32 head/tail index multiplication wraps and push/peek/pop
> +	 * address the wrong element. 8192 * 1MB = 8GB > U32_MAX.
> +	 */
> +	fd = bpf_map_create(BPF_MAP_TYPE_QUEUE, NULL, 0, big_value, 8192, &opts);
> +	saved_errno = errno;
> +	ASSERT_LT(fd, 0, "queue_oversize_fd");
> +	ASSERT_EQ(saved_errno, E2BIG, "queue_oversize_errno");

[Severity: Low]
Should this test block include a close() call in case bpf_map_create()
unexpectedly succeeds?

If the kernel fails to reject the oversized map, fd will be a non-negative
value. Since ASSERT_LT logs the test failure but doesn't abort execution,
fd is overwritten in the very next bpf_map_create() call without being
cleaned up.

The subsequent tests in this function include a check to close the file
descriptor on the failure path:

> +
> +	/*
> +	 * max_entries == U32_MAX would make the u32 capacity counter
> +	 * qs->size (max_entries + 1) wrap to 0, permanently breaking the
> +	 * map, so it must be rejected as well.
> +	 */
> +	fd = bpf_map_create(BPF_MAP_TYPE_QUEUE, NULL, 0, 1, U32_MAX, &opts);
> +	saved_errno = errno;
> +	ASSERT_LT(fd, 0, "queue_u32max_fd");
> +	ASSERT_EQ(saved_errno, E2BIG, "queue_u32max_errno");
> +	if (fd >= 0)
> +		close(fd);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2