Re: [PATCH net-next 08/15] selftests/bpf: Test passing metadata to sock_ops and LSM hooks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] bpf: flawed TCP free_count wait logic causes test race
- [Medium] bpf: unconditional bpf_tc_hook_destroy deletes root qdisc on error
--

commit b0a956d5cfe493a1e0f2beca58e7a9622aea9f34
Author: Jakub Sitnicki <jakub@cloudflare.com>

selftests/bpf: Test passing metadata to sock_ops and LSM hooks

This commit extends metadata cross-hook coverage to the TCP ingress path by
attaching a TC ingress program on loopback that tags the skb and reading the
stashed metadata back from a sock_ops program and an LSM hook.

> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c b/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
> --- a/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
> @@ -1162,6 +1162,112 @@ static void test_skb_trace__udp(struct test_xdp_meta *skel, const char *name,

[ ... ]

> +static void test_skb_trace__tcp_ingress(struct test_xdp_meta *skel, const char *name,
> +					enum tcp_reader_type reader)
> +{
> +	LIBBPF_OPTS(bpf_tc_hook, tc_hook,
> +		    .ifindex = 1 /* IFINDEX_LO */,
> +		    .attach_point = BPF_TC_INGRESS);
> +	LIBBPF_OPTS(bpf_tc_opts, tc_opts, .handle = 1, .priority = 1);

[ ... ]

> +	ns = netns_new(name, true);
> +	if (!ASSERT_OK_PTR(ns, "netns_new"))
> +		goto cleanup;
> +
> +	cgroup_fd = test__join_cgroup("/skb_ext_tcp");
> +	if (!ASSERT_GE(cgroup_fd, 0, "join_cgroup"))
> +		goto cleanup;

[ ... ]

> +	/* Poll every 10 msec up to 1 sec */
> +	for (i = 0; i < 100 && !skel->bss->test_pass; i++)
> +		usleep(10000);
> +	ASSERT_TRUE(skel->bss->test_pass, "test_pass");
> +
> +	/* Skb is freed after RX delivery; wait for free tp */
> +	for (i = 0; i < 100 && !skel->bss->free_count; i++)
> +		usleep(10000);
> +	ASSERT_GE(skel->bss->free_count, 1, "free_count");

[Severity: Medium]
Can this wait loop exit too early and cause a test flake? 

Since tc_meta_write_port() unconditionally tags all TCP packets matching the
target port, the SYN and SYN-ACK handshake packets will also increment
free_count when they are freed. 

The wait loop here will see a non-zero free_count from the handshake and
terminate immediately, bypassing the wait for the final ACK packet. 

Does this leave a window where the test proceeds to check the map state
while the final ACK packet might still be held by SoftIRQ?

> +
> +	assert_meta_stash_empty(skel->maps.meta_map);
> +
> +cleanup:
> +	if (conn_fd >= 0)
> +		close(conn_fd);
> +	if (client_fd >= 0)
> +		close(client_fd);
> +	bpf_link__destroy(reader_link);
> +	bpf_link__destroy(put_link);
> +	bpf_tc_hook_destroy(&tc_hook);

[Severity: Medium]
If netns_new() or test__join_cgroup() fails earlier in the function, the
execution jumps to the cleanup label while still in the original root network
namespace.

Since bpf_tc_hook_destroy(&tc_hook) is called unconditionally and tc_hook
defaults to ifindex = 1, will this blindly issue a netlink command to delete
the clsact qdisc on the root namespace's loopback interface? 

Could this destroy shared test state and cause cascading failures in other
concurrently running tests?

> +	if (server_fd >= 0)
> +		close(server_fd);
> +	if (cgroup_fd >= 0)
> +		close(cgroup_fd);
> +	netns_free(ns);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928-bpf-meta-gated-tracepoints-v1-0-844dbf3e1edf@cloudflare.com?part=8