ndo_features_check() can see a transport_header that does not point at
the L4 header, while ndo_start_xmit() for the same skb sees the right
one.
Commit 663ead3bb8d5 ("[NET]: Use csum_start offset instead of
skb_transport_header") made csum_start authoritative because receive
processing overwrites transport_header, and resets it from csum_start
"just before we push a packet with partial checksums off into the device
driver". That reset, which now sets inner_transport_header instead for
encapsulated skbs, sits in validate_xmit_skb(), on the non-GSO path
only, after netif_skb_features() has called ndo_features_check().
So the hook sees a wrong transport_header for a CHECKSUM_PARTIAL skb
(after GRO, or from a container or VM) when no stacked device (VLAN,
bond, tunnel) ran validate_xmit_skb() first and something earlier
pointed the header elsewhere:
* IPv6 forwarding with a destination options or routing header:
ip6_rcv_core() sets it just past the fixed header, and only hop-by-hop
options are stepped over before forwarding.
* br_nf_pre_routing_ipv6() does the same for bridged IPv6.
* seg6 inline, rpl and ioam6 point it at the header they insert.
A hook that bounds header lengths with transport_header can then leave
offloads enabled that the hardware cannot handle. For example,
i40e_features_check() limits IPLEN using skb_network_header_len(),
which returns 40, while i40e_tx_enable_csum() writes the real length
into the 7-bit IPLEN field, so more than 468 bytes of extension headers
overflow into L4_FC_LEN. Rather than make every driver measure from
csum_start, give the hook the headers the transmit path gets.
Move the reset above netif_skb_features(). Skbs that skip
skb_gso_segment() reach ndo_start_xmit() with the same values as
before, since VLAN insertion, the TLS device fallback (whose tcp_hdr()
now sees the reset header) and linearization keep csum_start and the
header offsets in step. Software segmentation parses the headers
itself, except for SCTP over UDP, where inner_transport_header already
equals csum_start. Encapsulated SKB_GSO_DODGY skbs that the device
segments now reach the driver with inner_transport_header at csum_start,
which TCP or UDP checked, rather than a value bpf_skb_adjust_room() or
bpf_lwt_push_ip_encap() may have copied from a stale transport_header.
Fixes: 5f35227ea34b ("net: Generalize ndo_gso_check to ndo_features_check")
Cc: <redacted>
Assisted-by: LLM
Signed-off-by: Matt Vollrath <redacted>
---
If this is considered too expensive, I'll make a documentation patch
instead and the drivers can fix the holes as they please. Either way,
the core-driver contract needs improvement.
Drivers exposed, by reading the code (none tested on hardware):
Every stale source above involves IPv6 extension headers. Since commit
5f0764cb1c81 ("net: extend IPv6 exthdr detection of tunneled packets"),
the core keeps IPV6_CSUM, TSO6 and GSO_UDP_L4 off such skbs by reading
nexthdr, so on net only HW_CSUM devices are exposed:
- i40e: as described above.
- ice in GCS mode (E830): the same IPLEN check, and ice_tx_csum() also
writes the real length unmasked into the 7-bit IPLEN field. GCS mode
has no TSO.
- gve DQO: gve_can_send_tso() counts buffers per segment from
skb_tcp_all_headers(), which has to match the header length that
gve_prep_tso() programs from the reset header. Device effect unknown.
- hns3 with HW Tx checksum: removes offloads when the transport offset
plus 60 exceeds its 480-byte header limit, which a stale offset can
hide. Device effect unknown.
Outside drivers, the TLS device fallback reads tcp_hdr()->seq between
the two points, for TLS offload traffic over an SRv6 inline, rpl or
ioam6 route that falls back to software.
Not exposed: drivers that measure from csum_start (igb, igc, ixgbe,
fbnic and others), and on net the IPV6_CSUM drivers (r8169, stmmac,
iavf and others).
For stable: i40e is HW_CSUM since v5.19, hns3's HW Tx checksum since
v5.11, gve's check since v6.8 and ice GCS since v6.15. 6.1.y (as of
6.1.191) has commit 864e3396976e ("net: gso: Forbid IPv6 TSO with
extensions on devices with only IPV6_CSUM") but not 5f0764cb1c81, and
that guard reads transport_header. A stale header lets a TSO6 skb with
extension headers past it, and validate_xmit_skb() then WARNs and drops
the skb on any IPV6_CSUM device with TSO6. This patch fixes that as
well, though a backport of 5f0764cb1c81 is the targeted fix.
In QEMU, a kprobe on ndo_features_check() found transport_header
differing from csum_start in 18425 of 18427 skbs of a routed IPv6 TCP
flow with destination options, and in all 23300 of a TCP flow over an
SRv6 inline route. With this patch, it found none.
This surfaced while working on a new ndo_features_check()
implementation [1].
[1] https://lore.kernel.org/all/20261028170239.28606-1-tactii@gmail.com/ (local)
---
net/core/dev.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
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))@@ -4239,16 +4248,9 @@ static struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device
* support checksumming for this protocol, complete
* checksumming here.
*/
- 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));
- if (skb_csum_hwoffload_help(skb, features))
- goto out_kfree_skb;
- }
+ if (skb->ip_summed == CHECKSUM_PARTIAL &&
+ skb_csum_hwoffload_help(skb, features))
+ goto out_kfree_skb;
}
skb = validate_xmit_xfrm(skb, features, again);
base-commit: 03897066069fd47caf2908183edf3b22b822ea73
--
2.43.0