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.
