Netdev List
 help / color / mirror / Atom feed
From: Simon Horman <horms@kernel.org>
To: Alice Mikityanska <alice@isovalent.com>
Cc: alice.kernel@fastmail.im, daniel@iogearbox.net,
	davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
	pabeni@redhat.com, lucien.xin@gmail.com,
	willemdebruijn.kernel@gmail.com, willemb@google.com,
	dsahern@kernel.org, razor@blackwall.org, shuah@kernel.org,
	stfomichev@gmail.com, andrew+netdev@lunn.ch, fw@strlen.de,
	netdev@vger.kernel.org
Subject: Re: [PATCH net-next v9 1/9] net: Use helpers to get/set UDP len tree-wide
Date: Fri, 17 Jul 2026 13:40:46 +0100	[thread overview]
Message-ID: <20260717124046.GO95246@horms.kernel.org> (raw)
In-Reply-To: <CAD0BsJWRPNsQrBT06QqCtB5atqjBQz4y8QWvGg-v2WZDqRiPRA@mail.gmail.com>

On Thu, Jul 16, 2026 at 06:18:59PM +0300, Alice Mikityanska wrote:
> On Thu, 16 Jul 2026 at 17:47, Simon Horman <horms@kernel.org> 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.
> > 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?
> 
> First, I'd like to emphasize that I'm merely converting direct uh.len
> access to the new helper. If there is an existing bug with underflow,
> my new helper will expose it with a WARN, which is good. But should
> such bugs exist before this refactoring, I don't think that fixing all
> of them is in scope of this series.

Yes, point taken.

...

> > [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 is the first patch that just converted direct access to helpers.
> Patch 4 "udp: Support gro_ipv4_max_size > 65536" explicitly enables
> len > 64k and replaces this line with udp_set_len. This comment on
> patch 1 is irrelevant because it assumes that functionality added in a
> later patch should work in a prior patch.

Agreed, the AI-generated review seems wide of the mark here.

...

> > [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?
> 
> Same here. This is replaced by udp_set_len, which supports len > 64k,
> in the patch that actually enables len > 64k.
> 
> > > [ ... ]
> > > 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?
> 
> Same here.
> 
> I'm sorry, but I highly doubt that these AI review comments were validated.

Clearly my validation was insufficient.
Sorry about that.

  reply	other threads:[~2026-07-17 12:40 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CGME20260715061259epcas5p378f743489493da324fc9092ed2dfe147@epcas5p3.samsung.com>
2026-07-10 13:42 ` [PATCH net-next v9 0/9] BIG TCP for UDP tunnels Alice Mikityanska
2026-07-10 13:42   ` [PATCH net-next v9 1/9] net: Use helpers to get/set UDP len tree-wide Alice Mikityanska
2026-07-16 14:46     ` Simon Horman
2026-07-16 15:18       ` Alice Mikityanska
2026-07-17 12:40         ` Simon Horman [this message]
2026-07-10 13:42   ` [PATCH net-next v9 2/9] net: Enable BIG TCP with partial GSO Alice Mikityanska
2026-07-16 14:47     ` Simon Horman
2026-07-16 16:28       ` Alice Mikityanska
2026-07-10 13:42   ` [PATCH net-next v9 3/9] udp: Support BIG TCP GSO packets where they can occur Alice Mikityanska
2026-07-10 13:42   ` [PATCH net-next v9 4/9] udp: Support gro_ipv4_max_size > 65536 Alice Mikityanska
2026-07-16 14:47     ` Simon Horman
2026-07-16 17:04       ` Alice Mikityanska
2026-07-10 13:42   ` [PATCH net-next v9 5/9] udp: Validate UDP length in udp_gro_receive Alice Mikityanska
2026-07-16 14:48     ` Simon Horman
2026-07-16 18:32       ` Alice Mikityanska
2026-07-10 13:42   ` [PATCH net-next v9 6/9] udp: Set length in UDP header to 0 for big GSO packets Alice Mikityanska
2026-07-10 13:42   ` [PATCH net-next v9 7/9] vxlan: Enable BIG TCP packets Alice Mikityanska
2026-07-10 13:42   ` [PATCH net-next v9 8/9] geneve: " Alice Mikityanska
2026-07-16 14:48     ` Simon Horman
2026-07-16 18:45       ` Alice Mikityanska
2026-07-10 13:42   ` [PATCH net-next v9 9/9] selftests: net: Add a test for BIG TCP in UDP tunnels Alice Mikityanska
2026-07-11  9:47   ` [PATCH net-next v9 0/9] BIG TCP for " Nikolay Aleksandrov
2026-07-15  6:11     ` zebang.li
     [not found]     ` <(raw)>
2026-07-15  9:06       ` Alice Mikityanska

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260717124046.GO95246@horms.kernel.org \
    --to=horms@kernel.org \
    --cc=alice.kernel@fastmail.im \
    --cc=alice@isovalent.com \
    --cc=andrew+netdev@lunn.ch \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=dsahern@kernel.org \
    --cc=edumazet@google.com \
    --cc=fw@strlen.de \
    --cc=kuba@kernel.org \
    --cc=lucien.xin@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=razor@blackwall.org \
    --cc=shuah@kernel.org \
    --cc=stfomichev@gmail.com \
    --cc=willemb@google.com \
    --cc=willemdebruijn.kernel@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox