netdev - Re: [PATCH bpf-next] udp, bpf: Ignore connections in reuseport group after BPF sk lookup

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] [day] [month] [year] [list]

Message-ID: <CA+FuTSfYRL18V7QCg+TXKp8gEcLP6S-jAYpO5HATQW+8Uv4Hhg@mail.gmail.com>
Date:   Tue, 28 Jul 2020 15:19:47 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Jakub Sitnicki <jakub@...udflare.com>
Cc:     Martin KaFai Lau <kafai@...com>, bpf <bpf@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>,
        kernel-team <kernel-team@...udflare.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Marek Majkowski <marek@...udflare.com>
Subject: Re: [PATCH bpf-next] udp, bpf: Ignore connections in reuseport group
 after BPF sk lookup

> >> >> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> >> >> index c394e674f486..29d9691359b9 100644
> >> >> --- a/net/ipv6/udp.c
> >> >> +++ b/net/ipv6/udp.c
> >> >> @@ -208,7 +208,7 @@ static inline struct sock *udp6_lookup_run_bpf(struct net *net,
> >> >>           return sk;
> >> >>
> >> >>   reuse_sk = lookup_reuseport(net, sk, skb, saddr, sport, daddr, hnum);
> >> >> - if (reuse_sk && !reuseport_has_conns(sk, false))
> >> >> + if (reuse_sk)
> >> > From __udp[46]_lib_lookup,
> >> > 1. The connected udp is picked by the kernel first.
> >> >    If a 4-tuple-matched connected udp is found.  It should have already
> >> >    been returned there.
> >> >
> >> > 2. If kernel cannot find a connected udp, the sk-lookup bpf prog can
> >> >    get a chance to pick another socket (likely bound to a different
> >> >    IP/PORT that the packet is destinated to) by bpf_sk_lookup_assign().
> >> >    However, bpf_sk_lookup_assign() does not allow TCP_ESTABLISHED.
> >> >
> >> >    With the change in this patch, it then allows the reuseport-bpf-prog
> >> >    to pick a connected udp which cannot be found in step (1).  Can you
> >> >    explain a use case for this?
> >>
> >> It is not intentional. It should not allow reuseport to pick a connected
> >> udp socket to be consistent with what sk-lookup prog can select. Thanks
> >> for pointing it out.
> >>
> >> I've incorrectly assumed that after acdcecc61285 ("udp: correct
> >> reuseport selection with connected sockets") reuseport returns only
> >> unconnected udp sockets, but thats not true for bpf reuseport.
> >>
> >> So this patch fixes one corner base, but breaks another one.
> >>
> >> I'll change the check to the below and respin:
> >>
> >> -    if (reuse_sk && !reuseport_has_conns(sk, false))
> >> +    if (reuse_sk && reuse_sk->sk_state != TCP_ESTABLISHED)
> > May be disallow TCP_ESTABLISHED in bpf_sk_select_reuseport() instead
> > so that the bpf reuseport prog can have a more consistent
> > behavior among sk-lookup and the regular sk-reuseport-select case.
> > Thought?
>
> Ah, I see now what you had in mind. If that option is on the table, I'm
> all for it. Being consistent makes it easier to explain and use.
>
> In that case, let me make that change in a separate submission. I want
> to get test coverage in for the three reuseport flavors.
>
> > From reuseport_select_sock(), it seems the kernel's select_by_hash
> > also avoids returning established sk.
>
> Right. CC'ing Willem to check if bpf was left out on purpose or not.

Not on purpose. I considered that this is up to the BPF program.