Re: [PATCH net-next v13 15/15] quic: add packet parser base

On Thu, Jun 11, 2026 at 4:20=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/
> ---
> quic: add packet parser base
>
> This patch adds the QUIC packet RX parser, including ALPN extraction
> from the Client Initial to demux to the right listener, an ICMP-driven
> PMTU update path, and a per-net backlog/work for deferred parsing.
>
> > diff --git a/net/quic/packet.c b/net/quic/packet.c
> > index ba5204aceda6..430fc06d0012 100644
> > --- a/net/quic/packet.c
> > +++ b/net/quic/packet.c
>
> [ ... ]
>
> > +static int quic_packet_get_alpn(struct quic_data *alpn, u8 *p, u32 len=
)
> > +{
> > +     int err =3D -EINVAL, found =3D 0, exts =3D 0;
> > +     u64 length, type;
> > +
>
> [ ... ]
>
> > +     while (len >=3D 4) { /* Scan extensions for ALPN (TLS_EXT_alpn). =
*/
> > +             if (!quic_get_int(&p, &len, &type, 2))
> > +                     break;
> > +             if (!quic_get_int(&p, &len, &length, 2))
> > +                     break;
> > +             if (len < (u32)length) /* Incomplete TLS extensions. */
> > +                     return 0;
>
> [Medium]
> Should this branch be treated as a parse failure rather than a success?
>

The rest of the TLS extensions could be in the CRYPTO frame of
the next initial packet.

Returning error will cause packets to be dropped in the callers.

> The other length-bound checks in this same function return err (-EINVAL)
> when a declared length exceeds the remaining buffer, but this one returns
> 0 without populating alpn.

The splitted TLS CLIENT_HELLO messages are very rare.
The ones I saw from some QUIC implementations are splitted from these
TLS EXT points. I will improve it when seeing more different split points.

> The callers initialise alpn as
>
>         struct quic_data alpns =3D {};
>
> and quic_listen_sock_lookup() then takes the no-ALPN branch:
>
>     if (!alpns->len) { /* No ALPNs or parse failed */
>         sk_nulls_for_each_rcu(tmp, node, &head->head) {
>             ...
>             if (... && (!alpns->data || !quic_alpn(tmp)->len)) {
>
> Because alpns->data is still NULL, the match collapses to true and any
> listener bound to the address/port will match, including ones that
> registered a specific ALPN list. Can a peer reach an ALPN-restricted
> listener by sending a Client Initial with one extension whose declared
> length is larger than what is left in the ClientHello?
>
> Would returning err here (or always assigning alpn->data before
> returning) match the comment in quic_listen_sock_lookup() that
> distinguishes "TLS parsing succeeded but no ALPN was found" from "parse
> failed"?
>
ALPN demux is a feature trying its best to dispatch the first initial
to the right process. If it goes to the "incorrect" because the peer
doesn't provide proper ALPNs, the userspace handshake will reject it.

It's kind of the peer responsibility if it wants to get processed in the
right process, send with the right ALPNs then.

> > +             if (type =3D=3D TLS_EXT_alpn) { /* Found ALPN extension. =
*/
> > +                     if (length > QUIC_ALPN_MAX_LEN)
> > +                             return err;
> > +                     len =3D length;
> > +                     found =3D 1;
> > +                     break;
> > +             }
> > +             /* Skip non-ALPN extensions. */
> > +             p +=3D length;
> > +             len -=3D length;
> > +             if (exts++ >=3D TLS_MAX_EXTENSIONS)
> > +                     return err;
>
> [Low]
> Is the post-increment intended here? With TLS_MAX_EXTENSIONS =3D=3D 128, =
the
> check (exts++ >=3D 128) first becomes true when exts has already reached
> 129, so 129 extensions are consumed before bail-out even though the
> constant name suggests a cap of 128. Would
>
>         if (++exts > TLS_MAX_EXTENSIONS)
>                 return err;
>
> (or moving the check to the loop top) better reflect the intended limit?
>

will change to ++exts.

> > +     }
>
> [ ... ]
>
> > +static int quic_packet_parse_alpn(struct sk_buff *skb, struct quic_dat=
a *alpn)
>
> [ ... ]
>
> > diff --git a/net/quic/socket.c b/net/quic/socket.c
> > index 8f348d7c678b..19b164baf55d 100644
> > --- a/net/quic/socket.c
> > +++ b/net/quic/socket.c
>
> [ ... ]
>
> > +struct sock *quic_sock_lookup(struct sk_buff *skb, union quic_addr *sa=
,
> > +                           union quic_addr *da, struct sock *usk,
> > +                           struct quic_conn_id *dcid)
> > +{
>
> [ ... ]
>
> > +     rcu_read_lock();
> > +begin:
> > +     sk_nulls_for_each_rcu(tmp, node, &head->head) {
> > +             if (net !=3D sock_net(tmp))
> > +                     continue;
> > +             paths =3D quic_paths(tmp);
> > +             odcid =3D quic_path_orig_dcid(paths);
> > +             if (quic_cmp_sk_addr(tmp, quic_path_saddr(paths, 0), sa) =
&&
> > +                 quic_cmp_sk_addr(tmp, quic_path_daddr(paths, 0), da) =
&&
> > +                 quic_path_usock(paths, 0) =3D=3D usk &&
> > +                 (!dcid || !quic_conn_id_cmp(odcid, dcid))) {
> > +                     sk =3D tmp;
> > +                     break;
> > +             }
> > +     }
>
> [High]
> Is the sk memory itself RCU-protected here? sk_nulls_for_each_rcu()
> dereferences fields of tmp (sock_net(tmp), quic_paths(tmp),
> quic_path_saddr(...), etc.) before any reference is taken. That is only
> safe when the sk allocation is freed via RCU.
>
> quic_init_sock() does not call sock_set_flag(sk, SOCK_RCU_FREE), and
> neither quic_prot nor quicv6_prot sets slab_flags to SLAB_TYPESAFE_BY_RCU=
,
> so sk_destruct() (and quic_sock_destruct()) runs synchronously on the
> last sock_put(). Can a parallel close on tmp free the sock between
> sk_nulls_for_each_rcu() exposing it and the field dereferences above?
>
> The same shape appears in quic_listen_sock_lookup() further down, where
> quic_alpn(tmp)->len is read inside the RCU walk before
> refcount_inc_not_zero().
>

SOCK_RCU_FREE will be set in the patchset-2 via quic_hash().

> > +     if (sk && unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
> > +             sk =3D NULL;
> > +     rcu_read_unlock();
> > +     return sk;
> > +}
>
> [ ... ]
>
> > +struct sock *quic_listen_sock_lookup(struct sk_buff *skb, union quic_a=
ddr *sa,
> > +                                  union quic_addr *da,
> > +                                  struct quic_data *alpns)
> > +{
>
> [ ... ]
>
> > +     rcu_read_lock();
> > +     if (!alpns->len) { /* No ALPNs or parse failed */
> > +             sk_nulls_for_each_rcu(tmp, node, &head->head) {
>
> [ ... ]
>
> > +                     a =3D quic_path_saddr(quic_paths(tmp), 0);
> > +                     if (net =3D=3D sock_net(tmp) &&
> > +                         quic_cmp_sk_addr(tmp, a, sa) &&
> > +                         quic_path_usock(quic_paths(tmp), 0) =3D=3D sk=
b->sk &&
> > +                         (!alpns->data || !quic_alpn(tmp)->len)) {
>
> [ ... ]
>
> > @@ -48,6 +196,9 @@ static void quic_sock_destruct(struct sock *sk)
> >       for (i =3D 0; i < QUIC_CRYPTO_MAX; i++)
> >               quic_crypto_free(quic_crypto(sk, i));
> >
> > +     /* Deferred ALPN free for RCU readers in quic_listen_sock_lookup(=
). */
> > +     quic_data_free(quic_alpn(sk));
> > +
>
> Without SOCK_RCU_FREE or SLAB_TYPESAFE_BY_RCU on quic_prot/quicv6_prot,
> does moving the alpn free into the destruct callback actually defer it
> past an RCU grace period? quic_sock_destruct() is invoked from
> sk_destruct() on the final sock_put() and is not itself an RCU callback,
> so the comment's "Deferred ALPN free for RCU readers" assumption seems to
> rely on a guarantee that is not configured on this protocol. Should this
> patch also set sock_set_flag(sk, SOCK_RCU_FREE) in quic_init_sock(), or
> alternatively take refcount_inc_not_zero() in the lookup helpers before
> touching any fields of tmp?
>

SOCK_RCU_FREE will be set in the patchset-2 via quic_hash().

> >       quic_sk_destruct(sk);
> >  }
> >
>
> [ ... ]
> --
> This is an AI-generated review.
>