Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.
Critical: 0 · High: 1 · Medium: 0 · Low: 1
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [High] skb_gso_output_max_segs() (net/core/dev.c:3954-3960) works out
hdr_len from a TCP header read at skb->transport_header.
- [Low] Parts of the commit message's cost claims do not match the code.
--
net: core: resegment oversized TCP GSO skbs
When an unencapsulated TCP GSO skb exceeds the egress device's
gso_max_size or gso_max_segs, validate_xmit_skb() now resegments it into
smaller GSO skbs instead of segmenting it into individual packets. A new
helper, skb_gso_output_max_segs(), works out the per-output segment bound
from the header length, the MSS and the device limits. The size limit is
capped at GSO_LEGACY_MAX_SIZE.
> The helper runs on the skb which is handed to the driver, after
> validate_xmit_vlan() and sk_validate_xmit_skb(), and only from the
> netif_needs_gso() branch, so an skb which is not segmented pays nothing.
> An over-limit skb pays one device limit test and one ndo_features_check()
> for the bound, in exchange for keeping the output a GSO skb.
[Severity: Low]
Is this description of the cost accurate?
Take SKB_GSO_DODGY TCP skbs, for example from tap/virtio guests or packet
sockets, sent through a device without NETIF_F_GSO_ROBUST. They fail
skb_gso_ok() and take the netif_needs_gso() branch, so they now pay the
gso_within_dev_limits() test in validate_xmit_skb().
They are then not segmented, because tcp_gso_segment() returns early:
if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) {
/* Packet is from an untrusted source, reset gso_segs. */
skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len, mss);
segs = NULL;
goto out;
}
CHECKSUM_NONE GSO skbs on a TSO device go through the same path.
In the over-limit case, the helper calls __netif_skb_features(skb, false).
That reruns gso_features_check(), the VLAN feature intersection,
ndo_features_check() or dflt_features_check(), and harmonize_features().
The helper also does an extra skb_header_pointer() read for the TCP header.
Could the commit message describe this cost more precisely?
> diff --git a/net/core/dev.c b/net/core/dev.c
> index d66b667071837..728260772f349 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3933,6 +3933,63 @@ netdev_features_t netif_skb_features(struct sk_buff
> *skb)
> }
> EXPORT_SYMBOL(netif_skb_features);
>
> +static unsigned int
> +skb_gso_output_max_segs(struct sk_buff *skb, struct net_device *dev)
> +{
> + unsigned int mss = skb_shinfo(skb)->gso_size;
> + unsigned int gso_max_size, hdr_len, max_segs;
> + netdev_features_t features;
> + struct tcphdr _tcph, *th;
> +
> + /*
> + * The TCP frag-list path segments through skb_segment_list(), which
> + * does not carry max_segs, so bounded calls skip those skbs.
> + */
> + if (!skb_is_gso(skb) || !skb_is_gso_tcp(skb) ||
> + skb->encapsulation || skb_has_frag_list(skb) ||
> + !skb_mac_header_was_set(skb) ||
> + !skb_transport_header_was_set(skb))
> + return 0;
> +
> + th = skb_header_pointer(skb, skb_transport_offset(skb), sizeof(_tcph),
> + &_tcph);
> + if (!th || th->doff < sizeof(*th) / 4)
> + return 0;
> +
> + hdr_len = skb_transport_header(skb) - skb_mac_header(skb) +
> + th->doff * 4;
[Severity: High]
Can hdr_len end up computed from a header that isn't the TCP header?
skb_transport_header_was_set() only says that the offset was set. It does
not say that the offset points at the TCP header.
The segmentation code does not use this offset. inet_gso_segment() and
ipv6_gso_segment() re-parse L3 and reset the transport header themselves,
so skb_segment() builds the outputs from the real header length.
There seem to be two ways a forwarded TCP GSO skb can arrive here with a
stale transport offset.
The first is IPv6 forwarding with a Destination Options or Routing header.
ipv6_gro_receive() sets the transport header to the real TCP header.
ip6_rcv_core() then overwrites it:
skb->transport_header = skb->network_header + sizeof(*hdr);
Only a Hop-by-Hop header moves it past that point. So on the
ip6_forward()->validate_xmit_skb() path, this helper reads doff from
extension header bytes that the remote sender controls.
The second is VXLAN decap followed by bridge forwarding, with GRO off on
the vxlan device or an XDP prog attached. gro_cells_receive() does:
skb_unset_transport_header(skb);
if (!gcells->cells || skb_cloned(skb) || netif_elide_gro(dev)) {
res = netif_rx(skb);
Without CONFIG_DEBUG_NET, __netif_receive_skb_core() then resets the
transport header to skb->data, which is the inner IP header, and the
bridge leaves it there.
doff then comes from the high nibble of the inner IPv4 saddr. An address
in 80.x to 95.x gives doff = 5, so hdr_len is 34 instead of 54 or more.
In both cases hdr_len comes out too small, so max_segs comes out too large.
Take gso_ipv4_max_size = 32160, MSS 1460 and no TCP options. This helper
returns 22 instead of 21, and skb_segment() emits a 32174 byte GSO skb.
That is above the limit the device advertises.
In the BIG TCP to 64 KiB case, the GSO_LEGACY_MAX_SIZE cap does not bound
the real output either. A Destination Options header of about 48 bytes is
enough in the IPv6 case for the output to go past what the 16-bit length
field can hold. ipv6_gso_segment() would then truncate here:
payload_len = skb->len - nhoff - sizeof(*ipv6h);
ipv6h->payload_len = htons(payload_len);
The same applies to iph->tot_len in inet_gso_segment(). The commit message
says the output "obeys the GSO feature and limit contract the device
already advertises". Before this patch, these over-limit skbs were fully
segmented.
Would it be safer to find the TCP header the same way the GSO code does?
One option is to require CHECKSUM_PARTIAL and use
skb_checksum_start_offset(), since tcp_gso_segment() already requires
csum_start to match the TCP header. Another is to return 0 unless
skb_checksum_start(skb) == skb_transport_header(skb).
[ ... ]
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928044102.1004310-1-wang.zhan%40smartx.com
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
