Re: [PATCH net-next v4 10/10] net: ntb_netdev: Preserve CHECKSUM_PARTIAL across NTB

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

Critical: 0 · High: 0 · Medium: 1 · Low: 3

- [Medium] When TX checksum offload is negotiated (skb->ip_summed ==
  CHECKSUM_PARTIAL and READ_ONCE(q->peer_csum) true),…
- [Low] Growing `struct ntb_netdev_hdr` from 4 to 8 bytes silently
  shrinks the usable MTU by 4 bytes for every user, including users who…
- [Low] ntb_netdev advertises NETIF_F_HW_CSUM in `ndev->hw_features` and
  installs an `ndo_features_check` that calls `vlan_features_check()`,…
- [Low] ntb_netdev_close() flips NETIF_F_RXCSUM (via
  netdev_update_features()) while the NTB transport RX callback…

--

net: ntb_netdev: Preserve CHECKSUM_PARTIAL across NTB

This adds an optional ntb_netdev header that carries csum_start and
csum_offset so CHECKSUM_PARTIAL survives the NTB hop, with receive support
exchanged at QP link-up and a software fallback when the peer does not
advertise it.  Both the TX and RX checksum features stay off by default and
NETIF_F_RXCSUM is frozen while the interface is up.

A few questions below.

[ ... ]

[Severity: Low]
Does growing struct ntb_netdev_hdr from 4 to 8 bytes shrink the usable MTU
for everyone, including users who never enable the new opt-in features?

Both the default MTU and the MTU ceiling are derived from sizeof(struct
ntb_netdev_hdr).  In ntb_netdev_probe():

	ndev->mtu = ntb_transport_max_size(dev->queues[0].qp) - ETH_HLEN -
		    sizeof(struct ntb_netdev_hdr);

and in ntb_netdev_change_mtu():

	size = new_mtu + ETH_HLEN + sizeof(struct ntb_netdev_hdr);
	if (size > ntb_transport_max_size(dev->queues[0].qp))
		return -EINVAL;

With the default transport_mtu of 0x10000, max_size is 65524, so the
default and maximum MTU move from 65506 to 65502.  Would an existing
"ip link set ntb0 mtu 65506" now fail with -EINVAL?  The RX buffer size and
ndev->needed_headroom also grow by 4 bytes unconditionally.

Could the commit message mention this, given it describes the change as
opt-in?

[ ... ]

[Severity: Medium]
Should the header only be prepended when skb->len + sizeof(hdr) still fits
the transport frame?

The MTU accounting reserves exactly 8 bytes on top of MTU + ETH_HLEN, so the
on-wire budget equals ntb_transport_max_size() and the default MTU already
sits at that maximum.  If skb->len is larger than ndev->mtu + ETH_HLEN, the
enqueue is rejected in ntb_transport_tx_enqueue():

	if (len > qp->tx_max_frame - sizeof(struct ntb_payload_header))
		return -EMSGSIZE;

-EMSGSIZE is neither -EAGAIN nor -EBUSY, so it lands in the drop path:

drop:
	dev_kfree_skb_any(skb);
	DEV_STATS_INC(ndev, tx_dropped);
	return NETDEV_TX_OK;

Frames slightly over MTU + ETH_HLEN do reach ndo_start_xmit with
CHECKSUM_PARTIAL: __is_skb_forwardable() allows dev->mtu +
dev->hard_header_len + VLAN_HLEN for bridged traffic, and packet_snd()
allows len <= dev->mtu + reserve + VLAN_HLEN while virtio_net_hdr_to_skb()
marks the skb CHECKSUM_PARTIAL.  ntb_netdev_features_check() only clears
NETIF_F_CSUM_MASK for csum_start < ETH_HLEN, so those frames keep
CHECKSUM_PARTIAL and take the header path.

Before this patch such a frame was transmitted with a software checksum;
now it is freed and counted as tx_dropped.  Would falling back to
skb_checksum_help() when the header does not fit preserve the old
behaviour?

[Severity: Low]
Can vlan_features_check() do anything useful here while ndev->vlan_features
stays at 0?

netif_skb_features() has already masked the features for any tagged skb
before ndo_features_check runs:

net/core/dev.c:netif_skb_features() {
	...
	if (skb_vlan_tagged(skb))
		features = netdev_intersect_features(features,
						     dev->vlan_features |
						     NETIF_F_HW_VLAN_CTAG_TX |
						     NETIF_F_HW_VLAN_STAG_TX);
	...
}

ntb_netdev_probe() only sets ndev->features and ndev->hw_features, so
NETIF_F_HW_CSUM is not in vlan_features and cannot be intersected back in.
vlan_dev_fix_features() derives the VLAN upper device features from
real_dev->vlan_features as well.

Does that mean VLAN traffic (on a VLAN upper device or as tagged frames on
ntb0) always takes the skb_checksum_help() path and never uses the new
header?  Should NETIF_F_HW_CSUM be added to ndev->vlan_features?

[ ... ]

[Severity: Low]
Is ndev->features stable here with respect to the transport RX callback?

netdev_update_features() mutates NETIF_F_RXCSUM while
ntb_netdev_rx_handler() may be reading the same flag from the tasklet:

		if (flags & NTB_NETDEV_HDR_F_CSUM) {
			if (!(ndev->features & NETIF_F_RXCSUM)) {

Nothing in the close path appears to quiesce that tasklet.
ntb_transport_link_down() only clears client_ready, withdraws the QP
readiness bit and sends the link-down descriptor:

drivers/ntb/ntb_transport.c:ntb_transport_link_down() {
	...
	qp->client_ready = false;
	...
	ntb_qp_up_request(qp, false);

	if (qp->link_is_up)
		ntb_send_link_down(qp);
}

tasklet_kill(&qp->rxc_db_work) only appears in ntb_transport_free_queue(),
and the doorbell is not masked, so ntb_transport_rxc_db() ->
ntb_process_rxc() -> qp->rx_handler can still run while
ntb_netdev_close() flips the feature bit.  Frames in flight would then
either get an extra rx_dropped or be pushed to the stack with
CHECKSUM_PARTIAL after RXCSUM was cleared.

Would using the per-QP capability negotiated with the peer instead of the
device-global ndev->features in the RX path avoid this?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914084838.2158249-1-den%40valinux.co.jp