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

[Paolo Abeni <pabeni@redhat.com>]

On 8/18/25 4:04 PM, Xin Long wrote:
> This patch introduces 'struct quic_conn_id_set' for managing Connection
> IDs (CIDs), which are represented by 'struct quic_source_conn_id'
> and 'struct quic_dest_conn_id'.
> 
> It provides helpers to add and remove CIDs from the set, and handles
> insertion of source CIDs into the global connection ID hash table
> when necessary.
> 
> - quic_conn_id_add(): Add a new Connection ID to the set, and inserts
>   it to conn_id hash table if it is a source conn_id.
> 
> - quic_conn_id_remove(): Remove connection IDs the set with sequence
>   numbers less than or equal to a number.

It's unclear how many connections are expected to be contained in each
set. If more than an handful you should consider using RB-tree instead
of lists.

[...]
> +static void quic_source_conn_id_free(struct quic_source_conn_id *s_conn_id)
> +{
> +	u8 *data = s_conn_id->common.id.data;
> +	struct quic_hash_head *head;
> +
> +	if (!hlist_unhashed(&s_conn_id->node)) {
> +		head = quic_source_conn_id_head(sock_net(s_conn_id->sk), data);
> +		spin_lock_bh(&head->s_lock);
> +		hlist_del_init(&s_conn_id->node);
> +		spin_unlock_bh(&head->s_lock);
> +	}
> +
> +	/* Freeing is deferred via RCU to avoid use-after-free during concurrent lookups. */
> +	call_rcu(&s_conn_id->rcu, quic_source_conn_id_free_rcu);
> +}
> +
> +static void quic_conn_id_del(struct quic_common_conn_id *common)
> +{
> +	list_del(&common->list);
> +	if (!common->hashed) {
> +		kfree(common);
> +		return;
> +	}
> +	quic_source_conn_id_free((struct quic_source_conn_id *)common);

It looks like the above cast is not needed.

> +}
> +
> +/* 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)
> +{
> +	struct quic_source_conn_id *s_conn_id;
> +	struct quic_dest_conn_id *d_conn_id;
> +	struct quic_common_conn_id *common;
> +	struct quic_hash_head *head;
> +	struct list_head *list;
> +
> +	/* 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 is already exists on the list. */
> +		if (number < common->number) {
> +			list = &common->list;
> +			break;
> +		}
> +	}
> +
> +	if (conn_id->len > QUIC_CONN_ID_MAX_LEN)
> +		return -EINVAL;
> +	common = kzalloc(id_set->entry_size, GFP_ATOMIC);
> +	if (!common)
> +		return -ENOMEM;
> +	common->id = *conn_id;
> +	common->number = number;
> +	if (id_set->entry_size == sizeof(struct quic_dest_conn_id)) {
> +		/* For destination connection IDs, copy the stateless reset token if available. */
> +		if (data) {
> +			d_conn_id = (struct quic_dest_conn_id *)common;
> +			memcpy(d_conn_id->token, data, QUIC_CONN_ID_TOKEN_LEN);
> +		}
> +	} else {
> +		/* For source connection IDs, mark as hashed and insert into the global source
> +		 * connection ID hashtable.
> +		 */
> +		common->hashed = 1;
> +		s_conn_id = (struct quic_source_conn_id *)common;
> +		s_conn_id->sk = data;
> +
> +		head = quic_source_conn_id_head(sock_net(s_conn_id->sk), common->id.data);
> +		spin_lock_bh(&head->s_lock);
> +		hlist_add_head(&s_conn_id->node, &head->head);
> +		spin_unlock_bh(&head->s_lock);
> +	}
> +	list_add_tail(&common->list, list);

It's unclear if/how id_set->list is protected vs concurrent accesses.

/P