Re: [PATCH net-next 08/14] selftests/bpf: Test skb_ext read from sock_ops and LSM hooks

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Invalid error check for `get_socket_local_port()` due to assignment to an unsigned `__be16` type.
--

--- Patch [8]: [PATCH net-next 08/14] selftests/bpf: Test skb_ext read from sock_ops and LSM hooks ---
commit e995e7626c7fc8d98cbc259f8c506f6ec68e6a00
Author: Jakub Sitnicki <[email protected]>

selftests/bpf: Test skb_ext read from sock_ops and LSM hooks

This commit extends skb_ext test coverage to the TCP ingress path. It adds
a new test that attaches a TC ingress program to write metadata to packets
and reads it back using sock_ops and LSM hooks.

> 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 808eb158fe0f1..e0ab0fccf9d7e 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
[ ... ]
> +/* Test skb_ext survival across TC ingress -> TCP reader hook */
> +static void test_skb_ext_tcp(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);
> +	struct bpf_link *reader_link = NULL;
> +	struct netns_obj *ns = NULL;
> +	int server_fd = -1;
> +	int cgroup_fd = -1;
> +	int client_fd = -1;
> +	int conn_fd = -1;
> +	__be16 port;
> +	int ret;
> +
> +	ns = netns_new(name, true);
[ ... ]
> +	server_fd = start_server(AF_INET, SOCK_STREAM, "127.0.0.1", 0, 0);
> +	if (!ASSERT_GE(server_fd, 0, "start_server"))
> +		goto cleanup;
> +
> +	port = get_socket_local_port(server_fd);
> +	if (!ASSERT_GE(port, 0, "get_port"))

[Severity: Medium]
Does assigning the result of get_socket_local_port() to the unsigned __be16
port variable defeat this error check?

get_socket_local_port() returns an int, which can be a negative error code.
If the function fails and returns a negative value, storing it in an
unsigned __be16 will result in a positive value. This would cause the
ASSERT_GE(port, 0, "get_port") condition to always pass, ignoring the
failure and potentially leading to unexpected test behavior.

> +		goto cleanup;
> +
> +	skel->bss->target_port = port;
> +	skel->bss->test_pass = false;

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