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

Submitted by:  Simon Goldschmidt <goldsimon>
Submitted on:  Thu 29 Sep 2011 06:57:11 PM UTC  
 
Category: TCPSeverity: 3 - Normal
Item Group: Crash ErrorStatus: Works For Me
Privacy: PublicAssigned to: Kieran Mansley <kieranm>
Open/Closed: ClosedPlanned Release: None
lwIP version: CVS Head

(Jump to the original submission Jump to the original submission)

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

Agreed.

Kieran Mansley <kieranm>
Project AdministratorIn charge of this item.
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>
Project 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>
Project 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>
Project AdministratorIn charge of this item.
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>
Project 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>
Project 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>
Project Administrator

 

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by kieranm (Posted a comment)
  • -unavailable- added by goldsimon (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 2 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Tue 01 Nov 2011 12:49:08 PM UTCkieranmStatusNone=>Works For Me
      Open/ClosedOpen=>Closed

    Back to the top


    Powered by Savane 3.1-cleanup1