Re: [PATCH net-next v9 1/9] net: Use helpers to get/set UDP len tree-wide

[Simon Horman <horms@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.
Full review at: https://sashiko.dev
---
--- Patch [1]: [PATCH net-next v9 1/9] net: Use helpers to get/set UDP len tree-wide ---
commit 113d28557bcedccc492f330fafdaaf1377eb2e79
Author: Alice Mikityanska <alice@isovalent.com>
    
net: Use helpers to get/set UDP len tree-wide
    
This commit introduces helpers for setting and getting the UDP header length
tree-wide. It prepares the codebase to support BIG TCP for UDP tunnels by
explicitly handling cases where UDP length might be 0 for packets larger
than 65535 bytes.

> diff --git a/net/6lowpan/nhc_udp.c b/net/6lowpan/nhc_udp.c
> index 0a506c77283dd..ed4227e6db744 100644
> --- a/net/6lowpan/nhc_udp.c
> +++ b/net/6lowpan/nhc_udp.c
> @@ -88,16 +88,16 @@ static int udp_uncompress(struct sk_buff *skb, size_t needed)
>  	switch (lowpan_dev(skb->dev)->lltype) {
>  	case LOWPAN_LLTYPE_IEEE802154:
>  		if (lowpan_802154_cb(skb)->d_size)
> -			uh.len = htons(lowpan_802154_cb(skb)->d_size -
> -				       sizeof(struct ipv6hdr));
> +			udp_set_len_short(&uh, lowpan_802154_cb(skb)->d_size -
> +					  sizeof(struct ipv6hdr));

[Severity: High]
If a crafted packet specifies a datagram size smaller than
sizeof(struct ipv6hdr), won't this subtraction underflow?

The resulting massive unsigned value passed to udp_set_len_short() would
then trigger the DEBUG_NET_WARN_ON_ONCE, potentially causing a panic if
panic_on_warn is set.

> [ ... ]
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index 29651b1a0bc70..493e2b9e16fbc 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> [ ... ]
> @@ -915,12 +914,12 @@ static int udp_gro_complete_segment(struct sk_buff *skb)
>  int udp_gro_complete(struct sk_buff *skb, int nhoff,
>  		     udp_lookup_t lookup)
>  {
> -	__be16 newlen = htons(skb->len - nhoff);
>  	struct udphdr *uh = (struct udphdr *)(skb->data + nhoff);
> +	unsigned int newlen = skb->len - nhoff;
>  	struct sock *sk;
>  	int err;
>  
> -	uh->len = newlen;
> +	udp_set_len_short(uh, newlen);

[Severity: High]
If gro_max_size is configured larger than 65535 (which is supported and used
for BIG TCP), can't UDP GRO aggregate packets such that newlen > 65535?

This would instantly trigger the DEBUG_NET_WARN_ON_ONCE inside
udp_set_len_short() for legitimate large packets.

> [ ... ]
> diff --git a/net/ipv4/udp_tunnel_core.c b/net/ipv4/udp_tunnel_core.c
> index 9ab3728f9630b..0fccb38f074d2 100644
> --- a/net/ipv4/udp_tunnel_core.c
> +++ b/net/ipv4/udp_tunnel_core.c
> @@ -178,7 +178,7 @@ void udp_tunnel_xmit_skb(struct rtable *rt, struct sock *sk, struct sk_buff *skb
>  
>  	uh->dest = dst_port;
>  	uh->source = src_port;
> -	uh->len = htons(skb->len);
> +	udp_set_len_short(uh, skb->len);

[Severity: High]
When transmitting a TCP BIG TCP packet (which can be > 64K) over a UDP
tunnel like VXLAN, skb->len will exceed 65535.

Does using udp_set_len_short() here incorrectly trigger the warning for
valid large GSO payloads?

> [ ... ]
> diff --git a/net/ipv6/ip6_udp_tunnel.c b/net/ipv6/ip6_udp_tunnel.c
> index 9adb5775487fa..dcff7fb16ff64 100644
> --- a/net/ipv6/ip6_udp_tunnel.c
> +++ b/net/ipv6/ip6_udp_tunnel.c
> @@ -93,7 +93,7 @@ void udp_tunnel6_xmit_skb(struct dst_entry *dst, struct sock *sk,
>  	uh->dest = dst_port;
>  	uh->source = src_port;
>  
> -	uh->len = htons(skb->len);
> +	udp_set_len_short(uh, skb->len);

[Severity: High]
This appears to have the same issue as IPv4 tunnels. If a large GSO payload
is sent through an IPv6 UDP tunnel, won't skb->len exceed 65535 and trigger
the warning?