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

[WIP] net: tcp: Explicitly manage TCP receive window. #81

Closed
wants to merge 1 commit into from

Conversation

pfalcon
Copy link
Contributor

@pfalcon pfalcon commented May 4, 2017

Challange the existing situation that "if application buffers data,
it's the problem of application". It's actually the problem of the
stack, as it doesn't allow application to control receive window,
and without this control, any buffer will overflow, peer packets
will be dropped, peer won't receive acks for them, and will employ
exponential backoff, the connection will crawl to a halt.

This patch adds net_context_tcp_recved() function which an
application must explicitly call when it processes data, to
advance receive window.

Jira: ZEP-1999

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

Challange the existing situation that "if application buffers data,
it's the problem of application". It's actually the problem of the
stack, as it doesn't allow application to control receive window,
and without this control, any buffer will overflow, peer packets
will be dropped, peer won't receive acks for them, and will employ
exponential backoff, the connection will crawl to a halt.

This patch adds net_context_tcp_recved() function which an
application must explicitly call when it *processes* data, to
advance receive window.

Jira: ZEP-1999

Change-Id: Id7255df3d4898e289a2d20e7a02fd5f3f8f05291
Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>
@nashif 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 4, 2017
@pfalcon
Copy link
Contributor Author

pfalcon commented May 4, 2017

This is trivial implementation of TCP receive window handling, to back discussion on how to do it properly in JIRA: GH-3440 and on mailing list: https://lists.zephyrproject.org/pipermail/zephyr-devel/2017-April/007621.html

@lpereira
Copy link
Collaborator

lpereira commented May 9, 2017

Maybe @andyross will want to take a look here.

@pfalcon
Copy link
Contributor Author

pfalcon commented May 10, 2017

@lpereira : Thanks for the ping. I actually wait for continuation of @andyross' review on #26 , and there's another which warrants his look ahead of this as it touches code written by him: #137.

@jukkar jukkar requested a review from andyross May 12, 2017 18:56
@jukkar jukkar added the net label May 12, 2017
if (data_len > get_recv_wnd(context->tcp)) {
NET_ERR("Context %p: overflow of recv window (%d vs %d), pkt dropped",
context, get_recv_wnd(context->tcp), data_len);
return NET_DROP;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like the wrong behavior. I mean, yeah, it's a stack bug on the other side and they blew the limit you set. Nonetheless, they transmitted the data and we received it without overflow. That's not an error condition, we should present the data to the app and attempt to continue.

I mean, just to geek out: I went to get a quote for the "be conservative in what you emit but liberal in what you accept" maxim and was reminded by wikipedia that it's literally from the TCP RFC (thus "Postel's Law"):

TCP implementations should follow a general principle of robustness: be conservative in what you do, be liberal in what you accept from others.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that's a dream scenario of any DoS attacker - they send you useless stuff which you try to hold, while dropping real precious things, because all your hands are tied with useless stuff.

@andyross
Copy link
Contributor

Really not liking this API. For one, it's asymmetric with respect to other net_context implementations. You need to know that your context is a TCP connection and use it differently than you do other types. Secondly, it's non-standard: other OSes don't do anything like this.

Third, I don't see value it provides. The only reason TCP windows exist (for our purposes, anyway) is to prevent buffer overflows in the receiver. I don't see how this helps the app do that.

If you want a way for the app to explicitly set windows, then (1) make it optional and (2) make it abstract. How about something like Linux's TCP_WINDOW_CLAMP sockopt instead?

@pfalcon
Copy link
Contributor Author

pfalcon commented May 12, 2017

You need to know that your context is a TCP connection and use it differently than you do other types.

You need to know the type of a socket - STREAM or DGRAM to work with it properly.

Secondly, it's non-standard: other OSes don't do anything like this.

What OSes do you mean here? Those which can no longer work with a gigibyte of RAM, all those linuxes, windowses, and macosxes? The mechanism presented in this patch is a direct translation of the same mechanism of lwIP, which is not an OS, but a portable reusable IP stack which works with almost any RTOS out there, a golden standard in embedded IP stacks, used in probably 80% of projects, tested and tried for a decade or two.

All these points were given in detail in the RFC which is by the link given above: https://lists.zephyrproject.org/pipermail/zephyr-devel/2017-May/007637.html .

Note that in further discussion of that IRC, I subscribed to a research of doing it "how big OSes do": https://lists.zephyrproject.org/pipermail/zephyr-devel/2017-May/007637.html . But until that code is written and tested to give the needed effect, the simple changes presented in this patch are the only ones needed to correctly manage TCP control flow.

The only reason TCP windows exist (for our purposes, anyway) is to prevent buffer overflows in the receiver. I don't see how this helps the app do that.

Please see GH-3440?focusedCommentId=18206&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-18206

@pfalcon pfalcon changed the title WIP net: tcp: Explicitly manage TCP receive window. [WIP, 1.9] net: tcp: Explicitly manage TCP receive window. May 15, 2017
@nashif
Copy link
Member

nashif commented Jun 22, 2017

what is the status of this one?

@nashif
Copy link
Member

nashif commented Jul 22, 2017

Can this be closed?

@pfalcon pfalcon changed the title [WIP, 1.9] net: tcp: Explicitly manage TCP receive window. [WIP] net: tcp: Explicitly manage TCP receive window. Aug 3, 2017
@pfalcon
Copy link
Contributor Author

pfalcon commented Aug 3, 2017

Thanks for removing 1.9 milestone. As this is a serious issue (blocker for large TCP xfers), I'd prefer to close this when an alternative patch will be available (which I hope to work on soon).

@pfalcon
Copy link
Contributor Author

pfalcon commented Aug 3, 2017

Superseded by #1002

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In progress For PRs: is work in progress and should not be merged yet. For issues: Is being worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants