Re: [PATCH net-next v2 3/6] eth: mpnic: TCP segmentation offload

["Daniel Zahka" <daniel.zahka@gmail.com>]

On Thu Nov 12, 2026 at 5:08 AM EST, netdev-bot+sashiko wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 2 · 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] At this commit, mpnic_xmit_frame_ring()
>   (drivers/net/ethernet/meta/mpnic/mpnic_txrx.c:306) calls…
> - [Medium] eth: mpnic: GSO validation bypass in mpnic_features_check()
>
> --
>
> eth: mpnic: TCP segmentation offload
>
> This adds hardware segmentation for non-encapsulated TCPv4/TCPv6 packets
> to the mpnic driver. It also programs the TOF mode and TCP flag clear
> registers, and reports hw_gso_wire_packets through the netdev qstats API.
>
>> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>> index a35ce8998409d..b78f8599bbe8f 100644
>> --- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>
> [ ... ]
>
>> @@ -312,6 +368,10 @@ netdev_features_t mpnic_features_check(struct sk_buff *skb,
>>  	    l2len + i3len + sizeof(struct tcphdr) + MAX_TCP_OPTION_SPACE > 256)
>>  		return features & ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);
>>  
>> +	if (skb_is_gso(skb) &&
>> +	    !FIELD_FIT(MPNIC_TWD_MSS, skb_shinfo(skb)->gso_size))
>> +		return features & ~NETIF_F_GSO_MASK;
>> +
>>  	return features;
>>  }
>
> [Severity: Medium]
> Does this MSS check cover every GSO skb that reaches mpnic_tx_lso()?
> Earlier in mpnic_features_check() there is an early return when
> skb->ip_summed != CHECKSUM_PARTIAL. For those skbs, neither the header
> geometry checks above nor the new gso_size check run, and NETIF_F_TSO
> and NETIF_F_TSO6 stay set.
>
> Those skbs can still end up offloaded. __virtio_net_hdr_to_skb()
> accepts VIRTIO_NET_HDR_GSO_TCPV4/TCPV6 without
> VIRTIO_NET_HDR_F_NEEDS_CSUM. So a tap or virtio guest, or a
> PACKET_VNET_HDR sender, can hand us a GSO skb with CHECKSUM_NONE and
> SKB_GSO_DODGY.
>
> The path from there looks like this:
>
> - validate_xmit_skb() sees netif_needs_gso() and calls
>   skb_gso_segment().
> - tcp4_gso_segment() or tcp6_gso_segment() seeds the pseudo-header
>   checksum and switches ip_summed to CHECKSUM_PARTIAL.
> - tcp_gso_segment() then returns NULL, because skb_gso_ok() is still
>   true for the advertised features.
>
> The original skb then goes to mpnic_xmit_frame() unsegmented, having
> skipped every check in this function.
>
> In mpnic_tx_lso(), shinfo->gso_size then goes into the 14-bit
> MPNIC_TWD_MSS field unchecked. gso_features_check() only bounds
> gso_segs and skb->len. A gso_size of 16384 with a total length under
> 64K therefore gets through, and FIELD_PREP() truncates it to an MSS
> of 0.
>
> The header lengths that mpnic_tx_offloads() writes into
> MPNIC_TWD_L2_HLEN and MPNIC_TWD_L3_IHLEN are also unvalidated for
> these skbs. The same goes for the even-length and 256 byte header
> limits. Here the transport header was set by the flow dissector, not
> by a trusted stack.
>
> What does the hardware do with an MSS of 0, or with truncated header
> lengths? At best it seems to put malformed segments on the wire.
>
> I see two possible fixes, and I'm not sure which you'd prefer:
>
> - Clear NETIF_F_GSO_MASK for GSO skbs that are not CHECKSUM_PARTIAL
>   yet.

This came up in a local sashiko run, but I declined it. The issue is
from vnet headers that request gso without csum. A handful of drivers
have an early return passing through the features if the skb is not
CHECKSUM_PARTIAL. Seems like something core might want to handle.

> - Move the gso_size check above the early return, and validate the
>   headers from skb_transport_header() in that case.
>
> Either way, this patch is the one that turns TSO on, so the fix
> probably belongs here.
>
> [Severity: Medium]
> Can a TCP GSO skb shorter than MPNIC_MIN_FRAME_LEN get to hardware LSO
> here? The only GSO fallback in this check is for a gso_size that doesn't
> fit MPNIC_TWD_MSS. Eth + IPv4 + TCP headers add up to 54 bytes, so a TCPv4
> GSO skb can be 56 bytes long with gso_size 1 and gso_segs 2.
>

This is the same feedback I addressed in the udp segmentation patch.
Shouldn't really be reachable for TSO skbs, but the check is there in
the next patch anyway.

> mpnic_xmit_frame_ring() pads every skb before the offloads are set up:
>
> 	if (skb_put_padto(skb, MPNIC_MIN_FRAME_LEN))
> 		goto err_drop;
>
> That puts the zero pad inside the buffer lengths that mpnic_tx_map()
> programs. Will the LSO engine then segment the pad as TCP payload? That
> would produce more wire segments than shinfo->gso_segs, carrying zero
> bytes at sequence numbers past the real data.
>
> The checksum seed also looks wrong for these skbs. mpnic_tx_lso() takes
> the length out of the pseudo-header using the padded skb->len:
>
> 	payload_len = cpu_to_be16(skb->len - skb_checksum_start_offset(skb));
> 	csum_replace_by_diff(&tcph->check, (__force __wsum)payload_len);
>
> The sender, or tcp4_gro_complete(), seeded tcph->check with the unpadded
> L4 length. So the seed ends up off by the pad size. Won't every segment
> the hardware generates then carry a bad TCP checksum?
>
> One way to reach this is a remote peer sending back-to-back non-PSH
> 1-byte segments. A forwarding host GRO-merges them and sends them out an
> mpnic port. tun/tap/virtio and PACKET_VNET_HDR sources can also build
> such skbs.
>
> Now that NETIF_F_TSO is advertised, these skbs reach the driver
> unsegmented. Before this patch GSO was never offloaded, so the padding
> did no harm.
>
> The next patch in the series, "eth: mpnic: UDP segmentation offload",
> adds skb->len < MPNIC_MIN_FRAME_LEN to this condition. Could that check
> move into this patch, so this commit doesn't send bad segments on its own?