Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] e1000e: missing skb_is_gso() and csum offset check in e1000_features_check() --
commit a74dc3b4f983f63146f4b296827445229df8d08e Author: Matt Vollrath <[email protected]> e1000e: fix Tx descriptor field wrapping This commit adds an ndo_features_check callback to e1000e to disable GSO and checksum offload when the header length or checksum offset exceeds U8_MAX, preventing 8-bit field wrapping in e1000_context_desc. > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c > b/drivers/net/ethernet/intel/e1000e/netdev.c > index 6f85dab5c55c5..11714032793ef 100644 > --- a/drivers/net/ethernet/intel/e1000e/netdev.c > +++ b/drivers/net/ethernet/intel/e1000e/netdev.c > @@ -7355,6 +7355,27 @@ static int e1000_set_features(struct net_device > *netdev, > return 1; > } > > +static netdev_features_t e1000_features_check(struct sk_buff *skb, > + struct net_device *netdev, > + netdev_features_t features) > +{ > + unsigned int hdr_len = 0; > + > + if (skb_is_gso_tcp(skb)) > + hdr_len = skb_tcp_all_headers(skb); > + else if (skb->ip_summed == CHECKSUM_PARTIAL) > + hdr_len = skb_checksum_start_offset(skb) + skb->csum_offset; [Severity: High] Should e1000_features_check() check skb_is_gso(skb) before calling skb_is_gso_tcp(skb), and also verify the checksum offset rather than making the two checks mutually exclusive? In include/linux/skbuff.h, skb_is_gso_tcp() only inspects gso_type and notes that the caller must check skb_is_gso(skb) first: /* Note: Should be called only if skb_is_gso(skb) is true */ static inline bool skb_is_gso_tcp(const struct sk_buff *skb) { return skb_shinfo(skb)->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6); } Because net/ipv4/tcp_output.c:__tcp_transmit_skb() unconditionally sets skb_shinfo(skb)->gso_type = sk->sk_gso_type on all outgoing TCP packets (including non-GSO packets where gso_size is 0), every non-GSO TCP packet will take the skb_is_gso_tcp(skb) branch and skip the CHECKSUM_PARTIAL branch: 1. For normal non-GSO TCP packets, e1000_tso() returns 0 and e1000_tx_csum() only writes css and css + csum_offset (16), but e1000_features_check() checks skb_tcp_all_headers(skb) and disables checksum offload early when the end of the TCP header exceeds U8_MAX even if tucso fits in u8. 2. If a non-GSO TCP skb is encapsulated in an outer header without setting skb->encapsulation (for example, L2TP over UDP in l2tp_xmit_core(), or an uncloned non-GSO TCP skb encrypted out-of-place by IPsec ESP in esp_output_head() / esp_output_tail()), skb->transport_header points to the outer 8-byte UDP or ESP header while gso_type still has SKB_GSO_TCPV4/V6 set. Calling skb_tcp_all_headers(skb) reads tcp_hdr(skb)->doff at offset 12 past the 8-byte header (which can read 4 bytes past skb->tail for ESP with a 0-length IV, or read a 0/1 nibble on UDP and allow css + csum_offset to wrap u8 tucso in e1000_tx_csum()). In addition, when __netif_skb_features() invokes ndo_features_check prior to GSO segmentation, skb_transport_header(skb) can be stale and differ from skb_checksum_start(skb), as noted in 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; For forwarded IPv6 packets with extension headers (via ip6_rcv_core()) or inline IPv6 extension header insertion (such as seg6_do_srh(), ioam6_do_inline(), or rpl_do_srh_inline()), skb->transport_header points to the first extension header immediately after struct ipv6hdr (offset 54), whereas skb->csum_start points to the actual TCP header after the extension headers (which may be past U8_MAX). In that case, skb_tcp_all_headers(skb) evaluates to at most 54 + 60 = 114 and skips the else-if branch, leaving NETIF_F_HW_CSUM and NETIF_F_TSO6 enabled. Later in validate_xmit_skb() or ipv6_gso_segment(), the transport header is updated to the real TCP header offset (> U8_MAX), allowing u8 css, tucso, or hdr_len to wrap in e1000_tx_csum() or e1000_tso(). Would it make sense to guard the TSO check with skb_is_gso(skb) and check skb->ip_summed == CHECKSUM_PARTIAL independently (or compute the TCP header length from skb_checksum_start(skb) when skb_is_gso(skb) is true)? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
