[PATCH 0/2] net: replace buggy tcp_optlen, and cleanup

[PATCH 0/2] net: replace buggy tcp_optlen, and cleanup

[William Allen Simpson]

This bugginess was reported in October, November, December, and
January.  I'm requesting some independent review and testing.

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

-static inline unsigned int tcp_optlen(const struct sk_buff *skb)
-{
-	return (tcp_hdr(skb)->doff - 5) * 4;
-}
-

This is replaced by tcp_header_len_th() and tcp_option_len_th().

The tcp_optlen() function is used *only* in two drivers, that
also have rather messy coding practices; such as:

-	if ((mss = skb_shinfo(skb)->gso_size)) {
...
-	} else
-		mss = 0;

Or:

-	mss = 0;
-	if ((mss = skb_shinfo(skb)->gso_size) != 0) {

Or:

-			iph->tot_len = htons(mss + ip_tcp_len + tcp_opt_len);
-			hdrlen = ip_tcp_len + tcp_opt_len;

Or mixing word and byte sized variables, without using defined constants
or useful sizeof() to make the code self-documenting:

-			if (tcp_opt_len || (iph->ihl > 5)) {
-				vlan_tag_flags |= ((iph->ihl - 5) +
-						   (tcp_opt_len >> 2)) << 8;
-			}

Stand-alone patch, originally developed for TCPCT.

Signed-off-by: William.Allen.Simpson@gmail.com

Re: [PATCH 1/2] net: tcp_header_len_th and tcp_option_len_th

[William Allen Simpson]

[-- Attachment #1: Type: text/plain, Size: 360 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+1v3.patch --]
[-- Type: text/plain, Size: 719 bytes --]

diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index 7fee8a4..d0133cf 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -223,6 +223,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

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

[William Allen Simpson]

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

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.

Untested.  No response from testers in 10+ weeks.

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+2v3.patch --]
[-- Type: text/plain, Size: 6936 bytes --]

diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
index 65df1de..45452c5 100644
--- a/drivers/net/bnx2.c
+++ b/drivers/net/bnx2.c
@@ -6352,6 +6352,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, per David Miller (see commit log).
  */
 static netdev_tx_t
 bnx2_start_xmit(struct sk_buff *skb, struct net_device *dev)
@@ -6396,19 +6398,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;
@@ -6421,14 +6423,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 3a74d21..0fd8182 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -5421,6 +5421,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, per David Miller (see commit log).
  */
 static netdev_tx_t tg3_start_xmit(struct sk_buff *skb,
 				  struct net_device *dev)
@@ -5456,9 +5458,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) &&
@@ -5466,18 +5468,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) {
@@ -5491,7 +5491,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)
@@ -5624,6 +5624,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, per David Miller (see commit log).
  */
 static netdev_tx_t tg3_start_xmit_dma_bug(struct sk_buff *skb,
 					  struct net_device *dev)
@@ -5663,20 +5665,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));
@@ -5688,13 +5691,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;
@@ -5705,19 +5709,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 d0133cf..74728f7 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -218,11 +218,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 1/2] net: tcp_header_len_th and tcp_option_len_th

[Eric Dumazet]

Le 06/01/2010 22:28, William Allen Simpson a écrit :
> 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(-)
> 

Its better to inline your patches so that we can comment them, without copy/paste

When I hit 'reply to', my mailer only quoted the ChangeLog, not the patch.

Anyway ..

+/* 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);
+}


The (int) cast is not necessary, since the function returns a signed int

->
	return th->doff * 4 - sizeof(*th);

Re: [PATCH 1/2] net: tcp_header_len_th and tcp_option_len_th

[William Allen Simpson]

Eric Dumazet wrote:
> Its better to inline your patches so that we can comment them, without copy/paste
> 
> When I hit 'reply to', my mailer only quoted the ChangeLog, not the patch.
> 
Seeing that we're both using Mozilla, how to you do it?

It took me many attempts to get this to work with Thunderbird on the Mac.


> Anyway ..
> 
> +/* 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);
> +}
> 
> 
> The (int) cast is not necessary, since the function returns a signed int
> 
> ->
> 	return th->doff * 4 - sizeof(*th);
> 
Then GCC must be smarter than it was in the past, as doff is an __u16
bit slice -- once upon a time, a cast was required before subtraction.
One of the dis/advantages of C programming for 30+ years is that my
fingers remember some fairly old practices....

(But this still works properly!)

Re: [PATCH 1/2] net: tcp_header_len_th and tcp_option_len_th

[Eric Dumazet]

Le 12/01/2010 18:42, William Allen Simpson a écrit :
> Eric Dumazet wrote:
>> Its better to inline your patches so that we can comment them, without
>> copy/paste
>>
>> When I hit 'reply to', my mailer only quoted the ChangeLog, not the
>> patch.
>>
> Seeing that we're both using Mozilla, how to you do it?
> 
> It took me many attempts to get this to work with Thunderbird on the Mac.
> 

Hmm, I followed Documentation/email-clients.txt tricks, I dont remember exact details.

I type my Changelog text, add my signature, then copy/paste patch from external editor
(this editor must preserve tabulations of course)

Its probably a bit odd, because I am stuck with a windows XP notebook here at work,
dont flame me :(



About cast games, maybe following way is the cleanest one.

int tcp_options_len_th(struct tcphdr *th)
{
	return tcp_header_len_th(th) - sizeof(*th);
}

Re: [PATCH 1/2] net: tcp_header_len_th and tcp_option_len_th

[Jarek Poplawski]

Eric Dumazet wrote, On 01/12/2010 06:53 PM:

> Its probably a bit odd, because I am stuck with a windows XP notebook here at work,
> dont flame me :(

You should be ashamed! (Linus promoted Windows 7 long time ago ;-)

Jarek P.

Re: [PATCH 1/2] net: tcp_header_len_th and tcp_option_len_th

[William Allen Simpson]

Eric Dumazet wrote:
> I type my Changelog text, add my signature, then copy/paste patch from external editor
> (this editor must preserve tabulations of course)
> 
That doesn't work properly on a Mac, copying from BBEdit to Thunderbird.

BBEdit preserves tabs and even understands and preserves Unix LF (and
I've been using it for a Unix editor since it was included with Xinu in
early '90s), but the MacOS copy and paste seems to mangle it.

I'll try again someday with Thunderbird 3, when it's had time to mature.


> About cast games, maybe following way is the cleanest one.
> 
> int tcp_options_len_th(struct tcphdr *th)
> {
> 	return tcp_header_len_th(th) - sizeof(*th);
> }
> 
If you'd have been one of my C students, you'd have failed the exam
question.  That's unsigned int tcp_header_len_th() -- subtracting an
untyped constant could be a negative number (stored in an unsigned).
Then demotion to int (which many compilers truncate to a very large
positive number).

It's one of the reasons that folks used to do all this with macros, so
that the types and truncation were handled well by the compiler.

Of course, this is an inline function, which is more like macros.  I've
not studied how gcc works internally since egcs.

Let's keep (int)(th->doff * 4) - sizeof(*th) -- self documenting, and
should work with a wide variety of compilers.

Re: [PATCH 1/2] net: tcp_header_len_th and tcp_option_len_th

[Eric Dumazet]

Le 13/01/2010 09:53, William Allen Simpson a écrit :
> Eric Dumazet wrote:
> 
>> About cast games, maybe following way is the cleanest one.
>>
>> int tcp_options_len_th(struct tcphdr *th)
>> {
>>     return tcp_header_len_th(th) - sizeof(*th);
>> }
>>
> If you'd have been one of my C students, you'd have failed the exam
> question.  That's unsigned int tcp_header_len_th() -- subtracting an
> untyped constant could be a negative number (stored in an unsigned).
> Then demotion to int (which many compilers truncate to a very large
> positive number).

Thats simply not true, you are very confused.

Once again you type too much text and ignore my comments.
I really would hate being your student, thanks God it wont happen in this life.

1) You wrote tcp_header_len_th(), you should know that it
returns an unsigned int.

2) You also should know that sizeof() is *strongly* typed (size_t),
not an "untyped constant".

unsigned int arg = some_expression;
size_t sz = sizeof(something)
int res = arg - sz;
return res;

is *perfectly* legal and very well defined by C standards.

It *will* return a negative value is arg < sz



> 
> It's one of the reasons that folks used to do all this with macros, so
> that the types and truncation were handled well by the compiler.
> 
> Of course, this is an inline function, which is more like macros.  I've
> not studied how gcc works internally since egcs.
> 
> Let's keep (int)(th->doff * 4) - sizeof(*th) -- self documenting, and
> should work with a wide variety of compilers.

So you wrote tcp_header_len_th(), but you keep (th->doff * 4) thing all over
the code...

The (int) cast it not only _not_ needed, its also confusing.

Re: [PATCH 1/2] net: tcp_header_len_th and tcp_option_len_th

[William Allen Simpson]

Eric Dumazet wrote:
  2) You also should know that sizeof() is *strongly* typed (size_t),
> not an "untyped constant".
> 
My apologies, it's fairly early in the morning here -- I meant
"unsigned" rather than "untyped".


> The (int) cast it not only _not_ needed, its also confusing.
> 
I'm sorry for your confusion.  I believe it adds clarity.

Moreover, it's fairly egregious that the old tcp_hdrlen()
contributor didn't take signed versus unsigned into account.

Perhaps we could move along to more substantive issues....

Have you had an opportunity to test PATCH 2/2 in this series?