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@xxxxxxxxxxxxx>
> >> Reported-by: Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx>
> >> Signed-off-by: Jakub Sitnicki <jakub@xxxxxxxxxxxxxx>
> >> ---
> >> 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.
