buglwIP - A Lightweight TCP/IP stack - Bugs: bug #34435, fast retransmits can result in...

 
 

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

bug #34435: fast retransmits can result in unordered unsent list

Submitter:  Simon Goldschmidt <goldsimon>
Submitted:  Thu 29 Sep 2011 06:57:11 PM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  Crash Error Status:  Works For Me
Privacy:  Public Assigned to:  kieranm
Open/Closed:  Closed Planned Release:  None
lwIP version:  CVS Head

Jump to the original submission

Tue 01 Nov 2011 12:49:08 PM UTC, comment #6: 

Agreed.

Kieran Mansley <kieranm>
Group Member
Sat 29 Oct 2011 07:53:20 PM UTC, comment #5: 

I've added the casts for clarity. I think we can close this as "works for me" and reopen it if someone still finds a bug here.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 28 Oct 2011 01:27:29 PM UTC, comment #4: 


> Perhaps we should be casting (a) and (b) to unsigned?


That would indeed be a safe thing to do. It also better explains what the macro is meant to do.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 28 Oct 2011 01:17:00 PM UTC, comment #3: 

I think I can see the potential problem.  It all hinges on the behaviour of TCP_SEQ_LT.

In tcp_rexmit() we take the first segment on the unacked list and return it to the unsent list.  We attempt to keep the unsent list ordered by comparing sequence numbers using TCP_SEQ_LT.  If this was just a straight comparison of sequence numbers, and the sequence number had recently wrapped, it could put the segment we are moving (with sequence number 0xFFFFFF00) at the tail of the unsent list rather than at the head of the unsent list.

However, the implementation of TCP_SEQ_LT that we currently have will I think give the right answer:

#define TCP_SEQ_LT(a,b) ((s32_t)((a)-(b)) < 0)

e.g. comparing a=0x00000300 to b=0xFFFFFF00 will give:
(a)-(b) = 1024 and so cast to s32_t this is > 0 and TCP_SEQ_LT returns false.  Because it is false the while loop in tcp_rexmit will terminate and it will put 0xFFFFFF00 segment at the head of unsent, not at the tail.

I.e. I agree it is fine.

One way that it could fail is if the reporter's ntohl() implementation wasn't returning unsigned values.  I'm not sure if our TCP_SEQ_LT macro would then give the right answer.  Perhaps we should be casting (a) and (b) to unsigned?

Kieran Mansley <kieranm>
Group Member
Tue 11 Oct 2011 07:53:36 PM UTC, comment #2: 

After coding 2 unit test cases to check unsent/unacked list sorting on fast/rto rexmit and checking the code (segments are only added to unacked in tcp_output), I don't think we have a problem with unsorted lists. We did however have such a problem until 1.3.0, so maybe he used an old version?

Being like that, I'd regard this as fixed or invalid...

Simon Goldschmidt <goldsimon>
Group administrator
Tue 11 Oct 2011 07:51:29 PM UTC, comment #1: 

In bug #32417, tao tang <opensms4> wrote:

When seqno is cycled from 2^32 to 0, it's low possible that the unack_list will become an unordered list.

Supposed that:
1: unack_list is NULL;
2: cwnd >= 8 && MSS == 1024
3: unsent_list is:
packet1: seqno = 0xFFFFFF00;
packet2: seqno = 0x00000300;
packet3: seqno = 0x00000700;
...;
packet8: seqno = 0x00001B00;
4: after tcp_output called, Fast-retransmission occurred(happens on a spurious event) , retransmitting the first segment of unack_list and now the unsent_list is(unordered unack_list):
packet2: seqno = 0x00000300;
packet3: seqno = 0x00000700;
...;
packet8: seqno = 0x00001B00;
packet1: seqno = 0xFFFFFF00;
5: unfortunately, timeout-retransmission occurred, the cwnd is reset to 1.
6: after total TCP_MAXRTX times retransmit packet2, the pcb will be purged.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 29 Sep 2011 06:57:11 PM UTC, original submission:  

This is a "follow up" to bug #32417
(TCP_OVERSIZE seems to have problems with (fast-)retransmission).

That bug was about pcb->unsent_oversize not matching the actual 'oversize' in the last segment on pcb->unsent.

However, during evalutating this bug, 'tao tang' discovered that 'When seqno is cycled from 2^32 to 0, it's low possible that the unack_list will become an unordered list', which is what this bug is about.

I've splitted the two to
a) show that this is a general problem and
b) show that the problem of "pcb->unsent_oversize vs. oversize-of-last-in-pcb->unsent" is fixed.


Simon Goldschmidt <goldsimon>
Group administrator

 

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

No files currently attached

 

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 (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 2 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2011-11-01 kieranm StatusNone Works For Me
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code