netdev - Re: [PATCH bpf-next v3] bpf: bpf_{g,s}etsockopt for struct bpf_sock

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:   Fri, 1 May 2020 15:07:28 -0700
From:   Stanislav Fomichev <sdf@...gle.com>
To:     Andrey Ignatov <rdna@...com>
Cc:     Netdev <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        John Fastabend <john.fastabend@...il.com>,
        Martin KaFai Lau <kafai@...com>
Subject: Re: [PATCH bpf-next v3] bpf: bpf_{g,s}etsockopt for struct bpf_sock_addr

On Fri, May 1, 2020 at 2:52 PM Andrey Ignatov <rdna@...com> wrote:
>
> Stanislav Fomichev <sdf@...gle.com> [Thu, 2020-04-30 16:32 -0700]:
> > Currently, bpf_getsockopt and bpf_setsockopt helpers operate on the
> > 'struct bpf_sock_ops' context in BPF_PROG_TYPE_SOCK_OPS program.
> > Let's generalize them and make them available for 'struct bpf_sock_addr'.
> > That way, in the future, we can allow those helpers in more places.
> >
> > As an example, let's expose those 'struct bpf_sock_addr' based helpers to
> > BPF_CGROUP_INET{4,6}_CONNECT hooks. That way we can override CC before the
> > connection is made.
> >
> > v3:
> > * Expose custom helpers for bpf_sock_addr context instead of doing
> >   generic bpf_sock argument (as suggested by Daniel). Even with
> >   try_socket_lock that doesn't sleep we have a problem where context sk
> >   is already locked and socket lock is non-nestable.
> >
> > v2:
> > * s/BPF_PROG_TYPE_CGROUP_SOCKOPT/BPF_PROG_TYPE_SOCK_OPS/
> >
> > Cc: John Fastabend <john.fastabend@...il.com>
> > Cc: Martin KaFai Lau <kafai@...com>
> > Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
>
> ...
>
> >  SEC("cgroup/connect4")
> >  int connect_v4_prog(struct bpf_sock_addr *ctx)
> >  {
> > @@ -66,6 +108,10 @@ int connect_v4_prog(struct bpf_sock_addr *ctx)
> >
> >       bpf_sk_release(sk);
> >
> > +     /* Rewrite congestion control. */
> > +     if (ctx->type == SOCK_STREAM && set_cc(ctx))
> > +             return 0;
>
> Hi Stas,
>
> This new check breaks one of tests in test_sock_addr:
>
>         root@...h-fb-vm1:/home/rdna/bpf-next/tools/testing/selftests/bpf ./test_sock_addr.sh
>         ...
>         (test_sock_addr.c:1199: errno: Operation not permitted) Fail to connect to server
>         Test case: connect4: rewrite IP & TCP port .. [FAIL]
>         ...
>         Summary: 34 PASSED, 1 FAILED
>
> What the test does is it sets up TCPv4 server:
>
>         [pid   386] socket(PF_INET, SOCK_STREAM, IPPROTO_IP) = 6
>         [pid   386] bind(6, {sa_family=AF_INET, sin_port=htons(4444), sin_addr=inet_addr("127.0.0.1")}, 128) = 0
>         [pid   386] listen(6, 128)              = 0
>
> Then tries to connect to a fake IPv4:port and this connect4 program
> should redirect it to that TCP server, but only if every field in
> context has expected value.
>
> But after that commit program started denying the connect:
>
>         [pid   386] socket(PF_INET, SOCK_STREAM, IPPROTO_IP) = 7
>         [pid   386] connect(7, {sa_family=AF_INET, sin_port=htons(4040), sin_addr=inet_addr("192.168.1.254")}, 128) = -1 EPERM (Operation not permitted)
>         (test_sock_addr.c:1201: errno: Operation not permitted) Fail to connect to server
>         Test case: connect4: rewrite IP & TCP port .. [FAIL]
>
> I verified that commenting out this new `if` fixes the problem, but
> haven't spent time root-causing it. Could you please look at it?
Could you please confirm that you have CONFIG_TCP_CONG_DCTCP=y in your kernel
config? (I've added it to tools/testing/selftests/bpf/config)
The test is now flipping CC to dctcp and back to default cubic. It can
fail if dctcp is not compiled in.