[PATCH net] net: reset transport_header for ndo_features_check

[PATCH net] net: reset transport_header for ndo_features_check

[Matt Vollrath]

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: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Matt Vollrath <tactii@gmail.com>
---
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/
---
 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

Re: [PATCH net] net: reset transport_header for ndo_features_check

[Eric Dumazet]

Le mer. 4 nov. 2026 à 18:55, Matt Vollrath <tactii@gmail.com> a écrit :
>
> 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.


> 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

Re: [PATCH net] net: reset transport_header for ndo_features_check

[Matt Vollrath]

On 11/4/26 13:16, Eric Dumazet wrote:
> Le mer. 4 nov. 2026 à 18:55, Matt Vollrath <tactii@gmail.com> a écrit :
>>
>> 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.

>> 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