Re: [PATCH bpf-next v2 2/2] selftests/bpf: Check that bpftool map dump fails for socket storage

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] bpf: missing unlink of pinned map causes permanent test failure
--

commit 602b2c7f146f62e2714d566e5a1fb9912bee7425
Author: Maxim Skokov <skokovmaksimevg@gmail.com>

selftests/bpf: Check that bpftool map dump fails for socket storage

This commit adds a test to verify that bpftool fails to dump a socket
storage map. It creates a map, attaches storage to a socket, pins it,
and checks various output formats.

> diff --git a/tools/testing/selftests/bpf/prog_tests/bpftool_map_dump.c b/tools/testing/selftests/bpf/prog_tests/bpftool_map_dump.c
> new file mode 100644
> index 0000000000000..0a0fa8926b239
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/bpftool_map_dump.c
> @@ -0,0 +1,88 @@
[ ... ]
> +void test_bpftool_map_dump(void)
> +{
> +	int map_fd, sk_fd = -1, value = 42, ret;
> +	char output[1024] = {};
> +
> +	map_fd = create_sk_storage_map();
> +	if (map_fd < 0)
> +		return;
> +
> +	sk_fd = socket(AF_INET, SOCK_STREAM, 0);
> +	if (!ASSERT_OK_FD(sk_fd, "socket"))
> +		goto out;
> +	if (!ASSERT_OK(bpf_map_update_elem(map_fd, &sk_fd, &value, BPF_NOEXIST),
> +		       "add socket storage"))
> +		goto out;
> +	if (!ASSERT_OK(bpf_obj_pin(map_fd, PIN_PATH), "pin map"))
> +		goto out;

[Severity: Medium]
Looking at test_bpftool_map_dump() in
tools/testing/selftests/bpf/prog_tests/bpftool_map_dump.c, will this cause a
permanent test failure on subsequent runs if the pin operation fails, or if the
test is interrupted early?

If bpf_obj_pin fails and we jump to the out label, the test skips over the
unlink cleanup. The map then remains pinned, which means the next run will
encounter EEXIST when it tries to pin the map again.

[ ... ]
> +	ASSERT_HAS_SUBSTR(output, "can't dump sk_storage map",
> +			  "plain dump explains why on stderr");
> +
> +	unlink(PIN_PATH);
> +out:
> +	if (sk_fd >= 0)
> +		close(sk_fd);
> +	close(map_fd);
> +}

[Severity: Medium]
Since the out label is positioned after unlink(PIN_PATH), the cleanup is
bypassed on failure paths.

Should the unlink be moved after the out label so the cleanup is always
executed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260925162031.27259-1-skokovmaksimevg@gmail.com?part=2