Re: [PATCH net-next 11/14] selftests/bpf: Test skb_ext from LWT in, out, and xmit hooks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The error check for `get_socket_local_port()` is rendered completely ineffective due to assigning the result to an unsigned 16-bit integer before comparison.
- [Medium] Unconditional execution of `bpf_tc_hook_destroy(&tc_hook)` on the error path can delete the `clsact` qdisc from the base network namespace.
- [Low] The multi-line comment for `test_skb_ext_lwt` violates the BPF subsystem comment style guidelines.
--

--- Patch [11]: [PATCH net-next 11/14] selftests/bpf: Test skb_ext from LWT in, out, and xmit hooks ---
commit 7845f94d2569f8018cf274a4a851280db3c38a3a
Author: Jakub Sitnicki <jakub@cloudflare.com>

selftests/bpf: Test skb_ext from LWT in, out, and xmit hooks

Add cross-hook tests for bpf_dynptr_from_skb_ext() covering all generic
LWT program types. For lwt_in, a TC ingress program writes skb_ext on
loopback and the LWT program reads it back. For lwt_out and lwt_xmit,
the LWT programs write skb_ext on the output path and a TC ingress
program reads it back after loopback.

> 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 82c57ea3957e3..7a7537ef80e75 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
> @@ -1303,6 +1303,95 @@ static void test_skb_ext_nf(struct test_xdp_meta *skel, const char *name)
>  	netns_free(ns);
>  }
>  
> +#define LWT_EXT_PIN_PATH "/sys/fs/bpf/skb_ext_lwt"
> +
> +/* Test skb_ext across LWT hooks on loopback.

[Severity: Low]
Does this violate the BPF subsystem multi-line comment style? The opening /*
usually needs to be on its own line before the text begins.

> + *
> + * @lwt_prog:  BPF program to pin and attach via ip route encap
> + * @encap_dir: "in", "out", or "xmit"
> + * @writer:    true if lwt_prog writes skb_ext (reader on TC ingress),
> + *             false if lwt_prog reads skb_ext (writer on TC ingress)
> + */
> +static void test_skb_ext_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);

[ ... ]

> +	__be16 port;
> +	int ret;
> +
> +	unlink(LWT_EXT_PIN_PATH);

[ ... ]

> +	port = get_socket_local_port(server_fd);
> +	if (!ASSERT_GE(port, 0, "get_port"))
> +		goto cleanup;

[Severity: Medium]
Will this error check work as intended? The get_socket_local_port() function
returns an int, which is -1 on error. Since port is declared as an unsigned
16-bit integer (__be16), the -1 gets converted to 65535, causing the
ASSERT_GE(port, 0) check to always evaluate to true.

[ ... ]

> +cleanup:
> +	if (fd >= 0)
> +		close(fd);
> +	bpf_tc_hook_destroy(&tc_hook);

[Severity: Medium]
Could this unconditionally tear down the clsact qdisc on the base namespace's
loopback interface if an error occurs early?

If a failure happens before the network namespace is created or before
bpf_tc_hook_create() is called, the thread is still in the original
namespace. Since tc_hook is statically initialized with .ifindex = 1,
will this destroy the hook in the wrong namespace?

> +	if (server_fd >= 0)
> +		close(server_fd);
> +	netns_free(ns);
> +	if (pinned)
> +		unlink(LWT_EXT_PIN_PATH);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814-bpf-meta-inside-skb-ext-v1-0-767edd862656@cloudflare.com?part=11