Re: [PATCH net-next 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] With this patch, mpnic_netdev_alloc() advertises
  NETIF_F_GSO_UDP_L4.

--

eth: mpnic: UDP segmentation offload

This commit adds UDP segmentation offload for non-encapsulated udp4/udp6
packets by advertising NETIF_F_GSO_UDP_L4. For GSO skbs, mpnic_tx_lso()
now seeds the UDP checksum with zero length, the same way it already does
for TCP.

> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
> index 3a4aa5be57a19..83be45031a30b 100644
> --- a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
> @@ -331,7 +331,8 @@ struct net_device *mpnic_netdev_alloc(struct mpnic_dev *mpd)
>  		NETIF_F_RXCSUM |
>  		NETIF_F_TSO |
>  		NETIF_F_TSO_ECN |
> -		NETIF_F_TSO6;
> +		NETIF_F_TSO6 |
> +		NETIF_F_GSO_UDP_L4;

[Severity: Medium]
Now that NETIF_F_GSO_UDP_L4 is advertised, can UDP GSO skbs shorter than
MPNIC_MIN_FRAME_LEN reach the hardware with the zero padding counted as
payload?

An unprivileged process can set UDP_SEGMENT with a small gso_size and
send a short datagram. For example, IPv4 with gso_size=1 and 2 bytes of
data makes udp_send_skb() build a GSO skb with gso_segs=2 and a 44 byte
frame (14 + 20 + 8 + 2).

mpnic_xmit_frame_ring() pads every skb, GSO or not, before the offload
setup runs:

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

The frame grows to 60 bytes. Then mpnic_tx_offloads()->mpnic_tx_lso() sets
MPNIC_TWD_FLAG_REQ_LSO with MSS=1 over a buffer that holds 18 bytes after
the 42 byte header, not 2.

If the segmentation engine works from the mapped length, would this send
18 one-byte datagrams instead of 2? 16 of them would be zero-filled and
delivered to the peer application.

That depends on whether the LSO engine uses the buffer length or the
IP/UDP length fields, and the driver source doesn't show which.

Even if the hardware ignores the padding, the accounting is still off.
MPNIC_XMIT_CB(skb)->bytecount, MPNIC_XMIT_CB(skb)->gso_segs and the
hw_gso_wire stat are all computed from shinfo->gso_segs. They won't match
the padded buffer the hardware is asked to segment.

Nothing appears to steer these skbs to software GSO. mpnic_features_check()
only checks an upper bound on gso_size:

drivers/net/ethernet/meta/mpnic/mpnic_txrx.c:mpnic_features_check() {
    ...
	if (skb_is_gso(skb) &&
	    !FIELD_FIT(MPNIC_TWD_MSS, skb_shinfo(skb)->gso_size))
		return features & ~NETIF_F_GSO_MASK;
    ...
}

In net/core/dev.c, gso_features_check() and gso_within_dev_limits() only
check gso_max_segs and gso_max_size.

Before this patch only TCP GSO reached the driver, and locally generated
TCP GSO frames are well over 60 bytes. The later patch in the series,
"eth: mpnic: add GSO_PARTIAL for other tunnels", still pads every skb and
doesn't add a minimum length check.

Would it make sense to skip the padding for GSO skbs? Another option is to
clear NETIF_F_GSO_MASK in mpnic_features_check() when skb->len is below
MPNIC_MIN_FRAME_LEN.

[ ... ]

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