Re: [net-next,v9,15/15] quic: add packet parser base

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
quic: add packet parser base

This patch uses 'quic_packet' to handle packing of QUIC packets on the
receive (RX) path. It introduces mechanisms to parse the ALPN from client
Initial packets to determine the correct listener socket, routes received
packets accordingly, and adds support for ICMP-based MTU updates by
locating the relevant socket and updating the stored PMTU.

> diff --git a/net/quic/packet.c b/net/quic/packet.c
> index cf64a6aaa795..16a7db6ccd87 100644
> --- a/net/quic/packet.c
> +++ b/net/quic/packet.c

[ ... ]

> @@ -14,6 +14,637 @@
>
>  #define QUIC_HLEN		1
>
> +/* Handle ICMP Toobig packet and update QUIC socket path MTU. */
> +static int quic_packet_rcv_err(struct sock *sk, struct sk_buff *skb)
> +{
> +	union quic_addr daddr, saddr;
> +	u32 info;
> +
> +	/* All we can do is lookup the matching QUIC socket by addresses. */
> +	quic_get_msg_addrs(skb, &saddr, &daddr);
> +	sk = quic_sock_lookup(skb, &daddr, &saddr, sk, NULL);

Are the address arguments swapped in quic_packet_rcv_err()? Looking at
other call sites in this file, quic_get_msg_addrs is consistently called
with &daddr first, then &saddr:

  Line 476 in quic_packet_get_listen_sock():
    quic_get_msg_addrs(skb, &daddr, &saddr);

  Line 513 in quic_packet_get_sock():
    quic_get_msg_addrs(skb, &daddr, &saddr);

  Line 536 in quic_packet_get_sock():
    quic_get_msg_addrs(skb, &daddr, &saddr);

But here quic_packet_rcv_err() passes &saddr first, then &daddr.

Looking at the implementation of quic_v4_get_msg_addrs() in
net/quic/family.c, the function extracts addresses from the packet as:

  sa->v4.sin_port = uh->source;
  sa->v4.sin_addr.s_addr = ip_hdr(skb)->saddr;
  da->v4.sin_port = uh->dest;
  da->v4.sin_addr.s_addr = ip_hdr(skb)->daddr;

So sa gets the packet's source address (remote), and da gets the packet's
destination address (local).

Then quic_sock_lookup() in net/quic/socket.c expects:

  if (quic_cmp_sk_addr(tmp, quic_path_saddr(paths, 0), sa) &&
      quic_cmp_sk_addr(tmp, quic_path_daddr(paths, 0), da) &&

This compares the socket's local address with sa and the socket's remote
address with da, meaning sa should be the local (packet dest) address and
da should be the remote (packet source) address.

With the swapped arguments, quic_packet_rcv_err() would pass the remote
address where the local is expected and vice versa, causing the socket
lookup to fail. This would prevent ICMP Packet Too Big messages from being
processed correctly, breaking Path MTU Discovery.

> +	if (!sk)
> +		return -ENOENT;
> +
> +	if (quic_get_mtu_info(skb, &info)) {
> +		sock_put(sk);
> +		return 0;
> +	}
> +
> +	/* Success: update socket path MTU info. */
> +	bh_lock_sock(sk);
> +	quic_paths(sk)->mtu_info = info;

[ ... ]