net: tcp: Fix tcp passive close

[askawu]

Previously, if passive close is peformed, the net context is released
after FIN is received and FIN,ACK is sent. The following last ack from
the peer will be treated as an improper packet, RST is sent to the peer.

This patch refines tcp_established() by centralizing the tcp state
transition and releases the net context only if NET_TCP_CLOSED is
reached.

Besides, the logic that releases the net pkt without appdata (i.e. ACK
or FIN) is moved from packet_received() to tcp_established(). This makes
packet_received() less dependent on the protocol and make the usage of
net pkt more clear in tcp_established().

Fixes: #4901

Signed-off-by: Aska Wu aska.wu@linaro.org

[jukkar]

This unref is a bit problematic as I am seeing warnings about double unref. Application will release the net_pkt when it receives it, and if the tcp_established() returns NET_DROP, then the packet is also freed in net_core.c:processing_data().
Was this unref here so that if we receive final ack, we return NET_OK and there would be memory leak otherwise?
If so, then better would be to put the net_pkt_unref() where you call clean_up.

In order to avoid the double unref, I used if (net_pkt_appdatalen(pkt) == 0 && ret == NET_OK) { here but there were still some unref warnings as the application might have released the net_pkt already.

I tested this with echo-client with CONFIG_NET_DEBUG_NET_PKT=y (in order to see memory allocations), and #4972 applied. It seemed to work but at some point the echo-client hang and stopped responding, I am trying to find out is this patch causing that or is there some unrelated issue.

If I remove this unref in this line, then echo-server runs out of memory, but with this unref in place, the echo-server runs just fine and I do not see any issues.

[askawu]

Indeed....it will cause double unref because the pkt is released by the application and appdata len is unpredictable. I will try to reproduce the issue by echo client as you mentioned.

Is it a good idea to copy the appdata len to data_len at the begining of tcp_establish()? So we can use data len to decide whether we should unref the pkt in tcp_establish() or not.

[pfalcon]

Btw, echo_server crash I quote in #5015 (comment) happens in net_context_unref(), so is likely the same issue.

[askawu]

Was this unref here so that if we receive final ack, we return NET_OK and there would be memory leak otherwise?

@jukkar, Yes. I also want to put the pkt unref together for clearness since it was hard for me to trace the pkt lifecycle.
However, to prevent double unref, I decide to remove pkt unref from clean_up.

[askawu]

Update:

• Fix the double unref problem.
• I tested echo-server based on this patch and Revert "net: pkt: Take into account MTU when adding data to a packet" #4972. CONFIG_NET_DEBUG_NET_PKT and CONFIG_NET_DEBUG_NET_PKT_ALL are enabled. I don't see any warning about double unref.

[jukkar]

I was testing this and did not see any issues so far.