Re: [v2,net-next,07/15] udp: Remove partial csum code in RX.

Re: [v2,net-next,07/15] udp: Remove partial csum code in RX.

[Paolo Abeni]

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
---
udp: Remove partial csum code in RX.

This commit removes UDP-Lite specific partial checksum handling from UDP
receive paths, including removal of UDP_SKB_CB fields, simplification of
checksum initialization functions, and elimination of UDP-Lite guards.

Link: https://lore.kernel.org/netdev/20260305215013.2984628-8-kuniyu@google.com/

> diff --git a/include/net/udp.h b/include/net/udp.h
> --- a/include/net/udp.h
> +++ b/include/net/udp.h

[ ... ]

> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c

[ ... ]

> @@ -2703,14 +2647,15 @@ static int __udp4_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
>  	if (ulen > skb->len)
>  		goto short_packet;
>
> -	if (proto == IPPROTO_UDP) {
> -		/* UDP validates ulen. */
> -		if (ulen < sizeof(*uh) || pskb_trim_rcsum(skb, ulen))
> +	/* UDP validates ulen. */
> +	if (ulen < sizeof(*uh)) {
> +		if (pskb_trim_rcsum(skb, ulen))
>  			goto short_packet;
> +
>  		uh = udp_hdr(skb);
>  	}
>
> -	if (udp4_csum_init(skb, uh, proto))
> +	if (udp4_csum_init(skb, uh))
>  		goto csum_error;
>

Does this refactoring introduce two bugs?

The old code checked both conditions with short-circuit OR:
  if (ulen < sizeof(*uh) || pskb_trim_rcsum(skb, ulen))
      goto short_packet;

This correctly (1) rejected packets where ulen < 8, and (2) trimmed the
SKB to the declared UDP length when ulen < skb->len.

The new code changes the logic structure:
  if (ulen < sizeof(*uh)) {
      if (pskb_trim_rcsum(skb, ulen))
          goto short_packet;
      uh = udp_hdr(skb);
  }

First potential bug: When sizeof(*uh) <= ulen < skb->len, the entire block
is skipped. This means pskb_trim_rcsum() is never called, so the SKB
retains trailing data beyond the declared UDP length. Won't this cause
checksum calculations to include extraneous bytes, leading to valid packets
being incorrectly rejected?

This scenario occurs when UDP length < IP payload length, which can happen
with malformed or crafted packets.

Second potential bug: When ulen < sizeof(*uh), the old code would
immediately goto short_packet via the || short-circuit. The new code enters
the block and tries to trim the SKB to a sub-header-size length. If
pskb_trim_rcsum() succeeds, processing continues with skb->len <
sizeof(struct udphdr). Is this safe?

Looking at the IPv6 counterpart __udp6_lib_rcv(), it handles this
differently:

  if (ulen < sizeof(*uh))
      goto short_packet;

  if (ulen < skb->len) {
      if (pskb_trim_rcsum(skb, ulen))
          goto short_packet;
      saddr = &ipv6_hdr(skb)->saddr;
      daddr = &ipv6_hdr(skb)->daddr;
      uh = udp_hdr(skb);
  }

Should the IPv4 code follow the same pattern, or should it preserve the
original short-circuit logic:

  if (ulen < sizeof(*uh) || pskb_trim_rcsum(skb, ulen))
      goto short_packet;
  uh = udp_hdr(skb);

Re: [v2,net-next,07/15] udp: Remove partial csum code in RX.

[Kuniyuki Iwashima]

On Tue, Mar 10, 2026 at 3:26 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> 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
> ---
> udp: Remove partial csum code in RX.
>
> This commit removes UDP-Lite specific partial checksum handling from UDP
> receive paths, including removal of UDP_SKB_CB fields, simplification of
> checksum initialization functions, and elimination of UDP-Lite guards.
>
> Link: https://lore.kernel.org/netdev/20260305215013.2984628-8-kuniyu@google.com/
>
> > diff --git a/include/net/udp.h b/include/net/udp.h
> > --- a/include/net/udp.h
> > +++ b/include/net/udp.h
>
> [ ... ]
>
> > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> > --- a/net/ipv4/udp.c
> > +++ b/net/ipv4/udp.c
>
> [ ... ]
>
> > @@ -2703,14 +2647,15 @@ static int __udp4_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
> >       if (ulen > skb->len)
> >               goto short_packet;
> >
> > -     if (proto == IPPROTO_UDP) {
> > -             /* UDP validates ulen. */
> > -             if (ulen < sizeof(*uh) || pskb_trim_rcsum(skb, ulen))
> > +     /* UDP validates ulen. */
> > +     if (ulen < sizeof(*uh)) {
> > +             if (pskb_trim_rcsum(skb, ulen))
> >                       goto short_packet;
> > +
> >               uh = udp_hdr(skb);
> >       }
> >
> > -     if (udp4_csum_init(skb, uh, proto))
> > +     if (udp4_csum_init(skb, uh))
> >               goto csum_error;
> >
>
> Does this refactoring introduce two bugs?
>
> The old code checked both conditions with short-circuit OR:
>   if (ulen < sizeof(*uh) || pskb_trim_rcsum(skb, ulen))
>       goto short_packet;
>
> This correctly (1) rejected packets where ulen < 8, and (2) trimmed the
> SKB to the declared UDP length when ulen < skb->len.
>
> The new code changes the logic structure:
>   if (ulen < sizeof(*uh)) {
>       if (pskb_trim_rcsum(skb, ulen))
>           goto short_packet;
>       uh = udp_hdr(skb);
>   }
>
> First potential bug: When sizeof(*uh) <= ulen < skb->len, the entire block
> is skipped. This means pskb_trim_rcsum() is never called, so the SKB
> retains trailing data beyond the declared UDP length. Won't this cause
> checksum calculations to include extraneous bytes, leading to valid packets
> being incorrectly rejected?
>
> This scenario occurs when UDP length < IP payload length, which can happen
> with malformed or crafted packets.
>
> Second potential bug: When ulen < sizeof(*uh), the old code would
> immediately goto short_packet via the || short-circuit. The new code enters
> the block and tries to trim the SKB to a sub-header-size length. If
> pskb_trim_rcsum() succeeds, processing continues with skb->len <
> sizeof(struct udphdr). Is this safe?
>
> Looking at the IPv6 counterpart __udp6_lib_rcv(), it handles this
> differently:
>
>   if (ulen < sizeof(*uh))
>       goto short_packet;
>
>   if (ulen < skb->len) {
>       if (pskb_trim_rcsum(skb, ulen))
>           goto short_packet;
>       saddr = &ipv6_hdr(skb)->saddr;
>       daddr = &ipv6_hdr(skb)->daddr;
>       uh = udp_hdr(skb);
>   }
>
> Should the IPv4 code follow the same pattern, or should it preserve the
> original short-circuit logic:

Indeed, I'll follow the IPv6 pattern there.

Thanks !