netdev - Re: [PATCH bpf-next v3 4/4] selftests/bpf: add icmp_send

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aIeedqGvdfO641Ht@gmail.com>
Date: Mon, 28 Jul 2025 17:59:50 +0200
From: Mahe Tardy <mahe.tardy@...il.com>
To: Yonghong Song <yonghong.song@...ux.dev>
Cc: lkp@...el.com, alexei.starovoitov@...il.com, andrii@...nel.org,
	ast@...nel.org, bpf@...r.kernel.org, coreteam@...filter.org,
	daniel@...earbox.net, fw@...len.de, john.fastabend@...il.com,
	martin.lau@...ux.dev, netdev@...r.kernel.org,
	netfilter-devel@...r.kernel.org, oe-kbuild-all@...ts.linux.dev,
	pablo@...filter.org
Subject: Re: [PATCH bpf-next v3 4/4] selftests/bpf: add icmp_send_unreach
 kfunc tests

On Mon, Jul 28, 2025 at 08:40:49AM -0700, Yonghong Song wrote:
> 
> 
> On 7/28/25 2:43 AM, Mahe Tardy wrote:

[...]

> > +
> > +void test_icmp_send_unreach_kfunc(void)
> > +{
> > +	struct icmp_send_unreach *skel;
> > +	int cgroup_fd = -1, client_fd = 1, srv_fd = -1;
> 
> Should set client_fd = -1? See below ...

Well spotted yes, it's a typo, thank you.

> > +	int *code;
> > +
> > +	skel = icmp_send_unreach__open_and_load();
> > +	if (!ASSERT_OK_PTR(skel, "skel_open"))
> > +		goto cleanup;
> > +
> > +	cgroup_fd = test__join_cgroup("/icmp_send_unreach_cgroup");
> > +	if (!ASSERT_GE(cgroup_fd, 0, "join_cgroup"))
> > +		goto cleanup;
> > +
> > +	skel->links.egress =
> > +		bpf_program__attach_cgroup(skel->progs.egress, cgroup_fd);
> > +	if (!ASSERT_OK_PTR(skel->links.egress, "prog_attach_cgroup"))
> > +		goto cleanup;
> > +
> > +	code = &skel->bss->unreach_code;
> > +
> > +	for (*code = 0; *code <= NR_ICMP_UNREACH; (*code)++) {
> > +		// The TCP stack reacts differently when asking for
> > +		// fragmentation, let's ignore it for now
> > +		if (*code == ICMP_FRAG_NEEDED)
> > +			continue;
> > +
> > +		skel->bss->kfunc_ret = -1;
> > +
> > +		srv_fd = start_server(AF_INET, SOCK_STREAM, "127.0.0.1",
> > +				      SRV_PORT, TIMEOUT_MS);
> > +		if (!ASSERT_GE(srv_fd, 0, "start_server"))
> > +			goto for_cleanup;
> 
> Otherwise if client_fd = 1, goto for_cleanup will close(1).
> 
> > +
> > +		client_fd = socket(AF_INET, SOCK_STREAM, 0);
> > +		ASSERT_GE(client_fd, 0, "client_socket");
> 
> The above two lines are not necessary since client_fd is
> actually set in the below.

Yep, must have been a leftover from when I was discovering the
network_helpers, oops!

> > +
> > +		client_fd = connect_to_fd(srv_fd, 0);
> > +		if (!ASSERT_GE(client_fd, 0, "client_connect"))
> > +			goto for_cleanup;
> > +
> > +		read_icmp_errqueue(client_fd, *code);
> > +
> > +		ASSERT_EQ(skel->bss->kfunc_ret, SK_DROP, "kfunc_ret");
> > +for_cleanup:
> > +		close(client_fd);
> > +		close(srv_fd);
> > +	}
> > +
> > +cleanup:
> > +	icmp_send_unreach__destroy(skel);
> > +	close(cgroup_fd);
> > +}
> [...]

I'm sending a v4 with those fixed + fixing the builds error when IPv6 is
built as a module from the kfunc patch. Thanks for the review.