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 <nathan.skrzypczak@gmail.com>

1 files 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;