net: replace buggy tcp_optlen, and cleanup

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.

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

index 7fee8a4..d0133cf 100644
@@ -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

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

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.

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

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);
}

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

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

Jarek P.

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.

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.

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

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.

Eric Dumazet wrote:
2) You also should know that sizeof() is *strongly* typed (size_t),

My apologies, it's fairly early in the morning here -- I meant "unsigned" rather than "untyped".

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?

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

---
 drivers/net/bnx2.c  |   29 +++++++++++++-----------
 drivers/net/tg3.c   |   60 +++++++++++++++++++++++---------------------------
 include/linux/tcp.h |    5 ----
 3 files changed, 44 insertions(+), 50 deletions(-)

index 65df1de..45452c5 100644
@@ -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)) {

index d0133cf..74728f7 100644
@@ -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