tcp: Introduce a TFO key-pool for clean cookie-rotation
Subsystems (3)
cover tcp: Introduce a TFO key-pool for clean cookie-rotation
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(-)
Signature · 1 line
2.16.2
PATCH 1/5 tcp: Create list of TFO-contexts
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).
--- include/net/tcp.h | 2 ++ net/ipv4/tcp_fastopen.c | 52 +++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 50 insertions(+), 4 deletions(-)
include/net/tcp.h +2 -0
@@ -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;
net/ipv4/tcp_fastopen.c +48 -4
75 lines not on this page · raw message
Signature · 1 line
2.16.2
Re: [PATCH net-next 1/5] tcp: Create list of TFO-contexts
Quoted 38 lines · Christoph Paasch
Instead of having a single TFO-context, we now have a list of
tcp_fastopen_context, bounded byTCP_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 <[email protected]>--- include/net/tcp.h | 2 ++ net/ipv4/tcp_fastopen.c | 52 +++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 50 insertions(+), 4 deletions(-)include/net/tcp.h +2
-0@@ -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;net/ipv4/tcp_fastopen.c +0
-1@@ -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);
net/ipv4/tcp_fastopen.c static void tcp_fastopen_ctx_free(struct rcu_head *head) · line 48
Quoted 10 lines
net/ipv4/tcp_fastopen.c +8
-1-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 1/5] tcp: Create list of TFO-contexts
Quoted 46 lines · Eric Dumazet
Quoted 43 lines · Christoph Paasch
Instead of having a single TFO-context, we now have a list of
tcp_fastopen_context, bounded byTCP_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 <[email protected]>--- include/net/tcp.h | 2 ++ net/ipv4/tcp_fastopen.c | 52 +++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 50 insertions(+), 4 deletions(-)include/net/tcp.h +2
-0@@ -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;net/ipv4/tcp_fastopen.c +4
-2@@ -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 */
Quoted 8 lines
net/ipv4/tcp_fastopen.c static void tcp_fastopen_ctx_free(struct rcu_head *head) · line 49
Quoted 5 lines
net/ipv4/tcp_fastopen.c +4
-0+ 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
Quoted 10 lines · Christoph Paasch
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_batchfor the namespace.We could of course split it in
tcp_fastopen_ctx_free_one()andtcp_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 1/5] tcp: Create list of TFO-contexts
In reply to Christoph PaaschQuoted 1 line · Christoph Paasch
...
net/ipv4/tcp_fastopen.c error: kfree(ctx); · line 135
Quoted 9 lines
int tcp_fastopen_reset_cipher(struct net *net, struct sock *sk,
void *key, unsigned int len)
{net/ipv4/tcp_fastopen.c +1
-0@@ -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;
net/ipv4/tcp_fastopen.c error: kfree(ctx); · line 135
Quoted 1 line
net/ipv4/tcp_fastopen.c +1
-0+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);
}net/ipv4/tcp_fastopen.c error: kfree(ctx); · line 142
Quoted 8 lines
net/ipv4/tcp_fastopen.c +4
-1+ 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.
Quoted 14 lines
net/ipv4/tcp_fastopen.c +5
-2+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
Quoted 13 lines · Eric Dumazet
Quoted 1 line · Christoph Paasch
...
Quoted 7 lines
int tcp_fastopen_reset_cipher(struct net *net, struct sock *sk,
void *key, unsigned int len)
{net/ipv4/tcp_fastopen.c +0
-0@@ -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;
Quoted 3 lines
net/ipv4/tcp_fastopen.c error: kfree(ctx); · line 134
Quoted 1 line
net/ipv4/tcp_fastopen.c +1
-0+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.
Quoted 6 lines
net/ipv4/tcp_fastopen.c error: kfree(ctx); · line 135
Quoted 3 lines
net/ipv4/tcp_fastopen.c +1
-0+rcu_assign_pointer (q->ctx, ctx);Note that readers could see 3 contexts in the chain, instead of maximum two.
Quoted 8 lines
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.
Quoted 10 lines
net/ipv4/tcp_fastopen.c error: kfree(ctx); · line 142
Quoted 8 lines
net/ipv4/tcp_fastopen.c +4
-1+ 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
Quoted 14 lines
Quoted 14 lines
net/ipv4/tcp_fastopen.c +5
-2+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
Quoted 36 lines · Christoph Paasch
Quoted 13 lines · Eric Dumazet
Quoted 1 line · Christoph Paasch
...
Quoted 7 lines
int tcp_fastopen_reset_cipher(struct net *net, struct sock *sk,
void *key, unsigned int len)
{net/ipv4/tcp_fastopen.c +0
-0@@ -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;Quoted 3 lines
net/ipv4/tcp_fastopen.c error: kfree(ctx); · line 134
Quoted 1 line
net/ipv4/tcp_fastopen.c +1
-0+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.
Quoted 6 lines
net/ipv4/tcp_fastopen.c error: kfree(ctx); · line 135
Quoted 3 lines
net/ipv4/tcp_fastopen.c +1
-0+rcu_assign_pointer (q->ctx, ctx);Note that readers could see 3 contexts in the chain, instead of maximum two.
Quoted 8 lines
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); }
Quoted 4 lines
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
Quoted 46 lines · Eric Dumazet
Quoted 36 lines · Christoph Paasch
Quoted 13 lines · Eric Dumazet
Quoted 1 line · Christoph Paasch
...
Quoted 7 lines
int tcp_fastopen_reset_cipher(struct net *net, struct sock *sk,
void *key, unsigned int len)
{net/ipv4/tcp_fastopen.c +0
-0@@ -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;Quoted 3 lines
net/ipv4/tcp_fastopen.c error: kfree(ctx); · line 134
Quoted 1 line
net/ipv4/tcp_fastopen.c +1
-0+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.
Quoted 6 lines
net/ipv4/tcp_fastopen.c error: kfree(ctx); · line 135
Quoted 3 lines
net/ipv4/tcp_fastopen.c +1
-0+rcu_assign_pointer (q->ctx, ctx);Note that readers could see 3 contexts in the chain, instead of maximum two.
Quoted 8 lines
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); }Quoted 4 lines
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.
Quoted 7 lines
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
PATCH 2/5 tcp: TFO: search for correct cookie and accept data
In reply to Christoph PaaschThis 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.
--- include/uapi/linux/snmp.h | 1 + net/ipv4/proc.c | 1 + net/ipv4/tcp_fastopen.c | 105 +++++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 101 insertions(+), 6 deletions(-)
include/uapi/linux/snmp.h +1 -0
@@ -283,6 +283,7 @@ enumLINUX_MIB_TCPACKCOMPRESSED ,/* TCPAckCompressed */ LINUX_MIB_TCPZEROWINDOWDROP ,/* TCPZeroWindowDrop */ LINUX_MIB_TCPRCVQDROP ,/* TCPRcvQDrop */ +LINUX_MIB_TCPFASTOPENPASSIVEALTKEY ,/* TCPFastOpenPassiveAltKey */ __LINUX_MIB_MAX };
net/ipv4/proc.c +1 -0
@@ -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 };
net/ipv4/tcp_fastopen.c +99 -6
129 lines not on this page · raw message
Signature · 1 line
2.16.2
Re: [PATCH net-next 2/5] tcp: TFO: search for correct cookie and accept data
Quoted 5 lines · 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.
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 2/5] tcp: TFO: search for correct cookie and accept data
Quoted 10 lines · Eric Dumazet
Quoted 5 lines · 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.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.
Quoted 2 lines
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
PATCH 3/5 tcp: Print list of TFO-keys from proc
In reply to Christoph PaaschPrint the list of the TFO-keys with a comma separated. For setting the keys, we still only allow a single one to be set.
--- net/ipv4/sysctl_net_ipv4.c | 41 ++++++++++++++++++++++++++++++----------- 1 file changed, 30 insertions(+), 11 deletions(-)
net/ipv4/sysctl_net_ipv4.c +30 -11
@@ -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, }, {
Signature · 1 line
2.16.2
Re: [PATCH net-next 3/5] tcp: Print list of TFO-keys from proc
Quoted 3 lines · 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.
I wonder if some applications expecting current format could break after a formatting change.
Re: [PATCH net-next 3/5] tcp: Print list of TFO-keys from proc
Quoted 10 lines · Eric Dumazet
Quoted 3 lines · 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.
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
Quoted 1 line
Re: [PATCH net-next 3/5] tcp: Print list of TFO-keys from proc
Quoted 5 lines · Yuchung Cheng
Quoted 4 lines · Eric Dumazet
On 12/14/2018 02:40 PM, Christoph Paasch wrote:
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
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
Quoted 13 lines · Christoph Paasch
Quoted 11 lines · Yuchung Cheng
Quoted 10 lines · Eric Dumazet
Quoted 3 lines · 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.
I wonder if some applications expecting current format could break after a formatting change.
Quoted 8 lines
Quoted 2 lines
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?
Quoted 7 lines
I'm fine with adding net.ipv4.tcp_fastopen_key_list or something like that, if we want to keep sysctl-API stable.
Christoph
PATCH 4/5 tcp: Allow getsockopt of listener's keypool
In reply to Christoph PaaschAllow to get the full list of the listener's keypool through a getsockopt.
--- net/ipv4/tcp.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)
net/ipv4/tcp.c +9 -6
@@ -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))
Signature · 1 line
2.16.2
PATCH 5/5 tcp: TFO - cleanup code duplication
In reply to Christoph PaaschWe 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.
--- net/ipv4/tcp_fastopen.c | 51 +++++++++++-------------------------------------- 1 file changed, 11 insertions(+), 40 deletions(-)
net/ipv4/tcp_fastopen.c +11 -40
@@ -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; }
Signature · 1 line
2.16.2
Re: [PATCH net-next 5/5] tcp: TFO - cleanup code duplication
Quoted 3 lines · Christoph Paasch
We can actually easily reuse
__tcp_fastopen_cookie_gen_with_ctxto generate the cookie based on a give TFO-context.
s/give/given/
Re: [PATCH net-next 5/5] tcp: TFO - cleanup code duplication
Quoted 8 lines · Eric Dumazet
Quoted 3 lines · Christoph Paasch
We can actually easily reuse
__tcp_fastopen_cookie_gen_with_ctxto generate the cookie based on a give TFO-context.s/give/given/
Thanks! Will be fixed in the v2.
Christoph
David Miller
maintainerFrom: Christoph Paasch <[email protected]>
Date: Fri, 14 Dec 2018 14:40:02 -0800
Quoted 11 lines
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.
Quoted 3 lines
We have seen a significant reduction of
LINUX_MIB_TCPFASTOPENPASSIVEFAILthanks 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.
Quoted 14 lines · David Miller
From: Christoph Paasch <[email protected]>
Date: Fri, 14 Dec 2018 14:40:02 -0800Quoted 11 lines
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.
Quoted 6 lines
Quoted 3 lines
We have seen a significant reduction of
LINUX_MIB_TCPFASTOPENPASSIVEFAILthanks 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.