Re: [PATCH net-next v13 07/15] quic: add connection id management

On Thu, Jun 11, 2026 at 4:19 AM Paolo Abeni [off-list ref] wrote:

From: AI Reviewer <redacted>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
Now I have enough information to produce the report.

commit e00266d0eea858a744fd827548eb8ebc6868f92c
Author: Xin Long [off-list ref]

quic: add connection id management

This commit introduces struct quic_conn_id_set for managing QUIC
Connection IDs along with helpers to add, remove, and look up source
and destination CIDs, including a global hash table for source CID
lookup on the RX path.

quoted

diff --git a/net/quic/connid.c b/net/quic/connid.c
new file mode 100644
index 000000000000..b1a39feb827e
--- /dev/null
+++ b/net/quic/connid.c

@@ -0,0 +1,256 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/* QUIC kernel implementation
+ * (C) Copyright Red Hat Corp. 2023
+ *
+ * This file is part of the QUIC kernel implementation
+ *
+ * Initialization/cleanup for QUIC protocol support.
+ *
+ * Written or modified by:
+ *    Xin Long <lucien.xin@gmail.com>
+ */
+
+#include <linux/quic.h>
+#include <net/sock.h>
+
+#include "common.h"
+#include "connid.h"
+
+/* Lookup a source connection ID (scid) in the global source connection ID hash
+ * table.
+ */
+struct quic_conn_id *quic_conn_id_lookup(struct net *net, u8 *scid, u32 len)
+{
+     struct quic_shash_head *head = quic_source_conn_id_head(net, scid, len);
+     struct quic_source_conn_id *s_conn_id;
+     struct quic_conn_id *conn_id = NULL;
+     struct hlist_nulls_node *node;
+
+     hlist_nulls_for_each_entry_rcu(s_conn_id, node, &head->head, node) {
+             if (net != sock_net(s_conn_id->sk))
+                     continue;
+             if (s_conn_id->common.id.len != len ||
+                 memcmp(scid, &s_conn_id->common.id.data, len))
+                     continue;
+             if (likely(refcount_inc_not_zero(&s_conn_id->sk->sk_refcnt)))
+                     conn_id = &s_conn_id->common.id;
+             break;
+     }

[Critical]
Can s_conn_id->sk be freed while this RCU walk is running?  The
s_conn_id itself is freed via call_rcu() in quic_source_conn_id_free(),
but the struct sock it points to does not appear to be RCU-protected.

A grep across this series for SOCK_RCU_FREE and SLAB_TYPESAFE_BY_RCU
under net/quic returns no matches, and quic_init_sock() does not call
sock_set_flag(sk, SOCK_RCU_FREE), so sk_destruct() will free the sock
synchronously rather than after a grace period.

Consider this sequence:

  CPU0 (RX path, rcu_read_lock held)        CPU1 (close path)
  hlist_nulls_for_each_entry_rcu(..)
    /* sees s_conn_id still linked */
                                            sk_destruct()
                                              quic_destroy_sock()
                                                quic_conn_id_set_free()
                                                  /* unhash + call_rcu
                                                     defers s_conn_id
                                                     free, but not sk */
                                              sk_prot_free()  /* sk gone */
    sock_net(s_conn_id->sk)         /* UAF read of freed sock */
    refcount_inc_not_zero(
        &s_conn_id->sk->sk_refcnt)  /* UAF refcount op */

Once quic_packet_get_sock() (in the later 'quic: add packet parser
base' patch) wires this lookup into every short- and long-header
packet, an incoming packet racing with close() on the matching socket
would dereference freed sock memory.

Would setting SOCK_RCU_FREE on the QUIC sockets, switching the proto
slabs to SLAB_TYPESAFE_BY_RCU, taking sock_hold() when the sk pointer
is stored in s_conn_id, or otherwise gating the sk free behind an RCU
grace period address this?

This is not an issue, as sock_set_flag(sk, SOCK_RCU_FREE) will be called
in quic_hash(), which will be added in the patchset-2.

quoted

+
+/* Check if a given stateless reset token exists in any connection ID in the
+ * connection ID set.
+ */
+bool quic_conn_id_token_exists(struct quic_conn_id_set *id_set, u8 *token)
+{
+     struct quic_common_conn_id *common;
+     struct quic_dest_conn_id *dcid;
+
+     dcid = (struct quic_dest_conn_id *)id_set->active;
+     if (memchr_inv(dcid->token, 0, QUIC_CONN_ID_TOKEN_LEN) &&
+         !memcmp(dcid->token, token, QUIC_CONN_ID_TOKEN_LEN))
+             return true; /* Fast path. */
+
+     list_for_each_entry(common, &id_set->head, list) {
+             dcid = (struct quic_dest_conn_id *)common;
+             if (common == id_set->active)
+                     continue;
+             if (memchr_inv(dcid->token, 0, QUIC_CONN_ID_TOKEN_LEN) &&
+                 !memcmp(dcid->token, token, QUIC_CONN_ID_TOKEN_LEN))
+                     return true;
+     }
+     return false;
+}

[Medium]
Is memcmp() the right comparison primitive for the stateless reset
token here?  RFC 9000 section 10.3 treats the token as a secret that
authorises tearing down a connection, and the commit message states
this helper is used during stateless reset processing on incoming
packets.

lib/string.c's memcmp() returns at the first differing byte (and
performs unaligned word-sized loads on architectures that allow it),
so the time taken to return false leaks how many leading bytes of the
supplied token match a stored token.  Would crypto_memneq() from
<crypto/algapi.h>, which is constant-time, be more appropriate for
comparing the token bytes?

OK, will change to crypto_memneq() and replace 4 similar places.

quoted

+

[ ... ]

quoted

+/* Add a connection ID with sequence number and associated private data to the
+ * connection ID set.
+ */
+int quic_conn_id_add(struct quic_conn_id_set *id_set,
+                  struct quic_conn_id *conn_id, u32 number, void *data)
+{

[ ... ]

quoted

+     /* Locate insertion point to keep list ordered by number. */
+     list = &id_set->head;
+     list_for_each_entry(common, list, list) {
+             if (number == common->number)
+                     return 0; /* Ignore if it already exists on the list. */
+             if (number < common->number) {
+                     list = &common->list;
+                     break;
+             }
+     }

[ ... ]

quoted

+     list_add_tail(&common->list, list);
+
+     if (number == quic_conn_id_last_number(id_set) + 1) {
+             if (!id_set->active)
+                     id_set->active = common;
+             id_set->count++;
+
+             /* Increment count for consecutive following IDs. */
+             list_for_each_entry_continue(common, &id_set->head, list) {
+                     if (common->number != ++number)
+                             break;
+                     id_set->count++;
+             }
+     }
+     return 0;
+}

[Medium]
Does this function correctly maintain id_set->count when a CID is
inserted before all existing entries?  The loop above explicitly
handles the "number < common->number" case, suggesting out-of-order
insertion is intended to be supported.

Consider starting from list=[5], count=1 (so first=5, last=5) and
calling quic_conn_id_add(..., number=3, ...):

  - The location loop sets list = &common(5)->list and breaks.
  - list_add_tail() inserts common(3) before common(5),
    yielding list=[3, 5].
  - The post-insert check evaluates:
        quic_conn_id_last_number(id_set)
            = quic_conn_id_first_number(id_set) + count - 1
            = 3 + 1 - 1
            = 3
    so the test "number == last + 1" becomes "3 == 4" and the count
    update branch is skipped.

The end state is list=[3, 5] with count still 1, so entry 5 is
silently uncounted.

This is expected, count only count contiguous connection IDs.

A subsequent quic_conn_id_remove(id_set, 3)
then trips the WARN_ON_ONCE(number >= last_number) because
last_number reads as 3.

This will never happen, the callers will prevent passing the last
seqno to quic_conn_id_remove().

Thanks.

Should the count adjustment also handle the case where the inserted
CID lowers first_number?

quoted

+
+/* Remove consecutive connection IDs from the set with sequence numbers less
+ * than or equal to a number.
+ */
+void quic_conn_id_remove(struct quic_conn_id_set *id_set, u32 number)

[ ... ]
--
This is an AI-generated review.