BUG/MEDIUM: quic: prevent crash on accept queue full

Handshake for quic_conn instances runs on a single non-chosen thread. On
completion, listener_accept() is performed to select the less loaded
thread before initializing connection instance. As such, quic_conn
instance is migrated to the thread with its upper connection.

In case accept queue is full, listener_accept() fallback to local accept
mode, which cause the connection to be assigned to the current thread.
However, this is not supported by QUIC as quic_conn instance is left on
the previously selected thread. In most cases, this will cause a
BUG_ON() due to a task manipulation from an outside thread.

To fix this, handle quic_conn thread rebind in multiple steps using the
new extended protocol API. Several operations have been moved from
qc_set_tid_affinity1() to newly defined qc_set_tid_affinity2(), in
particular CID TID update. This ensures that quic_conn instance is not
prematurely accessed on the new thread until accept queue push is
guaranteed to succeed.

qc_reset_tid_affinity() is also newly defined to reassign the newly
created tasks and tasklets to the current thread. This is necessary to
prevent the BUG_ON() crash described above.

This must be backported up to 2.8 after a period of observation. Note
that it depends on previous patch :
  MINOR: proto: extend connection thread rebind API

include/haproxy/quic_conn.h

@@ -177,8 +177,11 @@ void qc_kill_conn(struct quic_conn *qc);
 int qc_parse_hd_form(struct quic_rx_packet *pkt,
                      unsigned char **buf, const unsigned char *end);
 
-int qc_set_tid_affinity1(struct quic_conn *qc, uint new_tid, struct listener *new_li);
+int qc_set_tid_affinity1(struct quic_conn *qc, uint new_tid);
+void qc_set_tid_affinity2(struct quic_conn *qc, struct listener *new_li);
+void qc_reset_tid_affinity(struct quic_conn *qc);
 void qc_finalize_affinity_rebind(struct quic_conn *qc);
+
 int qc_handle_conn_migration(struct quic_conn *qc,
                              const struct sockaddr_storage *peer_addr,
                              const struct sockaddr_storage *local_addr);

src/proto_quic.c

@@ -62,6 +62,8 @@ static int quic_connect_server(struct connection *conn, int flags);
 static void quic_enable_listener(struct listener *listener);
 static void quic_disable_listener(struct listener *listener);
 static int quic_set_affinity1(struct connection *conn, int new_tid);
+static void quic_set_affinity2(struct connection *conn);
+static void quic_reset_affinity(struct connection *conn);
 
 /* Note: must not be declared <const> as its list will be overwritten */
 struct protocol proto_quic4 = {
@@ -81,6 +83,8 @@ struct protocol proto_quic4 = {
 	.get_dst        = quic_sock_get_dst,
 	.connect        = quic_connect_server,
 	.set_affinity1  = quic_set_affinity1,
+	.set_affinity2  = quic_set_affinity2,
+	.reset_affinity = quic_reset_affinity,
 
 	/* binding layer */
 	.rx_suspend     = udp_suspend_receiver,
@@ -125,6 +129,8 @@ struct protocol proto_quic6 = {
 	.get_dst        = quic_sock_get_dst,
 	.connect        = quic_connect_server,
 	.set_affinity1  = quic_set_affinity1,
+	.set_affinity2  = quic_set_affinity2,
+	.reset_affinity = quic_reset_affinity,
 
 	/* binding layer */
 	.rx_suspend     = udp_suspend_receiver,
@@ -671,7 +677,19 @@ static void quic_disable_listener(struct listener *l)
 static int quic_set_affinity1(struct connection *conn, int new_tid)
 {
 	struct quic_conn *qc = conn->handle.qc;
-	return qc_set_tid_affinity1(qc, new_tid, objt_listener(conn->target));
+	return qc_set_tid_affinity1(qc, new_tid);
+}
+
+static void quic_set_affinity2(struct connection *conn)
+{
+	struct quic_conn *qc = conn->handle.qc;
+	qc_set_tid_affinity2(qc, objt_listener(conn->target));
+}
+
+static void quic_reset_affinity(struct connection *conn)
+{
+	struct quic_conn *qc = conn->handle.qc;
+	qc_reset_tid_affinity(qc);
 }
 
 static int quic_alloc_dghdlrs(void)

src/quic_conn.c

@@ -1722,22 +1722,17 @@ void qc_notify_err(struct quic_conn *qc)
 	TRACE_LEAVE(QUIC_EV_CONN_CLOSE, qc);
 }
 
-/* Move a <qc> QUIC connection and its resources from the current thread to the
+/* Prepare <qc> QUIC connection rebinding to a new thread <new_tid>. Stop and
- * new one <new_tid> optionally in association with <new_li> (since it may need
+ * release associated tasks and tasklet and allocate new ones binded to the new
- * to change when migrating to a thread from a different group, otherwise leave
+ * thread.
- * it NULL). After this call, the connection cannot be dereferenced anymore on
- * the current thread.
  *
  * Returns 0 on success else non-zero.
  */
-int qc_set_tid_affinity1(struct quic_conn *qc, uint new_tid, struct listener *new_li)
+int qc_set_tid_affinity1(struct quic_conn *qc, uint new_tid)
 {
 	struct task *t1 = NULL, *t2 = NULL;
 	struct tasklet *t3 = NULL;
 
-	struct quic_connection_id *conn_id;
-	struct eb64_node *node;
-
 	TRACE_ENTER(QUIC_EV_CONN_SET_AFFINITY, qc);
 
 	/* Pre-allocate all required resources. This ensures we do not left a
@@ -1775,17 +1770,55 @@ int qc_set_tid_affinity1(struct quic_conn *qc, uint new_tid, struct listener *ne
 	qc->wait_event.tasklet->context = qc;
 	qc->wait_event.events = 0;
 
+	/* Remove conn from per-thread list instance. It will be hidden from
+	 * "show quic" until qc_finalize_affinity_rebind().
+	 */
+	qc_detach_th_ctx_list(qc, 0);
+
+	qc->flags |= QUIC_FL_CONN_AFFINITY_CHANGED;
+
+	TRACE_LEAVE(QUIC_EV_CONN_SET_AFFINITY, qc);
+	return 0;
+
+ err:
+	task_destroy(t1);
+	task_destroy(t2);
+	tasklet_free(t3);
+
+	TRACE_DEVEL("leaving on error", QUIC_EV_CONN_SET_AFFINITY, qc);
+	return 1;
+}
+
+/* Complete <qc> rebiding to an already selected new thread and associate it
+ * to <new_li> if necessary as required when migrating to a new thread group.
+ *
+ * After this function, <qc> instance must only be accessed via its newly
+ * associated thread. qc_finalize_affinity_rebind() must be called to
+ * reactivate quic_conn elements.
+ */
+void qc_set_tid_affinity2(struct quic_conn *qc, struct listener *new_li)
+{
+	const uint new_tid = qc->wait_event.tasklet->tid;
+	struct quic_connection_id *conn_id;
+	struct eb64_node *node;
+
+	TRACE_ENTER(QUIC_EV_CONN_SET_AFFINITY, qc);
+
+	/* Must only be called after qc_set_tid_affinity1(). */
+	BUG_ON(!(qc->flags & QUIC_FL_CONN_AFFINITY_CHANGED));
+
+	/* At this point no connection was accounted for yet on this
+	 * listener so it's OK to just swap the pointer.
+	 */
+	if (new_li && new_li != qc->li)
+		qc->li = new_li;
+
 	/* Rebind the connection FD. */
 	if (qc_test_fd(qc)) {
 		/* Reading is reactivated by the new thread. */
 		fd_migrate_on(qc->fd, new_tid);
 	}
 
-	/* Remove conn from per-thread list instance. It will be hidden from
-	 * "show quic" until rebinding is completed.
-	 */
-	qc_detach_th_ctx_list(qc, 0);
-
 	node = eb64_first(qc->cids);
 	/* One and only one CID must be present before affinity rebind.
 	 *
@@ -1797,29 +1830,35 @@ int qc_set_tid_affinity1(struct quic_conn *qc, uint new_tid, struct listener *ne
 	BUG_ON(!node || eb64_next(node));
 	conn_id = eb64_entry(node, struct quic_connection_id, seq_num);
 
-	/* At this point no connection was accounted for yet on this
+	/* Rebinding is considered done when CID points to the new
-	 * listener so it's OK to just swap the pointer.
+	 * thread. quic-conn instance cannot be derefence after it.
 	 */
-	if (new_li && new_li != qc->li)
-		qc->li = new_li;
-
-	/* Rebinding is considered done when CID points to the new thread. No
-	 * access should be done to quic-conn instance after it.
-	 */
-	qc->flags |= QUIC_FL_CONN_AFFINITY_CHANGED;
 	HA_ATOMIC_STORE(&conn_id->tid, new_tid);
 	qc = NULL;
 
 	TRACE_LEAVE(QUIC_EV_CONN_SET_AFFINITY, NULL);
-	return 0;
+}
 
- err:
+/* Interrupt <qc> thread migration and stick to the current tid.
-	task_destroy(t1);
+ * qc_finalize_affinity_rebind() must be called to reactivate quic_conn
-	task_destroy(t2);
+ * elements.
-	tasklet_free(t3);
+ */
+void qc_reset_tid_affinity(struct quic_conn *qc)
+{
+	TRACE_ENTER(QUIC_EV_CONN_SET_AFFINITY, qc);
 
-	TRACE_DEVEL("leaving on error", QUIC_EV_CONN_SET_AFFINITY, qc);
+	/* Must only be called after qc_set_tid_affinity1(). */
-	return 1;
+	BUG_ON(!(qc->flags & QUIC_FL_CONN_AFFINITY_CHANGED));
+
+	/* Reset tasks affinity to the current thread. quic_conn will remain
+	 * inactive until qc_finalize_affinity_rebind().
+	 */
+	task_set_thread(qc->idle_timer_task, tid);
+	if (qc->timer_task)
+		task_set_thread(qc->timer_task, tid);
+	tasklet_set_tid(qc->wait_event.tasklet, tid);
+
+	TRACE_LEAVE(QUIC_EV_CONN_SET_AFFINITY, qc);
 }
 
 /* Must be called after qc_set_tid_affinity() on the new thread. */