Re: [PATCH net-next v3 4/4] net: ntb_netdev: Preserve CHECKSUM_PARTIAL across NTB

[netdev-bot+sashiko]

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

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

- [High] ntb_netdev_start_xmit() unconditionally seeds `meta =
  NTB_NETDEV_META_CAP_CSUM` (drivers/net/ntb_netdev.c:317), so the…
- [Medium] dev->peer_csum is a device-wide lockless bool written from two
  unsynchronised contexts: ntb_netdev_event_handler() clears it…
- [Low] ntb_netdev_rx_handler() performs an unconditional
  `WRITE_ONCE(dev->peer_csum, true)` for every received frame carrying…

--

net: ntb_netdev: Preserve CHECKSUM_PARTIAL across NTB

This carries CHECKSUM_PARTIAL, csum_start and csum_offset across the NTB
link in the transport meta field, advertising the capability on every frame
and falling back to software checksumming until the peer capability is
observed.  TX and RX checksum offload are added to hw_features but left
disabled by default.

> diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
> index d03cbda1637d9..de8c4f320614d 100644
> --- a/drivers/net/ntb_netdev.c
> +++ b/drivers/net/ntb_netdev.c

[ ... ]

> @@ -29,6 +30,21 @@ static unsigned int tx_stop = 5;
>  #define NTB_NETDEV_MAX_QUEUES		64
>  #define NTB_NETDEV_DEFAULT_QUEUES	1
>  
> +/*
> + * Checksum metadata layout:
> + *   bit 23     capability, advertised on every packet
> + *   bit 22     per-packet CHECKSUM_PARTIAL flag
> + *   bit 21..6  skb_checksum_start_offset() (16 bits)
> + *   bit 5..0   skb->csum_offset (6 bits)
> + *
> + * Until the capability is observed, complete partial checksums in software.
> + * Six offset bits cover TCP/UDP. Larger offsets use software checksumming.
> + */
> +#define NTB_NETDEV_META_CAP_CSUM		BIT(23)
> +#define NTB_NETDEV_META_CSUM			BIT(22)
> +#define NTB_NETDEV_META_CSUM_START_SHIFT	6
> +#define NTB_NETDEV_META_CSUM_OFFSET_MASK	GENMASK(5, 0)
> +
>  struct ntb_netdev;
>  
>  struct ntb_netdev_queue {
> @@ -44,6 +60,7 @@ struct ntb_netdev {
>  	struct net_device *ndev;
>  	unsigned int num_queues;
>  	struct ntb_netdev_queue *queues;
> +	bool peer_csum;
>  };
>  
>  #define	NTB_TX_TIMEOUT_MS	1000
> @@ -108,6 +125,7 @@ static void ntb_netdev_event_handler(void *data, int link_is_up)
>  	struct net_device *ndev;
>  
>  	ndev = dev->ndev;
> +	WRITE_ONCE(dev->peer_csum, false);

[Severity: Medium]
Can this clear of peer_csum be lost?  ntb_netdev_event_handler() runs in
workqueue/process context out of the transport, while
ntb_netdev_rx_handler() sets the same field from the qp rxc_db tasklet or
from an RX DMA completion (ntb_rx_copy_callback() -> ntb_complete_rxc()).

Looking at ntb_qp_link_cleanup() in drivers/ntb/ntb_transport.c, the
event handler is invoked without stopping the rxc_db tasklet or draining
outstanding RX DMA completions:

	cancel_delayed_work_sync(&qp->link_work);
	ntb_qp_link_down_reset(qp);

	if (qp->event_handler)
		qp->event_handler(qp->cb_data, qp->link_is_up);

and ntb_complete_rxc() still calls into the driver because transport link
loss does not clear client_ready:

		if (qp->rx_handler && qp->client_ready)
			qp->rx_handler(qp, qp->cb_data, cb_data, len, meta);

So a completion belonging to the previous link generation can run
WRITE_ONCE(dev->peer_csum, true) after the link-down clear, and after the
following link-up clear too.  If the peer that comes back does not
implement the capability (for example it rebooted into a pre-patch
kernel), ntb_netdev_start_xmit() then sees peer_csum == true, sets
NTB_NETDEV_META_CSUM and skips skb_checksum_help(), and the old peer marks
those frames CHECKSUM_NONE with only the pseudo-header sum in place.
Wouldn't that silently break TCP/UDP over the link until it is bounced
again?  READ_ONCE()/WRITE_ONCE() only prevent torn access here.

A related question: peer_csum is device-wide while the event handler is
per-queue, and multi-queue is reachable through the driver's ethtool
set_channels.  Should one queue's link event clobber capability state
learned on the other queues that are still up?

>  
>  	netdev_dbg(ndev, "Event %x, Link %x, qp %u\n", link_is_up,
>  		   ntb_transport_link_query(q->qp), q->qid);

[ ... ]

> @@ -146,21 +174,32 @@ static void ntb_netdev_rx_handler(struct ntb_transport_qp *qp, void *qp_data,
>  		goto enqueue_again;
>  	}
>  
> -	tstats = this_cpu_ptr(ndev->tstats);
> -	flags = u64_stats_update_begin_irqsave(&tstats->syncp);
> -	u64_stats_inc(&tstats->rx_packets);
> -	u64_stats_add(&tstats->rx_bytes, len);
> -	u64_stats_update_end_irqrestore(&tstats->syncp, flags);
> -
>  	new_skb = netdev_alloc_skb(ndev, ndev->mtu + ETH_HLEN);
>  	if (!new_skb) {
> +		ntb_netdev_rx_stats_add(ndev, len);
>  		DEV_STATS_INC(ndev, rx_dropped);
>  		goto enqueue_again;
>  	}
>  
>  	skb_put(skb, len);
> +	if (meta & NTB_NETDEV_META_CAP_CSUM)
> +		WRITE_ONCE(dev->peer_csum, true);

[Severity: Low]
This isn't a bug, but does this need to store on every frame?  peer_csum
only ever goes false -> true within a link session, and it sits in the
shared struct ntb_netdev next to ndev/num_queues/queues, which
ntb_netdev_start_xmit() reads once per transmitted packet:

	q = &dev->queues[qid];
	...
		if (READ_ONCE(dev->peer_csum))

Every received frame therefore dirties a cacheline that each TX queue
reads per packet, possibly from another CPU (more so with multiple queues
via set_channels).  Would a test before the store, or per-queue state,
avoid that?

	if (!READ_ONCE(dev->peer_csum))
		WRITE_ONCE(dev->peer_csum, true);

> +
> +	if (meta & NTB_NETDEV_META_CSUM) {
> +		u16 csum_start = (meta >> NTB_NETDEV_META_CSUM_START_SHIFT) & U16_MAX;
> +		u16 csum_offset = meta & NTB_NETDEV_META_CSUM_OFFSET_MASK;
> +
> +		if (csum_start < ETH_HLEN ||
> +		    !skb_partial_csum_set(skb, csum_start, csum_offset))
> +			goto rx_drop;
> +
> +		if (!(ndev->features & NETIF_F_RXCSUM) &&
> +		    skb_checksum_help(skb))
> +			goto rx_drop;
> +	}

[Severity: High]
With rx-checksumming off, does this path still end up trusting the peer?

The frame is not rejected: skb_partial_csum_set() applies the
peer-supplied csum_start/csum_offset, then skb_checksum_help() in
net/core/dev.c computes the sum over the bytes that were just received and
writes it into the packet:

	offset = skb_checksum_start_offset(skb);
	...
	csum = skb_checksum(skb, offset, skb->len - offset, 0);

	offset += skb->csum_offset;
	...
	*(__sum16 *)(skb->data + offset) = csum_fold(csum) ?: CSUM_MANGLED_0;
out_set_summed:
	skb->ip_summed = CHECKSUM_NONE;

So the L4 checksum the stack later validates is one the receiver itself
generated.  Doesn't that mean corruption introduced anywhere on the NTB
path (bad DMA or memcpy, a misbehaving peer, a wrong csum_start) is turned
into an apparently valid checksum, in both feature states?  In the RXCSUM
on case skb_csum_unnecessary() treats CHECKSUM_PARTIAL with a valid
csum_start as needing no verification, and in the off case the sum is
recomputed locally, so the knob appears to select who spends the CPU
rather than whether the frame is trusted.

Together with the unconditional advertisement in ntb_netdev_start_xmit():

	unsigned int meta = NTB_NETDEV_META_CAP_CSUM;

the capability bit is sent on every frame with no reference to the local
ndev->features & NETIF_F_RXCSUM, and the peer latches it above.  The only
place peer_csum is cleared is a qp link event in
ntb_netdev_event_handler(), so once a peer has latched it there is no
negative advertisement, timeout or renegotiation short of bouncing the
link.

The commit message says:

    Leave the TX and RX checksum features disabled by default. Users can
    just enable them explicitly for links they trust for lower CPU usage
    and/or higher throughput.

Is that accurate for the receive direction, given the receive-side
decision is made entirely by the transmitting peer?  Would gating
NTB_NETDEV_META_CAP_CSUM on the local NETIF_F_RXCSUM state, and dropping
plus counting META_CSUM frames that arrive while RXCSUM is off instead of
repairing them, match the described behaviour better?

Also, in the off case a full software checksum over every payload now runs
in the RX tasklet at the remote peer's discretion, which is the opposite
of the CPU saving the change is aiming for.

> +
> +	ntb_netdev_rx_stats_add(ndev, len);
>  	skb->protocol = eth_type_trans(skb, ndev);
> -	skb->ip_summed = CHECKSUM_NONE;
>  	skb_record_rx_queue(skb, q->qid);
>  
>  	netif_rx(skb);

[ ... ]

> @@ -268,6 +314,7 @@ static const struct ntb_queue_handlers ntb_netdev_handlers = {
>  static netdev_tx_t ntb_netdev_start_xmit(struct sk_buff *skb,
>  					 struct net_device *ndev)
>  {
> +	unsigned int meta = NTB_NETDEV_META_CAP_CSUM;
>  	struct ntb_netdev *dev = netdev_priv(ndev);
>  	u16 qid = skb_get_queue_mapping(skb);
>  	struct ntb_netdev_queue *q;
> @@ -278,7 +325,17 @@ 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(dev->peer_csum))
> +			meta |= NTB_NETDEV_META_CSUM |
> +				(skb_checksum_start_offset(skb) <<
> +				 NTB_NETDEV_META_CSUM_START_SHIFT) |
> +				skb->csum_offset;
> +		else if (skb_checksum_help(skb))
> +			goto drop;
> +	}
> +
> +	rc = ntb_transport_tx_enqueue(q->qp, skb, skb->data, skb->len, meta);

[ ... ]

> @@ -671,7 +741,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;

This is where NETIF_F_RXCSUM becomes a user-visible knob, and the comment
states the bypass is opt-in.  Given the receive path above accepts and
repairs offloaded frames when the bit is clear, does turning
rx-checksumming off actually keep the bypass out of the picture?

>  	ndev->watchdog_timeo = msecs_to_jiffies(NTB_TX_TIMEOUT_MS);
>  
>  	eth_random_addr(ndev->perm_addr);

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