Re: [PATCH bpf-next v11 5/5] selftests/bpf: add bpf_icmp_send no route test

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

On Thu, Jul 09, 2026 at 03:00:10PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Low] The run_icmp_no_route_test subtest suffers from state pollution because it fails to reset skel->bss->unreach_code, passing stale or invalid ICMP codes to the BPF program.
> --
> 
> --- Patch [5]: [PATCH bpf-next v11 5/5] selftests/bpf: add bpf_icmp_send no route test ---
> commit 43111be9123ff27205045da2a59f2daf8fc9a986
> Author: Mahe Tardy <mahe.tardy@gmail.com>
> 
> selftests/bpf: add bpf_icmp_send no route test
> 
> This commit adds a test to verify that the bpf_icmp_send kfunc returns
> -ENETUNREACH when a test run is performed on an skb without a destination
> route.
> 
> > 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 bb532aa0d1580..9318d4bc7ce8b 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/icmp_send_kfunc.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/icmp_send_kfunc.c
> 
> [ ... ]
> 
> > @@ -169,6 +169,49 @@ static void run_icmp_test(struct icmp_send *skel, int af, const char *ip,
> > +static void run_icmp_no_route_test(struct icmp_send *skel, int af)
> > +{
> > +	union {
> > +		struct ipv4_packet v4;
> > +		struct ipv6_packet v6;
> > +	} pkt;
> > +	DECLARE_LIBBPF_OPTS(bpf_test_run_opts, opts,
> > +		.data_in = &pkt,
> > +	);
> > +	int err;
> > +
> > +	switch (af) {
> > +	case AF_INET:
> > +		pkt.v4 = pkt_v4;
> > +		pkt.v4.iph.version = 4;
> > +		pkt.v4.iph.daddr = htonl(INADDR_LOOPBACK);
> > +		pkt.v4.tcp.dest = htons(80);
> > +		opts.data_size_in = sizeof(pkt.v4);
> > +		skel->bss->unreach_type = ICMP_DEST_UNREACH;
> > +		break;
> > +	case AF_INET6:
> > +		pkt.v6 = pkt_v6;
> > +		pkt.v6.iph.version = 6;
> > +		pkt.v6.iph.daddr = in6addr_loopback;
> > +		pkt.v6.tcp.dest = htons(80);
> > +		opts.data_size_in = sizeof(pkt.v6);
> > +		skel->bss->unreach_type = ICMPV6_DEST_UNREACH;
> > +		break;
> > +	default:
> > +		ASSERT_FAIL("af_not_supported");
> > +		return;
> > +	}
> > +
> > +	skel->bss->server_port = 80;
> 
> [Severity: Low]
> Does this leave skel->bss->unreach_code uninitialized for this test?
> 
> Since the run_icmp_no_route_test subtests share the same BPF skeleton as the
> previous run_icmp_test subtests, if they run after the ipv6 subtest,
> unreach_code is left at 7 (max_code + 1 from testing invalid codes).
> 
> For no_route_ipv6, code 7 is passed to the kernel, which is an invalid ICMPv6
> code (max is 6). The test currently passes only because the kernel's
> bpf_icmp_send implementation checks for a missing destination route and returns
> early before validating the code parameter.
> 
> Would it be better to reset skel->bss->unreach_code to a valid value here to
> prevent test state pollution?

I'd say it's indeed not a big deal since the test triggers a check that
is before it reaches the EINVAL, but would be more theoritically correct
to reset to 0 for both v4 and v6. So mostly a nit.

> 
> > +	skel->data->kfunc_ret = KFUNC_RET_UNSET;
> > +
> > +	err = bpf_prog_test_run_opts(bpf_program__fd(skel->progs.egress), &opts);
> > +	if (!ASSERT_OK(err, "test_run"))
> > +		return;
> > +
> > +	ASSERT_EQ(skel->data->kfunc_ret, -ENETUNREACH, "kfunc_ret_no_route");
> > +}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260709144900.245904-1-mahe.tardy@gmail.com?part=5