Re: [PATCH net-next v4 15/15] quic: add packet builder and parser base

On 10/29/25 3:35 PM, Xin Long wrote:
> +/* Process PMTU reduction event on a QUIC socket. */
> +void quic_packet_rcv_err_pmtu(struct sock *sk)
> +{
> +	struct quic_path_group *paths = quic_paths(sk);
> +	struct quic_packet *packet = quic_packet(sk);
> +	struct quic_config *c = quic_config(sk);
> +	u32 pathmtu, info, taglen;
> +	struct dst_entry *dst;
> +	bool reset_timer;
> +
> +	if (!ip_sk_accept_pmtu(sk))
> +		return;
> +
> +	info = clamp(paths->mtu_info, QUIC_PATH_MIN_PMTU, QUIC_PATH_MAX_PMTU);
> +	/* If PLPMTUD is not enabled, update MSS using the route and ICMP info. */
> +	if (!c->plpmtud_probe_interval) {
> +		if (quic_packet_route(sk) < 0)
> +			return;
> +
> +		dst = __sk_dst_get(sk);
> +		dst->ops->update_pmtu(dst, sk, NULL, info, true);
> +		quic_packet_mss_update(sk, info - packet->hlen);
> +		return;
> +	}
> +	/* PLPMTUD is enabled: adjust to smaller PMTU, subtract headers and AEAD tag.  Also
> +	 * notify the QUIC path layer for possible state changes and probing.
> +	 */
> +	taglen = quic_packet_taglen(packet);
> +	info = info - packet->hlen - taglen;
> +	pathmtu = quic_path_pl_toobig(paths, info, &reset_timer);
> +	if (reset_timer)
> +		quic_timer_reset(sk, QUIC_TIMER_PMTU, c->plpmtud_probe_interval);
> +	if (pathmtu)
> +		quic_packet_mss_update(sk, pathmtu + taglen);
> +}
> +
> +/* Handle ICMP Toobig packet and update QUIC socket path MTU. */
> +static int quic_packet_rcv_err(struct sk_buff *skb)
> +{
> +	union quic_addr daddr, saddr;
> +	struct sock *sk = NULL;
> +	int ret = 0;
> +	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, NULL);
> +	if (!sk)
> +		return -ENOENT;
> +
> +	bh_lock_sock(sk);
> +	if (quic_is_listen(sk))

The above looks race-prone. You should check the status only when
holding the sk socket lock, i.e. if !sock_owned_by_user(sk)

> +		goto out;
> +
> +	if (quic_get_mtu_info(skb, &info))
> +		goto out;

This can be moved outside the lock.

> +
> +	ret = 1; /* Success: update socket path MTU info. */
> +	quic_paths(sk)->mtu_info = info;
> +	if (sock_owned_by_user(sk)) {
> +		/* Socket is in use by userspace context.  Defer MTU processing to later via
> +		 * tasklet.  Ensure the socket is not dropped before deferral.
> +		 */
> +		if (!test_and_set_bit(QUIC_MTU_REDUCED_DEFERRED, &sk->sk_tsq_flags))
> +			sock_hold(sk);
> +		goto out;
> +	}
> +	/* Otherwise, process the MTU reduction now. */
> +	quic_packet_rcv_err_pmtu(sk);
> +out:
> +	bh_unlock_sock(sk);
> +	sock_put(sk);
> +	return ret;
> +}
> +
> +#define QUIC_PACKET_BACKLOG_MAX		4096
> +
> +/* Queue a packet for later processing when sleeping is allowed. */
> +static int quic_packet_backlog_schedule(struct net *net, struct sk_buff *skb)
> +{
> +	struct quic_skb_cb *cb = QUIC_SKB_CB(skb);
> +	struct quic_net *qn = quic_net(net);
> +
> +	if (cb->backlog)
> +		return 0;

The above test is present also in the only caller of this function. It
should be removed from there.

[...]> +/* Work function to process packets in the backlog queue. */
> +void quic_packet_backlog_work(struct work_struct *work)
> +{
> +	struct quic_net *qn = container_of(work, struct quic_net, work);
> +	struct sk_buff *skb;
> +	struct sock *sk;
> +
> +	skb = skb_dequeue(&qn->backlog_list);
> +	while (skb) {
> +		sk = quic_packet_get_listen_sock(skb);
> +		if (!sk)
> +			continue;
> +
> +		lock_sock(sk);

Possibly lock_sock_fast(sk);

> +		quic_packet_process(sk, skb);
> +		release_sock(sk);
> +		sock_put(sk);
> +
> +		skb = skb_dequeue(&qn->backlog_list);
> +	}
> +}

[...]> +/* Create and transmit a new QUIC packet. */
> +int quic_packet_create(struct sock *sk)
Possibly rename the function accordingly to its actual action, i.e.
quic_packet_create_xmit()

[...]> @@ -291,6 +294,8 @@ static void __net_exit quic_net_exit(struct
net *net)
>  #ifdef CONFIG_PROC_FS
>  	quic_net_proc_exit(net);
>  #endif
> +	skb_queue_purge(&qn->backlog_list);
> +	cancel_work_sync(&qn->work);

Likely: disable_work_sync()

>  	quic_crypto_free(&qn->crypto);
>  	free_percpu(qn->stat);
>  	qn->stat = NULL;

EPATCHISTOOBIG, very hard to process. Please split this one it at least
2 (i.e. rx and tx part), even if the series will grow above 15

/P