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.

Sign up for GitHub

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
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion subsys/net/ip/net_context.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -825,10 +825,19 @@ NET_CONN_CB(tcp_established)
}

set_appdata_values(pkt, IPPROTO_TCP);
context->tcp->send_ack += net_pkt_appdatalen(pkt);

uint16_t data_len = net_pkt_appdatalen(pkt);
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.

}
context->tcp->recv_wnd -= data_len;

ret = packet_received(conn, pkt, context->tcp->recv_user_data);

context->tcp->send_ack += data_len;

if (tcp_flags & NET_TCP_FIN) {
/* Sending an ACK in the CLOSE_WAIT state will transition to
* LAST_ACK state
Expand Down Expand Up @@ -2043,3 +2052,8 @@ void net_context_init(void)

k_sem_give(&contexts_lock);
}

void net_context_tcp_recved(struct net_context *context, unsigned int len)
{
context->tcp->recv_wnd += len;
}
4 changes: 3 additions & 1 deletion subsys/net/ip/tcp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ struct net_tcp *net_tcp_alloc(struct net_context *context)

tcp_context[i].send_seq = init_isn();
tcp_context[i].recv_max_ack = tcp_context[i].send_seq + 1u;
tcp_context[i].recv_wnd = min(NET_TCP_MAX_WIN, NET_TCP_BUF_MAX_LEN);

tcp_context[i].accept_cb = NULL;

Expand Down Expand Up @@ -356,8 +357,9 @@ static struct net_pkt *prepare_segment(struct net_tcp *tcp,
return pkt;
}

static inline u32_t get_recv_wnd(struct net_tcp *tcp)
u32_t get_recv_wnd(struct net_tcp *tcp)
{
return tcp->recv_wnd;
ARG_UNUSED(tcp);

/* We don't queue received data inside the stack, we hand off
Expand Down
2 changes: 2 additions & 0 deletions subsys/net/ip/tcp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,8 @@ struct net_tcp {
* Semaphore to signal TCP connection completion
*/
struct k_sem connect_wait;

uint16_t recv_wnd;
};

static inline bool net_tcp_is_used(struct net_tcp *tcp)
Expand Down