Skip to content

TCP: fix deadlock on slow peer #14944

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

Closed
wants to merge 1 commit into from
Closed

TCP: fix deadlock on slow peer #14944

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
37 changes: 21 additions & 16 deletions subsys/net/ip/net_context.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1229,32 +1229,37 @@ static struct net_pkt *context_alloc_pkt(struct net_context *context,
size_t len, s32_t timeout)
{
struct net_pkt *pkt;
struct net_if *interface = net_context_get_iface(context);
sa_family_t family = net_context_get_family(context);
u16_t ip_proto = net_context_get_ip_proto(context);

#if defined(CONFIG_NET_CONTEXT_NET_PKT_POOL)
if (context->tx_slab) {
pkt = net_pkt_alloc_from_slab(context->tx_slab(), timeout);
if (!pkt) {
return NULL;
}
struct k_mem_slab *slab = context->tx_slab();

net_pkt_set_iface(pkt, net_context_get_iface(context));
net_pkt_set_family(pkt, net_context_get_family(context));
net_pkt_set_context(pkt, context);

if (net_pkt_alloc_buffer(pkt, len,
net_context_get_ip_proto(context),
timeout)) {
net_pkt_unref(pkt);
return NULL;
k_mutex_unlock(&context->lock);
pkt = net_pkt_alloc_from_slab(slab, timeout);
if (pkt != NULL) {
net_pkt_set_iface(pkt, interface);
net_pkt_set_family(pkt, family);
net_pkt_set_context(pkt, context);

if (net_pkt_alloc_buffer(pkt, len,
ip_proto,
timeout) < 0) {
net_pkt_unref(pkt);
pkt = NULL;
}
}

k_mutex_lock(&context->lock, K_FOREVER);
return pkt;
}
#endif
pkt = net_pkt_alloc_with_buffer(net_context_get_iface(context), len,
net_context_get_family(context),
net_context_get_ip_proto(context),
k_mutex_unlock(&context->lock);
pkt = net_pkt_alloc_with_buffer(interface, len, family, ip_proto,
timeout);
k_mutex_lock(&context->lock, K_FOREVER);
if (pkt) {
net_pkt_set_context(pkt, context);
}
Expand Down
65 changes: 44 additions & 21 deletions subsys/net/ip/tcp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,16 +396,23 @@ static int prepare_segment(struct net_tcp *tcp,
pkt->buffer = NULL;
pkt_allocated = false;

k_mutex_unlock(&context->lock);
status = net_pkt_alloc_buffer(pkt, segment->optlen,
IPPROTO_TCP, ALLOC_TIMEOUT);
k_mutex_lock(&context->lock, K_FOREVER);
if (status) {
goto fail;
}
} else {
pkt = net_pkt_alloc_with_buffer(net_context_get_iface(context),
struct net_if *interface = net_context_get_iface(context);
sa_family_t family = net_context_get_family(context);

k_mutex_unlock(&context->lock);
pkt = net_pkt_alloc_with_buffer(interface,
segment->optlen,
net_context_get_family(context),
family,
IPPROTO_TCP, ALLOC_TIMEOUT);
k_mutex_lock(&context->lock, K_FOREVER);
if (!pkt) {
return -ENOMEM;
}
Expand Down Expand Up @@ -1459,7 +1466,9 @@ static void backlog_ack_timeout(struct k_work *work)
* RST packet might be invalid. Cache local address
* and use it in RST message preparation.
*/
k_mutex_lock(&backlog->tcp->context->lock, K_FOREVER);
send_reset(backlog->tcp->context, NULL, &backlog->remote);
k_mutex_unlock(&backlog->tcp->context->lock);

(void)memset(backlog, 0, sizeof(struct tcp_backlog_entry));
}
Expand Down Expand Up @@ -2099,7 +2108,9 @@ NET_CONN_CB(tcp_synack_received)
struct net_tcp_hdr *tcp_hdr = proto_hdr->tcp;
int ret;

NET_ASSERT(context && context->tcp);
NET_ASSERT(context);
k_mutex_lock(&context->lock, K_FOREVER);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is lock stuffed between 2 asserts here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because context->tcp can in theory change if someone else is holding the lock.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Because context->tcp can in theory change if someone else is holding the lock.

Ok, ack. I bet I'd personally add a comment on that.

NET_ASSERT(context->tcp); /* protected by lock */

switch (net_tcp_get_state(context->tcp)) {
case NET_TCP_SYN_SENT:
Expand All @@ -2108,7 +2119,7 @@ NET_CONN_CB(tcp_synack_received)
default:
NET_DBG("Context %p in wrong state %d",
context, net_tcp_get_state(context->tcp));
return NET_DROP;
goto unlock;
}

net_pkt_set_context(pkt, context);
Expand All @@ -2119,7 +2130,7 @@ NET_CONN_CB(tcp_synack_received)
/* We only accept RST packet that has valid seq field. */
if (!net_tcp_validate_seq(context->tcp, tcp_hdr)) {
net_stats_update_tcp_seg_rsterr(net_pkt_iface(pkt));
return NET_DROP;
goto unlock;
}

net_stats_update_tcp_seg_rst(net_pkt_iface(pkt));
Expand All @@ -2131,7 +2142,7 @@ NET_CONN_CB(tcp_synack_received)
context->user_data);
}

return NET_DROP;
goto unlock;
}

if (NET_TCP_FLAGS(tcp_hdr) & NET_TCP_SYN) {
Expand Down Expand Up @@ -2166,7 +2177,7 @@ NET_CONN_CB(tcp_synack_received)
if (ret < 0) {
NET_DBG("Cannot register TCP handler (%d)", ret);
send_reset(context, &local_addr, &remote_addr);
return NET_DROP;
goto unlock;
}

net_tcp_change_state(context->tcp, NET_TCP_ESTABLISHED);
Expand All @@ -2181,6 +2192,8 @@ NET_CONN_CB(tcp_synack_received)
}
}

unlock:
k_mutex_unlock(&context->lock);
return NET_DROP;
}

Expand Down Expand Up @@ -2239,8 +2252,11 @@ NET_CONN_CB(tcp_syn_rcvd)
struct sockaddr_ptr pkt_src_addr;
struct sockaddr local_addr;
struct sockaddr remote_addr;
enum net_verdict verdict = NET_DROP;

NET_ASSERT(context && context->tcp);
NET_ASSERT(context);
k_mutex_lock(&context->lock, K_FOREVER);
NET_ASSERT(context->tcp); /* protected by lock */

tcp = context->tcp;

Expand All @@ -2250,13 +2266,13 @@ NET_CONN_CB(tcp_syn_rcvd)
break;
case NET_TCP_SYN_RCVD:
if (net_pkt_iface(pkt) != net_context_get_iface(context)) {
return NET_DROP;
goto unlock;
}
break;
default:
NET_DBG("Context %p in wrong state %d",
context, tcp->state);
return NET_DROP;
goto unlock;
}

net_pkt_set_context(pkt, context);
Expand Down Expand Up @@ -2286,7 +2302,7 @@ NET_CONN_CB(tcp_syn_rcvd)
* so call unconditionally.
*/
if (net_tcp_parse_opts(pkt, opt_totlen, &tcp_opts) < 0) {
return NET_DROP;
goto unlock;
}

net_tcp_change_state(tcp, NET_TCP_SYN_RCVD);
Expand All @@ -2307,15 +2323,16 @@ NET_CONN_CB(tcp_syn_rcvd)
NET_DBG("No free TCP backlog entries");
}

return NET_DROP;
goto unlock;
}

get_sockaddr_ptr(ip_hdr, tcp_hdr,
net_context_get_family(context),
&pkt_src_addr);
send_syn_ack(context, &pkt_src_addr, &remote_addr);
net_pkt_unref(pkt);
return NET_OK;
verdict = NET_OK;
goto unlock;
}

/*
Expand All @@ -2326,14 +2343,14 @@ NET_CONN_CB(tcp_syn_rcvd)

if (tcp_backlog_rst(pkt, ip_hdr, tcp_hdr) < 0) {
net_stats_update_tcp_seg_rsterr(net_pkt_iface(pkt));
return NET_DROP;
goto unlock;
}

net_stats_update_tcp_seg_rst(net_pkt_iface(pkt));

net_tcp_print_recv_info("RST", pkt, tcp_hdr->src_port);

return NET_DROP;
goto unlock;
}

/*
Expand Down Expand Up @@ -2423,7 +2440,7 @@ NET_CONN_CB(tcp_syn_rcvd)
NET_ASSERT_INFO(false, "Invalid protocol family %d",
new_context->remote.sa_family);
net_context_unref(new_context);
return NET_DROP;
goto unlock;
}

context->tcp->accept_cb(new_context,
Expand All @@ -2432,18 +2449,20 @@ NET_CONN_CB(tcp_syn_rcvd)
0,
context->user_data);
net_pkt_unref(pkt);
return NET_OK;
verdict = NET_OK;
}

return NET_DROP;
goto unlock;

conndrop:
net_stats_update_tcp_seg_conndrop(net_pkt_iface(pkt));

reset:
send_reset(tcp->context, &local_addr, &remote_addr);

return NET_DROP;
unlock:
k_mutex_unlock(&context->lock);
return verdict;
}

int net_tcp_accept(struct net_context *context,
Expand Down Expand Up @@ -2563,12 +2582,16 @@ int net_tcp_connect(struct net_context *context,

send_syn(context, addr);

k_mutex_unlock(&context->lock);
/* in tcp_synack_received() we give back this semaphore */
if (timeout != 0 && k_sem_take(&context->tcp->connect_wait, timeout)) {
return -ETIMEDOUT;
ret = -ETIMEDOUT;
} else {
ret = 0;
}
k_mutex_lock(&context->lock, K_FOREVER);

return 0;
return ret;
}

struct net_tcp_hdr *net_tcp_input(struct net_pkt *pkt,
Expand Down
Loading