netdev - Re: [PATCHv2 bpf-next 5/5] selftests: bpf: add test for sk

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]

Date:   Wed, 25 Mar 2020 22:28:11 -0700
From:   Joe Stringer <joe@...d.net.nz>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Joe Stringer <joe@...d.net.nz>, bpf <bpf@...r.kernel.org>,
        Lorenz Bauer <lmb@...udflare.com>,
        Networking <netdev@...r.kernel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Eric Dumazet <eric.dumazet@...il.com>,
        Martin Lau <kafai@...com>
Subject: Re: [PATCHv2 bpf-next 5/5] selftests: bpf: add test for sk_assign

On Wed, Mar 25, 2020 at 7:16 PM Andrii Nakryiko
<andrii.nakryiko@...il.com> wrote:
>
> On Tue, Mar 24, 2020 at 10:58 PM Joe Stringer <joe@...d.net.nz> wrote:
> >
> > From: Lorenz Bauer <lmb@...udflare.com>
> >
> > Attach a tc direct-action classifier to lo in a fresh network
> > namespace, and rewrite all connection attempts to localhost:4321
> > to localhost:1234 (for port tests) and connections to unreachable
> > IPv4/IPv6 IPs to the local socket (for address tests).
> >
> > Keep in mind that both client to server and server to client traffic
> > passes the classifier.
> >
> > Signed-off-by: Lorenz Bauer <lmb@...udflare.com>
> > Co-authored-by: Joe Stringer <joe@...d.net.nz>
> > Signed-off-by: Joe Stringer <joe@...d.net.nz>
> > ---

<snip>

> > +static void handle_timeout(int signum)
> > +{
> > +       if (signum == SIGALRM)
> > +               fprintf(stderr, "Timed out while connecting to server\n");
> > +       kill(0, SIGKILL);
> > +}
> > +
> > +static struct sigaction timeout_action = {
> > +       .sa_handler = handle_timeout,
> > +};
> > +
> > +static int connect_to_server(const struct sockaddr *addr, socklen_t len)
> > +{
> > +       int fd = -1;
> > +
> > +       fd = socket(addr->sa_family, SOCK_STREAM, 0);
> > +       if (CHECK_FAIL(fd == -1))
> > +               goto out;
> > +       if (CHECK_FAIL(sigaction(SIGALRM, &timeout_action, NULL)))
> > +               goto out;
>
> no-no-no, we are not doing this. It's part of prog_tests and shouldn't
> install its own signal handlers and sending asynchronous signals to
> itself. Please find another way to have a timeout.

I realise it didn't clean up after itself. How about signal(SIGALRM,
SIG_DFL); just like other existing tests do?

> > +       test__start_subtest("ipv4 addr redir");
> > +       if (run_test(server, (const struct sockaddr *)&addr4, sizeof(addr4)))
> > +               goto out;
> > +
> > +       test__start_subtest("ipv6 addr redir");
>
> test__start_subtest() returns false if subtest is supposed to be
> skipped. If you ignore that, then test_progs -t and -n filtering won't
> work properly.

Will fix.

> > +       if (run_test(server_v6, (const struct sockaddr *)&addr6, sizeof(addr6)))
> > +               goto out;
> > +
> > +       err = 0;
> > +out:
> > +       close(server);
> > +       close(server_v6);
> > +       return err;
> > +}
> > +
> > +void test_sk_assign(void)
> > +{
> > +       int self_net;
> > +
> > +       self_net = open(NS_SELF, O_RDONLY);
>
> I'm not familiar with what this does. Can you please explain briefly
> what are the side effects of this? Asking because of shared test_progs
> environment worries, of course.

This one is opening an fd to the current program's netns path on the
filesystem. The intention was to use it to switch back to the current
netns after the unshare() call elsewhere which switches to a new
netns. As per the other feedback the bit where it switches back to
this netns was dropped along the way so I'll fix it up in the next
revision.

> > +SEC("sk_assign_test")
>
> Please use "canonical" section name that libbpf recognizes. This
> woulde be "action/" or "classifier/", right?

Will fix.