tcp: bugs and cleanup for 2.6.34-rc1
Subsystems (4)
cover tcp: bugs and cleanup for 2.6.34-rc1
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
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.
--- include/linux/tcp.h | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-)
include/linux/tcp.h +12 -0
@@ -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;
Signature · 1 line
1.6.3.3
PATCH v4 2/7 net: remove old tcp_optlen function
In reply to William Allen SimpsonThe 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
--- 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
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
--- drivers/net/bnx2.c | 29 +++++++++++++----------- drivers/net/tg3.c | 60 +++++++++++++++++++++++--------------------------- include/linux/tcp.h | 5 ---- 3 files changed, 44 insertions(+), 50 deletions(-)
drivers/net/bnx2.c +16 -13
@@ -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)) {
drivers/net/tg3.c +28 -32
126 lines not on this page · raw message
include/linux/tcp.h +0 -5
@@ -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) {
Signature · 1 line
1.6.3.3
Re: [PATCH v4 2/7] net: remove old tcp_optlen function
In reply to William Allen SimpsonQuoted 5 lines · 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.
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.
Quoted 30 lines
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_thandtcp_option_len_thSigned-off-by [email protected]CC Michael Chan <[email protected]>--- 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 [email protected]
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
Quoted 12 lines · Simon Horman
Quoted 5 lines · 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.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.
Quoted 4 lines
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
In reply to William Allen SimpsonThe 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
--- 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]
drivers/net/bnx2.c +16 -13
@@ -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)) {
drivers/net/tg3.c +28 -32
126 lines not on this page · raw message
include/linux/tcp.h +0 -5
@@ -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) {
Signature · 1 line
1.6.3.3
Re: [PATCH v4 2/7] net: remove old tcp_optlen function
Quoted 10 lines · 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.
Quoted 7 lines
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
Quoted 235 lines
Requires:
net:tcp_header_len_thandtcp_option_len_thSigned-off-by [email protected]CC Michael Chan <[email protected]>--- 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]
drivers/net/bnx2.c +16
-13@@ -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)) {drivers/net/tg3.c +28
-32@@ -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 include/linux/tcp.h +0
-5@@ -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) {Signature · 1 line
1.6.3.3
Re: [PATCH v4 2/7] net: remove old tcp_optlen function
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.
Quoted 1 line · Dan Carpenter
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.
Quoted 1 line
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.
Quoted 1 line
2) u32 => int to ameliorate your static checker's complaints
Good idea. Actually, I simply looked at the code and its history.
Quoted 2 lines
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.
Quoted 2 lines
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!
Quoted 3 lines
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 v4 2/7] net: remove old tcp_optlen function
Quoted 10 lines · William Allen Simpson
In this particular instance, I suggest that you take a look at all the places that
gso_sizeis 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_netdevices, but I'm also particularly concerned withaf_packet.candskbuff.c-- and the general problem withinet_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
Quoted 7 lines · Eric Dumazet
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!
Quoted 3 lines
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!
Quoted 3 lines
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.
Quoted 4 lines
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
Quoted 4 lines · William Allen Simpson
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.
PATCH v6 3/7 tcp: harmonize tcp_vx_rcv header length assumptions
In reply to William Allen SimpsonHarmonize 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.
--- include/net/xfrm.h | 7 +++++ net/ipv4/tcp_ipv4.c | 57 ++++++++++++++++++++++++--------------- net/ipv6/tcp_ipv6.c | 72 +++++++++++++++++++++++++++++++------------------- 3 files changed, 87 insertions(+), 49 deletions(-)
include/net/xfrm.h +7 -0
@@ -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,
net/ipv4/tcp_ipv4.c +35 -22
135 lines not on this page · raw message
net/ipv6/tcp_ipv6.c +45 -27
145 lines not on this page · raw message
Signature · 1 line
1.6.3.3
PATCH v6 4/7 tcp: input header length, prediction, and timestamp bugs
In reply to William Allen SimpsonFix 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
--- 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(-)
include/linux/tcp.h +5 -1
@@ -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;
include/net/tcp.h +13 -5
@@ -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); }
net/ipv4/tcp_input.c +39 -57
169 lines not on this page · raw message
net/ipv4/tcp_ipv4.c +2 -2
@@ -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; }
net/ipv4/tcp_minisocks.c +1 -2
@@ -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 );
net/ipv4/tcp_probe.c +1 -1
@@ -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);
net/ipv6/tcp_ipv6.c +2 -2
@@ -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)
Signature · 1 line
1.6.3.3
PATCH v3 5/7 TCPCT part 2e: accept SYNACK data
In reply to William Allen SimpsonWhen 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
--- net/ipv4/tcp_input.c | 26 +++++++++++++++++++++++++- 1 files changed, 25 insertions(+), 1 deletions(-)
net/ipv4/tcp_input.c +25 -1
@@ -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 ; }
Signature · 1 line
1.6.3.3
PATCH v6 6/7 TCPCT part 2f: cleanup tcp_parse_options
In reply to William Allen SimpsonSplit 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
--- 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(-)
include/net/tcp.h +3 -2
@@ -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);
net/ipv4/syncookies.c +6 -3
@@ -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);
net/ipv4/tcp_input.c +139 -99
301 lines not on this page · raw message
net/ipv4/tcp_ipv4.c +7 -3
@@ -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;
net/ipv4/tcp_minisocks.c +19 -7
@@ -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. */
net/ipv6/syncookies.c +6 -3
@@ -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);
net/ipv6/tcp_ipv6.c +5 -1
@@ -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 &&
Signature · 1 line
1.6.3.3
PATCH v8 7/7 TCPCT part 2g: parse cookie pair and 64-bit timestamp
In reply to William Allen SimpsonEvery 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
--- 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(-)
include/linux/tcp.h +10 -2
@@ -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
include/net/tcp.h +23 -22
62 lines not on this page · raw message
net/ipv4/tcp_input.c +116 -11
202 lines not on this page · raw message
net/ipv4/tcp_minisocks.c +6 -0
@@ -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 */ }
Signature · 1 line
1.6.3.3
Quoted 17 lines · 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).
Quoted 8 lines
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.
<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.
Quoted 3 lines · Eric Dumazet
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?
Quoted 3 lines
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.
Quoted 5 lines
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.
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!
Quoted 10 lines
<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.
Quoted 4 lines
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.
Quoted 3 lines
Speaking for myself, but as your previous mails were ignored, I felt it was time to clarify the points.
Thank you.
Quoted 1 line · William Allen Simpson
In that patch, it might be better to use
u16 s_data_desired = 0; not int s_data_desired = 0;
Quoted 8 lines · Joe Perches
Quoted 1 line · William Allen Simpson
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....
Quoted 7 lines · William Allen Simpson
Quoted 3 lines · Eric Dumazet
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.