[bpf-next RFC 0/6] Introduce a BPF helper to generate SYN cookies

[bpf-next RFC 0/6] Introduce a BPF helper to generate SYN cookies

[Petar Penkov]

From: Petar Penkov <ppenkov@google.com>

This patch series introduces a BPF helper function that allows generating SYN
cookies from BPF.

The first two patches in the series modify several TCP helper functions to
allow for SKB-less operation, as is the case with XDP.

The third patch introduces the bpf_tcp_gen_syncookie helper function which
generates a SYN cookie for either XDP or TC programs. 

The last three patches sync tools/ and add a test. 

The primary design consideration I see in the patch series is the return value
of the helper function. Currently bpf_tcp_gen_syncookie returns a 64-bit value
that contains both the 32-bit syncookie, and the 16-bit mss value which is
encoded in the cookie. On error, it would return a negative value instead. I
chose this over writing the cookie into the provided TCP packet to avoid writing
packet data as currently if a helper changes the packet data, the first argument
has to point to the context (can this be relaxed?). 

To make the API cleaner we can instead return something like the struct below
though the return type would then not really be RET_INTEGER or any of the
currently existing return types.
struct bpf_syncookie {
	u16 error; // or u8 error, u8 unused for future use
	u16 mss;
	u32 syncookie;
}

Petar Penkov (6):
  tcp: tcp_syn_flood_action read port from socket
  tcp: add skb-less helpers to retrieve SYN cookie
  bpf: add bpf_tcp_gen_syncookie helper
  bpf: sync bpf.h to tools/
  selftests/bpf: bpf_tcp_gen_syncookie->bpf_helpers
  selftests/bpf: add test for bpf_tcp_gen_syncookie

 include/net/tcp.h                             | 11 +++
 include/uapi/linux/bpf.h                      | 30 ++++++-
 net/core/filter.c                             | 62 +++++++++++++
 net/ipv4/tcp_input.c                          | 87 +++++++++++++++++--
 net/ipv4/tcp_ipv4.c                           |  8 ++
 net/ipv6/tcp_ipv6.c                           |  8 ++
 tools/include/uapi/linux/bpf.h                | 37 +++++++-
 tools/testing/selftests/bpf/bpf_helpers.h     |  3 +
 .../bpf/progs/test_tcp_check_syncookie_kern.c | 28 ++++--
 .../bpf/test_tcp_check_syncookie_user.c       | 61 +++++++++++--
 10 files changed, 313 insertions(+), 22 deletions(-)

-- 
2.22.0.510.g264f2c817a-goog

[bpf-next RFC 1/6] tcp: tcp_syn_flood_action read port from socket

[Petar Penkov]

From: Petar Penkov <ppenkov@google.com>

This allows us to call this function before an SKB has been
allocated.

Signed-off-by: Petar Penkov <ppenkov@google.com>
---
 net/ipv4/tcp_input.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index c21e8a22fb3b..8892df6de1d4 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -6422,9 +6422,7 @@ EXPORT_SYMBOL(inet_reqsk_alloc);
 /*
  * Return true if a syncookie should be sent
  */
-static bool tcp_syn_flood_action(const struct sock *sk,
-				 const struct sk_buff *skb,
-				 const char *proto)
+static bool tcp_syn_flood_action(const struct sock *sk, const char *proto)
 {
 	struct request_sock_queue *queue = &inet_csk(sk)->icsk_accept_queue;
 	const char *msg = "Dropping request";
@@ -6444,7 +6442,7 @@ static bool tcp_syn_flood_action(const struct sock *sk,
 	    net->ipv4.sysctl_tcp_syncookies != 2 &&
 	    xchg(&queue->synflood_warned, 1) == 0)
 		net_info_ratelimited("%s: Possible SYN flooding on port %d. %s.  Check SNMP counters.\n",
-				     proto, ntohs(tcp_hdr(skb)->dest), msg);
+				     proto, sk->sk_num, msg);
 
 	return want_cookie;
 }
@@ -6487,7 +6485,7 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
 	 */
 	if ((net->ipv4.sysctl_tcp_syncookies == 2 ||
 	     inet_csk_reqsk_queue_is_full(sk)) && !isn) {
-		want_cookie = tcp_syn_flood_action(sk, skb, rsk_ops->slab_name);
+		want_cookie = tcp_syn_flood_action(sk, rsk_ops->slab_name);
 		if (!want_cookie)
 			goto drop;
 	}
-- 
2.22.0.510.g264f2c817a-goog

[bpf-next RFC 2/6] tcp: add skb-less helpers to retrieve SYN cookie

[Petar Penkov]

From: Petar Penkov <ppenkov@google.com>

This patch allows generation of a SYN cookie before an SKB has been
allocated, as is the case at XDP.

Signed-off-by: Petar Penkov <ppenkov@google.com>
---
 include/net/tcp.h    | 11 ++++++
 net/ipv4/tcp_input.c | 79 ++++++++++++++++++++++++++++++++++++++++++++
 net/ipv4/tcp_ipv4.c  |  8 +++++
 net/ipv6/tcp_ipv6.c  |  8 +++++
 4 files changed, 106 insertions(+)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index cca3c59b98bf..a128e22c0d5d 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -414,6 +414,17 @@ void tcp_parse_options(const struct net *net, const struct sk_buff *skb,
 		       int estab, struct tcp_fastopen_cookie *foc);
 const u8 *tcp_parse_md5sig_option(const struct tcphdr *th);
 
+/*
+ *	BPF SKB-less helpers
+ */
+u16 tcp_v4_get_syncookie(struct sock *sk, struct iphdr *iph,
+			 struct tcphdr *tch, u32 *cookie);
+u16 tcp_v6_get_syncookie(struct sock *sk, struct ipv6hdr *iph,
+			 struct tcphdr *tch, u32 *cookie);
+u16 tcp_get_syncookie(struct request_sock_ops *rsk_ops,
+		      const struct tcp_request_sock_ops *af_ops,
+		      struct sock *sk, void *iph, struct tcphdr *tch,
+		      u32 *cookie);
 /*
  *	TCP v4 functions exported for the inet6 API
  */
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 8892df6de1d4..1406d7e0953c 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -3782,6 +3782,52 @@ static void smc_parse_options(const struct tcphdr *th,
 #endif
 }
 
+/* Try to parse the MSS option from the TCP header. Return 0 on failure, clamped
+ * value on success.
+ *
+ * Invoked for BPF SYN cookie generation, so th should be a SYN.
+ */
+static u16 tcp_parse_mss_option(const struct net *net, const struct tcphdr *th,
+				u16 user_mss)
+{
+	const unsigned char *ptr = (const unsigned char *)(th + 1);
+	int length = (th->doff * 4) - sizeof(struct tcphdr);
+	u16 mss = 0;
+
+	while (length > 0) {
+		int opcode = *ptr++;
+		int opsize;
+
+		switch (opcode) {
+		case TCPOPT_EOL:
+			return mss;
+		case TCPOPT_NOP:	/* Ref: RFC 793 section 3.1 */
+			length--;
+			continue;
+		default:
+			if (length < 2)
+				return mss;
+			opsize = *ptr++;
+			if (opsize < 2) /* "silly options" */
+				return mss;
+			if (opsize > length)
+				return mss;	/* fail on partial options */
+			if (opcode == TCPOPT_MSS && opsize == TCPOLEN_MSS) {
+				u16 in_mss = get_unaligned_be16(ptr);
+
+				if (in_mss) {
+					if (user_mss && user_mss < in_mss)
+						in_mss = user_mss;
+					mss = in_mss;
+				}
+			}
+			ptr += opsize - 2;
+			length -= opsize;
+		}
+	}
+	return mss;
+}
+
 /* Look for tcp options. Normally only called on SYN and SYNACK packets.
  * But, this can also be called on packets in the established flow when
  * the fast version below fails.
@@ -6464,6 +6510,39 @@ static void tcp_reqsk_record_syn(const struct sock *sk,
 	}
 }
 
+u16 tcp_get_syncookie(struct request_sock_ops *rsk_ops,
+		      const struct tcp_request_sock_ops *af_ops,
+		      struct sock *sk, void *iph, struct tcphdr *th,
+		      u32 *cookie)
+{
+	u16 mss = 0;
+#ifdef CONFIG_SYN_COOKIES
+	bool is_v4 = rsk_ops->family == AF_INET;
+	struct tcp_sock *tp = tcp_sk(sk);
+
+	if (sock_net(sk)->ipv4.sysctl_tcp_syncookies != 2 &&
+	    !inet_csk_reqsk_queue_is_full(sk))
+		return 0;
+
+	if (!tcp_syn_flood_action(sk, rsk_ops->slab_name))
+		return 0;
+
+	if (sk_acceptq_is_full(sk)) {
+		NET_INC_STATS(sock_net(sk), LINUX_MIB_LISTENOVERFLOWS);
+		return 0;
+	}
+
+	mss = tcp_parse_mss_option(sock_net(sk), th, tp->rx_opt.user_mss);
+	if (!mss)
+		mss = af_ops->mss_clamp;
+
+	tcp_synq_overflow(sk);
+	*cookie = is_v4 ? __cookie_v4_init_sequence(iph, th, &mss)
+			: __cookie_v6_init_sequence(iph, th, &mss);
+#endif
+	return mss;
+}
+
 int tcp_conn_request(struct request_sock_ops *rsk_ops,
 		     const struct tcp_request_sock_ops *af_ops,
 		     struct sock *sk, struct sk_buff *skb)
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index d57641cb3477..0e06e59784bd 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1515,6 +1515,14 @@ static struct sock *tcp_v4_cookie_check(struct sock *sk, struct sk_buff *skb)
 	return sk;
 }
 
+u16 tcp_v4_get_syncookie(struct sock *sk, struct iphdr *iph,
+			 struct tcphdr *tch, u32 *cookie)
+{
+	return tcp_get_syncookie(&tcp_request_sock_ops,
+				 &tcp_request_sock_ipv4_ops, sk, iph, tch,
+				 cookie);
+}
+
 /* The socket must have it's spinlock held when we get
  * here, unless it is a TCP_LISTEN socket.
  *
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index d56a9019a0fe..ce46cdba54bc 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1058,6 +1058,14 @@ static struct sock *tcp_v6_cookie_check(struct sock *sk, struct sk_buff *skb)
 	return sk;
 }
 
+u16 tcp_v6_get_syncookie(struct sock *sk, struct ipv6hdr *iph,
+			 struct tcphdr *tch, u32 *cookie)
+{
+	return tcp_get_syncookie(&tcp6_request_sock_ops,
+				 &tcp_request_sock_ipv6_ops, sk, iph, tch,
+				 cookie);
+}
+
 static int tcp_v6_conn_request(struct sock *sk, struct sk_buff *skb)
 {
 	if (skb->protocol == htons(ETH_P_IP))
-- 
2.22.0.510.g264f2c817a-goog

[bpf-next RFC 3/6] bpf: add bpf_tcp_gen_syncookie helper

[Petar Penkov]

From: Petar Penkov <ppenkov@google.com>

This helper function allows BPF programs to try to generate SYN
cookies, given a reference to a listener socket. The function works
from XDP and with an skb context since bpf_skc_lookup_tcp can lookup a
socket in both cases.

Signed-off-by: Petar Penkov <ppenkov@google.com>
Suggested-by: Eric Dumazet <edumazet@google.com>
---
 include/uapi/linux/bpf.h | 30 ++++++++++++++++++-
 net/core/filter.c        | 62 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 91 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 6f68438aa4ed..abf4a85c76d1 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -2713,6 +2713,33 @@ union bpf_attr {
  *		**-EPERM** if no permission to send the *sig*.
  *
  *		**-EAGAIN** if bpf program can try again.
+ *
+ * s64 bpf_tcp_gen_syncookie(struct bpf_sock *sk, void *iph, u32 iph_len, struct tcphdr *th, u32 th_len)
+ *	Description
+ *		Try to issue a SYN cookie for the packet with corresponding
+ *		IP/TCP headers, *iph* and *th*, on 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
+ *		On success, lower 32 bits hold the generated SYN cookie in
+ *		network order and the higher 32 bits hold the MSS value for that
+ *		cookie.
+ *
+ *		On failure, the returned value is one of the following:
+ *
+ *		**-EINVAL** SYN cookie cannot be issued due to error
+ *
+ *		**-ENOENT** SYN cookie should not be issued (no SYN flood)
+ *
+ *		**-ENOTSUPP** kernel configuration does not enable SYN cookies
+ *
+ *		**-EPROTONOSUPPORT** *sk* family is not AF_INET/AF_INET6
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -2824,7 +2851,8 @@ union bpf_attr {
 	FN(strtoul),			\
 	FN(sk_storage_get),		\
 	FN(sk_storage_delete),		\
-	FN(send_signal),
+	FN(send_signal),		\
+	FN(tcp_gen_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 47f6386fb17a..109fd1e286f4 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5850,6 +5850,64 @@ static const struct bpf_func_proto bpf_tcp_check_syncookie_proto = {
 	.arg5_type	= ARG_CONST_SIZE,
 };
 
+BPF_CALL_5(bpf_tcp_gen_syncookie, struct sock *, sk, void *, iph, u32, iph_len,
+	   struct tcphdr *, th, u32, th_len)
+{
+#ifdef CONFIG_SYN_COOKIES
+	u32 cookie;
+	u16 mss;
+
+	if (unlikely(th_len < sizeof(*th)))
+		return -EINVAL;
+
+	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->syn || th->ack || th->fin || th->rst)
+		return -EINVAL;
+
+	switch (sk->sk_family) {
+	case AF_INET:
+		if (unlikely(iph_len < sizeof(struct iphdr)))
+			return -EINVAL;
+		mss = tcp_v4_get_syncookie(sk, iph, th, &cookie);
+		break;
+
+#if IS_BUILTIN(CONFIG_IPV6)
+	case AF_INET6:
+		if (unlikely(iph_len < sizeof(struct ipv6hdr)))
+			return -EINVAL;
+		mss = tcp_v6_get_syncookie(sk, iph, th, &cookie);
+		break;
+#endif /* CONFIG_IPV6 */
+
+	default:
+		return -EPROTONOSUPPORT;
+	}
+	if (mss <= 0)
+		return -ENOENT;
+
+	return htonl(cookie) | ((u64)mss << 32);
+#else
+	return -ENOTSUPP;
+#endif /* CONFIG_SYN_COOKIES */
+}
+
+static const struct bpf_func_proto bpf_tcp_gen_syncookie_proto = {
+	.func		= bpf_tcp_gen_syncookie,
+	.gpl_only	= true, /* __cookie_v*_init_sequence() is GPL */
+	.pkt_access	= true,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_PTR_TO_SOCK_COMMON,
+	.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)
@@ -6135,6 +6193,8 @@ tc_cls_act_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_tcp_check_syncookie_proto;
 	case BPF_FUNC_skb_ecn_set_ce:
 		return &bpf_skb_ecn_set_ce_proto;
+	case BPF_FUNC_tcp_gen_syncookie:
+		return &bpf_tcp_gen_syncookie_proto;
 #endif
 	default:
 		return bpf_base_func_proto(func_id);
@@ -6174,6 +6234,8 @@ xdp_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_xdp_skc_lookup_tcp_proto;
 	case BPF_FUNC_tcp_check_syncookie:
 		return &bpf_tcp_check_syncookie_proto;
+	case BPF_FUNC_tcp_gen_syncookie:
+		return &bpf_tcp_gen_syncookie_proto;
 #endif
 	default:
 		return bpf_base_func_proto(func_id);
-- 
2.22.0.510.g264f2c817a-goog

[bpf-next RFC 4/6] bpf: sync bpf.h to tools/

[Petar Penkov]

From: Petar Penkov <ppenkov@google.com>

Sync updated documentation for bpf_redirect_map.

Sync the bpf_tcp_gen_syncookie helper function definition with the one
in tools/uapi.

Signed-off-by: Petar Penkov <ppenkov@google.com>
---
 tools/include/uapi/linux/bpf.h | 37 +++++++++++++++++++++++++++++++---
 1 file changed, 34 insertions(+), 3 deletions(-)

diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index f506c68b2612..abf4a85c76d1 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1571,8 +1571,11 @@ union bpf_attr {
  * 		but this is only implemented for native XDP (with driver
  * 		support) as of this writing).
  *
- * 		All values for *flags* are reserved for future usage, and must
- * 		be left at zero.
+ * 		The lower two bits of *flags* are used as the return code if
+ * 		the map lookup fails. This is so that the return value can be
+ * 		one of the XDP program return codes up to XDP_TX, as chosen by
+ * 		the caller. Any higher bits in the *flags* argument must be
+ * 		unset.
  *
  * 		When used to redirect packets to net devices, this helper
  * 		provides a high performance increase over **bpf_redirect**\ ().
@@ -2710,6 +2713,33 @@ union bpf_attr {
  *		**-EPERM** if no permission to send the *sig*.
  *
  *		**-EAGAIN** if bpf program can try again.
+ *
+ * s64 bpf_tcp_gen_syncookie(struct bpf_sock *sk, void *iph, u32 iph_len, struct tcphdr *th, u32 th_len)
+ *	Description
+ *		Try to issue a SYN cookie for the packet with corresponding
+ *		IP/TCP headers, *iph* and *th*, on 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
+ *		On success, lower 32 bits hold the generated SYN cookie in
+ *		network order and the higher 32 bits hold the MSS value for that
+ *		cookie.
+ *
+ *		On failure, the returned value is one of the following:
+ *
+ *		**-EINVAL** SYN cookie cannot be issued due to error
+ *
+ *		**-ENOENT** SYN cookie should not be issued (no SYN flood)
+ *
+ *		**-ENOTSUPP** kernel configuration does not enable SYN cookies
+ *
+ *		**-EPROTONOSUPPORT** *sk* family is not AF_INET/AF_INET6
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -2821,7 +2851,8 @@ union bpf_attr {
 	FN(strtoul),			\
 	FN(sk_storage_get),		\
 	FN(sk_storage_delete),		\
-	FN(send_signal),
+	FN(send_signal),		\
+	FN(tcp_gen_syncookie),
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
  * function eBPF program intends to call
-- 
2.22.0.510.g264f2c817a-goog

[bpf-next RFC 5/6] selftests/bpf: bpf_tcp_gen_syncookie->bpf_helpers

[Petar Penkov]

From: Petar Penkov <ppenkov@google.com>

Expose bpf_tcp_gen_syncookie to selftests.

Signed-off-by: Petar Penkov <ppenkov@google.com>
---
 tools/testing/selftests/bpf/bpf_helpers.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
index 5a3d92c8bec8..19f01e967402 100644
--- a/tools/testing/selftests/bpf/bpf_helpers.h
+++ b/tools/testing/selftests/bpf/bpf_helpers.h
@@ -228,6 +228,9 @@ static void *(*bpf_sk_storage_get)(void *map, struct bpf_sock *sk,
 static int (*bpf_sk_storage_delete)(void *map, struct bpf_sock *sk) =
 	(void *)BPF_FUNC_sk_storage_delete;
 static int (*bpf_send_signal)(unsigned sig) = (void *)BPF_FUNC_send_signal;
+static long long (*bpf_tcp_gen_syncookie)(struct bpf_sock *sk, void *ip,
+					  int ip_len, void *tcp, int tcp_len) =
+	(void *) BPF_FUNC_tcp_gen_syncookie;
 
 /* llvm builtin functions that eBPF C program may use to
  * emit BPF_LD_ABS and BPF_LD_IND instructions
-- 
2.22.0.510.g264f2c817a-goog

[bpf-next RFC 6/6] selftests/bpf: add test for bpf_tcp_gen_syncookie

[Petar Penkov]

From: Petar Penkov <ppenkov@google.com>

Modify the existing bpf_tcp_check_syncookie test to also generate a
SYN cookie, pass the packet to the kernel, and verify that the two
cookies are the same (and both valid). Since cloned SKBs are skipped
during generic XDP, this test does not issue a SYN cookie when run in
XDP mode. We therefore only check that a valid SYN cookie was issued at
the TC hook.

Additionally, verify that the MSS for that SYN cookie is within
expected range.

Signed-off-by: Petar Penkov <ppenkov@google.com>
---
 .../bpf/progs/test_tcp_check_syncookie_kern.c | 28 +++++++--
 .../bpf/test_tcp_check_syncookie_user.c       | 61 ++++++++++++++++---
 2 files changed, 76 insertions(+), 13 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/test_tcp_check_syncookie_kern.c b/tools/testing/selftests/bpf/progs/test_tcp_check_syncookie_kern.c
index 1ab095bcacd8..229832766f42 100644
--- a/tools/testing/selftests/bpf/progs/test_tcp_check_syncookie_kern.c
+++ b/tools/testing/selftests/bpf/progs/test_tcp_check_syncookie_kern.c
@@ -19,8 +19,8 @@
 struct bpf_map_def SEC("maps") results = {
 	.type = BPF_MAP_TYPE_ARRAY,
 	.key_size = sizeof(__u32),
-	.value_size = sizeof(__u64),
-	.max_entries = 1,
+	.value_size = sizeof(__u32),
+	.max_entries = 3,
 };
 
 static __always_inline void check_syncookie(void *ctx, void *data,
@@ -33,8 +33,10 @@ static __always_inline void check_syncookie(void *ctx, void *data,
 	struct ipv6hdr *ipv6h;
 	struct tcphdr *tcph;
 	int ret;
+	__u32 key_mss = 2;
+	__u32 key_gen = 1;
 	__u32 key = 0;
-	__u64 value = 1;
+	__s64 seq_mss;
 
 	ethh = data;
 	if (ethh + 1 > data_end)
@@ -66,6 +68,8 @@ static __always_inline void check_syncookie(void *ctx, void *data,
 		if (sk->state != BPF_TCP_LISTEN)
 			goto release;
 
+		seq_mss = bpf_tcp_gen_syncookie(sk, ipv4h, sizeof(*ipv4h),
+						tcph, sizeof(*tcph));
 		ret = bpf_tcp_check_syncookie(sk, ipv4h, sizeof(*ipv4h),
 					      tcph, sizeof(*tcph));
 		break;
@@ -95,6 +99,9 @@ static __always_inline void check_syncookie(void *ctx, void *data,
 		if (sk->state != BPF_TCP_LISTEN)
 			goto release;
 
+		seq_mss = bpf_tcp_gen_syncookie(sk, ipv6h, sizeof(*ipv6h),
+						tcph, sizeof(*tcph));
+
 		ret = bpf_tcp_check_syncookie(sk, ipv6h, sizeof(*ipv6h),
 					      tcph, sizeof(*tcph));
 		break;
@@ -103,8 +110,19 @@ static __always_inline void check_syncookie(void *ctx, void *data,
 		return;
 	}
 
-	if (ret == 0)
-		bpf_map_update_elem(&results, &key, &value, 0);
+	if (seq_mss > 0) {
+		__u32 cookie = bpf_ntohl((__u32)seq_mss);
+		__u32 mss = seq_mss >> 32;
+
+		bpf_map_update_elem(&results, &key_gen, &cookie, 0);
+		bpf_map_update_elem(&results, &key_mss, &mss, 0);
+	}
+
+	if (ret == 0) {
+		__u32 cookie = bpf_ntohl(tcph->ack_seq) - 1;
+
+		bpf_map_update_elem(&results, &key, &cookie, 0);
+	}
 
 release:
 	bpf_sk_release(sk);
diff --git a/tools/testing/selftests/bpf/test_tcp_check_syncookie_user.c b/tools/testing/selftests/bpf/test_tcp_check_syncookie_user.c
index 87829c86c746..f3ff49ceb481 100644
--- a/tools/testing/selftests/bpf/test_tcp_check_syncookie_user.c
+++ b/tools/testing/selftests/bpf/test_tcp_check_syncookie_user.c
@@ -2,6 +2,7 @@
 // Copyright (c) 2018 Facebook
 // Copyright (c) 2019 Cloudflare
 
+#include <limits.h>
 #include <string.h>
 #include <stdlib.h>
 #include <unistd.h>
@@ -77,7 +78,7 @@ static int connect_to_server(int server_fd)
 	return fd;
 }
 
-static int get_map_fd_by_prog_id(int prog_id)
+static int get_map_fd_by_prog_id(int prog_id, bool *xdp)
 {
 	struct bpf_prog_info info = {};
 	__u32 info_len = sizeof(info);
@@ -104,6 +105,8 @@ static int get_map_fd_by_prog_id(int prog_id)
 		goto err;
 	}
 
+	*xdp = info.type == BPF_PROG_TYPE_XDP;
+
 	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]);
@@ -113,18 +116,32 @@ static int get_map_fd_by_prog_id(int prog_id)
 	return map_fd;
 }
 
-static int run_test(int server_fd, int results_fd)
+static int run_test(int server_fd, int results_fd, bool xdp)
 {
 	int client = -1, srv_client = -1;
 	int ret = 0;
 	__u32 key = 0;
-	__u64 value = 0;
+	__u32 key_gen = 1;
+	__u32 key_mss = 2;
+	__u32 value = 0;
+	__u32 value_gen = 0;
+	__u32 value_mss = 0;
 
 	if (bpf_map_update_elem(results_fd, &key, &value, 0) < 0) {
 		log_err("Can't clear results");
 		goto err;
 	}
 
+	if (bpf_map_update_elem(results_fd, &key_gen, &value_gen, 0) < 0) {
+		log_err("Can't clear results");
+		goto err;
+	}
+
+	if (bpf_map_update_elem(results_fd, &key_mss, &value_mss, 0) < 0) {
+		log_err("Can't clear results");
+		goto err;
+	}
+
 	client = connect_to_server(server_fd);
 	if (client == -1)
 		goto err;
@@ -140,8 +157,35 @@ static int run_test(int server_fd, int results_fd)
 		goto err;
 	}
 
-	if (value != 1) {
-		log_err("Didn't match syncookie: %llu", value);
+	if (value == 0) {
+		log_err("Didn't match syncookie: %u", value);
+		goto err;
+	}
+
+	if (bpf_map_lookup_elem(results_fd, &key_gen, &value_gen) < 0) {
+		log_err("Can't lookup result");
+		goto err;
+	}
+
+	if (xdp && value_gen == 0) {
+		// SYN packets do not get passed through generic XDP, skip the
+		// rest of the test.
+		log_err("Did not find SYN cookie at XDP.");
+		goto out;
+	}
+
+	if (bpf_map_lookup_elem(results_fd, &key_mss, &value_mss) < 0) {
+		log_err("Can't lookup result");
+		goto err;
+	}
+
+	if (value != value_gen) {
+		log_err("BPF generated cookie does not match kernel one");
+		goto err;
+	}
+
+	if (value_mss < 536 || value_mss > USHRT_MAX) {
+		log_err("Unexpected MSS retrieved");
 		goto err;
 	}
 
@@ -163,13 +207,14 @@ int main(int argc, char **argv)
 	int server_v6 = -1;
 	int results = -1;
 	int err = 0;
+	bool xdp;
 
 	if (argc < 2) {
 		fprintf(stderr, "Usage: %s prog_id\n", argv[0]);
 		exit(1);
 	}
 
-	results = get_map_fd_by_prog_id(atoi(argv[1]));
+	results = get_map_fd_by_prog_id(atoi(argv[1]), &xdp);
 	if (results < 0) {
 		log_err("Can't get map");
 		goto err;
@@ -194,10 +239,10 @@ int main(int argc, char **argv)
 	if (server_v6 == -1)
 		goto err;
 
-	if (run_test(server, results))
+	if (run_test(server, results, xdp))
 		goto err;
 
-	if (run_test(server_v6, results))
+	if (run_test(server_v6, results, xdp))
 		goto err;
 
 	printf("ok\n");
-- 
2.22.0.510.g264f2c817a-goog

Re: [bpf-next RFC 3/6] bpf: add bpf_tcp_gen_syncookie helper

[Eric Dumazet]

On 7/16/19 2:26 AM, Petar Penkov wrote:
> From: Petar Penkov <ppenkov@google.com>
> 
> This helper function allows BPF programs to try to generate SYN
> cookies, given a reference to a listener socket. The function works
> from XDP and with an skb context since bpf_skc_lookup_tcp can lookup a
> socket in both cases.
> 
...
>  
> +BPF_CALL_5(bpf_tcp_gen_syncookie, struct sock *, sk, void *, iph, u32, iph_len,
> +	   struct tcphdr *, th, u32, th_len)
> +{
> +#ifdef CONFIG_SYN_COOKIES
> +	u32 cookie;
> +	u16 mss;
> +
> +	if (unlikely(th_len < sizeof(*th)))


You probably need to check that th_len == th->doff * 4

> +		return -EINVAL;
> +
> +	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->syn || th->ack || th->fin || th->rst)
> +		return -EINVAL;
> +
> +	switch (sk->sk_family) {

This is strange, because a dual stack listener will have sk->sk_family set to AF_INET6.

What really matters here is if the packet is IPv4 or IPv6.

So you need to look at iph->version instead.

Then look if the socket family allows this packet to be processed
(For example AF_INET6 sockets might prevent IPv4 packets, see sk->sk_ipv6only )

> +	case AF_INET:
> +		if (unlikely(iph_len < sizeof(struct iphdr)))
> +			return -EINVAL;
> +		mss = tcp_v4_get_syncookie(sk, iph, th, &cookie);
> +		break;
> +
> +#if IS_BUILTIN(CONFIG_IPV6)
> +	case AF_INET6:
> +		if (unlikely(iph_len < sizeof(struct ipv6hdr)))
> +			return -EINVAL;
> +		mss = tcp_v6_get_syncookie(sk, iph, th, &cookie);
> +		break;
> +#endif /* CONFIG_IPV6 */
> +
> +	default:
> +		return -EPROTONOSUPPORT;
> +	}
> +	if (mss <= 0)
> +		return -ENOENT;
> +

Re: [bpf-next RFC 2/6] tcp: add skb-less helpers to retrieve SYN cookie

[Lorenz Bauer]

On Tue, 16 Jul 2019 at 01:27, Petar Penkov <ppenkov.kernel@gmail.com> wrote:
>
> From: Petar Penkov <ppenkov@google.com>
>
> This patch allows generation of a SYN cookie before an SKB has been
> allocated, as is the case at XDP.
>
> Signed-off-by: Petar Penkov <ppenkov@google.com>
> ---
>  include/net/tcp.h    | 11 ++++++
>  net/ipv4/tcp_input.c | 79 ++++++++++++++++++++++++++++++++++++++++++++
>  net/ipv4/tcp_ipv4.c  |  8 +++++
>  net/ipv6/tcp_ipv6.c  |  8 +++++
>  4 files changed, 106 insertions(+)
>
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index cca3c59b98bf..a128e22c0d5d 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -414,6 +414,17 @@ void tcp_parse_options(const struct net *net, const struct sk_buff *skb,
>                        int estab, struct tcp_fastopen_cookie *foc);
>  const u8 *tcp_parse_md5sig_option(const struct tcphdr *th);
>
> +/*
> + *     BPF SKB-less helpers
> + */
> +u16 tcp_v4_get_syncookie(struct sock *sk, struct iphdr *iph,
> +                        struct tcphdr *tch, u32 *cookie);
> +u16 tcp_v6_get_syncookie(struct sock *sk, struct ipv6hdr *iph,
> +                        struct tcphdr *tch, u32 *cookie);
> +u16 tcp_get_syncookie(struct request_sock_ops *rsk_ops,
> +                     const struct tcp_request_sock_ops *af_ops,
> +                     struct sock *sk, void *iph, struct tcphdr *tch,
> +                     u32 *cookie);
>  /*
>   *     TCP v4 functions exported for the inet6 API
>   */
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 8892df6de1d4..1406d7e0953c 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -3782,6 +3782,52 @@ static void smc_parse_options(const struct tcphdr *th,
>  #endif
>  }
>
> +/* Try to parse the MSS option from the TCP header. Return 0 on failure, clamped
> + * value on success.
> + *
> + * Invoked for BPF SYN cookie generation, so th should be a SYN.
> + */
> +static u16 tcp_parse_mss_option(const struct net *net, const struct tcphdr *th,
> +                               u16 user_mss)

net seems unused?

> +{
> +       const unsigned char *ptr = (const unsigned char *)(th + 1);
> +       int length = (th->doff * 4) - sizeof(struct tcphdr);
> +       u16 mss = 0;
> +
> +       while (length > 0) {
> +               int opcode = *ptr++;
> +               int opsize;
> +
> +               switch (opcode) {
> +               case TCPOPT_EOL:
> +                       return mss;
> +               case TCPOPT_NOP:        /* Ref: RFC 793 section 3.1 */
> +                       length--;
> +                       continue;
> +               default:
> +                       if (length < 2)
> +                               return mss;
> +                       opsize = *ptr++;
> +                       if (opsize < 2) /* "silly options" */
> +                               return mss;
> +                       if (opsize > length)
> +                               return mss;     /* fail on partial options */
> +                       if (opcode == TCPOPT_MSS && opsize == TCPOLEN_MSS) {
> +                               u16 in_mss = get_unaligned_be16(ptr);
> +
> +                               if (in_mss) {
> +                                       if (user_mss && user_mss < in_mss)
> +                                               in_mss = user_mss;
> +                                       mss = in_mss;
> +                               }
> +                       }
> +                       ptr += opsize - 2;
> +                       length -= opsize;
> +               }
> +       }
> +       return mss;
> +}
> +
>  /* Look for tcp options. Normally only called on SYN and SYNACK packets.
>   * But, this can also be called on packets in the established flow when
>   * the fast version below fails.
> @@ -6464,6 +6510,39 @@ static void tcp_reqsk_record_syn(const struct sock *sk,
>         }
>  }
>
> +u16 tcp_get_syncookie(struct request_sock_ops *rsk_ops,
> +                     const struct tcp_request_sock_ops *af_ops,
> +                     struct sock *sk, void *iph, struct tcphdr *th,
> +                     u32 *cookie)
> +{
> +       u16 mss = 0;
> +#ifdef CONFIG_SYN_COOKIES
> +       bool is_v4 = rsk_ops->family == AF_INET;
> +       struct tcp_sock *tp = tcp_sk(sk);
> +
> +       if (sock_net(sk)->ipv4.sysctl_tcp_syncookies != 2 &&
> +           !inet_csk_reqsk_queue_is_full(sk))
> +               return 0;
> +
> +       if (!tcp_syn_flood_action(sk, rsk_ops->slab_name))
> +               return 0;
> +
> +       if (sk_acceptq_is_full(sk)) {
> +               NET_INC_STATS(sock_net(sk), LINUX_MIB_LISTENOVERFLOWS);
> +               return 0;
> +       }
> +
> +       mss = tcp_parse_mss_option(sock_net(sk), th, tp->rx_opt.user_mss);
> +       if (!mss)
> +               mss = af_ops->mss_clamp;
> +
> +       tcp_synq_overflow(sk);
> +       *cookie = is_v4 ? __cookie_v4_init_sequence(iph, th, &mss)
> +                       : __cookie_v6_init_sequence(iph, th, &mss);
> +#endif
> +       return mss;
> +}
> +
>  int tcp_conn_request(struct request_sock_ops *rsk_ops,
>                      const struct tcp_request_sock_ops *af_ops,
>                      struct sock *sk, struct sk_buff *skb)
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index d57641cb3477..0e06e59784bd 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -1515,6 +1515,14 @@ static struct sock *tcp_v4_cookie_check(struct sock *sk, struct sk_buff *skb)
>         return sk;
>  }
>
> +u16 tcp_v4_get_syncookie(struct sock *sk, struct iphdr *iph,
> +                        struct tcphdr *tch, u32 *cookie)
> +{
> +       return tcp_get_syncookie(&tcp_request_sock_ops,
> +                                &tcp_request_sock_ipv4_ops, sk, iph, tch,
> +                                cookie);
> +}
> +
>  /* The socket must have it's spinlock held when we get
>   * here, unless it is a TCP_LISTEN socket.
>   *
> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> index d56a9019a0fe..ce46cdba54bc 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c
> @@ -1058,6 +1058,14 @@ static struct sock *tcp_v6_cookie_check(struct sock *sk, struct sk_buff *skb)
>         return sk;
>  }
>
> +u16 tcp_v6_get_syncookie(struct sock *sk, struct ipv6hdr *iph,
> +                        struct tcphdr *tch, u32 *cookie)
> +{
> +       return tcp_get_syncookie(&tcp6_request_sock_ops,
> +                                &tcp_request_sock_ipv6_ops, sk, iph, tch,
> +                                cookie);
> +}
> +
>  static int tcp_v6_conn_request(struct sock *sk, struct sk_buff *skb)
>  {
>         if (skb->protocol == htons(ETH_P_IP))
> --
> 2.22.0.510.g264f2c817a-goog
>


-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com

Re: [bpf-next RFC 3/6] bpf: add bpf_tcp_gen_syncookie helper

[Lorenz Bauer]

On Tue, 16 Jul 2019 at 01:27, Petar Penkov <ppenkov.kernel@gmail.com> wrote:
>
> From: Petar Penkov <ppenkov@google.com>
>
> This helper function allows BPF programs to try to generate SYN
> cookies, given a reference to a listener socket. The function works
> from XDP and with an skb context since bpf_skc_lookup_tcp can lookup a
> socket in both cases.
>
> Signed-off-by: Petar Penkov <ppenkov@google.com>
> Suggested-by: Eric Dumazet <edumazet@google.com>
> ---
>  include/uapi/linux/bpf.h | 30 ++++++++++++++++++-
>  net/core/filter.c        | 62 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 91 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 6f68438aa4ed..abf4a85c76d1 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -2713,6 +2713,33 @@ union bpf_attr {
>   *             **-EPERM** if no permission to send the *sig*.
>   *
>   *             **-EAGAIN** if bpf program can try again.
> + *
> + * s64 bpf_tcp_gen_syncookie(struct bpf_sock *sk, void *iph, u32 iph_len, struct tcphdr *th, u32 th_len)
> + *     Description
> + *             Try to issue a SYN cookie for the packet with corresponding
> + *             IP/TCP headers, *iph* and *th*, on 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
> + *             On success, lower 32 bits hold the generated SYN cookie in
> + *             network order and the higher 32 bits hold the MSS value for that
> + *             cookie.

I prefer returning the cookie vs. directly creating the packet.
Returning a struct would
be nicest, but it doesn't seem worth it to extend the verifier for
that. Taking a pointer
to a result struct would also be good, but we're out of arguments :/

Nit: the MSS is only 16 bit?

> + *
> + *             On failure, the returned value is one of the following:
> + *
> + *             **-EINVAL** SYN cookie cannot be issued due to error
> + *
> + *             **-ENOENT** SYN cookie should not be issued (no SYN flood)
> + *
> + *             **-ENOTSUPP** kernel configuration does not enable SYN cookies
> + *
> + *             **-EPROTONOSUPPORT** *sk* family is not AF_INET/AF_INET6
>   */
>  #define __BPF_FUNC_MAPPER(FN)          \
>         FN(unspec),                     \
> @@ -2824,7 +2851,8 @@ union bpf_attr {
>         FN(strtoul),                    \
>         FN(sk_storage_get),             \
>         FN(sk_storage_delete),          \
> -       FN(send_signal),
> +       FN(send_signal),                \
> +       FN(tcp_gen_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 47f6386fb17a..109fd1e286f4 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -5850,6 +5850,64 @@ static const struct bpf_func_proto bpf_tcp_check_syncookie_proto = {
>         .arg5_type      = ARG_CONST_SIZE,
>  };
>
> +BPF_CALL_5(bpf_tcp_gen_syncookie, struct sock *, sk, void *, iph, u32, iph_len,
> +          struct tcphdr *, th, u32, th_len)
> +{
> +#ifdef CONFIG_SYN_COOKIES
> +       u32 cookie;
> +       u16 mss;
> +
> +       if (unlikely(th_len < sizeof(*th)))
> +               return -EINVAL;
> +
> +       if (sk->sk_protocol != IPPROTO_TCP || sk->sk_state != TCP_LISTEN)
> +               return -EINVAL;
> +
> +       if (!sock_net(sk)->ipv4.sysctl_tcp_syncookies)
> +               return -EINVAL;

Should this be ENOENT instead?

> +
> +       if (!th->syn || th->ack || th->fin || th->rst)
> +               return -EINVAL;
> +
> +       switch (sk->sk_family) {
> +       case AF_INET:
> +               if (unlikely(iph_len < sizeof(struct iphdr)))
> +                       return -EINVAL;
> +               mss = tcp_v4_get_syncookie(sk, iph, th, &cookie);
> +               break;
> +
> +#if IS_BUILTIN(CONFIG_IPV6)
> +       case AF_INET6:
> +               if (unlikely(iph_len < sizeof(struct ipv6hdr)))
> +                       return -EINVAL;
> +               mss = tcp_v6_get_syncookie(sk, iph, th, &cookie);
> +               break;
> +#endif /* CONFIG_IPV6 */
> +
> +       default:
> +               return -EPROTONOSUPPORT;
> +       }
> +       if (mss <= 0)
> +               return -ENOENT;
> +
> +       return htonl(cookie) | ((u64)mss << 32);
> +#else
> +       return -ENOTSUPP;
> +#endif /* CONFIG_SYN_COOKIES */
> +}
> +
> +static const struct bpf_func_proto bpf_tcp_gen_syncookie_proto = {
> +       .func           = bpf_tcp_gen_syncookie,
> +       .gpl_only       = true, /* __cookie_v*_init_sequence() is GPL */
> +       .pkt_access     = true,
> +       .ret_type       = RET_INTEGER,
> +       .arg1_type      = ARG_PTR_TO_SOCK_COMMON,
> +       .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)
> @@ -6135,6 +6193,8 @@ tc_cls_act_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>                 return &bpf_tcp_check_syncookie_proto;
>         case BPF_FUNC_skb_ecn_set_ce:
>                 return &bpf_skb_ecn_set_ce_proto;
> +       case BPF_FUNC_tcp_gen_syncookie:
> +               return &bpf_tcp_gen_syncookie_proto;
>  #endif
>         default:
>                 return bpf_base_func_proto(func_id);
> @@ -6174,6 +6234,8 @@ xdp_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>                 return &bpf_xdp_skc_lookup_tcp_proto;
>         case BPF_FUNC_tcp_check_syncookie:
>                 return &bpf_tcp_check_syncookie_proto;
> +       case BPF_FUNC_tcp_gen_syncookie:
> +               return &bpf_tcp_gen_syncookie_proto;
>  #endif
>         default:
>                 return bpf_base_func_proto(func_id);
> --
> 2.22.0.510.g264f2c817a-goog
>


-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com

Re: [bpf-next RFC 3/6] bpf: add bpf_tcp_gen_syncookie helper

[Lorenz Bauer]

On Tue, 16 Jul 2019 at 08:59, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > +             return -EINVAL;
> > +
> > +     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->syn || th->ack || th->fin || th->rst)
> > +             return -EINVAL;
> > +
> > +     switch (sk->sk_family) {
>
> This is strange, because a dual stack listener will have sk->sk_family set to AF_INET6.
>
> What really matters here is if the packet is IPv4 or IPv6.
>
> So you need to look at iph->version instead.
>
> Then look if the socket family allows this packet to be processed
> (For example AF_INET6 sockets might prevent IPv4 packets, see sk->sk_ipv6only )

Does this apply for (the existing) tcp_check_syn_cookie as well?

-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com

Re: [bpf-next RFC 2/6] tcp: add skb-less helpers to retrieve SYN cookie

[Petar Penkov]

On Tue, Jul 16, 2019 at 4:35 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> On Tue, 16 Jul 2019 at 01:27, Petar Penkov <ppenkov.kernel@gmail.com> wrote:
> >
> > From: Petar Penkov <ppenkov@google.com>
> >
> > This patch allows generation of a SYN cookie before an SKB has been
> > allocated, as is the case at XDP.
> >
> > Signed-off-by: Petar Penkov <ppenkov@google.com>
> > ---
> >  include/net/tcp.h    | 11 ++++++
> >  net/ipv4/tcp_input.c | 79 ++++++++++++++++++++++++++++++++++++++++++++
> >  net/ipv4/tcp_ipv4.c  |  8 +++++
> >  net/ipv6/tcp_ipv6.c  |  8 +++++
> >  4 files changed, 106 insertions(+)
> >
> > diff --git a/include/net/tcp.h b/include/net/tcp.h
> > index cca3c59b98bf..a128e22c0d5d 100644
> > --- a/include/net/tcp.h
> > +++ b/include/net/tcp.h
> > @@ -414,6 +414,17 @@ void tcp_parse_options(const struct net *net, const struct sk_buff *skb,
> >                        int estab, struct tcp_fastopen_cookie *foc);
> >  const u8 *tcp_parse_md5sig_option(const struct tcphdr *th);
> >
> > +/*
> > + *     BPF SKB-less helpers
> > + */
> > +u16 tcp_v4_get_syncookie(struct sock *sk, struct iphdr *iph,
> > +                        struct tcphdr *tch, u32 *cookie);
> > +u16 tcp_v6_get_syncookie(struct sock *sk, struct ipv6hdr *iph,
> > +                        struct tcphdr *tch, u32 *cookie);
> > +u16 tcp_get_syncookie(struct request_sock_ops *rsk_ops,
> > +                     const struct tcp_request_sock_ops *af_ops,
> > +                     struct sock *sk, void *iph, struct tcphdr *tch,
> > +                     u32 *cookie);
> >  /*
> >   *     TCP v4 functions exported for the inet6 API
> >   */
> > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > index 8892df6de1d4..1406d7e0953c 100644
> > --- a/net/ipv4/tcp_input.c
> > +++ b/net/ipv4/tcp_input.c
> > @@ -3782,6 +3782,52 @@ static void smc_parse_options(const struct tcphdr *th,
> >  #endif
> >  }
> >
> > +/* Try to parse the MSS option from the TCP header. Return 0 on failure, clamped
> > + * value on success.
> > + *
> > + * Invoked for BPF SYN cookie generation, so th should be a SYN.
> > + */
> > +static u16 tcp_parse_mss_option(const struct net *net, const struct tcphdr *th,
> > +                               u16 user_mss)
>
> net seems unused?

Ah, good catch! Will remove this in the v2.

>
> > +{
> > +       const unsigned char *ptr = (const unsigned char *)(th + 1);
> > +       int length = (th->doff * 4) - sizeof(struct tcphdr);
> > +       u16 mss = 0;
> > +
> > +       while (length > 0) {
> > +               int opcode = *ptr++;
> > +               int opsize;
> > +
> > +               switch (opcode) {
> > +               case TCPOPT_EOL:
> > +                       return mss;
> > +               case TCPOPT_NOP:        /* Ref: RFC 793 section 3.1 */
> > +                       length--;
> > +                       continue;
> > +               default:
> > +                       if (length < 2)
> > +                               return mss;
> > +                       opsize = *ptr++;
> > +                       if (opsize < 2) /* "silly options" */
> > +                               return mss;
> > +                       if (opsize > length)
> > +                               return mss;     /* fail on partial options */
> > +                       if (opcode == TCPOPT_MSS && opsize == TCPOLEN_MSS) {
> > +                               u16 in_mss = get_unaligned_be16(ptr);
> > +
> > +                               if (in_mss) {
> > +                                       if (user_mss && user_mss < in_mss)
> > +                                               in_mss = user_mss;
> > +                                       mss = in_mss;
> > +                               }
> > +                       }
> > +                       ptr += opsize - 2;
> > +                       length -= opsize;
> > +               }
> > +       }
> > +       return mss;
> > +}
> > +
> >  /* Look for tcp options. Normally only called on SYN and SYNACK packets.
> >   * But, this can also be called on packets in the established flow when
> >   * the fast version below fails.
> > @@ -6464,6 +6510,39 @@ static void tcp_reqsk_record_syn(const struct sock *sk,
> >         }
> >  }
> >
> > +u16 tcp_get_syncookie(struct request_sock_ops *rsk_ops,
> > +                     const struct tcp_request_sock_ops *af_ops,
> > +                     struct sock *sk, void *iph, struct tcphdr *th,
> > +                     u32 *cookie)
> > +{
> > +       u16 mss = 0;
> > +#ifdef CONFIG_SYN_COOKIES
> > +       bool is_v4 = rsk_ops->family == AF_INET;
> > +       struct tcp_sock *tp = tcp_sk(sk);
> > +
> > +       if (sock_net(sk)->ipv4.sysctl_tcp_syncookies != 2 &&
> > +           !inet_csk_reqsk_queue_is_full(sk))
> > +               return 0;
> > +
> > +       if (!tcp_syn_flood_action(sk, rsk_ops->slab_name))
> > +               return 0;
> > +
> > +       if (sk_acceptq_is_full(sk)) {
> > +               NET_INC_STATS(sock_net(sk), LINUX_MIB_LISTENOVERFLOWS);
> > +               return 0;
> > +       }
> > +
> > +       mss = tcp_parse_mss_option(sock_net(sk), th, tp->rx_opt.user_mss);
> > +       if (!mss)
> > +               mss = af_ops->mss_clamp;
> > +
> > +       tcp_synq_overflow(sk);
> > +       *cookie = is_v4 ? __cookie_v4_init_sequence(iph, th, &mss)
> > +                       : __cookie_v6_init_sequence(iph, th, &mss);
> > +#endif
> > +       return mss;
> > +}
> > +
> >  int tcp_conn_request(struct request_sock_ops *rsk_ops,
> >                      const struct tcp_request_sock_ops *af_ops,
> >                      struct sock *sk, struct sk_buff *skb)
> > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> > index d57641cb3477..0e06e59784bd 100644
> > --- a/net/ipv4/tcp_ipv4.c
> > +++ b/net/ipv4/tcp_ipv4.c
> > @@ -1515,6 +1515,14 @@ static struct sock *tcp_v4_cookie_check(struct sock *sk, struct sk_buff *skb)
> >         return sk;
> >  }
> >
> > +u16 tcp_v4_get_syncookie(struct sock *sk, struct iphdr *iph,
> > +                        struct tcphdr *tch, u32 *cookie)
> > +{
> > +       return tcp_get_syncookie(&tcp_request_sock_ops,
> > +                                &tcp_request_sock_ipv4_ops, sk, iph, tch,
> > +                                cookie);
> > +}
> > +
> >  /* The socket must have it's spinlock held when we get
> >   * here, unless it is a TCP_LISTEN socket.
> >   *
> > diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> > index d56a9019a0fe..ce46cdba54bc 100644
> > --- a/net/ipv6/tcp_ipv6.c
> > +++ b/net/ipv6/tcp_ipv6.c
> > @@ -1058,6 +1058,14 @@ static struct sock *tcp_v6_cookie_check(struct sock *sk, struct sk_buff *skb)
> >         return sk;
> >  }
> >
> > +u16 tcp_v6_get_syncookie(struct sock *sk, struct ipv6hdr *iph,
> > +                        struct tcphdr *tch, u32 *cookie)
> > +{
> > +       return tcp_get_syncookie(&tcp6_request_sock_ops,
> > +                                &tcp_request_sock_ipv6_ops, sk, iph, tch,
> > +                                cookie);
> > +}
> > +
> >  static int tcp_v6_conn_request(struct sock *sk, struct sk_buff *skb)
> >  {
> >         if (skb->protocol == htons(ETH_P_IP))
> > --
> > 2.22.0.510.g264f2c817a-goog
> >
>
>
> --
> Lorenz Bauer  |  Systems Engineer
> 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
>
> www.cloudflare.com

Re: [bpf-next RFC 3/6] bpf: add bpf_tcp_gen_syncookie helper

[Petar Penkov]

Thank you for the reviews!

On Tue, Jul 16, 2019 at 4:56 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> On Tue, 16 Jul 2019 at 08:59, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > > +             return -EINVAL;
> > > +
> > > +     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->syn || th->ack || th->fin || th->rst)
> > > +             return -EINVAL;
> > > +
> > > +     switch (sk->sk_family) {
> >
> > This is strange, because a dual stack listener will have sk->sk_family set to AF_INET6.
> >
> > What really matters here is if the packet is IPv4 or IPv6.
> >
> > So you need to look at iph->version instead.
> >
> > Then look if the socket family allows this packet to be processed
> > (For example AF_INET6 sockets might prevent IPv4 packets, see sk->sk_ipv6only )

This makes a lot of sense, thanks Eric, will update!
>
> Does this apply for (the existing) tcp_check_syn_cookie as well?

I think we will probably have to update the check there, too.
>
> --
> Lorenz Bauer  |  Systems Engineer
> 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
>
> www.cloudflare.com

Re: [bpf-next RFC 3/6] bpf: add bpf_tcp_gen_syncookie helper

[Alexei Starovoitov]

On Tue, Jul 16, 2019 at 09:59:26AM +0200, Eric Dumazet wrote:
> 
> 
> On 7/16/19 2:26 AM, Petar Penkov wrote:
> > From: Petar Penkov <ppenkov@google.com>
> > 
> > This helper function allows BPF programs to try to generate SYN
> > cookies, given a reference to a listener socket. The function works
> > from XDP and with an skb context since bpf_skc_lookup_tcp can lookup a
> > socket in both cases.
> > 
> ...
> >  
> > +BPF_CALL_5(bpf_tcp_gen_syncookie, struct sock *, sk, void *, iph, u32, iph_len,
> > +	   struct tcphdr *, th, u32, th_len)
> > +{
> > +#ifdef CONFIG_SYN_COOKIES
> > +	u32 cookie;
> > +	u16 mss;
> > +
> > +	if (unlikely(th_len < sizeof(*th)))
> 
> 
> You probably need to check that th_len == th->doff * 4

+1
that is surely necessary for safety.

Considering the limitation of 5 args the api choice is good.
struct bpf_syncookie approach doesn't look natural to me.
And I couldn't come up with any better way to represent this helper.
So let's go with 
return htonl(cookie) | ((u64)mss << 32);
My only question is why htonl ?

Independently of that...
Since we've been hitting this 5 args limit too much,
we need to start thinking how to extend BPF ISA to pass
args 6,7,8 on stack.

Re: [bpf-next RFC 3/6] bpf: add bpf_tcp_gen_syncookie helper

[Petar Penkov]

Thank you for your feedback!

On Tue, Jul 16, 2019 at 7:26 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Jul 16, 2019 at 09:59:26AM +0200, Eric Dumazet wrote:
> >
> >
> > On 7/16/19 2:26 AM, Petar Penkov wrote:
> > > From: Petar Penkov <ppenkov@google.com>
> > >
> > > This helper function allows BPF programs to try to generate SYN
> > > cookies, given a reference to a listener socket. The function works
> > > from XDP and with an skb context since bpf_skc_lookup_tcp can lookup a
> > > socket in both cases.
> > >
> > ...
> > >
> > > +BPF_CALL_5(bpf_tcp_gen_syncookie, struct sock *, sk, void *, iph, u32, iph_len,
> > > +      struct tcphdr *, th, u32, th_len)
> > > +{
> > > +#ifdef CONFIG_SYN_COOKIES
> > > +   u32 cookie;
> > > +   u16 mss;
> > > +
> > > +   if (unlikely(th_len < sizeof(*th)))
> >
> >
> > You probably need to check that th_len == th->doff * 4
>
> +1
> that is surely necessary for safety.

I'll make sure to include this check in the next version.

>
> Considering the limitation of 5 args the api choice is good.
> struct bpf_syncookie approach doesn't look natural to me.
> And I couldn't come up with any better way to represent this helper.
> So let's go with
> return htonl(cookie) | ((u64)mss << 32);
> My only question is why htonl ?

I did it to mirror bpf_tcp_check_syncookie which sees a network order
cookie. That said, it is probably better for the caller to call
bpf_htonl() as they see fit, rather than to do it in the helper. Will
update, thanks.
>
> Independently of that...
> Since we've been hitting this 5 args limit too much,
> we need to start thinking how to extend BPF ISA to pass
> args 6,7,8 on stack.
>

Agreed, having an additional argument would have been helpful for this function.