[PATCH 0/7] tcp: bugs and cleanup for 2.6.34-rc1

[PATCH 0/7] tcp: bugs and cleanup for 2.6.34-rc1

[William Allen Simpson]

I'd have thought that there would be greater interest about patching
crashing bugs, signed versus unsigned (underflow) bugs, TCP DoS bugs,
TCP data corruption, and TCP performance problems....

There's been ample warning.  Zero-day security issues will be reported
to the usual announcement lists.  In particular, these 0day exploits
affect systems as far back as the 2005 changeover to git.

Combination of patches reported in October, November, December, January,
and February, for 2.6.32, 2.6.33, and now 2.6.34.

This code has had previous review and several months of limited testing.

Some portions were removed during the various TCPCT part 1 patch splits,
then were cut off by the sudden unexpected end of that merge window.
[03 Dec 2009]  I've restarted the sub-numbering (again).

Of particular interest are the TCPCT header extensions that already
appear in the next phase of testing with other platforms.  These patches
allow correct reception without data corruption.

The remainder of the original TCPCT part 2 will be merged with part 3.

[Updated to 2010 Mar 08 2.6.34-rc1.]

[PATCH v3 1/7] net: tcp_header_len_th and tcp_option_len_th

[William Allen Simpson]

[-- Attachment #1: Type: text/plain, Size: 359 bytes --]

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

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

Signed-off-by: William.Allen.Simpson@gmail.com
---
  include/linux/tcp.h |   12 ++++++++++++
  1 files changed, 12 insertions(+), 0 deletions(-)

[-- Attachment #2: len_th+2a3+2.6.34-rc1.patch --]
[-- Type: text/plain, Size: 719 bytes --]

diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index a778ee0..48ddeb8 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -225,6 +225,18 @@ static inline unsigned int tcp_optlen(const struct sk_buff *skb)
 	return (tcp_hdr(skb)->doff - 5) * 4;
 }
 
+/* Length of fixed header plus standard options. */
+static inline unsigned int tcp_header_len_th(const struct tcphdr *th)
+{
+	return th->doff * 4;
+}
+
+/* Length of standard options only.  This could be negative. */
+static inline int tcp_option_len_th(const struct tcphdr *th)
+{
+	return (int)(th->doff * 4) - sizeof(*th);
+}
+
 /* This defines a selective acknowledgement block. */
 struct tcp_sack_block_wire {
 	__be32	start_seq;
-- 
1.6.3.3

[PATCH v4 2/7] net: remove old tcp_optlen function

[William Allen Simpson]

The tcp_optlen() function returns a potential *negative* unsigned.

In the only two existing files using the old tcp_optlen() function,
clean up confusing and inconsistent mixing of both byte and word
offsets, and other coding style issues.  Document assumptions.

Quoth David Miller:
    This is transmit, and the packets can only come from the Linux
    TCP stack, not some external entity.

    You're being way too anal here, and adding these checks to
    drivers would be just a lot of rediculious bloat. [sic]

Therefore, there are *no* checks for bad TCP and IP header sizes, nor
any semantic changes.  The drivers should function exactly as existing,
although usage of int should ameliorate the issues.

No response from testers in 21+ weeks.

[removed comment references to commit log]

Requires:
   net: tcp_header_len_th and tcp_option_len_th

Signed-off-by: William.Allen.Simpson@gmail.com
CC: Michael Chan <mchan@broadcom.com>
---
  drivers/net/bnx2.c  |   29 +++++++++++++-----------
  drivers/net/tg3.c   |   60 +++++++++++++++++++++++---------------------------
  include/linux/tcp.h |    5 ----
  3 files changed, 44 insertions(+), 50 deletions(-)

Re: [PATCH v4 2/7] net: remove old tcp_optlen function

[William Allen Simpson]

[-- Attachment #1: Type: text/plain, Size: 1166 bytes --]

The tcp_optlen() function returns a potential *negative* unsigned.

In the only two existing files using the old tcp_optlen() function,
clean up confusing and inconsistent mixing of both byte and word
offsets, and other coding style issues.  Document assumptions.

Quoth David Miller:
    This is transmit, and the packets can only come from the Linux
    TCP stack, not some external entity.

    You're being way too anal here, and adding these checks to
    drivers would be just a lot of rediculious bloat. [sic]

Therefore, there are *no* checks for bad TCP and IP header sizes, nor
any semantic changes.  The drivers should function exactly as existing,
although usage of int should ameliorate the issues.

No response from testers in 21+ weeks.

[removed comment references to commit log]

Requires:
   net: tcp_header_len_th and tcp_option_len_th

Signed-off-by: William.Allen.Simpson@gmail.com
CC: Michael Chan <mchan@broadcom.com>
---
  drivers/net/bnx2.c  |   29 +++++++++++++-----------
  drivers/net/tg3.c   |   60 +++++++++++++++++++++++---------------------------
  include/linux/tcp.h |    5 ----
  3 files changed, 44 insertions(+), 50 deletions(-)

[-- Attachment #2: len_th+2b4+2.6.34-rc1.patch --]
[-- Type: text/plain, Size: 6918 bytes --]

diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
index 381887b..87607b8 100644
--- a/drivers/net/bnx2.c
+++ b/drivers/net/bnx2.c
@@ -6335,6 +6335,8 @@ bnx2_vlan_rx_register(struct net_device *dev, struct vlan_group *vlgrp)
 /* Called with netif_tx_lock.
  * bnx2_tx_int() runs without netif_tx_lock unless it needs to call
  * netif_wake_queue().
+ *
+ * No TCP or IP length checking, as required by David Miller.
  */
 static netdev_tx_t
 bnx2_start_xmit(struct sk_buff *skb, struct net_device *dev)
@@ -6378,19 +6380,19 @@ bnx2_start_xmit(struct sk_buff *skb, struct net_device *dev)
 			(TX_BD_FLAGS_VLAN_TAG | (vlan_tx_tag_get(skb) << 16));
 	}
 #endif
-	if ((mss = skb_shinfo(skb)->gso_size)) {
-		u32 tcp_opt_len;
-		struct iphdr *iph;
+	mss = skb_shinfo(skb)->gso_size;
+	if (mss != 0) {
+		struct tcphdr *th = tcp_hdr(skb);
+		int tcp_opt_words = th->doff - (sizeof(*th) >> 2);
+		/* assumes positive tcp_opt_words without checking */
 
 		vlan_tag_flags |= TX_BD_FLAGS_SW_LSO;
 
-		tcp_opt_len = tcp_optlen(skb);
-
 		if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV6) {
 			u32 tcp_off = skb_transport_offset(skb) -
 				      sizeof(struct ipv6hdr) - ETH_HLEN;
 
-			vlan_tag_flags |= ((tcp_opt_len >> 2) << 8) |
+			vlan_tag_flags |= (tcp_opt_words << 8) |
 					  TX_BD_FLAGS_SW_FLAGS;
 			if (likely(tcp_off == 0))
 				vlan_tag_flags &= ~TX_BD_FLAGS_TCP6_OFF0_MSK;
@@ -6403,14 +6405,15 @@ bnx2_start_xmit(struct sk_buff *skb, struct net_device *dev)
 				mss |= (tcp_off & 0xc) << TX_BD_TCP6_OFF2_SHL;
 			}
 		} else {
-			iph = ip_hdr(skb);
-			if (tcp_opt_len || (iph->ihl > 5)) {
-				vlan_tag_flags |= ((iph->ihl - 5) +
-						   (tcp_opt_len >> 2)) << 8;
-			}
+			struct iphdr *iph = ip_hdr(skb);
+			int ip_opt_words = iph->ihl - (sizeof(*iph) >> 2);
+			/* assumes positive ip_opt_words without checking */
+			int opt_words = ip_opt_words + tcp_opt_words;
+
+			if (opt_words > 0)
+				vlan_tag_flags |= opt_words << 8;
 		}
-	} else
-		mss = 0;
+	}
 
 	mapping = pci_map_single(bp->pdev, skb->data, len, PCI_DMA_TODEVICE);
 	if (pci_dma_mapping_error(bp->pdev, mapping)) {
diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index 0fa7688..6ad8184 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -5481,6 +5481,8 @@ static void tg3_set_txd(struct tg3_napi *tnapi, int entry,
 
 /* hard_start_xmit for devices that don't have any bugs and
  * support TG3_FLG2_HW_TSO_2 and TG3_FLG2_HW_TSO_3 only.
+ *
+ * No TCP or IP length checking, as required by David Miller.
  */
 static netdev_tx_t tg3_start_xmit(struct sk_buff *skb,
 				  struct net_device *dev)
@@ -5515,9 +5517,9 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb,
 
 	entry = tnapi->tx_prod;
 	base_flags = 0;
-	mss = 0;
-	if ((mss = skb_shinfo(skb)->gso_size) != 0) {
-		int tcp_opt_len, ip_tcp_len;
+	mss = skb_shinfo(skb)->gso_size;
+	if (mss != 0) {
+		struct tcphdr *th;
 		u32 hdrlen;
 
 		if (skb_header_cloned(skb) &&
@@ -5525,18 +5527,16 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb,
 			dev_kfree_skb(skb);
 			goto out_unlock;
 		}
+		th = tcp_hdr(skb);
 
 		if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV6)
 			hdrlen = skb_headlen(skb) - ETH_HLEN;
 		else {
 			struct iphdr *iph = ip_hdr(skb);
 
-			tcp_opt_len = tcp_optlen(skb);
-			ip_tcp_len = ip_hdrlen(skb) + sizeof(struct tcphdr);
-
+			hdrlen = ip_hdrlen(skb) + tcp_header_len_th(th);
+			iph->tot_len = htons(mss + hdrlen);
 			iph->check = 0;
-			iph->tot_len = htons(mss + ip_tcp_len + tcp_opt_len);
-			hdrlen = ip_tcp_len + tcp_opt_len;
 		}
 
 		if (tp->tg3_flags2 & TG3_FLG2_HW_TSO_3) {
@@ -5550,7 +5550,7 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb,
 		base_flags |= (TXD_FLAG_CPU_PRE_DMA |
 			       TXD_FLAG_CPU_POST_DMA);
 
-		tcp_hdr(skb)->check = 0;
+		th->check = 0;
 
 	}
 	else if (skb->ip_summed == CHECKSUM_PARTIAL)
@@ -5683,6 +5683,8 @@ tg3_tso_bug_end:
 
 /* hard_start_xmit for devices that have the 4G bug and/or 40-bit bug and
  * support TG3_FLG2_HW_TSO_1 or firmware TSO only.
+ *
+ * No TCP or IP length checking, as required by David Miller.
  */
 static netdev_tx_t tg3_start_xmit_dma_bug(struct sk_buff *skb,
 					  struct net_device *dev)
@@ -5721,20 +5723,21 @@ static netdev_tx_t tg3_start_xmit_dma_bug(struct sk_buff *skb,
 	if (skb->ip_summed == CHECKSUM_PARTIAL)
 		base_flags |= TXD_FLAG_TCPUDP_CSUM;
 
-	if ((mss = skb_shinfo(skb)->gso_size) != 0) {
+	mss = skb_shinfo(skb)->gso_size;
+	if (mss != 0) {
 		struct iphdr *iph;
-		u32 tcp_opt_len, ip_tcp_len, hdr_len;
+		struct tcphdr *th;
+		u32 hdr_len;
+		int opt_bytes;
 
 		if (skb_header_cloned(skb) &&
 		    pskb_expand_head(skb, 0, 0, GFP_ATOMIC)) {
 			dev_kfree_skb(skb);
 			goto out_unlock;
 		}
+		th = tcp_hdr(skb);
+		hdr_len = ip_hdrlen(skb) + tcp_header_len_th(th);
 
-		tcp_opt_len = tcp_optlen(skb);
-		ip_tcp_len = ip_hdrlen(skb) + sizeof(struct tcphdr);
-
-		hdr_len = ip_tcp_len + tcp_opt_len;
 		if (unlikely((ETH_HLEN + hdr_len) > 80) &&
 			     (tp->tg3_flags2 & TG3_FLG2_TSO_BUG))
 			return (tg3_tso_bug(tp, skb));
@@ -5746,13 +5749,14 @@ static netdev_tx_t tg3_start_xmit_dma_bug(struct sk_buff *skb,
 		iph->check = 0;
 		iph->tot_len = htons(mss + hdr_len);
 		if (tp->tg3_flags2 & TG3_FLG2_HW_TSO) {
-			tcp_hdr(skb)->check = 0;
+			th->check = 0;
 			base_flags &= ~TXD_FLAG_TCPUDP_CSUM;
 		} else
-			tcp_hdr(skb)->check = ~csum_tcpudp_magic(iph->saddr,
-								 iph->daddr, 0,
-								 IPPROTO_TCP,
-								 0);
+			th->check = ~csum_tcpudp_magic(iph->saddr, iph->daddr,
+						       0, IPPROTO_TCP, 0);
+
+		opt_bytes = hdr_len - sizeof(*iph) - sizeof(*th);
+		/* assumes positive opt_bytes without checking */
 
 		if (tp->tg3_flags2 & TG3_FLG2_HW_TSO_3) {
 			mss |= (hdr_len & 0xc) << 12;
@@ -5763,19 +5767,11 @@ static netdev_tx_t tg3_start_xmit_dma_bug(struct sk_buff *skb,
 			mss |= hdr_len << 9;
 		else if ((tp->tg3_flags2 & TG3_FLG2_HW_TSO_1) ||
 			 GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5705) {
-			if (tcp_opt_len || iph->ihl > 5) {
-				int tsflags;
-
-				tsflags = (iph->ihl - 5) + (tcp_opt_len >> 2);
-				mss |= (tsflags << 11);
-			}
+			if (opt_bytes > 0)
+				mss |= opt_bytes << (11 - 2);
 		} else {
-			if (tcp_opt_len || iph->ihl > 5) {
-				int tsflags;
-
-				tsflags = (iph->ihl - 5) + (tcp_opt_len >> 2);
-				base_flags |= tsflags << 12;
-			}
+			if (opt_bytes > 0)
+				base_flags |= opt_bytes << (12 - 2);
 		}
 	}
 #if TG3_VLAN_TAG_USED
diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index 48ddeb8..854ad65 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -220,11 +220,6 @@ static inline unsigned int tcp_hdrlen(const struct sk_buff *skb)
 	return tcp_hdr(skb)->doff * 4;
 }
 
-static inline unsigned int tcp_optlen(const struct sk_buff *skb)
-{
-	return (tcp_hdr(skb)->doff - 5) * 4;
-}
-
 /* Length of fixed header plus standard options. */
 static inline unsigned int tcp_header_len_th(const struct tcphdr *th)
 {
-- 
1.6.3.3

[PATCH v6 3/7] tcp: harmonize tcp_vx_rcv header length assumptions

[William Allen Simpson]

[-- Attachment #1: Type: text/plain, Size: 1086 bytes --]

Harmonize tcp_v4_rcv() and tcp_v6_rcv() -- better document tcp doff
and header length assumptions, and carefully compare implementations.

Reduces multiply/shifts, marginally improving speed.

Removes redundant tcp header length checks before checksumming.

Instead, assumes (and documents) that any backlog processing and
transform policies will carefully preserve the header, and will
ensure the socket buffer length remains >= the header size.

Quoth David Miller:
   Not true.

   The skb->len can be modified by the call to sk_filter() done
   by tcp_v4_rcv().

Therefore, added extra redundant checks for any sk_filter() problems,
also in tcp_v6_rcv().

[updated comments, resolved conflicts]

Stand-alone patch, originally developed for TCPCT.

Signed-off-by: William.Allen.Simpson@gmail.com
Reviewed-by: Andi Kleen <andi@firstfloor.org>
---
  include/net/xfrm.h  |    7 +++++
  net/ipv4/tcp_ipv4.c |   57 ++++++++++++++++++++++++---------------
  net/ipv6/tcp_ipv6.c |   72 +++++++++++++++++++++++++++++++-------------------
  3 files changed, 87 insertions(+), 49 deletions(-)

[-- Attachment #2: len_th+2c6+2.6.34-rc1.patch --]
[-- Type: text/plain, Size: 9288 bytes --]

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index a7df327..f61c44d 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -977,6 +977,13 @@ xfrm_state_addr_cmp(struct xfrm_tmpl *tmpl, struct xfrm_state *x, unsigned short
 }
 
 #ifdef CONFIG_XFRM
+/*
+ * For transport, the policy is checked before the presumed more expensive
+ * checksum. The transport header has already been checked for size, and is
+ * guaranteed to be contiguous. These policies must not alter the header or
+ * its position in the buffer, and should not shorten the buffer length
+ * without ensuring the length remains >= the header size.
+ */
 extern int __xfrm_policy_check(struct sock *, int dir, struct sk_buff *skb, unsigned short family);
 
 static inline int __xfrm_policy_check2(struct sock *sk, int dir,
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index c3588b4..c206024 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1561,7 +1561,8 @@ int tcp_v4_do_rcv(struct sock *sk, struct sk_buff *skb)
 		return 0;
 	}
 
-	if (skb->len < tcp_hdrlen(skb) || tcp_checksum_complete(skb))
+	/* Assumes skb->len includes header and options */
+	if (tcp_checksum_complete(skb))
 		goto csum_err;
 
 	if (sk->sk_state == TCP_LISTEN) {
@@ -1603,14 +1604,13 @@ csum_err:
 }
 
 /*
- *	From tcp_input.c
+ *	Called by ip_input.c: ip_local_deliver_finish()
  */
-
 int tcp_v4_rcv(struct sk_buff *skb)
 {
-	const struct iphdr *iph;
 	struct tcphdr *th;
 	struct sock *sk;
+	int tcp_header_len;
 	int ret;
 	struct net *net = dev_net(skb->dev);
 
@@ -1620,38 +1620,40 @@ int tcp_v4_rcv(struct sk_buff *skb)
 	/* Count it even if it's bad */
 	TCP_INC_STATS_BH(net, TCP_MIB_INSEGS);
 
+	/* Check too short header */
 	if (!pskb_may_pull(skb, sizeof(struct tcphdr)))
 		goto discard_it;
 
-	th = tcp_hdr(skb);
-
-	if (th->doff < sizeof(struct tcphdr) / 4)
+	/* Check bad doff, compare doff directly to constant value */
+	tcp_header_len = tcp_hdr(skb)->doff;
+	if (tcp_header_len < (sizeof(struct tcphdr) / 4))
 		goto bad_packet;
-	if (!pskb_may_pull(skb, th->doff * 4))
+
+	/* Check too short header and options */
+	tcp_header_len *= 4;
+	if (!pskb_may_pull(skb, tcp_header_len))
 		goto discard_it;
 
-	/* An explanation is required here, I think.
-	 * Packet length and doff are validated by header prediction,
-	 * provided case of th->doff==0 is eliminated.
-	 * So, we defer the checks. */
+	/* Packet length and doff are validated by header prediction,
+	 * provided case of th->doff == 0 is eliminated (above).
+	 */
 	if (!skb_csum_unnecessary(skb) && tcp_v4_checksum_init(skb))
 		goto bad_packet;
 
 	th = tcp_hdr(skb);
-	iph = ip_hdr(skb);
 	TCP_SKB_CB(skb)->seq = ntohl(th->seq);
 	TCP_SKB_CB(skb)->end_seq = (TCP_SKB_CB(skb)->seq + th->syn + th->fin +
-				    skb->len - th->doff * 4);
+				    skb->len - tcp_header_len);
 	TCP_SKB_CB(skb)->ack_seq = ntohl(th->ack_seq);
 	TCP_SKB_CB(skb)->when	 = 0;
-	TCP_SKB_CB(skb)->flags	 = iph->tos;
+	TCP_SKB_CB(skb)->flags	 = ip_hdr(skb)->tos;
 	TCP_SKB_CB(skb)->sacked	 = 0;
 
 	sk = __inet_lookup_skb(&tcp_hashinfo, skb, th->source, th->dest);
 	if (!sk)
 		goto no_tcp_socket;
 
-	if (iph->ttl < inet_sk(sk)->min_ttl)
+	if (ip_hdr(skb)->ttl < inet_sk(sk)->min_ttl)
 		goto discard_and_relse;
 
 process:
@@ -1660,9 +1662,17 @@ process:
 
 	if (!xfrm4_policy_check(sk, XFRM_POLICY_IN, skb))
 		goto discard_and_relse;
+
+	/* Sadly, there's currently no quick check for filters. According to
+	 * David Miller, filters are permitted to reduce skb->len past the
+	 * existing header and options. So, all the length checks above must be
+	 * painfully repeated here. --WAS
+	 */
 	nf_reset(skb);
 
-	if (sk_filter(sk, skb))
+	if (sk_filter(sk, skb) ||
+	    sizeof(struct tcphdr) > skb->len ||
+	    tcp_hdrlen(skb) > skb->len)
 		goto discard_and_relse;
 
 	skb->dev = NULL;
@@ -1687,14 +1697,14 @@ process:
 	bh_unlock_sock(sk);
 
 	sock_put(sk);
-
 	return ret;
 
 no_tcp_socket:
 	if (!xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb))
 		goto discard_it;
 
-	if (skb->len < (th->doff << 2) || tcp_checksum_complete(skb)) {
+	/* Assumes skb->len includes header and options */
+	if (tcp_checksum_complete(skb)) {
 bad_packet:
 		TCP_INC_STATS_BH(net, TCP_MIB_INERRS);
 	} else {
@@ -1716,18 +1726,21 @@ do_time_wait:
 		goto discard_it;
 	}
 
-	if (skb->len < (th->doff << 2) || tcp_checksum_complete(skb)) {
+	/* Assumes skb->len includes header and options */
+	if (tcp_checksum_complete(skb)) {
 		TCP_INC_STATS_BH(net, TCP_MIB_INERRS);
 		inet_twsk_put(inet_twsk(sk));
 		goto discard_it;
 	}
+
 	switch (tcp_timewait_state_process(inet_twsk(sk), skb, th)) {
 	case TCP_TW_SYN: {
 		struct sock *sk2 = inet_lookup_listener(dev_net(skb->dev),
 							&tcp_hashinfo,
-							iph->daddr, th->dest,
+							ip_hdr(skb)->daddr,
+							th->dest,
 							inet_iif(skb));
-		if (sk2) {
+		if (sk2 != NULL) {
 			inet_twsk_deschedule(inet_twsk(sk), &tcp_death_row);
 			inet_twsk_put(inet_twsk(sk));
 			sk = sk2;
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 6963a6b..04814bf 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1602,7 +1602,8 @@ static int tcp_v6_do_rcv(struct sock *sk, struct sk_buff *skb)
 		return 0;
 	}
 
-	if (skb->len < tcp_hdrlen(skb) || tcp_checksum_complete(skb))
+	/* Assumes skb->len includes header and options */
+	if (tcp_checksum_complete(skb))
 		goto csum_err;
 
 	if (sk->sk_state == TCP_LISTEN) {
@@ -1672,38 +1673,47 @@ ipv6_pktoptions:
 	return 0;
 }
 
+/*
+ *	Called by ip6_input.c: ip6_input_finish()
+ */
 static int tcp_v6_rcv(struct sk_buff *skb)
 {
 	struct tcphdr *th;
 	struct sock *sk;
+	int tcp_header_len;
 	int ret;
 	struct net *net = dev_net(skb->dev);
 
 	if (skb->pkt_type != PACKET_HOST)
 		goto discard_it;
 
-	/*
-	 *	Count it even if it's bad.
-	 */
+	/* Count it even if it's bad */
 	TCP_INC_STATS_BH(net, TCP_MIB_INSEGS);
 
+	/* Check too short header */
 	if (!pskb_may_pull(skb, sizeof(struct tcphdr)))
 		goto discard_it;
 
-	th = tcp_hdr(skb);
-
-	if (th->doff < sizeof(struct tcphdr)/4)
+	/* Check bad doff, compare doff directly to constant value */
+	tcp_header_len = tcp_hdr(skb)->doff;
+	if (tcp_header_len < (sizeof(struct tcphdr) / 4))
 		goto bad_packet;
-	if (!pskb_may_pull(skb, th->doff*4))
+
+	/* Check too short header and options */
+	tcp_header_len *= 4;
+	if (!pskb_may_pull(skb, tcp_header_len))
 		goto discard_it;
 
+	/* Packet length and doff are validated by header prediction,
+	 * provided case of th->doff == 0 is eliminated (above).
+	 */
 	if (!skb_csum_unnecessary(skb) && tcp_v6_checksum_init(skb))
 		goto bad_packet;
 
 	th = tcp_hdr(skb);
 	TCP_SKB_CB(skb)->seq = ntohl(th->seq);
 	TCP_SKB_CB(skb)->end_seq = (TCP_SKB_CB(skb)->seq + th->syn + th->fin +
-				    skb->len - th->doff*4);
+				    skb->len - tcp_header_len);
 	TCP_SKB_CB(skb)->ack_seq = ntohl(th->ack_seq);
 	TCP_SKB_CB(skb)->when = 0;
 	TCP_SKB_CB(skb)->flags = ipv6_get_dsfield(ipv6_hdr(skb));
@@ -1713,6 +1723,9 @@ static int tcp_v6_rcv(struct sk_buff *skb)
 	if (!sk)
 		goto no_tcp_socket;
 
+	/* Unlike tcp_v4_rcv(), no min_ttl check???
+	 */
+
 process:
 	if (sk->sk_state == TCP_TIME_WAIT)
 		goto do_time_wait;
@@ -1720,7 +1733,16 @@ process:
 	if (!xfrm6_policy_check(sk, XFRM_POLICY_IN, skb))
 		goto discard_and_relse;
 
-	if (sk_filter(sk, skb))
+	/* Sadly, there's currently no quick check for filters. According to
+	 * David Miller, filters are permitted to reduce skb->len past the
+	 * existing header and options. So, all the length checks above must be
+	 * painfully repeated here. --WAS
+	 *
+	 * nf_reset(skb); in ip6_input.c ip6_input_finish()
+	 */
+	if (sk_filter(sk, skb) ||
+	    sizeof(struct tcphdr) > skb->len ||
+	    tcp_hdrlen(skb) > skb->len)
 		goto discard_and_relse;
 
 	skb->dev = NULL;
@@ -1751,7 +1773,8 @@ no_tcp_socket:
 	if (!xfrm6_policy_check(NULL, XFRM_POLICY_IN, skb))
 		goto discard_it;
 
-	if (skb->len < (th->doff<<2) || tcp_checksum_complete(skb)) {
+	/* Assumes skb->len includes header and options */
+	if (tcp_checksum_complete(skb)) {
 bad_packet:
 		TCP_INC_STATS_BH(net, TCP_MIB_INERRS);
 	} else {
@@ -1759,11 +1782,7 @@ bad_packet:
 	}
 
 discard_it:
-
-	/*
-	 *	Discard frame
-	 */
-
+	/* Discard frame. */
 	kfree_skb(skb);
 	return 0;
 
@@ -1777,24 +1796,23 @@ do_time_wait:
 		goto discard_it;
 	}
 
-	if (skb->len < (th->doff<<2) || tcp_checksum_complete(skb)) {
+	/* Assumes skb->len includes header and options */
+	if (tcp_checksum_complete(skb)) {
 		TCP_INC_STATS_BH(net, TCP_MIB_INERRS);
 		inet_twsk_put(inet_twsk(sk));
 		goto discard_it;
 	}
 
 	switch (tcp_timewait_state_process(inet_twsk(sk), skb, th)) {
-	case TCP_TW_SYN:
-	{
-		struct sock *sk2;
-
-		sk2 = inet6_lookup_listener(dev_net(skb->dev), &tcp_hashinfo,
-					    &ipv6_hdr(skb)->daddr,
-					    ntohs(th->dest), inet6_iif(skb));
+	case TCP_TW_SYN: {
+		struct sock *sk2 = inet6_lookup_listener(dev_net(skb->dev),
+							 &tcp_hashinfo,
+							 &ipv6_hdr(skb)->daddr,
+							 ntohs(th->dest),
+							 inet6_iif(skb));
 		if (sk2 != NULL) {
-			struct inet_timewait_sock *tw = inet_twsk(sk);
-			inet_twsk_deschedule(tw, &tcp_death_row);
-			inet_twsk_put(tw);
+			inet_twsk_deschedule(inet_twsk(sk), &tcp_death_row);
+			inet_twsk_put(inet_twsk(sk));
 			sk = sk2;
 			goto process;
 		}
-- 
1.6.3.3

[PATCH v6 4/7] tcp: input header length, prediction, and timestamp bugs

[William Allen Simpson]

[-- Attachment #1: Type: text/plain, Size: 1881 bytes --]

Fix incorrect header prediction flags documentation.

Relieve register pressure in (the i386) fast path by accessing skb->len
directly, instead of carrying a rarely used len parameter.

Eliminate unused len parameters in two other functions.

Don't use output calculated tp->tcp_header_len for input decisions.
While the output header is usually the same as the input (same options
in both directions), that's a poor assumption. In particular, Sack will
be different. Newer options are not guaranteed.

Moreover, in the fast path, that only saved a shift or two. The other
efficiencies in this patch more than make up the difference.

Instead, use tp->rx_opt.tstamp_ok to accurately predict header length.

Likewise, use tp->rx_opt.tstamp_ok for received MSS calculations.

Don't use "sizeof(struct tcphdr) + TCPOLEN_TSTAMP_ALIGNED" to guess that
the timestamp is present. This may have been OK in the days with fewer
possible options, but various combinations of newer options may yield
the same header length. (This bug is in 3 places.)

Instead, use tp->rx_opt.saw_tstamp to determine a timestamp is present.

There's no need to test buffer length against header length, already
checked by tcp_v[4,6]_rcv(). Straighten code for minor efficiency gain.

Stand-alone patch, originally developed for TCPCT.

Requires:
   net: tcp_header_len_th and tcp_option_len_th
   tcp: harmonize tcp_vx_rcv header length assumptions

Signed-off-by: William.Allen.Simpson@gmail.com
Reviewed-by: Andi Kleen <andi@firstfloor.org>
---
  include/linux/tcp.h      |    6 ++-
  include/net/tcp.h        |   18 ++++++--
  net/ipv4/tcp_input.c     |   96 +++++++++++++++++++---------------------------
  net/ipv4/tcp_ipv4.c      |    4 +-
  net/ipv4/tcp_minisocks.c |    3 +-
  net/ipv4/tcp_probe.c     |    2 +-
  net/ipv6/tcp_ipv6.c      |    4 +-
  7 files changed, 63 insertions(+), 70 deletions(-)

[-- Attachment #2: len_th+2d6+2.6.34-rc1.patch --]
[-- Type: text/plain, Size: 10152 bytes --]

diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index 854ad65..a97ca8f 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -303,7 +303,11 @@ struct tcp_sock {
 
 /*
  *	Header prediction flags
- *	0x5?10 << 16 + snd_wnd in net byte order
+ *	S << 28 + TCP_FLAG_ACK + snd_wnd, in net byte order
+ *		(PSH flag is ignored)
+ *		S is 5 (no options), or 8 (timestamp aligned)
+ *	otherwise, 0 to turn it off -- for instance, when there are
+ *	holes in receive space.
  */
 	__be32	pred_flags;
 
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 56f0aec..ff2f1c3 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -315,13 +315,11 @@ extern int			tcp_ioctl(struct sock *sk,
 
 extern int			tcp_rcv_state_process(struct sock *sk, 
 						      struct sk_buff *skb,
-						      struct tcphdr *th,
-						      unsigned len);
+						      struct tcphdr *th);
 
 extern int			tcp_rcv_established(struct sock *sk, 
 						    struct sk_buff *skb,
-						    struct tcphdr *th, 
-						    unsigned len);
+						    struct tcphdr *th);
 
 extern void			tcp_rcv_space_adjust(struct sock *sk);
 
@@ -540,9 +538,19 @@ static inline u32 __tcp_set_rto(const struct tcp_sock *tp)
 	return (tp->srtt >> 3) + tp->rttvar;
 }
 
+static inline u8 __tcp_fast_path_header_length(const struct tcp_sock *tp)
+{
+	return tp->rx_opt.tstamp_ok
+		? sizeof(struct tcphdr) + TCPOLEN_TSTAMP_ALIGNED
+		: sizeof(struct tcphdr);
+}
+
 static inline void __tcp_fast_path_on(struct tcp_sock *tp, u32 snd_wnd)
 {
-	tp->pred_flags = htonl((tp->tcp_header_len << 26) |
+	/* See linux/tcp.h for pred_flags details.
+	 * 'S' (doff) is 32-bit words, convert from bytes: 26 = 28 - 2.
+	 */
+	tp->pred_flags = htonl((__tcp_fast_path_header_length(tp) << 26) |
 			       ntohl(TCP_FLAG_ACK) |
 			       snd_wnd);
 }
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 788851c..5541d08 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -154,7 +154,7 @@ static void tcp_measure_rcv_mss(struct sock *sk, const struct sk_buff *skb)
 			 * tcp header plus fixed timestamp option length.
 			 * Resulting "len" is MSS free of SACK jitter.
 			 */
-			len -= tcp_sk(sk)->tcp_header_len;
+			len -= __tcp_fast_path_header_length(tcp_sk(sk));
 			icsk->icsk_ack.last_seg_size = len;
 			if (len == lss) {
 				icsk->icsk_ack.rcv_mss = len;
@@ -5218,7 +5218,7 @@ discard:
  *	tcp_data_queue when everything is OK.
  */
 int tcp_rcv_established(struct sock *sk, struct sk_buff *skb,
-			struct tcphdr *th, unsigned len)
+			struct tcphdr *th)
 {
 	struct tcp_sock *tp = tcp_sk(sk);
 	int res;
@@ -5236,32 +5236,18 @@ int tcp_rcv_established(struct sock *sk, struct sk_buff *skb,
 	 *	Our current scheme is not silly either but we take the
 	 *	extra cost of the net_bh soft interrupt processing...
 	 *	We do checksum and copy also but from device to kernel.
+	 *
+	 *	See linux/tcp.h and net/tcp.h for pred_flags details.
 	 */
-
-	tp->rx_opt.saw_tstamp = 0;
-
-	/*	pred_flags is 0xS?10 << 16 + snd_wnd
-	 *	if header_prediction is to be made
-	 *	'S' will always be tp->tcp_header_len >> 2
-	 *	'?' will be 0 for the fast path, otherwise pred_flags is 0 to
-	 *  turn it off	(when there are holes in the receive
-	 *	 space for instance)
-	 *	PSH flag is ignored.
-	 */
-
 	if ((tcp_flag_word(th) & TCP_HP_BITS) == tp->pred_flags &&
 	    TCP_SKB_CB(skb)->seq == tp->rcv_nxt &&
 	    !after(TCP_SKB_CB(skb)->ack_seq, tp->snd_nxt)) {
-		int tcp_header_len = tp->tcp_header_len;
-
-		/* Timestamp header prediction: tcp_header_len
-		 * is automatically equal to th->doff*4 due to pred_flags
-		 * match.
-		 */
+		int tcp_header_len = tcp_header_len_th(th);
 
-		/* Check timestamp */
-		if (tcp_header_len == sizeof(struct tcphdr) + TCPOLEN_TSTAMP_ALIGNED) {
-			/* No? Slow path! */
+		/* Timestamp header prediction */
+		if (tcp_header_len != sizeof(*th) + TCPOLEN_TSTAMP_ALIGNED) {
+			tp->rx_opt.saw_tstamp = 0; /* false */
+		} else {
 			if (!tcp_parse_aligned_timestamp(tp, th))
 				goto slow_path;
 
@@ -5276,35 +5262,12 @@ int tcp_rcv_established(struct sock *sk, struct sk_buff *skb,
 			 */
 		}
 
-		if (len <= tcp_header_len) {
-			/* Bulk data transfer: sender */
-			if (len == tcp_header_len) {
-				/* Predicted packet is in window by definition.
-				 * seq == rcv_nxt and rcv_wup <= rcv_nxt.
-				 * Hence, check seq<=rcv_wup reduces to:
-				 */
-				if (tcp_header_len ==
-				    (sizeof(struct tcphdr) + TCPOLEN_TSTAMP_ALIGNED) &&
-				    tp->rcv_nxt == tp->rcv_wup)
-					tcp_store_ts_recent(tp);
-
-				/* We know that such packets are checksummed
-				 * on entry.
-				 */
-				tcp_ack(sk, skb, 0);
-				__kfree_skb(skb);
-				tcp_data_snd_check(sk);
-				return 0;
-			} else { /* Header too small */
-				TCP_INC_STATS_BH(sock_net(sk), TCP_MIB_INERRS);
-				goto discard;
-			}
-		} else {
+		if (tcp_header_len < skb->len) {
 			int eaten = 0;
 			int copied_early = 0;
 
 			if (tp->copied_seq == tp->rcv_nxt &&
-			    len - tcp_header_len <= tp->ucopy.len) {
+			    skb->len - tcp_header_len <= tp->ucopy.len) {
 #ifdef CONFIG_NET_DMA
 				if (tcp_dma_try_early_copy(sk, skb, tcp_header_len)) {
 					copied_early = 1;
@@ -5323,9 +5286,7 @@ int tcp_rcv_established(struct sock *sk, struct sk_buff *skb,
 					 * seq == rcv_nxt and rcv_wup <= rcv_nxt.
 					 * Hence, check seq<=rcv_wup reduces to:
 					 */
-					if (tcp_header_len ==
-					    (sizeof(struct tcphdr) +
-					     TCPOLEN_TSTAMP_ALIGNED) &&
+					if (tp->rx_opt.saw_tstamp &&
 					    tp->rcv_nxt == tp->rcv_wup)
 						tcp_store_ts_recent(tp);
 
@@ -5346,8 +5307,7 @@ int tcp_rcv_established(struct sock *sk, struct sk_buff *skb,
 				 * seq == rcv_nxt and rcv_wup <= rcv_nxt.
 				 * Hence, check seq<=rcv_wup reduces to:
 				 */
-				if (tcp_header_len ==
-				    (sizeof(struct tcphdr) + TCPOLEN_TSTAMP_ALIGNED) &&
+				if (tp->rx_opt.saw_tstamp &&
 				    tp->rcv_nxt == tp->rcv_wup)
 					tcp_store_ts_recent(tp);
 
@@ -5388,11 +5348,33 @@ no_ack:
 			else
 				sk->sk_data_ready(sk, 0);
 			return 0;
+		} else {
+			/* Bulk data transfer: sender
+			 *
+			 * tcp_header_len > skb->len never happens,
+			 * already checked by tcp_v[4,6]_rcv()
+			 *
+			 * Predicted packet is in window by definition.
+			 * seq == rcv_nxt and rcv_wup <= rcv_nxt.
+			 * Hence, check seq<=rcv_wup reduces to:
+			 */
+			if (tp->rx_opt.saw_tstamp &&
+			    tp->rcv_nxt == tp->rcv_wup)
+				tcp_store_ts_recent(tp);
+
+			/* We know that such packets are checksummed
+			 * on entry.
+			 */
+			tcp_ack(sk, skb, 0);
+			__kfree_skb(skb);
+			tcp_data_snd_check(sk);
+			return 0;
 		}
 	}
 
 slow_path:
-	if (len < (th->doff << 2) || tcp_checksum_complete_user(sk, skb))
+	/* Assumes skb->len includes header and options */
+	if (tcp_checksum_complete_user(sk, skb))
 		goto csum_error;
 
 	/*
@@ -5428,7 +5410,7 @@ discard:
 }
 
 static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb,
-					 struct tcphdr *th, unsigned len)
+					 struct tcphdr *th)
 {
 	u8 *hash_location;
 	struct inet_connection_sock *icsk = inet_csk(sk);
@@ -5705,7 +5687,7 @@ reset_and_undo:
  */
 
 int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb,
-			  struct tcphdr *th, unsigned len)
+			  struct tcphdr *th)
 {
 	struct tcp_sock *tp = tcp_sk(sk);
 	struct inet_connection_sock *icsk = inet_csk(sk);
@@ -5752,7 +5734,7 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb,
 		goto discard;
 
 	case TCP_SYN_SENT:
-		queued = tcp_rcv_synsent_state_process(sk, skb, th, len);
+		queued = tcp_rcv_synsent_state_process(sk, skb, th);
 		if (queued >= 0)
 			return queued;
 
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index c206024..1860590 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1553,7 +1553,7 @@ int tcp_v4_do_rcv(struct sock *sk, struct sk_buff *skb)
 
 	if (sk->sk_state == TCP_ESTABLISHED) { /* Fast path */
 		TCP_CHECK_TIMER(sk);
-		if (tcp_rcv_established(sk, skb, tcp_hdr(skb), skb->len)) {
+		if (tcp_rcv_established(sk, skb, tcp_hdr(skb))) {
 			rsk = sk;
 			goto reset;
 		}
@@ -1580,7 +1580,7 @@ int tcp_v4_do_rcv(struct sock *sk, struct sk_buff *skb)
 	}
 
 	TCP_CHECK_TIMER(sk);
-	if (tcp_rcv_state_process(sk, skb, tcp_hdr(skb), skb->len)) {
+	if (tcp_rcv_state_process(sk, skb, tcp_hdr(skb))) {
 		rsk = sk;
 		goto reset;
 	}
diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index f206ee5..37b7536 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -718,8 +718,7 @@ int tcp_child_process(struct sock *parent, struct sock *child,
 	int state = child->sk_state;
 
 	if (!sock_owned_by_user(child)) {
-		ret = tcp_rcv_state_process(child, skb, tcp_hdr(skb),
-					    skb->len);
+		ret = tcp_rcv_state_process(child, skb, tcp_hdr(skb));
 		/* Wakeup parent, send SIGIO */
 		if (state == TCP_SYN_RECV && child->sk_state != state)
 			parent->sk_data_ready(parent, 0);
diff --git a/net/ipv4/tcp_probe.c b/net/ipv4/tcp_probe.c
index 9bc805d..de2a32e 100644
--- a/net/ipv4/tcp_probe.c
+++ b/net/ipv4/tcp_probe.c
@@ -88,7 +88,7 @@ static inline int tcp_probe_avail(void)
  * Note: arguments must match tcp_rcv_established()!
  */
 static int jtcp_rcv_established(struct sock *sk, struct sk_buff *skb,
-			       struct tcphdr *th, unsigned len)
+			       struct tcphdr *th)
 {
 	const struct tcp_sock *tp = tcp_sk(sk);
 	const struct inet_sock *inet = inet_sk(sk);
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 04814bf..39e0d89 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1594,7 +1594,7 @@ static int tcp_v6_do_rcv(struct sock *sk, struct sk_buff *skb)
 
 	if (sk->sk_state == TCP_ESTABLISHED) { /* Fast path */
 		TCP_CHECK_TIMER(sk);
-		if (tcp_rcv_established(sk, skb, tcp_hdr(skb), skb->len))
+		if (tcp_rcv_established(sk, skb, tcp_hdr(skb)))
 			goto reset;
 		TCP_CHECK_TIMER(sk);
 		if (opt_skb)
@@ -1626,7 +1626,7 @@ static int tcp_v6_do_rcv(struct sock *sk, struct sk_buff *skb)
 	}
 
 	TCP_CHECK_TIMER(sk);
-	if (tcp_rcv_state_process(sk, skb, tcp_hdr(skb), skb->len))
+	if (tcp_rcv_state_process(sk, skb, tcp_hdr(skb)))
 		goto reset;
 	TCP_CHECK_TIMER(sk);
 	if (opt_skb)
-- 
1.6.3.3

[PATCH v3 5/7] TCPCT part 2e: accept SYNACK data

[William Allen Simpson]

[-- Attachment #1: Type: text/plain, Size: 713 bytes --]

When accompanied by cookie option, Initiator (client) queues incoming
SYNACK transaction data.

This is a straightforward re-implementation of an earlier (18 month 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

This function will also be used in subsequent patches that implement
additional features.

Requires:
   TCPCT part 1g: Responder Cookie => Initiator
   net: tcp_header_len_th and tcp_option_len_th

Signed-off-by: William.Allen.Simpson@gmail.com
---
  net/ipv4/tcp_input.c |   26 +++++++++++++++++++++++++-
  1 files changed, 25 insertions(+), 1 deletions(-)

[-- Attachment #2: TCPCT+2e3+2.6.34-rc1.patch --]
[-- Type: text/plain, Size: 2057 bytes --]

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 5541d08..7371406 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -5409,6 +5409,12 @@ discard:
 	return 0;
 }
 
+/*
+ * Returns:
+ *	+1 on reset,
+ *	0 success and/or SYNACK data,
+ *	-1 on discard.
+ */
 static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb,
 					 struct tcphdr *th)
 {
@@ -5417,6 +5423,7 @@ static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb,
 	struct tcp_sock *tp = tcp_sk(sk);
 	struct tcp_cookie_values *cvp = tp->cookie_values;
 	int saved_clamp = tp->rx_opt.mss_clamp;
+	int queued = 0;
 
 	tcp_parse_options(skb, &tp->rx_opt, &hash_location, 0);
 
@@ -5523,6 +5530,7 @@ static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb,
 					- TCPOLEN_COOKIE_BASE;
 			int cookie_pair_size = cookie_size
 					     + cvp->cookie_desired;
+			int tcp_header_len = tcp_header_len_th(th);
 
 			/* A cookie extension option was sent and returned.
 			 * Note that each incoming SYNACK replaces the
@@ -5538,6 +5546,19 @@ static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb,
 				       hash_location, cookie_size);
 				cvp->cookie_pair_size = cookie_pair_size;
 			}
+
+			queued = skb->len - tcp_header_len;
+			if (queued > 0) {
+				/* Queue incoming transaction data. */
+				__skb_pull(skb, tcp_header_len);
+				__skb_queue_tail(&sk->sk_receive_queue, skb);
+				skb_set_owner_r(skb, sk);
+				sk->sk_data_ready(sk, 0);
+				cvp->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();
@@ -5591,11 +5612,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 (queued <= 0)
+				__kfree_skb(skb);
 			return 0;
 		} else {
 			tcp_send_ack(sk);
 		}
+		if (queued > 0)
+			return 0;
 		return -1;
 	}
 
-- 
1.6.3.3

[PATCH v6 6/7] TCPCT part 2f: cleanup tcp_parse_options

[William Allen Simpson]

[-- Attachment #1: Type: text/plain, Size: 996 bytes --]

Split switch, shift cases to the left, fix most lines beyond column 80.

Add error return.

Harmonize parameter order with other tcp_input functions:
tcp_fast_parse_options() and tcp_validate_incoming().

Harmonize initialization in syncookies.
Fix initialization in tcp_minisocks.

Repair net/ipv4/tcp_ipv4.c errors with goto targets, overlooked by
David Miller in commit bb5b7c11263dbbe78253cd05945a6bf8f55add8e

[updated to latest posted internet draft-simpson-tcpct-00]

Requires:
   TCPCT part 1g: Responder Cookie => Initiator
   net: tcp_header_len_th and tcp_option_len_th

Signed-off-by: William.Allen.Simpson@gmail.com
---
  include/net/tcp.h        |    5 +-
  net/ipv4/syncookies.c    |    9 +-
  net/ipv4/tcp_input.c     |  238 +++++++++++++++++++++++++++-------------------
  net/ipv4/tcp_ipv4.c      |   10 ++-
  net/ipv4/tcp_minisocks.c |   26 ++++--
  net/ipv6/syncookies.c    |    9 +-
  net/ipv6/tcp_ipv6.c      |    6 +-
  7 files changed, 185 insertions(+), 118 deletions(-)

[-- Attachment #2: TCPCT+2f6+2.6.34-rc1.patch --]
[-- Type: text/plain, Size: 15996 bytes --]

diff --git a/include/net/tcp.h b/include/net/tcp.h
index ff2f1c3..f8c99fa 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -410,8 +410,9 @@ extern int			tcp_recvmsg(struct kiocb *iocb, struct sock *sk,
 					    size_t len, int nonblock, 
 					    int flags, int *addr_len);
 
-extern void			tcp_parse_options(struct sk_buff *skb,
-						  struct tcp_options_received *opt_rx,
+extern int			tcp_parse_options(struct tcp_options_received *opt_rx,
+						  struct sk_buff *skb,
+						  const struct tcphdr *th,
 						  u8 **hvpp,
 						  int estab);
 
diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
index 5c24db4..abaa7d7 100644
--- a/net/ipv4/syncookies.c
+++ b/net/ipv4/syncookies.c
@@ -255,15 +255,16 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb,
 {
 	struct tcp_options_received tcp_opt;
 	u8 *hash_location;
+	struct rtable *rt;
+	struct request_sock *req;
 	struct inet_request_sock *ireq;
 	struct tcp_request_sock *treq;
 	struct tcp_sock *tp = tcp_sk(sk);
 	const struct tcphdr *th = tcp_hdr(skb);
 	__u32 cookie = ntohl(th->ack_seq) - 1;
 	struct sock *ret = sk;
-	struct request_sock *req;
 	int mss;
-	struct rtable *rt;
+	int parsed;
 	__u8 rcv_wscale;
 
 	if (!sysctl_tcp_syncookies || !th->ack)
@@ -279,7 +280,9 @@ 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, &hash_location, 0);
+	parsed = tcp_parse_options(&tcp_opt, skb, th, &hash_location, 0);
+	if (parsed < 0)
+		goto out;
 
 	if (tcp_opt.saw_tstamp)
 		cookie_check_timestamp(&tcp_opt);
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 7371406..cc26090 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -3737,123 +3737,154 @@ old_ack:
 /* Look for tcp options. Normally only called on SYN and SYNACK packets.
  * But, this can also be called on packets in the established flow when
  * the fast version below fails.
+ *
+ * Returns:
+ *	0 on success
+ *	- on failure
  */
-void tcp_parse_options(struct sk_buff *skb, struct tcp_options_received *opt_rx,
-		       u8 **hvpp, int estab)
+int tcp_parse_options(struct tcp_options_received *opt_rx, struct sk_buff *skb,
+		      const struct tcphdr *th, u8 **hvpp, int estab)
 {
-	unsigned char *ptr;
-	struct tcphdr *th = tcp_hdr(skb);
-	int length = (th->doff * 4) - sizeof(struct tcphdr);
+	unsigned char *ptr = (unsigned char *)(th + 1);
+	int length = tcp_option_len_th(th);
+	bool syn = th->syn;
 
-	ptr = (unsigned char *)(th + 1);
-	opt_rx->saw_tstamp = 0;
+	opt_rx->cookie_plus = 0;
+	opt_rx->saw_tstamp = 0; /* false */
 
 	while (length > 0) {
-		int opcode = *ptr++;
 		int opsize;
+		int opcode = *ptr++;
 
 		switch (opcode) {
 		case TCPOPT_EOL:
-			return;
+			length = 0;
+			continue;
 		case TCPOPT_NOP:	/* Ref: RFC 793 section 3.1 */
 			length--;
 			continue;
 		default:
-			opsize = *ptr++;
-			if (opsize < 2) /* "silly options" */
-				return;
-			if (opsize > length)
-				return;	/* don't parse partial options */
-			switch (opcode) {
-			case TCPOPT_MSS:
-				if (opsize == TCPOLEN_MSS && th->syn && !estab) {
-					u16 in_mss = get_unaligned_be16(ptr);
-					if (in_mss) {
-						if (opt_rx->user_mss &&
-						    opt_rx->user_mss < in_mss)
-							in_mss = opt_rx->user_mss;
-						opt_rx->mss_clamp = in_mss;
-					}
-				}
-				break;
-			case TCPOPT_WINDOW:
-				if (opsize == TCPOLEN_WINDOW && th->syn &&
-				    !estab && sysctl_tcp_window_scaling) {
-					__u8 snd_wscale = *(__u8 *)ptr;
-					opt_rx->wscale_ok = 1;
-					if (snd_wscale > 14) {
-						if (net_ratelimit())
-							printk(KERN_INFO "tcp_parse_options: Illegal window "
-							       "scaling value %d >14 received.\n",
-							       snd_wscale);
-						snd_wscale = 14;
-					}
-					opt_rx->snd_wscale = snd_wscale;
-				}
-				break;
-			case TCPOPT_TIMESTAMP:
-				if ((opsize == TCPOLEN_TIMESTAMP) &&
-				    ((estab && opt_rx->tstamp_ok) ||
-				     (!estab && sysctl_tcp_timestamps))) {
-					opt_rx->saw_tstamp = 1;
-					opt_rx->rcv_tsval = get_unaligned_be32(ptr);
-					opt_rx->rcv_tsecr = get_unaligned_be32(ptr + 4);
-				}
-				break;
-			case TCPOPT_SACK_PERM:
-				if (opsize == TCPOLEN_SACK_PERM && th->syn &&
-				    !estab && sysctl_tcp_sack) {
-					opt_rx->sack_ok = 1;
-					tcp_sack_reset(opt_rx);
+			/* fallthru */
+			break;
+		};
+
+		opsize = *ptr++;
+		if (opsize < 2 || opsize > length) {
+			/* don't parse partial options */
+			break;
+		}
+
+		switch (opcode) {
+		case TCPOPT_MSS:
+			if (opsize == TCPOLEN_MSS && syn && !estab) {
+				u16 in_mss = get_unaligned_be16(ptr);
+				if (in_mss) {
+					if (opt_rx->user_mss &&
+					    opt_rx->user_mss < in_mss)
+						in_mss = opt_rx->user_mss;
+					opt_rx->mss_clamp = in_mss;
 				}
-				break;
+			}
+			break;
 
-			case TCPOPT_SACK:
-				if ((opsize >= (TCPOLEN_SACK_BASE + TCPOLEN_SACK_PERBLOCK)) &&
-				   !((opsize - TCPOLEN_SACK_BASE) % TCPOLEN_SACK_PERBLOCK) &&
-				   opt_rx->sack_ok) {
-					TCP_SKB_CB(skb)->sacked = (ptr - 2) - (unsigned char *)th;
+		case TCPOPT_WINDOW:
+			if (opsize == TCPOLEN_WINDOW && syn &&
+			    !estab && sysctl_tcp_window_scaling) {
+				__u8 snd_wscale = *(__u8 *)ptr;
+				opt_rx->wscale_ok = 1;
+				if (snd_wscale > 14) {
+					if (net_ratelimit())
+						printk(KERN_INFO
+						       "tcp_parse_options: "
+						       "window scaling value "
+						       "%d > 14 received.\n",
+						       snd_wscale);
+					snd_wscale = 14;
 				}
-				break;
+				opt_rx->snd_wscale = snd_wscale;
+			}
+			break;
+
+		case TCPOPT_SACK_PERM:
+			if (opsize == TCPOLEN_SACK_PERM && syn &&
+			    !estab && sysctl_tcp_sack) {
+				opt_rx->sack_ok = 1;
+				tcp_sack_reset(opt_rx);
+			}
+			break;
+
+		case TCPOPT_SACK:
+			if ((opsize >= (TCPOLEN_SACK_BASE + TCPOLEN_SACK_PERBLOCK)) &&
+			    !((opsize - TCPOLEN_SACK_BASE) % TCPOLEN_SACK_PERBLOCK) &&
+			    opt_rx->sack_ok) {
+				TCP_SKB_CB(skb)->sacked = (ptr - 2)
+							- (unsigned char *)th;
+			}
+			break;
+
+		case TCPOPT_TIMESTAMP:
+			if ((opsize == TCPOLEN_TIMESTAMP) &&
+			    ((estab && opt_rx->tstamp_ok) ||
+			     (!estab && sysctl_tcp_timestamps))) {
+				opt_rx->saw_tstamp = 1;
+				opt_rx->rcv_tsval = get_unaligned_be32(ptr);
+				opt_rx->rcv_tsecr = get_unaligned_be32(ptr + 4);
+			}
+			break;
 #ifdef CONFIG_TCP_MD5SIG
-			case TCPOPT_MD5SIG:
-				/*
-				 * The MD5 Hash has already been
-				 * checked (see tcp_v{4,6}_do_rcv()).
-				 */
-				break;
+		case TCPOPT_MD5SIG:
+			/*
+			 * The MD5 Hash has already been
+			 * checked (see tcp_v[4,6]_do_rcv()).
+			 */
+			break;
 #endif
-			case TCPOPT_COOKIE:
-				/* This option is variable length.
-				 */
-				switch (opsize) {
-				case TCPOLEN_COOKIE_BASE:
-					/* not yet implemented */
-					break;
-				case TCPOLEN_COOKIE_PAIR:
-					/* not yet implemented */
-					break;
-				case TCPOLEN_COOKIE_MIN+0:
-				case TCPOLEN_COOKIE_MIN+2:
-				case TCPOLEN_COOKIE_MIN+4:
-				case TCPOLEN_COOKIE_MIN+6:
-				case TCPOLEN_COOKIE_MAX:
-					/* 16-bit multiple */
+		case TCPOPT_COOKIE:
+			if (unlikely(opt_rx->cookie_plus > 0)) {
+				/* discard duplicate */
+				return -length;
+			}
+			switch (opsize) {
+			case TCPOLEN_COOKIE_BASE:
+				/* not yet implemented */
+				break;
+			case TCPOLEN_COOKIE_PAIR: {
+				/* not yet implemented */
+				break;
+			}
+			case TCPOLEN_COOKIE_MIN+0:
+			case TCPOLEN_COOKIE_MIN+2:
+			case TCPOLEN_COOKIE_MIN+4:
+			case TCPOLEN_COOKIE_MIN+6:
+			case TCPOLEN_COOKIE_MAX:
+				/* 16-bit multiple */
+				if (syn) {
 					opt_rx->cookie_plus = opsize;
 					*hvpp = ptr;
-				default:
-					/* ignore option */
-					break;
-				};
+				}
+				break;
+			default:
+				/* ignore option */
 				break;
 			};
+			break;
 
-			ptr += opsize-2;
-			length -= opsize;
-		}
+		default:
+			/* skip unrecognized options */
+			break;
+		};
+
+		ptr += opsize - 2;
+		length -= opsize;
 	}
+	return 0;
 }
 
+/*
+ * Returns:
+ *	1 on success
+ *	0 on failure
+ */
 static int tcp_parse_aligned_timestamp(struct tcp_sock *tp, struct tcphdr *th)
 {
 	__be32 *ptr = (__be32 *)(th + 1);
@@ -3872,9 +3903,14 @@ static int tcp_parse_aligned_timestamp(struct tcp_sock *tp, struct tcphdr *th)
 
 /* Fast parse options. This hopes to only see timestamps.
  * If it is wrong it falls back on tcp_parse_options().
+ *
+ * Returns:
+ *	1 on success, fast
+ *	0 on success, slow
+ *	- on failure
  */
-static int tcp_fast_parse_options(struct sk_buff *skb, struct tcphdr *th,
-				  struct tcp_sock *tp, u8 **hvpp)
+static int tcp_fast_parse_options(struct tcp_sock *tp, struct sk_buff *skb,
+				  struct tcphdr *th, u8 **hvpp)
 {
 	/* In the spirit of fast parsing, compare doff directly to constant
 	 * values.  Because equality is used, short doff can be ignored here.
@@ -3887,8 +3923,7 @@ static int tcp_fast_parse_options(struct sk_buff *skb, struct tcphdr *th,
 		if (tcp_parse_aligned_timestamp(tp, th))
 			return 1;
 	}
-	tcp_parse_options(skb, &tp->rx_opt, hvpp, 1);
-	return 1;
+	return tcp_parse_options(&tp->rx_opt, skb, th, hvpp, 1);
 }
 
 #ifdef CONFIG_TCP_MD5SIG
@@ -5135,14 +5170,17 @@ out:
  * play significant role here.
  */
 static int tcp_validate_incoming(struct sock *sk, struct sk_buff *skb,
-			      struct tcphdr *th, int syn_inerr)
+				 struct tcphdr *th, int syn_inerr)
 {
 	u8 *hash_location;
 	struct tcp_sock *tp = tcp_sk(sk);
+	int parsed = tcp_fast_parse_options(tp, skb, th, &hash_location);
+
+	if (parsed < 0)
+		goto discard;
 
 	/* RFC1323: H1. Apply PAWS check first. */
-	if (tcp_fast_parse_options(skb, th, tp, &hash_location) &&
-	    tp->rx_opt.saw_tstamp &&
+	if (tp->rx_opt.saw_tstamp &&
 	    tcp_paws_discard(sk, skb)) {
 		if (!th->rst) {
 			NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_PAWSESTABREJECTED);
@@ -5424,8 +5462,10 @@ static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb,
 	struct tcp_cookie_values *cvp = tp->cookie_values;
 	int saved_clamp = tp->rx_opt.mss_clamp;
 	int queued = 0;
+	int parsed = tcp_parse_options(&tp->rx_opt, skb, th, &hash_location, 0);
 
-	tcp_parse_options(skb, &tp->rx_opt, &hash_location, 0);
+	if (parsed < 0)
+		goto discard;
 
 	if (th->ack) {
 		/* rfc793:
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 1860590..bdfa9c5 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1217,6 +1217,7 @@ int tcp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
 {
 	struct tcp_extend_values tmp_ext;
 	struct tcp_options_received tmp_opt;
+	int parsed;
 	u8 *hash_location;
 	struct request_sock *req;
 	struct inet_request_sock *ireq;
@@ -1267,7 +1268,10 @@ int tcp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
 	tcp_clear_options(&tmp_opt);
 	tmp_opt.mss_clamp = TCP_MSS_DEFAULT;
 	tmp_opt.user_mss  = tp->rx_opt.user_mss;
-	tcp_parse_options(skb, &tmp_opt, &hash_location, 0);
+	parsed = tcp_parse_options(&tmp_opt, skb, tcp_hdr(skb),
+				   &hash_location, 0);
+	if (parsed < 0)
+		goto drop_and_free;
 
 	if (tmp_opt.cookie_plus > 0 &&
 	    tmp_opt.saw_tstamp &&
@@ -1280,7 +1284,7 @@ int tcp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
 		int l = tmp_opt.cookie_plus - TCPOLEN_COOKIE_BASE;
 
 		if (tcp_cookie_generator(&tmp_ext.cookie_bakery[0]) != 0)
-			goto drop_and_release;
+			goto drop_and_free;
 
 		/* Secret recipe starts with IP addresses */
 		*mess++ ^= daddr;
@@ -1301,7 +1305,7 @@ int tcp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
 		tmp_ext.cookie_out_never = 1; /* true */
 		tmp_ext.cookie_plus = 0;
 	} else {
-		goto drop_and_release;
+		goto drop_and_free;
 	}
 	tmp_ext.cookie_in_always = tp->rx_opt.cookie_in_always;
 
diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index 37b7536..0b764e7 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -95,15 +95,21 @@ tcp_timewait_state_process(struct inet_timewait_sock *tw, struct sk_buff *skb,
 	struct tcp_timewait_sock *tcptw = tcp_twsk((struct sock *)tw);
 	int paws_reject = 0;
 
-	tmp_opt.saw_tstamp = 0;
+	/* Fast check for options, compare doff directly to constant value. */
 	if (th->doff > (sizeof(*th) >> 2) && tcptw->tw_ts_recent_stamp) {
-		tcp_parse_options(skb, &tmp_opt, &hash_location, 0);
+		int parsed = tcp_parse_options(&tmp_opt, skb, th,
+					       &hash_location, 0);
 
-		if (tmp_opt.saw_tstamp) {
+		if (parsed < 0) {
+			paws_reject = 1; /* true */
+		} else if (tmp_opt.saw_tstamp) {
 			tmp_opt.ts_recent	= tcptw->tw_ts_recent;
 			tmp_opt.ts_recent_stamp	= tcptw->tw_ts_recent_stamp;
 			paws_reject = tcp_paws_reject(&tmp_opt, th->rst);
 		}
+	} else {
+		/* otherwise initialized by tcp_parse_options() */
+		tmp_opt.saw_tstamp = 0; /* false */
 	}
 
 	if (tw->tw_substate == TCP_FIN_WAIT2) {
@@ -526,11 +532,14 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
 	__be32 flg = tcp_flag_word(th) & (TCP_FLAG_RST|TCP_FLAG_SYN|TCP_FLAG_ACK);
 	int paws_reject = 0;
 
-	tmp_opt.saw_tstamp = 0;
-	if (th->doff > (sizeof(struct tcphdr)>>2)) {
-		tcp_parse_options(skb, &tmp_opt, &hash_location, 0);
+	/* Fast check for options, compare doff directly to constant value. */
+	if (th->doff > (sizeof(*th) >> 2)) {
+		int parsed = tcp_parse_options(&tmp_opt, skb, th,
+					       &hash_location, 0);
 
-		if (tmp_opt.saw_tstamp) {
+		if (parsed < 0) {
+			paws_reject = 1; /* true */
+		} else if (tmp_opt.saw_tstamp) {
 			tmp_opt.ts_recent = req->ts_recent;
 			/* We do not store true stamp, but it is not required,
 			 * it can be estimated (approximately)
@@ -539,6 +548,9 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
 			tmp_opt.ts_recent_stamp = get_seconds() - ((TCP_TIMEOUT_INIT/HZ)<<req->retrans);
 			paws_reject = tcp_paws_reject(&tmp_opt, th->rst);
 		}
+	} else {
+		/* otherwise initialized by tcp_parse_options() */
+		tmp_opt.saw_tstamp = 0; /* false */
 	}
 
 	/* Check for pure retransmitted SYN. */
diff --git a/net/ipv6/syncookies.c b/net/ipv6/syncookies.c
index 34d1f06..4f31ba3 100644
--- a/net/ipv6/syncookies.c
+++ b/net/ipv6/syncookies.c
@@ -161,6 +161,8 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
 {
 	struct tcp_options_received tcp_opt;
 	u8 *hash_location;
+	struct dst_entry *dst;
+	struct request_sock *req;
 	struct inet_request_sock *ireq;
 	struct inet6_request_sock *ireq6;
 	struct tcp_request_sock *treq;
@@ -169,9 +171,8 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
 	const struct tcphdr *th = tcp_hdr(skb);
 	__u32 cookie = ntohl(th->ack_seq) - 1;
 	struct sock *ret = sk;
-	struct request_sock *req;
 	int mss;
-	struct dst_entry *dst;
+	int parsed;
 	__u8 rcv_wscale;
 
 	if (!sysctl_tcp_syncookies || !th->ack)
@@ -187,7 +188,9 @@ 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, &hash_location, 0);
+	parsed = tcp_parse_options(&tcp_opt, skb, th, &hash_location, 0);
+	if (parsed < 0)
+		goto out;
 
 	if (tcp_opt.saw_tstamp)
 		cookie_check_timestamp(&tcp_opt);
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 39e0d89..1a79782 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1172,6 +1172,7 @@ static int tcp_v6_conn_request(struct sock *sk, struct sk_buff *skb)
 {
 	struct tcp_extend_values tmp_ext;
 	struct tcp_options_received tmp_opt;
+	int parsed;
 	u8 *hash_location;
 	struct request_sock *req;
 	struct inet6_request_sock *treq;
@@ -1215,7 +1216,10 @@ static int tcp_v6_conn_request(struct sock *sk, struct sk_buff *skb)
 	tcp_clear_options(&tmp_opt);
 	tmp_opt.mss_clamp = IPV6_MIN_MTU - sizeof(struct tcphdr) - sizeof(struct ipv6hdr);
 	tmp_opt.user_mss = tp->rx_opt.user_mss;
-	tcp_parse_options(skb, &tmp_opt, &hash_location, 0);
+	parsed = tcp_parse_options(&tmp_opt, skb, tcp_hdr(skb),
+				   &hash_location, 0);
+	if (parsed < 0)
+		goto drop_and_free;
 
 	if (tmp_opt.cookie_plus > 0 &&
 	    tmp_opt.saw_tstamp &&
-- 
1.6.3.3

[PATCH v8 7/7] TCPCT part 2g: parse cookie pair and 64-bit timestamp

[William Allen Simpson]

[-- Attachment #1: Type: text/plain, Size: 926 bytes --]

Every bit is sacred.  Use as few bits as possible in tcp_options_received.
Group related timestamp flag bits for cache line memory efficiency.

Fix #define spacing for TCP options.

Define and parse 64-bit timestamp extended option (and minor cleanup).
However, only 32-bits are used at this time (permitted by specification).

Parse cookie pair extended option (previously defined).

Handle header extension.

Fix initialization in tcp_minisocks.

[updated to latest posted internet draft-simpson-tcpct-00]

Requires:
   net: tcp_header_len_th and tcp_option_len_th
   TCPCT part 2f: cleanup tcp_parse_options

Signed-off-by: William.Allen.Simpson@gmail.com
---
  include/linux/tcp.h      |   12 ++++-
  include/net/tcp.h        |   45 ++++++++--------
  net/ipv4/tcp_input.c     |  127 ++++++++++++++++++++++++++++++++++++++++++----
  net/ipv4/tcp_minisocks.c |    6 ++
  4 files changed, 155 insertions(+), 35 deletions(-)

[-- Attachment #2: TCPCT+2g8+2.6.34-rc1.patch --]
[-- Type: text/plain, Size: 10711 bytes --]

diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index a97ca8f..8fdc64e 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -249,7 +249,7 @@ 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		*/
+	u16	tstamp64_ok:1,	/* Verified with cookie pair		*/
 		tstamp_ok : 1,	/* TIMESTAMP seen on SYN packet		*/
 		dsack : 1,	/* D-SACK is scheduled			*/
 		wscale_ok : 1,	/* Wscale seen on SYN packet		*/
@@ -262,13 +262,21 @@ struct tcp_options_received {
 	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 */
+
+	/* When the options are extended beyond the standard 40 bytes,
+	 * this holds the additional data offset (up to 1,020 bytes).
+	 */
+	u8	extended;	/* in 32-bit words			*/
+	u8	saw_tstamp64:1,	/* 64-bit TIMESTAMP seen on last packet	*/
+		saw_tstamp:1,	/* TIMESTAMP seen on last packet	*/
+		__unused:6;
 };
 
 static inline void tcp_clear_options(struct tcp_options_received *rx_opt)
 {
+	rx_opt->tstamp64_ok = 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
diff --git a/include/net/tcp.h b/include/net/tcp.h
index f8c99fa..1399c86 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -156,9 +156,8 @@ extern void tcp_time_wait(struct sock *sk, int state, int timeo);
 /*
  *	TCP option
  */
- 
-#define TCPOPT_NOP		1	/* Padding */
 #define TCPOPT_EOL		0	/* End of options */
+#define TCPOPT_NOP		1	/* Padding */
 #define TCPOPT_MSS		2	/* Segment size negotiating */
 #define TCPOPT_WINDOW		3	/* Window scaling */
 #define TCPOPT_SACK_PERM        4       /* SACK Permitted */
@@ -166,30 +165,32 @@ extern void tcp_time_wait(struct sock *sk, int state, int timeo);
 #define TCPOPT_TIMESTAMP	8	/* Better RTT estimations/PAWS */
 #define TCPOPT_MD5SIG		19	/* MD5 Signature (RFC2385) */
 #define TCPOPT_COOKIE		253	/* Cookie extension (experimental) */
-
-/*
- *     TCP option lengths
- */
-
-#define TCPOLEN_MSS            4
-#define TCPOLEN_WINDOW         3
-#define TCPOLEN_SACK_PERM      2
-#define TCPOLEN_TIMESTAMP      10
-#define TCPOLEN_MD5SIG         18
-#define TCPOLEN_COOKIE_BASE    2	/* Cookie-less header extension */
-#define TCPOLEN_COOKIE_PAIR    3	/* Cookie pair header extension */
-#define TCPOLEN_COOKIE_MIN     (TCPOLEN_COOKIE_BASE+TCP_COOKIE_MIN)
-#define TCPOLEN_COOKIE_MAX     (TCPOLEN_COOKIE_BASE+TCP_COOKIE_MAX)
-
-/* But this is what stacks really send out. */
-#define TCPOLEN_TSTAMP_ALIGNED		12
+#define TCPOPT_TIMESTAMP64	254	/* 64-bit extension (experimental) */
+
+/*	TCP option lengths (same order as above) */
+#define TCPOLEN_MSS		4
+#define TCPOLEN_WINDOW		3
+#define TCPOLEN_SACK_PERM	2
+#define TCPOLEN_SACK_BASE	2
+#define TCPOLEN_SACK_PERBLOCK	8
+#define TCPOLEN_TIMESTAMP	10
+#define TCPOLEN_MD5SIG		18
+#define TCPOLEN_COOKIE_BASE	2	/* Cookie-less negotiation */
+#define TCPOLEN_COOKIE_PLUS	4	/* Cookie pair header extension */
+#define TCPOLEN_COOKIE_MIN	(TCPOLEN_COOKIE_BASE+TCP_COOKIE_MIN)
+#define TCPOLEN_COOKIE_MAX	(TCPOLEN_COOKIE_BASE+TCP_COOKIE_MAX)
+#define TCPOLEN_TSTAMP64_PLUS	3	/* Timestamped header extension */
+
+/*	TCP options 32-bit aligned (same order as above) */
+#define TCPOLEN_MSS_ALIGNED		4
 #define TCPOLEN_WSCALE_ALIGNED		4
 #define TCPOLEN_SACKPERM_ALIGNED	4
-#define TCPOLEN_SACK_BASE		2
 #define TCPOLEN_SACK_BASE_ALIGNED	4
-#define TCPOLEN_SACK_PERBLOCK		8
+#define TCPOLEN_TSTAMP_ALIGNED		12
 #define TCPOLEN_MD5SIG_ALIGNED		20
-#define TCPOLEN_MSS_ALIGNED		4
+
+/*	TCP option extensions (same order as above) */
+#define TCPOEXT_TSTAMP64_PLUS		16
 
 /* Flags in tp->nonagle */
 #define TCP_NAGLE_OFF		1	/* Nagle's algo is disabled */
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index cc26090..1e07eb7 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -3745,13 +3745,19 @@ old_ack:
 int tcp_parse_options(struct tcp_options_received *opt_rx, struct sk_buff *skb,
 		      const struct tcphdr *th, u8 **hvpp, int estab)
 {
+	__be32 *xdp = (__be32 *)th + th->doff;
 	unsigned char *ptr = (unsigned char *)(th + 1);
+	int remaining = skb_headlen(skb) - tcp_header_len_th(th);
 	int length = tcp_option_len_th(th);
+	int extending = 0;
 	bool syn = th->syn;
 
 	opt_rx->cookie_plus = 0;
+	opt_rx->extended = 0;
+	opt_rx->saw_tstamp64 = 0; /* false */
 	opt_rx->saw_tstamp = 0; /* false */
 
+repeat:
 	while (length > 0) {
 		int opsize;
 		int opcode = *ptr++;
@@ -3823,6 +3829,10 @@ int tcp_parse_options(struct tcp_options_received *opt_rx, struct sk_buff *skb,
 			break;
 
 		case TCPOPT_TIMESTAMP:
+			if (unlikely(opt_rx->saw_tstamp)) {
+				/* discard duplicate */
+				return -length;
+			}
 			if ((opsize == TCPOLEN_TIMESTAMP) &&
 			    ((estab && opt_rx->tstamp_ok) ||
 			     (!estab && sysctl_tcp_timestamps))) {
@@ -3848,27 +3858,112 @@ int tcp_parse_options(struct tcp_options_received *opt_rx, struct sk_buff *skb,
 			case TCPOLEN_COOKIE_BASE:
 				/* not yet implemented */
 				break;
-			case TCPOLEN_COOKIE_PAIR: {
-				/* not yet implemented */
+			case TCPOLEN_COOKIE_PLUS: {
+				int words = ptr[1] & 0xf;
+
+				if (unlikely(opt_rx->extended > 0)) {
+					/* discard conflicting */
+					return -length;
+				}
+				if (!syn &&
+				    *ptr >= words &&
+				    words >= (TCP_COOKIE_MIN / 4) &&
+				    words <= (TCP_COOKIE_MAX / 4)) {
+					int bytes = words * 4;
+
+					extending = *ptr * 4;
+
+					if (unlikely(extending > remaining)) {
+						/* missing data!!! */
+						return -length;
+					}
+					opt_rx->extended = *ptr;
+
+					/* Adjust end_seq, set in
+					 * tcp_v[4,6]_rcv()
+					 */
+					TCP_SKB_CB(skb)->end_seq -= extending;
+					extending -= bytes;
+
+					opt_rx->cookie_plus = bytes
+						+ TCPOLEN_COOKIE_BASE;
+					*hvpp = (u8 *)xdp;
+					xdp += words;
+				}
 				break;
 			}
 			case TCPOLEN_COOKIE_MIN+0:
 			case TCPOLEN_COOKIE_MIN+2:
 			case TCPOLEN_COOKIE_MIN+4:
 			case TCPOLEN_COOKIE_MIN+6:
-			case TCPOLEN_COOKIE_MAX:
 				/* 16-bit multiple */
 				if (syn) {
 					opt_rx->cookie_plus = opsize;
 					*hvpp = ptr;
 				}
 				break;
+			case TCPOLEN_COOKIE_MAX+0:
+				/* either cookie or cookie pair */
+				if (syn || opt_rx->saw_tstamp64) {
+					opt_rx->cookie_plus = opsize;
+					*hvpp = ptr;
+				}
+				break;
+			case TCPOLEN_COOKIE_MAX+4:
+			case TCPOLEN_COOKIE_MAX+8:
+			case TCPOLEN_COOKIE_MAX+12:
+			case TCPOLEN_COOKIE_MAX+TCP_COOKIE_MAX:
+				/* 32-bit multiple (pair) */
+				if (opt_rx->saw_tstamp64) {
+					opt_rx->cookie_plus = opsize;
+					*hvpp = ptr;
+				}
+				break;
 			default:
 				/* ignore option */
 				break;
 			};
 			break;
 
+		case TCPOPT_TIMESTAMP64:
+			if (unlikely(opt_rx->saw_tstamp)) {
+				/* discard duplicate */
+				return -length;
+			}
+			if (opsize == TCPOLEN_TSTAMP64_PLUS) {
+				if (unlikely(opt_rx->extended > 0)) {
+					/* discard conflicting */
+					return -length;
+				}
+				if (!syn &&
+				    *ptr >= (TCPOEXT_TSTAMP64_PLUS / 4)) {
+					extending = *ptr * 4;
+
+					if (unlikely(extending > remaining)) {
+						/* missing data!!! */
+						return -length;
+					}
+					opt_rx->extended = *ptr;
+
+					/* Adjust end_seq, set in
+					 * tcp_v[4,6]_rcv()
+					 */
+					TCP_SKB_CB(skb)->end_seq -= extending;
+					extending -= TCPOEXT_TSTAMP64_PLUS;
+
+					/* 64-bits not yet implemented */
+					xdp++;
+					opt_rx->rcv_tsval = ntohl(*xdp);
+					xdp += 2;
+					opt_rx->rcv_tsecr = ntohl(*xdp);
+					xdp++;
+
+					opt_rx->saw_tstamp64 = 1; /* true */
+					opt_rx->saw_tstamp = 1; /* true */
+				}
+			}
+			break;
+
 		default:
 			/* skip unrecognized options */
 			break;
@@ -3877,6 +3972,13 @@ int tcp_parse_options(struct tcp_options_received *opt_rx, struct sk_buff *skb,
 		ptr += opsize - 2;
 		length -= opsize;
 	}
+
+	if (unlikely(extending != 0)) {
+		ptr = (unsigned char *)xdp;
+		length = extending;
+		extending = 0;
+		goto repeat;
+	}
 	return 0;
 }
 
@@ -3917,11 +4019,14 @@ static int tcp_fast_parse_options(struct tcp_sock *tp, struct sk_buff *skb,
 	 */
 	if (th->doff == (sizeof(*th) / 4)) {
 		tp->rx_opt.saw_tstamp = 0;
+		tp->rx_opt.extended = 0;
 		return 0;
-	} else if (tp->rx_opt.tstamp_ok &&
-		   th->doff == ((sizeof(*th) + TCPOLEN_TSTAMP_ALIGNED) / 4)) {
-		if (tcp_parse_aligned_timestamp(tp, th))
-			return 1;
+	}
+	if (th->doff == ((sizeof(*th) + TCPOLEN_TSTAMP_ALIGNED) / 4) &&
+	    tp->rx_opt.tstamp_ok &&
+	    tcp_parse_aligned_timestamp(tp, th)) {
+		tp->rx_opt.extended = 0;
+		return 1;
 	}
 	return tcp_parse_options(&tp->rx_opt, skb, th, hvpp, 1);
 }
@@ -3932,8 +4037,8 @@ static int tcp_fast_parse_options(struct tcp_sock *tp, struct sk_buff *skb,
  */
 u8 *tcp_parse_md5sig_option(struct tcphdr *th)
 {
-	int length = (th->doff << 2) - sizeof (*th);
 	u8 *ptr = (u8*)(th + 1);
+	int length = tcp_option_len_th(th);
 
 	/* If the TCP option is too short, we can short cut */
 	if (length < TCPOLEN_MD5SIG)
@@ -4398,7 +4503,7 @@ static void tcp_data_queue(struct sock *sk, struct sk_buff *skb)
 	if (TCP_SKB_CB(skb)->seq == TCP_SKB_CB(skb)->end_seq)
 		goto drop;
 
-	__skb_pull(skb, th->doff * 4);
+	__skb_pull(skb, (th->doff + tp->rx_opt.extended) * 4);
 
 	TCP_ECN_accept_cwr(tp, skb);
 
@@ -5059,8 +5164,8 @@ static void tcp_urg(struct sock *sk, struct sk_buff *skb, struct tcphdr *th)
 
 	/* Do we wait for any urgent data? - normally not... */
 	if (tp->urg_data == TCP_URG_NOTYET) {
-		u32 ptr = tp->urg_seq - ntohl(th->seq) + (th->doff * 4) -
-			  th->syn;
+		u32 ptr = ((th->doff + tp->rx_opt.extended) * 4)
+			+ tp->urg_seq - ntohl(th->seq) - th->syn;
 
 		/* Is the urgent pointer pointing into this packet? */
 		if (ptr < skb->len) {
diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index 0b764e7..613325d 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -109,6 +109,9 @@ tcp_timewait_state_process(struct inet_timewait_sock *tw, struct sk_buff *skb,
 		}
 	} else {
 		/* otherwise initialized by tcp_parse_options() */
+		tmp_opt.cookie_plus = 0;
+		tmp_opt.extended = 0;
+		tmp_opt.saw_tstamp64 = 0; /* false */
 		tmp_opt.saw_tstamp = 0; /* false */
 	}
 
@@ -550,6 +553,9 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
 		}
 	} else {
 		/* otherwise initialized by tcp_parse_options() */
+		tmp_opt.cookie_plus = 0;
+		tmp_opt.extended = 0;
+		tmp_opt.saw_tstamp64 = 0; /* false */
 		tmp_opt.saw_tstamp = 0; /* false */
 	}
 
-- 
1.6.3.3

Re: [PATCH 0/7] tcp: bugs and cleanup for 2.6.34-rc1

[Eric Dumazet]

Le jeudi 11 mars 2010 à 06:35 -0500, William Allen Simpson a écrit :
> I'd have thought that there would be greater interest about patching
> crashing bugs, signed versus unsigned (underflow) bugs, TCP DoS bugs,
> TCP data corruption, and TCP performance problems....
> 
> There's been ample warning.  Zero-day security issues will be reported
> to the usual announcement lists.  In particular, these 0day exploits
> affect systems as far back as the 2005 changeover to git.
> 
> Combination of patches reported in October, November, December, January,
> and February, for 2.6.32, 2.6.33, and now 2.6.34.
> 
> This code has had previous review and several months of limited testing.
> 
> Some portions were removed during the various TCPCT part 1 patch splits,
> then were cut off by the sudden unexpected end of that merge window.
> [03 Dec 2009]  I've restarted the sub-numbering (again).
> 
> Of particular interest are the TCPCT header extensions that already
> appear in the next phase of testing with other platforms.  These patches
> allow correct reception without data corruption.
> 
> The remainder of the original TCPCT part 2 will be merged with part 3.
> 
> [Updated to 2010 Mar 08 2.6.34-rc1.]
> --

Mr William Allen Simpson

It would be nice if you could update your knowledge of how linux
development works these days.

Please spend few hours for that, it will save us lot of time.
Our time is valuable as much as yours, I doubt we'll change our habits
to fit your wills.

You throw too many changes at once to let them being reviewed,
understood, and accepted.

For your information, we had to correct a fatal bug introduced by your
last commits, and as far as I know, you didnt help that much.

http://git2.kernel.org/?p=linux/kernel/git/davem/net-2.6.git;a=commit;h=28b2774a0d5852236dab77a4147b8b88548110f1

<at this moment of time>

We are post linux-2.6.34-rc1, so only bug fixes are wanted by Linus and
David, to be integrated in 2.6.34 (and previous versions if needed)

We are _not_ interested by new stuff at *this* moment, especially if it
takes lot of time to review.

</at this moment of time>

New network stuff (for 2.6.35 or 2.6.36) should be validated once
net-next-2.6 re-opens (in about one week I suppose, David will send a
mail to netdev to let us/you know the exact moment).

So please split your patches again and submit only bug fixes to netdev.

Once accepted by community and maintainer, David will push them
upstream.

Then, in about 10 days, please submit new stuff that hopefully find
their way if you accept our reviews and comments.

Last time I made some comments on your patches, you just ignored them or
loaned, because obviously who is Eric Dumazet to tell William Allen
Simpson how things should be done ? Silly me !

I remember this fairly well, this is why I ignored your last submissions
(and privately explained to you why I did this).

Speaking for myself, but as your previous mails were ignored, I felt it
was time to clarify the points.

Re: [PATCH 0/7] tcp: bugs and cleanup for 2.6.34-rc1

[William Allen Simpson]

On 3/11/10 10:01 AM, Eric Dumazet wrote:
> It would be nice if you could update your knowledge of how linux
> development works these days.
>
Perhaps you could supply pointers to the relevant documentation?


> You throw too many changes at once to let them being reviewed,
> understood, and accepted.
>
These were originally submitted in groups of 1, 2, and 3 patches for
review.  For example, as TCPCT parts 1h and 1i (and had been part of
earlier patch series, too), as of 2009-12-03:

   http://patchwork.ozlabs.org/patch/40284/

Resubmitted again in even finer grained patches, as of 2009-12-31 and
again 2010-01-06:

   http://lkml.org/lkml/2010/1/6/299

Since February, I've grouped them all together, as they've been reviewed,
and re-reviewed -- yet only deprecated by the netdev maintainer.


> For your information, we had to correct a fatal bug introduced by your
> last commits, and as far as I know, you didnt help that much.
>
> http://git2.kernel.org/?p=linux/kernel/git/davem/net-2.6.git;a=commit;h=28b2774a0d5852236dab77a4147b8b88548110f1
>
Splendid!  Thank you for the heads up!  Unfortunately, that 3 day old
email wasn't CC'd to me.  I'm glad to hear that Mika is testing.

And thank you for testing the patch.  (You did test, didn't you?)
Testing is always good!

That code always worked for me, and presumably for Adam (who wrote it).
We've always used small amounts of data -- only 64 bytes, as originally
specified.  The latest API document allows up to 1220.  Folks just keep
wanting more!

(The latest API also drops the subscript, so that patch would have been
changed eventually....)

This code (PATCH v3 5/7) handles the data on the receiving side of the
same transaction, a patch that was first submitted over 18 months ago!


> <at this moment of time>
>
> We are post linux-2.6.34-rc1, so only bug fixes are wanted by Linus and
> David, to be integrated in 2.6.34 (and previous versions if needed)
>
> We are _not_ interested by new stuff at *this* moment, especially if it
> takes lot of time to review.
>
> </at this moment of time>
>
Good.  Because this isn't new stuff.  It's bug fixes and related cleanup.
Generally, the cleanup was needed to find and test the bugs and patches.

They're already "split up" from the main set of patches, as Ilpo asked
over 4 months ago.

I've not been making *any* new submissions around here, until *existing*
submissions have been applied.


> Last time I made some comments on your patches, you just ignored them or
> loaned, because obviously who is Eric Dumazet to tell William Allen
> Simpson how things should be done ? Silly me !
>
Last time you made any comments at all, it was trivial argument about
parenthesis and casts.  I asked directly for more *substantive* review:

   http://lkml.org/lkml/2010/1/13/162

Thankfully, we've had substantive review from Andi over a period of
months, on parts 3 and 4 of the current patch series....

And a short attaboy of part 2 a couple of weeks ago.


> Speaking for myself, but as your previous mails were ignored, I felt it
> was time to clarify the points.
>
Thank you.

Re: [PATCH 0/7] tcp: bugs and cleanup for 2.6.34-rc1

[Joe Perches]

On Thu, 2010-03-11 at 12:38 -0500, William Allen Simpson wrote:
> > http://git2.kernel.org/?p=linux/kernel/git/davem/net-2.6.git;a=commit;h=28b2774a0d5852236dab77a4147b8b88548110f1

In that patch, it might be  better to use
	u16 s_data_desired = 0;
not
	int s_data_desired = 0;

Re: [PATCH v4 2/7] net: remove old tcp_optlen function

[Simon Horman]

On Thu, Mar 11, 2010 at 06:53:06AM -0500, William Allen Simpson wrote:
> The tcp_optlen() function returns a potential *negative* unsigned.
> 
> In the only two existing files using the old tcp_optlen() function,
> clean up confusing and inconsistent mixing of both byte and word
> offsets, and other coding style issues.  Document assumptions.

IIRC, Dave already pointed out that this quoting style which singles him
out isn't appropriate. Perhaps you should consider updating it if you want
him to consider merging this and other changes with similar quotes in the
changelog.

I suggest moving the "No response from testers in 21+ weeks." to
below the "--- " somewhere and dropping everything else except
the Signed-off line that appears after this point.

> Quoth David Miller:
>    This is transmit, and the packets can only come from the Linux
>    TCP stack, not some external entity.
> 
>    You're being way too anal here, and adding these checks to
>    drivers would be just a lot of rediculious bloat. [sic]
> 
> Therefore, there are *no* checks for bad TCP and IP header sizes, nor
> any semantic changes.  The drivers should function exactly as existing,
> although usage of int should ameliorate the issues.

> No response from testers in 21+ weeks.
> 
> [removed comment references to commit log]
> 
> Requires:
>   net: tcp_header_len_th and tcp_option_len_th
> 
> Signed-off-by: William.Allen.Simpson@gmail.com
> CC: Michael Chan <mchan@broadcom.com>
> ---
>  drivers/net/bnx2.c  |   29 +++++++++++++-----------
>  drivers/net/tg3.c   |   60 +++++++++++++++++++++++---------------------------
>  include/linux/tcp.h |    5 ----
>  3 files changed, 44 insertions(+), 50 deletions(-)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 2/7] net: remove old tcp_optlen function

[William Allen Simpson]

On 3/11/10 7:26 PM, Simon Horman wrote:
> On Thu, Mar 11, 2010 at 06:53:06AM -0500, William Allen Simpson wrote:
>> The tcp_optlen() function returns a potential *negative* unsigned.
>>
>> In the only two existing files using the old tcp_optlen() function,
>> clean up confusing and inconsistent mixing of both byte and word
>> offsets, and other coding style issues.  Document assumptions.
>
> IIRC, Dave already pointed out that this quoting style which singles him
> out isn't appropriate. Perhaps you should consider updating it if you want
> him to consider merging this and other changes with similar quotes in the
> changelog.
>
Thanks!  I'm primarily concerned with fixing the bug(s) introduced by
commit ab6a5bb6b28a970104a34f0f6959b73cf61bdc72 that stuffs a negative
number (after subtraction) into an unsigned result.  At the time, the
unsigned result was then stuffed back into int again.  Later folks
changed the int to u32 (in some places, inconsistently).

However, Dave's requirement that there be no TCP or IP length checking
needs to be documented.  Anybody coming to the code later will wonder why
that has not been done (eliminated from earlier versions of my patch).

Rather than spend valuable time refuting Dave and fixing all of the many
problems with TCP and IP lengths elsewhere in the tree (those are more
appropriate to other patches), just document his existing assumptions, and
move on....

Documentation/CodingStyle:
   ... put the comments at the head of the function, telling people what it
   does, and possibly WHY it does it.

The short requirement statement is in the function header comment(s).

The full quote is in the commit log.  Folks researching the "git blame"
for this patch in the future can read his rationale and hyperbole.

On Feb 17th, Dave mandated that "(see commit log)" in the source code
comments be removed:

   "We do not refer to commit log messages from the source code."

That has been done, causing the patch version to increment.


> I suggest moving the "No response from testers in 21+ weeks." to
> below the "--- " somewhere and dropping everything else except
> the Signed-off line that appears after this point.
>
Thanks!  I was unaware that putting such things after "--- " was allowed,
but checking Documentation/SubmittingPatches:

   - A marker line containing simply "---".

   - Any additional comments not suitable for the changelog.

In my next message, I'll update the change log.

Thanks again!

Re: [PATCH v4 2/7] net: remove old tcp_optlen function

[William Allen Simpson]

[-- Attachment #1: Type: text/plain, Size: 1167 bytes --]

The tcp_optlen() function returns a potential *negative* unsigned.

In the only two existing files using the old tcp_optlen() function,
clean up confusing and inconsistent mixing of both byte and word
offsets, and other coding style issues.  Document assumptions.

Quoth David Miller:
    This is transmit, and the packets can only come from the Linux
    TCP stack, not some external entity.

    You're being way too anal here, and adding these checks to
    drivers would be just a lot of rediculious bloat. [sic]

Therefore, there are *no* checks for bad TCP and IP header sizes, nor
any semantic changes.  The drivers should function exactly as existing,
although usage of int should ameliorate the issues.

Requires:
   net: tcp_header_len_th and tcp_option_len_th

Signed-off-by: William.Allen.Simpson@gmail.com
CC: Michael Chan <mchan@broadcom.com>
---
  drivers/net/bnx2.c  |   29 +++++++++++++-----------
  drivers/net/tg3.c   |   60 +++++++++++++++++++++++---------------------------
  include/linux/tcp.h |    5 ----
  3 files changed, 44 insertions(+), 50 deletions(-)

No response from testers in 21+ weeks.

[removed comment references to commit log]


[-- Attachment #2: len_th+2b4+2.6.34-rc1.patch --]
[-- Type: text/plain, Size: 6918 bytes --]

diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
index 381887b..87607b8 100644
--- a/drivers/net/bnx2.c
+++ b/drivers/net/bnx2.c
@@ -6335,6 +6335,8 @@ bnx2_vlan_rx_register(struct net_device *dev, struct vlan_group *vlgrp)
 /* Called with netif_tx_lock.
  * bnx2_tx_int() runs without netif_tx_lock unless it needs to call
  * netif_wake_queue().
+ *
+ * No TCP or IP length checking, as required by David Miller.
  */
 static netdev_tx_t
 bnx2_start_xmit(struct sk_buff *skb, struct net_device *dev)
@@ -6378,19 +6380,19 @@ bnx2_start_xmit(struct sk_buff *skb, struct net_device *dev)
 			(TX_BD_FLAGS_VLAN_TAG | (vlan_tx_tag_get(skb) << 16));
 	}
 #endif
-	if ((mss = skb_shinfo(skb)->gso_size)) {
-		u32 tcp_opt_len;
-		struct iphdr *iph;
+	mss = skb_shinfo(skb)->gso_size;
+	if (mss != 0) {
+		struct tcphdr *th = tcp_hdr(skb);
+		int tcp_opt_words = th->doff - (sizeof(*th) >> 2);
+		/* assumes positive tcp_opt_words without checking */
 
 		vlan_tag_flags |= TX_BD_FLAGS_SW_LSO;
 
-		tcp_opt_len = tcp_optlen(skb);
-
 		if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV6) {
 			u32 tcp_off = skb_transport_offset(skb) -
 				      sizeof(struct ipv6hdr) - ETH_HLEN;
 
-			vlan_tag_flags |= ((tcp_opt_len >> 2) << 8) |
+			vlan_tag_flags |= (tcp_opt_words << 8) |
 					  TX_BD_FLAGS_SW_FLAGS;
 			if (likely(tcp_off == 0))
 				vlan_tag_flags &= ~TX_BD_FLAGS_TCP6_OFF0_MSK;
@@ -6403,14 +6405,15 @@ bnx2_start_xmit(struct sk_buff *skb, struct net_device *dev)
 				mss |= (tcp_off & 0xc) << TX_BD_TCP6_OFF2_SHL;
 			}
 		} else {
-			iph = ip_hdr(skb);
-			if (tcp_opt_len || (iph->ihl > 5)) {
-				vlan_tag_flags |= ((iph->ihl - 5) +
-						   (tcp_opt_len >> 2)) << 8;
-			}
+			struct iphdr *iph = ip_hdr(skb);
+			int ip_opt_words = iph->ihl - (sizeof(*iph) >> 2);
+			/* assumes positive ip_opt_words without checking */
+			int opt_words = ip_opt_words + tcp_opt_words;
+
+			if (opt_words > 0)
+				vlan_tag_flags |= opt_words << 8;
 		}
-	} else
-		mss = 0;
+	}
 
 	mapping = pci_map_single(bp->pdev, skb->data, len, PCI_DMA_TODEVICE);
 	if (pci_dma_mapping_error(bp->pdev, mapping)) {
diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index 0fa7688..6ad8184 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -5481,6 +5481,8 @@ static void tg3_set_txd(struct tg3_napi *tnapi, int entry,
 
 /* hard_start_xmit for devices that don't have any bugs and
  * support TG3_FLG2_HW_TSO_2 and TG3_FLG2_HW_TSO_3 only.
+ *
+ * No TCP or IP length checking, as required by David Miller.
  */
 static netdev_tx_t tg3_start_xmit(struct sk_buff *skb,
 				  struct net_device *dev)
@@ -5515,9 +5517,9 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb,
 
 	entry = tnapi->tx_prod;
 	base_flags = 0;
-	mss = 0;
-	if ((mss = skb_shinfo(skb)->gso_size) != 0) {
-		int tcp_opt_len, ip_tcp_len;
+	mss = skb_shinfo(skb)->gso_size;
+	if (mss != 0) {
+		struct tcphdr *th;
 		u32 hdrlen;
 
 		if (skb_header_cloned(skb) &&
@@ -5525,18 +5527,16 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb,
 			dev_kfree_skb(skb);
 			goto out_unlock;
 		}
+		th = tcp_hdr(skb);
 
 		if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV6)
 			hdrlen = skb_headlen(skb) - ETH_HLEN;
 		else {
 			struct iphdr *iph = ip_hdr(skb);
 
-			tcp_opt_len = tcp_optlen(skb);
-			ip_tcp_len = ip_hdrlen(skb) + sizeof(struct tcphdr);
-
+			hdrlen = ip_hdrlen(skb) + tcp_header_len_th(th);
+			iph->tot_len = htons(mss + hdrlen);
 			iph->check = 0;
-			iph->tot_len = htons(mss + ip_tcp_len + tcp_opt_len);
-			hdrlen = ip_tcp_len + tcp_opt_len;
 		}
 
 		if (tp->tg3_flags2 & TG3_FLG2_HW_TSO_3) {
@@ -5550,7 +5550,7 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb,
 		base_flags |= (TXD_FLAG_CPU_PRE_DMA |
 			       TXD_FLAG_CPU_POST_DMA);
 
-		tcp_hdr(skb)->check = 0;
+		th->check = 0;
 
 	}
 	else if (skb->ip_summed == CHECKSUM_PARTIAL)
@@ -5683,6 +5683,8 @@ tg3_tso_bug_end:
 
 /* hard_start_xmit for devices that have the 4G bug and/or 40-bit bug and
  * support TG3_FLG2_HW_TSO_1 or firmware TSO only.
+ *
+ * No TCP or IP length checking, as required by David Miller.
  */
 static netdev_tx_t tg3_start_xmit_dma_bug(struct sk_buff *skb,
 					  struct net_device *dev)
@@ -5721,20 +5723,21 @@ static netdev_tx_t tg3_start_xmit_dma_bug(struct sk_buff *skb,
 	if (skb->ip_summed == CHECKSUM_PARTIAL)
 		base_flags |= TXD_FLAG_TCPUDP_CSUM;
 
-	if ((mss = skb_shinfo(skb)->gso_size) != 0) {
+	mss = skb_shinfo(skb)->gso_size;
+	if (mss != 0) {
 		struct iphdr *iph;
-		u32 tcp_opt_len, ip_tcp_len, hdr_len;
+		struct tcphdr *th;
+		u32 hdr_len;
+		int opt_bytes;
 
 		if (skb_header_cloned(skb) &&
 		    pskb_expand_head(skb, 0, 0, GFP_ATOMIC)) {
 			dev_kfree_skb(skb);
 			goto out_unlock;
 		}
+		th = tcp_hdr(skb);
+		hdr_len = ip_hdrlen(skb) + tcp_header_len_th(th);
 
-		tcp_opt_len = tcp_optlen(skb);
-		ip_tcp_len = ip_hdrlen(skb) + sizeof(struct tcphdr);
-
-		hdr_len = ip_tcp_len + tcp_opt_len;
 		if (unlikely((ETH_HLEN + hdr_len) > 80) &&
 			     (tp->tg3_flags2 & TG3_FLG2_TSO_BUG))
 			return (tg3_tso_bug(tp, skb));
@@ -5746,13 +5749,14 @@ static netdev_tx_t tg3_start_xmit_dma_bug(struct sk_buff *skb,
 		iph->check = 0;
 		iph->tot_len = htons(mss + hdr_len);
 		if (tp->tg3_flags2 & TG3_FLG2_HW_TSO) {
-			tcp_hdr(skb)->check = 0;
+			th->check = 0;
 			base_flags &= ~TXD_FLAG_TCPUDP_CSUM;
 		} else
-			tcp_hdr(skb)->check = ~csum_tcpudp_magic(iph->saddr,
-								 iph->daddr, 0,
-								 IPPROTO_TCP,
-								 0);
+			th->check = ~csum_tcpudp_magic(iph->saddr, iph->daddr,
+						       0, IPPROTO_TCP, 0);
+
+		opt_bytes = hdr_len - sizeof(*iph) - sizeof(*th);
+		/* assumes positive opt_bytes without checking */
 
 		if (tp->tg3_flags2 & TG3_FLG2_HW_TSO_3) {
 			mss |= (hdr_len & 0xc) << 12;
@@ -5763,19 +5767,11 @@ static netdev_tx_t tg3_start_xmit_dma_bug(struct sk_buff *skb,
 			mss |= hdr_len << 9;
 		else if ((tp->tg3_flags2 & TG3_FLG2_HW_TSO_1) ||
 			 GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5705) {
-			if (tcp_opt_len || iph->ihl > 5) {
-				int tsflags;
-
-				tsflags = (iph->ihl - 5) + (tcp_opt_len >> 2);
-				mss |= (tsflags << 11);
-			}
+			if (opt_bytes > 0)
+				mss |= opt_bytes << (11 - 2);
 		} else {
-			if (tcp_opt_len || iph->ihl > 5) {
-				int tsflags;
-
-				tsflags = (iph->ihl - 5) + (tcp_opt_len >> 2);
-				base_flags |= tsflags << 12;
-			}
+			if (opt_bytes > 0)
+				base_flags |= opt_bytes << (12 - 2);
 		}
 	}
 #if TG3_VLAN_TAG_USED
diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index 48ddeb8..854ad65 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -220,11 +220,6 @@ static inline unsigned int tcp_hdrlen(const struct sk_buff *skb)
 	return tcp_hdr(skb)->doff * 4;
 }
 
-static inline unsigned int tcp_optlen(const struct sk_buff *skb)
-{
-	return (tcp_hdr(skb)->doff - 5) * 4;
-}
-
 /* Length of fixed header plus standard options. */
 static inline unsigned int tcp_header_len_th(const struct tcphdr *th)
 {
-- 
1.6.3.3

Re: [PATCH 0/7] tcp: bugs and cleanup for 2.6.34-rc1

[William Allen Simpson]

On 3/11/10 1:14 PM, Joe Perches wrote:
> On Thu, 2010-03-11 at 12:38 -0500, William Allen Simpson wrote:
>>> http://git2.kernel.org/?p=linux/kernel/git/davem/net-2.6.git;a=commit;h=28b2774a0d5852236dab77a4147b8b88548110f1
>
> In that patch, it might be  better to use
> 	u16 s_data_desired = 0;
> not
> 	int s_data_desired = 0;
>
Actually, Eric wrote that part of the message, and that patch.  I expect
that ship has sailed, but you could submit a patch....

Re: [PATCH v4 2/7] net: remove old tcp_optlen function

[Dan Carpenter]

On Fri, Mar 12, 2010 at 08:25:16AM -0500, William Allen Simpson wrote:
> The tcp_optlen() function returns a potential *negative* unsigned.
>
> In the only two existing files using the old tcp_optlen() function,
> clean up confusing and inconsistent mixing of both byte and word
> offsets, and other coding style issues.  Document assumptions.
>
> Quoth David Miller:
>    This is transmit, and the packets can only come from the Linux
>    TCP stack, not some external entity.
>
>    You're being way too anal here, and adding these checks to
>    drivers would be just a lot of rediculious bloat. [sic]
>
> Therefore, there are *no* checks for bad TCP and IP header sizes, nor
> any semantic changes.  The drivers should function exactly as existing,
> although usage of int should ameliorate the issues.
>

So after you removed the checks this change includes:
1) Random slagging on the networking guys
2) u32 => int to ameliorate your static checker's complaints
3) cleanups

People have already explained that tcp_optlen() doesn't return
negative values.  It would really help us if you could show how 
tcp_hdr(skb)->doff can be less than 5?

regards,
dan carpenter

> Requires:
>   net: tcp_header_len_th and tcp_option_len_th
>
> Signed-off-by: William.Allen.Simpson@gmail.com
> CC: Michael Chan <mchan@broadcom.com>
> ---
>  drivers/net/bnx2.c  |   29 +++++++++++++-----------
>  drivers/net/tg3.c   |   60 +++++++++++++++++++++++---------------------------
>  include/linux/tcp.h |    5 ----
>  3 files changed, 44 insertions(+), 50 deletions(-)
>
> No response from testers in 21+ weeks.
>
> [removed comment references to commit log]
>

> diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
> index 381887b..87607b8 100644
> --- a/drivers/net/bnx2.c
> +++ b/drivers/net/bnx2.c
> @@ -6335,6 +6335,8 @@ bnx2_vlan_rx_register(struct net_device *dev, struct vlan_group *vlgrp)
>  /* Called with netif_tx_lock.
>   * bnx2_tx_int() runs without netif_tx_lock unless it needs to call
>   * netif_wake_queue().
> + *
> + * No TCP or IP length checking, as required by David Miller.
>   */
>  static netdev_tx_t
>  bnx2_start_xmit(struct sk_buff *skb, struct net_device *dev)
> @@ -6378,19 +6380,19 @@ bnx2_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  			(TX_BD_FLAGS_VLAN_TAG | (vlan_tx_tag_get(skb) << 16));
>  	}
>  #endif
> -	if ((mss = skb_shinfo(skb)->gso_size)) {
> -		u32 tcp_opt_len;
> -		struct iphdr *iph;
> +	mss = skb_shinfo(skb)->gso_size;
> +	if (mss != 0) {
> +		struct tcphdr *th = tcp_hdr(skb);
> +		int tcp_opt_words = th->doff - (sizeof(*th) >> 2);
> +		/* assumes positive tcp_opt_words without checking */
>  
>  		vlan_tag_flags |= TX_BD_FLAGS_SW_LSO;
>  
> -		tcp_opt_len = tcp_optlen(skb);
> -
>  		if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV6) {
>  			u32 tcp_off = skb_transport_offset(skb) -
>  				      sizeof(struct ipv6hdr) - ETH_HLEN;
>  
> -			vlan_tag_flags |= ((tcp_opt_len >> 2) << 8) |
> +			vlan_tag_flags |= (tcp_opt_words << 8) |
>  					  TX_BD_FLAGS_SW_FLAGS;
>  			if (likely(tcp_off == 0))
>  				vlan_tag_flags &= ~TX_BD_FLAGS_TCP6_OFF0_MSK;
> @@ -6403,14 +6405,15 @@ bnx2_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  				mss |= (tcp_off & 0xc) << TX_BD_TCP6_OFF2_SHL;
>  			}
>  		} else {
> -			iph = ip_hdr(skb);
> -			if (tcp_opt_len || (iph->ihl > 5)) {
> -				vlan_tag_flags |= ((iph->ihl - 5) +
> -						   (tcp_opt_len >> 2)) << 8;
> -			}
> +			struct iphdr *iph = ip_hdr(skb);
> +			int ip_opt_words = iph->ihl - (sizeof(*iph) >> 2);
> +			/* assumes positive ip_opt_words without checking */
> +			int opt_words = ip_opt_words + tcp_opt_words;
> +
> +			if (opt_words > 0)
> +				vlan_tag_flags |= opt_words << 8;
>  		}
> -	} else
> -		mss = 0;
> +	}
>  
>  	mapping = pci_map_single(bp->pdev, skb->data, len, PCI_DMA_TODEVICE);
>  	if (pci_dma_mapping_error(bp->pdev, mapping)) {
> diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
> index 0fa7688..6ad8184 100644
> --- a/drivers/net/tg3.c
> +++ b/drivers/net/tg3.c
> @@ -5481,6 +5481,8 @@ static void tg3_set_txd(struct tg3_napi *tnapi, int entry,
>  
>  /* hard_start_xmit for devices that don't have any bugs and
>   * support TG3_FLG2_HW_TSO_2 and TG3_FLG2_HW_TSO_3 only.
> + *
> + * No TCP or IP length checking, as required by David Miller.
>   */
>  static netdev_tx_t tg3_start_xmit(struct sk_buff *skb,
>  				  struct net_device *dev)
> @@ -5515,9 +5517,9 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb,
>  
>  	entry = tnapi->tx_prod;
>  	base_flags = 0;
> -	mss = 0;
> -	if ((mss = skb_shinfo(skb)->gso_size) != 0) {
> -		int tcp_opt_len, ip_tcp_len;
> +	mss = skb_shinfo(skb)->gso_size;
> +	if (mss != 0) {
> +		struct tcphdr *th;
>  		u32 hdrlen;
>  
>  		if (skb_header_cloned(skb) &&
> @@ -5525,18 +5527,16 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb,
>  			dev_kfree_skb(skb);
>  			goto out_unlock;
>  		}
> +		th = tcp_hdr(skb);
>  
>  		if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV6)
>  			hdrlen = skb_headlen(skb) - ETH_HLEN;
>  		else {
>  			struct iphdr *iph = ip_hdr(skb);
>  
> -			tcp_opt_len = tcp_optlen(skb);
> -			ip_tcp_len = ip_hdrlen(skb) + sizeof(struct tcphdr);
> -
> +			hdrlen = ip_hdrlen(skb) + tcp_header_len_th(th);
> +			iph->tot_len = htons(mss + hdrlen);
>  			iph->check = 0;
> -			iph->tot_len = htons(mss + ip_tcp_len + tcp_opt_len);
> -			hdrlen = ip_tcp_len + tcp_opt_len;
>  		}
>  
>  		if (tp->tg3_flags2 & TG3_FLG2_HW_TSO_3) {
> @@ -5550,7 +5550,7 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb,
>  		base_flags |= (TXD_FLAG_CPU_PRE_DMA |
>  			       TXD_FLAG_CPU_POST_DMA);
>  
> -		tcp_hdr(skb)->check = 0;
> +		th->check = 0;
>  
>  	}
>  	else if (skb->ip_summed == CHECKSUM_PARTIAL)
> @@ -5683,6 +5683,8 @@ tg3_tso_bug_end:
>  
>  /* hard_start_xmit for devices that have the 4G bug and/or 40-bit bug and
>   * support TG3_FLG2_HW_TSO_1 or firmware TSO only.
> + *
> + * No TCP or IP length checking, as required by David Miller.
>   */
>  static netdev_tx_t tg3_start_xmit_dma_bug(struct sk_buff *skb,
>  					  struct net_device *dev)
> @@ -5721,20 +5723,21 @@ static netdev_tx_t tg3_start_xmit_dma_bug(struct sk_buff *skb,
>  	if (skb->ip_summed == CHECKSUM_PARTIAL)
>  		base_flags |= TXD_FLAG_TCPUDP_CSUM;
>  
> -	if ((mss = skb_shinfo(skb)->gso_size) != 0) {
> +	mss = skb_shinfo(skb)->gso_size;
> +	if (mss != 0) {
>  		struct iphdr *iph;
> -		u32 tcp_opt_len, ip_tcp_len, hdr_len;
> +		struct tcphdr *th;
> +		u32 hdr_len;
> +		int opt_bytes;
>  
>  		if (skb_header_cloned(skb) &&
>  		    pskb_expand_head(skb, 0, 0, GFP_ATOMIC)) {
>  			dev_kfree_skb(skb);
>  			goto out_unlock;
>  		}
> +		th = tcp_hdr(skb);
> +		hdr_len = ip_hdrlen(skb) + tcp_header_len_th(th);
>  
> -		tcp_opt_len = tcp_optlen(skb);
> -		ip_tcp_len = ip_hdrlen(skb) + sizeof(struct tcphdr);
> -
> -		hdr_len = ip_tcp_len + tcp_opt_len;
>  		if (unlikely((ETH_HLEN + hdr_len) > 80) &&
>  			     (tp->tg3_flags2 & TG3_FLG2_TSO_BUG))
>  			return (tg3_tso_bug(tp, skb));
> @@ -5746,13 +5749,14 @@ static netdev_tx_t tg3_start_xmit_dma_bug(struct sk_buff *skb,
>  		iph->check = 0;
>  		iph->tot_len = htons(mss + hdr_len);
>  		if (tp->tg3_flags2 & TG3_FLG2_HW_TSO) {
> -			tcp_hdr(skb)->check = 0;
> +			th->check = 0;
>  			base_flags &= ~TXD_FLAG_TCPUDP_CSUM;
>  		} else
> -			tcp_hdr(skb)->check = ~csum_tcpudp_magic(iph->saddr,
> -								 iph->daddr, 0,
> -								 IPPROTO_TCP,
> -								 0);
> +			th->check = ~csum_tcpudp_magic(iph->saddr, iph->daddr,
> +						       0, IPPROTO_TCP, 0);
> +
> +		opt_bytes = hdr_len - sizeof(*iph) - sizeof(*th);
> +		/* assumes positive opt_bytes without checking */
>  
>  		if (tp->tg3_flags2 & TG3_FLG2_HW_TSO_3) {
>  			mss |= (hdr_len & 0xc) << 12;
> @@ -5763,19 +5767,11 @@ static netdev_tx_t tg3_start_xmit_dma_bug(struct sk_buff *skb,
>  			mss |= hdr_len << 9;
>  		else if ((tp->tg3_flags2 & TG3_FLG2_HW_TSO_1) ||
>  			 GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5705) {
> -			if (tcp_opt_len || iph->ihl > 5) {
> -				int tsflags;
> -
> -				tsflags = (iph->ihl - 5) + (tcp_opt_len >> 2);
> -				mss |= (tsflags << 11);
> -			}
> +			if (opt_bytes > 0)
> +				mss |= opt_bytes << (11 - 2);
>  		} else {
> -			if (tcp_opt_len || iph->ihl > 5) {
> -				int tsflags;
> -
> -				tsflags = (iph->ihl - 5) + (tcp_opt_len >> 2);
> -				base_flags |= tsflags << 12;
> -			}
> +			if (opt_bytes > 0)
> +				base_flags |= opt_bytes << (12 - 2);
>  		}
>  	}
>  #if TG3_VLAN_TAG_USED
> diff --git a/include/linux/tcp.h b/include/linux/tcp.h
> index 48ddeb8..854ad65 100644
> --- a/include/linux/tcp.h
> +++ b/include/linux/tcp.h
> @@ -220,11 +220,6 @@ static inline unsigned int tcp_hdrlen(const struct sk_buff *skb)
>  	return tcp_hdr(skb)->doff * 4;
>  }
>  
> -static inline unsigned int tcp_optlen(const struct sk_buff *skb)
> -{
> -	return (tcp_hdr(skb)->doff - 5) * 4;
> -}
> -
>  /* Length of fixed header plus standard options. */
>  static inline unsigned int tcp_header_len_th(const struct tcphdr *th)
>  {
> -- 
> 1.6.3.3
> 

Re: [PATCH v4 2/7] net: remove old tcp_optlen function

[William Allen Simpson]

All the drama is beside the point.  This patch merely removes a *rarely*
used function (2 drivers).  Not complicated....

There's a reason that this function isn't used much.  It doesn't work.


On 3/12/10 12:46 PM, Dan Carpenter wrote:
> So after you removed the checks this change includes:

I didn't remove any *existing* checks.  I had added *new* checks in my
earlier patch, then removed *my* checks from this patch as required by
David Miller.


> 1) Random slagging on the networking guys

I had to look up that "random slagging on" colloquialism.  Apparently,
the "random slagging" target would be *me* -- calling me "anal" and my
code "rediculious bloat" [sic] probably qualifies....

(Admittedly, I'm rather careful and may be overly cautious at times, after
some 30+ years of writing network drivers.  Once it's in half a billion
cell phones, it's hard/impossible to update.)

Since my first unpleasant interactions with David Miller on my very
earliest (October) netdev posts, I've conspicuously avoided contradicting
him.  I've merely *obeyed* his injunction here, and moved on....

The patch itself neutrally documents a coding requirement decision by that
networking maintainer by name.


> 2) u32 =>  int to ameliorate your static checker's complaints

Good idea.  Actually, I simply looked at the code and its history.


> 3) cleanups
>
Removing this function is really a *bug* fix (in several places), with
cleanups in the vicinity for obviously poor coding (variants in 3 places):

-	mss = 0;
-	if ((mss = skb_shinfo(skb)->gso_size) != 0) {
-		int tcp_opt_len, ip_tcp_len;

Cleaner as:

+	mss = skb_shinfo(skb)->gso_size;
+	if (mss != 0) {
+		struct tcphdr *th;

But I wouldn't have bothered had I not been changing that immediately
following line.  30+ years of experience with collaborative projects
informs me that it's best to make minor cleanups only where I'm already
improving the code nearby.  Otherwise, it creates patch conflicts.


> People have already explained that tcp_optlen() doesn't return
> negative values.

People?  The fact that the calculation itself can be negative appeared
the very first time I tested my own code using this bad function!


> negative values.  It would really help us if you could show how
> tcp_hdr(skb)->doff can be less than 5?
>
Oh, I've long since given up on lengthy explanations.  Both Eric and
Ilpo have repeatedly castigated me for being too wordy.

In this particular instance, I suggest that you take a look at all the
places that gso_size is set, and cross index with all the code paths that
place these TCP headers onto the txq without a check of doff -- as I did!

I'll specifically mention the tun and virtio_net devices, but I'm also
particularly concerned with af_packet.c and skbuff.c -- and the general
problem with inet_lro.c, too.

Amazingly enough, folks sometimes use Linux for routers....

Re: [PATCH 0/7] tcp: bugs and cleanup for 2.6.34-rc1

[Américo Wang]

On Thu, Mar 11, 2010 at 12:38:17PM -0500, William Allen Simpson wrote:
> On 3/11/10 10:01 AM, Eric Dumazet wrote:
>> It would be nice if you could update your knowledge of how linux
>> development works these days.
>>
> Perhaps you could supply pointers to the relevant documentation?
>
>

Documentation/development-process/* are still nice documents.

Re: [PATCH v4 2/7] net: remove old tcp_optlen function

[Eric Dumazet]

Le vendredi 12 mars 2010 à 18:05 -0500, William Allen Simpson a écrit :

> In this particular instance, I suggest that you take a look at all the
> places that gso_size is set, and cross index with all the code paths that
> place these TCP headers onto the txq without a check of doff -- as I did!
> 
> I'll specifically mention the tun and virtio_net devices, but I'm also
> particularly concerned with af_packet.c and skbuff.c -- and the general
> problem with inet_lro.c, too.
> 
> Amazingly enough, folks sometimes use Linux for routers....
> --

David already pointed out fact that this code path is not used in
forwardind / routing path. Your assumptions are clearly wrong.

Can you sit down and understand this difference ?

Only *locally* generated trafic by linux kernel can enter this path.

And if a bug in linux core network stack can feed any driver a buggy
skb, bad things can happen, even if a driver is perfect.

Please point out _this_ bug _if_ it really exists, so that we can
correct this bug instead of hiding it in one thousand of drivers.

Your attacks make no sense, you know nothing about linux kernel
internals and assume it was written like other projects you were
involved to.

Re: [PATCH v4 2/7] net: remove old tcp_optlen function

[William Allen Simpson]

On 3/13/10 4:11 AM, Eric Dumazet wrote:
> David already pointed out fact that this code path is not used in
> forwardind / routing path. Your assumptions are clearly wrong.
>
> Can you sit down and understand this difference ?
>
> Only *locally* generated trafic by linux kernel can enter this path.
>
Since you agree with David, perhaps you could kindly point out exactly
how your statement is true?  Proof by assertion is generally not good
argumentation.  I'll start a new thread for you to discuss.

More importantly, how is this relevant to this patch?

This patch removes a seldom used function that generates bad code, and
MUST NOT be used elsewhere in the code base -- an attractive nuisance,
speaking in legal terms.

This patch is also more elegant, and generates faster code.

Are you saying this patch is incorrect in any way?

Please point to the bad line of code (not David's comments or diatribe).
Certainly, I'll be happy to correct it!


> And if a bug in linux core network stack can feed any driver a buggy
> skb, bad things can happen, even if a driver is perfect.
>
Certainly.  But a driver cannot depend on the entirety of a large
project being perfect.  It *should* be perfect in and of itself!


> Please point out _this_ bug _if_ it really exists, so that we can
> correct this bug instead of hiding it in one thousand of drivers.
>
Again, I'll start a new thread for your edification.


> Your attacks make no sense, you know nothing about linux kernel
> internals and assume it was written like other projects you were
> involved to.
>
I've made no "attacks".  The above is a personal /ad hominem/ attack,
evidently the raison d'etre for Linux email lists.  :-(

Yes, I've been involved in many other projects, and am arguably the
most widely experienced TCP/IP implementer in the world.  I've been
programming in C since 1977, and my first TCP/IP implementation was
started in 1979 (in assembly).  Probably before you were born.

Why are you so threatened by that?

Why are you insulting and trying to drive me away?

Why, oh why, is there such a lack of community spirit here?!?!

Re: [PATCH v4 2/7] net: remove old tcp_optlen function

[Eric Dumazet]

Le samedi 13 mars 2010 à 06:12 -0500, William Allen Simpson a écrit :

> Since you agree with David, perhaps you could kindly point out exactly
> how your statement is true?  Proof by assertion is generally not good
> argumentation.
> 


You have to bring proof of your claims, not the reverse.

You started the whole game, adding whole universe into this minor netdev
discussion for an obscure and yet to be aknowledged bug, not me.

Therefore, I wont continue this trolling. You apparently want some
public exposure I have no idea why.

Have a good week end.