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

[Koichiro Den]

On Wed, Sep 09, 2026 at 08:24:48PM +0000, netdev-bot+sashiko@xxxxxxxxxx wrote:
> 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()).

Thanks for the catch.

You're absolutely right. ntb_complete_rxc() returns the entry to rx_free_q
before calling rx_handler. An empty rx_post_q therefore doesn't mean all client
callbacks have returned. An old callback can still set peer_csum after a link
reset.

We could count in-flight callbacks and wait for them during link cleanup, but
I'd rather not add that dependency (or, to put it another way, ad-hoc unclean
code) just for capability exchange. I put NTB_NETDEV_META_CAP_CSUM bit in the
in-band metadata to keep the transport changes as small as possible, but fixing
these races would complicate the implementation after all..

Thus, for v4 I'm now considering:

  - Exchange opaque per-QP capabilities using spare SPADs after the existing MW
    size and MSI information.
  - Extend the link-up API and client event callback like:

    void ntb_transport_link_up(struct ntb_transport_qp *qp, u32 local_caps);
    void (*event_handler)(void *data, int status, u32 peer_caps);

    Clients pass local_caps at link-up and receive peer_caps with the link-up
    event after the exchange. Both stay opaque to ntb_transport.

Legacy peers, or QPs without enough spare SPADs, would fall back to zero peer
capabilities.

This removes NTB_NETDEV_META_CAP_CSUM from in-band packet metadata. Capability
is learned at link-up, with no updates from RX callbacks.

This needs agreement on the NTB API (+ new SPAD use) change, but I think it's
cleaner long-term.
Dave, I'd appreciate your thoughts on this.

Best regards,
Koichiro

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