[PATCH net-next 0/5] tcp: Introduce a TFO key-pool for clean cookie-rotation

[PATCH net-next 0/5] tcp: Introduce a TFO key-pool for clean cookie-rotation

[Christoph Paasch]

Currently, TFO only allows a single TFO-secret. This means that whenever
the secret gets changed for key-rotation purposes, all the previously
issued TFO-cookies become invalid. This means that clients will fallback
to "regular" TCP, incurring a cost of one additional round-trip.

This patchset introduces a TFO key-pool that allows to more gracefully
change the key. The size of the pool is 2 (this could be changed in the
future through a sysctl if needed). When a client connects with an "old"
TFO cookie, the server will now accept the data in the SYN and at the
same time announce a new TFO-cookie to the client.

We have seen a significant reduction of LINUX_MIB_TCPFASTOPENPASSIVEFAIL
thanks to these patches. Invalid cookies are now solely observed when
clients behind a NAT are getting a new public IP.


Christoph Paasch (5):
  tcp: Create list of TFO-contexts
  tcp: TFO: search for correct cookie and accept data
  tcp: Print list of TFO-keys from proc
  tcp: Allow getsockopt of listener's keypool
  tcp: TFO - cleanup code duplication

 include/net/tcp.h          |   2 +
 include/uapi/linux/snmp.h  |   1 +
 net/ipv4/proc.c            |   1 +
 net/ipv4/sysctl_net_ipv4.c |  41 +++++++---
 net/ipv4/tcp.c             |  15 ++--
 net/ipv4/tcp_fastopen.c    | 192 +++++++++++++++++++++++++++++++++++----------
 6 files changed, 193 insertions(+), 59 deletions(-)

-- 
2.16.2

[PATCH net-next 1/5] tcp: Create list of TFO-contexts

[Christoph Paasch]

Instead of having a single TFO-context, we now have a list of
tcp_fastopen_context, bounded by TCP_FASTOPEN_CTXT_LEN (set to 2).

This enables us to do a rolling TFO-key update that allows the server to
accept old cookies and at the same time announce new ones to the client
(see follow-up patch).

Signed-off-by: Christoph Paasch <cpaasch@apple.com>
---
 include/net/tcp.h       |  2 ++
 net/ipv4/tcp_fastopen.c | 52 +++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index e0a65c067662..e629ea2e6c9d 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1622,9 +1622,11 @@ bool tcp_fastopen_cookie_check(struct sock *sk, u16 *mss,
 			     struct tcp_fastopen_cookie *cookie);
 bool tcp_fastopen_defer_connect(struct sock *sk, int *err);
 #define TCP_FASTOPEN_KEY_LENGTH 16
+#define TCP_FASTOPEN_CTXT_LEN 2
 
 /* Fastopen key context */
 struct tcp_fastopen_context {
+	struct tcp_fastopen_context __rcu *next;
 	struct crypto_cipher	*tfm;
 	__u8			key[TCP_FASTOPEN_KEY_LENGTH];
 	struct rcu_head		rcu;
diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
index 018a48477355..c52d5b8eabf0 100644
--- a/net/ipv4/tcp_fastopen.c
+++ b/net/ipv4/tcp_fastopen.c
@@ -37,8 +37,14 @@ static void tcp_fastopen_ctx_free(struct rcu_head *head)
 {
 	struct tcp_fastopen_context *ctx =
 	    container_of(head, struct tcp_fastopen_context, rcu);
-	crypto_free_cipher(ctx->tfm);
-	kfree(ctx);
+
+	while (ctx) {
+		struct tcp_fastopen_context *prev = ctx;
+		/* We own ctx, thus no need to hold the Fastopen-lock */
+		ctx = rcu_dereference_protected(ctx->next, 1);
+		crypto_free_cipher(prev->tfm);
+		kfree(prev);
+	}
 }
 
 void tcp_fastopen_destroy_cipher(struct sock *sk)
@@ -66,6 +72,35 @@ void tcp_fastopen_ctx_destroy(struct net *net)
 		call_rcu(&ctxt->rcu, tcp_fastopen_ctx_free);
 }
 
+static struct tcp_fastopen_context *
+tcp_fastopen_cut_keypool(struct tcp_fastopen_context *ctx,
+			 spinlock_t *lock)
+{
+	int cnt = 0;
+
+	while (ctx) {
+		/* We iterate the list to see if we have more than
+		 * TCP_FASTOPEN_CTXT_LEN contexts. If we do, we remove the rest
+		 * of the list and free it later
+		 */
+
+		cnt++;
+		if (cnt >= TCP_FASTOPEN_CTXT_LEN) {
+			/* It's the last one, return the rest so it gets freed */
+			struct tcp_fastopen_context *prev = ctx;
+
+			ctx = rcu_dereference_protected(ctx->next,
+							lockdep_is_held(lock));
+			rcu_assign_pointer(prev->next, NULL);
+			break;
+		}
+		ctx = rcu_dereference_protected(ctx->next,
+						lockdep_is_held(lock));
+	}
+
+	return ctx;
+}
+
 int tcp_fastopen_reset_cipher(struct net *net, struct sock *sk,
 			      void *key, unsigned int len)
 {
@@ -96,13 +131,22 @@ error:		kfree(ctx);
 	spin_lock(&net->ipv4.tcp_fastopen_ctx_lock);
 	if (sk) {
 		q = &inet_csk(sk)->icsk_accept_queue.fastopenq;
+		rcu_assign_pointer(ctx->next, q->ctx);
+		rcu_assign_pointer(q->ctx, ctx);
+
 		octx = rcu_dereference_protected(q->ctx,
 			lockdep_is_held(&net->ipv4.tcp_fastopen_ctx_lock));
-		rcu_assign_pointer(q->ctx, ctx);
+
+		octx = tcp_fastopen_cut_keypool(octx, &net->ipv4.tcp_fastopen_ctx_lock);
 	} else {
+		rcu_assign_pointer(ctx->next, net->ipv4.tcp_fastopen_ctx);
+		rcu_assign_pointer(net->ipv4.tcp_fastopen_ctx, ctx);
+
 		octx = rcu_dereference_protected(net->ipv4.tcp_fastopen_ctx,
 			lockdep_is_held(&net->ipv4.tcp_fastopen_ctx_lock));
-		rcu_assign_pointer(net->ipv4.tcp_fastopen_ctx, ctx);
+
+		octx = tcp_fastopen_cut_keypool(octx,
+						&net->ipv4.tcp_fastopen_ctx_lock);
 	}
 	spin_unlock(&net->ipv4.tcp_fastopen_ctx_lock);
 
-- 
2.16.2

[PATCH net-next 2/5] tcp: TFO: search for correct cookie and accept data

[Christoph Paasch]

This change allows to search for the right cookie and accepts old ones
(announcing a new one if it has changed).

__tcp_fastopen_cookie_gen_with_ctx() allows to generate a cookie based
on a given TFO-context. A later patch will cleanup the duplicate code.

Signed-off-by: Christoph Paasch <cpaasch@apple.com>
---
 include/uapi/linux/snmp.h |   1 +
 net/ipv4/proc.c           |   1 +
 net/ipv4/tcp_fastopen.c   | 105 +++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 101 insertions(+), 6 deletions(-)

diff --git a/include/uapi/linux/snmp.h b/include/uapi/linux/snmp.h
index 86dc24a96c90..74904e9d1b72 100644
--- a/include/uapi/linux/snmp.h
+++ b/include/uapi/linux/snmp.h
@@ -283,6 +283,7 @@ enum
 	LINUX_MIB_TCPACKCOMPRESSED,		/* TCPAckCompressed */
 	LINUX_MIB_TCPZEROWINDOWDROP,		/* TCPZeroWindowDrop */
 	LINUX_MIB_TCPRCVQDROP,			/* TCPRcvQDrop */
+	LINUX_MIB_TCPFASTOPENPASSIVEALTKEY,	/* TCPFastOpenPassiveAltKey */
 	__LINUX_MIB_MAX
 };
 
diff --git a/net/ipv4/proc.c b/net/ipv4/proc.c
index c3610b37bb4c..58daef27a560 100644
--- a/net/ipv4/proc.c
+++ b/net/ipv4/proc.c
@@ -291,6 +291,7 @@ static const struct snmp_mib snmp4_net_list[] = {
 	SNMP_MIB_ITEM("TCPAckCompressed", LINUX_MIB_TCPACKCOMPRESSED),
 	SNMP_MIB_ITEM("TCPZeroWindowDrop", LINUX_MIB_TCPZEROWINDOWDROP),
 	SNMP_MIB_ITEM("TCPRcvQDrop", LINUX_MIB_TCPRCVQDROP),
+	SNMP_MIB_ITEM("TCPFastOpenPassiveAltKey", LINUX_MIB_TCPFASTOPENPASSIVEALTKEY),
 	SNMP_MIB_SENTINEL
 };
 
diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
index c52d5b8eabf0..e856262ef4c2 100644
--- a/net/ipv4/tcp_fastopen.c
+++ b/net/ipv4/tcp_fastopen.c
@@ -176,6 +176,41 @@ static bool __tcp_fastopen_cookie_gen(struct sock *sk, const void *path,
 	return ok;
 }
 
+static void __tcp_fastopen_cookie_gen_with_ctx(struct request_sock *req,
+					       struct sk_buff *syn,
+					       struct tcp_fastopen_cookie *foc,
+					       struct tcp_fastopen_context *ctx)
+{
+	if (req->rsk_ops->family == AF_INET) {
+		const struct iphdr *iph = ip_hdr(syn);
+		__be32 path[4] = { iph->saddr, iph->daddr, 0, 0 };
+
+		crypto_cipher_encrypt_one(ctx->tfm, foc->val, (void *)path);
+		foc->len = TCP_FASTOPEN_COOKIE_SIZE;
+		return;
+	}
+
+#if IS_ENABLED(CONFIG_IPV6)
+	if (req->rsk_ops->family == AF_INET6) {
+		const struct ipv6hdr *ip6h = ipv6_hdr(syn);
+		struct tcp_fastopen_cookie tmp;
+		struct in6_addr *buf;
+		int i;
+
+		crypto_cipher_encrypt_one(ctx->tfm, tmp.val, (void *)&ip6h->saddr);
+
+		buf = &tmp.addr;
+		for (i = 0; i < 4; i++)
+			buf->s6_addr32[i] ^= ip6h->daddr.s6_addr32[i];
+
+		crypto_cipher_encrypt_one(ctx->tfm, foc->val, (void *)buf);
+		foc->len = TCP_FASTOPEN_COOKIE_SIZE;
+
+		return;
+	}
+#endif
+}
+
 /* Generate the fastopen cookie by doing aes128 encryption on both
  * the source and destination addresses. Pad 0s for IPv4 or IPv4-mapped-IPv6
  * addresses. For the longer IPv6 addresses use CBC-MAC.
@@ -256,6 +291,55 @@ void tcp_fastopen_add_skb(struct sock *sk, struct sk_buff *skb)
 		tcp_fin(sk);
 }
 
+static bool tcp_fastopen_cookie_gen_search(struct sock *sk,
+					   struct request_sock *req,
+					   struct sk_buff *syn,
+					   struct tcp_fastopen_cookie *valid_foc,
+					   struct tcp_fastopen_cookie *orig)
+{
+	struct tcp_fastopen_cookie search_foc = { .len = -1 };
+	struct tcp_fastopen_cookie *foc = &search_foc;
+	struct tcp_fastopen_context *ctx;
+	int copied = 0;
+
+	rcu_read_lock();
+
+	ctx = rcu_dereference(inet_csk(sk)->icsk_accept_queue.fastopenq.ctx);
+	if (!ctx)
+		ctx = rcu_dereference(sock_net(sk)->ipv4.tcp_fastopen_ctx);
+
+	while (ctx) {
+		__tcp_fastopen_cookie_gen_with_ctx(req, syn, foc, ctx);
+
+		if (foc->len == orig->len &&
+		    !memcmp(foc->val, orig->val, foc->len)) {
+			rcu_read_unlock();
+
+			if (copied) {
+				struct net *net = read_pnet(&inet_rsk(req)->ireq_net);
+
+				NET_INC_STATS(net,
+					      LINUX_MIB_TCPFASTOPENPASSIVEALTKEY);
+			}
+			return true;
+		}
+
+		/* We need to check older possible cookies, thus set valid_foc
+		 * so that the latest one will be announced to the peer.
+		 */
+		if (!copied) {
+			memcpy(valid_foc, foc, sizeof(*foc));
+			copied = 1;
+		}
+
+		ctx = rcu_dereference(ctx->next);
+	}
+
+	rcu_read_unlock();
+
+	return false;
+}
+
 static struct sock *tcp_fastopen_create_child(struct sock *sk,
 					      struct sk_buff *skb,
 					      struct request_sock *req)
@@ -390,11 +474,11 @@ struct sock *tcp_try_fastopen(struct sock *sk, struct sk_buff *skb,
 	    tcp_fastopen_no_cookie(sk, dst, TFO_SERVER_COOKIE_NOT_REQD))
 		goto fastopen;
 
-	if (foc->len >= 0 &&  /* Client presents or requests a cookie */
-	    tcp_fastopen_cookie_gen(sk, req, skb, &valid_foc) &&
-	    foc->len == TCP_FASTOPEN_COOKIE_SIZE &&
-	    foc->len == valid_foc.len &&
-	    !memcmp(foc->val, valid_foc.val, foc->len)) {
+	if (foc->len == 0) {
+		/* Client requests a cookie. */
+		tcp_fastopen_cookie_gen(sk, req, skb, &valid_foc);
+	} else if (foc->len > 0 &&
+		   tcp_fastopen_cookie_gen_search(sk, req, skb, &valid_foc, foc)) {
 		/* Cookie is valid. Create a (full) child socket to accept
 		 * the data in SYN before returning a SYN-ACK to ack the
 		 * data. If we fail to create the socket, fall back and
@@ -406,7 +490,16 @@ struct sock *tcp_try_fastopen(struct sock *sk, struct sk_buff *skb,
 fastopen:
 		child = tcp_fastopen_create_child(sk, skb, req);
 		if (child) {
-			foc->len = -1;
+			if (valid_foc.len != -1) {
+				/* Client used an old cookie, we announce the
+				 * latests one to the client.
+				 */
+				valid_foc.exp = foc->exp;
+				*foc = valid_foc;
+			} else {
+				foc->len = -1;
+			}
+
 			NET_INC_STATS(sock_net(sk),
 				      LINUX_MIB_TCPFASTOPENPASSIVE);
 			return child;
-- 
2.16.2

[PATCH net-next 3/5] tcp: Print list of TFO-keys from proc

[Christoph Paasch]

Print the list of the TFO-keys with a comma separated. For setting the
keys, we still only allow a single one to be set.

Signed-off-by: Christoph Paasch <cpaasch@apple.com>
---
 net/ipv4/sysctl_net_ipv4.c | 41 ++++++++++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 11 deletions(-)

diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index ba0fc4b18465..f0806bab5562 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c
@@ -282,11 +282,15 @@ static int proc_tcp_fastopen_key(struct ctl_table *table, int write,
 {
 	struct net *net = container_of(table->data, struct net,
 	    ipv4.sysctl_tcp_fastopen);
-	struct ctl_table tbl = { .maxlen = (TCP_FASTOPEN_KEY_LENGTH * 2 + 10) };
+	/* maxlen to print the list of keys in hex (*2), with a comma
+	 * in between (+ TCP_FASTOPEN_CTXT_LEN)
+	 */
+	struct ctl_table tbl = { .maxlen = (TCP_FASTOPEN_KEY_LENGTH * 2 * TCP_FASTOPEN_CTXT_LEN +
+					    TCP_FASTOPEN_CTXT_LEN + 10) };
 	struct tcp_fastopen_context *ctxt;
-	u32  user_key[4]; /* 16 bytes, matching TCP_FASTOPEN_KEY_LENGTH */
-	__le32 key[4];
-	int ret, i;
+	u32  user_key[TCP_FASTOPEN_CTXT_LEN * 4];
+	__le32 key[TCP_FASTOPEN_CTXT_LEN * 4];
+	int ret, i = 0, off = 0;
 
 	tbl.data = kmalloc(tbl.maxlen, GFP_KERNEL);
 	if (!tbl.data)
@@ -294,17 +298,28 @@ static int proc_tcp_fastopen_key(struct ctl_table *table, int write,
 
 	rcu_read_lock();
 	ctxt = rcu_dereference(net->ipv4.tcp_fastopen_ctx);
-	if (ctxt)
-		memcpy(key, ctxt->key, TCP_FASTOPEN_KEY_LENGTH);
-	else
-		memset(key, 0, sizeof(key));
+	while (ctxt) {
+		memcpy(&key[i], ctxt->key, TCP_FASTOPEN_KEY_LENGTH);
+		i += 4;
+		ctxt = rcu_dereference(ctxt->next);
+	}
 	rcu_read_unlock();
 
+	memset(&key[i], 0, sizeof(key) - i * sizeof(u32));
+
 	for (i = 0; i < ARRAY_SIZE(key); i++)
 		user_key[i] = le32_to_cpu(key[i]);
 
-	snprintf(tbl.data, tbl.maxlen, "%08x-%08x-%08x-%08x",
-		user_key[0], user_key[1], user_key[2], user_key[3]);
+	for (i = 0; i < TCP_FASTOPEN_CTXT_LEN; i++) {
+		off += snprintf(tbl.data + off, tbl.maxlen - off,
+				"%08x-%08x-%08x-%08x",
+				user_key[i * 4],
+				user_key[i * 4 + 1],
+				user_key[i * 4 + 2],
+				user_key[i * 4 + 3]);
+		if (i + 1 < TCP_FASTOPEN_CTXT_LEN)
+			off += snprintf(tbl.data + off, tbl.maxlen - off, ",");
+	}
 	ret = proc_dostring(&tbl, write, buffer, lenp, ppos);
 
 	if (write && ret == 0) {
@@ -923,7 +938,11 @@ static struct ctl_table ipv4_net_table[] = {
 		.procname	= "tcp_fastopen_key",
 		.mode		= 0600,
 		.data		= &init_net.ipv4.sysctl_tcp_fastopen,
-		.maxlen		= ((TCP_FASTOPEN_KEY_LENGTH * 2) + 10),
+		/* maxlen to print the list of keys in hex (*2), with a comma
+		 * in between (+ TCP_FASTOPEN_CTXT_LEN)
+		 */
+		.maxlen		= ((TCP_FASTOPEN_KEY_LENGTH * 2 * TCP_FASTOPEN_CTXT_LEN)
+				    + TCP_FASTOPEN_CTXT_LEN + 10),
 		.proc_handler	= proc_tcp_fastopen_key,
 	},
 	{
-- 
2.16.2

[PATCH net-next 4/5] tcp: Allow getsockopt of listener's keypool

[Christoph Paasch]

Allow to get the full list of the listener's keypool through a
getsockopt.

Signed-off-by: Christoph Paasch <cpaasch@apple.com>
---
 net/ipv4/tcp.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 27e2f6837062..cdb317392138 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -3420,21 +3420,24 @@ static int do_tcp_getsockopt(struct sock *sk, int level,
 		return 0;
 
 	case TCP_FASTOPEN_KEY: {
-		__u8 key[TCP_FASTOPEN_KEY_LENGTH];
+		__u8 key[TCP_FASTOPEN_KEY_LENGTH * TCP_FASTOPEN_CTXT_LEN];
 		struct tcp_fastopen_context *ctx;
+		unsigned int key_len = 0;
 
 		if (get_user(len, optlen))
 			return -EFAULT;
 
 		rcu_read_lock();
 		ctx = rcu_dereference(icsk->icsk_accept_queue.fastopenq.ctx);
-		if (ctx)
-			memcpy(key, ctx->key, sizeof(key));
-		else
-			len = 0;
+		while (ctx) {
+			memcpy(&key[key_len], ctx->key, TCP_FASTOPEN_KEY_LENGTH);
+
+			key_len += TCP_FASTOPEN_KEY_LENGTH;
+			ctx = rcu_dereference(ctx->next);
+		}
 		rcu_read_unlock();
 
-		len = min_t(unsigned int, len, sizeof(key));
+		len = min_t(unsigned int, len, key_len);
 		if (put_user(len, optlen))
 			return -EFAULT;
 		if (copy_to_user(optval, key, len))
-- 
2.16.2

[PATCH net-next 5/5] tcp: TFO - cleanup code duplication

[Christoph Paasch]

We can actually easily reuse __tcp_fastopen_cookie_gen_with_ctx to
generate the cookie based on a give TFO-context.

This cleans up some of the code.

Signed-off-by: Christoph Paasch <cpaasch@apple.com>
---
 net/ipv4/tcp_fastopen.c | 51 +++++++++++--------------------------------------
 1 file changed, 11 insertions(+), 40 deletions(-)

diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
index e856262ef4c2..81e8b3ae9ecd 100644
--- a/net/ipv4/tcp_fastopen.c
+++ b/net/ipv4/tcp_fastopen.c
@@ -155,27 +155,6 @@ error:		kfree(ctx);
 	return err;
 }
 
-static bool __tcp_fastopen_cookie_gen(struct sock *sk, const void *path,
-				      struct tcp_fastopen_cookie *foc)
-{
-	struct tcp_fastopen_context *ctx;
-	bool ok = false;
-
-	rcu_read_lock();
-
-	ctx = rcu_dereference(inet_csk(sk)->icsk_accept_queue.fastopenq.ctx);
-	if (!ctx)
-		ctx = rcu_dereference(sock_net(sk)->ipv4.tcp_fastopen_ctx);
-
-	if (ctx) {
-		crypto_cipher_encrypt_one(ctx->tfm, foc->val, path);
-		foc->len = TCP_FASTOPEN_COOKIE_SIZE;
-		ok = true;
-	}
-	rcu_read_unlock();
-	return ok;
-}
-
 static void __tcp_fastopen_cookie_gen_with_ctx(struct request_sock *req,
 					       struct sk_buff *syn,
 					       struct tcp_fastopen_cookie *foc,
@@ -222,29 +201,21 @@ static bool tcp_fastopen_cookie_gen(struct sock *sk,
 				    struct sk_buff *syn,
 				    struct tcp_fastopen_cookie *foc)
 {
-	if (req->rsk_ops->family == AF_INET) {
-		const struct iphdr *iph = ip_hdr(syn);
-
-		__be32 path[4] = { iph->saddr, iph->daddr, 0, 0 };
-		return __tcp_fastopen_cookie_gen(sk, path, foc);
-	}
+	struct tcp_fastopen_context *ctx;
+	bool ok = false;
 
-#if IS_ENABLED(CONFIG_IPV6)
-	if (req->rsk_ops->family == AF_INET6) {
-		const struct ipv6hdr *ip6h = ipv6_hdr(syn);
-		struct tcp_fastopen_cookie tmp;
+	rcu_read_lock();
 
-		if (__tcp_fastopen_cookie_gen(sk, &ip6h->saddr, &tmp)) {
-			struct in6_addr *buf = &tmp.addr;
-			int i;
+	ctx = rcu_dereference(inet_csk(sk)->icsk_accept_queue.fastopenq.ctx);
+	if (!ctx)
+		ctx = rcu_dereference(sock_net(sk)->ipv4.tcp_fastopen_ctx);
 
-			for (i = 0; i < 4; i++)
-				buf->s6_addr32[i] ^= ip6h->daddr.s6_addr32[i];
-			return __tcp_fastopen_cookie_gen(sk, buf, foc);
-		}
+	if (ctx) {
+		__tcp_fastopen_cookie_gen_with_ctx(req, syn, foc, ctx);
+		ok = true;
 	}
-#endif
-	return false;
+	rcu_read_unlock();
+	return ok;
 }
 
 
-- 
2.16.2

Re: [PATCH net-next 0/5] tcp: Introduce a TFO key-pool for clean cookie-rotation

[David Miller]

From: Christoph Paasch <cpaasch@apple.com>
Date: Fri, 14 Dec 2018 14:40:02 -0800

> Currently, TFO only allows a single TFO-secret. This means that whenever
> the secret gets changed for key-rotation purposes, all the previously
> issued TFO-cookies become invalid. This means that clients will fallback
> to "regular" TCP, incurring a cost of one additional round-trip.
> 
> This patchset introduces a TFO key-pool that allows to more gracefully
> change the key. The size of the pool is 2 (this could be changed in the
> future through a sysctl if needed). When a client connects with an "old"
> TFO cookie, the server will now accept the data in the SYN and at the
> same time announce a new TFO-cookie to the client.
> 
> We have seen a significant reduction of LINUX_MIB_TCPFASTOPENPASSIVEFAIL
> thanks to these patches. Invalid cookies are now solely observed when
> clients behind a NAT are getting a new public IP.

Yuchung and Eric, please review.

Re: [PATCH net-next 0/5] tcp: Introduce a TFO key-pool for clean cookie-rotation

[Eric Dumazet]

On 12/16/2018 12:19 PM, David Miller wrote:
> From: Christoph Paasch <cpaasch@apple.com>
> Date: Fri, 14 Dec 2018 14:40:02 -0800
> 
>> Currently, TFO only allows a single TFO-secret. This means that whenever
>> the secret gets changed for key-rotation purposes, all the previously
>> issued TFO-cookies become invalid. This means that clients will fallback
>> to "regular" TCP, incurring a cost of one additional round-trip.
>>
>> This patchset introduces a TFO key-pool that allows to more gracefully
>> change the key. The size of the pool is 2 (this could be changed in the
>> future through a sysctl if needed). When a client connects with an "old"
>> TFO cookie, the server will now accept the data in the SYN and at the
>> same time announce a new TFO-cookie to the client.
>>
>> We have seen a significant reduction of LINUX_MIB_TCPFASTOPENPASSIVEFAIL
>> thanks to these patches. Invalid cookies are now solely observed when
>> clients behind a NAT are getting a new public IP.
> 
> Yuchung and Eric, please review.
> 

Thanks David, I will do now.

Re: [PATCH net-next 2/5] tcp: TFO: search for correct cookie and accept data

[Eric Dumazet]

On 12/14/2018 02:40 PM, Christoph Paasch wrote:
> This change allows to search for the right cookie and accepts old ones
> (announcing a new one if it has changed).
> 
> __tcp_fastopen_cookie_gen_with_ctx() allows to generate a cookie based
> on a given TFO-context. A later patch will cleanup the duplicate code.

How long is kept the secondary (old) context ?

I do not know exact crypto_cipher_encrypt_one() cost, but it looks like
your patch could double the cost of some TFO based attacks ?

Re: [PATCH net-next 1/5] tcp: Create list of TFO-contexts

[Eric Dumazet]

On 12/14/2018 02:40 PM, Christoph Paasch wrote:
> Instead of having a single TFO-context, we now have a list of
> tcp_fastopen_context, bounded by TCP_FASTOPEN_CTXT_LEN (set to 2).
> 
> This enables us to do a rolling TFO-key update that allows the server to
> accept old cookies and at the same time announce new ones to the client
> (see follow-up patch).
> 
> Signed-off-by: Christoph Paasch <cpaasch@apple.com>
> ---
>  include/net/tcp.h       |  2 ++
>  net/ipv4/tcp_fastopen.c | 52 +++++++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 50 insertions(+), 4 deletions(-)
> 
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index e0a65c067662..e629ea2e6c9d 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1622,9 +1622,11 @@ bool tcp_fastopen_cookie_check(struct sock *sk, u16 *mss,
>  			     struct tcp_fastopen_cookie *cookie);
>  bool tcp_fastopen_defer_connect(struct sock *sk, int *err);
>  #define TCP_FASTOPEN_KEY_LENGTH 16
> +#define TCP_FASTOPEN_CTXT_LEN 2
>  
>  /* Fastopen key context */
>  struct tcp_fastopen_context {
> +	struct tcp_fastopen_context __rcu *next;
>  	struct crypto_cipher	*tfm;
>  	__u8			key[TCP_FASTOPEN_KEY_LENGTH];
>  	struct rcu_head		rcu;
> diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
> index 018a48477355..c52d5b8eabf0 100644
> --- a/net/ipv4/tcp_fastopen.c
> +++ b/net/ipv4/tcp_fastopen.c
> @@ -37,8 +37,14 @@ static void tcp_fastopen_ctx_free(struct rcu_head *head)
>  {
>  	struct tcp_fastopen_context *ctx =
>  	    container_of(head, struct tcp_fastopen_context, rcu);
> -	crypto_free_cipher(ctx->tfm);
> -	kfree(ctx);
> +
> +	while (ctx) {
> +		struct tcp_fastopen_context *prev = ctx;
> +		/* We own ctx, thus no need to hold the Fastopen-lock */
> +		ctx = rcu_dereference_protected(ctx->next, 1);
> +		crypto_free_cipher(prev->tfm);
> +		kfree(prev);
> +	}
>

It seems this function does not need to be changed, since at most one context
should be freed per run ?

Re: [PATCH net-next 3/5] tcp: Print list of TFO-keys from proc

[Eric Dumazet]

On 12/14/2018 02:40 PM, Christoph Paasch wrote:
> Print the list of the TFO-keys with a comma separated. For setting the
> keys, we still only allow a single one to be set.
> 

I wonder if some applications expecting current format could break
after a formatting change.

Re: [PATCH net-next 5/5] tcp: TFO - cleanup code duplication

[Eric Dumazet]

On 12/14/2018 02:40 PM, Christoph Paasch wrote:
> We can actually easily reuse __tcp_fastopen_cookie_gen_with_ctx to
> generate the cookie based on a give TFO-context.
>

s/give/given/

Re: [PATCH net-next 1/5] tcp: Create list of TFO-contexts

[Christoph Paasch]

On 16/12/18 - 22:31:41, Eric Dumazet wrote:
> 
> 
> On 12/14/2018 02:40 PM, Christoph Paasch wrote:
> > Instead of having a single TFO-context, we now have a list of
> > tcp_fastopen_context, bounded by TCP_FASTOPEN_CTXT_LEN (set to 2).
> > 
> > This enables us to do a rolling TFO-key update that allows the server to
> > accept old cookies and at the same time announce new ones to the client
> > (see follow-up patch).
> > 
> > Signed-off-by: Christoph Paasch <cpaasch@apple.com>
> > ---
> >  include/net/tcp.h       |  2 ++
> >  net/ipv4/tcp_fastopen.c | 52 +++++++++++++++++++++++++++++++++++++++++++++----
> >  2 files changed, 50 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/net/tcp.h b/include/net/tcp.h
> > index e0a65c067662..e629ea2e6c9d 100644
> > --- a/include/net/tcp.h
> > +++ b/include/net/tcp.h
> > @@ -1622,9 +1622,11 @@ bool tcp_fastopen_cookie_check(struct sock *sk, u16 *mss,
> >  			     struct tcp_fastopen_cookie *cookie);
> >  bool tcp_fastopen_defer_connect(struct sock *sk, int *err);
> >  #define TCP_FASTOPEN_KEY_LENGTH 16
> > +#define TCP_FASTOPEN_CTXT_LEN 2
> >  
> >  /* Fastopen key context */
> >  struct tcp_fastopen_context {
> > +	struct tcp_fastopen_context __rcu *next;
> >  	struct crypto_cipher	*tfm;
> >  	__u8			key[TCP_FASTOPEN_KEY_LENGTH];
> >  	struct rcu_head		rcu;
> > diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
> > index 018a48477355..c52d5b8eabf0 100644
> > --- a/net/ipv4/tcp_fastopen.c
> > +++ b/net/ipv4/tcp_fastopen.c
> > @@ -37,8 +37,14 @@ static void tcp_fastopen_ctx_free(struct rcu_head *head)
> >  {
> >  	struct tcp_fastopen_context *ctx =
> >  	    container_of(head, struct tcp_fastopen_context, rcu);
> > -	crypto_free_cipher(ctx->tfm);
> > -	kfree(ctx);
> > +
> > +	while (ctx) {
> > +		struct tcp_fastopen_context *prev = ctx;
> > +		/* We own ctx, thus no need to hold the Fastopen-lock */
> > +		ctx = rcu_dereference_protected(ctx->next, 1);
> > +		crypto_free_cipher(prev->tfm);
> > +		kfree(prev);
> > +	}
> >
> 
> It seems this function does not need to be changed, since at most one context
> should be freed per run ?

It gets called from tcp_fastopen_destroy_cipher() though (to destroy the
socket's TFO-keys when the socket gets closed). There it has to destroy the
whole list.

Same when going through exit_batch for the namespace.


We could of course split it in tcp_fastopen_ctx_free_one() and
tcp_fastopen_ctx_free_all(). But maybe that's overkill as it's a rare thing
to do?


Christoph

Re: [PATCH net-next 1/5] tcp: Create list of TFO-contexts

[Eric Dumazet]

On Fri, Dec 14, 2018 at 2:40 PM Christoph Paasch <cpaasch@apple.com> wrote:
>

...

>  int tcp_fastopen_reset_cipher(struct net *net, struct sock *sk,
>                               void *key, unsigned int len)
>  {
> @@ -96,13 +131,22 @@ error:             kfree(ctx);
>         spin_lock(&net->ipv4.tcp_fastopen_ctx_lock);
>         if (sk) {
>                 q = &inet_csk(sk)->icsk_accept_queue.fastopenq;

> +               rcu_assign_pointer(ctx->next, q->ctx);
At this point, ctx is not yet visible, so you do not need a barrier yet
                    ctx->next = q->ctx;


> +               rcu_assign_pointer(q->ctx, ctx);

Note that readers could see 3 contexts in the chain, instead of maximum two.

This means that proc_tcp_fastopen_key() (your 3/5 change) would
overflow an automatic array :

while (ctxt) {
        memcpy(&key[i], ctxt->key, TCP_FASTOPEN_KEY_LENGTH);
        i += 4;
        ctxt = rcu_dereference(ctxt->next);
}


> +
>                 octx = rcu_dereference_protected(q->ctx,
>                         lockdep_is_held(&net->ipv4.tcp_fastopen_ctx_lock));
> -               rcu_assign_pointer(q->ctx, ctx);
> +
> +               octx = tcp_fastopen_cut_keypool(octx, &net->ipv4.tcp_fastopen_ctx_lock);
>         } else {
> +               rcu_assign_pointer(ctx->next, net->ipv4.tcp_fastopen_ctx);

same remark here.

> +               rcu_assign_pointer(net->ipv4.tcp_fastopen_ctx, ctx);
> +
>                 octx = rcu_dereference_protected(net->ipv4.tcp_fastopen_ctx,
>                         lockdep_is_held(&net->ipv4.tcp_fastopen_ctx_lock));
> -               rcu_assign_pointer(net->ipv4.tcp_fastopen_ctx, ctx);
> +
> +               octx = tcp_fastopen_cut_keypool(octx,
> +                                               &net->ipv4.tcp_fastopen_ctx_lock);
>         }
>         spin_unlock(&net->ipv4.tcp_fastopen_ctx_lock);
>
> --
> 2.16.2
>

Re: [PATCH net-next 1/5] tcp: Create list of TFO-contexts

[Eric Dumazet]

On 12/17/2018 07:49 AM, Christoph Paasch wrote:

> It gets called from tcp_fastopen_destroy_cipher() though (to destroy the
> socket's TFO-keys when the socket gets closed). There it has to destroy the
> whole list.
> 
> Same when going through exit_batch for the namespace.
> 
> 
> We could of course split it in tcp_fastopen_ctx_free_one() and
> tcp_fastopen_ctx_free_all(). But maybe that's overkill as it's a rare thing
> to do?


No, this is fine, please see my second review of this first patch.

Thanks.

Re: [PATCH net-next 3/5] tcp: Print list of TFO-keys from proc

[Yuchung Cheng]

On Sun, Dec 16, 2018 at 10:32 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>
> On 12/14/2018 02:40 PM, Christoph Paasch wrote:
> > Print the list of the TFO-keys with a comma separated. For setting the
> > keys, we still only allow a single one to be set.
> >
>
> I wonder if some applications expecting current format could break
> after a formatting change.
I have the same concern as well. print the extra keys in a different
sysctl maybe? e.g. net.ipv4.tcp_fastopen_alt_keys
>

Re: [PATCH net-next 1/5] tcp: Create list of TFO-contexts

[Christoph Paasch]

On 17/12/18 - 08:04:08, Eric Dumazet wrote:
> On Fri, Dec 14, 2018 at 2:40 PM Christoph Paasch <cpaasch@apple.com> wrote:
> >
> 
> ...
> 
> >  int tcp_fastopen_reset_cipher(struct net *net, struct sock *sk,
> >                               void *key, unsigned int len)
> >  {
> > @@ -96,13 +131,22 @@ error:             kfree(ctx);
> >         spin_lock(&net->ipv4.tcp_fastopen_ctx_lock);
> >         if (sk) {
> >                 q = &inet_csk(sk)->icsk_accept_queue.fastopenq;
> 
> > +               rcu_assign_pointer(ctx->next, q->ctx);
> At this point, ctx is not yet visible, so you do not need a barrier yet
>                     ctx->next = q->ctx;

Thanks, I will change that.

> 
> 
> > +               rcu_assign_pointer(q->ctx, ctx);
> 
> Note that readers could see 3 contexts in the chain, instead of maximum two.
> 
> This means that proc_tcp_fastopen_key() (your 3/5 change) would
> overflow an automatic array :
> 
> while (ctxt) {
>         memcpy(&key[i], ctxt->key, TCP_FASTOPEN_KEY_LENGTH);
>         i += 4;
>         ctxt = rcu_dereference(ctxt->next);
> }

Ouch! Thanks for spotting this.

If it's ok to have a brief moment of 3 contexts for the readers, I would
protect against overflows the readers.

> > +
> >                 octx = rcu_dereference_protected(q->ctx,
> >                         lockdep_is_held(&net->ipv4.tcp_fastopen_ctx_lock));
> > -               rcu_assign_pointer(q->ctx, ctx);
> > +
> > +               octx = tcp_fastopen_cut_keypool(octx, &net->ipv4.tcp_fastopen_ctx_lock);
> >         } else {
> > +               rcu_assign_pointer(ctx->next, net->ipv4.tcp_fastopen_ctx);
> 
> same remark here.

Same, will change that.


Christoph

> > +               rcu_assign_pointer(net->ipv4.tcp_fastopen_ctx, ctx);
> > +
> >                 octx = rcu_dereference_protected(net->ipv4.tcp_fastopen_ctx,
> >                         lockdep_is_held(&net->ipv4.tcp_fastopen_ctx_lock));
> > -               rcu_assign_pointer(net->ipv4.tcp_fastopen_ctx, ctx);
> > +
> > +               octx = tcp_fastopen_cut_keypool(octx,
> > +                                               &net->ipv4.tcp_fastopen_ctx_lock);
> >         }
> >         spin_unlock(&net->ipv4.tcp_fastopen_ctx_lock);
> >
> > --
> > 2.16.2
> >

Re: [PATCH net-next 1/5] tcp: Create list of TFO-contexts

[Eric Dumazet]

On Mon, Dec 17, 2018 at 1:57 PM Christoph Paasch <cpaasch@apple.com> wrote:
>
> On 17/12/18 - 08:04:08, Eric Dumazet wrote:
> > On Fri, Dec 14, 2018 at 2:40 PM Christoph Paasch <cpaasch@apple.com> wrote:
> > >
> >
> > ...
> >
> > >  int tcp_fastopen_reset_cipher(struct net *net, struct sock *sk,
> > >                               void *key, unsigned int len)
> > >  {
> > > @@ -96,13 +131,22 @@ error:             kfree(ctx);
> > >         spin_lock(&net->ipv4.tcp_fastopen_ctx_lock);
> > >         if (sk) {
> > >                 q = &inet_csk(sk)->icsk_accept_queue.fastopenq;
> >
> > > +               rcu_assign_pointer(ctx->next, q->ctx);
> > At this point, ctx is not yet visible, so you do not need a barrier yet
> >                     ctx->next = q->ctx;
>
> Thanks, I will change that.
>
> >
> >
> > > +               rcu_assign_pointer(q->ctx, ctx);
> >
> > Note that readers could see 3 contexts in the chain, instead of maximum two.
> >
> > This means that proc_tcp_fastopen_key() (your 3/5 change) would
> > overflow an automatic array :
> >
> > while (ctxt) {
> >         memcpy(&key[i], ctxt->key, TCP_FASTOPEN_KEY_LENGTH);
> >         i += 4;
> >         ctxt = rcu_dereference(ctxt->next);
> > }
>
> Ouch! Thanks for spotting this.
>
> If it's ok to have a brief moment of 3 contexts for the readers, I would
> protect against overflows the readers.

I believe you can refactor the code here, to publish the new pointer
(rcu_assign_pointer(ctx->next, q->ctx);)
only after you have shorten the chain.

No worries if one incoming packet can see only the old primary key,
but not the fallback one,
since we are anyway about to remove the old fallback.

Ideally the rcu_assign_pointer(ctx->next, q->ctx) operation should be
the last one, when the new chain
is clean and ready to be used.

Re: [PATCH net-next 1/5] tcp: Create list of TFO-contexts

[Christoph Paasch]

On 17/12/18 - 14:01:41, Eric Dumazet wrote:
> On Mon, Dec 17, 2018 at 1:57 PM Christoph Paasch <cpaasch@apple.com> wrote:
> >
> > On 17/12/18 - 08:04:08, Eric Dumazet wrote:
> > > On Fri, Dec 14, 2018 at 2:40 PM Christoph Paasch <cpaasch@apple.com> wrote:
> > > >
> > >
> > > ...
> > >
> > > >  int tcp_fastopen_reset_cipher(struct net *net, struct sock *sk,
> > > >                               void *key, unsigned int len)
> > > >  {
> > > > @@ -96,13 +131,22 @@ error:             kfree(ctx);
> > > >         spin_lock(&net->ipv4.tcp_fastopen_ctx_lock);
> > > >         if (sk) {
> > > >                 q = &inet_csk(sk)->icsk_accept_queue.fastopenq;
> > >
> > > > +               rcu_assign_pointer(ctx->next, q->ctx);
> > > At this point, ctx is not yet visible, so you do not need a barrier yet
> > >                     ctx->next = q->ctx;
> >
> > Thanks, I will change that.
> >
> > >
> > >
> > > > +               rcu_assign_pointer(q->ctx, ctx);
> > >
> > > Note that readers could see 3 contexts in the chain, instead of maximum two.
> > >
> > > This means that proc_tcp_fastopen_key() (your 3/5 change) would
> > > overflow an automatic array :
> > >
> > > while (ctxt) {
> > >         memcpy(&key[i], ctxt->key, TCP_FASTOPEN_KEY_LENGTH);
> > >         i += 4;
> > >         ctxt = rcu_dereference(ctxt->next);
> > > }
> >
> > Ouch! Thanks for spotting this.
> >
> > If it's ok to have a brief moment of 3 contexts for the readers, I would
> > protect against overflows the readers.
> 
> I believe you can refactor the code here, to publish the new pointer
> (rcu_assign_pointer(ctx->next, q->ctx);)
> only after you have shorten the chain.
> 
> No worries if one incoming packet can see only the old primary key,
> but not the fallback one,
> since we are anyway about to remove the old fallback.
> 
> Ideally the rcu_assign_pointer(ctx->next, q->ctx) operation should be
> the last one, when the new chain
> is clean and ready to be used.

Sounds good, I will do that.

Thanks,
Christoph

Re: [PATCH net-next 2/5] tcp: TFO: search for correct cookie and accept data

[Christoph Paasch]

On 16/12/18 - 22:30:51, Eric Dumazet wrote:
> 
> 
> On 12/14/2018 02:40 PM, Christoph Paasch wrote:
> > This change allows to search for the right cookie and accepts old ones
> > (announcing a new one if it has changed).
> > 
> > __tcp_fastopen_cookie_gen_with_ctx() allows to generate a cookie based
> > on a given TFO-context. A later patch will cleanup the duplicate code.
> 
> How long is kept the secondary (old) context ?

There is no time-limit on keeping the older context.

In an older version of this series I had the pool-size as a sysctl so one
could try out different configurations. For us, a size of 2 was good enough.

I could bring that back if you think it's useful.

> I do not know exact crypto_cipher_encrypt_one() cost, but it looks like
> your patch could double the cost of some TFO based attacks ?

True, we are doing more crypto if we are getting a lot of invalid or old cookies.
I don't have a good answer to that besides that one should probably disable
TFO at that point ;-)

On the other hand, AFAICS tcp_conn_request will end up setting want_cookie
to true under SYN-flooding so we won't even enter tcp_try_fastopen if it's
really an attack.



Christoph

Re: [PATCH net-next 3/5] tcp: Print list of TFO-keys from proc

[Christoph Paasch]

On 17/12/18 - 08:52:22, Yuchung Cheng wrote:
> On Sun, Dec 16, 2018 at 10:32 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >
> >
> >
> > On 12/14/2018 02:40 PM, Christoph Paasch wrote:
> > > Print the list of the TFO-keys with a comma separated. For setting the
> > > keys, we still only allow a single one to be set.
> > >
> >
> > I wonder if some applications expecting current format could break
> > after a formatting change.
> I have the same concern as well. print the extra keys in a different
> sysctl maybe? e.g. net.ipv4.tcp_fastopen_alt_keys

True, some apps might break on that.


Having a single place where all the keys are shown is still useful as that
way the key-rotation can simply check the current keys in one place.

I'm fine with adding net.ipv4.tcp_fastopen_key_list or something like that,
if we want to keep sysctl-API stable.


Christoph

Re: [PATCH net-next 3/5] tcp: Print list of TFO-keys from proc

[Yuchung Cheng]

On Mon, Dec 17, 2018 at 3:35 PM Christoph Paasch <cpaasch@apple.com> wrote:
>
> On 17/12/18 - 08:52:22, Yuchung Cheng wrote:
> > On Sun, Dec 16, 2018 at 10:32 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > >
> > >
> > >
> > > On 12/14/2018 02:40 PM, Christoph Paasch wrote:
> > > > Print the list of the TFO-keys with a comma separated. For setting the
> > > > keys, we still only allow a single one to be set.
> > > >
> > >
> > > I wonder if some applications expecting current format could break
> > > after a formatting change.
> > I have the same concern as well. print the extra keys in a different
> > sysctl maybe? e.g. net.ipv4.tcp_fastopen_alt_keys
>
> True, some apps might break on that.
>
>
> Having a single place where all the keys are shown is still useful as that
> way the key-rotation can simply check the current keys in one place.
That's a good point - I am neutral now to use your existing proposal. Eric?

>
> I'm fine with adding net.ipv4.tcp_fastopen_key_list or something like that,
> if we want to keep sysctl-API stable.
>
>
> Christoph
>

Re: [PATCH net-next 5/5] tcp: TFO - cleanup code duplication

[Christoph Paasch]

On 16/12/18 - 22:33:52, Eric Dumazet wrote:
> 
> 
> On 12/14/2018 02:40 PM, Christoph Paasch wrote:
> > We can actually easily reuse __tcp_fastopen_cookie_gen_with_ctx to
> > generate the cookie based on a give TFO-context.
> >
> 
> s/give/given/

Thanks! Will be fixed in the v2.


Christoph