buglwIP - A Lightweight TCP/IP stack - Bugs: bug #27209, tcp_input discards frames even...

 
 

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

bug #27209: tcp_input discards frames even though they are within the announced window

Submitter:  Åke Rehnman <rehnmaak>
Submitted:  Sun 09 Aug 2009 02:25:24 PM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Planned Release:  None
lwIP version:  CVS Head

Jump to the original submission

Wed 12 Aug 2009 08:37:07 AM UTC, comment #23: 

Checked in, thanks for reporting and testing.

Microsoft may have made a mistake, or just tried to do things differently intentionally.  The windows stack is not very conformant to the standards, but usually gets away with it.

Kieran Mansley <kieranm>
Group Member
Tue 11 Aug 2009 09:57:53 PM UTC, comment #22: 

Ok, I tested the patch and it seem to work as expected. Even though windows xp is overrunning our window lwip just discards the bytes outside the window. After that a zero window is advertised.

I also fired up my linux box and did the same thing and linux is not violating our window after fast retransmission. Did microsoft really make a mistake here?


Åke Rehnman <rehnmaak>
Group Member
Tue 11 Aug 2009 02:30:49 PM UTC, comment #21: 

OK, I'll make those changes to my copy, and yes, it implements solution (2)

Kieran Mansley <kieranm>
Group Member
Tue 11 Aug 2009 02:11:21 PM UTC, comment #20: 

Reading the patch, I think we can drop the second "if (TCPH_FLAGS(inseg.tcphdr) & TCP_FIN)" (inside the "else if (TCP_SEQ_LEQ(pcb->ooseq->tcphdr->seqno, seqno + tcplen))" block) since the FIN flag has already been checked above.

Also, I'd strongly encourage using braces for every if block (even if the block is a single line) to prevent future developer's changes from forgetting them when adding a line.

Last, only to make sure I understood the patch correctly, am I right that it implements solution 2) of comment #9?

Unfortunately, I cannot really test the patch currently, but from reading, the rest seems OK.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 11 Aug 2009 01:51:47 PM UTC, comment #19: 

Updated patch to correctly handle FIN that gets trimmed from the packet.  This has turned out to be a much larger change than I first imagined, so I would welcome testing and code review, particularly as we're so close to the next release.

(file #18554)

Kieran Mansley <kieranm>
Group Member
Tue 11 Aug 2009 09:34:59 AM UTC, comment #18: 

The number of ACKs sent shouldn't have any effect.  The other end might use this to increase their idea of the congestion window, but the receive window is specified by the values in the packet headers not the number of packets.  I suppose that without the extra ACKs the sender retains a small congestion window and so doesn't send the frame that overruns the receive window.

I've attached a patch that might help.  It is untested, but compiles.  I wonder if you could try it out to see if it solves the problem.  I've also added some extra assertions to try and spot this sort of thing happening.

(file #18553)

Kieran Mansley <kieranm>
Group Member
Tue 11 Aug 2009 08:59:52 AM UTC, comment #17: 

I don't have the rest of that capture. But I could make a new one later tonight.

Anyway I was doing some more tests and it seem that the overrun only occurs when we are transmitting more than 3 acks for the missing frame. If I reduce the number of acks for the missing frame it doesn't overrun. Does this have anything to do with the fast retransmission?

Åke Rehnman <rehnmaak>
Group Member
Tue 11 Aug 2009 08:49:29 AM UTC, comment #16: 

OK, so I agree that it looks like the other end is over-running the advertised window in frame 25 of that capture, and this in turn will cause us to incorrectly handle the value of rcv_wnd causing it to wrap.

We should fix the way we handle rcv_wnd.  I'll try and look at that.

The only other point of concern is why the other end is over-running the window.  The only way this could be lwIP's fault is if we'd previously advertised a larger window, and subsequently shrunk it.  I'd need to see some more of that capture (the preceeding 50 or so frames) to make sure that wasn't the case.  Most likely it is just trying its luck and hoping it gets through.

Kieran Mansley <kieranm>
Group Member
Tue 11 Aug 2009 08:39:46 AM UTC, comment #15: 

sorry - my fault, got it now

Kieran Mansley <kieranm>
Group Member
Tue 11 Aug 2009 08:37:29 AM UTC, comment #14: 

I can't open the pcap file: wireshark thinks it is invalid.

Kieran Mansley <kieranm>
Group Member
Mon 10 Aug 2009 05:39:51 PM UTC, comment #13: 

Can someone have a look at the latest capture.

7) At no7 window is updated to 18815.

After that there are 12 frames * 1460 = 17520. The packet no9 is lost.

20) At no20 we get the first ack for frame no9 (which was lost)

21) An other 2 acks for no9

23) At no23 retransmission of frame no9. Here our window should be 18815-12*1460=1295

24) An other ack for no9

25) Next segment (number 13 after our window update). Here the other end is overrunning or window.

I added a debug message where rcv_wnd is decremented and this is what I got:

tcp_receive: rcv_wnd < tcp_len seqno 1295 1460 1209348364

So why is our window overrun?




(file #18552)

Åke Rehnman <rehnmaak>
Group Member
Mon 10 Aug 2009 11:57:03 AM UTC, comment #12: 


> Ultimately there are two possible solutions to this:[..]


You know I'm not the one limited in memory or processing power (until now, at least), so I'd vote for keeping the packet. However, for some devices it could waste too much memory keeping the packet (and converting the u16_t to s32_t makes the code slower on 16-bit architectures, too).

Simon Goldschmidt <goldsimon>
Group administrator
Mon 10 Aug 2009 11:41:30 AM UTC, comment #11: 

I've looked in the packet capture and don't see anything going outside the window.  It goes right up to the right window edge, but doesn't exceed it.

Kieran Mansley <kieranm>
Group Member
Mon 10 Aug 2009 11:35:14 AM UTC, comment #10: 

In that case it is probably something wrong where the out-of-order or retansmission seqments are handled. Because this problem only arises when I have lost frames at link level.

Åke Rehnman <rehnmaak>
Group Member
Mon 10 Aug 2009 11:32:31 AM UTC, comment #9: 

Ultimately there are two possible solutions to this:

1) Continue to allow packets that are a little longer than the current window to be received.  This is a good principle, as we've got the space to buffer it (it's already buffered) and throwing it away would just waste the work done already. To support this fully we'd have to convert rcv_wnd to signed arithmetic, which would probably mean increasing it from a u16 to a s32, and so use a little more space.

2) Trim any received packet that has some part out of the window so that we ignore the "bad" bit.  This would ensure that tcp_wnd would never go out of the range 0 -> TCP_WND, and so the rest of the code could be left as it is.

I'd welcome other peoples preferences.

Kieran Mansley <kieranm>
Group Member
Mon 10 Aug 2009 11:21:00 AM UTC, comment #8: 

This suggests that either tcp_recved() is being called inappropriately, or tcp_wnd is not being decremented correctly earlier in the process of receiving a packet.

The theory is that when a packet is received, tcp_wnd is decremented by that length, and then when the application has finished processing it, tcp_wnd is incremented by the same length.  For tcp_wnd to become larger than TCP_WND in tcp_recved() is therefore wrong, regardless of it causing an overflow or not.

Looking at the code where it is decremented, I see:
          if (pcb->rcv_wnd < TCP_TCPLEN(cseg)) {
            pcb->rcv_wnd = 0;
          } else {
            pcb->rcv_wnd -= TCP_TCPLEN(cseg);
          }

If we don't decrement it fully but set to zero, then when tcp_recved() is later called it will be inflated by too much.  I'm not sure why we're trying to deal with the case of rcv_wnd < tcp_len for a received frame, but this could conceivably be the source of your problem.  Could you try removing the code extract above and instead replace with something that does (untested):

LWIP_ASSERT(pcb->rcv_wnd >= TCP_TCPLEN(cseg), "received segment larger than the window");
pcb->rcv_wnd -= TCP_TCPLEN(cseg);

There are couple of places where this change is necessary: look for wherever rcv_wnd is decremented.

If you can get this tested soon there's a chance it could make it into 1.3.1.  Thanks!

Kieran Mansley <kieranm>
Group Member
Mon 10 Aug 2009 11:18:04 AM UTC, comment #7: 


> If adding a small number like 1460 to a value 65000 of
> pcb->rcv_wnd would be 66460 which is greater than a u16_t can
> handle.


Unfortunately, I didn't have the time to look into the pcaps yet, but the above statement would mean that the other side did overrun our receive window (unless tcp_recved() was called in a wrong way, which maybe we should be able to cope with, even with the maximum window size).

Simon Goldschmidt <goldsimon>
Group administrator
Mon 10 Aug 2009 11:12:10 AM UTC, comment #6: 

My TCP_WND is 65535.

If adding a small number like 1460 to a value 65000 of pcb->rcv_wnd would be 66460 which is greater than a u16_t can handle.

Åke Rehnman <rehnmaak>
Group Member
Mon 10 Aug 2009 11:03:46 AM UTC, comment #5: 

What value of TCP_WND are you using?

I can only see this overflowing if:
1) TCP_WND was set to an unsupported value (greater than a u16_t can handle).
2) tcp_recved() was called by the application or higher API layer with too large a value for len, or too often.

I thought (1) would be caught by the compiler due to the #error code in init.c, so I'm curious as to the value of TCP_WND that showed this up.

Kieran Mansley <kieranm>
Group Member
Mon 10 Aug 2009 10:44:24 AM UTC, comment #4: 

Thanks for spotting this, I'll take a look.

Kieran Mansley <kieranm>
Group Member
Sun 09 Aug 2009 10:36:56 PM UTC, comment #3: 

I've found the problem. In tcp_recved in tcp.c it is possible for the pcb->rcv_wnd to overflow if TCP_WND is large enough. I suggest the calculation should be done with u32_t and then checked if it is > TCP_WND.


Åke Rehnman <rehnmaak>
Group Member
Sun 09 Aug 2009 09:00:36 PM UTC, comment #2: 

The problem is trying to print a lot of info on a slow serial line is going to make us loose a lot of packets. Even so I made a capture and a log file. Only the lines when rcv_wnd < rcv_ann_wnd are printed.


(file #18549, file #18550)

Åke Rehnman <rehnmaak>
Group Member
Sun 09 Aug 2009 08:00:34 PM UTC, comment #1: 

After reading the code, I think it's quite odd that rcv_ann_wnd is grater than rcv_wnd. I would have expected it to be equal or less than rcv_wnd.

Could you
a) try to print rcv_wnd and rcv_ann_wnd when this happens and
b) attach packet captures as pcap files so they can be viewn and analyzed in wireshark (makes reading them much easier)

Simon Goldschmidt <goldsimon>
Group administrator
Sun 09 Aug 2009 02:25:24 PM UTC, original submission:  

tcp_input discards frames if they are outside pcb->rcv_nxt + pcb->rcv_wnd - 1.

This happens if we are loosing packets at link level and rcv_wnd is less than the announced window (why rcv_wnd is less than the announced window I don't know).

If the comparison in tcp_input is changed to pcb->rcv_nxt + pcb->rcv_ann_wnd - 1 instead the problem disappears.

Attached are capture files and a session.txt. It shows what packets are actually sent to lwip. Packets marked with a star are discarded (for test purposes).

Åke Rehnman <rehnmaak>
Group Member

 

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

Attached Files
file #18554:  bug27209 added by kieranm (5KiB - application/octet-stream)
file #18553:  bug27209 added by kieranm (4KiB - application/octet-stream)
file #18552:  rcv_wnd_bug2.pcap added by rehnmaak (30KiB - application/octet-stream)
file #18550:  session.log added by rehnmaak (6KiB - application/octet-stream)
file #18549:  rcv_wnd_bug1.pcap added by rehnmaak (168KiB - application/octet-stream)
file #18544:  session.txt added by rehnmaak (25KiB - text/plain)
file #18543:  rcv_wnd_bug.txt added by rehnmaak (474KiB - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by rehnmaak (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 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-08-12 kieranm StatusNone Fixed
        Open/ClosedOpen Closed
    2009-08-11 kieranm Attached File- Added bug27209, #18554
    2009-08-11 kieranm Attached File- Added bug27209, #18553
    2009-08-10 rehnmaak Attached File- Added rcv_wnd_bug2.pcap, #18552
    2009-08-10 kieranm Planned Release 1.3.1
    2009-08-09 rehnmaak Attached File- Added rcv_wnd_bug1.pcap, #18549
        Attached File- Added session.log, #18550
    2009-08-09 rehnmaak Attached File- Added session.txt, #18544
        Attached File- Added rcv_wnd_bug.txt, #18543

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code