Re: [PATCH net-next v7 04/11] net: Enable BIG TCP with partial GSO

[Paolo Abeni <pabeni@redhat.com>]

On 6/11/26 9:29 PM, Alice Mikityanska wrote:
> From: Alice Mikityanska <alice@isovalent.com>
> 
> skb_segment is called for partial GSO, when netif_needs_gso returns true
> in validate_xmit_skb. Partial GSO is needed, for example, when
> segmentation of tunneled traffic is offloaded to a NIC that only
> supports inner checksum offload.
> 
> Currently, skb_segment clamps the segment length to 65534 bytes, because
> gso_size == 65535 is a special value GSO_BY_FRAGS, and we don't want
> to accidentally assign mss = 65535, as it would fall into the
> GSO_BY_FRAGS check further in the function.
> 
> This implementation, however, artificially blocks len > 65534, which is
> possible since the introduction of BIG TCP. To allow bigger lengths and
> avoid resegmentation of BIG TCP packets, store the gso_by_frags flag in
> the beginning and don't use a special value of mss for this purpose
> after mss was modified.
> 
> Signed-off-by: Alice Mikityanska <alice@isovalent.com>
> Reviewed-by: Willem de Bruijn <willemb@google.com>
> ---
>  drivers/net/netdevsim/psp.c |  2 +-
>  net/core/skbuff.c           | 10 +++++-----
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/netdevsim/psp.c b/drivers/net/netdevsim/psp.c
> index d3e36c74be62..6b3532b5e360 100644
> --- a/drivers/net/netdevsim/psp.c
> +++ b/drivers/net/netdevsim/psp.c
> @@ -92,7 +92,7 @@ nsim_do_psp(struct sk_buff *skb, struct netdevsim *ns,
>  		 * provide a valid checksum here, so the skb isn't dropped.
>  		 */
>  		uh = udp_hdr(skb);
> -		udplen = ntohs(uh->len) ?: skb->len - skb_transport_offset(skb);
> +		udplen = udp_get_len(skb, uh, skb_transport_offset(skb));
>  		csum = skb_checksum(skb, skb_transport_offset(skb),
>  				    udplen, 0);
>  
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index c64693fcb2d1..5dcee79df8cf 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4773,6 +4773,7 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
>  	struct sk_buff *tail = NULL;
>  	struct sk_buff *list_skb = skb_shinfo(head_skb)->frag_list;
>  	unsigned int mss = skb_shinfo(head_skb)->gso_size;
> +	bool gso_by_frags = mss == GSO_BY_FRAGS;
>  	unsigned int doffset = head_skb->data - skb_mac_header(head_skb);
>  	unsigned int offset = doffset;
>  	unsigned int tnl_hlen = skb_tnl_header_len(head_skb);
> @@ -4788,7 +4789,7 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
>  	int nfrags, pos;
>  
>  	if ((skb_shinfo(head_skb)->gso_type & SKB_GSO_DODGY) &&
> -	    mss != GSO_BY_FRAGS && mss != skb_headlen(head_skb)) {
> +	    !gso_by_frags && mss != skb_headlen(head_skb)) {
>  		struct sk_buff *check_skb;
>  
>  		for (check_skb = list_skb; check_skb; check_skb = check_skb->next) {
> @@ -4816,7 +4817,7 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
>  	sg = !!(features & NETIF_F_SG);
>  	csum = !!can_checksum_protocol(features, proto);
>  
> -	if (sg && csum && (mss != GSO_BY_FRAGS))  {
> +	if (sg && csum && !gso_by_frags)  {
>  		if (!(features & NETIF_F_GSO_PARTIAL)) {
>  			struct sk_buff *iter;
>  			unsigned int frag_len;
> @@ -4850,9 +4851,8 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
>  		/* GSO partial only requires that we trim off any excess that
>  		 * doesn't fit into an MSS sized block, so take care of that
>  		 * now.
> -		 * Cap len to not accidentally hit GSO_BY_FRAGS.
>  		 */
> -		partial_segs = min(len, GSO_BY_FRAGS - 1) / mss;
> +		partial_segs = len / mss;

Sashiko/gemini says the above can lead to hit BUG_ON() later.

I *think* it's not a false positive, as it looks like skb_segment()
assumes an skb can hold `mss` bytes without resorting to frag_list
usage, and mss > MAX_SKB_FRAGS * PAGE_SIZE  breaks such assumption.

I think handling correctly this case will requires some non trivial
surgery to skb_segment: both `while (pos < offset + len) {` loops must
be updated to feed data from `frags` as needed instead of
BUG_ON()/net_warn_ratelimited(skb_shinfo(nskb)->nr_frags >= MAX_SKB_FRAGS);

/P