Re: [PATCH net-next v15 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: 1 · Medium: 5 · Low: 1

- [High] quic_request_sock_create() publishes a newly allocated request
  before its first packet is successfully queued: it links req into…
- [Medium] Commit message understates the scope of the patch and does not
  disclose that the newly added listen-side connection-establishment…
- [Medium] quic_listen_sock_lookup() passes hdr_len = 1 to
  reuseport_select_sock() (net/quic/socket.c:292-293), but by the time…
- [Medium] In quic_accept_sock_exists() (net/quic/socket.c:132-135) and
  quic_listen_sock_switch() (socket.c:~330) the indirect call is…
- [Medium] A QUIC Initial decryption failure inside
  quic_packet_get_alpn() is counted twice and once under the wrong…
- [Medium] quic_packet_listen_process() treats every non-zero return of
  quic_crypto_verify_token() as a peer token-validation failure.
- [Low] The documented meanings of struct quic_packet's saddr/daddr are
  the reverse of the new receive-side implementation.

--

quic: add packet parser base

This adds the RX side of the QUIC packet handling: quic_packet_rcv() looks
up the socket for an incoming datagram, quic_packet_process() dispatches it,
and ICMP Packet Too Big errors are turned into PMTU updates.  A listener
path with ALPN parsing, request sock creation and a per-socket deferred
workqueue is added as well.

A few questions on the new code below.

> diff --git a/net/quic/packet.c b/net/quic/packet.c
> index a1967f39c9242..db87c32d7469d 100644
> --- a/net/quic/packet.c
> +++ b/net/quic/packet.c
> @@ -14,6 +14,768 @@

[ ... ]

> +/* Decrypt Initial packet and extract ALPN from TLS ClientHello for ALPN-based
> + * socket demultiplexing. Marks packet as decrypted (cb->resume = 1) to avoid
> + * redundant decryption later.
> + */
> +static int quic_packet_get_alpn(struct sk_buff *skb, struct quic_data *alpn)
> +{

[ ... ]

> +	cb->sync = 1;
> +	net = sock_net(sk);
> +	err = quic_crypto_decrypt(crypto, skb, GFP_KERNEL);
> +	if (err) {
> +		QUIC_INC_STATS(net, QUIC_MIB_PKT_DECDROP);
> +		return err;
> +	}

[Severity: Medium]
Is a single dropped packet meant to bump two drop counters here?

quic_packet_get_alpn() counts the AEAD failure as QUIC_MIB_PKT_DECDROP and
returns the error, and the caller in quic_packet_listen_process() then counts
the same skb again:

	err = quic_packet_get_alpn(skb, &alpns);
	if (err) {
		QUIC_INC_STATS(net, QUIC_MIB_PKT_INVHDRDROP);
		kfree_skb(skb);
		return err;
	}

net/quic/protocol.h documents these as distinct reasons:

	QUIC_MIB_PKT_INVHDRDROP,     /* Dropped: invalid packet header */
	...
	QUIC_MIB_PKT_DECDROP,        /* Dropped: decryption failure */

So QuicPktInvHdrDrop ends up reporting drops for packets whose header parsed
fine.  The same applies to a quic_crypto_initial_keys_install() failure inside
quic_packet_get_alpn(), which is not a header error either.

[ ... ]

> +static int quic_packet_retry_create_and_xmit(struct sock *sk)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static int quic_packet_version_create_and_xmit(struct sock *sk, gfp_t gfp)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static int quic_packet_stateless_reset_create_and_xmit(struct sock *sk, u32 len,
> +						       gfp_t gfp)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static int quic_packet_refuse_close_create_and_xmit(struct sock *sk,
> +						    u32 errcode)
> +{
> +	return -EOPNOTSUPP;
> +}

[Severity: Medium]
Could the changelog describe the listener side that is added here?

The commit message says:

  "Similar to the TX path, handling for application and handshake packets is
   not yet implemented."

and names only quic_packet_get_alpn(), quic_packet_rcv(),
quic_packet_process() and quic_packet_rcv_err_pmtu().

The patch also adds the whole listen path: quic_packet_listen_process() with
token validation and Retry triggering, quic_request_sock_create() /
quic_request_sock_lookup() / quic_request_sock_backlog_tail() with accept
queue accounting, quic_accept_sock_exists(), quic_listen_sock_switch(), two
socket lookup helpers and a per-socket deferred workqueue.

At the same time every response the embedded RFC quotes promise is one of the
four stubs above, and the skb is then consumed, so those packets are silently
dropped rather than answered.

And the request socks produced by this path have no consumer in the resulting
tree, since the registered .accept callback is:

net/quic/socket.c:
	static struct sock *quic_accept(struct sock *sk, struct proto_accept_arg *arg)
	{
		arg->err = -EOPNOTSUPP;
		return NULL;
	}

so requests that are created, charged with sk_acceptq_added() and signalled
with sk_data_ready() can never be dequeued.

[ ... ]

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

[ ... ]

> +	/* Read Destination address (packet->saddr) and Source address
> +	 * (packet->daddr).
> +	 */
> +	quic_get_msg_addrs(skb, &packet->saddr, &packet->daddr);

[Severity: Low]
This isn't a bug, but the struct quic_packet field comments now say the
opposite of what this code stores.

quic_get_msg_addrs(skb, da, sa) fills its first output with the packet's
destination:

net/quic/family.c:
	static void quic_v4_get_msg_addrs(struct sk_buff *skb, union quic_addr *da,
					  union quic_addr *sa)
	{
		...
		da->v4.sin_addr.s_addr = ip_hdr(skb)->daddr;

so packet->saddr holds the local address and packet->daddr holds the peer,
which matches the local comment here and all the new consumers
(quic_request_sock_create/lookup, quic_accept_sock_exists,
quic_listen_sock_switch), but not the declarations in packet.h quoted further
down.  Could the struct comments be updated to match?

[ ... ]

> +		/* Verify Token. */
> +		crypto = quic_crypto(sk, QUIC_CRYPTO_INITIAL);
> +		err = quic_crypto_verify_token(crypto, &packet->daddr,
> +					       sizeof(packet->daddr),
> +					       &odcid, token.data, token.len);
> +		if (err) {
> +			if (!retry) {
> +				err = quic_packet_retry_create_and_xmit(sk);
> +				consume_skb(skb);
> +				return err;
> +			}

[Severity: Medium]
Should a local allocation failure here be reported to the peer as an invalid
token?

quic_crypto_verify_token() does not only return token mismatches:

net/quic/crypto.c:
	token_buf = kmemdup(token, len, GFP_KERNEL);
	if (!token_buf)
		return -ENOMEM;

and it also propagates -ENOMEM from quic_crypto_token_protect(), which
allocates via quic_crypto_aead_mem_alloc().

Under memory pressure a client presenting a perfectly valid NEW_TOKEN token
gets a Retry it should not receive, and one presenting a valid Retry token
takes the rfc9000#section-8.1.3 path:

	errcode = QUIC_TRANSPORT_ERROR_INVALID_TOKEN;
	quic_packet_refuse_close_create_and_xmit(sk, errcode);

Would it be better to distinguish -ENOMEM from a real verification failure
and just drop the packet?

[ ... ]

> +out:
> +	/* Add to backlog list and wake blocked accept() calls */
> +	return quic_request_sock_backlog_tail(sk, req, skb);
> +}

[ ... ]

> diff --git a/net/quic/packet.h b/net/quic/packet.h
> index 18b89f5051214..7f12bb53927b8 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 */
>  
> +	struct sk_buff_head deferred_list; /* Packets deferred to work queue */
> +	struct sk_buff_head backlog_list;  /* Packets waiting for crypto keys */

These are the two comments referred to above: daddr is described as the
received packet's destination and saddr as its source, while the new RX code
stores them the other way around.

[ ... ]

> diff --git a/net/quic/path.c b/net/quic/path.c
> index a9e02fc22e609..a41b99dd31eff 100644
> --- a/net/quic/path.c
> +++ b/net/quic/path.c
> @@ -27,14 +27,14 @@ static int quic_udp_rcv(struct sock *sk, struct sk_buff *skb)
>  
>  	skb_pull(skb, sizeof(struct udphdr));
>  	skb_dst_force(skb);
> -	kfree_skb(skb);
> +	quic_packet_rcv(sk, skb, false);

Note the skb_pull() of the UDP header before quic_packet_rcv() runs; see the
reuseport question in socket.c below.

[ ... ]

> diff --git a/net/quic/socket.c b/net/quic/socket.c
> index 1e278a2109b2f..0fcb123d31bba 100644
> --- a/net/quic/socket.c
> +++ b/net/quic/socket.c
> @@ -24,6 +24,324 @@ static void quic_enter_memory_pressure(struct sock *sk)

[ ... ]

> +/* Create and enqueue a QUIC request sock for a new incoming connection. */
> +struct quic_request_sock *quic_request_sock_create(struct sock *sk,
> +						   struct quic_conn_id *odcid,
> +						   u8 retry, gfp_t gfp)
> +{

[ ... ]

> +	/* Enqueue request into listen socket’s pending list for accept(). */
> +	list_add_tail(&req->list, quic_reqs(sk));
> +	sk_acceptq_added(sk);
> +	return req;
> +}
> +
> +int quic_request_sock_backlog_tail(struct sock *sk,
> +				   struct quic_request_sock *req,
> +				   struct sk_buff *skb)
> +{
> +	u32 limit = sk->sk_rcvbuf / sk->sk_max_ack_backlog;
> +	int len = skb->truesize;
> +
> +	limit =  max_t(u32, limit, QUIC_MIN_UDP_PAYLOAD * 4);
> +	if (req->blen + len > limit || !__sk_rmem_schedule(sk, len, false)) {
> +		QUIC_INC_STATS(sock_net(sk), QUIC_MIB_PKT_RCVDROP);
> +		kfree_skb(skb);
> +		return -ENOBUFS;
> +	}

[Severity: High]
Can this path leak the quic_request_sock and permanently consume an accept
queue slot?

quic_request_sock_create() already published the request before the first
packet was queued:

	list_add_tail(&req->list, quic_reqs(sk));
	sk_acceptq_added(sk);

and quic_packet_listen_process() then hands it straight to the enqueue:

net/quic/packet.c:
	out:
		/* Add to backlog list and wake blocked accept() calls */
		return quic_request_sock_backlog_tail(sk, req, skb);

On the -ENOBUFS path above only the skb is freed.  The request stays linked in
quic_reqs(sk) with an empty backlog, and sk_acceptq_added() is not undone.

The size test is deterministic for a given packet size, since limit is
max(sk_rcvbuf / sk_max_ack_backlog, QUIC_MIN_UDP_PAYLOAD * 4), so a client
sending a large Initial fails on the first attempt and on every
retransmission, and the slot is never reclaimed.  Repeating with different
addresses or DCIDs would then make sk_acceptq_is_full() refuse all further
connections.

Should this failure path unlink and free a freshly created request, or should
the request only be created after the first packet is successfully queued?

> +
> +	QUIC_SKB_CB(skb)->backlog = 1;
> +	skb_set_owner_r(skb, sk);
> +	__skb_queue_tail(&req->backlog_list, skb);
> +	req->blen += len;
> +
> +	sk->sk_data_ready(sk);
> +	return 0;
> +}
> +
> +/* Check if a matching accept socket exists. This is needed because an accept
> + * socket might have been created after this packet was enqueued in the listen
> + * socket's backlog.
> + */
> +bool quic_accept_sock_exists(struct sock *sk, struct sk_buff *skb)
> +{

[ ... ]

> +	} else {
> +		/* Socket not busy: process immediately. */
> +		sk->sk_backlog_rcv(sk, skb); /* quic_packet_process(). */
> +	}

[Severity: Medium]
Is the annotation on this indirect call right?  .backlog_rcv is
quic_backlog_rcv(), further down in this file:

	static int quic_backlog_rcv(struct sock *sk, struct sk_buff *skb)
	{
		return quic_packet_process(sk, skb, GFP_ATOMIC);
	}

The existing site in quic_packet_rcv() spells it as
/* quic_backlog_rcv(). */, so the two new sites here and in
quic_listen_sock_switch() look inconsistent.

More than the name: quic_listen_sock_switch() runs in sleepable context (it
calls release_sock()/lock_sock() around the hand-off and is reached from the
deferred workqueue), but routing through .backlog_rcv pins the allocation
flags to GFP_ATOMIC, which defeats the gfp argument this patch threads through
quic_packet_process() so that quic_packet_deferred_work() can pass
GFP_KERNEL.  Should those two sites call quic_packet_process() directly with
the appropriate gfp?

[ ... ]

> +struct sock *quic_listen_sock_lookup(struct sk_buff *skb, union quic_addr *sa,
> +				     union quic_addr *da, struct sock *usk,
> +				     struct quic_data *alpns)
> +{

[ ... ]

> +out:
> +	if (sk && sk->sk_reuseport)
> +		sk = reuseport_select_sock(sk, quic_addr_hash(net, da), skb, 1);

[Severity: Medium]
Should the hdr_len argument be 0 rather than 1 here?

quic_udp_rcv() already did skb_pull(skb, sizeof(struct udphdr)) before
quic_packet_rcv() is reached, so skb->data points at the QUIC payload, and
reuseport_select_sock() documents hdr_len as how far the pointer must advance
to reach the payload.

With a classic SO_ATTACH_REUSEPORT_CBPF program, run_bpf_filter() does:

net/core/sock_reuseport.c:
	if (unlikely(!pskb_may_pull(skb, hdr_len)))
		return -EINVAL;
	...
	pskb_pull(skb, hdr_len);
	...
	__skb_push(skb, hdr_len);

so the filter would see the datagram starting one byte into the QUIC header:
all its offsets are shifted and the first byte (form/fixed/type bits) is
invisible, which can select the wrong socket in the reuseport group.

[ ... ]

> +bool quic_listen_sock_switch(struct sk_buff *skb, struct quic_data *alpns)
> +{

[ ... ]

> +	lock_sock(nsk);
> +	nsk->sk_backlog_rcv(nsk, skb); /* quic_packet_process(). */
> +	release_sock(nsk);

This is the second site of the .backlog_rcv naming and GFP_ATOMIC question
raised above, and the one running in sleepable context.

[ ... ]

> @@ -262,8 +587,7 @@ static void quic_shutdown(struct sock *sk, int how)
>  
>  static int quic_backlog_rcv(struct sock *sk, struct sk_buff *skb)
>  {
> -	kfree_skb(skb);
> -	return 0;
> +	return quic_packet_process(sk, skb, GFP_ATOMIC);
>  }

[ ... ]

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