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

Message-ID: <20200728163758.2thfltlhsn2nse57@kafai-mbp>
Date:   Tue, 28 Jul 2020 09:37:59 -0700
From:   Martin KaFai Lau <kafai@...com>
To:     Jakub Sitnicki <jakub@...udflare.com>
CC:     <bpf@...r.kernel.org>, <netdev@...r.kernel.org>,
        <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>
Subject: Re: [PATCH bpf-next] udp, bpf: Ignore connections in reuseport group
 after BPF sk lookup

On Tue, Jul 28, 2020 at 05:46:29PM +0200, Jakub Sitnicki wrote:
> On Tue, Jul 28, 2020 at 03:20 AM CEST, Martin KaFai Lau wrote:
> > On Sun, Jul 26, 2020 at 02:02:28PM +0200, Jakub Sitnicki wrote:
> >> When BPF sk lookup invokes reuseport handling for the selected socket, it
> >> should ignore the fact that reuseport group can contain connected UDP
> >> sockets. With BPF sk lookup this is not relevant as we are not scoring
> >> sockets to find the best match, which might be a connected UDP socket.
> >>
> >> Fix it by unconditionally accepting the socket selected by reuseport.
> >>
> >> This fixes the following two failures reported by test_progs.
> >>
> >>   # ./test_progs -t sk_lookup
> >>   ...
> >>   #73/14 UDP IPv4 redir and reuseport with conns:FAIL
> >>   ...
> >>   #73/20 UDP IPv6 redir and reuseport with conns:FAIL
> >>   ...
> >>
> >> Fixes: a57066b1a019 ("Merge git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net")
> >> Cc: David S. Miller <davem@...emloft.net>
> >> Reported-by: Alexei Starovoitov <alexei.starovoitov@...il.com>
> >> Signed-off-by: Jakub Sitnicki <jakub@...udflare.com>
> >> ---
> >>  net/ipv4/udp.c | 2 +-
> >>  net/ipv6/udp.c | 2 +-
> >>  2 files changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> >> index 7ce31beccfc2..e88efba07551 100644
> >> --- a/net/ipv4/udp.c
> >> +++ b/net/ipv4/udp.c
> >> @@ -473,7 +473,7 @@ static struct sock *udp4_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)
> >>  		sk = reuse_sk;
> >>  	return sk;
> >>  }
> >> 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?

>From reuseport_select_sock(), it seems the kernel's select_by_hash
also avoids returning established sk.

In the mid term, we may consider to remove the connected udp
from the sockmap and reuseport_array.

I am a bit confused in the current situation on bpf@...seport returning
connected sk and I also can't think of a use case in the
sk-reuseport-prog-type side.  It was why I was curious on
the sk-lookup use case.