diff mbox series

[bpf-next,3/8] bpf, sockmap: Allow inserting listening TCP sockets into SOCKMAP

Message ID 20191123110751.6729-4-jakub@cloudflare.com
State Changes Requested
Delegated to: BPF Maintainers
Headers show
Series Extend SOCKMAP to store listening sockets | expand

Commit Message

Jakub Sitnicki Nov. 23, 2019, 11:07 a.m. UTC 
In order for SOCKMAP type to become a generic collection for storing TCP
sockets we need to loosen the checks in update callback.

Currently SOCKMAP requires the TCP socket to be in established state, which
prevents us from using it to keep references to listening sockets.

Change the update pre-checks so that it is sufficient for socket to be in a
hash table, i.e. have a local address/port assigned, to be inserted. Return
-EINVAL if the condition is not met to be consistent with
REUSEPORT_SOCKARRY map type.

This creates a possibility of pointing one of the BPF redirect helpers that
splice two SOCKMAP sockets on ingress or egress at a listening socket,
which doesn't make sense. Introduce appropriate checks in the helpers so
that only established TCP sockets can be a target for redirects.

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 net/core/sock_map.c                     | 28 ++++++++++++++++++-------
 tools/testing/selftests/bpf/test_maps.c |  6 +-----
 2 files changed, 21 insertions(+), 13 deletions(-)

Comments

John Fastabend Nov. 24, 2019, 5:38 a.m. UTC | #1 
Jakub Sitnicki wrote:
> In order for SOCKMAP type to become a generic collection for storing TCP
> sockets we need to loosen the checks in update callback.
> 
> Currently SOCKMAP requires the TCP socket to be in established state, which
> prevents us from using it to keep references to listening sockets.
> 
> Change the update pre-checks so that it is sufficient for socket to be in a
> hash table, i.e. have a local address/port assigned, to be inserted. Return
> -EINVAL if the condition is not met to be consistent with
> REUSEPORT_SOCKARRY map type.
> 
> This creates a possibility of pointing one of the BPF redirect helpers that
> splice two SOCKMAP sockets on ingress or egress at a listening socket,
> which doesn't make sense. Introduce appropriate checks in the helpers so
> that only established TCP sockets can be a target for redirects.
> 
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> ---

Acked-by: John Fastabend <john.fastabend@gmail.co,>
diff mbox series

Patch

diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index 9f572d56e81a..49744b344137 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -439,11 +439,14 @@  static int sock_map_update_elem(struct bpf_map *map, void *key,
 		ret = -EINVAL;
 		goto out;
 	}
-	if (!sock_map_sk_is_suitable(sk) ||
-	    sk->sk_state != TCP_ESTABLISHED) {
+	if (!sock_map_sk_is_suitable(sk)) {
 		ret = -EOPNOTSUPP;
 		goto out;
 	}
+	if (!sk_hashed(sk)) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	sock_map_sk_acquire(sk);
 	ret = sock_map_update_common(map, idx, sk, flags);
@@ -480,13 +483,17 @@  BPF_CALL_4(bpf_sk_redirect_map, struct sk_buff *, skb,
 	   struct bpf_map *, map, u32, key, u64, flags)
 {
 	struct tcp_skb_cb *tcb = TCP_SKB_CB(skb);
+	struct sock *sk;
 
 	if (unlikely(flags & ~(BPF_F_INGRESS)))
 		return SK_DROP;
-	tcb->bpf.flags = flags;
-	tcb->bpf.sk_redir = __sock_map_lookup_elem(map, key);
-	if (!tcb->bpf.sk_redir)
+
+	sk = __sock_map_lookup_elem(map, key);
+	if (!sk || sk->sk_state != TCP_ESTABLISHED)
 		return SK_DROP;
+
+	tcb->bpf.flags = flags;
+	tcb->bpf.sk_redir = sk;
 	return SK_PASS;
 }
 
@@ -503,12 +510,17 @@  const struct bpf_func_proto bpf_sk_redirect_map_proto = {
 BPF_CALL_4(bpf_msg_redirect_map, struct sk_msg *, msg,
 	   struct bpf_map *, map, u32, key, u64, flags)
 {
+	struct sock *sk;
+
 	if (unlikely(flags & ~(BPF_F_INGRESS)))
 		return SK_DROP;
-	msg->flags = flags;
-	msg->sk_redir = __sock_map_lookup_elem(map, key);
-	if (!msg->sk_redir)
+
+	sk = __sock_map_lookup_elem(map, key);
+	if (!sk || sk->sk_state != TCP_ESTABLISHED)
 		return SK_DROP;
+
+	msg->flags = flags;
+	msg->sk_redir = sk;
 	return SK_PASS;
 }
 
diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c
index 02eae1e864c2..c6766b2cff85 100644
--- a/tools/testing/selftests/bpf/test_maps.c
+++ b/tools/testing/selftests/bpf/test_maps.c
@@ -756,11 +756,7 @@  static void test_sockmap(unsigned int tasks, void *data)
 	/* Test update without programs */
 	for (i = 0; i < 6; i++) {
 		err = bpf_map_update_elem(fd, &i, &sfd[i], BPF_ANY);
-		if (i < 2 && !err) {
-			printf("Allowed update sockmap '%i:%i' not in ESTABLISHED\n",
-			       i, sfd[i]);
-			goto out_sockmap;
-		} else if (i >= 2 && err) {
+		if (err) {
 			printf("Failed noprog update sockmap '%i:%i'\n",
 			       i, sfd[i]);
 			goto out_sockmap;