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

From: Jakub Sitnicki <jakub@cloudflare.com>
To: Martin KaFai Lau <kafai@fb.com>
Cc: bpf@vger.kernel.org, netdev@vger.kernel.org,
	kernel-team@cloudflare.com, Alexei Starovoitov <ast@kernel.org>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Alexei Starovoitov <alexei.starovoitov@gmail.com>
Subject: Re: [PATCH bpf-next] udp, bpf: Ignore connections in reuseport group after BPF sk lookup
Date: Tue, 28 Jul 2020 17:46:29 +0200	[thread overview]
Message-ID: <87pn8fwskq.fsf@cloudflare.com> (raw)
In-Reply-To: <20200728012042.r3gkkeg6ib3r2diy@kafai-mbp>

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)

Thanks,
-jkbs