Re: [PATCH net-next v6 05/11] udp: Support gro_ipv4_max_size > 65536

[Jakub Kicinski <kuba@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.
---
udp: Support gro_ipv4_max_size > 65536

This commit allows GRO to aggregate UDP packets larger than 65535 bytes
by writing 0 to the UDP header length field (via udp_set_len()) and
restoring the real length from skb->len in udp_rcv().

> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 3e92575f1d55..856c23b9d6dd 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -2605,6 +2605,9 @@ int udp_rcv(struct sk_buff *skb)
>  	if (ulen > skb->len)
>  		goto short_packet;
>  
> +	if (!ulen)
> +		ulen = skb->len;
> +
>  	if (ulen < sizeof(*uh))
>  		goto short_packet;
>  
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index 96987b1c05cd..f7da760f046f 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -919,7 +919,7 @@ int udp_gro_complete(struct sk_buff *skb, int nhoff,
>  	struct sock *sk;
>  	int err;
>  
> -	udp_set_len_short(uh, newlen);
> +	udp_set_len(uh, newlen);

[High]
After this change, udp_gro_complete() may write uh->len = 0 when the
aggregated packet is >= GRO_LEGACY_MAX_SIZE (65536).  How does this
interact with conntrack on the receive path?

Looking at udp_validate_len() in net/netfilter/nf_conntrack_proto_udp.c
at this commit:

    static bool udp_validate_len(...)
    {
        unsigned int udplen = udp_get_len_short(hdr);
        ...
        if (udplen > skblen || udplen < sizeof(*hdr))
            return false;
        ...
    }

For a super-packet where uh->len has been zeroed, udp_get_len_short()
returns 0, which is less than sizeof(*hdr), so udp_validate_len() returns
false.  udp_error() then logs "truncated/malformed packet" and
nf_conntrack_udp_packet() returns -NF_ACCEPT, leaving the flow untracked.

The deployment this patch enables (gro_ipv4_max_size > 65536 with VXLAN
or Geneve TCP-in-UDP and conntrack) is the very example given in the
commit message.  Should this patch also update udp_validate_len() (or
otherwise account for the zeroed length) so that conntrack does not drop
out of the state machine for super-packets, breaking NAT and stateful
firewalling?

The follow-up patch in this series, "udp: Support BIG TCP GSO packets
where they can occur" (b73c2d663201), updates udp_validate_len() to
accept udplen == 0 when skblen >= GRO_LEGACY_MAX_SIZE, but until that
patch is applied this commit on its own appears to regress conntrack for
the configurations it is intended to support.

>  	sk = INDIRECT_CALL_INET(lookup, udp6_lib_lookup_skb,
>  				udp4_lib_lookup_skb, skb, uh->source, uh->dest);

[ ... ]