Re: [PATCH bpf-next v2 2/2] selftests/bpf: Cover recursive bpftool map dumps

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Lowering RLIMIT_NOFILE without closing inherited FDs from the test runner creates brittle test execution.
- [Low] Multi-line comments do not follow the BPF subsystem standard (opening `/*` on its own line).
--

commit bfea95162d84a988e16f2cd61621377edfd83849
Author: Tianyi Chen <hi@tychen.cc>

selftests/bpf: Cover recursive bpftool map dumps

Exercise recursive map dumping for array-of-maps and hash-of-maps,
including shared inner maps, empty outer and inner maps, BTF-formatted
values and multiple selected roots.

> diff --git a/tools/testing/selftests/bpf/prog_tests/bpftool_map_dump.c b/tools/testing/selftests/bpf/prog_tests/bpftool_map_dump.c
> index 0000000000000..330a3344770d1
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/bpftool_map_dump.c
> @@ -0,0 +1,419 @@

[ ... ]

> +	/* Compare the complete JSON document: a flat array with the root first,
> +	 * one copy of the shared inner map, and unchanged entry representations.
> +	 */

[Severity: Low]
Does this follow the BPF subsystem multi-line comment style? The opening
/* should be on its own line:

/*
 * Compare the complete JSON document...

[ ... ]

> +	/* Select the first inner map and both outers as roots. The other inner
> +	 * map must be appended after all three roots, even though it is found
> +	 * while dumping the first outer. A process-specific name avoids other
> +	 * tests' maps joining the selection.
> +	 */

[Severity: Low]
Does this follow the BPF subsystem multi-line comment style? The opening
/* should be on its own line.

[ ... ]

> +	/* Every key is enumerable, but PERF_EVENT_ARRAY lookup returns
> +	 * ENOTSUPP (the kernel-internal errno). Check both entries so an
> +	 * early exit on the first lookup failure cannot pass.
> +	 */

[Severity: Low]
Does this follow the BPF subsystem multi-line comment style? The opening
/* should be on its own line.

[ ... ]

> +	/* Create all fixtures before lowering the limit, and keep the test
> +	 * runner's limit unchanged. Retaining every discovered FD would exceed
> +	 * this limit before the recursive dump could visit all inner maps.
> +	 */

[Severity: Low]
Does this follow the BPF subsystem multi-line comment style? The opening
/* should be on its own line.

> +	pid = fork();
> +	if (!ASSERT_GE(pid, 0, "fork"))
> +		goto out;
> +	if (!pid) {
> +		close(outer_fd);
> +		if (setrlimit(RLIMIT_NOFILE, &limit))
> +			_exit(1);
> +		snprintf(command, sizeof(command), "%s -r map dump id %u",
> +			 json ? "-j" : "", root_id);
> +		if (get_bpftool_command_output(command, output, 65535))
> +			_exit(2);

[Severity: Medium]
Will this sequence cause sporadic test failures if the parent test runner
already has 32 or more file descriptors open?

Since the child process inherits all file descriptors from the parent,
lowering RLIMIT_NOFILE to 32 here leaves no available file descriptor
numbers if the parent already holds >= 32 descriptors (which is common
when running the full test suite).

Later in the block, get_bpftool_command_output() relies on popen(), which
needs to allocate new descriptors for a pipe. If no descriptors are
available, opening the pipe will fail with EMFILE, aborting the test
prematurely.

Should the child process explicitly close all inherited file descriptors
(e.g., using close_range(3, ~0U, 0)) before calling setrlimit?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/178870998583.979169.16526173782819097354@tychen.cc?part=2