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

[netdev-bot+sashiko@kernel.org]

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.

> 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

[ ... ]

> +/* 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.

> 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

[ ... ]

> +/* 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 */
> +};

[ ... ]

> +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