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

[Mahe Tardy <mahe.tardy@gmail.com>]

On Fri, May 15, 2026 at 09:00:52PM +0000, sashiko-bot@kernel.org wrote:
> 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?

Yes it's done in trigger_prog_read_icmp_errqueue.

> 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.

I think Sashiko is confused here as the type is indeed set during this
test that actually passes correctly as expected.

> 
> [ ... ]
> 
> > 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:

Ok let's forget about this, I'll just revert to SK_DROP and confirm that
this will not cause a system-wide disruption and state corruption for
this test case. I need to use the host root cgroup for my test to work
and this should be fine.

> > +
> > +	/* 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