From 0525cb25f8d4a1914f8a9d8ead8868e0278eef9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Gl=C3=B6ckner?= Date: Mon, 18 Mar 2019 21:35:43 +0100 Subject: [PATCH] net: tcp: fix deadlock on slow peer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unlocks the mutex needed by the code for processing ACKs while waiting for TX packets to become available. Before the mutex can be unlocked, it must be locked by tcp_syn_rcvd(), tcp_synack_received(), and backlog_ack_timeout(), which in turn requires us to unlock it in net_tcp_connect() while waiting for tcp_synack_received() to be called. Fixes #14571 Signed-off-by: Daniel Glöckner --- subsys/net/ip/net_context.c | 37 ++++++++++++--------- subsys/net/ip/tcp.c | 65 +++++++++++++++++++++++++------------ tests/net/tcp/src/main.c | 24 ++++++++++++++ 3 files changed, 89 insertions(+), 37 deletions(-) diff --git a/subsys/net/ip/net_context.c b/subsys/net/ip/net_context.c index 6ebe27868316b..5f8df10f8979c 100644 --- a/subsys/net/ip/net_context.c +++ b/subsys/net/ip/net_context.c @@ -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); } diff --git a/subsys/net/ip/tcp.c b/subsys/net/ip/tcp.c index 49b44dd4c79c1..66972f28f82c3 100644 --- a/subsys/net/ip/tcp.c +++ b/subsys/net/ip/tcp.c @@ -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; } @@ -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)); } @@ -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); + NET_ASSERT(context->tcp); /* protected by lock */ switch (net_tcp_get_state(context->tcp)) { case NET_TCP_SYN_SENT: @@ -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); @@ -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)); @@ -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) { @@ -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); @@ -2181,6 +2192,8 @@ NET_CONN_CB(tcp_synack_received) } } +unlock: + k_mutex_unlock(&context->lock); return NET_DROP; } @@ -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; @@ -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); @@ -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); @@ -2307,7 +2323,7 @@ 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, @@ -2315,7 +2331,8 @@ NET_CONN_CB(tcp_syn_rcvd) &pkt_src_addr); send_syn_ack(context, &pkt_src_addr, &remote_addr); net_pkt_unref(pkt); - return NET_OK; + verdict = NET_OK; + goto unlock; } /* @@ -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; } /* @@ -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, @@ -2432,10 +2449,10 @@ 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)); @@ -2443,7 +2460,9 @@ NET_CONN_CB(tcp_syn_rcvd) 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, @@ -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, diff --git a/tests/net/tcp/src/main.c b/tests/net/tcp/src/main.c index e4f9aeb9322ad..d862bb8a5bb61 100644 --- a/tests/net/tcp/src/main.c +++ b/tests/net/tcp/src/main.c @@ -196,10 +196,12 @@ static void v6_send_syn_ack(struct net_pkt *req) struct net_tcp_hdr *tcp_rsp, *tcp_req; int ret; + k_mutex_lock(&reply_v6_ctx->lock, K_FOREVER); ret = net_tcp_prepare_segment(reply_v6_ctx->tcp, NET_TCP_SYN | NET_TCP_ACK, NULL, 0, NULL, (struct sockaddr *)&my_v6_addr, &rsp); + k_mutex_unlock(&reply_v6_ctx->lock); if (ret) { DBG("TCP packet creation failed\n"); return; @@ -964,8 +966,10 @@ static bool test_create_v6_reset_packet(void) struct net_tcp_hdr hdr, *tcp_hdr; int ret; + k_mutex_lock(&v6_ctx->lock, K_FOREVER); ret = net_tcp_prepare_segment(tcp, flags, NULL, 0, NULL, (struct sockaddr *)&peer_v6_addr, &pkt); + k_mutex_unlock(&v6_ctx->lock); if (ret) { DBG("Prepare segment failed (%d)\n", ret); return false; @@ -1001,8 +1005,10 @@ static bool test_create_v4_reset_packet(void) struct net_tcp_hdr hdr, *tcp_hdr; int ret; + k_mutex_lock(&v4_ctx->lock, K_FOREVER); ret = net_tcp_prepare_segment(tcp, flags, NULL, 0, NULL, (struct sockaddr *)&peer_v4_addr, &pkt); + k_mutex_unlock(&v4_ctx->lock); if (ret) { DBG("Prepare segment failed (%d)\n", ret); return false; @@ -1038,8 +1044,10 @@ static bool test_create_v6_syn_packet(void) struct net_tcp_hdr hdr, *tcp_hdr; int ret; + k_mutex_lock(&v6_ctx->lock, K_FOREVER); ret = net_tcp_prepare_segment(tcp, flags, NULL, 0, NULL, (struct sockaddr *)&peer_v6_addr, &pkt); + k_mutex_unlock(&v6_ctx->lock); if (ret) { DBG("Prepare segment failed (%d)\n", ret); return false; @@ -1075,8 +1083,10 @@ static bool test_create_v4_syn_packet(void) struct net_tcp_hdr hdr, *tcp_hdr; int ret; + k_mutex_lock(&v4_ctx->lock, K_FOREVER); ret = net_tcp_prepare_segment(tcp, flags, NULL, 0, NULL, (struct sockaddr *)&peer_v4_addr, &pkt); + k_mutex_unlock(&v4_ctx->lock); if (ret) { DBG("Prepare segment failed (%d)\n", ret); return false; @@ -1112,8 +1122,10 @@ static bool test_create_v6_synack_packet(void) struct net_tcp_hdr hdr, *tcp_hdr; int ret; + k_mutex_lock(&v6_ctx->lock, K_FOREVER); ret = net_tcp_prepare_segment(tcp, flags, NULL, 0, NULL, (struct sockaddr *)&peer_v6_addr, &pkt); + k_mutex_unlock(&v6_ctx->lock); if (ret) { DBG("Prepare segment failed (%d)\n", ret); return false; @@ -1150,8 +1162,10 @@ static bool test_create_v4_synack_packet(void) struct net_tcp_hdr hdr, *tcp_hdr; int ret; + k_mutex_lock(&v4_ctx->lock, K_FOREVER); ret = net_tcp_prepare_segment(tcp, flags, NULL, 0, NULL, (struct sockaddr *)&peer_v4_addr, &pkt); + k_mutex_unlock(&v4_ctx->lock); if (ret) { DBG("Prepare segment failed (%d)\n", ret); return false; @@ -1188,8 +1202,10 @@ static bool test_create_v6_fin_packet(void) struct net_tcp_hdr hdr, *tcp_hdr; int ret; + k_mutex_lock(&v6_ctx->lock, K_FOREVER); ret = net_tcp_prepare_segment(tcp, flags, NULL, 0, NULL, (struct sockaddr *)&peer_v6_addr, &pkt); + k_mutex_unlock(&v6_ctx->lock); if (ret) { DBG("Prepare segment failed (%d)\n", ret); return false; @@ -1225,8 +1241,10 @@ static bool test_create_v4_fin_packet(void) struct net_tcp_hdr hdr, *tcp_hdr; int ret; + k_mutex_lock(&v4_ctx->lock, K_FOREVER); ret = net_tcp_prepare_segment(tcp, flags, NULL, 0, NULL, (struct sockaddr *)&peer_v4_addr, &pkt); + k_mutex_unlock(&v4_ctx->lock); if (ret) { DBG("Prepare segment failed (%d)\n", ret); return false; @@ -1263,8 +1281,10 @@ static bool test_v6_seq_check(void) u32_t seq; int ret; + k_mutex_lock(&v6_ctx->lock, K_FOREVER); ret = net_tcp_prepare_segment(tcp, flags, NULL, 0, NULL, (struct sockaddr *)&peer_v6_addr, &pkt); + k_mutex_unlock(&v6_ctx->lock); if (ret) { DBG("Prepare segment failed (%d)\n", ret); return false; @@ -1298,8 +1318,10 @@ static bool test_v4_seq_check(void) u32_t seq; int ret; + k_mutex_lock(&v4_ctx->lock, K_FOREVER); ret = net_tcp_prepare_segment(tcp, flags, NULL, 0, NULL, (struct sockaddr *)&peer_v4_addr, &pkt); + k_mutex_unlock(&v4_ctx->lock); if (ret) { DBG("Prepare segment failed (%d)\n", ret); return false; @@ -1486,8 +1508,10 @@ static bool test_tcp_seq_validity(void) struct net_tcp_hdr hdr, *tcp_hdr; int ret; + k_mutex_lock(&v6_ctx->lock, K_FOREVER); ret = net_tcp_prepare_segment(tcp, flags, NULL, 0, NULL, (struct sockaddr *)&peer_v6_addr, &pkt); + k_mutex_unlock(&v6_ctx->lock); if (ret) { DBG("Prepare segment failed (%d)\n", ret); return false;