Re: [PATCH net-next 05/14] selftests/bpf: Test skb_ext on cloned skbs

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Flawed assertion for `if_nametoindex` return value silently ignores errors.
- [Low] Multi-line comments do not follow the BPF subsystem comment style guidelines.
--

--- Patch [5]: [PATCH net-next 05/14] selftests/bpf: Test skb_ext on cloned skbs ---
commit 52378eb340683567339caa62c46774dede8267b1
Author: Jakub Sitnicki <[email protected]>

selftests/bpf: Test skb_ext on cloned skbs

This patch covers skb_ext behavior when an skb is cloned by TC mirred. It adds
tests to verify that the extension written at tap ingress is readable from the
mirred clone and that COW works when the extension is modified.

> 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 fd340e5538a34..58b793cf14e59 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
> @@ -694,6 +694,95 @@ void test_xdp_context_lwt_encap(void)
>  	test_xdp_meta__destroy(skel);
>  }
>  
> +/* Test if skb_ext survives skb clone (via tc mirred).
> + * dummy_prog runs on the clone (dummy ingress).
> + */

[Severity: Low]
This isn't a bug, but the multi-line comment does not follow the BPF subsystem
style guidelines. Should the opening /* be placed on its own line?

> +static void test_mirred_clone_ext(struct test_xdp_meta *skel,
> +				  struct bpf_program *dummy_prog)
> +{
> +	LIBBPF_OPTS(bpf_tc_hook, tc_hook, .attach_point = BPF_TC_INGRESS);
> +	LIBBPF_OPTS(bpf_tc_opts, tc_opts, .handle = 1, .priority = 1);
> +	struct netns_obj *ns = NULL;
> +	int dummy_ifindex;
> +	int tap_ifindex;
> +	int tap_fd = -1;
> +	int ret;
> +
> +	skel->bss->test_pass = false;
> +
> +	ns = netns_new("mirred_clone", true);
> +	if (!ASSERT_OK_PTR(ns, "netns_new"))
> +		return;
> +
> +	/* Dummy dev: attach reader */
> +	SYS(close, "ip link add name " DUMMY_NAME " type dummy");
> +	SYS(close, "ip link set dev " DUMMY_NAME " up");
> +
> +	dummy_ifindex = if_nametoindex(DUMMY_NAME);
> +	if (!ASSERT_GE(dummy_ifindex, 0, "dummy_ifindex"))

[Severity: Medium]
Since if_nametoindex() returns 0 on failure, will ASSERT_GE(..., 0) incorrectly
evaluate 0 as a success? If the interface lookup fails, won't the test proceed
with an invalid interface index (0) instead of catching the failure here?

> +		goto close;
> +
> +	tc_hook.ifindex = dummy_ifindex;
> +	ret = bpf_tc_hook_create(&tc_hook);
> +	if (!ASSERT_OK(ret, "dummy_hook_create"))
> +		goto close;
> +
> +	tc_opts.prog_fd = bpf_program__fd(dummy_prog);
> +	ret = bpf_tc_attach(&tc_hook, &tc_opts);
> +	if (!ASSERT_OK(ret, "dummy_attach"))
> +		goto close;
> +
> +	/* TAP dev: attach writer + mirred to dummy */
> +	tap_fd = open_tuntap(TAP_NAME, true);
> +	if (!ASSERT_GE(tap_fd, 0, "open_tuntap"))
> +		goto close;
> +
> +	SYS(close, "ip link set dev " TAP_NAME " up");
> +
> +	tap_ifindex = if_nametoindex(TAP_NAME);
> +	if (!ASSERT_GE(tap_ifindex, 0, "tap_ifindex"))

[Severity: Medium]
Similarly, will this assertion accept 0 as a valid interface index on failure
and result in confusing downstream errors during bpf_tc_hook_create()?

> +		goto close;

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/test_xdp_meta.c b/tools/testing/selftests/bpf/progs/test_xdp_meta.c
> index 43840ee32d359..1b6bb20ee7e85 100644
> --- a/tools/testing/selftests/bpf/progs/test_xdp_meta.c
> +++ b/tools/testing/selftests/bpf/progs/test_xdp_meta.c
> @@ -874,4 +875,49 @@ int tc_skb_ext_double_alloc(struct __sk_buff *ctx)
>  	return TC_ACT_UNSPEC;
>  }
>  
> +static const __u8 meta_zero[META_SIZE] = {};
> +
> +volatile bool clone_cow_done;
> +
> +/* Overwrite skb_ext on the clone via F_CREATE (COW) -- must not affect original.
> + * Runs on the dummy ingress (clone side), synchronously during tc mirred.
> + */

[Severity: Low]
This isn't a bug, but could the opening /* be placed on its own line to adhere
to the preferred kernel multi-line comment style used in the BPF subsystem?

> +SEC("tc")
> +int tc_skb_ext_clone_redir_cow(struct __sk_buff *ctx)
> +{
> +	struct bpf_dynptr meta;
> +
> +	if (bpf_dynptr_from_skb_ext(ctx, 0, BPF_SKB_EXT_F_CREATE, &meta))
> +		return TC_ACT_SHOT;
> +
> +	/* Zero out the clone's ext -- must not affect original */
> +	bpf_dynptr_write(&meta, 0, (void *)meta_zero, META_SIZE, 0);
> +
> +	clone_cow_done = true;
> +	return TC_ACT_SHOT;
> +}
> +
> +/* Verify COW isolation at kfree_skb time: once clone_cow_done is set,
> + * check that the original skb still has meta_want.
> + */

[Severity: Low]
This isn't a bug, but could the opening /* be placed on its own line to adhere
to the preferred kernel multi-line comment style used in the BPF subsystem?

> +SEC("tp_btf/kfree_skb")
> +int BPF_PROG(tp_kfree_skb_cow_check, struct sk_buff *skb)
> +{

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