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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 3 · Low: 4

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The permanent commit message (the part above '---') describes
  much less than the patch does, and some of it is wrong.
- [Medium] When PLPMTUD is disabled (paths->plpmtud_interval == 0, the
  default in this tree because nothing sets it),…
- [Medium] quic_packet_listen_process() replies to, and creates state
  for, long-header packets of any size.
- [Low] The kernel-doc for quic_listen_sock_lookup() in net/quic/socket.c
  says that when several listening sockets share the same address, port…
- [Low] The patch adds definitions that nothing in this commit uses.
- [Low] struct quic_packet in net/quic/packet.h documents `daddr` as
  'Dest address from received packet' and `saddr` as 'Source address…
- [Low] The header comment of quic_packet_deferred_work() (packet.c:776)
  reads 'Work function to process packets in the backlog queue', but the…

--

quic: add packet parser base

This commit adds the QUIC receive path. quic_packet_rcv() finds the
owning socket by connection ID, by address, or through the listener, then
passes the packet to quic_packet_process(). The patch also adds
listen-side Initial handling, request sockets, ALPN-based listener
switching through a per-socket work item, and ICMP Packet Too Big
handling through quic_packet_rcv_err() and quic_packet_rcv_err_pmtu().

> This patch uses 'quic_packet' to handle packing of QUIC packets on the
> receive (RX) path.

[ ... ]

> - quic_packet_get_alpn(): Parse the ALPN from a client Initial packet,
>   then locate the appropriate listener using the ALPN.

[ ... ]

> - quic_packet_rcv_err_pmtu(): Find the socket and update the PMTU via
>   quic_packet_mss_update().

[Severity: Medium]
Some parts of the commit message don't match the code.

Should "packing" in the first sentence be "parsing"?

quic_packet_get_alpn() installs the Initial keys, decrypts the packet and
parses the ALPN list. It doesn't look up a listener. The lookup and the
switch happen in quic_listen_sock_switch() in net/quic/socket.c.

For ICMP errors, the socket lookup is done in quic_packet_rcv_err(), not
in quic_packet_rcv_err_pmtu().

The message also leaves out much of what the patch adds:

  - quic_packet_listen_process(), with token verification and the
    Stateless Reset, Version Negotiation, Retry and CONNECTION_REFUSED
    triggers
  - struct quic_request_sock and quic_request_sock_lookup(),
    quic_request_sock_create() and quic_request_sock_backlog_tail(),
    which charge sk_ack_backlog and receive memory
  - the per-socket work_struct with deferred_list/backlog_list on
    quic_wq, and flush_work() in quic_packet_free()
  - quic_accept_sock_exists() and quic_listen_sock_switch()
  - quic_backlog_rcv() now processing packets instead of dropping them
  - the ALPN free moving from quic_destroy_sock() to quic_sock_destruct()

Most of this is described only in the changelog below the --- line, and
git am drops that part. Could the permanent commit message cover these
pieces?

> diff --git a/net/quic/packet.c b/net/quic/packet.c
> index 56d2b8df292a0..673f1cd491438 100644
> --- a/net/quic/packet.c
> +++ b/net/quic/packet.c
> @@ -14,6 +14,781 @@
>  
>  #define QUIC_HLEN		1
>  
> +#define QUIC_LONG_HLEN(dcid, scid) \
> +	(QUIC_HLEN + QUIC_VERSION_LEN + 1 + (dcid)->len + 1 + (scid)->len)

[Severity: Low]
This isn't a bug, but nothing in this patch uses QUIC_LONG_HLEN().

Two of the new struct quic_packet members are in a similar state.

backlog_list is described as "Packets waiting for crypto keys". It is
only initialized in quic_packet_init() and purged in quic_packet_free(),
and nothing enqueues to it yet.

quic_packet_listen_process() reads validate_peer_address, but nothing
ever sets it, so the token validation and Retry branch can't be reached:

	if (packet->validate_peer_address) {

The changelog says these are staged for the next patchset. Could they be
added together with the code that uses them? Failing that, could the
backlog_list comment describe what the field does today?

[ ... ]

> +/* Process PMTU reduction event on a QUIC socket. */
> +void quic_packet_rcv_err_pmtu(struct sock *sk)
> +{

[ ... ]

> +	info = clamp(paths->mtu_info, QUIC_PATH_MIN_PMTU, QUIC_PATH_MAX_PMTU);
> +	/* If PLPMTUD is not enabled, update MSS using route and ICMP info. */
> +	if (!paths->plpmtud_interval) {
> +		if (quic_packet_route(sk))
> +			return;
> +
> +		dst = __sk_dst_get(sk);
> +		if (dst)
> +			dst->ops->update_pmtu(dst, sk, NULL, info, true);
> +		quic_packet_mss_update(sk, info - packet->hlen);

[Severity: Medium]
Can a Packet Too Big that reports a larger MTU than the current path MTU
raise the MSS here?

info is only clamped to [QUIC_PATH_MIN_PMTU, QUIC_PATH_MAX_PMTU], and
quic_packet_mss_update() stores whatever it is given. The route layer
ignores increases:

net/ipv4/route.c:__ip_rt_update_pmtu() {
    ...
	old_mtu = ipv4_mtu(dst);
	if (old_mtu < mtu)
		return;
    ...
}

__ip6_rt_update_pmtu() also returns early when mtu >= dst6_mtu(dst).

Nothing in this tree sets plpmtud_interval, so this branch is the
default. A delayed or spoofed ICMP that arrives through quic_udp_err()
-> quic_packet_rcv() -> quic_packet_rcv_err() -> quic_packet_rcv_err_pmtu()
and claims 9000 on a 1500-byte path would set the MSS to 9000 - hlen.

The route is left unchanged, so the cached dst stays valid. Later
quic_packet_route() calls then get 1 back from quic_flow_route() and
never recompute the MSS from dst_mtu().

Would it be safer to re-read dst_mtu(dst) after update_pmtu(), or to only
ever lower the MSS, as tcp_v4_mtu_reduced() does?

[ ... ]

> +static int quic_packet_listen_process(struct sock *sk, struct sk_buff *skb,
> +				      gfp_t gfp)
> +{

[ ... ]

> +	if (!quic_packet_compatible_versions(version)) {
> +		/* rfc9000#section-6.1:
> +		 *
> +		 * If the version selected by the client is not acceptable to
> +		 * the server, the server responds with a Version Negotiation
> +		 * packet. This includes a list of versions that the server
> +		 * will accept.
> +		 */
> +		err = quic_packet_version_create_and_xmit(sk, gfp);
> +		consume_skb(skb);
> +		return err;
> +	}

[Severity: Medium]
Is a minimum datagram size check missing before this reply?

RFC 9000 sections 5.2.2 and 6.1 say a server MUST drop smaller packets
that specify an unsupported version. Section 14.1 says a server MUST
discard an Initial packet in a UDP datagram whose payload is smaller than
1200 bytes.

On the receive path nothing compares skb->len with QUIC_MIN_UDP_PAYLOAD.
quic_packet_rcv() and quic_packet_get_sock() only require
skb->len >= QUIC_HLEN.

Packets of any size can therefore reach the Version Negotiation reply
above, the Retry and refuse-close paths, and this call further down:

	req = quic_request_sock_create(sk, &odcid, retry, gfp);

When ALPN demux is disabled, the Length field isn't parsed at all. A
16-byte spoofed long-header Initial (8-byte DCID, empty SCID, empty
token) would allocate a quic_request_sock and call sk_acceptq_added().

Could a burst of these fill the accept queue, so that legitimate clients
hit sk_acceptq_is_full() and get CONNECTION_REFUSED?

Once the Version Negotiation and Retry builders are implemented, would
they also reply to spoofed sources for these small datagrams?

[ ... ]

> +/* Work function to process packets in the backlog queue. */
> +static void quic_packet_deferred_work(struct work_struct *work)
> +{

[ ... ]

> +	head = &quic_packet(sk)->deferred_list;

[Severity: Low]
This isn't a bug, but the comment says "backlog queue" while the function
drains deferred_list. struct quic_packet now has a separate backlog_list,
so should the comment say deferred list instead?

[ ... ]

> diff --git a/net/quic/packet.h b/net/quic/packet.h
> index cf0b97112d9b1..4a7bcf959e997 100644
> --- a/net/quic/packet.h
> +++ b/net/quic/packet.h
> @@ -14,6 +14,8 @@ struct quic_packet {
>  	union quic_addr daddr;    /* Dest address from received packet */
>  	union quic_addr saddr;    /* Source address from received packet */

[Severity: Low]
These comments seem reversed compared to how this patch fills the fields.

quic_get_msg_addrs() takes (skb, da, sa), and quic_packet_listen_process()
calls it like this:

	quic_get_msg_addrs(skb, &packet->saddr, &packet->daddr);

The inline comment there says "Read Destination address (packet->saddr)
and Source address (packet->daddr)".

So packet->saddr holds the received packet's destination (local) address
and packet->daddr holds its source (peer) address. The users,
quic_crypto_verify_token(), quic_accept_sock_exists() and
quic_listen_sock_switch(), all follow that convention.

Could the struct comments be changed to match?

[ ... ]

> diff --git a/net/quic/socket.c b/net/quic/socket.c
> index 0173416d0368a..be8dc30f8208a 100644
> --- a/net/quic/socket.c
> +++ b/net/quic/socket.c

[ ... ]

> +/* Find the listening QUIC socket for an incoming packet.
> + *
> + * This function searches the QUIC socket table for a listening socket that
> + * matches the dest address and port, and the ALPN(s) if presented in the
> + * ClientHello.  If multiple listening sockets are bound to the same address,
> + * port, and ALPN(s) (e.g., via SO_REUSEPORT), this function selects a socket
> + * from the reuseport group.

[Severity: Low]
This isn't a bug, but the function body doesn't select from a reuseport
group.

It walks the hash chain with sk_nulls_for_each_rcu() and returns the
first specific-address match, or an ANY-address candidate. It never calls
reuseport_select_sock() or looks at sk_reuseport_cb.

The v16 changelog says reuseport_select_sock() moved to the next
patchset. Should this part of the comment move with it?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1791227050.git.lucien.xin%40gmail.com