Re: [PATCH bpf v4 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:
- [Medium] Massive memory leak and file descriptor leak in the test runner on unpatched kernels.
--

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

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

This commit adds a regression test to verify that queue/stack maps whose
element storage overflows the u32 head/tail index multiplication are rejected
at creation time. It also checks that max_entries == U32_MAX is rejected.

> 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..efe808eedd9a8 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 +103,43 @@ 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;
> +
> +	/*
> +	 * 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);
> +	ASSERT_EQ(fd, -E2BIG, "queue_oversize");
> +
> +	/*
> +	 * 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);

[Severity: Medium]
If this test runs on an unpatched kernel, the previous bpf_map_create()
call might unexpectedly succeed and return a valid file descriptor.

Because ASSERT_EQ() doesn't abort the test upon failure, won't this next
bpf_map_create() call overwrite the 'fd' variable without closing the previous
one?

This could leak the map file descriptor and the large amount of memory
(up to 8GB) allocated by the map, potentially triggering the OOM killer and
crashing the test environment.

Should we check if 'fd >= 0' and close it after the ASSERT_EQ() for each of
these expected-to-fail allocations?

> +	ASSERT_EQ(fd, -E2BIG, "queue_u32max");
> +
> +	fd = bpf_map_create(BPF_MAP_TYPE_STACK, NULL, 0, big_value, 8192, &opts);
> +	ASSERT_EQ(fd, -E2BIG, "stack_oversize");
> +
> +	/* A normal-sized map must still be created successfully. */
> +	fd = bpf_map_create(BPF_MAP_TYPE_QUEUE, NULL, 0, 64, 100, &opts);
> +	ASSERT_GE(fd, 0, "queue_normal");
> +	if (fd >= 0)
> +		close(fd);
> +}

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