[PATCH 0/3] Allow checking SYN cookies from XDP and tc cls act

[PATCH 0/3] Allow checking SYN cookies from XDP and tc cls act

[Lorenz Bauer]

This series adds a helper bpf_sk_check_syncookie, which allows checking
whether a packet is a valid SYN cookie for a given socket. This is
useful for implementing load-balancing approaches like glb-director [1]
or Beamer [2] in pure eBPF.

Specifically, we'd like to replace the functionality of the glb-redirect
kernel module by an XDP program or tc classifier.

1: https://github.com/github/glb-director
2: https://www.usenix.org/conference/nsdi18/presentation/olteanu

Lorenz Bauer (3):
  bpf: add helper to check for a valid SYN cookie
  tools: sync changes to uapi/linux/bpf.h
  selftests/bpf: add tests for bpf_sk_check_syncookie

 include/uapi/linux/bpf.h                      |  18 +-
 net/core/filter.c                             |  68 ++++++
 tools/include/uapi/linux/bpf.h                |  18 +-
 tools/testing/selftests/bpf/.gitignore        |   1 +
 tools/testing/selftests/bpf/Makefile          |   5 +-
 tools/testing/selftests/bpf/bpf_helpers.h     |   3 +
 .../bpf/progs/test_sk_syncookie_kern.c        | 119 ++++++++++
 .../selftests/bpf/test_sk_syncookie.sh        |  81 +++++++
 .../selftests/bpf/test_sk_syncookie_user.c    | 212 ++++++++++++++++++
 9 files changed, 521 insertions(+), 4 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/test_sk_syncookie_kern.c
 create mode 100755 tools/testing/selftests/bpf/test_sk_syncookie.sh
 create mode 100644 tools/testing/selftests/bpf/test_sk_syncookie_user.c

[PATCH 1/3] bpf: add helper to check for a valid SYN cookie

[Lorenz Bauer]

Using bpf_sk_lookup_tcp it's possible to ascertain whether a packet belongs
to a known connection. However, there is one corner case: no sockets are
created if SYN cookies are active. This means that the final ACK in the
3WHS is misclassified.

Using the helper, we can look up the listening socket via bpf_sk_lookup_tcp
and then check whether a packet is a valid SYN cookie ACK.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 include/uapi/linux/bpf.h | 18 ++++++++++-
 net/core/filter.c        | 68 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 85 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index bcdd2474eee7..bc2af87e9621 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -2359,6 +2359,21 @@ union bpf_attr {
  *	Return
  *		A **struct bpf_tcp_sock** pointer on success, or NULL in
  *		case of failure.
+ *
+ * int bpf_sk_check_syncookie(struct bpf_sock *sk, void *iph, u32 iph_len, struct tcphdr *th, u32 th_len)
+ * 	Description
+ * 		Check whether iph and th contain a valid SYN cookie ACK for
+ * 		the listening socket in sk.
+ *
+ * 		iph points to the start of the IPv4 or IPv6 header, while
+ * 		iph_len contains sizeof(struct iphdr) or sizeof(struct ip6hdr).
+ *
+ * 		th points to the start of the TCP header, while th_len contains
+ * 		sizeof(struct tcphdr).
+ *
+ * 	Return
+ * 		0 if iph and th are a valid SYN cookie ACK, or a negative error
+ * 		otherwise.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -2457,7 +2472,8 @@ union bpf_attr {
 	FN(spin_lock),			\
 	FN(spin_unlock),		\
 	FN(sk_fullsock),		\
-	FN(tcp_sock),
+	FN(tcp_sock),			\
+	FN(sk_check_syncookie),
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
  * function eBPF program intends to call
diff --git a/net/core/filter.c b/net/core/filter.c
index 85749f6ec789..9e68897cc7ed 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5426,6 +5426,70 @@ static const struct bpf_func_proto bpf_tcp_sock_proto = {
 	.arg1_type	= ARG_PTR_TO_SOCK_COMMON,
 };
 
+BPF_CALL_5(bpf_sk_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len,
+	   struct tcphdr *, th, u32, th_len)
+{
+#if IS_ENABLED(CONFIG_SYN_COOKIES)
+	u32 cookie;
+	int ret;
+
+	if (unlikely(th_len < sizeof(*th)))
+		return -EINVAL;
+
+	/* sk_listener() allows TCP_NEW_SYN_RECV, which makes no sense here. */
+	if (sk->sk_protocol != IPPROTO_TCP || sk->sk_state != TCP_LISTEN)
+		return -EINVAL;
+
+	if (!sock_net(sk)->ipv4.sysctl_tcp_syncookies)
+		return -EINVAL;
+
+	if (!th->ack || th->rst)
+		return -ENOENT;
+
+	cookie = ntohl(th->ack_seq) - 1;
+
+	switch (sk->sk_family) {
+	case AF_INET:
+		if (unlikely(iph_len < sizeof(struct iphdr)))
+			return -EINVAL;
+
+		ret = __cookie_v4_check((struct iphdr *)iph, th, cookie);
+		break;
+
+#if IS_ENABLED(CONFIG_IPV6)
+	case AF_INET6:
+		if (unlikely(iph_len < sizeof(struct ipv6hdr)))
+			return -EINVAL;
+
+		ret = __cookie_v6_check((struct ipv6hdr *)iph, th, cookie);
+		break;
+#endif /* CONFIG_IPV6 */
+
+	default:
+		return -EPROTONOSUPPORT;
+	}
+
+	if (ret > 0)
+		return 0;
+
+	return -ENOENT;
+#else
+	return -ENOTSUP;
+#endif
+}
+
+static const struct bpf_func_proto bpf_sk_check_syncookie_proto = {
+	.func		= bpf_sk_check_syncookie,
+	.gpl_only	= true,
+	.pkt_access	= true,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_PTR_TO_SOCKET,
+	.arg2_type	= ARG_PTR_TO_MEM,
+	.arg3_type	= ARG_CONST_SIZE,
+	.arg4_type	= ARG_PTR_TO_MEM,
+	.arg5_type	= ARG_CONST_SIZE,
+};
+
 #endif /* CONFIG_INET */
 
 bool bpf_helper_changes_pkt_data(void *func)
@@ -5678,6 +5742,8 @@ tc_cls_act_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_sk_release_proto;
 	case BPF_FUNC_tcp_sock:
 		return &bpf_tcp_sock_proto;
+	case BPF_FUNC_sk_check_syncookie:
+		return &bpf_sk_check_syncookie_proto;
 #endif
 	default:
 		return bpf_base_func_proto(func_id);
@@ -5713,6 +5779,8 @@ xdp_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_xdp_sk_lookup_tcp_proto;
 	case BPF_FUNC_sk_release:
 		return &bpf_sk_release_proto;
+	case BPF_FUNC_sk_check_syncookie:
+		return &bpf_sk_check_syncookie_proto;
 #endif
 	default:
 		return bpf_base_func_proto(func_id);
-- 
2.19.1

[PATCH 2/3] tools: sync changes to uapi/linux/bpf.h

[Lorenz Bauer]

Pull in definitions for bpf_sk_check_syncookie.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 tools/include/uapi/linux/bpf.h | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index bcdd2474eee7..bc2af87e9621 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -2359,6 +2359,21 @@ union bpf_attr {
  *	Return
  *		A **struct bpf_tcp_sock** pointer on success, or NULL in
  *		case of failure.
+ *
+ * int bpf_sk_check_syncookie(struct bpf_sock *sk, void *iph, u32 iph_len, struct tcphdr *th, u32 th_len)
+ * 	Description
+ * 		Check whether iph and th contain a valid SYN cookie ACK for
+ * 		the listening socket in sk.
+ *
+ * 		iph points to the start of the IPv4 or IPv6 header, while
+ * 		iph_len contains sizeof(struct iphdr) or sizeof(struct ip6hdr).
+ *
+ * 		th points to the start of the TCP header, while th_len contains
+ * 		sizeof(struct tcphdr).
+ *
+ * 	Return
+ * 		0 if iph and th are a valid SYN cookie ACK, or a negative error
+ * 		otherwise.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -2457,7 +2472,8 @@ union bpf_attr {
 	FN(spin_lock),			\
 	FN(spin_unlock),		\
 	FN(sk_fullsock),		\
-	FN(tcp_sock),
+	FN(tcp_sock),			\
+	FN(sk_check_syncookie),
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
  * function eBPF program intends to call
-- 
2.19.1

[PATCH 3/3] selftests/bpf: add tests for bpf_sk_check_syncookie

[Lorenz Bauer]

Add tests which verify that the new helper works for both IPv4 and
IPv6, by forcing SYN cookies to always on. Use a new network namespace
to avoid clobbering the global SYN cookie settings.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 tools/testing/selftests/bpf/.gitignore        |   1 +
 tools/testing/selftests/bpf/Makefile          |   5 +-
 tools/testing/selftests/bpf/bpf_helpers.h     |   3 +
 .../bpf/progs/test_sk_syncookie_kern.c        | 119 ++++++++++
 .../selftests/bpf/test_sk_syncookie.sh        |  81 +++++++
 .../selftests/bpf/test_sk_syncookie_user.c    | 212 ++++++++++++++++++
 6 files changed, 419 insertions(+), 2 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/test_sk_syncookie_kern.c
 create mode 100755 tools/testing/selftests/bpf/test_sk_syncookie.sh
 create mode 100644 tools/testing/selftests/bpf/test_sk_syncookie_user.c

diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
index e47168d1257d..44b2853b8534 100644
--- a/tools/testing/selftests/bpf/.gitignore
+++ b/tools/testing/selftests/bpf/.gitignore
@@ -29,4 +29,5 @@ test_netcnt
 test_section_names
 test_tcpnotify_user
 test_libbpf
+test_sk_syncookie_user
 alu32
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index ccffaa0a0787..4ad1f7a915b3 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -51,7 +51,8 @@ TEST_PROGS := test_kmod.sh \
 	test_skb_cgroup_id.sh \
 	test_flow_dissector.sh \
 	test_xdp_vlan.sh \
-	test_lwt_ip_encap.sh
+	test_lwt_ip_encap.sh \
+	test_sk_syncookie.sh
 
 TEST_PROGS_EXTENDED := with_addr.sh \
 	with_tunnels.sh \
@@ -60,7 +61,7 @@ TEST_PROGS_EXTENDED := with_addr.sh \
 
 # Compile but not part of 'make run_tests'
 TEST_GEN_PROGS_EXTENDED = test_libbpf_open test_sock_addr test_skb_cgroup_id_user \
-	flow_dissector_load test_flow_dissector
+	flow_dissector_load test_flow_dissector test_sk_syncookie_user
 
 include ../lib.mk
 
diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
index d9999f1ed1d2..b80cec7af445 100644
--- a/tools/testing/selftests/bpf/bpf_helpers.h
+++ b/tools/testing/selftests/bpf/bpf_helpers.h
@@ -166,6 +166,9 @@ static struct bpf_sock *(*bpf_sk_lookup_udp)(void *ctx,
 	(void *) BPF_FUNC_sk_lookup_udp;
 static int (*bpf_sk_release)(struct bpf_sock *sk) =
 	(void *) BPF_FUNC_sk_release;
+static int (*bpf_sk_check_syncookie)(struct bpf_sock *sk,
+	    void *ip, int ip_len, void *tcp, int tcp_len) =
+	(void *) BPF_FUNC_sk_check_syncookie;
 static int (*bpf_skb_vlan_push)(void *ctx, __be16 vlan_proto, __u16 vlan_tci) =
 	(void *) BPF_FUNC_skb_vlan_push;
 static int (*bpf_skb_vlan_pop)(void *ctx) =
diff --git a/tools/testing/selftests/bpf/progs/test_sk_syncookie_kern.c b/tools/testing/selftests/bpf/progs/test_sk_syncookie_kern.c
new file mode 100644
index 000000000000..4918457efc1e
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_sk_syncookie_kern.c
@@ -0,0 +1,119 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2018 Facebook
+// Copyright (c) 2019 Cloudflare
+
+#include <string.h>
+
+#include <linux/bpf.h>
+#include <linux/pkt_cls.h>
+#include <linux/if_ether.h>
+#include <linux/in.h>
+#include <linux/ip.h>
+#include <linux/ipv6.h>
+#include <sys/socket.h>
+#include <linux/tcp.h>
+
+#include "bpf_helpers.h"
+#include "bpf_endian.h"
+
+struct bpf_map_def SEC("maps") results = {
+	.type = BPF_MAP_TYPE_ARRAY,
+	.key_size = sizeof(__u32),
+	.value_size = sizeof(__u64),
+	.max_entries = 1,
+};
+
+static __always_inline void check_syncookie(void *ctx, void *data,
+					    void *data_end)
+{
+	struct bpf_sock_tuple tup;
+	struct bpf_sock *sk;
+	struct ethhdr *ethh;
+	struct iphdr *ipv4h;
+	struct ipv6hdr *ipv6h;
+	struct tcphdr *tcph;
+	int ret;
+	__u32 key = 0;
+	__u64 value = 1;
+
+	ethh = data;
+	if (ethh + 1 > data_end)
+		return;
+
+	switch (bpf_ntohs(ethh->h_proto)) {
+	case ETH_P_IP:
+		ipv4h = data + sizeof(struct ethhdr);
+		if (ipv4h + 1 > data_end)
+			return;
+
+		tcph = data + sizeof(struct ethhdr) + sizeof(struct iphdr);
+		if (tcph + 1 > data_end)
+			return;
+
+		tup.ipv4.saddr = ipv4h->saddr;
+		tup.ipv4.daddr = ipv4h->daddr;
+		tup.ipv4.sport = tcph->source;
+		tup.ipv4.dport = tcph->dest;
+
+		sk = bpf_sk_lookup_tcp(ctx, &tup, sizeof(tup.ipv4),
+				       BPF_F_CURRENT_NETNS, 0);
+		if (!sk)
+			return;
+
+		ret = bpf_sk_check_syncookie(sk, ipv4h, sizeof(*ipv4h),
+					     tcph, sizeof(*tcph));
+		break;
+
+	case ETH_P_IPV6:
+		ipv6h = data + sizeof(struct ethhdr);
+		if (ipv6h + 1 > data_end)
+			return;
+
+		if (ipv6h->nexthdr != IPPROTO_TCP)
+			return;
+
+		tcph = data + sizeof(struct ethhdr) + sizeof(struct ipv6hdr);
+		if (tcph + 1 > data_end)
+			return;
+
+		memcpy(tup.ipv6.saddr, &ipv6h->saddr, sizeof(tup.ipv6.saddr));
+		memcpy(tup.ipv6.daddr, &ipv6h->daddr, sizeof(tup.ipv6.daddr));
+		tup.ipv6.sport = tcph->source;
+		tup.ipv6.dport = tcph->dest;
+
+		sk = bpf_sk_lookup_tcp(ctx, &tup, sizeof(tup.ipv6),
+				       BPF_F_CURRENT_NETNS, 0);
+		if (!sk)
+			return;
+
+		ret = bpf_sk_check_syncookie(sk, ipv6h, sizeof(*ipv6h),
+					     tcph, sizeof(*tcph));
+		break;
+
+	default:
+		return;
+	}
+
+	if (ret == 0)
+		bpf_map_update_elem(&results, &key, &value, 0);
+
+	bpf_sk_release(sk);
+}
+
+SEC("clsact/check_syncookie")
+int check_syncookie_clsact(struct __sk_buff *skb)
+{
+	check_syncookie(skb, (void *)(long)skb->data,
+			(void *)(long)skb->data_end);
+	return TC_ACT_OK;
+}
+
+SEC("xdp/check_syncookie")
+int check_syncookie_xdp(struct xdp_md *ctx)
+{
+	check_syncookie(ctx, (void *)(long)ctx->data,
+			(void *)(long)ctx->data_end);
+	return XDP_PASS;
+}
+
+char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/test_sk_syncookie.sh b/tools/testing/selftests/bpf/test_sk_syncookie.sh
new file mode 100755
index 000000000000..429ca8c04c5e
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_sk_syncookie.sh
@@ -0,0 +1,81 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2018 Facebook
+# Copyright (c) 2019 Cloudflare
+
+set -eu
+
+wait_for_ip()
+{
+	local _i
+	printf "Wait for IP %s to become available " "$1"
+	for _i in $(seq ${MAX_PING_TRIES}); do
+		printf "."
+		if ns1_exec ping -c 1 -W 1 "$1" >/dev/null 2>&1; then
+			echo " OK"
+			return
+		fi
+		sleep 1
+	done
+	echo 1>&2 "ERROR: Timeout waiting for test IP to become available."
+	exit 1
+}
+
+get_prog_id()
+{
+	awk '/ id / {sub(/.* id /, "", $0); print($1)}'
+}
+
+ns1_exec()
+{
+	ip netns exec ns1 "$@"
+}
+
+setup()
+{
+	ip netns add ns1
+	ns1_exec ip link set lo up
+
+	ns1_exec sysctl -w net.ipv4.tcp_syncookies=2
+
+	wait_for_ip 127.0.0.1
+	wait_for_ip ::1
+}
+
+cleanup()
+{
+	ip netns del ns1 2>/dev/null || :
+}
+
+main()
+{
+	trap cleanup EXIT 2 3 6 15
+	setup
+
+	printf "Testing clsact..."
+	ns1_exec tc qdisc add dev "${TEST_IF}" clsact
+	ns1_exec tc filter add dev "${TEST_IF}" ingress \
+		bpf obj "${BPF_PROG_OBJ}" sec "${CLSACT_SECTION}" da
+
+	BPF_PROG_ID=$(ns1_exec tc filter show dev "${TEST_IF}" ingress | \
+		      get_prog_id)
+	ns1_exec "${PROG}" "${BPF_PROG_ID}"
+	ns1_exec tc qdisc del dev "${TEST_IF}" clsact
+
+	printf "Testing XDP..."
+	ns1_exec ip link set "${TEST_IF}" xdp \
+		object "${BPF_PROG_OBJ}" section "${XDP_SECTION}"
+	BPF_PROG_ID=$(ns1_exec ip link show "${TEST_IF}" | get_prog_id)
+	ns1_exec "${PROG}" "${BPF_PROG_ID}"
+}
+
+DIR=$(dirname $0)
+TEST_IF=lo
+MAX_PING_TRIES=5
+BPF_PROG_OBJ="${DIR}/test_sk_syncookie_kern.o"
+CLSACT_SECTION="clsact/check_syncookie"
+XDP_SECTION="xdp/check_syncookie"
+BPF_PROG_ID=0
+PROG="${DIR}/test_sk_syncookie_user"
+
+main
diff --git a/tools/testing/selftests/bpf/test_sk_syncookie_user.c b/tools/testing/selftests/bpf/test_sk_syncookie_user.c
new file mode 100644
index 000000000000..87829c86c746
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_sk_syncookie_user.c
@@ -0,0 +1,212 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2018 Facebook
+// Copyright (c) 2019 Cloudflare
+
+#include <string.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+#include <arpa/inet.h>
+#include <netinet/in.h>
+#include <sys/types.h>
+#include <sys/socket.h>
+
+#include <bpf/bpf.h>
+#include <bpf/libbpf.h>
+
+#include "bpf_rlimit.h"
+#include "cgroup_helpers.h"
+
+static int start_server(const struct sockaddr *addr, socklen_t len)
+{
+	int fd;
+
+	fd = socket(addr->sa_family, SOCK_STREAM, 0);
+	if (fd == -1) {
+		log_err("Failed to create server socket");
+		goto out;
+	}
+
+	if (bind(fd, addr, len) == -1) {
+		log_err("Failed to bind server socket");
+		goto close_out;
+	}
+
+	if (listen(fd, 128) == -1) {
+		log_err("Failed to listen on server socket");
+		goto close_out;
+	}
+
+	goto out;
+
+close_out:
+	close(fd);
+	fd = -1;
+out:
+	return fd;
+}
+
+static int connect_to_server(int server_fd)
+{
+	struct sockaddr_storage addr;
+	socklen_t len = sizeof(addr);
+	int fd = -1;
+
+	if (getsockname(server_fd, (struct sockaddr *)&addr, &len)) {
+		log_err("Failed to get server addr");
+		goto out;
+	}
+
+	fd = socket(addr.ss_family, SOCK_STREAM, 0);
+	if (fd == -1) {
+		log_err("Failed to create client socket");
+		goto out;
+	}
+
+	if (connect(fd, (const struct sockaddr *)&addr, len) == -1) {
+		log_err("Fail to connect to server");
+		goto close_out;
+	}
+
+	goto out;
+
+close_out:
+	close(fd);
+	fd = -1;
+out:
+	return fd;
+}
+
+static int get_map_fd_by_prog_id(int prog_id)
+{
+	struct bpf_prog_info info = {};
+	__u32 info_len = sizeof(info);
+	__u32 map_ids[1];
+	int prog_fd = -1;
+	int map_fd = -1;
+
+	prog_fd = bpf_prog_get_fd_by_id(prog_id);
+	if (prog_fd < 0) {
+		log_err("Failed to get fd by prog id %d", prog_id);
+		goto err;
+	}
+
+	info.nr_map_ids = 1;
+	info.map_ids = (__u64)(unsigned long)map_ids;
+
+	if (bpf_obj_get_info_by_fd(prog_fd, &info, &info_len)) {
+		log_err("Failed to get info by prog fd %d", prog_fd);
+		goto err;
+	}
+
+	if (!info.nr_map_ids) {
+		log_err("No maps found for prog fd %d", prog_fd);
+		goto err;
+	}
+
+	map_fd = bpf_map_get_fd_by_id(map_ids[0]);
+	if (map_fd < 0)
+		log_err("Failed to get fd by map id %d", map_ids[0]);
+err:
+	if (prog_fd >= 0)
+		close(prog_fd);
+	return map_fd;
+}
+
+static int run_test(int server_fd, int results_fd)
+{
+	int client = -1, srv_client = -1;
+	int ret = 0;
+	__u32 key = 0;
+	__u64 value = 0;
+
+	if (bpf_map_update_elem(results_fd, &key, &value, 0) < 0) {
+		log_err("Can't clear results");
+		goto err;
+	}
+
+	client = connect_to_server(server_fd);
+	if (client == -1)
+		goto err;
+
+	srv_client = accept(server_fd, NULL, 0);
+	if (srv_client == -1) {
+		log_err("Can't accept connection");
+		goto err;
+	}
+
+	if (bpf_map_lookup_elem(results_fd, &key, &value) < 0) {
+		log_err("Can't lookup result");
+		goto err;
+	}
+
+	if (value != 1) {
+		log_err("Didn't match syncookie: %llu", value);
+		goto err;
+	}
+
+	goto out;
+
+err:
+	ret = 1;
+out:
+	close(client);
+	close(srv_client);
+	return ret;
+}
+
+int main(int argc, char **argv)
+{
+	struct sockaddr_in addr4;
+	struct sockaddr_in6 addr6;
+	int server = -1;
+	int server_v6 = -1;
+	int results = -1;
+	int err = 0;
+
+	if (argc < 2) {
+		fprintf(stderr, "Usage: %s prog_id\n", argv[0]);
+		exit(1);
+	}
+
+	results = get_map_fd_by_prog_id(atoi(argv[1]));
+	if (results < 0) {
+		log_err("Can't get map");
+		goto err;
+	}
+
+	memset(&addr4, 0, sizeof(addr4));
+	addr4.sin_family = AF_INET;
+	addr4.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
+	addr4.sin_port = 0;
+
+	memset(&addr6, 0, sizeof(addr6));
+	addr6.sin6_family = AF_INET6;
+	addr6.sin6_addr = in6addr_loopback;
+	addr6.sin6_port = 0;
+
+	server = start_server((const struct sockaddr *)&addr4, sizeof(addr4));
+	if (server == -1)
+		goto err;
+
+	server_v6 = start_server((const struct sockaddr *)&addr6,
+				 sizeof(addr6));
+	if (server_v6 == -1)
+		goto err;
+
+	if (run_test(server, results))
+		goto err;
+
+	if (run_test(server_v6, results))
+		goto err;
+
+	printf("ok\n");
+	goto out;
+err:
+	err = 1;
+out:
+	close(server);
+	close(server_v6);
+	close(results);
+	return err;
+}
-- 
2.19.1

Re: [PATCH 1/3] bpf: add helper to check for a valid SYN cookie

[Martin Lau]

On Fri, Feb 22, 2019 at 09:50:55AM +0000, Lorenz Bauer wrote:
> Using bpf_sk_lookup_tcp it's possible to ascertain whether a packet belongs
> to a known connection. However, there is one corner case: no sockets are
> created if SYN cookies are active. This means that the final ACK in the
> 3WHS is misclassified.
> 
> Using the helper, we can look up the listening socket via bpf_sk_lookup_tcp
> and then check whether a packet is a valid SYN cookie ACK.
> 
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> ---
>  include/uapi/linux/bpf.h | 18 ++++++++++-
>  net/core/filter.c        | 68 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 85 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index bcdd2474eee7..bc2af87e9621 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -2359,6 +2359,21 @@ union bpf_attr {
>   *	Return
>   *		A **struct bpf_tcp_sock** pointer on success, or NULL in
>   *		case of failure.
> + *
> + * int bpf_sk_check_syncookie(struct bpf_sock *sk, void *iph, u32 iph_len, struct tcphdr *th, u32 th_len)
> + * 	Description
> + * 		Check whether iph and th contain a valid SYN cookie ACK for
> + * 		the listening socket in sk.
> + *
> + * 		iph points to the start of the IPv4 or IPv6 header, while
> + * 		iph_len contains sizeof(struct iphdr) or sizeof(struct ip6hdr).
> + *
> + * 		th points to the start of the TCP header, while th_len contains
> + * 		sizeof(struct tcphdr).
> + *
> + * 	Return
> + * 		0 if iph and th are a valid SYN cookie ACK, or a negative error
> + * 		otherwise.
>   */
>  #define __BPF_FUNC_MAPPER(FN)		\
>  	FN(unspec),			\
> @@ -2457,7 +2472,8 @@ union bpf_attr {
>  	FN(spin_lock),			\
>  	FN(spin_unlock),		\
>  	FN(sk_fullsock),		\
> -	FN(tcp_sock),
> +	FN(tcp_sock),			\
> +	FN(sk_check_syncookie),
>  
>  /* integer value in 'imm' field of BPF_CALL instruction selects which helper
>   * function eBPF program intends to call
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 85749f6ec789..9e68897cc7ed 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -5426,6 +5426,70 @@ static const struct bpf_func_proto bpf_tcp_sock_proto = {
>  	.arg1_type	= ARG_PTR_TO_SOCK_COMMON,
>  };
>  
> +BPF_CALL_5(bpf_sk_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len,
s/bpf_sk_check_syncookie/bpf_tcp_check_syncookie/

> +	   struct tcphdr *, th, u32, th_len)
> +{
> +#if IS_ENABLED(CONFIG_SYN_COOKIES)
nit. "#ifdef CONFIG_SYN_COOKIES" such that it is clear it is a bool kconfig.

> +	u32 cookie;
> +	int ret;
> +
> +	if (unlikely(th_len < sizeof(*th)))
> +		return -EINVAL;
> +
> +	/* sk_listener() allows TCP_NEW_SYN_RECV, which makes no sense here. */
> +	if (sk->sk_protocol != IPPROTO_TCP || sk->sk_state != TCP_LISTEN)
>From the test program in patch 3, the "sk" here is obtained from
bpf_sk_lookup_tcp() which does a sk_to_full_sk() before returning.
AFAICT, meaning bpf_sk_lookup_tcp() will return the listening sk
even if there is a request_sock.  Does it make sense to check
syncookie if there is already a request_sock?

> +		return -EINVAL;
> +
> +	if (!sock_net(sk)->ipv4.sysctl_tcp_syncookies)
Should tcp_synq_no_recent_overflow(tp) be checked also?

> +		return -EINVAL;
> +
> +	if (!th->ack || th->rst)
How about th->syn?

> +		return -ENOENT;
> +
> +	cookie = ntohl(th->ack_seq) - 1;
> +
> +	switch (sk->sk_family) {
> +	case AF_INET:
> +		if (unlikely(iph_len < sizeof(struct iphdr)))
> +			return -EINVAL;
> +
> +		ret = __cookie_v4_check((struct iphdr *)iph, th, cookie);
> +		break;
> +
> +#if IS_ENABLED(CONFIG_IPV6)
> +	case AF_INET6:
> +		if (unlikely(iph_len < sizeof(struct ipv6hdr)))
> +			return -EINVAL;
> +
> +		ret = __cookie_v6_check((struct ipv6hdr *)iph, th, cookie);
> +		break;
> +#endif /* CONFIG_IPV6 */
> +
> +	default:
> +		return -EPROTONOSUPPORT;
> +	}
> +
> +	if (ret > 0)
> +		return 0;
> +
> +	return -ENOENT;
> +#else
> +	return -ENOTSUP;
> +#endif
> +}
> +
> +static const struct bpf_func_proto bpf_sk_check_syncookie_proto = {
> +	.func		= bpf_sk_check_syncookie,
> +	.gpl_only	= true,
> +	.pkt_access	= true,
> +	.ret_type	= RET_INTEGER,
> +	.arg1_type	= ARG_PTR_TO_SOCKET,
I think it should be ARG_PTR_TO_TCP_SOCK

> +	.arg2_type	= ARG_PTR_TO_MEM,
> +	.arg3_type	= ARG_CONST_SIZE,
> +	.arg4_type	= ARG_PTR_TO_MEM,
> +	.arg5_type	= ARG_CONST_SIZE,
> +};
> +
>  #endif /* CONFIG_INET */

Re: [PATCH 1/3] bpf: add helper to check for a valid SYN cookie

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1582 bytes --]

Hi Lorenz,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on bpf-next/master]
[also build test ERROR on next-20190222]
[cannot apply to v5.0-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Lorenz-Bauer/Allow-checking-SYN-cookies-from-XDP-and-tc-cls-act/20190224-180755
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: x86_64-kexec (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All error/warnings (new ones prefixed by >>):

   net/core/filter.c: In function '____bpf_sk_check_syncookie':
>> net/core/filter.c:5477:10: error: 'ENOTSUP' undeclared (first use in this function); did you mean 'ENOTSUPP'?
     return -ENOTSUP;
             ^~~~~~~
             ENOTSUPP
   net/core/filter.c:5477:10: note: each undeclared identifier is reported only once for each function it appears in
>> net/core/filter.c:5479:1: warning: control reaches end of non-void function [-Wreturn-type]
    }
    ^

vim +5477 net/core/filter.c

  5467	
  5468		default:
  5469			return -EPROTONOSUPPORT;
  5470		}
  5471	
  5472		if (ret > 0)
  5473			return 0;
  5474	
  5475		return -ENOENT;
  5476	#else
> 5477		return -ENOTSUP;
  5478	#endif
> 5479	}
  5480	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26394 bytes --]

Re: [PATCH 1/3] bpf: add helper to check for a valid SYN cookie

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1329 bytes --]

Hi Lorenz,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on bpf-next/master]
[also build test ERROR on next-20190222]
[cannot apply to v5.0-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Lorenz-Bauer/Allow-checking-SYN-cookies-from-XDP-and-tc-cls-act/20190224-180755
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: m68k-sun3_defconfig (attached as .config)
compiler: m68k-linux-gnu-gcc (Debian 8.2.0-11) 8.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=8.2.0 make.cross ARCH=m68k 

All errors (new ones prefixed by >>):

   m68k-linux-gnu-ld: drivers/rtc/proc.o: in function `is_rtc_hctosys.isra.0':
   proc.c:(.text+0x178): undefined reference to `strcmp'
   m68k-linux-gnu-ld: net/core/filter.o: in function `bpf_sk_check_syncookie':
>> filter.c:(.text+0x5a58): undefined reference to `__cookie_v6_check'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 12145 bytes --]

Re: [PATCH 1/3] bpf: add helper to check for a valid SYN cookie

[Lorenz Bauer]

On Sat, 23 Feb 2019 at 00:44, Martin Lau <kafai@fb.com> wrote:
>
> On Fri, Feb 22, 2019 at 09:50:55AM +0000, Lorenz Bauer wrote:
> > Using bpf_sk_lookup_tcp it's possible to ascertain whether a packet belongs
> > to a known connection. However, there is one corner case: no sockets are
> > created if SYN cookies are active. This means that the final ACK in the
> > 3WHS is misclassified.
> >
> > Using the helper, we can look up the listening socket via bpf_sk_lookup_tcp
> > and then check whether a packet is a valid SYN cookie ACK.
> >
> > Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> > ---
> >  include/uapi/linux/bpf.h | 18 ++++++++++-
> >  net/core/filter.c        | 68 ++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 85 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index bcdd2474eee7..bc2af87e9621 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -2359,6 +2359,21 @@ union bpf_attr {
> >   *   Return
> >   *           A **struct bpf_tcp_sock** pointer on success, or NULL in
> >   *           case of failure.
> > + *
> > + * int bpf_sk_check_syncookie(struct bpf_sock *sk, void *iph, u32 iph_len, struct tcphdr *th, u32 th_len)
> > + *   Description
> > + *           Check whether iph and th contain a valid SYN cookie ACK for
> > + *           the listening socket in sk.
> > + *
> > + *           iph points to the start of the IPv4 or IPv6 header, while
> > + *           iph_len contains sizeof(struct iphdr) or sizeof(struct ip6hdr).
> > + *
> > + *           th points to the start of the TCP header, while th_len contains
> > + *           sizeof(struct tcphdr).
> > + *
> > + *   Return
> > + *           0 if iph and th are a valid SYN cookie ACK, or a negative error
> > + *           otherwise.
> >   */
> >  #define __BPF_FUNC_MAPPER(FN)                \
> >       FN(unspec),                     \
> > @@ -2457,7 +2472,8 @@ union bpf_attr {
> >       FN(spin_lock),                  \
> >       FN(spin_unlock),                \
> >       FN(sk_fullsock),                \
> > -     FN(tcp_sock),
> > +     FN(tcp_sock),                   \
> > +     FN(sk_check_syncookie),
> >
> >  /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> >   * function eBPF program intends to call
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 85749f6ec789..9e68897cc7ed 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -5426,6 +5426,70 @@ static const struct bpf_func_proto bpf_tcp_sock_proto = {
> >       .arg1_type      = ARG_PTR_TO_SOCK_COMMON,
> >  };
> >
> > +BPF_CALL_5(bpf_sk_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len,
> s/bpf_sk_check_syncookie/bpf_tcp_check_syncookie/>
>
> > +        struct tcphdr *, th, u32, th_len)
> > +{
> > +#if IS_ENABLED(CONFIG_SYN_COOKIES)
> nit. "#ifdef CONFIG_SYN_COOKIES" such that it is clear it is a bool kconfig.
>
> > +     u32 cookie;
> > +     int ret;
> > +
> > +     if (unlikely(th_len < sizeof(*th)))
> > +             return -EINVAL;
> > +
> > +     /* sk_listener() allows TCP_NEW_SYN_RECV, which makes no sense here. */
> > +     if (sk->sk_protocol != IPPROTO_TCP || sk->sk_state != TCP_LISTEN)
> From the test program in patch 3, the "sk" here is obtained from
> bpf_sk_lookup_tcp() which does a sk_to_full_sk() before returning.
> AFAICT, meaning bpf_sk_lookup_tcp() will return the listening sk
> even if there is a request_sock.  Does it make sense to check
> syncookie if there is already a request_sock?

No, that doesn't make a lot of sense. I hadn't realised that
sk_lookup_tcp only returns full sockets.
This means we need a way to detect that there is a request sock for a
given tuple.

* adding a reqsk_exists(tuple) helper means we have to pay the lookup cost twice
* drop the sk argument and do the necessary lookups in the helper
itself, but that also
  wastes a call to __inet_lookup_listener
* skip sk_to_full_sk() in a helper and return RET_PTR_TO_SOCK_COMMON,
  but that violates a bunch of assumptions (e.g. calling bpf_sk_release on them)

For context: ultimately we want use this to answer the question: does
this (encapsulated)
packet contain a payload destined to a local socket? Amongst the edge
cases we need to
handle are ICMP Packet Too Big messages and SYN cookies. A solution
would be to hide
all this in an "uber" helper that takes pointers to the L3 / L4
headers and returns a verdict,
but that seems a bit gross.

>
> > +             return -EINVAL;
> > +
> > +     if (!sock_net(sk)->ipv4.sysctl_tcp_syncookies)
> Should tcp_synq_no_recent_overflow(tp) be checked also?
>

Yes, not sure how that slipped out.

> > +             return -EINVAL;
> > +
> > +     if (!th->ack || th->rst)
> How about th->syn?
>

Yes, I missed the fact that the callers in tcp_ipv{4,6}.c check this.

> > +             return -ENOENT;
> > +
> > +     cookie = ntohl(th->ack_seq) - 1;
> > +
> > +     switch (sk->sk_family) {
> > +     case AF_INET:
> > +             if (unlikely(iph_len < sizeof(struct iphdr)))
> > +                     return -EINVAL;
> > +
> > +             ret = __cookie_v4_check((struct iphdr *)iph, th, cookie);
> > +             break;
> > +
> > +#if IS_ENABLED(CONFIG_IPV6)
> > +     case AF_INET6:
> > +             if (unlikely(iph_len < sizeof(struct ipv6hdr)))
> > +                     return -EINVAL;
> > +
> > +             ret = __cookie_v6_check((struct ipv6hdr *)iph, th, cookie);
> > +             break;
> > +#endif /* CONFIG_IPV6 */
> > +
> > +     default:
> > +             return -EPROTONOSUPPORT;
> > +     }
> > +
> > +     if (ret > 0)
> > +             return 0;
> > +
> > +     return -ENOENT;
> > +#else
> > +     return -ENOTSUP;
> > +#endif
> > +}
> > +
> > +static const struct bpf_func_proto bpf_sk_check_syncookie_proto = {
> > +     .func           = bpf_sk_check_syncookie,
> > +     .gpl_only       = true,
> > +     .pkt_access     = true,
> > +     .ret_type       = RET_INTEGER,
> > +     .arg1_type      = ARG_PTR_TO_SOCKET,
> I think it should be ARG_PTR_TO_TCP_SOCK
>
> > +     .arg2_type      = ARG_PTR_TO_MEM,
> > +     .arg3_type      = ARG_CONST_SIZE,
> > +     .arg4_type      = ARG_PTR_TO_MEM,
> > +     .arg5_type      = ARG_CONST_SIZE,
> > +};
> > +
> >  #endif /* CONFIG_INET */



-- 
Lorenz Bauer  |  Systems Engineer
25 Lavington St., London SE1 0NZ

www.cloudflare.com

Re: [PATCH 1/3] bpf: add helper to check for a valid SYN cookie

[Martin Lau]

On Mon, Feb 25, 2019 at 06:26:42PM +0000, Lorenz Bauer wrote:
> On Sat, 23 Feb 2019 at 00:44, Martin Lau <kafai@fb.com> wrote:
> >
> > On Fri, Feb 22, 2019 at 09:50:55AM +0000, Lorenz Bauer wrote:
> > > Using bpf_sk_lookup_tcp it's possible to ascertain whether a packet belongs
> > > to a known connection. However, there is one corner case: no sockets are
> > > created if SYN cookies are active. This means that the final ACK in the
> > > 3WHS is misclassified.
> > >
> > > Using the helper, we can look up the listening socket via bpf_sk_lookup_tcp
> > > and then check whether a packet is a valid SYN cookie ACK.
> > >
> > > Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> > > ---
> > >  include/uapi/linux/bpf.h | 18 ++++++++++-
> > >  net/core/filter.c        | 68 ++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 85 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > index bcdd2474eee7..bc2af87e9621 100644
> > > --- a/include/uapi/linux/bpf.h
> > > +++ b/include/uapi/linux/bpf.h
> > > @@ -2359,6 +2359,21 @@ union bpf_attr {
> > >   *   Return
> > >   *           A **struct bpf_tcp_sock** pointer on success, or NULL in
> > >   *           case of failure.
> > > + *
> > > + * int bpf_sk_check_syncookie(struct bpf_sock *sk, void *iph, u32 iph_len, struct tcphdr *th, u32 th_len)
> > > + *   Description
> > > + *           Check whether iph and th contain a valid SYN cookie ACK for
> > > + *           the listening socket in sk.
> > > + *
> > > + *           iph points to the start of the IPv4 or IPv6 header, while
> > > + *           iph_len contains sizeof(struct iphdr) or sizeof(struct ip6hdr).
> > > + *
> > > + *           th points to the start of the TCP header, while th_len contains
> > > + *           sizeof(struct tcphdr).
> > > + *
> > > + *   Return
> > > + *           0 if iph and th are a valid SYN cookie ACK, or a negative error
> > > + *           otherwise.
> > >   */
> > >  #define __BPF_FUNC_MAPPER(FN)                \
> > >       FN(unspec),                     \
> > > @@ -2457,7 +2472,8 @@ union bpf_attr {
> > >       FN(spin_lock),                  \
> > >       FN(spin_unlock),                \
> > >       FN(sk_fullsock),                \
> > > -     FN(tcp_sock),
> > > +     FN(tcp_sock),                   \
> > > +     FN(sk_check_syncookie),
> > >
> > >  /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> > >   * function eBPF program intends to call
> > > diff --git a/net/core/filter.c b/net/core/filter.c
> > > index 85749f6ec789..9e68897cc7ed 100644
> > > --- a/net/core/filter.c
> > > +++ b/net/core/filter.c
> > > @@ -5426,6 +5426,70 @@ static const struct bpf_func_proto bpf_tcp_sock_proto = {
> > >       .arg1_type      = ARG_PTR_TO_SOCK_COMMON,
> > >  };
> > >
> > > +BPF_CALL_5(bpf_sk_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len,
> > s/bpf_sk_check_syncookie/bpf_tcp_check_syncookie/>
> >
> > > +        struct tcphdr *, th, u32, th_len)
> > > +{
> > > +#if IS_ENABLED(CONFIG_SYN_COOKIES)
> > nit. "#ifdef CONFIG_SYN_COOKIES" such that it is clear it is a bool kconfig.
> >
> > > +     u32 cookie;
> > > +     int ret;
> > > +
> > > +     if (unlikely(th_len < sizeof(*th)))
> > > +             return -EINVAL;
> > > +
> > > +     /* sk_listener() allows TCP_NEW_SYN_RECV, which makes no sense here. */
> > > +     if (sk->sk_protocol != IPPROTO_TCP || sk->sk_state != TCP_LISTEN)
> > From the test program in patch 3, the "sk" here is obtained from
> > bpf_sk_lookup_tcp() which does a sk_to_full_sk() before returning.
> > AFAICT, meaning bpf_sk_lookup_tcp() will return the listening sk
> > even if there is a request_sock.  Does it make sense to check
> > syncookie if there is already a request_sock?
> 
> No, that doesn't make a lot of sense. I hadn't realised that
> sk_lookup_tcp only returns full sockets.
> This means we need a way to detect that there is a request sock for a
> given tuple.
> 
> * adding a reqsk_exists(tuple) helper means we have to pay the lookup cost twice
> * drop the sk argument and do the necessary lookups in the helper
> itself, but that also
>   wastes a call to __inet_lookup_listener
> * skip sk_to_full_sk() in a helper and return RET_PTR_TO_SOCK_COMMON,
>   but that violates a bunch of assumptions (e.g. calling bpf_sk_release on them)
How about creating a new lookup helper, bpf_sk"c"_lookup_tcp,
that does not call sk_to_full_sk() before returning.
Its ".ret_type" will be RET_PTR_TO_SOCK_COMMON_OR_NULL which its
reference(-counting) state has to be tracked in the verifier also.
Mainly in check_helper_call(), iirc.

The bpf_prog can then check bpf_sock->state for TCP_LISTEN,
call bpf_tcp_sock() to get the TCP listener sock and pass to
the bpf_tcp_check_syncookie()

> 
> For context: ultimately we want use this to answer the question: does
> this (encapsulated)
> packet contain a payload destined to a local socket? Amongst the edge
> cases we need to
> handle are ICMP Packet Too Big messages and SYN cookies. A solution
> would be to hide
> all this in an "uber" helper that takes pointers to the L3 / L4
> headers and returns a verdict,
> but that seems a bit gross.
Please include this use case in the commit message.
It is useful.

> 
> >
> > > +             return -EINVAL;
> > > +
> > > +     if (!sock_net(sk)->ipv4.sysctl_tcp_syncookies)
> > Should tcp_synq_no_recent_overflow(tp) be checked also?
> >
> 
> Yes, not sure how that slipped out.
> 
> > > +             return -EINVAL;
> > > +
> > > +     if (!th->ack || th->rst)
> > How about th->syn?
> >
> 
> Yes, I missed the fact that the callers in tcp_ipv{4,6}.c check this.
> 
> > > +             return -ENOENT;
> > > +
> > > +     cookie = ntohl(th->ack_seq) - 1;
> > > +
> > > +     switch (sk->sk_family) {
> > > +     case AF_INET:
> > > +             if (unlikely(iph_len < sizeof(struct iphdr)))
> > > +                     return -EINVAL;
> > > +
> > > +             ret = __cookie_v4_check((struct iphdr *)iph, th, cookie);
> > > +             break;
> > > +
> > > +#if IS_ENABLED(CONFIG_IPV6)
> > > +     case AF_INET6:
> > > +             if (unlikely(iph_len < sizeof(struct ipv6hdr)))
> > > +                     return -EINVAL;
> > > +
> > > +             ret = __cookie_v6_check((struct ipv6hdr *)iph, th, cookie);
> > > +             break;
> > > +#endif /* CONFIG_IPV6 */
> > > +
> > > +     default:
> > > +             return -EPROTONOSUPPORT;
> > > +     }
> > > +
> > > +     if (ret > 0)
> > > +             return 0;
> > > +
> > > +     return -ENOENT;
> > > +#else
> > > +     return -ENOTSUP;
> > > +#endif
> > > +}
> > > +
> > > +static const struct bpf_func_proto bpf_sk_check_syncookie_proto = {
> > > +     .func           = bpf_sk_check_syncookie,
> > > +     .gpl_only       = true,
> > > +     .pkt_access     = true,
> > > +     .ret_type       = RET_INTEGER,
> > > +     .arg1_type      = ARG_PTR_TO_SOCKET,
> > I think it should be ARG_PTR_TO_TCP_SOCK
> >
> > > +     .arg2_type      = ARG_PTR_TO_MEM,
> > > +     .arg3_type      = ARG_CONST_SIZE,
> > > +     .arg4_type      = ARG_PTR_TO_MEM,
> > > +     .arg5_type      = ARG_CONST_SIZE,
> > > +};
> > > +
> > >  #endif /* CONFIG_INET */
> 
> 
> 
> -- 
> Lorenz Bauer  |  Systems Engineer
> 25 Lavington St., London SE1 0NZ
> 
> https://urldefense.proofpoint.com/v2/url?u=http-3A__www.cloudflare.com&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=VQnoQ7LvghIj0gVEaiQSUw&m=xhDwvX3iD-mbqSrx-L8XQNaZiYFZzMWNo_2Y38Z9j34&s=I4Ag3HflabFppFv7UtMp8WnMVSqCDW0W28ziWIvuwDE&e=

Re: [PATCH 1/3] bpf: add helper to check for a valid SYN cookie

[Lorenz Bauer]

On Tue, 26 Feb 2019 at 05:38, Martin Lau <kafai@fb.com> wrote:
>
> On Mon, Feb 25, 2019 at 06:26:42PM +0000, Lorenz Bauer wrote:
> > On Sat, 23 Feb 2019 at 00:44, Martin Lau <kafai@fb.com> wrote:
> > >
> > > On Fri, Feb 22, 2019 at 09:50:55AM +0000, Lorenz Bauer wrote:
> > > > Using bpf_sk_lookup_tcp it's possible to ascertain whether a packet belongs
> > > > to a known connection. However, there is one corner case: no sockets are
> > > > created if SYN cookies are active. This means that the final ACK in the
> > > > 3WHS is misclassified.
> > > >
> > > > Using the helper, we can look up the listening socket via bpf_sk_lookup_tcp
> > > > and then check whether a packet is a valid SYN cookie ACK.
> > > >
> > > > Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> > > > ---
> > > >  include/uapi/linux/bpf.h | 18 ++++++++++-
> > > >  net/core/filter.c        | 68 ++++++++++++++++++++++++++++++++++++++++
> > > >  2 files changed, 85 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > > index bcdd2474eee7..bc2af87e9621 100644
> > > > --- a/include/uapi/linux/bpf.h
> > > > +++ b/include/uapi/linux/bpf.h
> > > > @@ -2359,6 +2359,21 @@ union bpf_attr {
> > > >   *   Return
> > > >   *           A **struct bpf_tcp_sock** pointer on success, or NULL in
> > > >   *           case of failure.
> > > > + *
> > > > + * int bpf_sk_check_syncookie(struct bpf_sock *sk, void *iph, u32 iph_len, struct tcphdr *th, u32 th_len)
> > > > + *   Description
> > > > + *           Check whether iph and th contain a valid SYN cookie ACK for
> > > > + *           the listening socket in sk.
> > > > + *
> > > > + *           iph points to the start of the IPv4 or IPv6 header, while
> > > > + *           iph_len contains sizeof(struct iphdr) or sizeof(struct ip6hdr).
> > > > + *
> > > > + *           th points to the start of the TCP header, while th_len contains
> > > > + *           sizeof(struct tcphdr).
> > > > + *
> > > > + *   Return
> > > > + *           0 if iph and th are a valid SYN cookie ACK, or a negative error
> > > > + *           otherwise.
> > > >   */
> > > >  #define __BPF_FUNC_MAPPER(FN)                \
> > > >       FN(unspec),                     \
> > > > @@ -2457,7 +2472,8 @@ union bpf_attr {
> > > >       FN(spin_lock),                  \
> > > >       FN(spin_unlock),                \
> > > >       FN(sk_fullsock),                \
> > > > -     FN(tcp_sock),
> > > > +     FN(tcp_sock),                   \
> > > > +     FN(sk_check_syncookie),
> > > >
> > > >  /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> > > >   * function eBPF program intends to call
> > > > diff --git a/net/core/filter.c b/net/core/filter.c
> > > > index 85749f6ec789..9e68897cc7ed 100644
> > > > --- a/net/core/filter.c
> > > > +++ b/net/core/filter.c
> > > > @@ -5426,6 +5426,70 @@ static const struct bpf_func_proto bpf_tcp_sock_proto = {
> > > >       .arg1_type      = ARG_PTR_TO_SOCK_COMMON,
> > > >  };
> > > >
> > > > +BPF_CALL_5(bpf_sk_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len,
> > > s/bpf_sk_check_syncookie/bpf_tcp_check_syncookie/>
> > >
> > > > +        struct tcphdr *, th, u32, th_len)
> > > > +{
> > > > +#if IS_ENABLED(CONFIG_SYN_COOKIES)
> > > nit. "#ifdef CONFIG_SYN_COOKIES" such that it is clear it is a bool kconfig.
> > >
> > > > +     u32 cookie;
> > > > +     int ret;
> > > > +
> > > > +     if (unlikely(th_len < sizeof(*th)))
> > > > +             return -EINVAL;
> > > > +
> > > > +     /* sk_listener() allows TCP_NEW_SYN_RECV, which makes no sense here. */
> > > > +     if (sk->sk_protocol != IPPROTO_TCP || sk->sk_state != TCP_LISTEN)
> > > From the test program in patch 3, the "sk" here is obtained from
> > > bpf_sk_lookup_tcp() which does a sk_to_full_sk() before returning.
> > > AFAICT, meaning bpf_sk_lookup_tcp() will return the listening sk
> > > even if there is a request_sock.  Does it make sense to check
> > > syncookie if there is already a request_sock?
> >
> > No, that doesn't make a lot of sense. I hadn't realised that
> > sk_lookup_tcp only returns full sockets.
> > This means we need a way to detect that there is a request sock for a
> > given tuple.
> >
> > * adding a reqsk_exists(tuple) helper means we have to pay the lookup cost twice
> > * drop the sk argument and do the necessary lookups in the helper
> > itself, but that also
> >   wastes a call to __inet_lookup_listener
> > * skip sk_to_full_sk() in a helper and return RET_PTR_TO_SOCK_COMMON,
> >   but that violates a bunch of assumptions (e.g. calling bpf_sk_release on them)
> How about creating a new lookup helper, bpf_sk"c"_lookup_tcp,
> that does not call sk_to_full_sk() before returning.
> Its ".ret_type" will be RET_PTR_TO_SOCK_COMMON_OR_NULL which its
> reference(-counting) state has to be tracked in the verifier also.
> Mainly in check_helper_call(), iirc.
>
> The bpf_prog can then check bpf_sock->state for TCP_LISTEN,
> call bpf_tcp_sock() to get the TCP listener sock and pass to
> the bpf_tcp_check_syncookie()

I've started working on this, and I've hit a snag with the reference
tracking behaviour
of bpf_tcp_sock. From what I can tell, the assumption is that a PTR_TO_TCP_SOCK
doesn't need reference tracking, because its either skb->sk or a TCP listener.
In the former case, the socket is refcounted via the sk_buff, in the
latter we don't need
to worry since the eBPF is called with the RCU read lock held.

However, non-listening sockets returned by bpf_sk_lookup_tcp, can be
freed before the
end of the eBPF program. Doing bpf_sk_lookup_tcp, bpf_tcp_sock,
bpf_sk_release allows
eBPF to gain a (read-only) reference to a freed socket. I've attached
a patch with a testcase
which illustrates this issue.

Is this the intended behaviour? If not, maybe it would be the easiest
to make bpf_tcp_sock
increase the refcount if !SOCK_RCU_FREE and require a corresponding
bpf_sk_release?
That would simplify my work to add RET_PTR_TO_SOCK_COMMON as wel..

>
> >
> > For context: ultimately we want use this to answer the question: does
> > this (encapsulated)
> > packet contain a payload destined to a local socket? Amongst the edge
> > cases we need to
> > handle are ICMP Packet Too Big messages and SYN cookies. A solution
> > would be to hide
> > all this in an "uber" helper that takes pointers to the L3 / L4
> > headers and returns a verdict,
> > but that seems a bit gross.
> Please include this use case in the commit message.
> It is useful.
>
> >
> > >
> > > > +             return -EINVAL;
> > > > +
> > > > +     if (!sock_net(sk)->ipv4.sysctl_tcp_syncookies)
> > > Should tcp_synq_no_recent_overflow(tp) be checked also?
> > >
> >
> > Yes, not sure how that slipped out.
> >
> > > > +             return -EINVAL;
> > > > +
> > > > +     if (!th->ack || th->rst)
> > > How about th->syn?
> > >
> >
> > Yes, I missed the fact that the callers in tcp_ipv{4,6}.c check this.
> >
> > > > +             return -ENOENT;
> > > > +
> > > > +     cookie = ntohl(th->ack_seq) - 1;
> > > > +
> > > > +     switch (sk->sk_family) {
> > > > +     case AF_INET:
> > > > +             if (unlikely(iph_len < sizeof(struct iphdr)))
> > > > +                     return -EINVAL;
> > > > +
> > > > +             ret = __cookie_v4_check((struct iphdr *)iph, th, cookie);
> > > > +             break;
> > > > +
> > > > +#if IS_ENABLED(CONFIG_IPV6)
> > > > +     case AF_INET6:
> > > > +             if (unlikely(iph_len < sizeof(struct ipv6hdr)))
> > > > +                     return -EINVAL;
> > > > +
> > > > +             ret = __cookie_v6_check((struct ipv6hdr *)iph, th, cookie);
> > > > +             break;
> > > > +#endif /* CONFIG_IPV6 */
> > > > +
> > > > +     default:
> > > > +             return -EPROTONOSUPPORT;
> > > > +     }
> > > > +
> > > > +     if (ret > 0)
> > > > +             return 0;
> > > > +
> > > > +     return -ENOENT;
> > > > +#else
> > > > +     return -ENOTSUP;
> > > > +#endif
> > > > +}
> > > > +
> > > > +static const struct bpf_func_proto bpf_sk_check_syncookie_proto = {
> > > > +     .func           = bpf_sk_check_syncookie,
> > > > +     .gpl_only       = true,
> > > > +     .pkt_access     = true,
> > > > +     .ret_type       = RET_INTEGER,
> > > > +     .arg1_type      = ARG_PTR_TO_SOCKET,
> > > I think it should be ARG_PTR_TO_TCP_SOCK
> > >
> > > > +     .arg2_type      = ARG_PTR_TO_MEM,
> > > > +     .arg3_type      = ARG_CONST_SIZE,
> > > > +     .arg4_type      = ARG_PTR_TO_MEM,
> > > > +     .arg5_type      = ARG_CONST_SIZE,
> > > > +};
> > > > +
> > > >  #endif /* CONFIG_INET */
> >
> >
> >
> > --
> > Lorenz Bauer  |  Systems Engineer
> > 25 Lavington St., London SE1 0NZ
> >
> > https://urldefense.proofpoint.com/v2/url?u=http-3A__www.cloudflare.com&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=VQnoQ7LvghIj0gVEaiQSUw&m=xhDwvX3iD-mbqSrx-L8XQNaZiYFZzMWNo_2Y38Z9j34&s=I4Ag3HflabFppFv7UtMp8WnMVSqCDW0W28ziWIvuwDE&e=

---
 tools/testing/selftests/bpf/verifier/sock.c | 23 +++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/tools/testing/selftests/bpf/verifier/sock.c
b/tools/testing/selftests/bpf/verifier/sock.c
index 0ddfdf76aba5..3307cca6bdd5 100644
--- a/tools/testing/selftests/bpf/verifier/sock.c
+++ b/tools/testing/selftests/bpf/verifier/sock.c
@@ -382,3 +382,26 @@
        .result = REJECT,
        .errstr = "type=tcp_sock expected=sock",
 },
+{
+       "use bpf_tcp_sock after bpf_sk_release",
+       .insns = {
+       BPF_SK_LOOKUP,
+       BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
+       BPF_EXIT_INSN(),
+       BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
+       BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+       BPF_EMIT_CALL(BPF_FUNC_tcp_sock),
+       BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 3),
+       BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+       BPF_EMIT_CALL(BPF_FUNC_sk_release),
+       BPF_EXIT_INSN(),
+       BPF_MOV64_REG(BPF_REG_7, BPF_REG_0),
+       BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
+       BPF_EMIT_CALL(BPF_FUNC_sk_release),
+       BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_7, offsetof(struct
bpf_tcp_sock, snd_cwnd)),
+       BPF_EXIT_INSN(),
+       },
+       .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+       .result = REJECT,
+       .errstr = "bogus",
+},

Re: [PATCH 1/3] bpf: add helper to check for a valid SYN cookie

[Martin Lau]

On Thu, Feb 28, 2019 at 03:11:09PM +0000, Lorenz Bauer wrote:

> I've started working on this, and I've hit a snag with the reference
> tracking behaviour
> of bpf_tcp_sock. From what I can tell, the assumption is that a PTR_TO_TCP_SOCK
> doesn't need reference tracking, because its either skb->sk or a TCP listener.
> In the former case, the socket is refcounted via the sk_buff, in the
> latter we don't need
> to worry since the eBPF is called with the RCU read lock held.
> 
> However, non-listening sockets returned by bpf_sk_lookup_tcp, can be
> freed before the
> end of the eBPF program. Doing bpf_sk_lookup_tcp, bpf_tcp_sock,
> bpf_sk_release allows
> eBPF to gain a (read-only) reference to a freed socket. I've attached
> a patch with a testcase
> which illustrates this issue.
> 
> Is this the intended behaviour? If not, maybe it would be the easiest
> to make bpf_tcp_sock
> increase the refcount if !SOCK_RCU_FREE and require a corresponding
> bpf_sk_release?
Increase the refcount at runtime may be a too big hammer for this.
Let me think if it can be resolved within the verifier.

> That would simplify my work to add RET_PTR_TO_SOCK_COMMON as wel..
> 
> ---
>  tools/testing/selftests/bpf/verifier/sock.c | 23 +++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/verifier/sock.c
> b/tools/testing/selftests/bpf/verifier/sock.c
> index 0ddfdf76aba5..3307cca6bdd5 100644
> --- a/tools/testing/selftests/bpf/verifier/sock.c
> +++ b/tools/testing/selftests/bpf/verifier/sock.c
> @@ -382,3 +382,26 @@
>         .result = REJECT,
>         .errstr = "type=tcp_sock expected=sock",
>  },
> +{
> +       "use bpf_tcp_sock after bpf_sk_release",
> +       .insns = {
> +       BPF_SK_LOOKUP,
> +       BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
> +       BPF_EXIT_INSN(),
> +       BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
> +       BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
> +       BPF_EMIT_CALL(BPF_FUNC_tcp_sock),
> +       BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 3),
> +       BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
> +       BPF_EMIT_CALL(BPF_FUNC_sk_release),
> +       BPF_EXIT_INSN(),
> +       BPF_MOV64_REG(BPF_REG_7, BPF_REG_0),
> +       BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
> +       BPF_EMIT_CALL(BPF_FUNC_sk_release),
> +       BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_7, offsetof(struct
> bpf_tcp_sock, snd_cwnd)),
> +       BPF_EXIT_INSN(),
> +       },
> +       .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> +       .result = REJECT,
> +       .errstr = "bogus",
> +},
> --
> 2.19.1