Skip to contentSkip to conversation

TCPCT+1: initial SYN exchange with SYNACK data

PATCH In review William Allen Simpson 3 participants 11 messages Oct 3, 2009 – Oct 5, 2009

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 TCPCT+1: initial SYN exchange with SYNACK data

This is a straightforward re-implementation of an earlier (year-old) patch that no longer applies cleanly, with permission of the original author (Adam Langley). The patch was previously reviewed:

http://thread.gmane.org/gmane.linux.network/102586

The principle difference is using a TCP option to carry the cookie nonce, instead of a user configured offset in the data. This is more flexible and less subject to user configuration error. Such a cookie option has been suggested for many years, and is also useful without SYN data, allowing several related concepts to use the same extension option.

"Re: SYN floods (was: does history repeat itself?)", September 9, 1996.
http://www.merit.net/mail.archives/nanog/1996-09/msg00235.html

"Re: what a new TCP header might look like", May 12, 1998. ftp://ftp.isi.edu/end2end/end2end-interest-1998.mail

As suggested, the CONFIG_ was replaced by a sysctl (tcp_cookie_size) to turn on and off the cookie option default globally.

These functions will also be used in subsequent patches that implement additional features.

---
 include/linux/tcp.h        |   92 +++++++++++++++++--
 include/net/tcp.h          |   48 +++++++++-
 net/ipv4/sysctl_net_ipv4.c |    8 ++
 net/ipv4/tcp.c             |   97 +++++++++++++++++++-
 net/ipv4/tcp_input.c       |   38 ++++++++-
 net/ipv4/tcp_ipv4.c        |   54 ++++++++++-
 net/ipv4/tcp_minisocks.c   |   32 +++++-
 net/ipv4/tcp_output.c      |  220 +++++++++++++++++++++++++++++++++++++++++---
 net/ipv6/tcp_ipv6.c        |   35 +++++++-
 9 files changed, 580 insertions(+), 44 deletions(-)
include/linux/tcp.h +85 -7
index 61723a7..bdd1a7f 100644
@@ -96,6 +96,7 @@ enum {
 #define TCP_QUICKACK		12	/* Block/reenable quick acks */
 #define TCP_CONGESTION		13	/* Congestion control algorithm */
 #define TCP_MD5SIG		14	/* TCP MD5 Signature (RFC2385) */
+#define TCP_COOKIE_DATA		15	/* TCP Cookie Transactions extension */
 
 #define TCPI_OPT_TIMESTAMPS	1
 #define TCPI_OPT_SACK		2
@@ -170,6 +171,33 @@ struct tcp_md5sig {
 	__u8	tcpm_key[TCP_MD5SIG_MAXKEYLEN];		/* key (binary) */
 };
 
+/* for TCP_COOKIE_DATA socket option */
+#define TCP_COOKIE_MAX		16		/* 128-bits */
+#define TCP_COOKIE_MIN		 8		/*  64-bits */
+#define TCP_COOKIE_PAIR_SIZE	(2*TCP_COOKIE_MAX)
+
+#define TCP_S_DATA_MAX		64U		/* after TCP+IP options */
+#define TCP_S_DATA_MSS_DEFAULT	536U		/* default MSS (RFC1122) */
+
+/* Flags for both getsockopt and setsockopt */
+#define TCP_COOKIE_IN_ALWAYS	(1 << 0)	/* Discard SYN without cookie */
+#define TCP_COOKIE_OUT_NEVER	(1 << 1)	/* Prohibit outgoing cookies.
+						   Supercedes the others. */
+
+/* Flags for getsockopt */
+#define TCP_S_DATA_IN		(1 << 2)	/* Was data received? */
+#define TCP_S_DATA_OUT		(1 << 3)	/* Was data sent? */
+
+/* TCP Cookie Transactions data */
+struct tcp_cookie_data {
+	__u16	tcpcd_flags;			/* see above */
+	__u8	__tcpcd_pad1;			/* zero */
+	__u8	tcpcd_cookie_desired;		/* bytes */
+	__u16	tcpcd_s_data_desired;		/* bytes of variable data */
+	__u16	tcpcd_used;			/* bytes in value */
+	__u8	tcpcd_value[TCP_S_DATA_MSS_DEFAULT];
+};
+
 #ifdef __KERNEL__
 
 #include <linux/skbuff.h>
@@ -210,33 +238,53 @@ struct tcp_options_received {
 	u32	ts_recent;	/* Time stamp to echo next		*/
 	u32	rcv_tsval;	/* Time stamp value             	*/
 	u32	rcv_tsecr;	/* Time stamp echo reply        	*/
-	u16 	saw_tstamp : 1,	/* Saw TIMESTAMP on last packet		*/
+	u32 	saw_tstamp : 1,	/* Saw TIMESTAMP on last packet		*/
 		tstamp_ok : 1,	/* TIMESTAMP seen on SYN packet		*/
 		dsack : 1,	/* D-SACK is scheduled			*/
 		wscale_ok : 1,	/* Wscale seen on SYN packet		*/
 		sack_ok : 4,	/* SACK seen on SYN packet		*/
 		snd_wscale : 4,	/* Window scaling received from sender	*/
-		rcv_wscale : 4;	/* Window scaling to send to receiver	*/
-/*	SACKs data	*/
+		rcv_wscale : 4,	/* Window scaling to send to receiver	*/
+		extend_ok:1;	/* Cookie{less,pair} option seen	*/
+	u8	*cookie_copy;	/* temporary pointer			*/
+	u8	cookie_size;	/* bytes in copy			*/
 	u8	num_sacks;	/* Number of SACK blocks		*/
-	u16	user_mss;  	/* mss requested by user in ioctl */
+	u16	user_mss;	/* mss requested by user in ioctl	*/
 	u16	mss_clamp;	/* Maximal mss, negotiated at connection setup */
 };
 
+static inline void tcp_clear_options(struct tcp_options_received *rx_opt)
+{
+	rx_opt->tstamp_ok = rx_opt->sack_ok = 0;
+	rx_opt->wscale_ok = rx_opt->snd_wscale = 0;
+	rx_opt->cookie_size = rx_opt->extend_ok = 0;
+}
+
 /* This is the max number of SACKS that we'll generate and process. It's safe
  * to increse this, although since:
  *   size = TCPOLEN_SACK_BASE_ALIGNED (4) + n * TCPOLEN_SACK_PERBLOCK (8)
  * only four options will fit in a standard TCP header */
 #define TCP_NUM_SACKS 4
 
+struct tcp_cookie_pair;
+struct tcp_s_data_payload;
+
 struct tcp_request_sock {
 	struct inet_request_sock 	req;
 #ifdef CONFIG_TCP_MD5SIG
 	/* Only used by TCP MD5 Signature so far. */
 	const struct tcp_request_sock_ops *af_specific;
 #endif
-	u32			 	rcv_isn;
-	u32			 	snt_isn;
+	u32				rcv_isn;
+	u32				snt_isn;
+
+	/* Cookie Transactions */
+	u8				*cookie_copy;	/* temporary pointer */
+	u8				cookie_size;	/* bytes in copy */
+	u8				s_data_in:1,
+					s_data_out:1,
+					cookie_in_always:1,
+					cookie_out_never:1;
 };
 
 static inline struct tcp_request_sock *tcp_rsk(const struct request_sock *req)
@@ -406,6 +454,32 @@ struct tcp_sock {
 /* TCP MD5 Signature Option information */
 	struct tcp_md5sig_info	*md5sig_info;
 #endif
+
+	/* If s_data_desired > 0 and s_data_payload is non-NULL, then this
+	 * object holds a reference to it (s_data_payload->kref)
+	 */
+	struct tcp_s_data_payload	*s_data_payload;
+
+	/* When the cookie options are generated and exchanged, then this
+	 * object holds a reference to them (cookie_pair->kref)
+	 */
+	struct tcp_cookie_pair	  	*cookie_pair;
+
+	/* If s_data_payload is non-NULL, then this holds a copy of
+	 * s_data_payload->tsdpl_size.  Otherwise, this holds the user
+	 * specified tcpcd_s_data_desired (variable data).
+	 */
+	u16				s_data_desired;	/* bytes */
+
+	/* Initially, this holds the user specified tcpcd_cookie_desired.
+	 * Zero indicates default (sysctl_tcp_cookie_size).  After the
+	 * option has been exchanged, this holds the actual size.
+	 */
+	u8				cookie_desired;	/* bytes */
+	u8				s_data_in:1,
+					s_data_out:1,
+					cookie_in_always:1,
+					cookie_out_never:1;
 };
 
 static inline struct tcp_sock *tcp_sk(const struct sock *sk)
@@ -424,6 +498,10 @@ struct tcp_timewait_sock {
 	u16			  tw_md5_keylen;
 	u8			  tw_md5_key[TCP_MD5SIG_MAXKEYLEN];
 #endif
+	/* Few sockets in timewait have cookies; in that case, then this
+	 * object holds a reference to it (tw_cookie_pair->kref)
+	 */
+	struct tcp_cookie_pair	  *tw_cookie_pair;
 };
 
 static inline struct tcp_timewait_sock *tcp_twsk(const struct sock *sk)
@@ -431,6 +509,6 @@ static inline struct tcp_timewait_sock *tcp_twsk(const struct sock *sk)
 	return (struct tcp_timewait_sock *)sk;
 }
 
-#endif
+#endif	/* __KERNEL__ */
 
 #endif	/* _LINUX_TCP_H */
include/net/tcp.h +43 -5

84 lines not on this page · raw message

net/ipv4/sysctl_net_ipv4.c +8 -0
index 2dcf04d..3422c54 100644
@@ -714,6 +714,14 @@ static struct ctl_table ipv4_table[] = {
 	},
 	{
 		.ctl_name	= CTL_UNNUMBERED,
+		.procname	= "tcp_cookie_size",
+		.data		= &sysctl_tcp_cookie_size,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec
+	},
+	{
+		.ctl_name	= CTL_UNNUMBERED,
 		.procname	= "udp_mem",
 		.data		= &sysctl_udp_mem,
 		.maxlen		= sizeof(sysctl_udp_mem),
net/ipv4/tcp.c +95 -2

115 lines not on this page · raw message

net/ipv4/tcp_input.c +37 -1
index d86784b..88ffca9 100644
@@ -3782,6 +3782,21 @@ void tcp_parse_options(struct sk_buff *skb, struct tcp_options_received *opt_rx,
 				 */
 				break;
 #endif
+			case TCPOPT_COOKIE:
+				/* This option carries 3 different lengths.
+				 */
+				if (TCPOLEN_COOKIE_MAX >= opsize
+				 && TCPOLEN_COOKIE_MIN <= opsize) {
+					opt_rx->cookie_size =
+						opsize - TCPOLEN_COOKIE_BASE;
+					opt_rx->cookie_copy = ptr;
+					opt_rx->extend_ok = 1;
+				} else if (TCPOLEN_COOKIE_PAIR == opsize) {
+					/* not yet implemented */
+				} else if (TCPOLEN_COOKIE_BASE == opsize) {
+					/* not yet implemented */
+				}
+				break;
 			}
 
 			ptr += opsize-2;
@@ -5364,6 +5379,7 @@ static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb,
 	struct tcp_sock *tp = tcp_sk(sk);
 	struct inet_connection_sock *icsk = inet_csk(sk);
 	int saved_clamp = tp->rx_opt.mss_clamp;
+	bool s_data_queued = false;
 
 	tcp_parse_options(skb, &tp->rx_opt, 0);
 
@@ -5462,6 +5478,23 @@ static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb,
 		 * Change state from SYN-SENT only after copied_seq
 		 * is initialized. */
 		tp->copied_seq = tp->rcv_nxt;
+
+		/* If the cookie extension option is present, and there's
+		 * some incoming transaction data, queue it.
+		 */
+		if (tp->rx_opt.extend_ok
+		 && skb->len > (th->doff << 2)) {
+			__skb_pull(skb, th->doff << 2);
+			__skb_queue_tail(&sk->sk_receive_queue, skb);
+			skb_set_owner_r(skb, sk);
+			sk->sk_data_ready(sk, 0);
+			s_data_queued = true;
+			tp->s_data_in = 1; /* true */
+			tp->rcv_nxt = TCP_SKB_CB(skb)->end_seq;
+			tp->rcv_wup = TCP_SKB_CB(skb)->end_seq;
+			tp->copied_seq = TCP_SKB_CB(skb)->seq + 1;
+		}
+
 		smp_mb();
 		tcp_set_state(sk, TCP_ESTABLISHED);
 
@@ -5513,11 +5546,14 @@ static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb,
 						  TCP_DELACK_MAX, TCP_RTO_MAX);
 
 discard:
-			__kfree_skb(skb);
+			if (!s_data_queued)
+				__kfree_skb(skb);
 			return 0;
 		} else {
 			tcp_send_ack(sk);
 		}
+		if (s_data_queued)
+			return 0;
 		return -1;
 	}
 
net/ipv4/tcp_ipv4.c +48 -6

99 lines not on this page · raw message

net/ipv4/tcp_minisocks.c +26 -6
index 624c3c9..c38e901 100644
@@ -375,6 +375,13 @@ static inline void TCP_ECN_openreq_child(struct tcp_sock *tp,
 	tp->ecn_flags = inet_rsk(req)->ecn_ok ? TCP_ECN_OK : 0;
 }
 
+static inline int tcp_s_data_size(const struct tcp_sock *tp)
+{
+	return (0 < tp->s_data_desired && NULL != tp->s_data_payload)
+		? tp->s_data_payload->tsdpl_size
+		: 0;
+}
+
 /* This is not only more efficient than what we used to do, it eliminates
  * a lot of code duplication between IPv4/IPv6 SYN recv processing. -DaveM
  *
@@ -394,9 +401,12 @@ struct sock *tcp_create_openreq_child(struct sock *sk, struct request_sock *req,
 		/* Now setup tcp_sock */
 		newtp = tcp_sk(newsk);
 		newtp->pred_flags = 0;
-		newtp->rcv_wup = newtp->copied_seq = newtp->rcv_nxt = treq->rcv_isn + 1;
-		newtp->snd_sml = newtp->snd_una = newtp->snd_nxt = treq->snt_isn + 1;
-		newtp->snd_up = treq->snt_isn + 1;
+
+		newtp->rcv_wup = newtp->copied_seq =
+		newtp->rcv_nxt = treq->rcv_isn + 1;
+
+		newtp->snd_sml = newtp->snd_una = newtp->snd_nxt =
+		newtp->snd_up = treq->snt_isn + 1 + tcp_s_data_size(tcp_sk(sk));
 
 		tcp_prequeue_init(newtp);
 
@@ -429,8 +439,17 @@ struct sock *tcp_create_openreq_child(struct sock *sk, struct request_sock *req,
 		tcp_set_ca_state(newsk, TCP_CA_Open);
 		tcp_init_xmit_timers(newsk);
 		skb_queue_head_init(&newtp->out_of_order_queue);
-		newtp->write_seq = treq->snt_isn + 1;
-		newtp->pushed_seq = newtp->write_seq;
+		newtp->write_seq = newtp->pushed_seq =
+			treq->snt_isn + 1 + tcp_s_data_size(tcp_sk(sk));
+
+		newtp->s_data_payload = NULL;
+		newtp->cookie_pair = NULL;
+		newtp->s_data_desired = 0;
+		newtp->cookie_desired = treq->cookie_size;
+		newtp->s_data_in = treq->s_data_in;
+		newtp->s_data_out = treq->s_data_out;
+		newtp->cookie_in_always = treq->cookie_in_always;
+		newtp->cookie_out_never = treq->cookie_out_never;
 
 		newtp->rx_opt.saw_tstamp = 0;
 
@@ -596,7 +615,8 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
 	 * Invalid ACK: reset will be sent by listening socket
 	 */
 	if ((flg & TCP_FLAG_ACK) &&
-	    (TCP_SKB_CB(skb)->ack_seq != tcp_rsk(req)->snt_isn + 1))
+	    (TCP_SKB_CB(skb)->ack_seq != tcp_rsk(req)->snt_isn + 1 +
+					 tcp_s_data_size(tcp_sk(sk))))
 		return sk;
 
 	/* Also, it would be not so bad idea to check rcv_tsecr, which
net/ipv4/tcp_output.c +205 -15

320 lines not on this page · raw message

net/ipv6/tcp_ipv6.c +33 -2
index 21d100b..af33758 100644
@@ -1159,11 +1159,12 @@ static struct sock *tcp_v6_hnd_req(struct sock *sk,struct sk_buff *skb)
  */
 static int tcp_v6_conn_request(struct sock *sk, struct sk_buff *skb)
 {
+	u8 bakery[TCP_COOKIE_MAX];
+	struct tcp_options_received tmp_opt;
 	struct inet6_request_sock *treq;
 	struct ipv6_pinfo *np = inet6_sk(sk);
-	struct tcp_options_received tmp_opt;
-	struct tcp_sock *tp = tcp_sk(sk);
 	struct request_sock *req = NULL;
+	struct tcp_sock *tp = tcp_sk(sk);
 	__u32 isn = TCP_SKB_CB(skb)->when;
 #ifdef CONFIG_SYN_COOKIES
 	int want_cookie = 0;
@@ -1205,6 +1206,28 @@ static int tcp_v6_conn_request(struct sock *sk, struct sk_buff *skb)
 
 	tcp_parse_options(skb, &tmp_opt, 0);
 
+	if (tmp_opt.extend_ok
+	 && tmp_opt.saw_tstamp
+	 && !tp->cookie_out_never
+	 && (0 < tp->cookie_desired || 0 < sysctl_tcp_cookie_size)) {
+#ifdef CONFIG_SYN_COOKIES
+		want_cookie = 0;	/* not our kind of cookie */
+#endif
+		tcp_rsk(req)->cookie_out_never = 0;
+		tcp_rsk(req)->cookie_copy = bakery;
+		tcp_rsk(req)->cookie_size = tmp_opt.cookie_size;
+
+		/* secret recipe not yet implemented */
+		get_random_bytes(bakery, tmp_opt.cookie_size);
+	} else if (!tp->cookie_in_always) {
+		/* redundant indications, but ensure initialization. */
+		tcp_rsk(req)->cookie_out_never = 1;
+		tcp_rsk(req)->cookie_size = 0;
+	} else {
+		goto drop;
+	}
+	tcp_rsk(req)->cookie_in_always = tp->cookie_in_always;
+
 	if (want_cookie && !tmp_opt.saw_tstamp)
 		tcp_clear_options(&tmp_opt);
 
@@ -1864,6 +1887,14 @@ static int tcp_v6_init_sock(struct sock *sk)
 	tp->af_specific = &tcp_sock_ipv6_specific;
 #endif
 
+/* For grep, in order of appearance:
+ *	tp->s_data_payload = NULL;
+ *	tp->cookie_pair = NULL;
+ *	tp->s_data_desired = tp->cookie_desired = 0;
+ *	tp->s_data_in = tp->s_data_out = 0;
+ *	tp->cookie_in_always = tp->cookie_out_never = 0;
+ */
+
 	sk->sk_sndbuf = sysctl_tcp_wmem[1];
 	sk->sk_rcvbuf = sysctl_tcp_rmem[1];
 
Signature · 1 line
1.6.0.4
maintainer
#1 lore raw

From: William Allen Simpson <[email protected]>
Date: Sat, 03 Oct 2009 14:33:41 -0400

Quoted 1 line
(quoted diff) +0 -0
@@ -406,6 +454,32 @@ struct tcp_sock {

...

Quoted 1 line
include/linux/tcp.h +1 -0
+	struct tcp_s_data_payload	*s_data_payload;

...

Quoted 1 line
include/linux/tcp.h +1 -0
+	struct tcp_cookie_pair	  	*cookie_pair;

...

Quoted 1 line
include/linux/tcp.h +1 -0
+	u16				s_data_desired;	/* bytes */

...

include/linux/tcp.h struct tcp_sock { · line 482

Quoted 5 lines
include/linux/tcp.h +5 -0
+	u8				cookie_desired;	/* bytes */
+	u8				s_data_in:1,
+					s_data_out:1,
+					cookie_in_always:1,
+					cookie_out_never:1;

struct tcp_sock is already WAY TOO BIG on 64-bit systems, adding 20 more bytes to it for some odd-ball feature is not something I'm willing to do, sorry.

#2 lore raw
Quoted 11 lines · David Miller

From: William Allen Simpson <[email protected]>
Date: Sat, 03 Oct 2009 14:33:41 -0400

Quoted 1 line
(quoted diff) +0 -0
@@ -406,6 +454,32 @@ struct tcp_sock {

...

Quoted 1 line
include/linux/tcp.h +1 -0
+	struct tcp_s_data_payload	*s_data_payload;

...

Quoted 1 line
include/linux/tcp.h +1 -0
+	struct tcp_cookie_pair	  	*cookie_pair;

...

Quoted 1 line
include/linux/tcp.h +1 -0
+	u16				s_data_desired;	/* bytes */

...

Quoted 10 lines

include/linux/tcp.h struct tcp_sock { · line 482

Quoted 5 lines
include/linux/tcp.h +5 -0
+	u8				cookie_desired;	/* bytes */
+	u8				s_data_in:1,
+					s_data_out:1,
+					cookie_in_always:1,
+					cookie_out_never:1;

struct tcp_sock is already WAY TOO BIG on 64-bit systems, adding 20 more bytes to it for some odd-ball feature is not something I'm willing to do, sorry.

I see we're cross-posting at the same time.... Since in your previous review (last year) this issue was not mentioned, is there some other data organization that you would suggest?

http://article.gmane.org/gmane.linux.network/102779

"This looks mostly fine to me. I would even advocate not using a config
option for this."

Perhaps you would prefer this as a config option after all?

As for "odd-ball feature", of course this is the initial pass on a much larger project....

maintainer
#3 lore raw

From: William Allen Simpson <[email protected]>
Date: Mon, 05 Oct 2009 03:58:12 -0400

Quoted 9 lines

I see we're cross-posting at the same time.... Since in your previous review (last year) this issue was not mentioned, is there some other data organization that you would suggest?

http://article.gmane.org/gmane.linux.network/102779

"This looks mostly fine to me. I would even advocate not using a config
option for this."

I'm looking at the patch more closely, and as I learn more things about your change, my position changes.

Quoted 1 line

Perhaps you would prefer this as a config option after all?

No, then nobody is going to deploy this thing (meaning it really will be useless), or everyone will and everyone will enable it and thus eat the space.

It's bad either way.

Store the data either somewhere else or in an extremely compact form.

#4 lore raw
Quoted 5 lines · David Miller

I'm looking at the patch more closely, and as I learn more things about your change, my position changes.

Quoted 1 line

Perhaps you would prefer this as a config option after all?

Quoted 8 lines

No, then nobody is going to deploy this thing (meaning it really will be useless), or everyone will and everyone will enable it and thus eat the space.

It's bad either way.

Store the data either somewhere else or in an extremely compact form.

More cross-posting, a good thing for responsiveness....

OK, no config option.

This will be widely deployed. Deployment in DNS root servers is expected by December. It will be hammered with 600,000+ queries per minute. Does that impact your design expectations?

Tomorrow, I'll send out a new patch with everything buried in a single kref, the various tests will only be a trifle slower....

maintainer
#5 lore raw

From: William Allen Simpson <[email protected]>
Date: Mon, 05 Oct 2009 04:23:48 -0400

Quoted 3 lines

This will be widely deployed. Deployment in DNS root servers is expected by December. It will be hammered with 600,000+ queries per minute. Does that impact your design expectations?

Great, so 600,000 times per second we'll take two new atomic operations with your most recent suggestion...

Quoted 2 lines

Tomorrow, I'll send out a new patch with everything buried in a single kref, the various tests will only be a trifle slower....

This one..

The reason we want tcp_sock smaller is to get better performance (less memory references) and to save memory. If you're exchanging the space savings for atomic operations and whatnot, you're defeating half of the point of making the tcp_sock state smaller.

Make your state take up less space in tcp_sock without making it cost more in some other form.

And btw, it's not our problem that all of a sudden this is critical and important for you, and that you don't like how not all aspects of your implementation were commented upon long ago when the first implementation was posted.

Your insistence to keep harping on those issues will only irritate me (and probably others). So please keep that out of the discussion, thanks.

#6 lore raw
In reply to William Allen Simpson
Quoted 4 lines · William Allen Simpson

This will be widely deployed. Deployment in DNS root servers is expected by December. It will be hammered with 600,000+ queries per minute. Does that impact your design expectations?

10.000 queries per second are challenging, definitly worth to study.

I could not find exact RFC number for this new feature.

Could you give us the up2date info ?

http://ietfreport.isoc.org/all-ids/draft-agl-tcpm-sadata-00.txt is expired.

#7 lore raw
In reply to William Allen Simpson
Quoted 5 lines · William Allen Simpson
Quoted 4 lines · David Miller

struct tcp_sock is already WAY TOO BIG on 64-bit systems, adding 20 more bytes to it for some odd-ball feature is not something I'm willing to do, sorry.

Quoted 9 lines

I see we're cross-posting at the same time.... Since in your previous review (last year) this issue was not mentioned, is there some other data organization that you would suggest?

http://article.gmane.org/gmane.linux.network/102779

"This looks mostly fine to me. I would even advocate not using a config
option for this."

As a quick followup, I wish this had been raised in my previous posting, where I'd given the headers, for exactly this kind of feedback. :-)

Given that size is now a concern, would a single kref pointer with a u16 field for flags be acceptable? I could bury the rest in the kref block.

Would that be acceptable without a config option?

maintainer
#8 lore raw

From: William Allen Simpson <[email protected]>
Date: Mon, 05 Oct 2009 04:11:42 -0400

Quoted 5 lines

Given that size is now a concern, would a single kref pointer with a u16 field for flags be acceptable? I could bury the rest in the kref block.

Would that be acceptable without a config option?

Then we'll eat an atomic operation every connect() or something like that?

That's bad too. We're trying desperately to remove as many atomic operations as possible from the socket paths.

Compress your state, really compress it, don't just externalize it somewhere else in exchange for a different cost.

#9 lore raw
Quoted 3 lines · David Miller

Then we'll eat an atomic operation every connect() or something like that?

Yes, on the client side. On the server side, I was trying to avoid any until after a 3-way handshake (the reason that I removed the kref'd struct in Adam's request sock).

Adam:
+ struct tcp_sadata_payload *sadata_payload;
+ u8 sadata_nonce[4]; /* generated nonce */
+ u8 sadata_ok:1; /* send sadata? */

Mine:
+ u8 *cookie_copy; /* temporary pointer */
+ u8 cookie_size; /* bytes in copy */
+ u8 s_data_in:1,
+ s_data_out:1,
+ cookie_in_always:1,
+ cookie_out_never:1;

Mine makes the code more tricky, though. I'm depending on the various locks and hashes to keep things kosher, whereas Adam kept more kref'd pointer instances lying around for a longer time. Tradeoffs....

But I *was* patting myself on the back for saving 3 bytes here. ;-)

Quoted 3 lines

That's bad too. We're trying desperately to remove as many atomic operations as possible from the socket paths.

And Adam had done a good job, too. The state has to go somewhere!

After all, I've plenty of experience with small TCP implementations, as *all* implementations were constrained back in the day. At least one of my tiny TCP/IP/PPP stack implementations is in hundreds of millions of cell phones (originally on 186s with only a few K of RAM).

Quoted 3 lines

Compress your state, really compress it, don't just externalize it somewhere else in exchange for a different cost.

Cookies are pseudo-random nonces, not compressible by definition.

I think I'll be able to join everything into a single kref'd block, with some modification/reuse of various function md5sig parameters to pass pointers.... Cookie nonces are redundant to the authenticator option(s), so they're never around at the same time.

That will whittle the tcp_sock size down some more, although it could increase the overall memory footprint, as there will be wasted space for the case where there are cookies without SYN data. By keeping the sizes and flags in the sock itself, both Adam and I were optimizing performance.

It's just tradeoffs, always and forever tradeoffs.

#10 lore raw
In reply to William Allen Simpson

As I'm new to Linux kernel development, this was based entirely on code previously reviewed (by Miller), as that seemed a good path for me to learn proper coding and style.

Now that I'm trying to grok Linux locking functions for the next patch, I've noticed that setsockopt code uses lock_sock(), but getsockopt doesn't. In a preemptive kernel, or with SMP, isn't there a possibility that these socket values could be modified or destroyed at the same time?

I'm especially concerned here, as there are kref blocks, and they could be left pointing into the weeds?