Re: [PATCH net-next 09/14] selftests/bpf: Test skb_ext read from kfree_skb tracepoint

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing validation of `recv()` failure before reading `errno`.
- [Medium] Missing return value check for `send()` can lead to infinite test hang.
--

--- Patch [9]: [PATCH net-next 09/14] selftests/bpf: Test skb_ext read from kfree_skb tracepoint ---
commit 8897bd2269141881d412c939eda870a1cef5354d
Author: Jakub Sitnicki <[email protected]>

selftests/bpf: Test skb_ext read from kfree_skb tracepoint

This commit adds a test that writes skb_ext at cgroup/skb egress, sends UDP
to a closed loopback port so the packet is dropped, and reads the extension
back from a tp_btf/kfree_skb program.

> 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 e0ab0fccf9d7e..1e4e12d2b041c 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
[ ... ]
> @@ -1186,6 +1186,69 @@ static void test_skb_ext_tcp(struct test_xdp_meta *skel, const char *name,
[ ... ]
> +static void test_cgrp_egress_to_kfree_skb(struct test_xdp_meta *skel)
> +{
[ ... ]
> +	ret = connect(fd, (void *)&addr, sizeof(addr));
> +	if (!ASSERT_OK(ret, "connect"))
> +		goto cleanup;
> +
> +	send(fd, test_payload, TEST_PAYLOAD_LEN, 0);

[Severity: Medium]
Is there a risk of an infinite hang here if send() fails locally?
If send() fails (for example, due to local buffer exhaustion or routing
issues), the packet is never sent, and the kernel will not generate the
expected ICMP error. Consequently, the test might hang indefinitely in the
subsequent blocking recv() call.

> +
> +	/* Wait for ICMP error -- confirms the packet was freed */
> +	ret = recv(fd, buf, sizeof(buf), 0);
> +	ASSERT_EQ(errno, ECONNREFUSED, "recv_econnrefused");

[Severity: Medium]
Should ret be checked for -1 before evaluating errno?
According to POSIX semantics, errno is only meaningful if a system call
returns an error. If recv() happens to succeed (for example, by receiving
a stray UDP packet), errno will contain a stale value, which could cause the
test to spuriously pass or fail.

> +
> +	ASSERT_TRUE(skel->bss->test_pass, "test_pass");
> +
> +cleanup:
[ ... ]

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