TCP Reassembly Issues

[Lawrence Stewart]

Hi Jeremy,

On 11/26/11 18:56, Jeremy Chadwick wrote:
> On Sat, Nov 26, 2011 at 12:49:24AM -0600, Kris Bauer wrote:
>> On Fri, Nov 25, 2011 at 11:23 PM, Lawrence Stewart<lstewart at freebsd.org>wrote:
>>
>>> On 11/25/11 13:01, Lawrence Stewart wrote:

[snip]

>>>> Thanks Kris, Raul and Stefan for the reports, I'll look into this.
>>>>
>>>
>>> I think I've got it - a stupid 1 line logic bug. My apologies for missing
>>> it when I reviewed the patch which introduced the bug (patch was committed
>>> to head as r226113, MFCed to stable/9 as r226228).
>>>
>>> Due to some miscommunication, the initial patch was committed to and MFCed
>>> from head much later than it should have been in the 9.0 release cycle and
>>> instead of being included in the BETAs, didn't make it in until 9.0-RC1 I
>>> believe i.e. only RC1 and RC2 should be experiencing the issue.
>>>
>>> Could those who have reported the bug and are able to recompile their
>>> kernel to test a patch please try the following and report back to the list:
>>>
>>>
>>> http://people.freebsd.org/~lstewart/patches/misctcp/tcp_reass_plugzoneleak_10.x.r227986.patch
>>>
>>> The patch is against head r227986 but will apply and work correctly for
>>> 9.0 as well.
>>>
>>> Cheers,
>>> Lawrence
>>>
>>
>> I have patched, recompiled, and rebooted.  net.inet.tcp.reass.cursegments
>> is no longer incrementing, and connectivity is holding steady.  If anything
>> changes over the next couple of hours, I'll be sure to report it; but all
>> preliminary signs of the problem are gone.
>>
>> Thanks for all the help!
>
> Let's not be hasty in concluding everything is fixed.  Why I'm a bit on
> edge about this: I took the time to find the CVS commits that induced
> this issue in the first place, and it seems there is some history.
>
> The commit that caused this problem to begin with was supposedly a fix
> for a different problem:
>
> http://www.freebsd.org/cgi/cvsweb.cgi/src/sys/netinet/tcp_reass.c#rev1.375

The original patch you reference (equivalent to svn r226113 as noted in 
my previous email) was indeed for a separate problem. Unfortunately the 
fix introduced a new problem.

> A week later, that commit went from HEAD/MAIN into RELENG_9:
>
> http://www.freebsd.org/cgi/cvsweb.cgi/src/sys/netinet/tcp_reass.c#rev1.374.2.2
>
> Be sure to read the description of the problem that was being fixed in
> the first place.  I've also CC'd the original problem reporter, Steven
> Hartland, because we're going to need him to try the above patch from
> Lawrence to make sure there aren't other problems.  Meaning: for all we
> know, the above fix might work great for Kris but cause problems for
> Steve.

Even though my patch is a multi-line diff, it only effectively changes 
one thing - that the "te == NULL" condition must be true for both the 
case where "th->th_seq != tp->rcv_nxt" (current segment does not plug 
the hole) and where they are equal (current segment does plug the hole; 
a new case introduced in r226113). I can say with confidence based on 
the change in the logic that my patch is not a regression as far as 
Steven's original bug report is concerned.

> This entire situation leads me to believe very few people are doing
> quality testing of RELENG_9, yet we're already into 9.0-RC2.  Please
> don't tell me "that's exactly why you should be running RELENG_9!"; that
> is completely backwards and I refuse to get into a flame war about it,
> because it's this simple: 90%+ of those running FreeBSD on servers need
> something that's stable, we can't risk wonkiness (especially of this
> degree!) on systems taking production traffic.  Did no one actually test
> the change *thoroughly*?  Imagine had this lay dormant until 9.0-RELEASE.

The latter half is fair criticism, more comments below. The fact we're 
having this discussion now prior to 9.0 being released somewhat negates 
the assertion in the former part of your paragraph.

> Lawrence: please don't take my comments personally or to mean "you broke
> it and caused this mess!"  It's meant to read more along the lines of

All good, not taken personally.

> "you committed a fix for something that broke other bits badly, but
> nobody noticed this, including the original reporter of a different
> problem?  How/why?"  You get the idea.

Your concerns are valid.

To clarify, I did not propose or commit the patch which introduced this 
bug (r226113). Generally speaking, it is a committer's responsibility to 
ensure that a patch which they commit has been sufficiently tested prior 
to commit.

Normally the committer will solicit testing from the original problem 
reporter and do some testing themselves. I believe Steven tested Andre's 
patch and reported to the mailing list that it resolved his immediate 
problem. I was not privy to any other testing conducted by Andre, so 
can't comment further on that.

As to how this could have been missed: TCP is impressively robust, 
capable of working even when it has both arms tied behind its back and 
is missing a leg. It may not work well, but will limp along all the 
same. People tend to notice and report scenarios where something is 
definitively broken far more readily than the more complicated case 
where it degrades to an unreasonable but still working state after a 
non-deterministic amount of time.

Even if basic testing had been performed, it would be fairly easy to 
miss the effects of this bug, which only manifest after a relatively 
long time, except for those on the receiving end of large 
bandwidth-delay product, non-zero-loss TCP connections which would 
exacerbate the issue quicker.

My involvement in all this was that I introduced the original bug 
r226113 was designed to solve, and reviewed a pre-cursor to, but did not 
test the r226113 patch.

Hope I've provided some useful insight.

Cheers,
Lawrence