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

["Emil Tsalapatis" <emil@etsalapatis.com>]

On Tue Jun 23, 2026 at 3:00 PM EDT, sashiko-bot wrote:
> 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();
>> +}
>

I think these issues are borderline cosmetic but valid.

> [ ... ]
>
>> 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.
>

AFAICT this is actually not a problem for this specific test, because we
fix it in patch 1/2. We have already added the socket to the sockmap so
udp_bpf_update_proto should have triggered. It looks like Sashiko missed
this?

Specifically for UDP, I think the code is fine.

Reviewed-by: Emil Tsalapatis <emil@etsalapatis.com>

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