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

[Paolo Abeni <pabeni@redhat.com>]

From: AI Reviewer <ai@example.com>

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 = -EINVAL, found = 0, exts = 0;
> +	u64 length, type;
> +

[ ... ]

> +	while (len >= 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 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 callers initialise alpn as

	struct quic_data alpns = {};

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"?

> +		if (type == TLS_EXT_alpn) { /* Found ALPN extension. */
> +			if (length > QUIC_ALPN_MAX_LEN)
> +				return err;
> +			len = length;
> +			found = 1;
> +			break;
> +		}
> +		/* Skip non-ALPN extensions. */
> +		p += length;
> +		len -= length;
> +		if (exts++ >= TLS_MAX_EXTENSIONS)
> +			return err;

[Low]
Is the post-increment intended here? With TLS_MAX_EXTENSIONS == 128, the
check (exts++ >= 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?

> +	}

[ ... ]

> +static int quic_packet_parse_alpn(struct sk_buff *skb, struct quic_data *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 != sock_net(tmp))
> +			continue;
> +		paths = quic_paths(tmp);
> +		odcid = 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) == usk &&
> +		    (!dcid || !quic_conn_id_cmp(odcid, dcid))) {
> +			sk = 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().

> +	if (sk && unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
> +		sk = NULL;
> +	rcu_read_unlock();
> +	return sk;
> +}

[ ... ]

> +struct sock *quic_listen_sock_lookup(struct sk_buff *skb, union quic_addr *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 = quic_path_saddr(quic_paths(tmp), 0);
> +			if (net == sock_net(tmp) &&
> +			    quic_cmp_sk_addr(tmp, a, sa) &&
> +			    quic_path_usock(quic_paths(tmp), 0) == skb->sk &&
> +			    (!alpns->data || !quic_alpn(tmp)->len)) {

[ ... ]

> @@ -48,6 +196,9 @@ static void quic_sock_destruct(struct sock *sk)
>  	for (i = 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?

>  	quic_sk_destruct(sk);
>  }
>  

[ ... ]
-- 
This is an AI-generated review.