Re: [PATCH bpf-next v5 4/4] selftests/bpf: add bpf_icmp_send recursion test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing packet filtering in root cgroup program causes system disruption and state corruption.
- [Medium] Uninitialized `unreach_type` prevents ICMP generation and causes guaranteed test failure.
--

commit 58dd624e2dc9a29c48c701ba76fcd20e713e626f
Author: Mahe Tardy <mahe.tardy@gmail.com>

selftests/bpf: add bpf_icmp_send recursion test

This commit adds a new test to verify that bpf_icmp_send returns -EBUSY
when recursion occurs. It attaches a BPF program to the root cgroup to
intercept the generated ICMP packets.

> diff --git a/tools/testing/selftests/bpf/prog_tests/icmp_send_kfunc.c b/tools/testing/selftests/bpf/prog_tests/icmp_send_kfunc.c
> index d9badfc6e620c..2363b1c7c711c 100644
> --- a/tools/testing/selftests/bpf/prog_tests/icmp_send_kfunc.c
> +++ b/tools/testing/selftests/bpf/prog_tests/icmp_send_kfunc.c

[ ... ]

> @@ -173,3 +175,41 @@ void test_icmp_send_unreach(void)
>  	icmp_send__destroy(skel);
>  	close(cgroup_fd);
>  }
> +
> +void test_icmp_send_unreach_recursion(void)
> +{
> +	struct icmp_send *skel;
> +	int cgroup_fd = -1;
> +
> +	skel = icmp_send__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "skel_open"))
> +		goto cleanup;
> +
> +	if (setup_cgroup_environment()) {
> +		fprintf(stderr, "Failed to setup cgroup environment\n");
> +		goto cleanup;
> +	}
> +
> +	cgroup_fd = get_root_cgroup();
> +	if (!ASSERT_GE(cgroup_fd, 0, "get_root_cgroup"))
> +		goto cleanup;
> +
> +	skel->links.recursion =
> +		bpf_program__attach_cgroup(skel->progs.recursion, cgroup_fd);
> +	if (!ASSERT_OK_PTR(skel->links.recursion, "prog_attach_cgroup"))
> +		goto cleanup;
> +
> +	trigger_prog_read_icmp_errqueue(skel, ICMP_HOST_UNREACH, AF_INET, "127.0.0.1");

Is it necessary to set skel->bss->unreach_type before triggering the program?

In progs/icmp_send.c, unreach_type defaults to 0. Looking at bpf_icmp_send()
in net/core/filter.c, for IPv4 it requires type to be exactly ICMP_DEST_UNREACH (3),
otherwise it returns -EOPNOTSUPP:

net/core/filter.c:bpf_icmp_send() {
...
	case htons(ETH_P_IP):
		if (type != ICMP_DEST_UNREACH)
			return -EOPNOTSUPP;
...
}

If the kfunc returns -EOPNOTSUPP (-95), no ICMP packet is generated, which seems
like it would prevent the recursion this test is designed to verify and cause
the assertions below to fail.

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/icmp_send.c b/tools/testing/selftests/bpf/progs/icmp_send.c
> index 6e1ba539eeb0f..a2ef6a02c2cb0 100644
> --- a/tools/testing/selftests/bpf/progs/icmp_send.c
> +++ b/tools/testing/selftests/bpf/progs/icmp_send.c

[ ... ]

> +SEC("cgroup_skb/egress")
> +int recursion(struct __sk_buff *skb)
> +{
> +	void *data = (void *)(long)skb->data;
> +	void *data_end = (void *)(long)skb->data_end;
> +	struct iphdr *iph;
> +	int ret;
> +
> +	iph = data;
> +	if ((void *)(iph + 1) > data_end || iph->version != 4)
> +		return SK_PASS;

Will this cause system-wide disruption and state corruption by intercepting all
IPv4 traffic?

Because this program is attached to the global root cgroup, it will intercept
all egress traffic on the host. Without further filtering for specific test
traffic (like protocol, port, or destination IP), any background IPv4 traffic
will trigger the program and call bpf_icmp_send().

This seems like it would generate ICMP Destination Unreachable messages for all
legitimate background connections (e.g., SSH, DNS, or other selftests),
causing an ICMP storm.

Additionally, concurrent background traffic might cause races on rec_kfunc_rets
and rec_count:

> +
> +	/* This call will provoke a recursion: the ICMP packet generated by the
> +	 * kfunc will re-trigger this program since we are in the root cgroup in
> +	 * which the kernel ICMP socket belongs. However when re-entering the
> +	 * kfunc, it should return EBUSY.
> +	 */
> +	ret = bpf_icmp_send(skb, unreach_type, unreach_code);
> +	rec_kfunc_rets[rec_count & 1] = ret;
> +	__sync_fetch_and_add(&rec_count, 1);
> +
> +	/* Because we attach the root cgroup, let all traffic pass to avoid
> +	 * disrupting unrelated network activity. Otherwise we could just let the
> +	 * first ICMP error message pass to trigger recursion and drop the rest.
> +	 */
> +	return SK_PASS;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515194746.50920-1-mahe.tardy@gmail.com?part=4