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

On Thu, Jun 11, 2026 at 4:19=E2=80=AFAM Paolo Abeni <[email protected]> wro=
te:
>
> From: AI Reviewer <[email protected]>
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://netdev-ai.bots.linux.dev/sashiko/
> ---
> Now I have enough information to produce the report.
>
> commit e00266d0eea858a744fd827548eb8ebc6868f92c
> Author: Xin Long <[email protected]>
>
> quic: add connection id management
>
> This commit introduces struct quic_conn_id_set for managing QUIC
> Connection IDs along with helpers to add, remove, and look up source
> and destination CIDs, including a global hash table for source CID
> lookup on the RX path.
>
> > diff --git a/net/quic/connid.c b/net/quic/connid.c
> > new file mode 100644
> > index 000000000000..b1a39feb827e
> > --- /dev/null
> > +++ b/net/quic/connid.c
> > @@ -0,0 +1,256 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/* QUIC kernel implementation
> > + * (C) Copyright Red Hat Corp. 2023
> > + *
> > + * This file is part of the QUIC kernel implementation
> > + *
> > + * Initialization/cleanup for QUIC protocol support.
> > + *
> > + * Written or modified by:
> > + *    Xin Long <[email protected]>
> > + */
> > +
> > +#include <linux/quic.h>
> > +#include <net/sock.h>
> > +
> > +#include "common.h"
> > +#include "connid.h"
> > +
> > +/* Lookup a source connection ID (scid) in the global source connectio=
n ID hash
> > + * table.
> > + */
> > +struct quic_conn_id *quic_conn_id_lookup(struct net *net, u8 *scid, u3=
2 len)
> > +{
> > +     struct quic_shash_head *head =3D quic_source_conn_id_head(net, sc=
id, len);
> > +     struct quic_source_conn_id *s_conn_id;
> > +     struct quic_conn_id *conn_id =3D NULL;
> > +     struct hlist_nulls_node *node;
> > +
> > +     hlist_nulls_for_each_entry_rcu(s_conn_id, node, &head->head, node=
) {
> > +             if (net !=3D sock_net(s_conn_id->sk))
> > +                     continue;
> > +             if (s_conn_id->common.id.len !=3D len ||
> > +                 memcmp(scid, &s_conn_id->common.id.data, len))
> > +                     continue;
> > +             if (likely(refcount_inc_not_zero(&s_conn_id->sk->sk_refcn=
t)))
> > +                     conn_id =3D &s_conn_id->common.id;
> > +             break;
> > +     }
>
> [Critical]
> Can s_conn_id->sk be freed while this RCU walk is running?  The
> s_conn_id itself is freed via call_rcu() in quic_source_conn_id_free(),
> but the struct sock it points to does not appear to be RCU-protected.
>
> A grep across this series for SOCK_RCU_FREE and SLAB_TYPESAFE_BY_RCU
> under net/quic returns no matches, and quic_init_sock() does not call
> sock_set_flag(sk, SOCK_RCU_FREE), so sk_destruct() will free the sock
> synchronously rather than after a grace period.
>
> Consider this sequence:
>
>   CPU0 (RX path, rcu_read_lock held)        CPU1 (close path)
>   hlist_nulls_for_each_entry_rcu(..)
>     /* sees s_conn_id still linked */
>                                             sk_destruct()
>                                               quic_destroy_sock()
>                                                 quic_conn_id_set_free()
>                                                   /* unhash + call_rcu
>                                                      defers s_conn_id
>                                                      free, but not sk */
>                                               sk_prot_free()  /* sk gone =
*/
>     sock_net(s_conn_id->sk)         /* UAF read of freed sock */
>     refcount_inc_not_zero(
>         &s_conn_id->sk->sk_refcnt)  /* UAF refcount op */
>
> Once quic_packet_get_sock() (in the later 'quic: add packet parser
> base' patch) wires this lookup into every short- and long-header
> packet, an incoming packet racing with close() on the matching socket
> would dereference freed sock memory.
>
> Would setting SOCK_RCU_FREE on the QUIC sockets, switching the proto
> slabs to SLAB_TYPESAFE_BY_RCU, taking sock_hold() when the sk pointer
> is stored in s_conn_id, or otherwise gating the sk free behind an RCU
> grace period address this?
>

This is not an issue, as sock_set_flag(sk, SOCK_RCU_FREE) will be called
in quic_hash(), which will be added in the patchset-2.


> > +
> > +/* 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 *to=
ken)
> > +{
> > +     struct quic_common_conn_id *common;
> > +     struct quic_dest_conn_id *dcid;
> > +
> > +     dcid =3D (struct quic_dest_conn_id *)id_set->active;
> > +     if (memchr_inv(dcid->token, 0, QUIC_CONN_ID_TOKEN_LEN) &&
> > +         !memcmp(dcid->token, token, QUIC_CONN_ID_TOKEN_LEN))
> > +             return true; /* Fast path. */
> > +
> > +     list_for_each_entry(common, &id_set->head, list) {
> > +             dcid =3D (struct quic_dest_conn_id *)common;
> > +             if (common =3D=3D id_set->active)
> > +                     continue;
> > +             if (memchr_inv(dcid->token, 0, QUIC_CONN_ID_TOKEN_LEN) &&
> > +                 !memcmp(dcid->token, token, QUIC_CONN_ID_TOKEN_LEN))
> > +                     return true;
> > +     }
> > +     return false;
> > +}
>
> [Medium]
> Is memcmp() the right comparison primitive for the stateless reset
> token here?  RFC 9000 section 10.3 treats the token as a secret that
> authorises tearing down a connection, and the commit message states
> this helper is used during stateless reset processing on incoming
> packets.
>
> lib/string.c's memcmp() returns at the first differing byte (and
> performs unaligned word-sized loads on architectures that allow it),
> so the time taken to return false leaks how many leading bytes of the
> supplied token match a stored token.  Would crypto_memneq() from
> <crypto/algapi.h>, which is constant-time, be more appropriate for
> comparing the token bytes?
>

OK, will change to crypto_memneq() and replace 4 similar places.

> > +
> [ ... ]
> > +/* Add a connection ID with sequence number and associated private dat=
a 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=
)
> > +{
> [ ... ]
> > +     /* Locate insertion point to keep list ordered by number. */
> > +     list =3D &id_set->head;
> > +     list_for_each_entry(common, list, list) {
> > +             if (number =3D=3D common->number)
> > +                     return 0; /* Ignore if it already exists on the l=
ist. */
> > +             if (number < common->number) {
> > +                     list =3D &common->list;
> > +                     break;
> > +             }
> > +     }
> [ ... ]
> > +     list_add_tail(&common->list, list);
> > +
> > +     if (number =3D=3D quic_conn_id_last_number(id_set) + 1) {
> > +             if (!id_set->active)
> > +                     id_set->active =3D common;
> > +             id_set->count++;
> > +
> > +             /* Increment count for consecutive following IDs. */
> > +             list_for_each_entry_continue(common, &id_set->head, list)=
 {
> > +                     if (common->number !=3D ++number)
> > +                             break;
> > +                     id_set->count++;
> > +             }
> > +     }
> > +     return 0;
> > +}
>
> [Medium]
> Does this function correctly maintain id_set->count when a CID is
> inserted before all existing entries?  The loop above explicitly
> handles the "number < common->number" case, suggesting out-of-order
> insertion is intended to be supported.
>
> Consider starting from list=3D[5], count=3D1 (so first=3D5, last=3D5) and
> calling quic_conn_id_add(..., number=3D3, ...):
>
>   - The location loop sets list =3D &common(5)->list and breaks.
>   - list_add_tail() inserts common(3) before common(5),
>     yielding list=3D[3, 5].
>   - The post-insert check evaluates:
>         quic_conn_id_last_number(id_set)
>             =3D quic_conn_id_first_number(id_set) + count - 1
>             =3D 3 + 1 - 1
>             =3D 3
>     so the test "number =3D=3D last + 1" becomes "3 =3D=3D 4" and the cou=
nt
>     update branch is skipped.
>
> The end state is list=3D[3, 5] with count still 1, so entry 5 is
> silently uncounted.
This is expected, count only count contiguous connection IDs.

> A subsequent quic_conn_id_remove(id_set, 3)
> then trips the WARN_ON_ONCE(number >=3D last_number) because
> last_number reads as 3.
>

This will never happen, the callers will prevent passing the last
seqno to quic_conn_id_remove().

Thanks.

> Should the count adjustment also handle the case where the inserted
> CID lowers first_number?
>
> > +
> > +/* Remove consecutive connection IDs from the set with sequence number=
s less
> > + * than or equal to a number.
> > + */
> > +void quic_conn_id_remove(struct quic_conn_id_set *id_set, u32 number)
> [ ... ]
> --
> This is an AI-generated review.
>