login
Search
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.] --
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(-)
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
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(-)
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
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(-)
--
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 --
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." 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! --
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.
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(-)
No response from testers in 21+ weeks.
[removed comment references to commit log]
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, --
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.
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
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
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
People? The fact that the calculation itself can be negative appeared
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 ...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. --
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. But a driver cannot depend on the entirety of a large 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?!?! --
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. --
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. 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.Allen.Simpson@gmail.com Reviewed-by: Andi Kleen <andi@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(-)
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(-)
When 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
Signed-off-by: William.Allen.Simpson@gmail.com
---
net/ipv4/tcp_input.c | 26 +++++++++++++++++++++++++-
1 files changed, 25 insertions(+), 1 deletions(-)
Split 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 Signed-off-by: William.Allen.Simpson@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(-)
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. [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.Allen.Simpson@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(-)
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. http://git2.kernel.org/?p=linux/kernel/git/davem/net-2.6.git;a=commit;h=28b2774a0d5852... <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. --
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, 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 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* 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.... Thank you. --
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.... --
Documentation/development-process/* are still nice documents. --
| Greg KH | Og dreams of kernels |
| Jens Axboe | [PATCH 31/33] Fusion: sg chaining support |
| Arnd Bergmann | Re: finding your own dead "CONFIG_" variables |
| Mark Brown | [PATCH 2/2] Subject: natsemi: Allow users to disable workaround for DspCfg reset |
| Tony Breeds | [LGUEST] Look in object dir for .config |
git: | |
| Brian Downing | Re: Git in a Nutshell guide |
| John Benes | Re: master has some toys |
| Matthias Lederhofer | [PATCH 4/7] introduce GIT_WORK_TREE to specify the work tree |
| Alexander Sulfrian | [RFC/PATCH] RE: git calls SSH_ASKPASS even if DISPLAY is not set |
| Junio C Hamano | Re: Rss produced by git is not valid xml? |
| Linux Kernel Mailing List | iSeries: fix section mismatch in iseries_veth |
| Linux Kernel Mailing List | ixbge: remove TX lock and redo TX |