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

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.

[ ... ]

[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);

[ ... ]

[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);

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

[ ... ]

[ ... ]

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