Re: [PATCH net-next v16 07/15] quic: add connection id management
From: netdev-bot+sashiko@kernel.org
Date: 2026-10-07 01:05:13
Also in:
linux-cifs
Thank you for your contribution! Sashiko AI review found more severe issue(s) in other patches of this series. This mail is only an FYI, in case you want to address the nit picks below when you respin. Low priority / nit picks for your consideration: - [Low] quic_conn_id_token_exists() (net/quic/connid.c:52-71) checks the active destination CID's token first. -- quic: add connection id management This patch adds struct quic_conn_id_set for managing source and destination connection IDs. It adds helpers to add, remove and find CIDs, and inserts source CIDs into the global hash table. It also adds lookup helpers for the RX path and for stateless reset token checks.
quoted hunk
diff --git a/net/quic/connid.c b/net/quic/connid.c new file mode 100644 index 0000000000000..85b8fc481e608 --- /dev/null +++ b/net/quic/connid.c
[ ... ]
quoted hunk
+/* 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) && + !crypto_memneq(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) && + !crypto_memneq(dcid->token, token, QUIC_CONN_ID_TOKEN_LEN)) + return true; + } + return false; +}
[Severity: Low]
Should quic_conn_id_token_exists() match against every destination CID in
id_set->head?
The loop compares the token with all entries in the set. That includes
destination CIDs that the peer supplied with NEW_CONNECTION_ID frames
through quic_conn_id_add() but that were never used to send packets.
RFC 9000 section 10.3.1 says:
An endpoint MUST NOT check for any stateless reset tokens associated
with connection IDs it has not used or for connection IDs that have
been retired.
It also limits the comparison to tokens associated with the remote
address on which the datagram was received.
Retired CIDs are covered, because quic_conn_id_remove() unlinks them. But
nothing filters out unused CIDs. As a result, up to QUIC_CONN_ID_LIMIT
tokens could accept a stateless reset and tear down the connection,
instead of only the tokens for CIDs that have been used.
No caller of this helper exists yet in the series. The commit message
says it is meant for stateless reset processing, though, so the
semantics are being set here.
quoted hunk
diff --git a/net/quic/connid.h b/net/quic/connid.h new file mode 100644 index 0000000000000..105c2b4b0f2ec --- /dev/null +++ b/net/quic/connid.h
[ ... ]
quoted hunk
+/* Common fields shared by both source and destination Connection IDs */ +struct quic_common_conn_id { + struct quic_conn_id id; /* Connection ID value and its length */ + struct list_head list; /* List node for connection ID management */ + u32 number; /* Sequence number assigned to this Connection ID */ + bool hashed; /* true if stored in source_conn_id hash table */ +};
[ ... ]
quoted hunk
+struct quic_dest_conn_id { + struct quic_common_conn_id common; + /* Stateless reset token in rfc9000#section-10.3 */ + u8 token[QUIC_CONN_ID_TOKEN_LEN]; +};
Neither struct quic_common_conn_id nor struct quic_dest_conn_id records whether a CID has been used or which remote address it is tied to. How would a later caller limit the check to used CIDs without that state? Would it make sense to track a used flag here, or to limit quic_conn_id_token_exists() to id_set->active and id_set->alt? -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1791227050.git.lucien.xin%40gmail.com