Re: [PATCH bpf-next v7 7/7] selftests/bpf: add bpf_icmp_send recursion test

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

On Tue, May 26, 2026 at 08:22:31PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] Attaching to the system-wide root cgroup without PID filtering intercepts all system-wide loopback ICMP traffic, causing data races and network disruption.
> - [Low] Multi-line comments do not follow the mandated BPF subsystem style.
> --
> 
> commit a40313de8eec16152c739a5ecc571fcd166b9bba
> Author: Mahe Tardy <mahe.tardy@gmail.com>
> 
> selftests/bpf: add bpf_icmp_send recursion test
> 
> This commit adds a test to verify that recursive calls to bpf_icmp_send
> from a BPF program return -EBUSY and stop early.
> 
> > diff --git a/tools/testing/selftests/bpf/prog_tests/icmp_send_kfunc.c b/tools/testing/selftests/bpf/prog_tests/icmp_send_kfunc.c
> > --- a/tools/testing/selftests/bpf/prog_tests/icmp_send_kfunc.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/icmp_send_kfunc.c
> 
> [ ... ]
> 
> > @@ -201,3 +202,42 @@ void test_icmp_send_unreach_tc(void)
> >  	bpf_link__destroy(link);
> >  	icmp_send__destroy(skel);
> >  }
> > +
> > +void test_icmp_send_unreach_recursion(void)
> > +{
> 
> [ ... ]
> 
> > +	trigger_prog_read_icmp_errqueue(skel, ICMP_HOST_UNREACH, AF_INET,
> > +					"127.0.0.1");
> > +
> > +	/* Because there's recursion involved, the first call will return at
> > +	 * index 1 since it will return the second, and the second call will
> > +	 * return at index 0 since it will return the first.
> > +	 */
> 
> [Severity: Low]
> This isn't a bug, but BPF subsystem multi-line comments should have the
> opening /* on its own line.

Style issue could be fixed if required.

> 
> > diff --git a/tools/testing/selftests/bpf/progs/icmp_send.c b/tools/testing/selftests/bpf/progs/icmp_send.c
> > --- a/tools/testing/selftests/bpf/progs/icmp_send.c
> > +++ b/tools/testing/selftests/bpf/progs/icmp_send.c
> 
> [ ... ]
> 
> > @@ -125,4 +128,45 @@ int tc_egress(struct __sk_buff *skb)
> >  	return TCX_DROP;
> >  }
> >  
> > +SEC("cgroup_skb/egress")
> > +int recursion(struct __sk_buff *skb)
> > +{
> 
> [ ... ]
> 
> > +	} else if (iph->protocol != IPPROTO_ICMP) {
> > +		return SK_PASS;
> > +	}
> > +
> > +	/* 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.
> > +	 */
> 
> [Severity: Low]
> This isn't a bug, but this multi-line comment should also have the opening
> /* on its own line.

Dito.

> 
> > +	ret = bpf_icmp_send(skb, unreach_type, unreach_code);
> > +	rec_kfunc_rets[rec_count & 1] = ret;
> > +	__sync_fetch_and_add(&rec_count, 1);
> 
> [Severity: Medium]
> Could attaching to the system-wide root cgroup without PID filtering
> intercept all system-wide loopback ICMP traffic and disrupt other processes?
> 
> Because ICMP traffic is not filtered by port, any ICMP traffic to 127.0.0.1
> on the host will trigger this BPF program and generate unintended ICMP
> Destination Unreachable replies.
> 
> Also, can concurrent executions cause a data race here? Two CPUs could read
> the same rec_count, overwrite each other's return values in the array, and
> corrupt the state, causing test flakes.
> 
> Would filtering by the test's PID securely isolate the test? The recursive
> kernel socket call executes synchronously in the same task context, which
> should make PID filtering reliable here.

I don't think it's necessary, we already added some filtering here, we
can always add more but I'm really not sure it's useful.

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