buglwIP - A Lightweight TCP/IP stack - Bugs: bug #49533, TCP connection deadlocks upon...

 
 

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

bug #49533: TCP connection deadlocks upon dropped window update due to not filling window

Submitter:  Joel Cunningham <jcunningham>
Submitted:  Fri 04 Nov 2016 06:40:18 PM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  jcunningham
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

Jump to the original submission

Wed 16 Nov 2016 03:19:09 PM UTC, comment #10: 

Patch committed in f79eabd24b0b266d0263b331afdc4df1fc545e22

Joel Cunningham <jcunningham>
Group Member
Wed 16 Nov 2016 08:11:58 AM UTC, comment #9: 


> tcp_output() is called from tcp_input(), so the logic will execute in the case you described.


Right, that was what I missed. In that case I'm OK with your patch.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 15 Nov 2016 02:53:31 PM UTC, comment #8: 

tcp_output() is called from tcp_input(), so the logic will execute in the case you described.

I think if we just had the check in tcp_receive(), we'd have a race condition between the application submitting the next segment and processing the window update.  Consider the following case:

1. Window update comes in, ACKS all in-flight data and reduces the window to < 1 MSS (default segment allocation size)
2. No data is buffered in TCP, send pathway idle
3. Application sends at least 1 MSS of data
4. Logic is triggered via tcp_output() and persist timer is started

Using tcp_output() seemed appropriate because we are starting the persist timer in place of where we would normally fill the remaining window with a segment.  So any execution paths that send segments should be triggering this logic

Joel Cunningham <jcunningham>
Group Member
Tue 15 Nov 2016 11:36:41 AM UTC, comment #7: 

Your patch starts the persist timer in tcp_output(). I wonder if that is enough. Couldn't we come into the same siatuation when we receive a window that is smaller than the next segment to be sent?

Might be as easy as changing tcp_receive() to start the persist timer if "pcb->snd_wnd < pcb->unacked->len" or something like that...

Simon Goldschmidt <goldsimon>
Group administrator
Mon 14 Nov 2016 09:56:13 PM UTC, comment #6: 

Simon,

I've attached a new patch which starts the persist timer in the case we've identified.

I've tested in my environment and the issue is handled correctly.

Should I also update the CHANGELOG bugfix section with an entry for this issue?  I'm not sure what level of bugs get recorded there

(file #38961)

Joel Cunningham <jcunningham>
Group Member
Mon 14 Nov 2016 03:57:13 PM UTC, comment #5: 

Let me sum up what I think would be correct behavior for lwIP:
- RTO timer prevents connection deadlock while unacked != NULL
- persist timer prevents connection deadlock when unacked == NULL and we have a zero-window-like condition (either real zero window or received window too small for next segment to be sent, which might also happen when enqueueing a segment)

Simon Goldschmidt <goldsimon>
Group administrator
Mon 14 Nov 2016 03:53:36 PM UTC, comment #4: 


> this was LwIP's legacy behavior which was removed in bug
> #34517


Right. Sorry I missed your post in that bug 2 weeks ago...

And I'm also sorry I can't remember which bug I referenced there. Too long ago. I should have put the number there!

Anyway, seems like Kieran was right back then and this is what you have found now.

However, the behaviour before that was that RTO and persist timer were running at the same time, so that when persist timer fired, it sent 1 byte of an unacked segment instead of a retransmission. That was wrong, but my fix was obviously wrong, too.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 14 Nov 2016 03:27:56 PM UTC, comment #3: 

Simon,

I'm fine with starting the persistent timer in the specific case I've identified.  I'm a little surprised at the suggestion because this was LwIP's legacy behavior which was removed in bug #34517 (commit d8f090a7595a79050c435545d00efc1261b9691c).

Are you more comfortable with using the persistent timer with the extra conditions I've identified (making sure wnd is snd.wnd and pcb->unacked == NULL)?  The old behavior would start the timer at the end of sending a window's worth of data if there was still a segment that didn't fit

I can do some testing with this solution in my test environment.

Joel Cunningham <jcunningham>
Group Member
Mon 14 Nov 2016 10:20:57 AM UTC, comment #2: 

The patch looks written well, but it's still a lot of code. Can't we instead use the persist timer here by starting it when we enter such a condition where we can't send because of segment sizes?
I agree with your patch, performance might be better, but our TCP has already grown so much in the last years, I'd love to keep it as small as we can.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 11 Nov 2016 02:27:29 PM UTC, comment #1: 

Simon,

Did you have a chance to examine at this issue yet?  What are you thoughts about a solution that splits the pbuf?

Joel Cunningham <jcunningham>
Group Member
Fri 04 Nov 2016 06:40:18 PM UTC, original submission:  

This bug continues the discussion that happened way back in bug #34517 where it was identified that if LwIP receives a window value smaller than the current unsent segment and there is no unacknowledged data, LwIP won't send and the subsequent window update can be dropped.  When the update is dropped, the connection deadlocks because we didn't fill the window entirely and engage zero window probing

I've been able to observe this problem with git master on my products.  I'm seeing the receiver's window is less than 1 MSS but the next packet on the unsent queue is 1 MSS, so LwIP stops sending. Then the window update from the receiver is dropped (verified update is sent in wireshark, but tcphdr in tcp_in.c only shows the previous ACK with < 1 MSS window).

In order to address this.  I started with the original persistent timer patch from bug #20511 which contained support for splitting the unsent segment to fill the window (which was rejected due to complexity)

I modified the logic to use the splitting very conservative and target only this case (rather than always filling the window by splitting).  Only when unsent segment is smaller than the send window (not congestion) AND the unacked queue is empty.

I further re-wrote the tcp_split_unsent_seg() function to be vastly simpler.  The original implementation didn't use pbuf and TCP segment utility functions, vastly complicating the segment creation and pbuf copying.  Also it didn't seem to be correct for read-only pbufs as it used memmove on the pbuf being split.  My implementation creates a new segment for the remainder payload (copying the remainder) and then trims the original pbuf.

Please see attached patch and provide feedback

Joel Cunningham <jcunningham>
Group Member

 

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

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Date Changed by Updated Field Previous Value => Replaced by
    2016-11-16 jcunningham StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2016-11-14 jcunningham StatusNone In Progress
        Assigned toNone jcunningham
    2016-11-14 jcunningham Attached File- Added 0001-bug-49533-start-persist-timer-when-unsent-seg-can-t-.patch, #38961
    2016-11-04 jcunningham Attached File- Added 0001-TCP-fill-send-window-completely-when-no-data-is-infl.patch, #38890

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code