Re: [PATCH net-next 12/15] selftests/bpf: Test passing metadata to/from LWT hooks

[sashiko-bot@kernel.org]

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

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

selftests/bpf: Test passing metadata to/from LWT hooks

This commit adds cross-hook tests for the metadata stash covering all generic
LWT program types. It tests LWT ingress, egress, and transmit programs by
tagging the skb on loopback and reading the stashed metadata from TC ingress.

> 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 eb0d36e7a45d9..aea7a1c8a7552 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
> @@ -1638,6 +1638,118 @@ static void test_skb_trace__nf(struct test_xdp_meta *skel, const char *name)

[ ... ]

> +static void test_skb_trace__lwt(struct test_xdp_meta *skel, const char *name,
> +				struct bpf_program *lwt_prog,
> +				const char *encap_dir, bool writer)
> +{
> +	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);

[ ... ]

> +	unlink(LWT_EXT_PIN_PATH);
> +	ret = bpf_program__pin(lwt_prog, LWT_EXT_PIN_PATH);
> +	if (!ASSERT_OK(ret, "pin lwt"))
> +		goto cleanup;

[Severity: Medium]
Will this error path inadvertently flush the host's loopback TC filters?

Because bpf_tc_hook_destroy() is called in the cleanup block, and netns_new()
has not been called yet, jumping to cleanup here means bpf_tc_hook_destroy()
executes in the host's network namespace on ifindex 1 (loopback). This could
disrupt the host configuration or other parallel tests by flushing all ingress
filters on the host's loopback interface.

Should this jump to cleanup_prehook instead?

> +	pinned = true;
> +
> +	ns = netns_new(name, true);
> +	if (!ASSERT_OK_PTR(ns, "netns_new"))
> +		goto cleanup_prehook;

[ ... ]

> +cleanup:
> +	if (fd >= 0)
> +		close(fd);
> +	bpf_tc_hook_destroy(&tc_hook);
> +cleanup_prehook:
> +	bpf_link__destroy(put_link);
> +	if (server_fd >= 0)
> +		close(server_fd);
> +	netns_free(ns);
> +	if (pinned)
> +		unlink(LWT_EXT_PIN_PATH);
> +}

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