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

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

Critical: 0 · High: 1 · Medium: 1 · 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).

- [High] When PLPMTUD is disabled, ICMP Packet Too Big messages never
  lower the MSS.
- [Medium] quic_accept_sock_exists() (net/quic/socket.c) passes an skb to
  an accept socket's sk_backlog_rcv() while atomic: local_bh_disable()…
- [Low] quic_packet_get_connid() rejects any connection ID longer than
  QUIC_CONN_ID_MAX_LEN (20) regardless of version.
- [Low] quic_request_sock_backlog_tail() calls __sk_rmem_schedule(sk,
  len) before skb_set_owner_r().
- [Low] quic_packet_deferred_work() drains deferred_list while holding
  lock_sock(sk) and never calls cond_resched(): `while ((skb =…
- [Low] quic_packet_get_alpn() prints the u32 field alpn->len (struct
  quic_data in net/quic/common.h) with %d: `pr_debug("%s: alpn_len:…

--

quic: add packet parser base

This patch adds the QUIC receive path. Sockets are looked up by
connection ID and address, and packets are dispatched by socket state.
Listen sockets handle client Initial packets (request sockets, ALPN
demux, deferred work), and ICMP Packet Too Big errors update the path
MTU.

[ ... ]

[Severity: Low]
Should the 20-byte connection ID limit be applied before we know the
version is supported?

quic_packet_get_long_header() is called from quic_packet_get_sock() and
from quic_packet_listen_process(). Both calls happen before the
!quic_packet_compatible_versions(version) branch that sends Version
Negotiation.

So a long-header packet with an unknown version and a CID longer than
20 bytes is dropped as an invalid header, and no Version Negotiation is
sent.

RFC 9000 section 17.2 limits CIDs to 20 bytes only for version 1.
Sections 5.2.2 and 6.1 say a server SHOULD be able to read longer CIDs
from other versions so that it can send Version Negotiation.

[ ... ]

[ ... ]

[Severity: High]
Does this ever lower the MSS when PLPMTUD is disabled?

With skb == NULL, ip_rt_update_pmtu() builds its flow key through
ip_rt_build_flow_key()->build_sk_flow_key(), which takes the
destination from the socket:

net/ipv4/route.c:build_sk_flow_key() {
    ...
	__be32 daddr = inet->inet_daddr;
    ...
}

When there is no packet header, __ip6_rt_update_pmtu() does the same
with sk->sk_v6_daddr.

Nothing in net/quic sets inet_daddr or sk_v6_daddr, and ip6_dst_store()
is called with daddr_set false. The exception would then be created for
0.0.0.0 or :: instead of the peer.

Even with the right key there is a second problem. When
update_or_create_fnhe() creates a new exception, it only marks the
cached routes obsolete:

	WRITE_ONCE(rt->dst.obsolete, DST_OBSOLETE_KILL);

It does not change their MTU. The IPv6 path inserts a new RTF_CACHE
route and leaves the old dst unchanged.

So re-reading dst_mtu(dst) on the same cached dst would return the old
MTU, and quic_packet_mss_update() keeps the old MSS.

If that's right, the v17 change to re-read the MTU after update_pmtu()
makes the ICMP-driven reduction a no-op. plpmtud_interval is 0 by
default, so a path whose PMTU is below the local MTU could stall:
full-size DF packets are dropped and the PTBs are ignored.

For comparison, SCTP temporarily sets the socket daddr around
update_pmtu() (to_sk_daddr()), and inet_csk_update_pmtu() re-checks the
dst with __sk_dst_check() afterwards. Is something similar needed here?

[ ... ]

+	pr_debug("%s: alpn_len: %d\n", __func__, alpn->len);

[Severity: Low]
This isn't a bug, but alpn->len is a u32 in struct quic_data, so %u
would match it. The v16 changelog made the same fix in quic_packet_rcv().

[ ... ]

[Severity: Low]
Was the cond_resched() from the v12 changelog lost in the v15 rework?
The v12 entry reads:

  "Call cond_resched() in the loop inside quic_packet_backlog_work()."

Each iteration here can run synchronous AEAD decryption (cb->sync = 1),
GFP_KERNEL allocations and token verification, all under lock_sock().

On the quic_listen_sock_switch() path, the listener lock is dropped and
re-taken in the middle of this loop. Under sustained traffic, packets
processed from the socket backlog in that window can keep adding to
deferred_list, so the loop keeps running.

[ ... ]

[ ... ]

[Severity: Low]
Can this push sk_forward_alloc negative when the skb is already charged
to this listener?

Consider a second Initial packet for the same connection that was
deferred through quic_packet_deferred_schedule(). Its destructor is
already sock_rfree, and in quic_packet_listen_process() it takes this
path, which skips skb_set_owner_sk_safe():

	req = quic_request_sock_lookup(sk);
	if (req) /* If request sock already exists, enqueue packet directly. */
		goto out;

In quic_request_sock_backlog_tail(), __sk_rmem_schedule() reserves the
memory first. skb_set_owner_r() then orphans the skb
(sock_rfree()->sk_mem_uncharge()->sk_mem_reclaim()) and charges it
again.

The reclaim can return the pages that were just reserved, so the
sk_mem_charge() that follows can leave sk_forward_alloc below zero.

For example, start with forward_alloc 2000 and truesize 2304:

  schedule adds a page:      6096
  uncharge:                  8400
  reclaim leaves:            208
  charge leaves:             -2096

Would orphaning the skb before the __sk_rmem_schedule() check avoid
this?

+	__skb_queue_tail(&req->backlog_list, skb);
+	req->blen += len;

[ ... ]

[Severity: Medium]
Should cb->backlog and cb->sync be cleared before the skb is handed to
the accept socket here?

When this runs from quic_packet_deferred_work(), the skb came off
deferred_list with cb->backlog = 1. An skb that went through
quic_listen_sock_switch() also has cb->sync = 1 and cb->resume = 1, set
by quic_packet_get_alpn().

The call path here is atomic:

quic_packet_deferred_work()
  quic_packet_process()
    quic_packet_listen_process()
      quic_accept_sock_exists()
        local_bh_disable()
        bh_lock_sock_nested(accept_sk)
        accept_sk->sk_backlog_rcv()
          quic_packet_process(..., GFP_ATOMIC)

Elsewhere the patch treats cb->backlog as meaning the context can sleep,
for example in quic_packet_deferred_schedule():

	if (cb->backlog)
		return 0;

quic_crypto_decrypt() also checks !cb->backlog before doing a key
update, and cb->sync forces a blocking crypto wait.

The handshake and app handlers are still stubs in this patch. Once they
are filled in, could these leftover flags make them sleep while holding
the spinlock with BH disabled?

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