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

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

> diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
> index 67cfe0f1a49a2..d889c8538be0c 100644
> --- a/drivers/net/ntb_netdev.c
> +++ b/drivers/net/ntb_netdev.c

[ ... ]

> @@ -29,12 +30,19 @@ static unsigned int tx_stop = 5;
>  #define NTB_NETDEV_MAX_QUEUES		64
>  #define NTB_NETDEV_DEFAULT_QUEUES	1
>  
> +#define NTB_NETDEV_CAP_CSUM		BIT(0)
> +
>  /* An ntb_netdev_hdr precedes the packet. */
>  #define NTB_NETDEV_META_HDR		BIT(0)
>  
> +#define NTB_NETDEV_HDR_F_CSUM		BIT(0)
> +
>  struct ntb_netdev_hdr {
>  	__le16 len;		/* Header length in bytes, a multiple of 2. */
>  	__le16 flags;
> +	/* From the packet start, excluding this header. */
> +	__le16 csum_start;
> +	__le16 csum_offset;
>  };

[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?

> @@ -189,16 +199,29 @@ static void ntb_netdev_rx_handler(struct ntb_transport_qp *qp, void *qp_data,
>  
>  	skb_put(skb, len + hdr_len);
>  	if (hdr) {
> +		u16 offset = le16_to_cpu(hdr->csum_offset);
> +		u16 start = le16_to_cpu(hdr->csum_start);
>  		u16 flags = le16_to_cpu(hdr->flags);
>  
>  		skb_pull(skb, hdr_len);
> -		if (flags)
> +		if (flags & ~NTB_NETDEV_HDR_F_CSUM)
>  			goto rx_drop;
> +
> +		if (flags & NTB_NETDEV_HDR_F_CSUM) {
> +			if (!(ndev->features & NETIF_F_RXCSUM)) {
> +				ntb_netdev_rx_stats_add(ndev, len);
> +				DEV_STATS_INC(ndev, rx_dropped);
> +				goto rx_free;
> +			}
> +
> +			if (start < ETH_HLEN ||
> +			    !skb_partial_csum_set(skb, start, offset))
> +				goto rx_drop;
> +		}
>  	}

[ ... ]

> @@ -316,6 +340,8 @@ static netdev_tx_t ntb_netdev_start_xmit(struct sk_buff *skb,
>  	struct ntb_netdev *dev = netdev_priv(ndev);
>  	u16 qid = skb_get_queue_mapping(skb);
>  	struct ntb_netdev_queue *q;
> +	unsigned int hdr_len = 0;
> +	unsigned int meta = 0;
>  	int rc;
>  
>  	q = &dev->queues[qid];
> @@ -323,7 +349,29 @@ static netdev_tx_t ntb_netdev_start_xmit(struct sk_buff *skb,
>  	if (unlikely(ntb_netdev_maybe_stop_tx(ndev, q, tx_stop)))
>  		return NETDEV_TX_BUSY;
>  
> -	rc = ntb_transport_tx_enqueue(q->qp, skb, skb->data, skb->len, 0);
> +	if (skb->ip_summed == CHECKSUM_PARTIAL) {
> +		if (READ_ONCE(q->peer_csum)) {
> +			struct ntb_netdev_hdr hdr = {
> +				.len = cpu_to_le16(sizeof(hdr)),
> +				.flags = cpu_to_le16(NTB_NETDEV_HDR_F_CSUM),
> +			};
> +
> +			if (skb_cow_head(skb, sizeof(hdr)))
> +				goto drop;
> +
> +			hdr.csum_start = cpu_to_le16(skb_checksum_start_offset(skb));
> +			hdr.csum_offset = cpu_to_le16(skb->csum_offset);
> +			hdr_len = sizeof(hdr);
> +			/* Keep skb->len unchanged for retries and byte accounting. */
> +			memcpy(skb->data - hdr_len, &hdr, hdr_len);
> +			meta = NTB_NETDEV_META_HDR;
> +		} else if (skb_checksum_help(skb)) {
> +			goto drop;
> +		}
> +	}
> +
> +	rc = ntb_transport_tx_enqueue(q->qp, skb, skb->data - hdr_len,
> +				      skb->len + hdr_len, meta);

[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?

> @@ -346,6 +394,28 @@ static netdev_tx_t ntb_netdev_start_xmit(struct sk_buff *skb,
>  	return NETDEV_TX_OK;
>  }
>  
> +static netdev_features_t ntb_netdev_features_check(struct sk_buff *skb,
> +						   struct net_device *ndev,
> +						   netdev_features_t features)
> +{
> +	if (skb->ip_summed == CHECKSUM_PARTIAL &&
> +	    skb_checksum_start_offset(skb) < ETH_HLEN)
> +		features &= ~NETIF_F_CSUM_MASK;
> +
> +	return vlan_features_check(skb, features);
> +}

[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?

[ ... ]

> @@ -420,6 +500,9 @@ static int ntb_netdev_close(struct net_device *ndev)
>  		timer_delete_sync(&queue->tx_timer);
>  	}
>  
> +	/* Apply RX checksum changes deferred while the interface was up. */
> +	netdev_update_features(ndev);
> +
>  	return 0;
>  }

[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?

> @@ -716,7 +801,8 @@ static int ntb_netdev_probe(struct device *client_dev)
>  
>  	ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
>  
> -	ndev->hw_features = ndev->features;
> +	/* Checksum bypass assumes a trusted NTB link, so keep it opt-in. */
> +	ndev->hw_features = ndev->features | NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
>  	ndev->watchdog_timeo = msecs_to_jiffies(NTB_TX_TIMEOUT_MS);

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