TCP related and other fixes #6058

rveerama1 · 2018-02-08T14:20:40Z

This PR contains fixes related to TCP issues and other network related issues.

codecov-io · 2018-02-08T14:38:34Z

Codecov Report

Merging #6058 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #6058      +/-   ##
==========================================
- Coverage   52.63%   52.61%   -0.02%     
==========================================
  Files         410      410              
  Lines       40048    40057       +9     
  Branches     7781     7781              
==========================================
- Hits        21078    21077       -1     
- Misses      15768    15779      +11     
+ Partials     3202     3201       -1

Impacted Files	Coverage Δ
subsys/net/ip/tcp.h	`100% <ø> (ø)`	⬆️
subsys/net/ip/tcp.c	`42.95% <0%> (-0.86%)`	⬇️
subsys/net/ip/net_context.c	`60.3% <0%> (-0.12%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa8850b...3146804. Read the comment docs.

pfalcon

At least one issue (with NET_DEFAULT_IF_NONE option naming) requires change IMHO.

pfalcon · 2018-02-08T14:55:55Z

subsys/net/ip/Kconfig

+	  to choose default interface. Otherwise first interface will be the
+	  default interface.
+
+config NET_DEFAULT_IF_NONE


The name of the option contradicts its description: the option says that there's not default interface, while description says the first is default.

pfalcon · 2018-02-08T14:58:25Z

subsys/net/ip/net_if.c

 	if (__net_if_start == __net_if_end) {
-		return NULL;
+		return iface;


I wouldn't make this change. The original code makes (more) clear that the return value is NULL.

I agree with Paul

pfalcon · 2018-02-08T15:04:46Z

subsys/net/ip/net_context.c

@@ -1246,6 +1246,14 @@ NET_CONN_CB(tcp_established)
 		return NET_DROP;
 	}

+	/* Increment the ack */


This is not a correct change. First, application gets a chance to process incoming data. An application may reject or fail to process data (e.g., due to lack of memory). ACK gets sent only after that, and includes outcomes of the processing status (e.g., updated/not updated receive window, etc.).

If you look thru the history, you'll likely find that the opposite change was done, which you now try to revert.

pfalcon · 2018-02-08T15:08:20Z

subsys/net/ip/tcp.c

@@ -961,32 +961,14 @@ bool net_tcp_ack_received(struct net_context *ctx, u32_t ack)
 		valid_ack = true;
 	}

-	/* No need to re-send stuff we are closing down */


Comment for the commit message:

Now ACK's are in rx_thread queue or ACK for first packet received.

I'm not sure I understand this phrase. Can you please clarify the description in commit message?

Actually, whole this part is not very clear:

Now ACK's are
in rx_thread queue or ACK for first packet received. Then
net_tcp_ack_received() function will delete the first packet.
Because it's valid ACK and connection state is in ESTABLISHED state
sent list is not empty. It restarts the timer and resend packets
in queue. In e.g. case Zephyr sends second packet in a list
immediately.

pfalcon · 2018-02-08T15:12:53Z

subsys/net/ip/tcp.c

@@ -661,12 +661,47 @@ int net_tcp_prepare_ack(struct net_tcp *tcp, const struct sockaddr *remote,
 	return -EINVAL;
 }

+static void get_src_addr_ptr(struct net_tcp *tcp,


I don't think this is a right change, see #5481

I tried to find helper function for this purpose. May be I missed it. I will take a look.

pfalcon · 2018-02-08T15:14:21Z

subsys/net/lib/app/net_app.c

@@ -562,27 +562,45 @@ struct net_context *select_client_ctx(struct net_app_ctx *ctx,

 #if defined(CONFIG_NET_APP_SERVER)
 #if defined(CONFIG_NET_TCP)
-static struct net_context *get_server_ctx(struct net_app_ctx *ctx,


Commit message:

E.g. One net_app_ctx and two net_contexts. 'For' loop goes through array of net_contexts and searching for matching context. Let's say caller required net_context is second in the

What's the purpose of that intent? Please reformat.

Also, that description also doesn't sound too clear to me (but I'm not familiar with net_app_ctx internals, maybe that's it).

pfalcon · 2018-02-08T15:30:53Z

My usual comment applies: this PR packs the full spectrum of changes - from obviously correct to obviously incorrect, with anything inbetween.

At this point, I'm keen to say that Github reviews are failing for us, and we should go back to Gerrit which enforces "one commit per review" rule. <--- @nashif: sharing an opinion.

nashif · 2018-02-08T17:06:02Z

At this point, I'm keen to say that Github reviews are failing for us, and we should go back to Gerrit which enforces "one commit per review" rule. <--- @nashif: sharing an opinion.

I am not sure how this can be true, you could have 1000s of commits in 1 single gerrit change set, same thing as a PR in GH.

Review per commit, and not as a PR, just the same way you would have done in gerrit.

i agree we should not put too much unrelated stuff into one PR, it can be confusing, commits in 1 single PR need to be related.

And finally, No, we are not going back to gerrit.

jukkar · 2018-02-09T09:06:32Z

subsys/net/ip/Kconfig

+	  default interface.
+
+config NET_DEFAULT_IF_NONE
+	bool "Default interface is first interface"


This could be shorter so what about "First available interface"

jukkar · 2018-02-09T09:06:52Z

subsys/net/ip/Kconfig

+	  default interface.
+
+config NET_DEFAULT_IF_ETHERNET
+	bool "Ethernet interface is the default interface"


Just "Ethernet" here

jukkar · 2018-02-09T09:07:23Z

subsys/net/ip/Kconfig

+	depends on NET_L2_ETHERNET
+
+config NET_DEFAULT_IF_BLUETOOTH
+	bool "Bluetooth interface is the default interface"


"Bluetooth"

jukkar · 2018-02-09T09:07:36Z

subsys/net/ip/Kconfig

+	depends on NET_L2_BT
+
+config NET_DEFAULT_IF_IEEE802154
+	bool "IEEE802.15.4 interface is the default interface"


"IEEE 802.15.4"

jukkar · 2018-02-09T09:07:57Z

subsys/net/ip/Kconfig

+	depends on NET_L2_IEEE802154
+
+config NET_DEFAULT_IF_OFFLOAD
+	bool "OFFLOAD interface is the default interface"


"Offloaded interface"

jukkar · 2018-02-09T09:08:08Z

subsys/net/ip/Kconfig

+	depends on NET_L2_OFFLOAD
+
+config NET_DEFAULT_IF_DUMMY
+	bool "Dummy interface is the default interface"


"Dummy testing interface"

jukkar · 2018-02-09T09:08:39Z

subsys/net/ip/net_if.c

 	if (__net_if_start == __net_if_end) {
-		return NULL;
+		return iface;


I agree with Paul

jukkar · 2018-02-09T09:22:59Z

Most of the patches seem not to be related to each other but I am not sure if it is worth sending them one by one. But if needed at least TCP stuff could be a separate PR, and interface selection and net-app fixes. So this could be split into three PRs.

rveerama1 · 2018-02-09T10:49:38Z

subsys/net/ip/net_context.c

-	}
-
-	send_ack(context, &conn->remote_addr, false);
-


@pfalcon "ret = packet_received(conn, pkt, context->tcp->recv_user_data);" Sending ack doesn't depend on returned value. Irrespective of return value, ACK will be prepared and sent.

I noticed an issue related to this while trying with RPL BR sample. When TCP handover the payload in callback, then net_app callback, and then http call back. (e.g connected) all the way goes to application. Here an application has kilobytes of payload to reply. Now net_pkt_append() doesn't append more than "datalen in net_pkt", http_prepare_and_send() create new packets until all the payload filled in buffers. After all these shebang ACK will be prepared and sent. Which is causing sometimes a delay in ACK, and the peer sends another DUP packet. Scenarios like this delay ACK preparation. And like I said current code doesn't really check return value, I thought it would make more sense to send the ACK first.

Irrespective of return value, ACK will be prepared and sent.

Yes, an ACK packet will be sent in either case. But contents of the ACK packet are expected to depend on app's receive callback (i.e. receive callback may affect data put into an ACK reply), specifically, the receive window: 19ff963 .

I noticed an issue related to this while trying with RPL BR sample. ...

Thanks for the clear description of the problem you saw. I would say, this description should go into the commit message.

But then, we found yet another issue with callback-based data processing. And it can be classified as a case of https://cwe.mitre.org/data/definitions/405.html - for a small request, "http_prepare_and_send() create new packets until all the payload filled in buffers", and all that happens in a not very appropriate place - synchronous callback from the network stack, which affects the stack performance.

I'm afraid, we'd need to find another solution, and such solution exists - BSD Sockets. How it works is that there's decoupling between packet reception of packets by network stack and consumption by an application - they both run in separate threads and communicate via queue. There's also the flow control implemented, mandated by TCP, using the concept of "receive window": when a packet is just received from peer peer and put into the queue, the recv_wnd is decreased (and communicated back in an ACK packet), signaling the peer that it has decreased quota to send more data (which eventually may get to 0). recv_wnd is increased back only when data is actually consumed from the queue.

Due to the fact that we have usecases of both updating and not update recv_wnd, the (optional) update happens in recv callback. Thus, this patch would break this processing, and will break cases of communication large amounts of data (megabytes) over sockets.

My suggestion is to separate this patch into a separate PR, and further analyze and test it from different sides, but opinion is that we'd need to find an alternative way to address this issue. Thanks.

But then, we found yet another issue with callback-based data processing. And it can be classified as a case of https://cwe.mitre.org/data/definitions/405.html - for a small request, "http_prepare_and_send() create new packets until all the payload filled in buffers", and all that happens in a not very appropriate place - synchronous callback from the network stack, which affects the stack performance.

I am experimenting that in my WIP PR of RPL BR. Running a separate thread to reply websocket messages. Checking for proper solution.
b61d0a5#diff-780b2211d7789425a2584af4afc52b77R1881

Overall, let's take it away patch from this PR, time being I will keep it in #5035. I will create a separate PR for this and list issues over there.

rveerama1

Added some comments, please have a look.

rveerama1 · 2018-02-09T11:50:53Z

Split the patches into separate PRs (#6073, #6074).
@pfalcon : addressed your comments, except duplicate function to get src socket ptr. if issue #5481 provides any generic API, I will re-use that.

pfalcon · 2018-02-09T12:01:40Z

@pfalcon : addressed your comments, except duplicate function to get src socket ptr. if issue #5481 provides any generic API, I will re-use that.

Thanks. Please let me check the current situation with that, and come up with a specific suggestion.

rveerama1 · 2018-02-09T12:38:10Z

Dropped the ACK sending before payload handler patch.

jukkar

LGTM

jukkar · 2018-02-12T07:40:40Z

@pfalcon could you re-review the PR as now the controversial "ACK sending before payload handler" commit is dropped from this set.

pfalcon · 2018-02-12T08:51:56Z

subsys/net/ip/tcp.c

+#endif
+
+end:
+	addr = &tcp->context->local;


This is a dead statement, and apparently doesn't do what was intended by the author (what?).

pfalcon · 2018-02-12T09:08:46Z

@pfalcon could you re-review the PR as now the controversial "ACK sending before payload handler" commit is dropped from this set.

Sure, as my last comment 2 comments above yours says, the last concern is still get_src_addr_ptr() function added in 3146804#diff-a278935f8d5d5e0b4424ca3e1182d48cR664

@jukkar, @rveerama1 : And I'm glad that I took another look at it. Here're concerns with it:

It contains an obviously dead statement, assignment to a local var without further use (like logical thinko re: logic that assignment should perform).
As I mentioned, my concern (based on the name of the function) was that it would be 4th function with almost similar purpose in the codebase (variability of src/dst addr_ptr/no-ptr), tracked in net: Another duplicate function to get pkt address #5481. But looking in more detail into it, turns out "get_src_addr_ptr" isn't what it does! What it appears to do is to copy struct sockaddr into struct sockaddr_ptr, with an adhoc exception for a case when struct sockaddr is NULL (coded improperly per p.1). In particular, it doesn't copy "src" addr, it can copy any addr. Summing: the problem with that function is the naming: it's named based on the usage in the single place, instead of based on what it does. Yet, it's named generically enough to cause confusion re: if it can be reused or should reuse existing similar functions. Given that it's called from a single place, I'd suggest to just put the code directly there. Failing that, I'd suggest to make a generic copy_sockaddr_to_sockaddr_ptr() function, and still remove adhoc check if (!local) ... &tcp->context->local check from it. @jukkar , maybe you can suggest if such a generic copy function already exists.

k_delayed_work_cancel(&context->tcp->fin_timer) called twice immediately one after other. Signed-off-by: Ravi kumar Veeramally <[email protected]>

In case of failed to get source address from the net packet, release the assgined tcp backlog entry. Otherwise it will never be freed. Signed-off-by: Ravi kumar Veeramally <[email protected]>

Zephyr doesn't have luxury to create re-transmit timer per packet. So it has different methods to handle packets in queue. But re-sending packets on valid ack messages causing issues. E.g. A TCP node sent two packets (packet-1, packet-2). Peer replied two ACKs (ACK-1 and ACK-2), and these two ACK's are at rx_thread queue. Now ACK-1 is handled and reference of packet-1 is freed from sent list. Then if condiftion (valid ACK and connection state is ESTABLISHED) notices that, sent list is not empty. Restart the timer, modify sent flag and resend packets in a list. Here packet-2 is sent again, even though ACK-2 is already received. Situation is worse if there are more packets in the list. So only start the re-transmit timer in-case queue is not empty. It allows rx_thread to handle all incoming packets (in this e.g ACKs). When the re-trasmit timer expires, it sends the packets which are left in queue. Signed-off-by: Ravi kumar Veeramally <[email protected]>

If context is bound to IPv6 unspecified addresss and some port number, then unspecified address is passed in TCP reset packet message preparation. Eventually packet dropped at the peer. Signed-off-by: Ravi kumar Veeramally <[email protected]>

rveerama1 · 2018-02-13T09:59:03Z

@pfalcon updated PR. Let me know if I address your comments.

pfalcon

updated PR. Let me know if I address your comments.

Looks good, thanks much for reworking it!

rveerama1 requested review from jukkar and tbursztyka as code owners February 8, 2018 14:20

rveerama1 requested a review from lpereira February 8, 2018 14:21

rveerama1 added the area: Networking label Feb 8, 2018

pfalcon self-requested a review February 8, 2018 14:52

pfalcon requested changes Feb 8, 2018

View reviewed changes

pfalcon reviewed Feb 8, 2018

View reviewed changes

jukkar requested changes Feb 9, 2018

View reviewed changes

rveerama1 commented Feb 9, 2018

View reviewed changes

rveerama1 force-pushed the tcp-fixes branch from 9c1d652 to 1f760e9 Compare February 9, 2018 11:41

rveerama1 force-pushed the tcp-fixes branch from 1f760e9 to 3146804 Compare February 9, 2018 12:37

jukkar approved these changes Feb 9, 2018

View reviewed changes

pfalcon reviewed Feb 12, 2018

View reviewed changes

rveerama1 added 3 commits February 13, 2018 11:51

net: context: Remove duplicate call

43729f1

k_delayed_work_cancel(&context->tcp->fin_timer) called twice immediately one after other. Signed-off-by: Ravi kumar Veeramally <[email protected]>

net: context: Handle failure case of tcp backlog entry

583b7c7

In case of failed to get source address from the net packet, release the assgined tcp backlog entry. Otherwise it will never be freed. Signed-off-by: Ravi kumar Veeramally <[email protected]>

rveerama1 force-pushed the tcp-fixes branch from 3146804 to d3960d3 Compare February 13, 2018 09:57

pfalcon approved these changes Feb 13, 2018

View reviewed changes

jukkar approved these changes Feb 13, 2018

View reviewed changes

jukkar merged commit 18422de into zephyrproject-rtos:master Feb 13, 2018

pfalcon mentioned this pull request Feb 13, 2018

net: TCP retransmit queue implementation is broken #5857

Closed

TCP related and other fixes #6058

TCP related and other fixes #6058

Conversation

rveerama1 commented Feb 8, 2018

codecov-io commented Feb 8, 2018 • edited Loading

Codecov Report

pfalcon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pfalcon commented Feb 8, 2018

nashif commented Feb 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jukkar commented Feb 9, 2018

rveerama1 Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rveerama1 left a comment

Choose a reason for hiding this comment

rveerama1 commented Feb 9, 2018

pfalcon commented Feb 9, 2018

rveerama1 commented Feb 9, 2018

jukkar left a comment

Choose a reason for hiding this comment

jukkar commented Feb 12, 2018

Choose a reason for hiding this comment

pfalcon commented Feb 12, 2018

rveerama1 commented Feb 13, 2018

pfalcon left a comment

Choose a reason for hiding this comment

codecov-io commented Feb 8, 2018 •

edited

Loading

rveerama1 Feb 9, 2018 •

edited

Loading