Re: [PATCH bpf-next v4 6/6] selftests/bpf: add icmp_send_unreach_recursion test

[sashiko-bot@kernel.org]

Sashiko AI review found 3 potential issue(s):
- [High] The `recursion` BPF program drops all unrelated IPv4 traffic on the system, causing severe network disruption and test flakiness.
- [Medium] The test leaks the cgroup testing environment by failing to call `cleanup_cgroup_environment()`.
- [Medium] The assignment to `rec_kfunc_rets` relies on an indeterminately sequenced evaluation order, making the test brittle and compiler-dependent.
--

commit 61d5cc3970ae5880f18e7f23d796670f691dcd7f
Author: Mahe Tardy <mahe.tardy@gmail.com>

selftests/bpf: add icmp_send_unreach_recursion test

This test is similar to icmp_send_unreach_kfunc but checks that, in case
of recursion, meaning that the BPF program calling the kfunc was
re-triggered by the icmp_send done by the kfunc, the kfunc will stop
early and return -EBUSY.

> diff --git a/tools/testing/selftests/bpf/prog_tests/icmp_send_unreach_kfunc.c b/tools/testing/selftests/bpf/prog_tests/icmp_send_unreach_kfunc.c
> index 047bfd4d80f75..a4f4324b2b99d 100644
> --- a/tools/testing/selftests/bpf/prog_tests/icmp_send_unreach_kfunc.c
> +++ b/tools/testing/selftests/bpf/prog_tests/icmp_send_unreach_kfunc.c
[ ... ]
> @@ -157,3 +159,44 @@ void test_icmp_send_unreach_kfunc(void)
>  	icmp_send_unreach__destroy(skel);
>  	close(cgroup_fd);
>  }
> +
> +void test_icmp_send_unreach_recursion(void)
> +{
[ ... ]
> +	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);

Since this test manually invokes setup_cgroup_environment(), shouldn't there
be a corresponding call to cleanup_cgroup_environment() in the cleanup path?

By bypassing the test__join_cgroup() wrapper, the test framework won't
automatically clean up the cgroup state, leaving the test worker in a modified
mount namespace which might cause subsequent tests to fail.

[ ... ]
> diff --git a/tools/testing/selftests/bpf/progs/icmp_send_unreach.c b/tools/testing/selftests/bpf/progs/icmp_send_unreach.c
> index 112b9cbfab6fe..9aca7c0b12e1f 100644
> --- a/tools/testing/selftests/bpf/progs/icmp_send_unreach.c
> +++ b/tools/testing/selftests/bpf/progs/icmp_send_unreach.c
[ ... ]
> @@ -67,4 +70,31 @@ int egress(struct __sk_buff *skb)
>  	return SK_DROP;
>  }
>  
> +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;
> +
> +	iph = data;
> +	if ((void *)(iph + 1) > data_end || iph->version != 4)
> +		return SK_PASS;
> +

Because this BPF program is attached to the system-wide root cgroup, will this
unconditionally drop all unrelated IPv4 traffic on the system?

Unlike the egress program which explicitly filters for IPPROTO_TCP and
SERVER_IP, this program intercepts all outbound IPv4 traffic, drops it
(returning SK_DROP), and sends ICMP errors.

This could disrupt host networking and cause test flakiness if background
traffic concurrently increments rec_count. Could the BPF program be updated
to explicitly filter for the test's traffic?

> +	/* This call will provoke a recursion: the ICMP package 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.
> +	 */
> +	rec_kfunc_rets[rec_count & 1] =
> +		bpf_icmp_send_unreach(skb, unreach_code);

Is it possible for the array index (rec_count & 1) to be evaluated before the
function call returns?

In C, the evaluation order of the left-hand side and right-hand side of an
assignment is not sequenced. If the compiler evaluates the array index before
the function call, both the outer and inner executions could use
rec_count == 0. This would cause the outer execution to overwrite the inner
execution's -EBUSY result with 0.

Would it be safer to store the kfunc return value in a local variable before
assigning it to the array?

> +	__sync_fetch_and_add(&rec_count, 1);
> +
> +	/* Let the first ICMP error message pass */
> +	if (iph->protocol == IPPROTO_ICMP)
> +		return SK_PASS;
> +
> +	return SK_DROP;
> +}

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