Re: [PATCH 0/7] tcp: bugs and cleanup for 2.6.33

Previous thread: [PATCH] Export smbios strings associated with onboard devices to sysfs by Narendra K on Thursday, February 25, 2010 - 1:29 pm. (12 messages)

Next thread: C/R: Checkpoint and restore network namespaces and devices by Dan Smith on Thursday, February 25, 2010 - 1:43 pm. (24 messages)
From: William Allen Simpson
Date: Thursday, February 25, 2010 - 1:30 pm

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....

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

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 Feb 24 2.6.33 release.]
--

From: William Allen Simpson
Date: Thursday, February 25, 2010 - 1:34 pm

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(-)
From: Ben Hutchings
Date: Thursday, February 25, 2010 - 1:43 pm

I don't see  the point of this cast; the left operand of the subtraction
will in any case be promoted to size_t to match the right operand.

Did you mean
	return (int)(th->doff * 4 - sizeof(*th));
?

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--

From: William Allen Simpson
Date: Thursday, February 25, 2010 - 2:05 pm

Kindly ignore this 1st patch, as I missed the subject line.  The proper
patch is in the next message with the proper subject.
--

From: William Allen Simpson
Date: Thursday, February 25, 2010 - 1:35 pm

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


From: William Allen Simpson
Date: Thursday, February 25, 2010 - 2:03 pm

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

Fix #define spacing for TCP options.

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

Parse cookie pair extended option (previously defined).

Handle header extension.

Fix initialization in tcp_minisocks.

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

Signed-off-by: William.Allen.Simpson@gmail.com
---
  include/linux/tcp.h      |   12 ++++-
  include/net/tcp.h        |   45 +++++++++--------
  net/ipv4/tcp_input.c     |  123 ++++++++++++++++++++++++++++++++++++++++++----
  net/ipv4/tcp_minisocks.c |    6 ++
  4 files changed, 152 insertions(+), 34 deletions(-)
From: William Allen Simpson
Date: Thursday, February 25, 2010 - 1:39 pm

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.

No response from testers in 19+ 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(-)
From: Ben Hutchings
Date: Thursday, February 25, 2010 - 1:55 pm

There's no need for these passive-aggressive comments and they certainly

It would be far more helpful to add a comment to the top of this block

[...]

That *is* clearer.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--

From: William Allen Simpson
Date: Thursday, February 25, 2010 - 1:44 pm

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

Reduces multiply/shifts, marginally improving speed.

Removes redundant tcp header length checks before checksumming.

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

Stand-alone patch, originally developed for TCPCT.

Signed-off-by: William.Allen.Simpson@gmail.com
Reviewed-by: Andi Kleen <andi@firstfloor.org>
---
  include/net/xfrm.h  |    7 ++++++
  net/ipv4/tcp_ipv4.c |   45 +++++++++++++++++++++-----------------
  net/ipv6/tcp_ipv6.c |   59 ++++++++++++++++++++++++++++----------------------
  3 files changed, 65 insertions(+), 46 deletions(-)
From: William Allen Simpson
Date: Thursday, February 25, 2010 - 1:49 pm

Fix incorrect header prediction flags documentation.

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

Eliminate unused len parameters in two other functions.

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

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

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

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

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

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

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

Stand-alone patch, originally developed for TCPCT.

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

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


From: William Allen Simpson
Date: Thursday, February 25, 2010 - 1:56 pm

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

This is a straightforward re-implementation of an earlier (year-old)
patch that no longer applies cleanly, with permission of the original
author (Adam Langley).  The patch was previously reviewed:

    http://thread.gmane.org/gmane.linux.network/102586

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

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

Signed-off-by: William.Allen.Simpson@gmail.com
---
  net/ipv4/tcp_input.c |   26 +++++++++++++++++++++++++-
  1 files changed, 25 insertions(+), 1 deletions(-)
From: William Allen Simpson
Date: Thursday, February 25, 2010 - 2:01 pm

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

Prepare (future) error return.

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

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

Signed-off-by: William.Allen.Simpson@gmail.com
---
  include/net/tcp.h        |    3 +-
  net/ipv4/syncookies.c    |    9 ++-
  net/ipv4/tcp_input.c     |  223 ++++++++++++++++++++++++++--------------------
  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, 172 insertions(+), 114 deletions(-)
From: David Miller
Date: Thursday, February 25, 2010 - 10:33 pm

From: William Allen Simpson <william.allen.simpson@gmail.com>

Your patches add as many bugs and problems as they claim to solve.

You also attack, in your commit messages and code coments, the
very people you want to look at your changes and integrate them.

How you hope to make forward progress in these circumstances is
beyond me.

Just remember William: Whilst people have a right to say whatever
they want, they must earn the privilege to being listened to.

And currently many people have you set strictly to ignore.
--

Previous thread: [PATCH] Export smbios strings associated with onboard devices to sysfs by Narendra K on Thursday, February 25, 2010 - 1:29 pm. (12 messages)

Next thread: C/R: Checkpoint and restore network namespaces and devices by Dan Smith on Thursday, February 25, 2010 - 1:43 pm. (24 messages)