IP stack: TCP FIN handling is incorrect for both local close and peer close

[zephyrbot]

Reported by Paul Sokolovsky:

"Some cases" for me are: MicroPython BSD Sockets prototype (which of course just calls thru to native API), HTTP server sample, net_context received via accept_cb, first HTTP request is recv'ed from it, then HTTP response is sent thru it, then context_put is called. In Wireshark:

18	25.922225000	192.0.2.1	192.0.2.2	TCP	54	http-alt > 54717 [FIN] Seq=3203909488 Win=1280 Len=0

So, lone FIN is being sent, which is suspicious on its own (it's known that modern IP stacks prefer to see ACKs on all packets). Drilling into packet:

Acknowledgment Number: 0x09beee8e [should be 0x00000000 because ACK flag is not set]
Expert Info (Warn/Protocol): Acknowledgment number: Broken TCP. The acknowledge field is nonzero while the ACK flag is not set

This packet gets dropped by Linux, so telnet I use to connect just continues to hang after receiving the response, doesn't terminate.

So, my first idea was to see if Linux could actually digest FIN without ACK, so I tried to make sure that ack number is set to 0:

{code:java}
@@ -433,7 +434,12 @@ int net_tcp_prepare_segment(struct net_tcp *tcp, u8_t flags,
segment.src_addr = (struct sockaddr_ptr *)local;
segment.dst_addr = remote;
segment.seq = tcp->send_seq;

•   segment.ack = tcp->send_ack;
  

•   if (flags & NET_TCP_ACK) {
  

•           segment.ack = tcp->send_ack;
  

•   } else {
  

+printf("no ack\n");

•           segment.ack = 0;
  

•   }
    segment.flags = flags;
    segment.wnd = wnd;
    segment.options = options;
  

{code}

Unfortunately, that didn't work - "no ack" was printed, but something else set .ack to non-zero later still.

Ok, making sure that FIN always has ACK:

{code:java}
@@ -416,6 +416,7 @@ int net_tcp_prepare_segment(struct net_tcp *tcp, u8_t flags,

    if (flags & NET_TCP_FIN) {
            tcp->flags |= NET_TCP_FINAL_SENT;

•           flags |= NET_TCP_ACK;
            seq++;
  
            if (net_tcp_get_state(tcp) == NET_TCP_ESTABLISHED ||
  

{code}

That worked better, telnet properly terminated. But Linux still worried:

24	23.361271000	192.0.2.1	192.0.2.2	TCP	54	http-alt > 55140 [FIN, ACK] Seq=1787749380 Ack=3646086400 Win=1280 Len=0
25	23.361306000	192.0.2.2	192.0.2.1	TCP	54	55140 > http-alt [FIN, ACK] Seq=3646086400 Ack=1787749381 Win=29200 Len=0
26	23.368550000	192.0.2.1	192.0.2.2	TCP	54	[TCP Keep-Alive] http-alt > 55140 [ACK] Seq=1787749380 Ack=3646086401 Win=1280 Len=0
27	23.368587000	192.0.2.2	192.0.2.1	TCP	54	[TCP Keep-Alive ACK] 55140 > http-alt [ACK] Seq=3646086401 Ack=1787749381 Win=29200 Len=0
28	23.376983000	192.0.2.1	192.0.2.2	TCP	54	http-alt > 55140 [RST] Seq=0 Win=0 Len=0
29	23.568034000	192.0.2.2	192.0.2.1	TCP	54	[TCP Retransmission] 55140 > http-alt [FIN, ACK] Seq=3646086400 Ack=1787749381 Win=29200 Len=0
30	23.984010000	192.0.2.2	192.0.2.1	TCP	54	[TCP Retransmission] 55140 > http-alt [FIN, ACK] Seq=3646086400 Ack=1787749381 Win=29200 Len=0

So, in packet 26, Zephyr tries to send ACK to Linux' FIN/ACK, but instead comes out what Wireshark recognizes as TCP keep-alive. Apparently, Linux agrees as it keeps retransmitting its FIN/ACK.

(Imported from Jira ZEP-2104)

[zephyrbot]

by Paul Sokolovsky:

So, in packet 26, Zephyr tries to send ACK to Linux' FIN/ACK, but instead comes out what Wireshark recognizes as TCP keep-alive. Apparently, Linux agrees as it keeps retransmitting its FIN/ACK.

Crude fix for this is:

{code:java}
@@ -402,6 +402,8 @@ int net_tcp_prepare_segment(struct net_tcp *tcp, u8_t flags,
*/
flags &= ~NET_TCP_FIN;
net_tcp_change_state(tcp, NET_TCP_TIME_WAIT);
+printk("Sending ACK to fin\n");

•           tcp->send_seq++;
                    } else {
                            net_tcp_change_state(tcp, NET_TCP_CLOSING);
                    }
  

{code}

[zephyrbot]

by Paul Sokolovsky:

Perhaps less crude hack is:

{code:java}
@@ -416,7 +418,8 @@ int net_tcp_prepare_segment(struct net_tcp *tcp, u8_t flags,

    if (flags & NET_TCP_FIN) {
            tcp->flags |= NET_TCP_FINAL_SENT;

•           seq++;
  

•           flags |= NET_TCP_ACK;
  

•           seq+=2;
  
            if (net_tcp_get_state(tcp) == NET_TCP_ESTABLISHED ||
                net_tcp_get_state(tcp) == NET_TCP_SYN_RCVD) {
  

{code}

It apparently depends on the fact that each FIN is ORed with ACK. But likely there's even better place to increment sequence number one by one.

[zephyrbot]

by Paul Sokolovsky:

Ok, so here's capture from echo_server w/o any patches like above:

11	4.946317000	192.0.2.2	192.0.2.1	TCP	54	51178 > 4242 [FIN, ACK] Seq=2085349120 Ack=230546044 Win=29200 Len=0
12	4.951868000	192.0.2.1	192.0.2.2	TCP	54	4242 > 51178 [FIN, ACK] Seq=230546044 Ack=2085349121 Win=1280 Len=0
13	4.951907000	192.0.2.2	192.0.2.1	TCP	54	51178 > 4242 [ACK] Seq=2085349121 Ack=230546045 Win=29200 Len=0
14	4.956379000	192.0.2.1	192.0.2.2	TCP	54	4242 > 51178 [RST] Seq=0 Win=0 Len=0

So, Zephyr doesn't acknowledge peer's FIN, instead rushes to send its FIN, peer acknowledges it, but Zephyr doesn't understand it, and aborts with RST instead.

[zephyrbot]

by Paul Sokolovsky:

Ok, so the whole intrigue was how http_server sample runs. Well... It pretends to support HTTP 1.1, while being not compliant with HTTP at all: it doesn't accept any headers after the request line (so I can't even tell it "Connection: close", HTTP1.1, ahem).

So, I'd love to be wrong, but looks like a proper, baseline TCP server - which accepts a connection, receives a request, sends a reply and closes connection - was never implemented for Zephyr, or the original issue with which this ticket starts, was spotted.

And let me adjust the title - FIN handling is incorrect not in some, but in all cases. In some cases only Wireshark can see that though.

[zephyrbot]

by Paul Sokolovsky:

From https://tools.ietf.org/html/rfc793

  Acknowledgment Number:  32 bits

    If the ACK control bit is set this field contains the value of the
    next sequence number the sender of the segment is expecting to
    receive.  Once a connection is established this is always sent.

The most straightforward interpretation of that is: it's a violation of TCP protocol to send any packet without ACK flag, beyond the initial SYN.

[zephyrbot]

by Jukka Rissanen:

I noticed while implementing HTTP server library, that we do not properly handle the case if we receive ACK | RST packet while being in ESTABLISHED state. The RST flag is just ignored and we do not close the receive context properly which leads to context leak. I have a fix for this case and will send it shortly.

[zephyrbot]

by Paul Sokolovsky:

#104