Re: [PATCH net-next 07/15] selftests/bpf: Test metadata passing to cgroup_skb and sk_filter hooks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] bpf: unconditional bpf_tc_hook_destroy on error path flushes host TC filters
--

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

selftests/bpf: Test metadata passing to cgroup_skb and sk_filter hooks

Extend metadata cross-hook coverage to UDP readers: attach a TC ingress
program on loopback that tags the skb, send a UDP datagram over loopback,
and read the stashed metadata back from a cgroup_skb/ingress program and
from a socket filter attached with SO_ATTACH_BPF.

> 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
> index 1155e868ffbd7..25a16daf5fdf3 100644
> --- a/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
[ ... ]
> +static void test_skb_trace__udp(struct test_xdp_meta *skel, const char *name,
> +				enum udp_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);
> +	struct bpf_link *reader_link = NULL;
> +	struct bpf_link *put_link = NULL;
> +	struct netns_obj *ns = NULL;
[ ... ]
> +	ns = netns_new(name, true);
> +	if (!ASSERT_OK_PTR(ns, "netns_new"))
> +		goto cleanup;

[Severity: Medium]
If netns_new() fails here, the process remains in the original network
namespace and jumps to the cleanup label.

Could this lead to unintended side effects when bpf_tc_hook_destroy() is called
below?

> +
> +	cgroup_fd = test__join_cgroup("/skb_ext_udp");
> +	if (!ASSERT_GE(cgroup_fd, 0, "join_cgroup"))
> +		goto cleanup;
[ ... ]
> +cleanup:
> +	bpf_link__destroy(reader_link);
> +	bpf_link__destroy(put_link);
> +	bpf_tc_hook_destroy(&tc_hook);

[Severity: Medium]
Since tc_hook is statically initialized with ifindex = 1 (loopback), and it
is destroyed unconditionally on the error path, could this unintentionally
delete the clsact qdiscs and TC filters on the original (host) namespace's
loopback interface if netns_new() fails?

Would it be safer to only destroy the hook if it was successfully created?

> +	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=7