Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] Possible truncation of the 16-bit IP length for grouped outputs
larger than 64 KiB (fixed by the next patch in the series).
--
net: gso: support re-segmentation of TCP GSO skbs
This commit adds a max_segs argument to __skb_gso_segment() and stores it
in the skb_gso_cb scratch area. When max_segs is non-zero, skb_segment()
skips the partial/frag_list decision block and groups up to max_segs MSS
segments into each output GSO skb. Existing callers pass zero to keep
their current behaviour.
> diff --git a/net/core/gso.c b/net/core/gso.c
> index bcd156372f4df..21a259ca392f4 100644
> --- a/net/core/gso.c
> +++ b/net/core/gso.c
[ ... ]
> @@ -117,6 +120,7 @@ struct sk_buff *__skb_gso_segment(struct sk_buff *skb,
>
> SKB_GSO_CB(skb)->mac_offset = skb_headroom(skb);
> SKB_GSO_CB(skb)->encap_level = 0;
> + SKB_GSO_CB(skb)->max_segs = min(max_segs, GSO_MAX_SEGS);
>
> skb_reset_mac_header(skb);
> skb_reset_mac_len(skb);
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 8912a66cd9097..fd10a7cdc1883 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
[ ... ]
> @@ -4874,7 +4875,10 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
> * now.
> */
> DEBUG_NET_WARN_ON_ONCE(len / mss > GSO_MAX_SEGS);
> - partial_segs = min(len / mss, GSO_MAX_SEGS);
> + if (max_segs)
> + partial_segs = min(len / mss, max_segs);
> + else
> + partial_segs = min(len / mss, GSO_MAX_SEGS);
> if (partial_segs > 1)
> mss *= partial_segs;
> else
[Severity: Low]
When max_segs is non-zero, the grouped outputs are not SKB_GSO_PARTIAL
skbs. Their size is bounded only by max_segs * gso_size. The only clamp in
__skb_gso_segment() is on the segment count (GSO_MAX_SEGS), and nothing
limits the number of bytes.
For non-partial GSO outputs, the L3 length is taken from the whole output
skb and stored in a 16-bit field:
net/ipv4/af_inet.c:inet_gso_segment() {
...
else
tot_len = skb->len - nhoff;
...
iph->tot_len = htons(tot_len);
ip_send_check(iph);
...
}
net/ipv6/ip6_offload.c:ipv6_gso_segment() {
...
else
payload_len = skb->len - nhoff - sizeof(*ipv6h);
ipv6h->payload_len = htons(payload_len);
...
}
If a caller passed a max_segs where max_segs * gso_size plus headers is
more than 65535, would tot_len and payload_len be truncated, and would the
IPv4 header checksum then be computed over the wrong length?
No caller in this patch passes a non-zero value. The only non-zero caller
in the series is skb_gso_output_max_segs(), added by the next patch ("net:
core: re-segment oversized TCP GSO skbs"). It caps gso_max_size at
GSO_LEGACY_MAX_SIZE before it computes max_segs, so this cannot be hit
anywhere in the series. What is left is the API contract of
__skb_gso_segment().
Should the kernel-doc for max_segs say that the caller must keep
max_segs * gso_size plus headers within 64KB? Alternatively, could
__skb_gso_segment() or skb_segment() enforce a byte limit, so a future
caller cannot produce outputs with a truncated IP length?
--
Sashiko AI review ยท
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930111526.2183107-1-wang.zhan%40smartx.com
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
