net: tcp: When sending FIN, make sure it goes with ACK and proper seq #104

pfalcon · 2017-05-05T12:40:21Z

Without change to add ACK to FIN, invalid TCP packet is generated,
where ack sequence number is non-zero. Without adjusting sequence
number as done, ACK which we send in response to peer's FIN/ACK is
not recognized by peer, and peer keeps retransmitting its FIN/ACK.

Signed-off-by: Paul Sokolovsky paul.sokolovsky@linaro.org

pfalcon · 2017-05-05T12:42:58Z

This patch is for illustrative purposes for GH-3545 . It should not be merged until:

We understand why "most" FINs are sent properly, while there's such path when it's sent improperly.
We understand where seq+2 comes from (in this patch, it's just a result of applying "genetic search algorithm" to make Linux host happy, as watched by Wireshark).

A stray printk() should help avoid premature merge.

nashif

remove Change-Id... you probably want to remove the hook from your git tree...

nashif · 2017-05-05T13:04:59Z

subsys/net/ip/tcp.c

@@ -417,7 +417,9 @@ int net_tcp_prepare_segment(struct net_tcp *tcp, u8_t flags,

 	if (flags & NET_TCP_FIN) {
 		tcp->flags |= NET_TCP_FINAL_SENT;
-		seq++;
+printk("Making sure FIN goes with ACK; adjusting seq by 2; now fire up Wireshark and check that all's ok\n");


that line looks off, align properly.

It's on purpose, as mentioned in the comment:

A stray printk() should help avoid premature merge.

As https://jira.zephyrproject.org/browse/ZEP-2104 alone didn't catch attention, maybe this draft patch will.

pfalcon · 2017-05-05T13:14:03Z

remove Change-Id... you probably want to remove the hook from your git tree...

What if we go back to gerrit tomorrow? ;-)

lpereira · 2017-05-09T22:33:20Z

Maybe @andyross will want to take a look as well.

pfalcon · 2017-05-10T12:09:58Z

@lpereira : Thanks. Actually, this could use more detailed look from you, as you worked on FIN handling previously, e.g. 564d8cd . What caught my eye in that commit is part of its commit message:

These changes makes Wireshark happy when the connection is closed.

So, this patch was also made using Wireshark (not RFC793, not looking thru thousands of lines of the code). I'm not much happy about this patch, but if nobody else have better idea, maybe we just merge it? ;-). Because it's done with a methodology already used before, so shouldn't be much worse than previous patches already pushed ;-).

jukkar

Looks good but I suggest we add also a comment to the code why we are adding seq+2 there.

jukkar · 2017-05-12T07:52:36Z

subsys/net/ip/tcp.c

-		seq++;
+printk("Making sure FIN goes with ACK; adjusting seq by 2; now fire up Wireshark and check that all's ok\n");
+		flags |= NET_TCP_ACK;
+		seq += 2;


This seq+=2 looks indeed weird. At least we could add a comment about why we do it like this. I agree with @pfalcon that it does not change much if we fix this according to wireshark output.

Yes, likely the sequence should have been incremented in some other state, but it wasn't, that's why we need to increment it by 2 here. It would take me some good time to trace thru Z TCP state machine implementation (nor I know TCP state machine itself by heart and will need to refresh myself on it). So I just hope that folks who know more about Z stack can look at this patch and get an idea what may be the underlying cause. But I'm glad that we have in store an option to merge this, if no better fix surfaces ;-).

That "magic extra byte" in sequence numbers for SYN and FIN packets is really annoying. But I agree with Jukka: it'd be better to implement this as an "increment because we received a FIN" and not a "+2" for clarity.

Also, a warning: I ran into a spot (I forget the precise details) where wireshark was lying to me about packet parsing. It was making assumptions based on the ethernet frame that happened to be wrong, or something. Use tcpdump to verify before making a change based solely on wireshark. :)

pfalcon · 2017-05-17T11:17:47Z

I'm off thru the end of week, so I polish this patch into being OK from codestyle point of view, while still having a FIXME comment. If this won't be merged until Friday, I assume it still would be usable as a fallback workaround for 1.8, just gives us a week or two to still try to investigate the underlying cause.

Without change to add ACK to FIN, invalid TCP packet is generated, where ack sequence number is non-zero. Without adjusting sequence number as done, ACK which we send in response to peer's FIN/ACK is not recognized by peer, and peer keeps retransmitting its FIN/ACK. Jira: ZEP-2104 Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>

jukkar

I think this is good enough for merging, especially if you can investigate the root cause for that seg+2 thing.

nashif added the In progress For PRs: is work in progress and should not be merged yet. For issues: Is being worked on label May 5, 2017

nashif requested changes May 5, 2017

View reviewed changes

nashif reviewed May 5, 2017

View reviewed changes

pfalcon changed the title ~~net: tcp: When sending FIN, make sure it goes with ACK and proper seq~~ [DRAFT] net: tcp: When sending FIN, make sure it goes with ACK and proper seq May 5, 2017

jukkar self-requested a review May 10, 2017 07:56

jukkar requested changes May 12, 2017

View reviewed changes

jukkar requested a review from andyross May 12, 2017 18:54

jukkar added the net label May 12, 2017

pfalcon force-pushed the net-fin-ack-seq branch from f4135b1 to 08dafa1 Compare May 17, 2017 11:15

pfalcon changed the title ~~[DRAFT] net: tcp: When sending FIN, make sure it goes with ACK and proper seq~~ net: tcp: When sending FIN, make sure it goes with ACK and proper seq May 17, 2017

pfalcon force-pushed the net-fin-ack-seq branch from 08dafa1 to 4433b77 Compare May 17, 2017 11:20

jukkar approved these changes May 17, 2017

View reviewed changes

nashif approved these changes May 17, 2017

View reviewed changes

jukkar merged commit d6b93b3 into zephyrproject-rtos:master May 18, 2017

This was referenced Sep 18, 2017

IP stack: TCP FIN handling is incorrect for both local close and peer close zephyriot/zep-jira14#1945

Closed

IP stack: TCP FIN handling is incorrect for both local close and peer close zephyriot/zephyr-issues#1945

Closed

zephyrbot mentioned this pull request Sep 23, 2017

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

Closed

agross-oss mentioned this pull request Feb 8, 2019

tests: dynamic_thread: increase stack size #13199

Closed

This was referenced Oct 19, 2022

hal: nxp: ARRAY_SIZE collision #51371

Closed

sys: util: Prevent multiple definitions of ARRAY_SIZE #50239

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: tcp: When sending FIN, make sure it goes with ACK and proper seq #104

net: tcp: When sending FIN, make sure it goes with ACK and proper seq #104

pfalcon commented May 5, 2017 •

edited

Loading

pfalcon commented May 5, 2017 •

edited by zephyrbot

Loading

nashif left a comment

nashif May 5, 2017

pfalcon May 5, 2017

pfalcon commented May 5, 2017

lpereira commented May 9, 2017

pfalcon commented May 10, 2017

jukkar left a comment

jukkar May 12, 2017

pfalcon May 12, 2017

andyross May 12, 2017

pfalcon commented May 17, 2017

jukkar left a comment

net: tcp: When sending FIN, make sure it goes with ACK and proper seq #104

net: tcp: When sending FIN, make sure it goes with ACK and proper seq #104

Conversation

pfalcon commented May 5, 2017 • edited Loading

pfalcon commented May 5, 2017 • edited by zephyrbot Loading

nashif left a comment

Choose a reason for hiding this comment

nashif May 5, 2017

Choose a reason for hiding this comment

pfalcon May 5, 2017

Choose a reason for hiding this comment

pfalcon commented May 5, 2017

lpereira commented May 9, 2017

pfalcon commented May 10, 2017

jukkar left a comment

Choose a reason for hiding this comment

jukkar May 12, 2017

Choose a reason for hiding this comment

pfalcon May 12, 2017

Choose a reason for hiding this comment

andyross May 12, 2017

Choose a reason for hiding this comment

pfalcon commented May 17, 2017

jukkar left a comment

Choose a reason for hiding this comment

pfalcon commented May 5, 2017 •

edited

Loading

pfalcon commented May 5, 2017 •

edited by zephyrbot

Loading