From d9577b4aa3a325e46c09796835d22122bec9e3d0 Mon Sep 17 00:00:00 2001 From: Nathan Skrzypczak Date: Mon, 2 Dec 2019 17:01:40 +0100 Subject: [PATCH] quic: clean accept/connect error codepath Type: fix First attempt to clean the leftover state when accept_notify / connect_notify fails due to mq size constraints. vpp should now be left in a state such that clean state will eventually be reached when timers fire. Change-Id: I9e1166dab2778bf05d5af42d437769651369cae0 Signed-off-by: Nathan Skrzypczak --- src/plugins/quic/quic.c | 99 +++++++++++++++++++++++++------------------------ 1 file changed, 51 insertions(+), 48 deletions(-) diff --git a/src/plugins/quic/quic.c b/src/plugins/quic/quic.c index 6c2dd6af316..d1f188443d6 100644 --- a/src/plugins/quic/quic.c +++ b/src/plugins/quic/quic.c @@ -38,7 +38,11 @@ static char *quic_error_strings[] = { static quic_main_t quic_main; static void quic_update_timer (quic_ctx_t * ctx); -static int quic_check_quic_session_connected (quic_ctx_t * ctx); +static void quic_check_quic_session_connected (quic_ctx_t * ctx); +static int quic_reset_connection (u64 udp_session_handle, + quic_rx_packet_ctx_t * pctx); +static void quic_proto_on_close (u32 ctx_index, u32 thread_index); + static quicly_stream_open_t on_stream_open; static quicly_closed_by_peer_t on_closed_by_peer; static quicly_now_t quicly_vpp_now_cb; @@ -383,9 +387,8 @@ quic_connection_closed (quic_ctx_t * ctx) /* App already confirmed close, we can delete the connection */ quic_connection_delete (ctx); break; - case QUIC_CONN_STATE_PASSIVE_CLOSING_QUIC_CLOSED: - QUIC_DBG (0, "BUG"); - break; + case QUIC_CONN_STATE_OPENED: + case QUIC_CONN_STATE_HANDSHAKE: case QUIC_CONN_STATE_ACTIVE_CLOSING: quic_connection_delete (ctx); break; @@ -677,9 +680,9 @@ int quic_fifo_egress_emit (quicly_stream_t * stream, size_t off, void *dst, size_t * len, int *wrote_all) { + u32 deq_max, first_deq, max_rd_chunk, rem_offset; session_t *stream_session; svm_fifo_t *f; - u32 deq_max, first_deq, max_rd_chunk, rem_offset; stream_session = get_stream_session_from_stream (stream); f = stream_session->tx_fifo; @@ -731,6 +734,11 @@ static const quicly_stream_callbacks_t quic_stream_callbacks = { static int quic_on_stream_open (quicly_stream_open_t * self, quicly_stream_t * stream) { + /* Return code for this function ends either + * - in quicly_receive : if not QUICLY_ERROR_PACKET_IGNORED, will close connection + * - in quicly_open_stream, returned directly + */ + session_t *stream_session, *quic_session; quic_stream_data_t *stream_data; app_worker_t *app_wrk; @@ -786,7 +794,6 @@ quic_on_stream_open (quicly_stream_open_t * self, quicly_stream_t * stream) if ((rv = app_worker_init_connected (app_wrk, stream_session))) { QUIC_ERR ("failed to allocate fifos"); - session_free (stream_session); quicly_reset_stream (stream, QUIC_APP_ALLOCATION_ERROR); return 0; /* Frame is still valid */ } @@ -797,7 +804,6 @@ quic_on_stream_open (quicly_stream_open_t * self, quicly_stream_t * stream) if ((rv = app_worker_accept_notify (app_wrk, stream_session))) { QUIC_ERR ("failed to notify accept worker app"); - session_free_w_fifos (stream_session); quicly_reset_stream (stream, QUIC_APP_ACCEPT_NOTIFY_ERROR); return 0; /* Frame is still valid */ } @@ -1014,13 +1020,17 @@ quic_connect_stream (session_t * quic_session, u32 opaque) session_type_from_proto_and_ip (TRANSPORT_PROTO_QUIC, qctx->udp_is_ip4); sctx->c_s_index = stream_session->session_index; + stream_data = (quic_stream_data_t *) stream->data; + stream_data->ctx_id = sctx->c_c_index; + stream_data->thread_index = sctx->c_thread_index; + stream_data->app_rx_data_len = 0; + stream_session->session_state = SESSION_STATE_READY; + /* For now we only reset streams. Cleanup will be triggered by timers */ if (app_worker_init_connected (app_wrk, stream_session)) { QUIC_ERR ("failed to app_worker_init_connected"); - quicly_reset_stream (stream, QUIC_APP_ALLOCATION_ERROR); - session_free_w_fifos (stream_session); - quic_ctx_free (sctx); + quicly_reset_stream (stream, QUIC_APP_CONNECT_NOTIFY_ERROR); return app_worker_connect_notify (app_wrk, NULL, opaque); } @@ -1028,19 +1038,13 @@ quic_connect_stream (session_t * quic_session, u32 opaque) SVM_FIFO_WANT_DEQ_NOTIF_IF_FULL | SVM_FIFO_WANT_DEQ_NOTIF_IF_EMPTY); - stream_session->session_state = SESSION_STATE_READY; if (app_worker_connect_notify (app_wrk, stream_session, opaque)) { QUIC_ERR ("failed to notify app"); quicly_reset_stream (stream, QUIC_APP_CONNECT_NOTIFY_ERROR); - session_free_w_fifos (stream_session); - quic_ctx_free (sctx); return -1; } - stream_data = (quic_stream_data_t *) stream->data; - stream_data->ctx_id = sctx->c_c_index; - stream_data->thread_index = sctx->c_thread_index; - stream_data->app_rx_data_len = 0; + return 0; } @@ -1134,6 +1138,8 @@ quic_proto_on_close (u32 ctx_index, u32 thread_index) switch (ctx->conn_state) { + case QUIC_CONN_STATE_OPENED: + case QUIC_CONN_STATE_HANDSHAKE: case QUIC_CONN_STATE_READY: ctx->conn_state = QUIC_CONN_STATE_ACTIVE_CLOSING; quicly_conn_t *conn = ctx->conn; @@ -1151,8 +1157,10 @@ quic_proto_on_close (u32 ctx_index, u32 thread_index) case QUIC_CONN_STATE_PASSIVE_CLOSING_QUIC_CLOSED: quic_connection_delete (ctx); break; + case QUIC_CONN_STATE_ACTIVE_CLOSING: + break; default: - QUIC_DBG (0, "BUG"); + QUIC_ERR ("Trying to close conn in state %d", ctx->conn_state); break; } } @@ -1348,7 +1356,7 @@ quic_build_sockaddr (struct sockaddr *sa, socklen_t * salen, } } -static int +static void quic_on_quic_session_connected (quic_ctx_t * ctx) { session_t *quic_session; @@ -1357,13 +1365,6 @@ quic_on_quic_session_connected (quic_ctx_t * ctx) u32 thread_index = ctx->c_thread_index; int rv; - app_wrk = app_worker_get_if_valid (ctx->parent_app_wrk_id); - if (!app_wrk) - { - quic_disconnect_transport (ctx); - return 0; - } - quic_session = session_alloc (thread_index); QUIC_DBG (2, "Allocated quic session 0x%lx", session_handle (quic_session)); @@ -1374,11 +1375,14 @@ quic_on_quic_session_connected (quic_ctx_t * ctx) quic_session->session_type = session_type_from_proto_and_ip (TRANSPORT_PROTO_QUIC, ctx->udp_is_ip4); + /* If quic session connected fails, immediatly close connection */ + app_wrk = app_worker_get (ctx->parent_app_wrk_id); if (app_worker_init_connected (app_wrk, quic_session)) { QUIC_ERR ("failed to app_worker_init_connected"); quic_proto_on_close (ctx_id, thread_index); - return app_worker_connect_notify (app_wrk, NULL, ctx->client_opaque); + app_worker_connect_notify (app_wrk, NULL, ctx->client_opaque); + return; } quic_session->session_state = SESSION_STATE_CONNECTING; @@ -1387,7 +1391,7 @@ quic_on_quic_session_connected (quic_ctx_t * ctx) { QUIC_ERR ("failed to notify app %d", rv); quic_proto_on_close (ctx_id, thread_index); - return -1; + return; } /* If the app opens a stream in its callback it may invalidate ctx */ @@ -1398,11 +1402,9 @@ quic_on_quic_session_connected (quic_ctx_t * ctx) */ quic_session = session_get (ctx->c_s_index, thread_index); quic_session->session_state = SESSION_STATE_LISTENING; - - return 0; } -static int +static void quic_check_quic_session_connected (quic_ctx_t * ctx) { /* Called when we need to trigger quic session connected @@ -1411,13 +1413,13 @@ quic_check_quic_session_connected (quic_ctx_t * ctx) /* Conn may be set to null if the connection is terminated */ if (!ctx->conn || ctx->conn_state != QUIC_CONN_STATE_HANDSHAKE) - return 0; + return; if (!quicly_connection_is_ready (ctx->conn)) - return 0; + return; ctx->conn_state = QUIC_CONN_STATE_READY; if (!quicly_is_client (ctx->conn)) - return 0; - return quic_on_quic_session_connected (ctx); + return; + quic_on_quic_session_connected (ctx); } static void @@ -1776,7 +1778,7 @@ quic_accept_connection (u32 ctx_index, quic_rx_packet_ctx_t * pctx) if (ctx->c_s_index != QUIC_SESSION_INVALID) { QUIC_DBG (2, "already accepted ctx 0x%x", ctx_index); - return -1; + return 0; } quicly_ctx = quic_get_quicly_ctx_from_ctx (ctx); @@ -1795,7 +1797,6 @@ quic_accept_connection (u32 ctx_index, quic_rx_packet_ctx_t * pctx) /* Save ctx handle in quicly connection */ quic_store_conn_ctx (conn, ctx); ctx->conn = conn; - ctx->conn_state = QUIC_CONN_STATE_HANDSHAKE; quic_session = session_alloc (ctx->c_thread_index); QUIC_DBG (2, "Allocated quic_session, 0x%lx ctx %u", @@ -1811,26 +1812,28 @@ quic_accept_connection (u32 ctx_index, quic_rx_packet_ctx_t * pctx) session_type_from_proto_and_ip (TRANSPORT_PROTO_QUIC, ctx->udp_is_ip4); quic_session->listener_handle = lctx->c_s_index; - /* TODO: don't alloc fifos when we don't transfer data on this session - * but we still need fifos for the events? */ + /* Register connection in connections map */ + quic_make_connection_key (&kv, quicly_get_master_id (conn)); + kv.value = ((u64) thread_index) << 32 | (u64) ctx_index; + clib_bihash_add_del_16_8 (&quic_main.connection_hash, &kv, 1 /* is_add */ ); + QUIC_DBG (2, "Registering conn with id %lu %lu", kv.key[0], kv.key[1]); + + /* If notify fails, reset connection immediatly */ if ((rv = app_worker_init_accepted (quic_session))) { QUIC_ERR ("failed to allocate fifos"); - session_free (quic_session); + quic_proto_on_close (ctx_index, thread_index); return rv; } + app_wrk = app_worker_get (quic_session->app_wrk_index); if ((rv = app_worker_accept_notify (app_wrk, quic_session))) { QUIC_ERR ("failed to notify accept worker app"); + quic_proto_on_close (ctx_index, thread_index); return rv; } - - /* Register connection in connections map */ - quic_make_connection_key (&kv, quicly_get_master_id (conn)); - kv.value = ((u64) thread_index) << 32 | (u64) ctx_index; - clib_bihash_add_del_16_8 (&quic_main.connection_hash, &kv, 1 /* is_add */ ); - QUIC_DBG (2, "Registering conn with id %lu %lu", kv.key[0], kv.key[1]); + ctx->conn_state = QUIC_CONN_STATE_HANDSHAKE; return quic_send_packets (ctx); } @@ -1994,9 +1997,9 @@ rx_start: ctx = quic_ctx_get (packets_ctx[i].ctx_index, thread_index); rv = quicly_receive (ctx->conn, NULL, &packets_ctx[i].sa, &packets_ctx[i].packet); - if (rv) + if (rv && rv != QUICLY_ERROR_PACKET_IGNORED) { - QUIC_DBG (1, "quicly_receive return error %U", + QUIC_ERR ("quicly_receive return error %U", quic_format_err, rv); } break; -- 2.16.6