[RFC bpf-next 0/5] Extend SOCKMAP to store listening sockets

[RFC bpf-next 0/5] Extend SOCKMAP to store listening sockets

[Jakub Sitnicki]

This patch set is a follow up on a suggestion from LPC '19 discussions to
make SOCKMAP (or a new map type derived from it) a generic type for storing
established as well as listening sockets.

We found ourselves in need of a map type that keeps references to listening
sockets when working on making the socket lookup programmable, aka BPF
inet_lookup [1].  Initially we repurposed REUSEPORT_SOCKARRAY but found it
problematic to extend due to being tightly coupled with reuseport
logic (see slides [2]). So we've turned our attention to SOCKMAP instead.

As it turns out the changes needed to make SOCKMAP suitable for storing
listening sockets are self-contained and have use outside of programming
the socket lookup. Hence this patch set.

With these patches SOCKMAP can be used in SK_REUSEPORT BPF programs as a
drop-in replacement for REUSEPORT_SOCKARRAY for TCP. This can hopefully
lead to code consolidation between the two map types in the future.

Having said that, the main intention here is to lay groundwork for using
SOCKMAP in the next iteration of programmable socket lookup patches.

I'm looking for feedback if there's anything fundamentally wrong with
extending SOCKMAP map type like this that I might have missed.

Thanks,
Jakub

[1] https://lore.kernel.org/bpf/20190828072250.29828-1-jakub@cloudflare.com/
[2] https://linuxplumbersconf.org/event/4/contributions/487/attachments/238/417/Programmable_socket_lookup_LPC_19.pdf


Jakub Sitnicki (5):
  bpf, sockmap: Let BPF helpers use lookup operation on SOCKMAP
  bpf, sockmap: Allow inserting listening TCP sockets into SOCKMAP
  bpf, sockmap: Don't let child socket inherit psock or its ops on copy
  bpf: Allow selecting reuseport socket from a SOCKMAP
  selftests/bpf: Extend SK_REUSEPORT tests to cover SOCKMAP

 kernel/bpf/verifier.c                         |   6 +-
 net/core/sock_map.c                           |  11 +-
 net/ipv4/tcp_bpf.c                            |  30 ++++
 tools/testing/selftests/bpf/Makefile          |   7 +-
 .../selftests/bpf/test_select_reuseport.c     | 141 ++++++++++++++----
 .../selftests/bpf/test_select_reuseport.sh    |  14 ++
 6 files changed, 173 insertions(+), 36 deletions(-)
 create mode 100755 tools/testing/selftests/bpf/test_select_reuseport.sh

-- 
2.20.1

[RFC bpf-next 1/5] bpf, sockmap: Let BPF helpers use lookup operation on SOCKMAP

[Jakub Sitnicki]

Don't require the BPF helpers that need to access SOCKMAP maps to live in
the sock_map module. Expose SOCKMAP lookup to all kernel-land.

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 net/core/sock_map.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index eb114ee419b6..facacc296e6c 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -271,7 +271,9 @@ static struct sock *__sock_map_lookup_elem(struct bpf_map *map, u32 key)
 
 static void *sock_map_lookup(struct bpf_map *map, void *key)
 {
-	return ERR_PTR(-EOPNOTSUPP);
+	u32 index = *(u32 *)key;
+
+	return __sock_map_lookup_elem(map, index);
 }
 
 static int __sock_map_delete(struct bpf_stab *stab, struct sock *sk_test,
-- 
2.20.1

[RFC bpf-next 2/5] bpf, sockmap: Allow inserting listening TCP sockets into SOCKMAP

[Jakub Sitnicki]

In order for SOCKMAP type to become a generic collection for storing socket
references 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, to be inserted.

Return -EINVAL if the condition is not met to be consistent with
REUSEPORT_SOCKARRY map type.

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 net/core/sock_map.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index facacc296e6c..222036393b90 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -415,11 +415,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);
-- 
2.20.1

[RFC bpf-next 3/5] bpf, sockmap: Don't let child socket inherit psock or its ops on copy

[Jakub Sitnicki]

New sockets cloned from listening sockets that are in a sockmap must not
inherit the psock that has the link to the sockmap. Otherwise child sockets
unintentionally share the sockmap entry with the listening socket, which
leads to double-free on socket close.

Prevent it by overloading the accept callback. In it we restore the
protocol and write buffer callbacks and clear the pointer to psock.

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 net/ipv4/tcp_bpf.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
index 8a56e09cfb0e..5838aaba4ce0 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c
@@ -582,6 +582,35 @@ static void tcp_bpf_close(struct sock *sk, long timeout)
 	saved_close(sk, timeout);
 }
 
+static struct sock *tcp_bpf_accept(struct sock *sk, int flags, int *err,
+				   bool kern)
+{
+	void (*saved_write_space)(struct sock *sk);
+	struct proto *saved_proto;
+	struct sk_psock *psock;
+	struct sock *child;
+
+	rcu_read_lock();
+	psock = sk_psock(sk);
+	if (unlikely(!psock)) {
+		rcu_read_unlock();
+		return sk->sk_prot->accept(sk, flags, err, kern);
+	}
+	saved_proto = psock->sk_proto;
+	saved_write_space = psock->saved_write_space;
+	rcu_read_unlock();
+
+	child = saved_proto->accept(sk, flags, err, kern);
+	if (!child)
+		return NULL;
+
+	/* Child must not inherit psock or its ops. */
+	rcu_assign_sk_user_data(child, NULL);
+	child->sk_prot = saved_proto;
+	child->sk_write_space = saved_write_space;
+	return child;
+}
+
 enum {
 	TCP_BPF_IPV4,
 	TCP_BPF_IPV6,
@@ -606,6 +635,7 @@ static void tcp_bpf_rebuild_protos(struct proto prot[TCP_BPF_NUM_CFGS],
 	prot[TCP_BPF_BASE].close		= tcp_bpf_close;
 	prot[TCP_BPF_BASE].recvmsg		= tcp_bpf_recvmsg;
 	prot[TCP_BPF_BASE].stream_memory_read	= tcp_bpf_stream_read;
+	prot[TCP_BPF_BASE].accept		= tcp_bpf_accept;
 
 	prot[TCP_BPF_TX]			= prot[TCP_BPF_BASE];
 	prot[TCP_BPF_TX].sendmsg		= tcp_bpf_sendmsg;
-- 
2.20.1

[RFC bpf-next 4/5] bpf: Allow selecting reuseport socket from a SOCKMAP

[Jakub Sitnicki]

SOCKMAP now supports storing references to listening sockets. Nothing
keeps us from using it as array of sockets to select from in reuseport
programs. Whitelist the map type with the socket selection helper.

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 kernel/bpf/verifier.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 556e82f8869b..77319248e357 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3648,7 +3648,8 @@ static int check_map_func_compatibility(struct bpf_verifier_env *env,
 		if (func_id != BPF_FUNC_sk_redirect_map &&
 		    func_id != BPF_FUNC_sock_map_update &&
 		    func_id != BPF_FUNC_map_delete_elem &&
-		    func_id != BPF_FUNC_msg_redirect_map)
+		    func_id != BPF_FUNC_msg_redirect_map &&
+		    func_id != BPF_FUNC_sk_select_reuseport)
 			goto error;
 		break;
 	case BPF_MAP_TYPE_SOCKHASH:
@@ -3729,7 +3730,8 @@ static int check_map_func_compatibility(struct bpf_verifier_env *env,
 			goto error;
 		break;
 	case BPF_FUNC_sk_select_reuseport:
-		if (map->map_type != BPF_MAP_TYPE_REUSEPORT_SOCKARRAY)
+		if (map->map_type != BPF_MAP_TYPE_REUSEPORT_SOCKARRAY &&
+		    map->map_type != BPF_MAP_TYPE_SOCKMAP)
 			goto error;
 		break;
 	case BPF_FUNC_map_peek_elem:
-- 
2.20.1

[RFC bpf-next 5/5] selftests/bpf: Extend SK_REUSEPORT tests to cover SOCKMAP

[Jakub Sitnicki]

Parametrize the SK_REUSEPORT tests so that the map type for storing sockets
can be selected at run-time. Also allow choosing which L4 protocols get
tested.

Run the extended reuseport program test two times, once for
REUSEPORT_ARRAY, and once for SOCKMAP but just with TCP to cover the newly
enabled map type.

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 tools/testing/selftests/bpf/Makefile          |   7 +-
 .../selftests/bpf/test_select_reuseport.c     | 141 ++++++++++++++----
 .../selftests/bpf/test_select_reuseport.sh    |  14 ++
 3 files changed, 131 insertions(+), 31 deletions(-)
 create mode 100755 tools/testing/selftests/bpf/test_select_reuseport.sh

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 4ff5f4aada08..a32646f2e80a 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -28,7 +28,7 @@ LDLIBS += -lcap -lelf -lrt -lpthread
 TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test_progs \
 	test_align test_verifier_log test_dev_cgroup test_tcpbpf_user \
 	test_sock test_btf test_sockmap get_cgroup_id_user test_socket_cookie \
-	test_cgroup_storage test_select_reuseport test_section_names \
+	test_cgroup_storage test_section_names \
 	test_netcnt test_tcpnotify_user test_sock_fields test_sysctl test_hashmap \
 	test_cgroup_attach xdping
 
@@ -69,7 +69,8 @@ TEST_PROGS := test_kmod.sh \
 	test_tc_tunnel.sh \
 	test_tc_edt.sh \
 	test_xdping.sh \
-	test_bpftool_build.sh
+	test_bpftool_build.sh \
+	test_select_reuseport.sh
 
 TEST_PROGS_EXTENDED := with_addr.sh \
 	with_tunnels.sh \
@@ -80,7 +81,7 @@ TEST_PROGS_EXTENDED := with_addr.sh \
 # Compile but not part of 'make run_tests'
 TEST_GEN_PROGS_EXTENDED = test_sock_addr test_skb_cgroup_id_user \
 	flow_dissector_load test_flow_dissector test_tcp_check_syncookie_user \
-	test_lirc_mode2_user
+	test_lirc_mode2_user test_select_reuseport
 
 TEST_CUSTOM_PROGS = urandom_read
 
diff --git a/tools/testing/selftests/bpf/test_select_reuseport.c b/tools/testing/selftests/bpf/test_select_reuseport.c
index 7566c13eb51a..732cfeee189f 100644
--- a/tools/testing/selftests/bpf/test_select_reuseport.c
+++ b/tools/testing/selftests/bpf/test_select_reuseport.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright (c) 2018 Facebook */
 
+#define _GNU_SOURCE
 #include <stdlib.h>
 #include <unistd.h>
 #include <stdbool.h>
@@ -29,6 +30,12 @@
 #define TCP_FO_SYSCTL "/proc/sys/net/ipv4/tcp_fastopen"
 #define REUSEPORT_ARRAY_SIZE 32
 
+#define BIND_TO_INANY		true
+#define BIND_TO_LOOPBACK	(!BIND_TO_INANY)
+
+static enum bpf_map_type cfg_map_type = BPF_MAP_TYPE_REUSEPORT_SOCKARRAY;
+static unsigned int cfg_sock_types = (1 << SOCK_STREAM) | (1 << SOCK_DGRAM);
+
 static int result_map, tmp_index_ovr_map, linum_map, data_check_map;
 static enum result expected_results[NR_RESULTS];
 static int sk_fds[REUSEPORT_ARRAY_SIZE];
@@ -61,7 +68,7 @@ static void create_maps(void)
 
 	/* Creating reuseport_array */
 	attr.name = "reuseport_array";
-	attr.map_type = BPF_MAP_TYPE_REUSEPORT_SOCKARRAY;
+	attr.map_type = cfg_map_type;
 	attr.key_size = sizeof(__u32);
 	attr.value_size = sizeof(__u32);
 	attr.max_entries = REUSEPORT_ARRAY_SIZE;
@@ -680,53 +687,131 @@ static void cleanup(void)
 	bpf_object__close(obj);
 }
 
+static const char *family_to_str(int family)
+{
+	switch (family) {
+	case AF_INET:
+		return "IPv4";
+	case AF_INET6:
+		return "IPv6";
+	default:
+		return "unknown";
+	}
+}
+
+static const char *type_to_str(int type)
+{
+	switch (type) {
+	case SOCK_STREAM:
+		return "TCP";
+	case SOCK_DGRAM:
+		return "UDP";
+	default:
+		return "unknown";
+	}
+}
+
+static void test_one(int family, int type, bool inany)
+{
+	int err;
+
+	printf("######## %s/%s %-8s ########\n",
+	       family_to_str(family), type_to_str(type),
+	       inany ? "INANY" : "LOOPBACK");
+
+	setup_per_test(type, family, inany);
+
+	test_err_inner_map(type, family);
+
+	/* Install reuseport_array to the outer_map */
+	err = bpf_map_update_elem(outer_map, &index_zero, &reuseport_array,
+				  BPF_ANY);
+	CHECK(err == -1, "update_elem(outer_map)",
+	      "err:%d errno:%d\n", err, errno);
+
+	test_err_skb_data(type, family);
+	test_err_sk_select_port(type, family);
+	test_pass(type, family);
+	test_syncookie(type, family);
+	test_pass_on_err(type, family);
+	/* Must be the last test */
+	test_detach_bpf(type, family);
+
+	cleanup_per_test();
+	printf("\n");
+}
+
 static void test_all(void)
 {
-	/* Extra SOCK_STREAM to test bind_inany==true */
-	const int types[] = { SOCK_STREAM, SOCK_DGRAM, SOCK_STREAM };
-	const char * const type_strings[] = { "TCP", "UDP", "TCP" };
-	const char * const family_strings[] = { "IPv6", "IPv4" };
+	const int types[] = { SOCK_STREAM, SOCK_DGRAM };
 	const unsigned short families[] = { AF_INET6, AF_INET };
-	const bool bind_inany[] = { false, false, true };
-	int t, f, err;
+	int t, f;
 
 	for (f = 0; f < ARRAY_SIZE(families); f++) {
 		unsigned short family = families[f];
 
 		for (t = 0; t < ARRAY_SIZE(types); t++) {
-			bool inany = bind_inany[t];
 			int type = types[t];
 
-			printf("######## %s/%s %s ########\n",
-			       family_strings[f], type_strings[t],
-				inany ? " INANY  " : "LOOPBACK");
+			/* Socket type excluded from tests? */
+			if (~cfg_sock_types & (1 << type))
+				continue;
 
-			setup_per_test(type, family, inany);
+			test_one(family, type, BIND_TO_LOOPBACK);
+			test_one(family, type, BIND_TO_INANY);
+		}
+	}
+}
 
-			test_err_inner_map(type, family);
+static void __attribute__((noreturn)) usage(void)
+{
+	fprintf(stderr,
+		"Usage: %s [-m reuseport_sockarray|sockmap] [-t] [-u]\n",
+		program_invocation_short_name);
+	exit(1);
+}
 
-			/* Install reuseport_array to the outer_map */
-			err = bpf_map_update_elem(outer_map, &index_zero,
-						  &reuseport_array, BPF_ANY);
-			CHECK(err == -1, "update_elem(outer_map)",
-			      "err:%d errno:%d\n", err, errno);
+static enum bpf_map_type parse_map_type(const char *optarg)
+{
+	if (!strcmp(optarg, "reuseport_sockarray"))
+		return BPF_MAP_TYPE_REUSEPORT_SOCKARRAY;
+	if (!strcmp(optarg, "sockmap"))
+		return BPF_MAP_TYPE_SOCKMAP;
 
-			test_err_skb_data(type, family);
-			test_err_sk_select_port(type, family);
-			test_pass(type, family);
-			test_syncookie(type, family);
-			test_pass_on_err(type, family);
-			/* Must be the last test */
-			test_detach_bpf(type, family);
+	return BPF_MAP_TYPE_UNSPEC;
+}
 
-			cleanup_per_test();
-			printf("\n");
+static void parse_opts(int argc, char **argv)
+{
+	unsigned int sock_types = 0;
+	int c;
+
+	while ((c = getopt(argc, argv, "hm:tu")) != -1) {
+		switch (c) {
+		case 'h':
+			usage();
+			break;
+		case 'm':
+			cfg_map_type = parse_map_type(optarg);
+			break;
+		case 't':
+			sock_types |= 1 << SOCK_STREAM;
+			break;
+		case 'u':
+			sock_types |= 1 << SOCK_DGRAM;
+			break;
 		}
 	}
+
+	if (cfg_map_type == BPF_MAP_TYPE_UNSPEC)
+		usage();
+	if (sock_types != 0)
+		cfg_sock_types = sock_types;
 }
 
-int main(int argc, const char **argv)
+int main(int argc, char **argv)
 {
+	parse_opts(argc, argv);
 	create_maps();
 	prepare_bpf_obj();
 	saved_tcp_fo = read_int_sysctl(TCP_FO_SYSCTL);
diff --git a/tools/testing/selftests/bpf/test_select_reuseport.sh b/tools/testing/selftests/bpf/test_select_reuseport.sh
new file mode 100755
index 000000000000..1951b4886021
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_select_reuseport.sh
@@ -0,0 +1,14 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+set -eu
+
+DIR=$(dirname $0)
+
+echo "Testing reuseport with REUSEPORT_SOCKARRAY..."
+$DIR/test_select_reuseport -m reuseport_sockarray
+
+echo "Testing reuseport with SOCKMAP (TCP only)..."
+$DIR/test_select_reuseport -m sockmap -t
+
+exit 0
-- 
2.20.1

Re: [RFC bpf-next 0/5] Extend SOCKMAP to store listening sockets

[Alexei Starovoitov]

On Tue, Oct 22, 2019 at 01:37:25PM +0200, Jakub Sitnicki wrote:
> This patch set is a follow up on a suggestion from LPC '19 discussions to
> make SOCKMAP (or a new map type derived from it) a generic type for storing
> established as well as listening sockets.
> 
> We found ourselves in need of a map type that keeps references to listening
> sockets when working on making the socket lookup programmable, aka BPF
> inet_lookup [1].  Initially we repurposed REUSEPORT_SOCKARRAY but found it
> problematic to extend due to being tightly coupled with reuseport
> logic (see slides [2]). So we've turned our attention to SOCKMAP instead.
> 
> As it turns out the changes needed to make SOCKMAP suitable for storing
> listening sockets are self-contained and have use outside of programming
> the socket lookup. Hence this patch set.
> 
> With these patches SOCKMAP can be used in SK_REUSEPORT BPF programs as a
> drop-in replacement for REUSEPORT_SOCKARRAY for TCP. This can hopefully
> lead to code consolidation between the two map types in the future.
> 
> Having said that, the main intention here is to lay groundwork for using
> SOCKMAP in the next iteration of programmable socket lookup patches.
> 
> I'm looking for feedback if there's anything fundamentally wrong with
> extending SOCKMAP map type like this that I might have missed.

John, Martin,
comments?

RE: [RFC bpf-next 0/5] Extend SOCKMAP to store listening sockets

[John Fastabend]

Jakub Sitnicki wrote:
> This patch set is a follow up on a suggestion from LPC '19 discussions to
> make SOCKMAP (or a new map type derived from it) a generic type for storing
> established as well as listening sockets.
> 
> We found ourselves in need of a map type that keeps references to listening
> sockets when working on making the socket lookup programmable, aka BPF
> inet_lookup [1].  Initially we repurposed REUSEPORT_SOCKARRAY but found it
> problematic to extend due to being tightly coupled with reuseport
> logic (see slides [2]). So we've turned our attention to SOCKMAP instead.
> 
> As it turns out the changes needed to make SOCKMAP suitable for storing
> listening sockets are self-contained and have use outside of programming
> the socket lookup. Hence this patch set.
> 
> With these patches SOCKMAP can be used in SK_REUSEPORT BPF programs as a
> drop-in replacement for REUSEPORT_SOCKARRAY for TCP. This can hopefully
> lead to code consolidation between the two map types in the future.
> 
> Having said that, the main intention here is to lay groundwork for using
> SOCKMAP in the next iteration of programmable socket lookup patches.
> 
> I'm looking for feedback if there's anything fundamentally wrong with
> extending SOCKMAP map type like this that I might have missed.

I think this looks good. The main reason I blocked it off before is mostly
because I had no use-case for it and the complication with what to do with
child sockets. Clearing the psock state seems OK to me if user wants to
add it back to a map they can simply grab it again from a sockops event.

By the way I would eventually like to see the lookup hook return the
correct type (PTR_TO_SOCKET_OR_NULL) so that the verifier "knows" the type
and the socket can be used the same as if it was pulled from a sk_lookup
helper.

> 
> Thanks,
> Jakub
> 
> [1] https://lore.kernel.org/bpf/20190828072250.29828-1-jakub@cloudflare.com/
> [2] https://linuxplumbersconf.org/event/4/contributions/487/attachments/238/417/Programmable_socket_lookup_LPC_19.pdf
> 

RE: [RFC bpf-next 1/5] bpf, sockmap: Let BPF helpers use lookup operation on SOCKMAP

[John Fastabend]

Jakub Sitnicki wrote:
> Don't require the BPF helpers that need to access SOCKMAP maps to live in
> the sock_map module. Expose SOCKMAP lookup to all kernel-land.
> 
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> ---
>  net/core/sock_map.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> index eb114ee419b6..facacc296e6c 100644
> --- a/net/core/sock_map.c
> +++ b/net/core/sock_map.c
> @@ -271,7 +271,9 @@ static struct sock *__sock_map_lookup_elem(struct bpf_map *map, u32 key)
>  
>  static void *sock_map_lookup(struct bpf_map *map, void *key)
>  {
> -	return ERR_PTR(-EOPNOTSUPP);
> +	u32 index = *(u32 *)key;
> +
> +	return __sock_map_lookup_elem(map, index);
>  }
>  
>  static int __sock_map_delete(struct bpf_stab *stab, struct sock *sk_test,
> -- 
> 2.20.1
> 

OK, I'm looking into the latest BTF bits to see if we can do something
more useful here to keep the type information when the lookup is done so
the sock can be feed from sk_lookup and actually read.

As this series stands after the lookup its just an opaque ptr_to_map_value
right?

RE: [RFC bpf-next 2/5] bpf, sockmap: Allow inserting listening TCP sockets into SOCKMAP

[John Fastabend]

Jakub Sitnicki wrote:
> In order for SOCKMAP type to become a generic collection for storing socket
> references 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, to be inserted.
> 
> Return -EINVAL if the condition is not met to be consistent with
> REUSEPORT_SOCKARRY map type.
> 
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> ---

We need to also have some tests then to verify redirecting to this listen socket
does the correct thing. Once its in the map we can redirect (ingress or egress)
to it and need to be sure the semantics are sane.

>  net/core/sock_map.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> index facacc296e6c..222036393b90 100644
> --- a/net/core/sock_map.c
> +++ b/net/core/sock_map.c
> @@ -415,11 +415,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);
> -- 
> 2.20.1
> 

Re: [RFC bpf-next 0/5] Extend SOCKMAP to store listening sockets

[Jakub Sitnicki]

On Thu, Oct 24, 2019 at 06:56 PM CEST, John Fastabend wrote:
> Jakub Sitnicki wrote:

[...]

>> I'm looking for feedback if there's anything fundamentally wrong with
>> extending SOCKMAP map type like this that I might have missed.
>
> I think this looks good. The main reason I blocked it off before is mostly
> because I had no use-case for it and the complication with what to do with
> child sockets. Clearing the psock state seems OK to me if user wants to
> add it back to a map they can simply grab it again from a sockops
> event.

Thanks for taking a look at the code.

> By the way I would eventually like to see the lookup hook return the
> correct type (PTR_TO_SOCKET_OR_NULL) so that the verifier "knows" the type
> and the socket can be used the same as if it was pulled from a sk_lookup
> helper.

Wait... you had me scratching my head there for a minute.

I haven't whitelisted bpf_map_lookup_elem for SOCKMAP in
check_map_func_compatibility so verifier won't allow lookups from BPF.

If we wanted to do that, I don't actually have a use-case for it, I
think would have to extend get_func_proto for SK_SKB and SK_REUSEPORT
prog types. At least that's what docs for bpf_map_lookup_elem suggest:

/* If kernel subsystem is allowing eBPF programs to call this function,
 * inside its own verifier_ops->get_func_proto() callback it should return
 * bpf_map_lookup_elem_proto, so that verifier can properly check the arguments
 *
 * Different map implementations will rely on rcu in map methods
 * lookup/update/delete, therefore eBPF programs must run under rcu lock
 * if program is allowed to access maps, so check rcu_read_lock_held in
 * all three functions.
 */
BPF_CALL_2(bpf_map_lookup_elem, struct bpf_map *, map, void *, key)
{
	WARN_ON_ONCE(!rcu_read_lock_held());
	return (unsigned long) map->ops->map_lookup_elem(map, key);
}

-Jakub

Re: [RFC bpf-next 2/5] bpf, sockmap: Allow inserting listening TCP sockets into SOCKMAP

[Jakub Sitnicki]

On Thu, Oct 24, 2019 at 07:06 PM CEST, John Fastabend wrote:
> Jakub Sitnicki wrote:
>> In order for SOCKMAP type to become a generic collection for storing socket
>> references 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, to be inserted.
>>
>> Return -EINVAL if the condition is not met to be consistent with
>> REUSEPORT_SOCKARRY map type.
>>
>> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
>> ---
>
> We need to also have some tests then to verify redirecting to this listen socket
> does the correct thing. Once its in the map we can redirect (ingress or egress)
> to it and need to be sure the semantics are sane.

You're right. The redirect BPF helpers that operate on SOCMAP might be
relying on an assumption that sockets are in established state. I need
look into that.

Thanks,
Jakub

Re: [RFC bpf-next 0/5] Extend SOCKMAP to store listening sockets

[John Fastabend]

Jakub Sitnicki wrote:
> On Thu, Oct 24, 2019 at 06:56 PM CEST, John Fastabend wrote:
> > Jakub Sitnicki wrote:
> 
> [...]
> 
> >> I'm looking for feedback if there's anything fundamentally wrong with
> >> extending SOCKMAP map type like this that I might have missed.
> >
> > I think this looks good. The main reason I blocked it off before is mostly
> > because I had no use-case for it and the complication with what to do with
> > child sockets. Clearing the psock state seems OK to me if user wants to
> > add it back to a map they can simply grab it again from a sockops
> > event.
> 
> Thanks for taking a look at the code.
> 
> > By the way I would eventually like to see the lookup hook return the
> > correct type (PTR_TO_SOCKET_OR_NULL) so that the verifier "knows" the type
> > and the socket can be used the same as if it was pulled from a sk_lookup
> > helper.
> 
> Wait... you had me scratching my head there for a minute.
> 
> I haven't whitelisted bpf_map_lookup_elem for SOCKMAP in
> check_map_func_compatibility so verifier won't allow lookups from BPF.
> 
> If we wanted to do that, I don't actually have a use-case for it, I
> think would have to extend get_func_proto for SK_SKB and SK_REUSEPORT
> prog types. At least that's what docs for bpf_map_lookup_elem suggest:

Right, so its not required for your series just letting you know I will
probably look to do this shortly. It would be useful for some use cases
we have.

> 
> /* If kernel subsystem is allowing eBPF programs to call this function,
>  * inside its own verifier_ops->get_func_proto() callback it should return
>  * bpf_map_lookup_elem_proto, so that verifier can properly check the arguments
>  *
>  * Different map implementations will rely on rcu in map methods
>  * lookup/update/delete, therefore eBPF programs must run under rcu lock
>  * if program is allowed to access maps, so check rcu_read_lock_held in
>  * all three functions.
>  */
> BPF_CALL_2(bpf_map_lookup_elem, struct bpf_map *, map, void *, key)
> {
> 	WARN_ON_ONCE(!rcu_read_lock_held());
> 	return (unsigned long) map->ops->map_lookup_elem(map, key);
> }
> 
> -Jakub

Re: [RFC bpf-next 0/5] Extend SOCKMAP to store listening sockets

[Martin Lau]

On Tue, Oct 22, 2019 at 01:37:25PM +0200, Jakub Sitnicki wrote:
> This patch set is a follow up on a suggestion from LPC '19 discussions to
> make SOCKMAP (or a new map type derived from it) a generic type for storing
> established as well as listening sockets.
> 
> We found ourselves in need of a map type that keeps references to listening
> sockets when working on making the socket lookup programmable, aka BPF
> inet_lookup [1].  Initially we repurposed REUSEPORT_SOCKARRAY but found it
> problematic to extend due to being tightly coupled with reuseport
> logic (see slides [2]).
> So we've turned our attention to SOCKMAP instead.
> 
> As it turns out the changes needed to make SOCKMAP suitable for storing
> listening sockets are self-contained and have use outside of programming
> the socket lookup. Hence this patch set.
> 
> With these patches SOCKMAP can be used in SK_REUSEPORT BPF programs as a
> drop-in replacement for REUSEPORT_SOCKARRAY for TCP. This can hopefully
> lead to code consolidation between the two map types in the future.
What is the plan for UDP support in sockmap?

> 
> Having said that, the main intention here is to lay groundwork for using
> SOCKMAP in the next iteration of programmable socket lookup patches.
What may be the minimal to get only lookup work for UDP sockmap?
.close() and .unhash()?

> 
> I'm looking for feedback if there's anything fundamentally wrong with
> extending SOCKMAP map type like this that I might have missed.
> 
> Thanks,
> Jakub
> 
> [1] https://lore.kernel.org/bpf/20190828072250.29828-1-jakub@cloudflare.com/
> [2] https://urldefense.proofpoint.com/v2/url?u=https-3A__linuxplumbersconf.org_event_4_contributions_487_attachments_238_417_Programmable-5Fsocket-5Flookup-5FLPC-5F19.pdf&d=DwIDAg&c=5VD0RTtNlTh3ycd41b3MUw&r=VQnoQ7LvghIj0gVEaiQSUw&m=Y-Ap1QuRBsqsu8gATb1wH3rPT89No2mam2qINt1BGDI&s=_sfXVfJhB2eR7znE7-WBk660dQXIBxuDLRi7jvXVpsg&e= 
> 
> 
> Jakub Sitnicki (5):
>   bpf, sockmap: Let BPF helpers use lookup operation on SOCKMAP
>   bpf, sockmap: Allow inserting listening TCP sockets into SOCKMAP
>   bpf, sockmap: Don't let child socket inherit psock or its ops on copy
>   bpf: Allow selecting reuseport socket from a SOCKMAP
>   selftests/bpf: Extend SK_REUSEPORT tests to cover SOCKMAP
> 
>  kernel/bpf/verifier.c                         |   6 +-
>  net/core/sock_map.c                           |  11 +-
>  net/ipv4/tcp_bpf.c                            |  30 ++++
>  tools/testing/selftests/bpf/Makefile          |   7 +-
>  .../selftests/bpf/test_select_reuseport.c     | 141 ++++++++++++++----
>  .../selftests/bpf/test_select_reuseport.sh    |  14 ++
>  6 files changed, 173 insertions(+), 36 deletions(-)
>  create mode 100755 tools/testing/selftests/bpf/test_select_reuseport.sh
> 
> -- 
> 2.20.1
> 

Re: [RFC bpf-next 0/5] Extend SOCKMAP to store listening sockets

[Jakub Sitnicki]

On Mon, Oct 28, 2019 at 06:52 AM CET, Martin Lau wrote:
> On Tue, Oct 22, 2019 at 01:37:25PM +0200, Jakub Sitnicki wrote:
>> This patch set is a follow up on a suggestion from LPC '19 discussions to
>> make SOCKMAP (or a new map type derived from it) a generic type for storing
>> established as well as listening sockets.
>>
>> We found ourselves in need of a map type that keeps references to listening
>> sockets when working on making the socket lookup programmable, aka BPF
>> inet_lookup [1].  Initially we repurposed REUSEPORT_SOCKARRAY but found it
>> problematic to extend due to being tightly coupled with reuseport
>> logic (see slides [2]).
>> So we've turned our attention to SOCKMAP instead.
>>
>> As it turns out the changes needed to make SOCKMAP suitable for storing
>> listening sockets are self-contained and have use outside of programming
>> the socket lookup. Hence this patch set.
>>
>> With these patches SOCKMAP can be used in SK_REUSEPORT BPF programs as a
>> drop-in replacement for REUSEPORT_SOCKARRAY for TCP. This can hopefully
>> lead to code consolidation between the two map types in the future.
> What is the plan for UDP support in sockmap?

It's on our road-map because without SOCKMAP support for UDP we won't be
able to move away from TPROXY [1] and custom SO_BINDTOPREFIX extension
[2] for steering new UDP flows to receiving sockets. Also we would like
to look into using SOCKMAP for connected UDP socket splicing in the
future [3].

I was planning to split work as follows:

1. SOCKMAP support for listening sockets (this series)
2. programmable socket lookup for TCP (cut-down version of [4])
3. SOCKMAP support for UDP (work not started)
4. programmable socket lookup for UDP (rest of [4])

I'm open to suggestions on how to organize it.

>> Having said that, the main intention here is to lay groundwork for using
>> SOCKMAP in the next iteration of programmable socket lookup patches.
> What may be the minimal to get only lookup work for UDP sockmap?
> .close() and .unhash()?

John would know better. I haven't tried doing it yet.

From just reading the code - override the two proto ops you mentioned,
close and unhash, and adapt the socket checks in SOCKMAP.

-Jakub

[1] https://blog.cloudflare.com/how-we-built-spectrum/
[2] https://lore.kernel.org/netdev/1458699966-3752-1-git-send-email-gilberto.bertin@gmail.com/
[3] https://lore.kernel.org/bpf/20190828072250.29828-1-jakub@cloudflare.com/
[4] https://blog.cloudflare.com/sockmap-tcp-splicing-of-the-future/

Re: [RFC bpf-next 0/5] Extend SOCKMAP to store listening sockets

[John Fastabend]

Jakub Sitnicki wrote:
> On Mon, Oct 28, 2019 at 06:52 AM CET, Martin Lau wrote:
> > On Tue, Oct 22, 2019 at 01:37:25PM +0200, Jakub Sitnicki wrote:
> >> This patch set is a follow up on a suggestion from LPC '19 discussions to
> >> make SOCKMAP (or a new map type derived from it) a generic type for storing
> >> established as well as listening sockets.
> >>
> >> We found ourselves in need of a map type that keeps references to listening
> >> sockets when working on making the socket lookup programmable, aka BPF
> >> inet_lookup [1].  Initially we repurposed REUSEPORT_SOCKARRAY but found it
> >> problematic to extend due to being tightly coupled with reuseport
> >> logic (see slides [2]).
> >> So we've turned our attention to SOCKMAP instead.
> >>
> >> As it turns out the changes needed to make SOCKMAP suitable for storing
> >> listening sockets are self-contained and have use outside of programming
> >> the socket lookup. Hence this patch set.
> >>
> >> With these patches SOCKMAP can be used in SK_REUSEPORT BPF programs as a
> >> drop-in replacement for REUSEPORT_SOCKARRAY for TCP. This can hopefully
> >> lead to code consolidation between the two map types in the future.
> > What is the plan for UDP support in sockmap?
> 
> It's on our road-map because without SOCKMAP support for UDP we won't be
> able to move away from TPROXY [1] and custom SO_BINDTOPREFIX extension
> [2] for steering new UDP flows to receiving sockets. Also we would like
> to look into using SOCKMAP for connected UDP socket splicing in the
> future [3].
> 
> I was planning to split work as follows:
> 
> 1. SOCKMAP support for listening sockets (this series)
> 2. programmable socket lookup for TCP (cut-down version of [4])
> 3. SOCKMAP support for UDP (work not started)
> 4. programmable socket lookup for UDP (rest of [4])
> 
> I'm open to suggestions on how to organize it.

Looks good to me. I've had UDP support on my todo list for awhile now
but it hasn't got to the top yet so glad to see this.

Also perhaps not necessary for your work but I have some patches on my
stack I'll try to get out soon to get ktls + receive hooks working.

> 
> >> Having said that, the main intention here is to lay groundwork for using
> >> SOCKMAP in the next iteration of programmable socket lookup patches.
> > What may be the minimal to get only lookup work for UDP sockmap?
> > .close() and .unhash()?
> 
> John would know better. I haven't tried doing it yet.

Right, I don't think its too complicated we just need the hooks and then
to be sure the socket state checks are OK. Having listening support should
help with the UDP case.

> 
> From just reading the code - override the two proto ops you mentioned,
> close and unhash, and adapt the socket checks in SOCKMAP.

+1.

> 
> -Jakub
> 
> [1] https://blog.cloudflare.com/how-we-built-spectrum/
> [2] https://lore.kernel.org/netdev/1458699966-3752-1-git-send-email-gilberto.bertin@gmail.com/
> [3] https://lore.kernel.org/bpf/20190828072250.29828-1-jakub@cloudflare.com/
> [4] https://blog.cloudflare.com/sockmap-tcp-splicing-of-the-future/

Re: [RFC bpf-next 0/5] Extend SOCKMAP to store listening sockets

[Martin Lau]

On Mon, Oct 28, 2019 at 01:35:26PM +0100, Jakub Sitnicki wrote:
> On Mon, Oct 28, 2019 at 06:52 AM CET, Martin Lau wrote:
> > On Tue, Oct 22, 2019 at 01:37:25PM +0200, Jakub Sitnicki wrote:
> >> This patch set is a follow up on a suggestion from LPC '19 discussions to
> >> make SOCKMAP (or a new map type derived from it) a generic type for storing
> >> established as well as listening sockets.
> >>
> >> We found ourselves in need of a map type that keeps references to listening
> >> sockets when working on making the socket lookup programmable, aka BPF
> >> inet_lookup [1].  Initially we repurposed REUSEPORT_SOCKARRAY but found it
> >> problematic to extend due to being tightly coupled with reuseport
> >> logic (see slides [2]).
> >> So we've turned our attention to SOCKMAP instead.
> >>
> >> As it turns out the changes needed to make SOCKMAP suitable for storing
> >> listening sockets are self-contained and have use outside of programming
> >> the socket lookup. Hence this patch set.
> >>
> >> With these patches SOCKMAP can be used in SK_REUSEPORT BPF programs as a
> >> drop-in replacement for REUSEPORT_SOCKARRAY for TCP. This can hopefully
> >> lead to code consolidation between the two map types in the future.
> > What is the plan for UDP support in sockmap?
> 
> It's on our road-map because without SOCKMAP support for UDP we won't be
> able to move away from TPROXY [1] and custom SO_BINDTOPREFIX extension
> [2] for steering new UDP flows to receiving sockets. Also we would like
> to look into using SOCKMAP for connected UDP socket splicing in the
> future [3].
> 
> I was planning to split work as follows:
> 
> 1. SOCKMAP support for listening sockets (this series)
> 2. programmable socket lookup for TCP (cut-down version of [4])
> 3. SOCKMAP support for UDP (work not started)
hmm...It is hard to comment how the full UDP sockmap may
work out without a code attempt because I am not fluent in
sock_map ;)

From a quick look, it seems there are quite a few things to do.
For example, the TCP_SKB_CB(skb) usage and how that may look
like in UDP.  "struct udp_skb_cb" is 28 bytes while "struct napi_gro_cb"
seems to be 48 bytes already which may need a closer look.

> 4. programmable socket lookup for UDP (rest of [4])
> 
> I'm open to suggestions on how to organize it.
> 
> >> Having said that, the main intention here is to lay groundwork for using
> >> SOCKMAP in the next iteration of programmable socket lookup patches.
> > What may be the minimal to get only lookup work for UDP sockmap?
> > .close() and .unhash()?
> 
> John would know better. I haven't tried doing it yet.
> 
> From just reading the code - override the two proto ops you mentioned,
> close and unhash, and adapt the socket checks in SOCKMAP.
Do your use cases need bpf prog attached to sock_map?

If not, would it be cleaner to delicate another map_type
for lookup-only use case to have both TCP and UDP support.

> 
> -Jakub
> 
> [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__blog.cloudflare.com_how-2Dwe-2Dbuilt-2Dspectrum_&d=DwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=VQnoQ7LvghIj0gVEaiQSUw&m=lSo-FsOeNl_8znZZ07H8I6ZYAinPKTR5C3Cn_Ol3QYQ&s=DZgW8-2Xl1P8NU59ji4ieQLzwWpx4t3gGq_tqB0l3Bo&e= 
> [2] https://lore.kernel.org/netdev/1458699966-3752-1-git-send-email-gilberto.bertin@gmail.com/
> [3] https://lore.kernel.org/bpf/20190828072250.29828-1-jakub@cloudflare.com/
> [4] https://urldefense.proofpoint.com/v2/url?u=https-3A__blog.cloudflare.com_sockmap-2Dtcp-2Dsplicing-2Dof-2Dthe-2Dfuture_&d=DwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=VQnoQ7LvghIj0gVEaiQSUw&m=lSo-FsOeNl_8znZZ07H8I6ZYAinPKTR5C3Cn_Ol3QYQ&s=NerUqb4j7IsGBTcni6Yxk40wf6kTkckHXn3Nx5i4mCU&e= 

Re: [RFC bpf-next 0/5] Extend SOCKMAP to store listening sockets

[John Fastabend]

Martin Lau wrote:
> On Mon, Oct 28, 2019 at 01:35:26PM +0100, Jakub Sitnicki wrote:
> > On Mon, Oct 28, 2019 at 06:52 AM CET, Martin Lau wrote:
> > > On Tue, Oct 22, 2019 at 01:37:25PM +0200, Jakub Sitnicki wrote:
> > >> This patch set is a follow up on a suggestion from LPC '19 discussions to
> > >> make SOCKMAP (or a new map type derived from it) a generic type for storing
> > >> established as well as listening sockets.
> > >>
> > >> We found ourselves in need of a map type that keeps references to listening
> > >> sockets when working on making the socket lookup programmable, aka BPF
> > >> inet_lookup [1].  Initially we repurposed REUSEPORT_SOCKARRAY but found it
> > >> problematic to extend due to being tightly coupled with reuseport
> > >> logic (see slides [2]).
> > >> So we've turned our attention to SOCKMAP instead.
> > >>
> > >> As it turns out the changes needed to make SOCKMAP suitable for storing
> > >> listening sockets are self-contained and have use outside of programming
> > >> the socket lookup. Hence this patch set.
> > >>
> > >> With these patches SOCKMAP can be used in SK_REUSEPORT BPF programs as a
> > >> drop-in replacement for REUSEPORT_SOCKARRAY for TCP. This can hopefully
> > >> lead to code consolidation between the two map types in the future.
> > > What is the plan for UDP support in sockmap?
> > 
> > It's on our road-map because without SOCKMAP support for UDP we won't be
> > able to move away from TPROXY [1] and custom SO_BINDTOPREFIX extension
> > [2] for steering new UDP flows to receiving sockets. Also we would like
> > to look into using SOCKMAP for connected UDP socket splicing in the
> > future [3].
> > 
> > I was planning to split work as follows:
> > 
> > 1. SOCKMAP support for listening sockets (this series)
> > 2. programmable socket lookup for TCP (cut-down version of [4])
> > 3. SOCKMAP support for UDP (work not started)
> hmm...It is hard to comment how the full UDP sockmap may
> work out without a code attempt because I am not fluent in
> sock_map ;)
> 
> From a quick look, it seems there are quite a few things to do.
> For example, the TCP_SKB_CB(skb) usage and how that may look
> like in UDP.  "struct udp_skb_cb" is 28 bytes while "struct napi_gro_cb"
> seems to be 48 bytes already which may need a closer look.

The extra bits sockmap needs are used for redirecting between
between sockets. These will fit in the udp cb area with some
extra room to spare. If that is paticularly challenging we can
also create a program attach type which would preclude using
those bits in the sk_reuseport bpf program types. We already
have types for rx, tx, nop progs, so one more should be fine.

So at least that paticular concern is not difficult to fix.

> 
> > 4. programmable socket lookup for UDP (rest of [4])
> > 
> > I'm open to suggestions on how to organize it.
> > 
> > >> Having said that, the main intention here is to lay groundwork for using
> > >> SOCKMAP in the next iteration of programmable socket lookup patches.
> > > What may be the minimal to get only lookup work for UDP sockmap?
> > > .close() and .unhash()?
> > 
> > John would know better. I haven't tried doing it yet.
> > 
> > From just reading the code - override the two proto ops you mentioned,
> > close and unhash, and adapt the socket checks in SOCKMAP.
> Do your use cases need bpf prog attached to sock_map?

Perhaps not specifically sock_map but they do need to be consolidated
into a map somewhere IMO this has proven to be the most versatile. We
can add sockets from the various BPF hooks or from user space and have
the ability to use the existing map tools, etc.

> 
> If not, would it be cleaner to delicate another map_type
> for lookup-only use case to have both TCP and UDP support.

But we (Cilium project and above splicing use case is also interested)
will need UDP support so it will be supported regardless of the
SK_REUSEPORT_BPF so I think it makes sense to consolidate all these
use cases on to the existing sockmap.

Also sockmap supports inserting sockets from BPF and from userspace
which actually requires a bit of logic to track state, etc. Its been
in use and been beat on by various automated test tools so I think
at minimum this needs to be reused. Re-implementing this logic seems
a waste of time and it wasn't exactly trivial and took some work.

Being able to insert the sockets from XDP (support coming soon) and
from sock_ops programs turns out to be fairly powerful.

So in short I think it makes most sense to consolidate on sock_map
because

  (a) we need and will add udp support regardless,
  (b) we already handle the tricky parts inerting/removing live sockets
  (c) from this series it looks like its fairly straight forward
  (d) we get lots of shared code

Thanks,
John


> 
> > 
> > -Jakub
> > 
> > [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__blog.cloudflare.com_how-2Dwe-2Dbuilt-2Dspectrum_&d=DwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=VQnoQ7LvghIj0gVEaiQSUw&m=lSo-FsOeNl_8znZZ07H8I6ZYAinPKTR5C3Cn_Ol3QYQ&s=DZgW8-2Xl1P8NU59ji4ieQLzwWpx4t3gGq_tqB0l3Bo&e= 
> > [2] https://lore.kernel.org/netdev/1458699966-3752-1-git-send-email-gilberto.bertin@gmail.com/
> > [3] https://lore.kernel.org/bpf/20190828072250.29828-1-jakub@cloudflare.com/
> > [4] https://urldefense.proofpoint.com/v2/url?u=https-3A__blog.cloudflare.com_sockmap-2Dtcp-2Dsplicing-2Dof-2Dthe-2Dfuture_&d=DwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=VQnoQ7LvghIj0gVEaiQSUw&m=lSo-FsOeNl_8znZZ07H8I6ZYAinPKTR5C3Cn_Ol3QYQ&s=NerUqb4j7IsGBTcni6Yxk40wf6kTkckHXn3Nx5i4mCU&e= 

Re: [RFC bpf-next 0/5] Extend SOCKMAP to store listening sockets

[Martin Lau]

On Mon, Oct 28, 2019 at 02:05:24PM -0700, John Fastabend wrote:
> Martin Lau wrote:
> > On Mon, Oct 28, 2019 at 01:35:26PM +0100, Jakub Sitnicki wrote:
> > > On Mon, Oct 28, 2019 at 06:52 AM CET, Martin Lau wrote:
> > > > On Tue, Oct 22, 2019 at 01:37:25PM +0200, Jakub Sitnicki wrote:
> > > >> This patch set is a follow up on a suggestion from LPC '19 discussions to
> > > >> make SOCKMAP (or a new map type derived from it) a generic type for storing
> > > >> established as well as listening sockets.
> > > >>
> > > >> We found ourselves in need of a map type that keeps references to listening
> > > >> sockets when working on making the socket lookup programmable, aka BPF
> > > >> inet_lookup [1].  Initially we repurposed REUSEPORT_SOCKARRAY but found it
> > > >> problematic to extend due to being tightly coupled with reuseport
> > > >> logic (see slides [2]).
> > > >> So we've turned our attention to SOCKMAP instead.
> > > >>
> > > >> As it turns out the changes needed to make SOCKMAP suitable for storing
> > > >> listening sockets are self-contained and have use outside of programming
> > > >> the socket lookup. Hence this patch set.
> > > >>
> > > >> With these patches SOCKMAP can be used in SK_REUSEPORT BPF programs as a
> > > >> drop-in replacement for REUSEPORT_SOCKARRAY for TCP. This can hopefully
> > > >> lead to code consolidation between the two map types in the future.
> > > > What is the plan for UDP support in sockmap?
> > > 
> > > It's on our road-map because without SOCKMAP support for UDP we won't be
> > > able to move away from TPROXY [1] and custom SO_BINDTOPREFIX extension
> > > [2] for steering new UDP flows to receiving sockets. Also we would like
> > > to look into using SOCKMAP for connected UDP socket splicing in the
> > > future [3].
> > > 
> > > I was planning to split work as follows:
> > > 
> > > 1. SOCKMAP support for listening sockets (this series)
> > > 2. programmable socket lookup for TCP (cut-down version of [4])
> > > 3. SOCKMAP support for UDP (work not started)
> > hmm...It is hard to comment how the full UDP sockmap may
> > work out without a code attempt because I am not fluent in
> > sock_map ;)
> > 
> > From a quick look, it seems there are quite a few things to do.
> > For example, the TCP_SKB_CB(skb) usage and how that may look
> > like in UDP.  "struct udp_skb_cb" is 28 bytes while "struct napi_gro_cb"
> > seems to be 48 bytes already which may need a closer look.
> 
> The extra bits sockmap needs are used for redirecting between
> between sockets. These will fit in the udp cb area with some
> extra room to spare. If that is paticularly challenging we can
> also create a program attach type which would preclude using
> those bits in the sk_reuseport bpf program types. We already
> have types for rx, tx, nop progs, so one more should be fine.
> 
> So at least that paticular concern is not difficult to fix.
> 
> > 
> > > 4. programmable socket lookup for UDP (rest of [4])
> > > 
> > > I'm open to suggestions on how to organize it.
> > > 
> > > >> Having said that, the main intention here is to lay groundwork for using
> > > >> SOCKMAP in the next iteration of programmable socket lookup patches.
> > > > What may be the minimal to get only lookup work for UDP sockmap?
> > > > .close() and .unhash()?
> > > 
> > > John would know better. I haven't tried doing it yet.
> > > 
> > > From just reading the code - override the two proto ops you mentioned,
> > > close and unhash, and adapt the socket checks in SOCKMAP.
> > Do your use cases need bpf prog attached to sock_map?
> 
> Perhaps not specifically sock_map but they do need to be consolidated
> into a map somewhere IMO this has proven to be the most versatile. We
> can add sockets from the various BPF hooks or from user space and have
> the ability to use the existing map tools, etc.
> 
> > 
> > If not, would it be cleaner to delicate another map_type
> > for lookup-only use case to have both TCP and UDP support.
> 
> But we (Cilium project and above splicing use case is also interested)
> will need UDP support so it will be supported regardless of the
> SK_REUSEPORT_BPF so I think it makes sense to consolidate all these
> use cases on to the existing sockmap.
> 
> Also sockmap supports inserting sockets from BPF and from userspace
> which actually requires a bit of logic to track state, etc. Its been
> in use and been beat on by various automated test tools so I think
> at minimum this needs to be reused. Re-implementing this logic seems
> a waste of time and it wasn't exactly trivial and took some work.
> 
> Being able to insert the sockets from XDP (support coming soon) and
> from sock_ops programs turns out to be fairly powerful.
> 
> So in short I think it makes most sense to consolidate on sock_map
> because
> 
>   (a) we need and will add udp support regardless,

>   (b) we already handle the tricky parts inerting/removing live sockets
I didn't mean not to reuse the existing sockmap logic on tracking
socks life-time.  I was exploring options if the first step for UDP
could be lookup-only support first.

It is always better to get full UDP support ;)
It seems to be confident also, then there is little reason not to do
so in UDP sockmap support v1.

>   (c) from this series it looks like its fairly straight forward
>   (d) we get lots of shared code
> 
> Thanks,
> John
> 
> 
> > 
> > > 
> > > -Jakub
> > > 
> > > [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__blog.cloudflare.com_how-2Dwe-2Dbuilt-2Dspectrum_&d=DwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=VQnoQ7LvghIj0gVEaiQSUw&m=lSo-FsOeNl_8znZZ07H8I6ZYAinPKTR5C3Cn_Ol3QYQ&s=DZgW8-2Xl1P8NU59ji4ieQLzwWpx4t3gGq_tqB0l3Bo&e= 
> > > [2] https://lore.kernel.org/netdev/1458699966-3752-1-git-send-email-gilberto.bertin@gmail.com/
> > > [3] https://lore.kernel.org/bpf/20190828072250.29828-1-jakub@cloudflare.com/
> > > [4] https://urldefense.proofpoint.com/v2/url?u=https-3A__blog.cloudflare.com_sockmap-2Dtcp-2Dsplicing-2Dof-2Dthe-2Dfuture_&d=DwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=VQnoQ7LvghIj0gVEaiQSUw&m=lSo-FsOeNl_8znZZ07H8I6ZYAinPKTR5C3Cn_Ol3QYQ&s=NerUqb4j7IsGBTcni6Yxk40wf6kTkckHXn3Nx5i4mCU&e= 
> 
> 

Re: [RFC bpf-next 0/5] Extend SOCKMAP to store listening sockets

[Jakub Sitnicki]

On Mon, Oct 28, 2019 at 10:38 PM CET, Martin Lau wrote:
> It is always better to get full UDP support ;)
> It seems to be confident also, then there is little reason not to do
> so in UDP sockmap support v1.

Let me give it a shot :-)

Re: [RFC bpf-next 0/5] Extend SOCKMAP to store listening sockets

[Jakub Sitnicki]

On Mon, Oct 28, 2019 at 08:04 PM CET, John Fastabend wrote:
> Jakub Sitnicki wrote:
>> I was planning to split work as follows:
>>
>> 1. SOCKMAP support for listening sockets (this series)
>> 2. programmable socket lookup for TCP (cut-down version of [4])
>> 3. SOCKMAP support for UDP (work not started)
>> 4. programmable socket lookup for UDP (rest of [4])
>>
>> I'm open to suggestions on how to organize it.
>
> Looks good to me. I've had UDP support on my todo list for awhile now
> but it hasn't got to the top yet so glad to see this.
>
> Also perhaps not necessary for your work but I have some patches on my
> stack I'll try to get out soon to get ktls + receive hooks working.

Thanks for the heads-up. If you have a dev branch somewhere, I'm curious
to see what's cooking. Otherwise I'll keep an eye out for your patches.