Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 · Low: 0
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).
- [Medium] The patch moves the csum_start-based transport_header reset in
validate_xmit_skb() (net/core/dev.c) above netif_skb_features(), so it…
--
net: reset transport_header for ndo_features_check
This patch moves the csum_start based transport_header (or
inner_transport_header) reset in validate_xmit_skb() so it runs before
netif_skb_features(). ndo_features_check() then sees the same L4 header
offset that ndo_start_xmit() sees. The motivation is drivers such as i40e,
whose features_check hook bounds header lengths using a transport_header
that can be stale after IPv6 forwarding with extension headers, bridged
IPv6, or seg6/rpl/ioam6.
quoted hunk ↗ jump to hunk
diff --git a/net/core/dev.c b/net/core/dev.c
index df1a840b03343..155be1f51494c 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4207,6 +4207,15 @@ static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device
if (unlikely(!skb))
goto out_null;
+ if (skb->ip_summed == CHECKSUM_PARTIAL) {
+ if (skb->encapsulation)
+ skb_set_inner_transport_header(skb,
+ skb_checksum_start_offset(skb));
+ else
+ skb_set_transport_header(skb,
+ skb_checksum_start_offset(skb));
+ }
+
features = netif_skb_features(skb);
skb = validate_xmit_vlan(skb, features);
if (unlikely(!skb))
[Severity: Medium]
The reset now runs before the netif_needs_gso() branch, so the GSO path is
affected too. Is the commit message's statement that "Software segmentation
parses the headers itself" accurate for that path?
On the GSO branch, validate_xmit_skb() calls skb_gso_output_max_segs()
before __skb_gso_segment() when gso_within_dev_limits() fails:
if (netif_needs_gso(skb, features)) {
...
if (unlikely(!gso_within_dev_limits(skb, dev)))
max_segs = skb_gso_output_max_segs(skb, dev);
skb_gso_output_max_segs() reads transport_header, and it has a guard
written for stale headers:
net/core/dev.c:skb_gso_output_max_segs() {
...
/*
* The transport offset can be unset or stale, so hdr_len is only taken
* from a header at the checksum start.
*/
if (!skb_transport_header_was_set(skb) ||
skb_checksum_start(skb) != skb_transport_header(skb))
return 0;
...
}
Commit 32bfd220cd6f ("net: core: re-segment oversized TCP GSO skbs") added
this guard. Its message says that an skb whose transport header is unset or
stale keeps today's segmentation.
After this patch, every non-encapsulated CHECKSUM_PARTIAL skb that reaches
this point has transport_header == csum_start. The guard can no longer fire
for those skbs on this path.
That changes behaviour for over-limit TCP GSO skbs whose header used to be
stale. Examples are forwarded GRO or BIG TCP IPv6 skbs with destination
options or a routing header, and seg6 inline, rpl or ioam6 skbs.
Before this patch they were fully segmented to MSS (max_segs = 0). Now they
are re-segmented into smaller GSO skbs. hdr_len is computed from the reset
header, and __netif_skb_features() is evaluated again.
The new behaviour looks correct, since the header is no longer stale. Could
the commit message mention this change?
Should the comment and the now redundant check in skb_gso_output_max_segs()
also be updated for this caller?
32bfd220cd6f appears to be in mainline only, so this should not affect the
stable backport.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104175517.2395704-1-tactii%40gmail.com