Re: [PATCH bpf 2/2] selftests/bpf: Add test for UDP sock leak on sockmap lookup-bind-release

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] The cgroup file descriptor `cg` is leaked because it is never closed in `test_sockmap_lookup_bind_release()`.
- [Low] The `struct sockaddr_in sa` is passed to the kernel via `xconnect` with uninitialized bytes in its `sin_zero` padding.

Pre-existing issues:
- [High] Calling `bpf_bind()` between `bpf_map_lookup_elem()` and `bpf_sk_release()` on a UDP socket mutates the socket's `SOCK_RCU_FREE` state, causing `bpf_sk_release()` to silently skip decrementing the reference count.
--

commit f32ef88a53a5c519bf4b22a337fa782c27f0ecda
Author: Michal Luczaj <mhal@rbox.co>

selftests/bpf: Add test for UDP sock leak on sockmap lookup-bind-release

This patch adds a selftest to reproduce a UDP socket leak. It sets up a cgroup,
attaches a connect4 BPF program, and triggers a sequence of socket operations
(lookup, bind, release) that leaks the socket on an unpatched kernel.

> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> index cb3229711f93a..11972ffdb16ed 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c

[ ... ]

> @@ -1373,6 +1375,52 @@ static void test_sockmap_multi_channels(int sotype)
>  	test_sockmap_pass_prog__destroy(skel);
>  }
>  
> +#define LOOKUP_BIND_RELEASE_CG	"/sockmap_lookup-bind-release"
> +#define LOOKUP_BIND_RELEASE_REP	64
> +
> +static void test_sockmap_lookup_bind_release(void)
> +{
> +	struct test_sockmap_lookup_bind_release *skel;
> +	struct sockaddr_in sa;
> +	int cg, i;
> +
> +	cg = cgroup_setup_and_join(LOOKUP_BIND_RELEASE_CG);

[Severity: Medium]
Does this leak the cgroup file descriptor? 

The fd is acquired here and stored in cg, but close(cg) doesn't appear to be
called on either the success or error paths before the function returns. The
cleanup_cgroup_environment() function cleans up directories but doesn't close
the test's held file descriptors.

> +	if (!ASSERT_OK_FD(cg, "cgroup_setup_and_join"))
> +		return;
> +
> +	skel = test_sockmap_lookup_bind_release__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "open_and_load"))
> +		goto cleanup;
> +
> +	skel->links.connect = bpf_program__attach_cgroup(skel->progs.connect, cg);
> +	if (!ASSERT_OK_PTR(skel->links.connect, "attach_cgroup"))
> +		goto destroy;
> +
> +	sa.sin_family = AF_INET;
> +	sa.sin_port = bpf_htons(1234);
> +	sa.sin_addr.s_addr = bpf_htonl(INADDR_LOOPBACK);

[Severity: Low]
Does this leave the sin_zero padding of the sa structure uninitialized?

It looks like this partially initializes the struct on the stack.

[ ... ]

> +		if (xbpf_map_update_elem(bpf_map__fd(skel->maps.sockmap), &u32(0),
> +					 &sk, BPF_ANY))
> +			break;
> +
> +		if (xconnect(sk, (struct sockaddr *)&sa, sizeof(sa)))

When sa is passed to xconnect() here, can it leak the uninitialized padding
bytes from the userspace stack to the kernel? 

Would it be better to fully initialize it using "struct sockaddr_in sa = {};"
or explicitly zero it?

> +			break;
> +	}
> +
> +destroy:
> +	test_sockmap_lookup_bind_release__destroy(skel);
> +cleanup:
> +	cleanup_cgroup_environment();
> +}

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/test_sockmap_lookup_bind_release.c b/tools/testing/selftests/bpf/progs/test_sockmap_lookup_bind_release.c
> new file mode 100644
> index 0000000000000..cc77b193893bd
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_sockmap_lookup_bind_release.c
> @@ -0,0 +1,37 @@

[ ... ]

> +SEC("cgroup/connect4")
> +int connect(struct bpf_sock_addr *ctx)
> +{
> +	struct bpf_sock *sk;
> +	int ret = SK_DROP;
> +
> +	sk = bpf_map_lookup_elem(&sockmap, &(int){0});
> +	if (sk) {
> +		if (sk == ctx->sk) {
> +			struct sockaddr_in sa = {
> +				.sin_family = ctx->user_family,
> +				.sin_port = ctx->user_port,
> +				.sin_addr.s_addr = ctx->user_ip4
> +			};
> +
> +			ret = !bpf_bind(ctx, (struct sockaddr *)&sa, sizeof(sa));
> +		}
> +
> +		bpf_sk_release(sk);

[Severity: High]
This is a pre-existing issue, but does calling bpf_bind() here mutate the
SOCK_RCU_FREE state and cause bpf_sk_release() to skip decrementing the
reference count?

If bpf_map_lookup_elem() looks up a newly created UDP socket, SOCK_RCU_FREE
is not yet set, so sk_is_refcounted() evaluates to true and the reference
count is incremented.

Then bpf_bind() resolves to udp_v4_get_port(), which dynamically sets the
SOCK_RCU_FREE flag on the socket.

When bpf_sk_release() is called, it evaluates sk_is_refcounted() again. Because
SOCK_RCU_FREE is now set, it evaluates to false, causing bpf_sk_release() to
skip the sock_gen_put() call and permanently leak the socket reference.

> +	}
> +
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623-sockmap-lookup-udp-leak-v1-0-05804f9308e4@rbox.co?part=2