Re: [PATCH net-next 06/14] selftests/bpf: Test skb_ext survival across veth and GRE

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Incorrect assertions for if_nametoindex() return values allow errors to pass silently.
--

--- Patch [6]: [PATCH net-next 06/14] selftests/bpf: Test skb_ext survival across veth and GRE ---
commit 9eef2d71b261cfaa465a5233213fb272dc34f416
Author: Jakub Sitnicki <[email protected]>

selftests/bpf: Test skb_ext survival across veth and GRE
    
This commit tests if skb_ext survives veth cross-netns forwarding and GRE
encap/decap. It writes skb_ext at TC egress and reads it back at TC ingress
after the packet crosses device boundaries.

> 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 58b793cf14e59..de60303025361 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

[ ... ]

> @@ -783,6 +783,156 @@ static void test_mirred_clone_ext_cow(struct test_xdp_meta *skel)
> +/* Test if skb_ext survives veth cross-netns forward */
> +static void test_skb_ext_scrub_veth(struct test_xdp_meta *skel)
> +{

[ ... ]

> +	SYS(close, "ip link set dev " RX_NAME " up");
> +
> +	rx_ifindex = if_nametoindex(RX_NAME);
> +	if (!ASSERT_GE(rx_ifindex, 0, "if_nametoindex rx"))
> +		goto close;

[Severity: Low]
Does this code correctly catch interface lookup failures?

Since if_nametoindex() returns 0 on failure, the ASSERT_GE(rx_ifindex, 0, ...)
check will evaluate 0 >= 0 as true, failing to catch the error. The test will
instead fail slightly later with a less descriptive error. 

Should this be using ASSERT_GT() or ASSERT_NEQ() instead?

[ ... ]

> +	SYS(close, "ip link set dev " TX_NAME " up");
> +
> +	tx_ifindex = if_nametoindex(TX_NAME);
> +	if (!ASSERT_GE(tx_ifindex, 0, "if_nametoindex tx"))
> +		goto close;

[Severity: Low]
Is this the same issue here? if_nametoindex() returning 0 would bypass this
ASSERT_GE check.

[ ... ]

> +/* Test if skb_ext survives GRE tunnel encap+decap */
> +static void test_skb_ext_scrub_gre(struct test_xdp_meta *skel)
> +{

[ ... ]

> +	/* Write skb_ext on TC egress on GRE tx */
> +	tx_ifindex = if_nametoindex("gre_tx");
> +	if (!ASSERT_GE(tx_ifindex, 0, "tx_ifindex"))
> +		goto close;

[Severity: Low]
Is this the same issue here as well?

[ ... ]

> +	/* Read skb_ext on TC ingress on GRE rx */
> +	rx_ifindex = if_nametoindex("gre_rx");
> +	if (!ASSERT_GE(rx_ifindex, 0, "rx_ifindex"))
> +		goto close;

[Severity: Low]
And one more instance here where ASSERT_GE allows if_nametoindex() failure
to pass silently.

[ ... ]

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