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

diff --git a/include/haproxy/quic_conn.h b/include/haproxy/quic_conn.h
index c7005c06cd0e5ed2286e3f1fc978f165bf0748d3..7082fa07390df0a92ec746f5817078877ae9f084 100644 (file)
--- a/include/haproxy/quic_conn.h
+++ b/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);

diff --git a/src/proto_quic.c b/src/proto_quic.c
index 4b8f0c20f00d4bc884ed40a8f049e47ddcf354d2..0899cd9709d87c16f503204ed256984e14f1fcf8 100644 (file)
--- a/src/proto_quic.c
+++ b/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)

diff --git a/src/quic_conn.c b/src/quic_conn.c
index 24e52d0363212d8f522f2ecc3b5fc6dfbcccdbc6..b3edc99fedb6b008ac48a4a265e52f1e4b87f001 100644 (file)
--- a/src/quic_conn.c
+++ b/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
- * new one <new_tid> optionally in association with <new_li> (since it may need
- * to change when migrating to a thread from a different group, otherwise leave
- * it NULL). After this call, the connection cannot be dereferenced anymore on
- * the current thread.
+/* Prepare <qc> QUIC connection rebinding to a new thread <new_tid>. Stop and
+ * release associated tasks and tasklet and allocate new ones binded to the new
+ * 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
-        * listener so it's OK to just swap the pointer.
-        */
-       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.
+       /* Rebinding is considered done when CID points to the new
+        * thread. quic-conn instance cannot be derefence 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:
-       task_destroy(t1);
-       task_destroy(t2);
-       tasklet_free(t3);
+/* Interrupt <qc> thread migration and stick to the current tid.
+ * qc_finalize_affinity_rebind() must be called to reactivate quic_conn
+ * elements.
+ */
+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);
-       return 1;
+       /* Must only be called after qc_set_tid_affinity1(). */
+       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. */