Message-ID: <CADvbK_fLKuUaB1_M4DyLC6V==7ThXt+4heyZykBrLM5nL28DYw@mail.gmail.com>
Date: Sat, 23 Aug 2025 11:57:56 -0400
From: Xin Long <lucien.xin@...il.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: network dev <netdev@...r.kernel.org>, davem@...emloft.net, kuba@...nel.org, 
	Eric Dumazet <edumazet@...gle.com>, Simon Horman <horms@...nel.org>, 
	Stefan Metzmacher <metze@...ba.org>, Moritz Buhl <mbuhl@...nbsd.org>, Tyler Fanelli <tfanelli@...hat.com>, 
	Pengtao He <hepengtao@...omi.com>, linux-cifs@...r.kernel.org, 
	Steve French <smfrench@...il.com>, Namjae Jeon <linkinjeon@...nel.org>, 
	Paulo Alcantara <pc@...guebit.com>, Tom Talpey <tom@...pey.com>, kernel-tls-handshake@...ts.linux.dev, 
	Chuck Lever <chuck.lever@...cle.com>, Jeff Layton <jlayton@...nel.org>, 
	Benjamin Coddington <bcodding@...hat.com>, Steve Dickson <steved@...hat.com>, Hannes Reinecke <hare@...e.de>, 
	Alexander Aring <aahringo@...hat.com>, David Howells <dhowells@...hat.com>, 
	Cong Wang <xiyou.wangcong@...il.com>, "D . Wythe" <alibuda@...ux.alibaba.com>, 
	Jason Baron <jbaron@...mai.com>, illiliti <illiliti@...tonmail.com>, 
	Sabrina Dubroca <sd@...asysnail.net>, Marcelo Ricardo Leitner <marcelo.leitner@...il.com>, 
	Daniel Stenberg <daniel@...x.se>, Andy Gospodarek <andrew.gospodarek@...adcom.com>
Subject: Re: [PATCH net-next v2 07/15] quic: add connection id management

On Thu, Aug 21, 2025 at 9:55 AM Paolo Abeni <pabeni@...hat.com> wrote:
>
> 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.
>
We limit the max number of issued CIDs to 8 per connection, and the CID
per connection traversal is not on the data path, so it's fine to use
lists here.

Note that one connection/sk has one source CID set which contains a
couple of CIDs used for connection migration, and one dest CID set
to saving peer's CIDs.

> [...]
> > +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.
there will be a compiling error:

/root/quic/modules/net/quic/connid.c:68:66: note: expected ‘struct
quic_source_conn_id *’ but argument is of type ‘struct
quic_common_conn_id *’
   68 | static void quic_source_conn_id_free(struct
quic_source_conn_id *s_conn_id)

Or you mean change the parameter type of quic_source_conn_id_free() to:

static void quic_source_conn_id_free(struct quic_common_conn_id *common)

>
> > +}
> > +
> > +/* 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.
>
id_set is per connection/socket, it's always protected by sock lock.
I will leave an annotation in the description of the function for that.

Thanks.

Powered by blists - more mailing lists