Re: [net-next,v10,14/15] quic: add packet builder base

[Paolo Abeni <pabeni@redhat.com>]

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 builder base

This patch introduces 'quic_packet' to handle packing of QUIC packets on
the transmit path, including frame packing, packet construction,
encryption, bundling, and transmission logic.

> diff --git a/net/quic/Makefile b/net/quic/Makefile
> index 2ccf01ad9e22..0f903f4a7ff1 100644
> --- a/net/quic/Makefile
> +++ b/net/quic/Makefile
> @@ -6,4 +6,4 @@
>  obj-$(CONFIG_IP_QUIC) += quic.o
>
>  quic-y := common.o family.o protocol.o socket.o stream.o connid.o path.o \
> -	  cong.o pnspace.o crypto.o timer.o
> +	  cong.o pnspace.o crypto.o timer.o packet.o
> diff --git a/net/quic/packet.c b/net/quic/packet.c
> new file mode 100644
> index 000000000000..a56edc745bb1
> --- /dev/null
> +++ b/net/quic/packet.c

[ ... ]

> +/* Perform routing for the QUIC packet on the specified path, update header length and MSS
> + * accordingly, reset path and start PMTU timer.
> + */
> +int quic_packet_route(struct sock *sk)
> +{
> +	struct quic_path_group *paths = quic_paths(sk);
> +	struct quic_packet *packet = quic_packet(sk);
> +	union quic_addr *sa, *da;
> +	u32 pmtu;
> +	int err;
> +
> +	da = quic_path_daddr(paths, packet->path);
> +	sa = quic_path_saddr(paths, packet->path);
> +	err = quic_flow_route(sk, da, sa, &paths->fl);
> +	if (err)
> +		return err;
> +
> +	packet->hlen = quic_encap_len(da);
> +	pmtu = min_t(u32, dst_mtu(__sk_dst_get(sk)), QUIC_PATH_MAX_PMTU);
> +	quic_packet_mss_update(sk, pmtu - packet->hlen);
> +
> +	quic_path_pl_reset(paths);
> +	quic_timer_reset(sk, QUIC_TIMER_PMTU, paths->plpmtud_interval);
> +	return 0;
> +}
> +
> +/* Configure the QUIC packet header and routing based on encryption level and path. */
> +int quic_packet_config(struct sock *sk, u8 level, u8 path)
> +{

[ ... ]

> +
> +	/* Perform routing and MSS update for the configured packet. */
> +	if (quic_packet_route(sk) < 0)
> +		return -1;
> +	return 0;
> +}

Should quic_packet_config() propagate the error code from
quic_packet_route() instead of returning -1?

The function quic_packet_route() returns proper negative errno values
(via quic_flow_route()), but quic_packet_config() replaces any error
with -1 (which is -EPERM). This could be misleading when diagnosing
routing failures.

> diff --git a/net/quic/packet.h b/net/quic/packet.h
> new file mode 100644
> index 000000000000..8c23be386207
> --- /dev/null
> +++ b/net/quic/packet.h

[ ... ]

> +static inline u8 quic_packet_taglen(struct quic_packet *packet)
> +{
> +	return packet->taglen[!!packet->level];
> +}
> +
> +static inline void quic_packet_set_taglen(struct quic_packet *packet, u8 taglen)
> +{
> +	packet->taglen[0] = taglen;
> +}
> +
> +static inline u32 quic_packet_mss(struct quic_packet *packet)
> +{
> +	return packet->mss[0] - packet->taglen[!!packet->level];
> +}
> +
> +static inline u32 quic_packet_max_payload(struct quic_packet *packet)
> +{
> +	return packet->mss[0] - packet->overhead - packet->taglen[!!packet->level];
> +}
> +
> +static inline u32 quic_packet_max_payload_dgram(struct quic_packet *packet)
> +{
> +	return packet->mss[1] - packet->overhead - packet->taglen[!!packet->level];
> +}

This isn't a bug, but quic_packet_mss(), quic_packet_max_payload(),
and quic_packet_max_payload_dgram() all repeat the expression
'packet->taglen[!!packet->level]'.

Could these use quic_packet_taglen(packet) instead for consistency?

[ ... ]