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

[Jakub Sitnicki <jakub@cloudflare.com>]

On Tue, Jul 28, 2020 at 06:37 PM CEST, Martin KaFai Lau wrote:
> 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?

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.

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

SGTM.

> 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.

I don't know about any use cases for selecting a connected udp socket
from bpf reuseport either. It certainly sounds like unexpected behavior
for the receiving process, which expects traffic from just one remote
peer.

Marek, does anything come to mind?

If not, I guess we'll see if anyone screams when the change is proposed.