Re: [PATCH net-next v2 4/6] eth: mpnic: UDP segmentation offload

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · 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] This patch adds `skb->len < MPNIC_MIN_FRAME_LEN ||` to the
  generic `skb_is_gso(skb)` branch of mpnic_features_check()…

--

eth: mpnic: UDP segmentation offload

This commit adds NETIF_F_GSO_UDP_L4 support for non-encapsulated udp4
and udp6 packets. As for TCP, the UDP checksum is seeded with a zero
length. It also makes mpnic_features_check() fall back to software GSO
for skbs shorter than MPNIC_MIN_FRAME_LEN.

> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> index b78f8599bbe8f..2a0560b7dd19e 100644
> --- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c

[ ... ]

> @@ -369,7 +384,8 @@ netdev_features_t mpnic_features_check(struct sk_buff *skb,
>  		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))
> +	    (skb->len < MPNIC_MIN_FRAME_LEN ||
> +	     !FIELD_FIT(MPNIC_TWD_MSS, skb_shinfo(skb)->gso_size)))
>  		return features & ~NETIF_F_GSO_MASK;

[Severity: Medium]
This check applies to every GSO type. That means TCP GSO skbs shorter
than 60 bytes now fall back to software GSO too, not just UDP ones.

The commit message body only talks about UDP. The only mention of this
check is the v2 changelog below the "---", and that text is dropped when
the patch is applied:

  v2:
  - fall back to SW GSO for skbs shorter than MPNIC_MIN_FRAME_LEN, since
    HW LSO would segment the zero padding as payload

Should the commit message describe the effect on TCP? Or should the
check move into the earlier "eth: mpnic: TCP segmentation offload"
patch?

With only the TSO patch applied, TCP seems to have the same problem. In
that version, mpnic_features_check() rejected GSO only when gso_size
didn't fit MPNIC_TWD_MSS. So a short TCP GSO skb reaches
mpnic_xmit_frame_ring() with HW TSO enabled, and gets padded first:

drivers/net/ethernet/meta/mpnic/mpnic_txrx.c:mpnic_xmit_frame_ring() {
    ...
	if (skb_put_padto(skb, MPNIC_MIN_FRAME_LEN))
		goto err_drop;
    ...
}

Then mpnic_tx_lso() seeds the TCP pseudo-checksum from the padded
length:

	payload_len = cpu_to_be16(skb->len - skb_checksum_start_offset(skb));
	csum_replace_by_diff(&tcph->check, (__force __wsum)payload_len);

Locally generated TCP GSO skbs are always longer than 60 bytes. A GSO
skb built from a virtio_net_hdr can be shorter, though. Sources include
tun/vhost and packet sockets with a vnet header. Such an skb can carry
a tiny gso_size and only a few bytes of payload.

In that case, wouldn't the receiver get the zero padding as TCP stream
data, with a valid checksum?

The check is still present at the end of the series, so the final tree
is fine. The TSO commit on its own would still corrupt data if someone
bisects to it or backports it alone.

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