Re: [PATCH net-next v2 4/6] eth: mpnic: UDP 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 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.
>

Local sashiko review surfaced the same complaint and wanted me to pull
the check forward one commit. I decided against it. The UDP case is
easily reachable because the user chooses the gso size. I don't think
producing a TSO skb shorter than 60B is really possible. The
virtio_net_hdr cases should have a min GSO size of TCP_MIN_GSO_SIZE set
in virtio_net_hdr_to_skb().

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