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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 2

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] mpnic_tx_lso() writes skb_shinfo(skb)->gso_size straight into
  the 14-bit MPNIC_TWD_MSS descriptor field with MPNIC_TWD_FLAG_REQ_LSO…
- [Medium] mpnic_xmit_frame_ring() pads every frame to 60 bytes in
  software (`skb_put_padto(skb, MPNIC_MIN_FRAME_LEN)`, line 307).
- [Low] The two places that feed the per-ring `twq.dropped` counter now
  count in different units.
- [Low] mpnic_tx_lso() bumps the new qstats counters `twq.hw_gso` and
  `twq.hw_gso_wire` (mpnic_txrx.c lines 259-260) before the frame is…

--

eth: mpnic: TCP segmentation offload

This adds hardware segmentation for non-encapsulated TCPv4/TCPv6 packets.
The TCP checksum is seeded with zero length, and the TOF block is set up
for incrementing IPv4 IDs and for clearing TCP flags per segment. GSO super
and wire packets are counted and reported 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 680625810ec03..e2e6ab6d3c591 100644
> --- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c

[ ... ]

> @@ -215,30 +218,82 @@ mpnic_tx_map(struct mpnic_ring *ring, struct sk_buff *skb, __le64 *meta)

[ ... ]

> +static bool
> +mpnic_tx_lso(struct mpnic_ring *ring, struct sk_buff *skb, __le64 *meta)
> +{

[ ... ]

> +	*meta |= cpu_to_le64(FIELD_PREP(MPNIC_TWD_L4_HLEN, l4len / 4) |
> +			     FIELD_PREP(MPNIC_TWD_MSS, shinfo->gso_size) |
> +			     MPNIC_TWD_FLAG_REQ_LSO);

[Severity: Medium]
Can the device send runt frames for the last segment here?

mpnic_xmit_frame_ring() pads every frame in software:

	if (skb_put_padto(skb, MPNIC_MIN_FRAME_LEN))
		goto err_drop;

That suggests the TX path needs at least 60 bytes from the driver. For a
TSO skb the padding does nothing, because the super-packet is always longer
than 60 bytes, and the device builds each segment itself.

The last segment carries (skb->len - hdrlen) % gso_size bytes of payload.
For IPv4 TCP without options (54 bytes of headers) and a remainder of 1 to
5 bytes, that is a 55 to 59 byte frame. A TCP_NODELAY sender with
timestamps disabled can produce this, for example 1449 bytes at MSS 1448.

Neither mpnic_features_check() nor mpnic_tx_lso() guards against this.
mpnic_txglb_init() also doesn't configure any MAC or TOF auto-pad.

Does the segmentation engine pad the segments it generates to the Ethernet
minimum? If it doesn't, the link partner would drop those tail segments.
This also looks unchanged at the end of the series.

> +
> +	hdrlen = skb_checksum_start_offset(skb) + l4len;
> +	MPNIC_XMIT_CB(skb)->bytecount += (shinfo->gso_segs - 1) * hdrlen;
> +	MPNIC_XMIT_CB(skb)->gso_segs = shinfo->gso_segs;
> +
> +	u64_stats_update_begin(&ring->stats.syncp);
> +	u64_stats_inc(&ring->stats.twq.hw_gso);
> +	u64_stats_add(&ring->stats.twq.hw_gso_wire, shinfo->gso_segs);
> +	u64_stats_update_end(&ring->stats.syncp);

[Severity: Low]
Should hw_gso and hw_gso_wire be incremented only after the frame has
actually been handed to the device?

mpnic_tx_lso() is called from mpnic_tx_offloads(), which runs before
mpnic_tx_map() in mpnic_xmit_frame_ring():

	if (mpnic_tx_offloads(ring, skb, meta))
		goto err_free;

	if (mpnic_tx_map(ring, skb, meta))
		goto err_free;

Say mpnic_tx_map() then fails, either on a DMA mapping error or on a
fragment larger than MPNIC_TWD_LEN. The skb is freed and counted as
dropped, but hw_gso and hw_gso_wire are not rolled back. The same happens
to queued TSO skbs that mpnic_clean_twq0() discards with discard set.

mpnic_get_queue_stats_tx() and mpnic_get_base_stats() export these as
tx-hw-gso-packets and tx-hw-gso-wire-packets. They also add hw_gso_wire
into tx-needs-csum.

netdev.yaml defines these counters as packets the device segmented and the
wire packets it generated. On these paths the reported values include
super-packets the device never segmented. The existing csum_partial
counter has the same count-at-enqueue pattern.

[ ... ]

> @@ -312,6 +369,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;

[Severity: Medium]
Is the 14-bit field width the only limit the hardware puts on gso_size?

This check only rejects gso_size > 16383. There is no minimum MSS, and
nothing compares gso_size plus the header length against the largest frame
the device supports (MPNIC_MAX_JUMBO_FRAME_SIZE, 9742).

gso_size can come from an untrusted sender:

- virtio_net_hdr_to_skb() is used by tun, tap, macvtap and packet sockets
  with vnet headers. It only raises a small TCP gso_size to
  TCP_MIN_GSO_SIZE, which is 8, and marks the skb SKB_GSO_DODGY.
- mpnic doesn't advertise NETIF_F_GSO_ROBUST, so tcp_gso_segment() only
  recomputes gso_segs and passes the original skb to the driver.
- On bridged paths, is_skb_forwardable() skips the MTU check for GSO skbs.

So a guest or tap user could make mpnic_tx_lso() program MPNIC_TWD_MSS in
two problem ranges:

- 8, which means thousands of segments per 64KB descriptor.
- Roughly 9.7K to 16383, which asks for frames larger than the device
  supports.

How does the device handle either case?

Drivers such as i40e and ice clear NETIF_F_GSO_MASK in ndo_features_check
when the MSS is too small. Would a similar lower bound make sense here,
along with an upper bound tied to the maximum frame size? This also looks
unchanged at the end of the series.

>  	return features;
>  }
>  
> @@ -357,7 +418,7 @@ static void mpnic_clean_twq0(struct mpnic_napi_vector *nv, int napi_budget,
>  		}
>  
>  		total_bytes += MPNIC_XMIT_CB(skb)->bytecount;
> -		total_packets++;
> +		total_packets += MPNIC_XMIT_CB(skb)->gso_segs;

[Severity: Low]
Does this leave twq.dropped counting in two different units?

When mpnic_clean_twq0() runs with discard set, total_packets now adds wire
segments to the drop counter:

	if (discard) {
		...
		u64_stats_add(&ring->stats.twq.dropped, total_packets);

The error path in mpnic_xmit_frame_ring() still counts one per skb:

err_drop:
	...
	u64_stats_inc(&ring->stats.twq.dropped);

TSO skbs also reach that path when the new skb_cow_head() in
mpnic_tx_lso() fails, or when mpnic_tx_map() fails.

Both counts reach rtnl_link_stats64.tx_dropped through mpnic_get_stats64()
and mpnic_aggregate_ring_twq_counters(). A dropped TSO super-packet
therefore counts as gso_segs or as 1, depending on where it was dropped.
Before this patch both sites counted skbs. This also looks unchanged at the
end of the series.

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