buglwIP - A Lightweight TCP/IP stack - Bugs: bug #20199, TCP appears to be "Shrinking...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #20199: TCP appears to be "Shrinking the Window"

Submitter:  Tom Evans <tom_evans>
Submitted:  Mon 18 Jun 2007 05:03:58 AM UTC
   
 
Category:  TCP Severity:  4 - Important
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  kieranm
Open/Closed:  Closed Planned Release:  None
lwIP version:  None

Jump to the original submission

Tue 04 Mar 2008 01:13:04 PM UTC, comment #10: 

Closing this as fixed due to lack of reports to the contrary.

Kieran Mansley <kieranm>
Group Member
Thu 20 Dec 2007 02:52:13 PM UTC, comment #9: 

I've checked this patch in.  If anyone is in a position to test it that would be most helpful

Kieran Mansley <kieranm>
Group Member
Thu 20 Dec 2007 12:45:51 PM UTC, comment #8: 

I've looked through the patch supplied by Per-Henrik Lundblom and extracted the relevant bits of silly window avoidance and produced a new patch against the current CVS.

(file #14675)

Kieran Mansley <kieranm>
Group Member
Tue 18 Dec 2007 01:51:51 PM UTC, comment #7: 

(from lwip-users, by Per-Henrik Lundblom)

Hi,

I guess it is about time to commit the mods and fixes we have done on the lwIP stack. I have included and attached one large patch (I know it's bad with one patch to fix several problems but take it or leave it). The patch was made against a CVS copy from 2007-09-20.

The patch should fix the following things:

- Silly window problem (bug #20199)

- Implementation of the TCP persist timer which lwIP curretly lacks. The
  persist timer is MANDATORY for any TCP implementation. This
  implementation can be slimmed down. This implementation doesn't share
  any code with the retransmit timer even though it should be possible.
  Also, the tcp_split_unsent_seq() function has code copied from other
  tcp_*() functions. I guess you could leave out the split part of the
  persist timer if you want to reduce code size but then you don't fully
  implement the functionality for the persist timer.

Note that this code was 100% written during work time at connectBlue AB
(www.connectblue.se) but is released under the same license as the rest of lwIP. We really hope it will find its way into the the trunk and that others will find it useful.

Merry Christmas to all of you!

/PH

(@see bug #20511)


(file #14653)

Frédéric Bernon <fbernon>
Group Member
Fri 07 Dec 2007 03:46:44 PM UTC, comment #6: 

If say about current SWS avoidance implementation.
It is not consistent.

in tcp_output_segment() avertised window calculated such way:
  if (pcb->rcv_wnd < pcb->mss) {
    seg->tcphdr->wnd = 0;
  } else {
    /* advertise our receive window size in this TCP segment */
    seg->tcphdr->wnd = htons(pcb->rcv_wnd);
  }
In tcp_output()
for TF_ACK_NOW flag
  tcphdr->wnd = htons(pcb->rcv_wnd);

Oleg Tyshev <olegreen>
Mon 16 Jul 2007 08:30:51 AM UTC, comment #5: 

(comment #4 is a reply to a Tom's email outside the bugtracker. He wrote ):

> Perhaps, even if you can't test it, you could provide us a
> patch file to see how you would change the code to be RFC compliant?


I'm not using TCP at all. We're only using IP and UDP (for TFTP) at the moment.

So I don't have any TCP code at all in the code base I'm working from.

So I couldn't even compile it without creating a separate project.

I've ported TCP before - the Comer/Stephens Xinu code base. That's why I'm interested in it.

And that was horrid. It had RFC1122-busting bugs that had been in there for decades causing problems.

At least there was no active support on it. It has been "abandoned" in the early 1990's. It was only ever meant as running sample code supporting student labs. I can imagine the students having bad problems and searching their code in vain for the problems.

With the right client/server mix it was easy to get a throughput of three BYTES PER SECOND over Ethernet! That was due to Xinu getting the PERSIST state wrong.

Not as bad as lwIP which doesn't support PERSIST at all!

I'm sad to see that lwIP is no better 8 years later. It is worse given how long these bugs have been in there (that it was written from scratch and BSD and others and still written with these holes) and how old the requirements are that it is breaking.

Anyone who is relying on the TCP working should have far more incentive than I have to fix it.

I wasn't joking all that much when I typed ''Anyone using lwIP's TCP with these bugs unfixed who isn't sweating and shaking "doesn't understand the problem".''

Frédéric Bernon <fbernon>
Group Member
Mon 16 Jul 2007 08:22:22 AM UTC, comment #4: 

Thank you Tom for your constructive comments.  I think sweating and shaking because your window might shrink is perhaps getting a little over excited, but then maybe I don't understand the problem!

In the existing code the reason it has gone unfixed for so long is because this bug has very few side-effects.  The window shrinking is bad, don't get me wrong, but there are two possible ways another stack might react:
 i) use the old value of the window (before we shrank it) and send anyway.  I think in this case as it is only the advertised window we've shrunk, rather than the real value, we'll still accept the data and so everything will be OK.
 ii) use the new value of the window and not send a packet when potentially it could.  This will hurt performance when it happens, but if you're about to run out of window anyway, this isn't going to be a big factor.

Note that I'm not justifying the current code or putting an argument for not fixing this, but trying to explain why this bug has remained for so long.

As to the suggested fix being a backwards step, it's hard to work out exactly which MUSTs, SRONGLY_DISCOURAGEDs, etc, you're refering to given that you only quote a year as reference, but assuming you mean that we'll go from swapping the shrinking window problem for a silly window problem I wasn't suggesting that both would be solved by this one change, which is why I mentioned that in comment #1.  I personally prefer to isolate issues and fix them independently.  Sometimes you have two dependent bugs such as in this case where fixing one makes another worse.  That's OK as long as both are tracked and fixed.

Finally, your email follow-up could be taken as something of an attack on the lwIP project and current developers: to paraphrase (and you might not have intended it like this) "your lwIP sucks and is worse than some student project from 15 years ago, why haven't you fixed all the bugs yet?  I'm amazed it can send traffic at all!".  Sadly lwIP does have bugs, and it would have been nice if they had all been fixed long ago.  lwIP has a few active developers who do their best to fix things when reported, often in spare time.  This problem has never been reported before, despite the large community using it, which is one of the reasons it's not been fixed.  Personally I think the most constructive way to improve the code base, which clearly you feel strongly would be good thing, is by doing things like reporting problems, filing this bug report (thanks! and please file any more you find) etc, rather than describing how bad you think things are.

Kieran Mansley <kieranm>
Group Member
Mon 16 Jul 2007 07:04:52 AM UTC, comment #3: 

Hi Tom,

Perhaps, even if you can't test it, you could provide us a patch file to see how you would change the code to be RFC compliant?

Frédéric Bernon <fbernon>
Group Member
Mon 16 Jul 2007 03:19:20 AM UTC, comment #2: 

The current (2007) code looks to be doing something that was "STRONGLY DISCOURAGED in 1981 and also disobeying a "SHOULD NOT" from 1989.

The suggested change would result in it disobeying a "MUST" (in other words changing to do a "MUST NOT") from 1989.

Going from doing something "STRONGLY DISCOURAGED" 26 years ago and "SHOULD NOT" 18 years ago to an 18-year-old "MUST NOT" is probably a backwards step.

This isn't that hard to fix. Just check the BSD, Linux or later Xinu code for examples and follow the RFCs.

I'd fix this except we're not using TCP here, and this sort of thing needs to be tested properly to ensure the window does the right thing under all circumstances. Not hard, just test via 127.0.0.1 with test client and server.

Anyone using lwIP's TCP with these bugs unfixed who isn't sweating and shaking "doesn't understand the problem".

Tom Evans <tom_evans>
Fri 13 Jul 2007 11:56:27 AM UTC, comment #1: 

So I think the solution to this is to replace the code quoted in comment #1 with just
  seg->tcphdr->wnd = htons(pcb->rcv_wnd);

There may then be another bug where we don't prevent the window opening by a small amount.  To solve that we should keep track of what the last window advertised was, and only advertise the new one if the condition quoted in comment #1 is satisfied.

Kieran Mansley <kieranm>
Group Member
Mon 18 Jun 2007 05:03:58 AM UTC, original submission:  

This report is from code inspection. It may be wrong. I have not been able to demonstrate this condition as I don't have a running test harness for TCP (yet). I raised this issue in:

http://lists.nongnu.org/archive/html/lwip-users/2007-06/msg00089.html and Kieran Mansley replied "Could you file a bug report on savannah?". So this is it.

The "silly window avoidance" code in tcp_output_segment() is as follows:

  /* silly window avoidance */
  if (pcb->rcv_wnd < pcb->mss) {
    seg->tcphdr->wnd = 0;
  } else {
    /* advertise our receive window size in this TCP segment */
    seg->tcphdr->wnd = htons(pcb->rcv_wnd);
  }

Assume data is arriving in small chunks and is not being
removed, so the window is getting smaller. Now when (pcb->rcv_wnd < pcb->mss), all of a sudden the window advertised in the transmitted packet (in seg->tcphdr->wnd) looks like it slams shut to zero.

This is mentioned in section "Managing the Window" on Page 42 of the TCP RFC:

    ftp://ftp.rfc-editor.org/in-notes/rfc793.txt

    The mechanisms provided allow a TCP to advertise a
    large window and to subsequently advertise a much
    smaller window without having accepted that much
    data.  This, so called "shrinking the window,"
    is strongly discouraged.

"Requirements for Internet Hosts -- Communication Layers", on Page 91:

    ftp://ftp.rfc-editor.org/in-notes/rfc1122.txt

    4.2.2.16  Managing the Window: RFC-793 Section 3.7, page 41

       A TCP receiver SHOULD NOT shrink the window,
       i.e., move the right window edge to the left.

"SWS Avoidance" (the recommended way to do it) is documented in the following section:

    4.2.3.3  When to Send a Window Update" of RFC1122.

       A TCP MUST include a SWS avoidance algorithm
       in the receiver [TCP:5].

       IMPLEMENTATION:
       The receiver's SWS avoidance algorithm determines
       when the right window edge may be advanced; this
       is customarily known as "updating the window".
       ...
       The suggested SWS avoidance algorithm for the receiver
       is to keep RCV.NXT+RCV.WND fixed until the reduction
       satisfies:

         RCV.BUFF - RCV.USER - RCV.WND  >=
                    min( Fr * RCV.BUFF, Eff.snd.MSS )

       where Fr is a fraction whose recommended value is 1/2,
       and Eff.snd.MSS is the effective send MSS for the
       connection (see Section 4.2.2.6).  When the inequality
       is satisfied, RCV.WND is set to RCV.BUFF-RCV.USER.

In summary, the window should only shrink "naturally" (because of incoming data). SWS Avoidance delays the window OPENING until it can be opened EITHER to the MSS or to half of the receive buffer size.

As a separate item, the lwIP code is only performing the former comparison, and not the latter. If it did both it would also be immune to a misconfiguration where accidentally setting (TCP_WND < TCP_MSS) would lock it up.

The code (by inspection) doesn't calculate pcb->rcv_wnd from the sequence numbers, but only runs it up and down (between zero and TCP_WND) based on data arrival and removal. It looks like it might need a copy of pcb->rcv_wnd that is the "advertised window" and which "lags behind" the "true window", following RFC1122.



Tom Evans <tom_evans>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #14675:  sws_patch added by kieranm (6KiB - application/octet-stream)
file #14653:  lwip_working.diff added by fbernon (21KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by olegreen (Posted a comment)
  • -email is unavailable- added by fbernon (Posted a comment)
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by tom_evans (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2008-03-04 kieranm StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2007-12-31 kieranm StatusNone Ready For Test
    2007-12-20 kieranm Attached File- Added sws_patch, #14675
    2007-12-18 fbernon Attached File- Added lwip_working.diff, #14653
    2007-08-29 kieranm Severity3 - Normal 4 - Important
        Assigned toNone kieranm
    2007-07-13 kieranm Planned Release 1.3.0

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code