[PATCH bpf-next 0/2] Fix BPF socket lookup with reuseport groups with connections

[PATCH bpf-next 0/2] Fix BPF socket lookup with reuseport groups with connections

[Jakub Sitnicki]

This mini series contains a fix for a bug noticed when analyzing a reported
merge conflict between bpf-next and net tree [0].

Apart from fixing a corner-case that affects use of BPF sk_lookup in tandem
with UDP reuseport groups with connected sockets, it should make the
conflict resolution with net tree easier.

These changes don't replicate the improved UDP socket lookup behavior from
net tree, where commit efc6b6f6c311 ("udp: Improve load balancing for
SO_REUSEPORT.") is present.

Happy to do it as a follow up. For the moment I didn't want to make things
more confusing when it comes to what got fixed where and why.

Thanks,
-jkbs

Cc: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>

[0] https://lore.kernel.org/linux-next/20200722132143.700a5ccc@canb.auug.org.au/

Jakub Sitnicki (2):
  udp: Don't discard reuseport selection when group has connections
  selftests/bpf: Test BPF socket lookup and reuseport with connections

 net/ipv4/udp.c                                |  5 +-
 net/ipv6/udp.c                                |  5 +-
 .../selftests/bpf/prog_tests/sk_lookup.c      | 54 ++++++++++++++++++-
 3 files changed, 55 insertions(+), 9 deletions(-)

-- 
2.25.4

[PATCH bpf-next 1/2] udp: Don't discard reuseport selection when group has connections

[Jakub Sitnicki]

When BPF socket lookup prog selects a socket that belongs to a reuseport
group, and the reuseport group has connected sockets in it, the socket
selected by reuseport will be discarded, and socket returned by BPF socket
lookup will be used instead.

Modify this behavior so that the socket selected by reuseport running after
BPF socket lookup always gets used. Ignore the fact that the reuseport
group might have connections because it is only relevant when scoring
sockets during regular hashtable-based lookup.

Fixes: 72f7e9440e9b ("udp: Run SK_LOOKUP BPF program on socket lookup")
Fixes: 6d4201b1386b ("udp6: Run SK_LOOKUP BPF program on socket lookup")
Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 net/ipv4/udp.c | 5 +----
 net/ipv6/udp.c | 5 +----
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index b5231ab350e0..487740d0088c 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -421,9 +421,6 @@ static inline struct sock *lookup_reuseport(struct net *net, struct sock *sk,
 		hash = udp_ehashfn(net, daddr, hnum, saddr, sport);
 		reuse_sk = reuseport_select_sock(sk, hash, skb,
 						 sizeof(struct udphdr));
-		/* Fall back to scoring if group has connections */
-		if (reuseport_has_conns(sk, false))
-			return NULL;
 	}
 	return reuse_sk;
 }
@@ -447,7 +444,7 @@ static struct sock *udp4_lib_lookup2(struct net *net,
 		if (score > badness) {
 			result = lookup_reuseport(net, sk, skb,
 						  saddr, sport, daddr, hnum);
-			if (result)
+			if (result && !reuseport_has_conns(sk, false))
 				return result;
 
 			badness = score;
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index ff8be202726a..8fd8eb04994c 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -155,9 +155,6 @@ static inline struct sock *lookup_reuseport(struct net *net, struct sock *sk,
 		hash = udp6_ehashfn(net, daddr, hnum, saddr, sport);
 		reuse_sk = reuseport_select_sock(sk, hash, skb,
 						 sizeof(struct udphdr));
-		/* Fall back to scoring if group has connections */
-		if (reuseport_has_conns(sk, false))
-			return NULL;
 	}
 	return reuse_sk;
 }
@@ -180,7 +177,7 @@ static struct sock *udp6_lib_lookup2(struct net *net,
 		if (score > badness) {
 			result = lookup_reuseport(net, sk, skb,
 						  saddr, sport, daddr, hnum);
-			if (result)
+			if (result && !reuseport_has_conns(sk, false))
 				return result;
 
 			result = sk;
-- 
2.25.4

[PATCH bpf-next 2/2] selftests/bpf: Test BPF socket lookup and reuseport with connections

[Jakub Sitnicki]

Cover the case when BPF socket lookup returns a socket that belongs to a
reuseport group, and the reuseport group contains connected UDP sockets.

Ensure that the presence of connected UDP sockets in reuseport group does
not affect the socket lookup result. Socket selected by reuseport should
always be used as result in such case.

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 .../selftests/bpf/prog_tests/sk_lookup.c      | 54 ++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/sk_lookup.c b/tools/testing/selftests/bpf/prog_tests/sk_lookup.c
index f1784ae4565a..9bbd2b2b7630 100644
--- a/tools/testing/selftests/bpf/prog_tests/sk_lookup.c
+++ b/tools/testing/selftests/bpf/prog_tests/sk_lookup.c
@@ -74,6 +74,7 @@ struct test {
 	struct inet_addr connect_to;
 	struct inet_addr listen_at;
 	enum server accept_on;
+	bool reuseport_has_conns; /* Add a connected socket to reuseport group */
 };
 
 static __u32 duration;		/* for CHECK macro */
@@ -559,7 +560,8 @@ static void query_lookup_prog(struct test_sk_lookup *skel)
 
 static void run_lookup_prog(const struct test *t)
 {
-	int client_fd, server_fds[MAX_SERVERS] = { -1 };
+	int server_fds[MAX_SERVERS] = { -1 };
+	int client_fd, reuse_conn_fd = -1;
 	struct bpf_link *lookup_link;
 	int i, err;
 
@@ -583,6 +585,32 @@ static void run_lookup_prog(const struct test *t)
 			break;
 	}
 
+	/* Regular UDP socket lookup with reuseport behaves
+	 * differently when reuseport group contains connected
+	 * sockets. Check that adding a connected UDP socket to the
+	 * reuseport group does not affect how reuseport works with
+	 * BPF socket lookup.
+	 */
+	if (t->reuseport_has_conns) {
+		struct sockaddr_storage addr = {};
+		socklen_t len = sizeof(addr);
+
+		/* Add an extra socket to reuseport group */
+		reuse_conn_fd = make_server(t->sotype, t->listen_at.ip,
+					    t->listen_at.port,
+					    t->reuseport_prog);
+		if (reuse_conn_fd < 0)
+			goto close;
+
+		/* Connect the extra socket to itself */
+		err = getsockname(reuse_conn_fd, (void *)&addr, &len);
+		if (CHECK(err, "getsockname", "errno %d\n", errno))
+			goto close;
+		err = connect(reuse_conn_fd, (void *)&addr, len);
+		if (CHECK(err, "connect", "errno %d\n", errno))
+			goto close;
+	}
+
 	client_fd = make_client(t->sotype, t->connect_to.ip, t->connect_to.port);
 	if (client_fd < 0)
 		goto close;
@@ -594,6 +622,8 @@ static void run_lookup_prog(const struct test *t)
 
 	close(client_fd);
 close:
+	if (reuse_conn_fd != -1)
+		close(reuse_conn_fd);
 	for (i = 0; i < ARRAY_SIZE(server_fds); i++) {
 		if (server_fds[i] != -1)
 			close(server_fds[i]);
@@ -710,6 +740,17 @@ static void test_redirect_lookup(struct test_sk_lookup *skel)
 			.listen_at	= { INT_IP4, INT_PORT },
 			.accept_on	= SERVER_B,
 		},
+		{
+			.desc		= "UDP IPv4 redir and reuseport with conns",
+			.lookup_prog	= skel->progs.select_sock_a,
+			.reuseport_prog	= skel->progs.select_sock_b,
+			.sock_map	= skel->maps.redir_map,
+			.sotype		= SOCK_DGRAM,
+			.connect_to	= { EXT_IP4, EXT_PORT },
+			.listen_at	= { INT_IP4, INT_PORT },
+			.accept_on	= SERVER_B,
+			.reuseport_has_conns = true,
+		},
 		{
 			.desc		= "UDP IPv4 redir skip reuseport",
 			.lookup_prog	= skel->progs.select_sock_a_no_reuseport,
@@ -754,6 +795,17 @@ static void test_redirect_lookup(struct test_sk_lookup *skel)
 			.listen_at	= { INT_IP6, INT_PORT },
 			.accept_on	= SERVER_B,
 		},
+		{
+			.desc		= "UDP IPv6 redir and reuseport with conns",
+			.lookup_prog	= skel->progs.select_sock_a,
+			.reuseport_prog	= skel->progs.select_sock_b,
+			.sock_map	= skel->maps.redir_map,
+			.sotype		= SOCK_DGRAM,
+			.connect_to	= { EXT_IP6, EXT_PORT },
+			.listen_at	= { INT_IP6, INT_PORT },
+			.accept_on	= SERVER_B,
+			.reuseport_has_conns = true,
+		},
 		{
 			.desc		= "UDP IPv6 redir skip reuseport",
 			.lookup_prog	= skel->progs.select_sock_a_no_reuseport,
-- 
2.25.4

Re: [PATCH bpf-next 0/2] Fix BPF socket lookup with reuseport groups with connections

[Kuniyuki Iwashima]

From:   Jakub Sitnicki <jakub@cloudflare.com>
Date:   Wed, 22 Jul 2020 18:17:18 +0200
> This mini series contains a fix for a bug noticed when analyzing a reported
> merge conflict between bpf-next and net tree [0].
> 
> Apart from fixing a corner-case that affects use of BPF sk_lookup in tandem
> with UDP reuseport groups with connected sockets, it should make the
> conflict resolution with net tree easier.
> 
> These changes don't replicate the improved UDP socket lookup behavior from
> net tree, where commit efc6b6f6c311 ("udp: Improve load balancing for
> SO_REUSEPORT.") is present.
> 
> Happy to do it as a follow up. For the moment I didn't want to make things
> more confusing when it comes to what got fixed where and why.
> 
> Thanks,
> -jkbs

Acked-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>

Thank you.

Re: [PATCH bpf-next 0/2] Fix BPF socket lookup with reuseport groups with connections

[Alexei Starovoitov]

On Wed, Jul 22, 2020 at 9:59 AM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote:
>
> From:   Jakub Sitnicki <jakub@cloudflare.com>
> Date:   Wed, 22 Jul 2020 18:17:18 +0200
> > This mini series contains a fix for a bug noticed when analyzing a reported
> > merge conflict between bpf-next and net tree [0].
> >
> > Apart from fixing a corner-case that affects use of BPF sk_lookup in tandem
> > with UDP reuseport groups with connected sockets, it should make the
> > conflict resolution with net tree easier.
> >
> > These changes don't replicate the improved UDP socket lookup behavior from
> > net tree, where commit efc6b6f6c311 ("udp: Improve load balancing for
> > SO_REUSEPORT.") is present.
> >
> > Happy to do it as a follow up. For the moment I didn't want to make things
> > more confusing when it comes to what got fixed where and why.
> >
> > Thanks,
> > -jkbs
>
> Acked-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>

Applied to bpf-next. Thanks