Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FinWait1: ACK of previous data segment may trigger transition to FinWait2 (preventing direct transition to TimeWait by the following FIN|ACK) #22

Open
knieriem opened this issue May 7, 2024 · 4 comments

Comments

@knieriem
Copy link

knieriem commented May 7, 2024

When requesting a web page from the cyw43439 based http-server example (using wget, curl, or a browser), I am observing a wireshark log similar to the following excerpt:

No.	Time Source	Dest   Proto Len  Info
 4 0.097 client pico_w HTTP  180  GET / HTTP/1.1 
 5 0.201 pico_w client TCP    54  http(80) → 58856   [ACK] Seq=1 Ack=127 Win=1410 Len=0
 6 0.254 pico_w client HTTP 1244  HTTP/1.1 200 OK  (text/html)
 7 0.254 client pico_w TCP    54  58856 → http(80) ❶ [ACK] Seq=127 Ack=1191 Win=63070 Len=0
 8 0.254 pico_w client TCP    54  http(80) → 58856 ➋ [FIN, ACK] Seq=1191 Ack=127 Win=1536 Len=0
 9 0.258 client pico_w TCP    54  58856 → http(80) ➌ [FIN, ACK] Seq=127 Ack=1192 Win=63070 Len=0
10 0.307                      54  Ignore >
11 0.310 pico_w client TCP    54  http(80) → 58856   [ACK] Seq=1192 Ack=128 Win=1536 Len=0

After the server pico_w has transmitted the web page of 1244 bytes, the client sends an ACK segment (❶).
At nearly the same time the server already sends its FIN+ACK (➋), which the client acknowledges
(➌, the client uses the ACK number 1192, which is 1191+1, hence the ACK refers to the server's FIN+ACK).

In the server's log a slightly different order can be observed: The ACK (1) is received (resp. processed) after the FIN+ACK (2).

Got webpage request!
0.335 DEBUG TCP:delayed-close port=80
0.337 DEBUG TCP:send plen=12440.338 level=DBG-2 tcb:snd state=FinWait1 pend=0 snd.nxt=1192 snd.una=1 snd.wnd=64240
0.338 DBG-2 tcb:snd seg.seq=1191 seg.ack=1723085760 seg.wnd=2030 seg.flags=17 seg.data=0
0.340 INFO  TCP:tx-statechange port=80 old=Established new=FinWait1 txflags=[FIN,ACK] ➋
0.341 DEBUG TCP:send plen=540.395 level=DBG-2 Stack.RecvEth:start plen=54

0.397 DBG-2 tcb:rcv state=FinWait2 rcv.nxt=1723085760 rcv.wnd=2030 challenge=false
0.398 DBG-2 recv:seg seg.seq=1723085760 seg.ack=1191 seg.wnd=63070 seg.flags=16 seg.data=0
0.399 INFO  TCP:rx-statechange port=80 old=FinWait1 new=FinWait2 rxflags=[ACK] ❶

0.400 DBG-2 TCPConn.stateCheck:hasPending
0.401 DBG-2 tcb:snd state=FinWait2 pend=0 snd.nxt=1192 snd.una=1191 snd.wnd=63070
0.402 DBG-2 tcb:snd seg.seq=1192 seg.ack=1723085760 seg.wnd=2030 seg.flags=16 seg.data=0
0.403 DBG-2 TCPConn.send:start
0.404 DEBUG TCP:send plen=54

0.406 DBG-2 Stack.RecvEth:start plen=54
0.407 DEBUG TCP:recv opt=0 ipopt=0 payload=0
0.408 DBG-2 TCPConn.recv:start
0.409 DBG-2 tcb:rcv state=TimeWait rcv.nxt=1723085761 rcv.wnd=2030 challenge=false
0.410 DBG-2 recv:seg seg.seq=1723085760 seg.ack=1192 seg.wnd=63070 seg.flags=17 seg.data=0
0.410 INFO  TCP:rx-statechange port=80 old=FinWait2 new=TimeWait rxflags=[FIN,ACK] ➌

0.411 DBG-2 TCPConn.stateCheck:hasPending
0.412 DBG-2 tcb:snd state=TimeWait pend=0 snd.nxt=1192 snd.una=1192 snd.wnd=63070
0.413 DBG-2 tcb:snd seg.seq=1192 seg.ack=1723085761 seg.wnd=2030 seg.flags=16 seg.data=0
0.414 DBG-2 TCPConn.stateCheck:closed

As client and server send ❶ and ➋ independently, it can happen that ❶ is captured in the wireshark log before ➋, but on the server side ❶ is received and processed slightly after ➋.

Since the server is in FinWait1 state, when ❶ is received, ControlBlock.rcvFinWait1 (control.go:246) is called. Because the hasAck switch case applies, FinWait2 state is entered.

Wouldn't it be more correct if ❶ would be ignored by rcvFinWait1, since it does not acknowledge the FIN, but a previous date segment? The server could wait some more time until ➌ arrives, which contains the matching FIN and ACK.

The first case expression of the switch statement in ControlBlock.rcvFinWait1 already contains the
sub condition seg.ACK == tcb.snd.NXT. I think moving this condition to the initialization of hasAck would change the behaviour so that TimeWait is entered directly from FinWait1:

  • orig: hasAck := flags&FlagACK != 0
  • mod: hasAck := flags&FlagACK != 0 && seg.ACK == tcb.snd.NXT

It would make sure that only an ACK that actually acknowledges the FIN previously sent by the server is accepted. With this change, I am observing the following transition in Pico W's log:

0.904 INFO TCP:tx-statechange port=80 old=Established new=FinWait1 txflags=[FIN,ACK]
0.015 INFO TCP:rx-statechange port=80 old=FinWait1 new=TimeWait rxflags=[FIN,ACK]

As an example, I have attached a patch containing the change to rcvFinWait1, and a test case. finwait1_ignore_data_ack.patch.gz

@soypat
Copy link
Owner

soypat commented May 7, 2024

I might get around to this one by the weekend. Thank you for submitting the issue :)

@soypat
Copy link
Owner

soypat commented May 7, 2024

@knieriem That said, I did not ask you if you were available and wanted to contribute?

@knieriem
Copy link
Author

If you suggest me to provide a pull request, I will be happy to do so.
(BTW, it appears that if keeping track of acknowledgments / retransmission was already implemented (or is it implemented?), the ACK observed here would have been consumed earlier, so this behavior would likely not be observable).

@soypat
Copy link
Owner

soypat commented May 13, 2024

Sure! feel free to provide a PR. I've been busy with other gnarly bugs on seqs and have not had time to look into this particular issue. Right now I'm looking at a bug where the TCP connection remains forever open in FinWait2.

As for retransmission logic in seqs, as far as I recall there is none or very basic support for certain cases, though I have been thinking it would be nice to have.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants