TCPCT part1: cookie option exchange

I'm torn at the moment, as the net-next tree modules don't compile today. Although 'make vmlinux' compiles this successfully, it is not tested. Keep in mind that earlier variants were extensively tested. But at least this helps move the review comment process along....

Updated to patch cleanly with recent commits.

Changes from previous version:
* swapped conditional tests, requested by Eric and Joe.
* separate patch for constant cleanup, requested by Ilpo.
* added hash generation of Responder Cookie with RCU locking.
* shuffled a few bits to save another byte of tcp socket space.

Add optional function parameters associated with sending SYNACK. These parameters are not needed after sending SYNACK, and are not used for retransmission. Avoids extending struct tcp_request_sock, and avoids allocating kernel memory.

Also affects DCCP as it uses common struct request_sock_ops, but this parameter is currently reserved for future use.

---
 include/net/request_sock.h      |    8 +++++++-
 include/net/tcp.h               |    1 +
 net/dccp/ipv4.c                 |    5 +++--
 net/dccp/ipv6.c                 |    5 +++--
 net/dccp/minisocks.c            |    2 +-
 net/ipv4/inet_connection_sock.c |    2 +-
 net/ipv4/tcp_ipv4.c             |   17 ++++++++++-------
 net/ipv4/tcp_minisocks.c        |    2 +-
 net/ipv4/tcp_output.c           |    1 +
 net/ipv6/tcp_ipv6.c             |   28 +++++++++++++---------------
 10 files changed, 41 insertions(+), 30 deletions(-)

index c719084..c9b50eb 100644
@@ -27,13 +27,19 @@ struct sk_buff;
 struct dst_entry;
 struct proto;
 
+/* empty to "strongly type" an otherwise void parameter.
+ */
+struct request_values {
+};
+
 struct request_sock_ops {
 	int		family;
 	int		obj_size;
 	struct kmem_cache	*slab;
 	char		*slab_name;
 	int		(*rtx_syn_ack)(struct sock *sk,
-				       struct request_sock *req);
+				       struct request_sock *req,
+				       struct request_values *rvp);
 	void		(*send_ack)(struct sock *sk, struct sk_buff *skb,
 				    struct request_sock *req);
 	void		(*send_reset)(struct sock *sk,

index bf20f88..25bf3ba 100644
@@ -443,6 +443,7 @@ extern int			tcp_connect(struct sock *sk);
 
 extern struct sk_buff *		tcp_make_synack(struct sock *sk,
 						struct dst_entry *dst,
+						struct request_values *rvp,
 						struct request_sock *req);
 
 extern int			tcp_disconnect(struct sock *sk, int flags);

index 2423a08..efbcfdc 100644
@@ -477,7 +477,8 @@ static struct dst_entry* dccp_v4_route_skb(struct net *net, struct sock *sk,
 	return &rt->u.dst;
 }
 
-static int dccp_v4_send_response(struct sock *sk, struct request_sock *req)
+static int dccp_v4_send_response(struct sock *sk, struct request_sock *req,
+				 struct request_values *rv_unused)
 {
 	int err = -1;
 	struct sk_buff *skb;

@@ -626,7 +627,7 @@ int dccp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
 	dreq->dreq_iss	   = dccp_v4_init_sequence(skb);
 	dreq->dreq_service = service;
 
-	if (dccp_v4_send_response(sk, req))
+	if (dccp_v4_send_response(sk, req, NULL))
 		goto drop_and_free;
 
 	inet_csk_reqsk_queue_hash_add(sk, req, DCCP_TIMEOUT_INIT);

index 50ea91a..6574215 100644
@@ -241,7 +241,8 @@ out:
 }
 
 
-static int dccp_v6_send_response(struct sock *sk, struct request_sock *req)
+static int dccp_v6_send_response(struct sock *sk, struct request_sock *req,
+				 struct request_values *rv_unused)
 {
 	struct inet6_request_sock *ireq6 = inet6_rsk(req);
 	struct ipv6_pinfo *np = inet6_sk(sk);

@@ -468,7 +469,7 @@ static int dccp_v6_conn_request(struct sock *sk, struct sk_buff *skb)
 	dreq->dreq_iss	   = dccp_v6_init_sequence(skb);
 	dreq->dreq_service = service;
 
-	if (dccp_v6_send_response(sk, req))
+	if (dccp_v6_send_response(sk, req, NULL))
 		goto drop_and_free;
 
 	inet6_csk_reqsk_queue_hash_add(sk, req, DCCP_TIMEOUT_INIT);

index 5ca49ce..af226a0 100644
@@ -184,7 +184,7 @@ struct sock *dccp_check_req(struct sock *sk, struct sk_buff *skb,
 			 * counter (backoff, monitored by dccp_response_timer).
 			 */
 			req->retrans++;
-			req->rsk_ops->rtx_syn_ack(sk, req);
+			req->rsk_ops->rtx_syn_ack(sk, req, NULL);
 		}
 		/* Network Duplicate, discard packet */
 		return NULL;

index 26fb50e..ad098d6 100644
@@ -531,7 +531,7 @@ void inet_csk_reqsk_queue_prune(struct sock *parent,
 					       &expire, &resend);
 				if (!expire &&
 				    (!resend ||
-				     !req->rsk_ops->rtx_syn_ack(parent, req) ||
+				     !req->rsk_ops->rtx_syn_ack(parent, req, NULL) ||
 				     inet_rsk(req)->acked)) {
 					unsigned long timeo;
 

index 657ae33..f83ac91 100644
@@ -743,7 +743,8 @@ static void tcp_v4_reqsk_send_ack(struct sock *sk, struct sk_buff *skb,
  *	socket.
  */
 static int __tcp_v4_send_synack(struct sock *sk, struct request_sock *req,
-				struct dst_entry *dst)
+				struct dst_entry *dst,
+				struct request_values *rvp)
 {
 	const struct inet_request_sock *ireq = inet_rsk(req);
 	int err = -1;

@@ -753,7 +754,7 @@ static int __tcp_v4_send_synack(struct sock *sk, struct request_sock *req,
 	if (!dst && (dst = inet_csk_route_req(sk, req)) == NULL)
 		return -1;
 
-	skb = tcp_make_synack(sk, dst, req);
+	skb = tcp_make_synack(sk, dst, rvp, req);
 
 	if (skb) {
 		struct tcphdr *th = tcp_hdr(skb);

@@ -774,9 +775,10 @@ static int __tcp_v4_send_synack(struct sock *sk, struct request_sock *req,
 	return err;
 }
 
-static int tcp_v4_send_synack(struct sock *sk, struct request_sock *req)
+static int tcp_v4_send_synack(struct sock *sk, struct request_sock *req,
+			      struct request_values *rvp)
 {
-	return __tcp_v4_send_synack(sk, req, NULL);
+	return __tcp_v4_send_synack(sk, req, NULL, rvp);
 }
 
 /*

@@ -1211,13 +1213,13 @@ static struct timewait_sock_ops tcp_timewait_sock_ops = {
 
 int tcp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
 {
-	struct inet_request_sock *ireq;
 	struct tcp_options_received tmp_opt;
 	struct request_sock *req;
+	struct inet_request_sock *ireq;
+	struct dst_entry *dst = NULL;
 	__be32 saddr = ip_hdr(skb)->saddr;
 	__be32 daddr = ip_hdr(skb)->daddr;
 	__u32 isn = TCP_SKB_CB(skb)->when;
-	struct dst_entry *dst = NULL;
 #ifdef CONFIG_SYN_COOKIES
 	int want_cookie = 0;
 #else

@@ -1337,7 +1339,8 @@ int tcp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
 	}
 	tcp_rsk(req)->snt_isn = isn;
 
-	if (__tcp_v4_send_synack(sk, req, dst) || want_cookie)
+	if (__tcp_v4_send_synack(sk, req, dst, NULL)
+	 || want_cookie)
 		goto drop_and_free;
 
 	inet_csk_reqsk_queue_hash_add(sk, req, TCP_TIMEOUT_INIT);

index a9d34e2..fb68bab 100644
@@ -537,7 +537,7 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
 		 * Enforce "SYN-ACK" according to figure 8, figure 6
 		 * of RFC793, fixed by RFC1122.
 		 */
-		req->rsk_ops->rtx_syn_ack(sk, req);
+		req->rsk_ops->rtx_syn_ack(sk, req, NULL);
 		return NULL;
 	}
 

index 616c686..784e6ee 100644
@@ -2224,6 +2224,7 @@ int tcp_send_synack(struct sock *sk)
 
 /* Prepare a SYN-ACK. */
 struct sk_buff *tcp_make_synack(struct sock *sk, struct dst_entry *dst,
+				struct request_values *rvp,
 				struct request_sock *req)
 {
 	struct inet_request_sock *ireq = inet_rsk(req);

index 696a22f..6951827 100644
@@ -461,7 +461,8 @@ out:
 }
 
 
-static int tcp_v6_send_synack(struct sock *sk, struct request_sock *req)
+static int tcp_v6_send_synack(struct sock *sk, struct request_sock *req,
+			      struct request_values *rvp)
 {
 	struct inet6_request_sock *treq = inet6_rsk(req);
 	struct ipv6_pinfo *np = inet6_sk(sk);

@@ -499,7 +500,7 @@ static int tcp_v6_send_synack(struct sock *sk, struct request_sock *req)
 	if ((err = xfrm_lookup(sock_net(sk), &dst, &fl, sk, 0)) < 0)
 		goto done;
 
-	skb = tcp_make_synack(sk, dst, req);
+	skb = tcp_make_synack(sk, dst, rvp, req);
 	if (skb) {
 		struct tcphdr *th = tcp_hdr(skb);
 

@@ -1161,13 +1162,13 @@ 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)
 {
+	struct tcp_options_received tmp_opt;
+	struct request_sock *req;
 	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;
-	__u32 isn = TCP_SKB_CB(skb)->when;
 	struct dst_entry *dst = __sk_dst_get(sk);
+	__u32 isn = TCP_SKB_CB(skb)->when;
 #ifdef CONFIG_SYN_COOKIES
 	int want_cookie = 0;
 #else

@@ -1239,23 +1240,20 @@ static int tcp_v6_conn_request(struct sock *sk, struct sk_buff *skb)
 
 		isn = tcp_v6_init_sequence(skb);
 	}
-
 	tcp_rsk(req)->snt_isn = isn;
 
 	security_inet_conn_request(sk, skb, req);
 
-	if (tcp_v6_send_synack(sk, req))
-		goto drop;
+	if (tcp_v6_send_synack(sk, req, NULL)
+	 || want_cookie)
+		goto drop_and_free;
 
-	if (!want_cookie) {
-		inet6_csk_reqsk_queue_hash_add(sk, req, TCP_TIMEOUT_INIT);
-		return 0;
-	}
+	inet6_csk_reqsk_queue_hash_add(sk, req, TCP_TIMEOUT_INIT);
+	return 0;
 
+drop_and_free:
+	reqsk_free(req);
 drop:
-	if (req)
-		reqsk_free(req);
-
 	return 0; /* don't send reset */
 }
 

1.6.3.3

Define two symbols needed in both kernel and user space.

Remove old (somewhat incorrect) variant that wasn't used in many cases. Default applies to both RMSS and SMSS.

Replace numeric constants with defined symbols.

---
 include/linux/tcp.h      |    6 ++++++
 include/net/tcp.h        |    3 ---
 net/ipv4/tcp_input.c     |    4 ++--
 net/ipv4/tcp_ipv4.c      |    6 +++---
 net/ipv4/tcp_minisocks.c |    2 +-
 net/ipv6/tcp_ipv6.c      |    2 +-
 6 files changed, 13 insertions(+), 10 deletions(-)

index eeecb85..32d7d77 100644
@@ -81,6 +81,12 @@ enum {
 	TCP_DATA_OFFSET = __cpu_to_be32(0xF0000000)
 }; 
 
+/*
+ * TCP general constants
+ */
+#define TCP_MSS_DEFAULT		 536U	/* IPv4 (RFC1122, RFC2581) */
+#define TCP_MSS_DESIRED		1220U	/* IPv6 (tunneled), EDNS0 (RFC3226) */
+
 /* TCP socket options */
 #define TCP_NODELAY		1	/* Turn off Nagle's algorithm. */
 #define TCP_MAXSEG		2	/* Limit MSS */

index 25bf3ba..a413e9f 100644
@@ -62,9 +62,6 @@ extern void tcp_time_wait(struct sock *sk, int state, int timeo);
 /* Minimal accepted MSS. It is (60+60+8) - (20+20). */
 #define TCP_MIN_MSS		88U
 
-/* Minimal RCV_MSS. */
-#define TCP_MIN_RCVMSS		536U
-
 /* The least MTU to use for probing */
 #define TCP_BASE_MSS		512
 

index be0c5bf..cc306ac 100644
@@ -140,7 +140,7 @@ static void tcp_measure_rcv_mss(struct sock *sk, const struct sk_buff *skb)
 		 * "len" is invariant segment length, including TCP header.
 		 */
 		len += skb->data - skb_transport_header(skb);
-		if (len >= TCP_MIN_RCVMSS + sizeof(struct tcphdr) ||
+		if (len >= TCP_MSS_DEFAULT + sizeof(struct tcphdr) ||
 		    /* If PSH is not set, packet should be
 		     * full sized, provided peer TCP is not badly broken.
 		     * This observation (if it is correct 8)) allows

@@ -411,7 +411,7 @@ void tcp_initialize_rcv_mss(struct sock *sk)
 	unsigned int hint = min_t(unsigned int, tp->advmss, tp->mss_cache);
 
 	hint = min(hint, tp->rcv_wnd / 2);
-	hint = min(hint, TCP_MIN_RCVMSS);
+	hint = min(hint, TCP_MSS_DEFAULT);
 	hint = max(hint, TCP_MIN_MSS);
 
 	inet_csk(sk)->icsk_ack.rcv_mss = hint;

index f83ac91..0718fde 100644
@@ -217,7 +217,7 @@ int tcp_v4_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
 	if (inet->opt)
 		inet_csk(sk)->icsk_ext_hdr_len = inet->opt->optlen;
 
-	tp->rx_opt.mss_clamp = 536;
+	tp->rx_opt.mss_clamp = TCP_MSS_DEFAULT;
 
 	/* Socket identity is still unknown (sport may be zero).
 	 * However we set state to SYN-SENT and not releasing socket

@@ -1270,7 +1270,7 @@ int tcp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
 		goto drop_and_free;
 
 	tcp_clear_options(&tmp_opt);
-	tmp_opt.mss_clamp = 536;
+	tmp_opt.mss_clamp = TCP_MSS_DEFAULT;
 	tmp_opt.user_mss  = tcp_sk(sk)->rx_opt.user_mss;
 
 	tcp_parse_options(skb, &tmp_opt, 0, dst);

@@ -1818,7 +1818,7 @@ static int tcp_v4_init_sock(struct sock *sk)
 	 */
 	tp->snd_ssthresh = TCP_INFINITE_SSTHRESH;
 	tp->snd_cwnd_clamp = ~0;
-	tp->mss_cache = 536;
+	tp->mss_cache = TCP_MSS_DEFAULT;
 
 	tp->reordering = sysctl_tcp_reordering;
 	icsk->icsk_ca_ops = &tcp_init_congestion_ops;

index fb68bab..7a42990 100644
@@ -476,7 +476,7 @@ struct sock *tcp_create_openreq_child(struct sock *sk, struct request_sock *req,
 		if (newtp->af_specific->md5_lookup(sk, newsk))
 			newtp->tcp_header_len += TCPOLEN_MD5SIG_ALIGNED;
 #endif
-		if (skb->len >= TCP_MIN_RCVMSS+newtp->tcp_header_len)
+		if (skb->len >= TCP_MSS_DEFAULT + newtp->tcp_header_len)
 			newicsk->icsk_ack.last_seg_size = skb->len - newtp->tcp_header_len;
 		newtp->rx_opt.mss_clamp = req->mss;
 		TCP_ECN_openreq_child(newtp, req);

index 6951827..b528f75 100644
@@ -1849,7 +1849,7 @@ static int tcp_v6_init_sock(struct sock *sk)
 	 */
 	tp->snd_ssthresh = TCP_INFINITE_SSTHRESH;
 	tp->snd_cwnd_clamp = ~0;
-	tp->mss_cache = 536;
+	tp->mss_cache = TCP_MSS_DEFAULT;
 
 	tp->reordering = sysctl_tcp_reordering;
 

1.6.3.3

Define sysctl (tcp_cookie_size) to turn on and off the cookie option default globally, instead of a compiled configuration option.

Define per socket option (TCP_COOKIE_TRANSACTIONS) for setting constant data values, retrieving variable cookie values, and other facilities.

Redefine two TCP header functions to accept TCP header pointer. When subtracting, return signed int to allow error checking.

Move inline tcp_clear_options() unchanged from net/tcp.h to linux/tcp.h, near its corresponding struct tcp_options_received (prior to changes).

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

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

Requires:
TCPCT part 1b: TCP_MSS_DEFAULT, TCP_MSS_DESIRED

---
 Documentation/networking/ip-sysctl.txt |    8 ++++++
 include/linux/tcp.h                    |   44 +++++++++++++++++++++++++++++++-
 include/net/tcp.h                      |    6 +---
 net/ipv4/sysctl_net_ipv4.c             |    8 ++++++
 net/ipv4/tcp_output.c                  |    8 ++++++
 5 files changed, 68 insertions(+), 6 deletions(-)

index a0e134d..d47c000 100644
@@ -164,6 +164,14 @@ tcp_congestion_control - STRING
 	additional choices may be available based on kernel configuration.
 	Default is set as part of kernel configuration.
 
+tcp_cookie_size - INTEGER
+	Default size of TCP Cookie Transactions (TCPCT) option, that may be
+	overridden on a per socket basis by the TCPCT socket option.
+	Values greater than the maximum (16) are interpreted as the maximum.
+	Values greater than zero and less than the minimum (8) are interpreted
+	as the minimum.
+	Default: 0 (off).
+
 tcp_dsack - BOOLEAN
 	Allows TCP to send "duplicate" SACKs.
 

index a413e9f..f15924b 100644
@@ -234,6 +234,7 @@ extern int sysctl_tcp_base_mss;
 extern int sysctl_tcp_workaround_signed_windows;
 extern int sysctl_tcp_slow_start_after_idle;
 extern int sysctl_tcp_max_ssthresh;
+extern int sysctl_tcp_cookie_size;
 
 extern atomic_t tcp_memory_allocated;
 extern struct percpu_counter tcp_sockets_allocated;

@@ -340,11 +341,6 @@ static inline void tcp_dec_quickack_mode(struct sock *sk,
 
 extern void tcp_enter_quickack_mode(struct sock *sk);
 
-static inline void tcp_clear_options(struct tcp_options_received *rx_opt)
-{
- 	rx_opt->tstamp_ok = rx_opt->sack_ok = rx_opt->wscale_ok = rx_opt->snd_wscale = 0;
-}
-
 #define	TCP_ECN_OK		1
 #define	TCP_ECN_QUEUE_CWR	2
 #define	TCP_ECN_DEMAND_CWR	4

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),

index 784e6ee..492ccdd 100644
@@ -59,6 +59,14 @@ int sysctl_tcp_base_mss __read_mostly = 512;
 /* By default, RFC2861 behavior.  */
 int sysctl_tcp_slow_start_after_idle __read_mostly = 1;
 
+#ifdef CONFIG_SYSCTL
+/* By default, let the user enable it. */
+int sysctl_tcp_cookie_size __read_mostly = 0;
+#else
+int sysctl_tcp_cookie_size __read_mostly = TCP_COOKIE_MAX;
+#endif
+
+
 /* Account for new data that has been sent to the network. */
 static void tcp_event_new_data_sent(struct sock *sk, struct sk_buff *skb)
 {

1.6.3.3

Define (missing) hash message size for SHA1.

Define hashing size constants specific to TCP cookies, and add new function.

Maintain global secret values for tcp_cookie_generator().

This is a significantly revised implementation of earlier (15-year-old) Photuris [RFC-2522] code for the KA9Q cooperative multitasking platform.

Linux RCU technique appears to be well-suited to this application, though neither of the circular queue items are freed.

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

---
 include/linux/cryptohash.h |    1 +
 include/net/tcp.h          |    8 +++
 net/ipv4/tcp.c             |  146 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 155 insertions(+), 0 deletions(-)

index c118b2a..ec78a4b 100644
@@ -2,6 +2,7 @@
 #define __CRYPTOHASH_H
 
 #define SHA_DIGEST_WORDS 5
+#define SHA_MESSAGE_BYTES (512 /*bits*/ / 8)
 #define SHA_WORKSPACE_WORDS 80
 
 void sha_init(__u32 *buf);

index f15924b..702fd0a 100644
@@ -1474,6 +1474,14 @@ struct tcp_request_sock_ops {
 #endif
 };
 
+/* Using SHA1 for now, define some constants.
+ */
+#define COOKIE_DIGEST_WORDS (SHA_DIGEST_WORDS)
+#define COOKIE_MESSAGE_WORDS (SHA_MESSAGE_BYTES / 4)
+#define COOKIE_WORKSPACE_WORDS (COOKIE_DIGEST_WORDS + COOKIE_MESSAGE_WORDS)
+
+extern int tcp_cookie_generator(u32 *bakery);
+
 extern void tcp_v4_init(void);
 extern void tcp_init(void);
 

1.6.3.3

This is a significantly revised implementation of an earlier (year-old) patch that no longer applies cleanly, with permission of the original author (Adam Langley). That 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

Data structures are carefully composed to require minimal additions. For example, the struct tcp_options_received cookie_plus variable fits between existing 16-bit and 8-bit variables, requiring no additional space (taking alignment into consideration). There are no additions to tcp_request_sock, and only 1 pointer in tcp_sock.

Allocations have been rearranged to avoid requiring GFP_ATOMIC, with only one unavoidable exception in tcp_create_openreq_child(), where the tcp_sock itself is created GFP_ATOMIC.

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

Requires:
TCPCT part 1a: add request_values parameter for sending SYNACK
TCPCT part 1b: TCP_MSS_DEFAULT, TCP_MSS_DESIRED
TCPCT part 1c: sysctl_tcp_cookie_size, socket option TCP_COOKIE_TRANSACTIONS, functions TCPCT part 1d: generate Responder Cookie

---
 include/linux/tcp.h      |   29 ++++-
 include/net/tcp.h        |   72 +++++++++++++
 net/ipv4/syncookies.c    |    5 +-
 net/ipv4/tcp.c           |  127 ++++++++++++++++++++++-
 net/ipv4/tcp_input.c     |   86 +++++++++++++--
 net/ipv4/tcp_ipv4.c      |   69 ++++++++++++-
 net/ipv4/tcp_minisocks.c |   59 ++++++++---
 net/ipv4/tcp_output.c    |  255 +++++++++++++++++++++++++++++++++++++++++-----
 net/ipv6/syncookies.c    |    5 +-
 net/ipv6/tcp_ipv6.c      |   65 +++++++++++-
 10 files changed, 701 insertions(+), 71 deletions(-)

index 57921c1..80ebb52 100644
@@ -258,31 +258,38 @@ struct tcp_options_received {
 		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	*/
+	u8	cookie_plus:6,	/* bytes in authenticator/cookie option	*/
+		cookie_out_never:1,
+		cookie_in_always:1;
 	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 = rx_opt->wscale_ok = rx_opt->snd_wscale = 0;
+	rx_opt->tstamp_ok = rx_opt->sack_ok = 0;
+	rx_opt->wscale_ok = rx_opt->snd_wscale = 0;
+	rx_opt->cookie_plus = 0;
 }
 
 /* This is the max number of SACKS that we'll generate and process. It's safe
- * to increse this, although since:
+ * to increase 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_values;
+struct tcp_request_sock_ops;
+
 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;
 };
 
 static inline struct tcp_request_sock *tcp_rsk(const struct request_sock *req)

@@ -452,6 +459,12 @@ struct tcp_sock {
 /* TCP MD5 Signature Option information */
 	struct tcp_md5sig_info	*md5sig_info;
 #endif
+
+	/* When the cookie options are generated and exchanged, then this
+	 * object holds a reference to them (cookie_values->kref).  Also
+	 * contains related tcp_cookie_transactions fields.
+	 */
+	struct tcp_cookie_values  	*cookie_values;
 };
 
 static inline struct tcp_sock *tcp_sk(const struct sock *sk)

@@ -470,6 +483,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_values->kref)
+	 */
+	struct tcp_cookie_values  *tw_cookie_values;
 };
 
 static inline struct tcp_timewait_sock *tcp_twsk(const struct sock *sk)

index 3146cc4..3503762 100644
@@ -253,6 +253,8 @@ EXPORT_SYMBOL(cookie_check_timestamp);
 struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb,
 			     struct ip_options *opt)
 {
+	struct tcp_options_received tcp_opt;
+	u8 *cryptic_value;
 	struct inet_request_sock *ireq;
 	struct tcp_request_sock *treq;
 	struct tcp_sock *tp = tcp_sk(sk);

@@ -263,7 +265,6 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb,
 	int mss;
 	struct rtable *rt;
 	__u8 rcv_wscale;
-	struct tcp_options_received tcp_opt;
 
 	if (!sysctl_tcp_syncookies || !th->ack)
 		goto out;

@@ -341,7 +342,7 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb,
 
 	/* check for timestamp cookie support */
 	memset(&tcp_opt, 0, sizeof(tcp_opt));
-	tcp_parse_options(skb, &tcp_opt, 0, &rt->u.dst);
+	tcp_parse_options(skb, &tcp_opt, &cryptic_value, 0, &rt->u.dst);
 
 	if (tcp_opt.saw_tstamp)
 		cookie_check_timestamp(&tcp_opt);

index 612fc53..a60b934 100644
@@ -159,6 +159,8 @@ static inline int cookie_check(struct sk_buff *skb, __u32 cookie)
 
 struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
 {
+	struct tcp_options_received tcp_opt;
+	u8 *cryptic_value;
 	struct inet_request_sock *ireq;
 	struct inet6_request_sock *ireq6;
 	struct tcp_request_sock *treq;

@@ -171,7 +173,6 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
 	int mss;
 	struct dst_entry *dst;
 	__u8 rcv_wscale;
-	struct tcp_options_received tcp_opt;
 
 	if (!sysctl_tcp_syncookies || !th->ack)
 		goto out;

@@ -254,7 +255,7 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
 
 	/* check for timestamp cookie support */
 	memset(&tcp_opt, 0, sizeof(tcp_opt));
-	tcp_parse_options(skb, &tcp_opt, 0, dst);
+	tcp_parse_options(skb, &tcp_opt, &cryptic_value, 0, dst);
 
 	if (tcp_opt.saw_tstamp)
 		cookie_check_timestamp(&tcp_opt);

1.6.3.3

One general comment. ...This particular patch still has lots of noise which does not belong to the context of this change. ...Please try to minimize. Eg., if you don't like sizeof(struct tcphdr) but prefer sizeof(*th), you certainly don't have to do it in this particular patch! ...Also some comment changes which certainly are not mandatory nor even related.

 i.

I guess most of the cookie stuff have nothing to do with the next, please make them separate:

Convert the users here, not in the fifth patch to avoid noise in the large fifth patch.

And, I read more that fifth patch... seriously, please consider now to apply _all_ the coding style changes that have been brought to your attention by multiple people (you should know them now but for some reason again you choose to resent without complying -- I guess on purpose) into all upcoming patches you submit.

 i.

This *is* actually in CodingStyle (line 679), and I'm trying to conform:

<blockquote>
The preferred form for passing a size of a struct is the following:

	p = kmalloc(sizeof(*p), ...);

The alternative form where struct name is spelled out hurts readability and introduces an opportunity for a bug when the pointer variable type is changed but the corresponding sizeof that is passed to a memory allocator is not. </blockquote>

Maybe some of these anticipate part 2, so I can defer them to later. I've already coded much of part 2, so things are bleeding back and forth.

Hmmm....

1) The first is a hole left by the removal of the data fields some time ago, but they left the old (now incorrect) comment:

-/* SACKs data */
+ u8 cookie_plus:6, /* bytes in authenticator/cookie option */
+ cookie_out_never:1,
+ cookie_in_always:1;
u8 num_sacks; /* Number of SACK blocks */

Although it's not evident from the diff -u, I'm *filling* that hole, putting everything on a nice 32-bit boundary, thus taking *NO* space (already mentioned in my patch description above):

u16 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 */
u8 cookie_plus:6, /* bytes in authenticator/cookie option */
cookie_out_never:1,
cookie_in_always:1;
u8 num_sacks; /* Number of SACK blocks */
u16 user_mss; /* mss requested by user in ioctl */
u16 mss_clamp; /* Maximal mss, negotiated at connection setup */

The only reason that it's done this way was Miller's imperative that required cramming this into as small a space as possible.

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

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

Of course, it costs more, and I have to keep copying it from place to place, adding to the code complexity. But I was feeling rather clever to have found that hole!

2) The second is a spelling error that I noticed in passing. It's within the same diff -u area (close to other insertions both before and after), so fixing it was trivial:

- * to increse this, although since:
+ * to increase this, although since:

Are we not supposed to fix spelling errors in comments?

I skipped reading after this statement.... Please re-read what I said.

 i.

I really tried hard to understand what was going on, and failed, because I dont have much time these days...

Lack of documentation maybe ? Some DATA flow could help...

Please please, cook up elementatry patches to perform one action at a time, even if they are not fully functionnal ?

One patch to be able to send SYN + COOKIE (if we are the client)

One patch to be able to receive this SYN + COOKIE and answer a SYNACK + cookies (we are the server)

One patch to ... Receive the ACK from client (if we are the server) and check cookies

One patch to ... Send the ACK (if we are the client)

Patches to receive FIN + cookies

One patch to ... enable the whole thing and setsockopt() bits

That way we could review your patches step by step, and not 770 lines in one block.

After some time I returned to it. My apologies for my previous harsh response. My point was to say that you must separate such changes because they add to noise which makes it rather annoying to review. At least my brains are rather limited still in keeping track of different things.

Sure, fell free to but not in some random places like you now do in this patch. If you are not touching a line because of a real change and want do a syntax cleanup, please do it in another patch. Especially when your actual change is as complicated as this is.

I'm sorry but this response tells me that you don't seem to know even yourself what you were submitting. Does this ring a bell:

- if (th->doff == sizeof(struct tcphdr) >> 2) {
+ /* In the spirit of fast parsing, compare doff directly to shifted

+        * constant values.  Because equality is used, short doff can be
+        * ignored here, and checked later.
+        */

+ if (th->doff == (sizeof(*th) >> 2)) {

Besides, I don't understand even what you're trying to say. ...I think we already checked the length in tcp_v[46]_rcv against underflow. Why you add such a non-sense comment here (plus the sizeof change I covered elsewhere)? This is just a noise to annoy reviewer, nothing else.

How many extra lines a reviewer has to read because of that? Why not do it in a separate patch. git add -i / git stash would help you there. ...To be honest, I'd rather have typos in comments which like this are not trouble for the reader than clutter feature patches with all this random junk.

...I really wish that next time when you submit you get rid of all these showstoppers and annoyances (ie., at least honestly try to do your best) that we could finally concentrate to the actual change and reviewing that :-). My recommendation is that before hitting the send button you review (read through) your own patches, and if you find something to correct instead of submitting, postpone that until the problem is solved. Like DaveM once noted somebody, you'll not be timed :-). ...I usually do that (read my own patches) and end up rather often to not submit (and find even real bugs that way quite often).

 i.

These comments are directly contrary to advice back on the 2nd or 3rd round, where I'd both converted and removed the old functions here, before going on to use them everywhere. I was told to *only* define the new ones here, use them in later patches as they applied, and then remove the old functions in a final cleanup patch.

When I'm done (apparently very far in the distant future), there will be approximately 100 (maybe 150 or more) uses.

Pardon me, but I'm having difficulty finding a substantive comment. Are you attempting humor or sarcasm? If so, it's not readily apparent. It seems more of an /ad hominem/ attack to me.

There were many painstaking hours of coding style changes, every single one of them had to be individually examined side by side for correctness after I ran a series of regexp over the patches -- in 148 blocks (I just checked) of diff between the patch diffs, comprising eye-glazing trivial changes (although I used BBEdit's very nice GUI utility for the side-by-side comparison).

If you don't like something else, please be specific (as others did privately). There may be places the regexp missed.

But more importantly, I'd prefer actual analysis. Is there a better Linux function to use for something (like RCU that Eric recommended)?

This is the *easy* patch series, based on code previously reviewed. The next series are much more conceptually difficult. Maybe that's the problem, this is too easy, and so the only things people find to comment about are trivia.

Please, more thoughtful code review, less interpersonal sniping.

TIA.

Thanks very much for your time.

As I just wrote in a previous message, this is the easy part. It's going to get much more complicated -- as you know, since I sent you the papers with 30+ references....

Where should that be? In the commit messages?

Well, that's what I did with the (now) parts 1b, 1c, and 1d, but somebody complained that the callers didn't exist yet.

I'll split them. Remember, I started with a code base previously submitted, and everything was in one *single* large patch. I thought that's what you (Linux folks in general) wanted.

Maybe that's what you are missing, and why you're have difficulty wrapping your mind around it. None of that is there yet!

Receive the Ack options from the client is currently part 2. I've also lately divided part 2 into 4 sub-patches, although it all has to be committed at one time:
2a) extending the TCP option space,
2b) receiving 64-bit timestamps,
2c) receiving extended SACKs,
2d) receiving the cookie pair.
Perhaps that could be split even further, although the order has to stay the same. A lot is written, although I gave up updating regularly as the part 1 process has taken so long....

Send the Ack is currently part 3.

Handling FIN is currently part 4, although it might be split, too.

Handling RST is probably part 5.

Handling transactions was part 4 or 5, but could be a future part 6 instead (although it's rather tied up with both 4 and 5).

Ummm, then there'd be no way to test. This is TCP we're talking about. It has to be tested extensively. This stuff needs to work. I need the bits to initiate all the tests....

That's why I like to get all my data structures nailed down first, and then incrementally add code and test.

Apology accepted.

I'd think we agree, and that's why I wouldn't go around fixing all the misspelled comments -- just those in the code I'm actually revising.

Not a single one is in a random place.

You merely think? You don't know? (I didn't find one on the code walk through that got me to this point in the code, but perhaps I missed something.) If that's the case, there should at least be a comment that underflow was already checked, and *where* it was checked!

The lack of pertinent comments and error checking annoys the next coder that comes down the pike.

I'll have to look up the usage of git add -i / git stash, but in any case, I'm not likely to go around fixing comments later.

Each patch requires a great deal of time. I'd rather combine such things, or not do them at all.

Maybe you've been dealing too much with some folks that submitted a patch that was accepted (over my expressed reservations), and then had to send out 2 emergency patches to handle crashing bugs last week....

Before I send any patch, it's been compared against my last patch sent, *and* tested for compilation individually, *and* _usually_ tested pretty thoroughly -- although not this time, because the net-next-2.6 modules don't compile, as I warned at the head of this patch series.

Because the compile blew up, I actually spent my Sunday evening re-reviewing the patches again! I wanted to ensure it wasn't something that I'd done, so I recompiled everything again incrementally.

It took me more than a day to make the syntax changes requested, and another 6 hours to prepare and send these 5 patches.

I cannot promise there will never be any bugs, this patch series will be much too complicated for that, but the very idea that I'd send without reviewing.... At this point, I'm rather personally insulted.

And now I'm done with email for the day. I'll do the split that Eric suggested, and send it out whenever the damn modules compile again.

In the meantime, I'd appreciate that you "finally concentrate"....

I guess there's misunderstanding here. Yes, if you bring in new helpers that solely are used with your feature, then it that feature patch (and I found DaveM commenting specifically this point I think), or right beforehand with a clear note that it will be used in the subsequent patch. But if there are uses in the existing code without any of your changes, add the helpers and convert existing callers in a single go without doing some other things (preferrably convert all callers, not just those you intend to touch -- there is nothing extraordinary in this, other people do the same kind of cleaning up work while they go, so please don't find that this is something we try to force specifically for you :-)). ...Therefore each change becomes rather self-sufficient to understand and review. There was serious problems in your series in this as I constantly had to jump between 3 patches to see what you said (changed) in the other. But also, please don't make this an over-statement, I'd still occassionally have to do that "patch hopping" even if nicely split (so this is not something to be turned into all in a single patch statement :-)).

On positive side, there certainly has been clear progress already, as patches 1 and 2 were quite nice already (except for some weird local initializer order change which I could sort of "tolerate" but I didn't find any particular reason why those had to be switched around.

And, one thing to not be afraid is making many short patches (nor long one either). So if a self-contained change is a small one do it such and no more. ...It is often so that you will be doing many relatively small changes first to support the actual large one, and those small changes might actually not end up being that small in lines if there are e.g. multiple callsites converted but nevertheless they often are rather simple, so that a quick browsing will already be enough to reveal that it is ok (like in the MSS define change you made).

Only if the large patch is only for a single logical change. A syntax cleanup, literal cleanup, cleanup to use helpers and a feature patch are not logically same changes, and thus should be separated. It's like you have already being doing for a part of the changes. Many times the cleanup things are even such that they would be beneficial even without any feature addition (but nobody has just done them so far).

 i.

I disagree.

Sure, I'll find that out from your patches instead, and it will be more definitive than what the specs say ;-).

Let me say it out loud then. I know that among the very first things we check is this underflow check. I quoted is below.

You added a comment which says neither but something else? :-D I wonder how trustworthy such comment then would be to that poor "next coder" that comes along ;-).

This is something I can hardly understand... these are among the first things we check before we even begin processing a TCP segment:

        if (!pskb_may_pull(skb, sizeof(struct tcphdr)))
                goto discard_it;

        th = tcp_hdr(skb);

        if (th->doff < sizeof(struct tcphdr) / 4)
                goto bad_packet;
        if (!pskb_may_pull(skb, th->doff * 4))
                goto discard_it;

With git add -i you can change them while you go but easily split them on before commit time to multiple patches. Rather easy unless except if you you're also modifying the line right next or before to the comment it.

 i.

If I understand correctly, in that case I look forward to your Ack for part 1d, a self-contained "helper" feature patch "with a clear note".