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

[Martin KaFai Lau <kafai@fb.com>]

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@davemloft.net>
> >> Reported-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> >> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.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@reuseport 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.