[PATCH 0/7] tcp: bugs and cleanup for 2.6.34-rc1
William Allen Simpson
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.]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
William Allen Simpson
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.Al...@gmail.com
---
include/linux/tcp.h | 12 ++++++++++++
1 files changed, 12 insertions(+), 0 deletions(-)
William Allen Simpson
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.Al...@gmail.com
CC: Michael Chan <mc...@broadcom.com>
---
drivers/net/bnx2.c | 29 +++++++++++++-----------
drivers/net/tg3.c | 60 +++++++++++++++++++++++---------------------------
include/linux/tcp.h | 5 ----
3 files changed, 44 insertions(+), 50 deletions(-)
William Allen Simpson
William Allen Simpson
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.Al...@gmail.com
Reviewed-by: Andi Kleen <an...@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(-)
William Allen Simpson
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.Al...@gmail.com
Reviewed-by: Andi Kleen <an...@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(-)
William Allen Simpson
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.Al...@gmail.com
---
net/ipv4/tcp_input.c | 26 +++++++++++++++++++++++++-
1 files changed, 25 insertions(+), 1 deletions(-)
William Allen Simpson
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.Al...@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(-)
William Allen Simpson
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.Al...@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(-)
Eric Dumazet
> 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.
<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.
William Allen Simpson
> It would be nice if you could update your knowledge of how linux
> development works these days.
>
> 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.
Joe Perches
> > 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;
Simon Horman
> 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.
William Allen Simpson
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:
William Allen Simpson
> 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;
>
that ship has sailed, but you could submit a patch....
William Allen Simpson
> 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.
>
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!
Dan Carpenter
> 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.Al...@gmail.com
> CC: Michael Chan <mc...@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
William Allen Simpson
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....
Américo Wang
> 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.
Eric Dumazet
> 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.
William Allen Simpson
> 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.
>
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?!?!
Eric Dumazet
> 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.