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

[netdev-bot+sashiko@kernel.org]

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

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?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110-mpnic-lso-v2-0-ccd37b051d08%40gmail.com