buglwIP - A Lightweight TCP/IP stack - Bugs: bug #26267, Simultaneous close

 
 

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

bug #26267: Simultaneous close

Submitted by:  Oleg Tyshev <olegreen>
Submitted on:  Tue 21 Apr 2009 09:19:12 AM 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)

Thu 25 Jun 2009 09:58:32 AM UTC, comment #22: 

I've checked in some code which I hope should fix this, but I would welcome a second opinion if you're able to test it.

Kieran Mansley <kieranm>
Project Member
Thu 18 Jun 2009 08:36:18 AM UTC, comment #21: 

Greg, you can try patch applied
to bug #26301: Out of order FIN closes half-closed connection
It solves this issue.

Oleg Tyshev <olegreen>
Tue 16 Jun 2009 08:22:21 PM UTC, comment #20: 

I have a possibly related issue that needs to be considered when looking at what to do with SND_NXT and SND_MAX.

When an LWIP client closes a connection (leaving the socket in LAST_ACK state), and then receives an ACK that covers outstanding data AND the FIN, the following test in tcp_process() fails:

    if (flags & TCP_ACK && ackno == pcb->snd_nxt) ...

because ackno is greater than pcb->snd_nxt.  It is equal to pcb->snd_max, actually.  As far as I can tell, this is completely legal (comprehensive ACK) but not handled correctly.

Greg Nelson <gnelson>
Wed 10 Jun 2009 03:26:11 PM UTC, comment #19: 

Simultaneous close is easy to reproduce:
1. Two hosts are connected and send data each other.
2. They are physically disconnected.
3. Each host makes close() for socket (for example after 3 seconds timeout).
4. After hosts again physically connected we gets after retransmition "Simultaneous close"

Oleg Tyshev <olegreen>
Fri 24 Apr 2009 11:59:41 AM UTC, comment #18: 

In reply to comment #13.
I think it would be better get ride of snd_max.
SND_NXT is used in all RFCs.
Why snd_nxt is overwritten by retransmission?
By retransmission segment is moved from unacked queue to unsent queue.

Instead

pcb->snd_nxt = ntohl(seg->tcphdr->seqno) + TCP_TCPLEN(seg);
if (TCP_SEQ_LT(pcb->snd_max, pcb->snd_nxt)) {
   pcb->snd_max = pcb->snd_nxt;
}

we should make something like this

tmp_snd_nxt = ntohl(seg->tcphdr->seqno) + TCP_TCPLEN(seg);
if (TCP_SEQ_LT(pcb->snd_nxt, tmp_snd_nxt)) {
  pcb->snd_nxt = tmp_snd_nxt;
}

tcp_rexmit_rto() and tcp_rexmit() shouldn't modify snd_nxt.

Oleg Tyshev <olegreen>
Thu 23 Apr 2009 07:59:04 AM UTC, comment #17: 

In reply to comment #14: that issue now being tracked in BUG #26301

Kieran Mansley <kieranm>
Project Member
Wed 22 Apr 2009 03:23:50 PM UTC, comment #16: 

In reply to comment #15

I agree with that.

Kieran Mansley <kieranm>
Project Member
Wed 22 Apr 2009 03:06:52 PM UTC, comment #15: 

Again about CLOSING :)

I think in CLOSING if we received ACK to the FIN, it doesn't necessary to call tcp_ack_now().
If other site doesn't receive ACK, it sends FIN again and would be ACKed from TIME_WAIT state.

Oleg Tyshev <olegreen>
Wed 22 Apr 2009 12:26:06 PM UTC, comment #14: 

Exists one more issue with close of the connection.
FIN could be inserted to out of sequence queue.
If we want to support later shutdown() for sockets,
FIN should be accepted for transition to other TCP states only if it was accepted in sequence.

Now tcp_receive() returns true for any segment that was received in sequence, we could change it.
The function can return true if it was received FIN in sequence (in current packet or as result of filling gap in out of sequence queue)

Oleg Tyshev <olegreen>
Wed 22 Apr 2009 12:05:31 PM UTC, comment #13: 

I've had another look and think I understand now: I was wrong about what snd_max is.  I thought it was the maximum value that we could send, but it is actually the maximum value we have sent.  snd_nxt can change to be smaller when we retransmit things.  This is a problem because lots of code (e.g. stuff I've written) uses snd_nxt where snd_max would be better.  I'm not sure why we need both, so I'd quite like to get rid of snd_nxt.

Kieran Mansley <kieranm>
Project Member
Wed 22 Apr 2009 11:47:57 AM UTC, comment #12: 

I'm getting more confused: we should not be comparing ACK fields to snd_max.  snd_nxt, as the code is now, seems correct to me.  Why do you think snd_max is right?

Kieran Mansley <kieranm>
Project Member
Wed 22 Apr 2009 11:35:29 AM UTC, comment #11: 

reply to comment 9
You are right, Kieran.

We should use snd_max instead of snd_nxt.

It could be one segment retransmitted and snd_nxt would have previous value, but snd_max value won't be changed.

Oleg Tyshev <olegreen>
Wed 22 Apr 2009 09:20:07 AM UTC, comment #10: 

If we wait ACK to the FIN, we should compare not with pcb->snd_nxt but with pcb->snd_max.

case CLOSING:
tcp_receive(pcb);
-if (flags & TCP_ACK && ackno == pcb->snd_nxt) {
+if (flags & TCP_ACK && ackno == pcb->snd_max) {
LWIP_DEBUGF(TCP_DEBUG, ("TCP connection closed %"U16_F" -> %"U16_F".\n", inseg.tcphdr->src, inseg.tcphdr->dest));
tcp_ack_now(pcb);

It works.

I think the same should be done for FIN_WAIT_1 and LAST_ACK

case FIN_WAIT_1:
tcp_receive(pcb);
if (flags & TCP_FIN) {
-if (flags & TCP_ACK && ackno == pcb->snd_nxt) {
+if (flags & TCP_ACK && ackno == pcb->snd_max) {
LWIP_DEBUGF(TCP_DEBUG,

case LAST_ACK:
tcp_receive(pcb);
if (flags & TCP_ACK && ackno == pcb->snd_max) {

Oleg Tyshev <olegreen>
Wed 22 Apr 2009 08:37:17 AM UTC, comment #9: 

I'm afraid I don't understand the last comment.

The awaited ackno should be the same in FIN_WAIT1 and and CLOSING - in both cases we're waiting for the ACK of our FIN.

Kieran Mansley <kieranm>
Project Member
Tue 21 Apr 2009 05:40:24 PM UTC, comment #8: 

I debugged
for host2 pcb->snd_nxt in processing FIN_WAIT_1 and CLOSING is the same.

FIN wasn't send to the other side
In CLOSING would be awaited ACK to the FIN.
It means
(awaited ackno in FIN_WAIT_1) + 1 == (awaited ackno in CLOSING)

Oleg Tyshev <olegreen>
Tue 21 Apr 2009 02:25:34 PM UTC, comment #7: 

Taking a look at tcp_slowtmr() it's hard to see how this isn't working.  To do a RTO retransmission you have to have:
 - pcb in tcp_active_pcbs (definitely should be)
 - pcb->state != SYN_SENT (we know it's CLOSING)
 - pcb->nrtx < TCP_MAXRTX (should be, unless you've set that value smaller in your stack)
 - pcb->persist_backoff == 0 (if it wasn't we'd see a probe in the packet trace)
 - pcb->unacked != NULL (should still have the FIN there)

The only bit there that I'm not sure about is the persist_timer.  There is a packet at the end of the trace that could either be a duplicate ACK (due to the retransmissions coming from the other side) or a persist timer probe.  If the persist timer were going off that would explain why the FIN doesn't get retransmitted, but I doubt this is the case.

It would really help if you could enable some of the logging in tcp_slowtmr() and see what path it takes, and why tcp_rexmit_rto() is not called in this case.

Kieran Mansley <kieranm>
Project Member
Tue 21 Apr 2009 02:15:03 PM UTC, comment #6: 

The second capture shows, as you said, no FIN being retransmitted by one side.  This capture only lasts for a few seconds.  Is that because no more packets were sent after this, or because the capture was stopped?  I wonder if lwIP would have retransmitted the FIN eventually.

Kieran Mansley <kieranm>
Project Member
Tue 21 Apr 2009 02:08:22 PM UTC, comment #5: 

Sorry, applied capture log is false. (host2 was 1st who sent FIN)
I attach another one (simult_2.pcap).

Yes, the fix, that I suggested is only a temporary solution.
But it is not bad to retransmit immediately unacked data, because other side won't send anything after FIN.

May be better

if (pcb->unacked) {
 tcp_rexmit_rto(pcb);
}
else {
 tcp_ack_now(pcb);
}

(file #17985)

Oleg Tyshev <olegreen>
Tue 21 Apr 2009 12:47:14 PM UTC, comment #4: 

If the retransmission timer isn't working, that is definitely a bug, but adding an explicit retransmission there isn't the right solution - we should fix the retransmission timer.

However, in the packet capture you attached both sides do seem to retransmit their FIN, so it looks like the timer is working.

Kieran Mansley <kieranm>
Project Member
Tue 21 Apr 2009 12:17:16 PM UTC, comment #3: 

After both sides calls close() they are in FIN_WAIT_1 state,
but other side doesn't receive last data and FIN because cable is disconnected.

Cable connected again:
host1 retransmit data and FIN,
but host 2 makes only ACK and doesn't send own FIN.

To go to CLOSING state we need only to receive FIN.
And in CLOSING state we waits ACK for FIN.
But to receive ACK we needs FIN sometimes to send.

http://www.freesoft.org/CIE/Course/Section4/11.htm
http://en.wikipedia.org/wiki/File:Tcp_state_diagram_fixed.svg

I suggest in FIN_WAIT_1 handler following fix
  case FIN_WAIT_1:
...
      } else {
-       tcp_ack_now(pcb);
+       tcp_rexmit_rto(pcb);
        pcb->state = CLOSING;
      }

It looks like retransmission timer doesn't work.

Oleg Tyshev <olegreen>
Tue 21 Apr 2009 09:43:49 AM UTC, comment #2: 

I'm not sure what the problem is here.  Can you explain more what you think is wrong?

The state transitions that we're performing match those on the TCP state diagram, and the packet capture looks mostly OK too.  From your comments marked "!!!" it sounds like you want the stack to retransmit its FIN rather than just send an ACK when moving from FIN_WAIT1 to CLOSING, but I'm not sure that would be correct.

Kieran Mansley <kieranm>
Project Member
Tue 21 Apr 2009 09:25:51 AM UTC, comment #1: 

but host1 never received FIN from host2.

(In pcap file host1 - 172.16.2.100,
host2 - 172.16.4.13)

Oleg Tyshev <olegreen>
Tue 21 Apr 2009 09:19:12 AM UTC, original submission:  

It seems to me simultaneous close doesn't work right way.

Host1 and host2 sends data each other.
Cable between hosts is disconnected.
Host1 and host2 makes close().
Host1 and host2 are in FIN_WAIT_1 state.
Cable between hosts is connected again.

host1 retransmits FIN

host2 receives this FIN,ACK
in tcp_process()
case FIN_WAIT_1:
tcp_receive(pcb);
if (flags & TCP_FIN) {
if (flags & TCP_ACK && ackno == pcb->snd_nxt) {
...
} else {
!!! we are here
!!! tcp_ack_sends only ACK without data and without FIN
!!! because unsent queue is empty and FIN placed in unacked queue.
tcp_ack_now(pcb);
pcb->state = CLOSING;
}
} else if (flags & TCP_ACK && ackno == pcb->snd_nxt) {
pcb->state = FIN_WAIT_2;
}

Now host2 in CLOSING state.
Host2 again receives FIN,ACK and ... it goes to TIME_WAIT

Oleg Tyshev <olegreen>

 

Attached Files
file #17985:  simult_2.pcap added by olegreen (2KiB - application/octet-stream)
file #17982:  simult.pcap added by olegreen (2KiB - 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 gnelson (Posted a comment)
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by olegreen (Submitted the item)
  •  

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

    Only logged-in users can vote.

     

     

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-07-27 kieranm StatusReady For Test => Fixed
        Open/ClosedOpen => Closed
    2009-06-25 kieranm StatusNone => Ready For Test
    2009-04-21 olegreen Attached File- => Added simult_2.pcap, #17985
    2009-04-21 olegreen Attached File- => Added simult.pcap, #17982

    Back to the top


    Powered by Savane 3.5