Re: [PATCH net-next v17 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] The comment on the `count` field of `struct quic_conn_id_set`
  (net/quic/connid.h:46) does not match the code.

--

quic: add connection id management

This patch adds struct quic_conn_id_set to manage QUIC source and
destination connection IDs. It provides helpers to add, remove and find
connection IDs in a set, and inserts source connection IDs into a global
RCU-protected hash table. It also adds helpers for RX path lookup and for
checking stateless reset tokens.

> 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
> @@ -0,0 +1,184 @@

[ ... ]

> +struct quic_conn_id_set {
> +	/* Connection ID in use on the current path */
> +	struct quic_common_conn_id *active;
> +	/* Connection ID to use for a new path (e.g., after migration) */
> +	struct quic_common_conn_id *alt;
> +	struct list_head head; /* List head of available connection IDs */
> +	u8 entry_size; /* Size of each connection ID entry in the list */
> +	u8 max_count;  /* active_connection_id_limit in rfc9000#section-18.2 */
> +	u8 count;      /* Current number of connection IDs in the list */

[Severity: Low]

This isn't a bug, but is this comment accurate?

quic_conn_id_add() links every new entry into the list. It only increments
count when the new number extends the consecutive run:

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

For example, after adding numbers 0 and 5 to an empty set, the list holds
two entries but count is 1.

The consumers rely on count being the length of the consecutive run that
starts at the first entry:

	return quic_conn_id_first_number(id_set) + id_set->count - 1;

The WARN_ON_ONCE() guard in quic_conn_id_remove() and the
active->number != last_number check in quic_conn_id_select_alt() both
depend on that meaning.

Could the comment describe count as the number of consecutive connection
IDs starting from the first entry in the list? Otherwise a later caller
that compares count against max_count (active_connection_id_limit) would
be comparing a run length against a list-size limit.

> +	bool source;   /* true if this is a source conn_id set, false if dest */
> +};
> +
> +static inline u32 quic_conn_id_first_number(struct quic_conn_id_set *id_set)
> +{

[ ... ]

> +static inline u32 quic_conn_id_last_number(struct quic_conn_id_set *id_set)
> +{
> +	return quic_conn_id_first_number(id_set) + id_set->count - 1;
> +}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1792164438.git.lucien.xin%40gmail.com