On 11/4/26 13:16, Eric Dumazet wrote:
Le mer. 4 nov. 2026 à 18:55, Matt Vollrath [off-list ref] a écrit :
quoted
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.
The change looks fine to me, but this is net-next material: the bug is
very old, there is no report from real hardware, and the patch changes
what every ndo_features_check() sees. Please drop the Cc: stable and the
Fixes: tag. For 6.1.y, send a backport of 5f0764cb1c81 to stable@
separately.
I'll be proposing a different series which fixes transport_header at the
source instead of patching one place where it matters that it's wrong. It
turns out there is more than one place.
I've sent the backport of 5f0764cb1c81 to stable.
quoted
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.
Only from validate_xmit_skb(). ip_finish_output_gso(), some qdiscs and
netpoll still call netif_skb_features() without the reset.
The changelog is also too long, something like this would be enough
for v2:
net: reset transport header before ndo_features_check()
For CHECKSUM_PARTIAL skbs, validate_xmit_skb() resets
transport_header (inner_transport_header for encapsulated skbs)
from csum_start, but only after netif_skb_features() has called
ndo_features_check().
The hook can therefore see a transport_header left elsewhere by
the receive path, while ndo_start_xmit() sees the reset one. For
example, ip6_rcv_core() leaves it after the fixed header of
forwarded IPv6 packets with destination options or a routing
header, and seg6, rpl and ioam6 point it at the header they insert.
i40e_features_check() then bounds IPLEN with a 40 byte
skb_network_header_len(), while i40e_tx_enable_csum() programs the
real length, which can overflow the 7-bit field.
Move the reset before netif_skb_features(). Skbs that are not
segmented in software reach ndo_start_xmit() with the same headers
as before, and software GSO parses the headers itself.
pw-bot: cr