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

On Tue, Aug 18, 2026 at 09:27:41AM -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: ntb_netdev: Preserve CHECKSUM_PARTIAL across NTB
> 
> This carries CHECKSUM_PARTIAL (csum_start and csum_offset) in the NTB
> transport metadata word so that L4 checksums do not have to be computed
> on either side of the link. A capability bit is advertised on every
> transmitted frame, and both TX and RX checksum offload are left off by
> default and exposed through hw_features so users can opt in.
> 
> > diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
> > index 5c7fe6883cb93..b9a78ff695c87 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,8 @@ static void ntb_netdev_event_handler(void *data, int link_is_up)
> >  	struct net_device *ndev;
> >  
> >  	ndev = dev->ndev;
> > +	if (!link_is_up)
> > +		WRITE_ONCE(dev->peer_csum, false);
> >  
> >  	netdev_dbg(ndev, "Event %x, Link %x, qp %u\n", link_is_up,
> >  		   ntb_transport_link_query(q->qp), q->qid);
> > @@ -151,8 +170,21 @@ static void ntb_netdev_rx_handler(struct ntb_transport_qp *qp, void *qp_data,
> >  	}
> >  
> >  	skb_put(skb, len);
> > +	if (meta & NTB_NETDEV_META_CAP_CSUM)
> > +		WRITE_ONCE(dev->peer_csum, true);
> 

Thanks for the review. To summarize:

  #1: Already addressed in v2.
  #2: Thanks for catching it! I'll address it in v3.
  #3: Already being handled in a separate series.

> [Severity: Medium]
> Can the clear of dev->peer_csum in ntb_netdev_event_handler() be lost
> against this set in ntb_netdev_rx_handler()?
> 
> The two writes run from different contexts with no common lock, and
> WRITE_ONCE() only prevents tearing:
> 
> CPU0 (workqueue, ntb_qp_link_cleanup() -> qp->event_handler())
> 	ntb_netdev_event_handler(link_is_up = 0)
> 	    WRITE_ONCE(dev->peer_csum, false);
> 
> CPU1 (rx tasklet, ntb_complete_rxc() -> qp->rx_handler(), called after
>       spin_unlock_irqrestore(&qp->ntb_rx_q_lock, irqflags))
> 	ntb_netdev_rx_handler()
> 	    WRITE_ONCE(dev->peer_csum, true);   /* stale frame */
> 
> ntb_qp_link_context_reset() only sets qp->active = false, which gates
> future tasklet_schedule() calls; tasklet_kill(&qp->rxc_db_work) happens
> only in ntb_transport_free_queue(). So an rx callback from the previous
> link generation can still run after, or overlap with, the clear.
> 
> Since there is no matching re-initialisation on the link-up transition
> (ntb_qp_link_work() just calls the event handler with link_is_up = 1),
> does the stale true then persist? After reconnecting to a peer that
> ignores the metadata word, ntb_netdev_start_xmit() would read true, set
> NTB_NETDEV_META_CSUM and skip skb_checksum_help(), so frames leave with
> only a pseudo-header checksum and the old peer's stack drops them without
> incrementing any counter on either side. Would tying the flag to a link
> generation, or clearing it unconditionally in the event handler
> (including on link-up), close this?

This was address in v2 by resetting peer_csum on every link event:
https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/

> 
> > +
> > +	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 (!skb_partial_csum_set(skb, csum_start, csum_offset))
> > +			goto rx_drop;
> 
> [Severity: High]
> Is csum_start validated enough here? It is a full 16-bit peer-supplied
> value, and skb_partial_csum_set() only bounds it against the frame as
> received:
> 
> net/core/skbuff.c:skb_partial_csum_set() {
> 	u32 csum_end = (u32)start + (u32)off + sizeof(__sum16);
> 	u32 csum_start = skb_headroom(skb) + (u32)start;
> 
> 	if (unlikely(csum_start >= U16_MAX || csum_end > skb_headlen(skb)))
> 		...
> 		return false;
> 	skb->ip_summed = CHECKSUM_PARTIAL;
> 	skb->csum_start = csum_start;
> 	...
> }
> 
> So csum_start = 0 with csum_offset = 16 is accepted. eth_type_trans()
> then pulls ETH_HLEN, and skb_checksum_start_offset() becomes negative
> (start - 14) while ip_summed stays CHECKSUM_PARTIAL when
> NETIF_F_RXCSUM is enabled.
> 
> The other consumer of untrusted partial-checksum metadata rejects this
> explicitly:
> 
> include/linux/virtio_net.h:__virtio_net_hdr_to_skb() {
> 		if (!skb_partial_csum_set(skb, start, off))
> 			return -EINVAL;
> 		if (skb_transport_offset(skb) < nh_min_len)
> 			return -EINVAL;
> }
> 
> Downstream, a negative offset is not caught cleanly. skb_checksum_help()
> compares the signed offset against unsigned skb_headlen() and fires
> WARN_ONCE() plus skb_dump(), which a peer can trigger at will (fatal
> with panic_on_warn). And skb_copy_and_csum_dev() does:
> 
> net/core/skbuff.c:skb_copy_and_csum_dev() {
> 	long csstart;
> 
> 	if (skb->ip_summed == CHECKSUM_PARTIAL)
> 		csstart = skb_checksum_start_offset(skb);
> 	...
> 	BUG_ON(csstart > skb_headlen(skb));
> 
> 	skb_copy_from_linear_data(skb, to, csstart);
> }
> 
> The BUG_ON does not trigger for a negative csstart on 64-bit (the
> unsigned int result is promoted to long), and the length parameter is
> unsigned:
> 
> include/linux/skbuff.h:skb_copy_from_linear_data() {
> 	memcpy(to, skb->data, len);   /* const unsigned int len */
> }
> 
> Can that turn a negative csstart into a multi-gigabyte memcpy into a
> fixed-size driver TX buffer? Drivers that advertise checksum offload and
> call this helper unconditionally still exist (8139too sets
> NETIF_F_HW_CSUM in rtl8139_init_one() and calls skb_copy_and_csum_dev()
> from rtl8139_start_xmit(); sc92031 fakes NETIF_F_IP_CSUM the same way),
> so skb_csum_hwoffload_help() will not sanitize the skb first. A negative
> offset survives to egress whenever the ingress pull exceeds the egress
> push, for example locally terminated IPIP/GRE
> (__iptunnel_pull_header() advances skb->data via skb_pull_rcsum() and
> iptunnel_pull_offloads() leaves ip_summed alone) followed by forwarding
> the inner packet out such a NIC.
> 
> Would a check that skb_transport_offset(skb) leaves room for the
> Ethernet and L3 headers, mirroring the virtio_net one, be appropriate
> here?

The csum_start issue is real. Thanks for catching this. The missing lower bound
check needs to be added, but I don't think requiring room for an L3 header as
virtio_net does is appropriate here. It would be too restrictive for
ntb_netdev's protocol-agnostic NETIF_F_HW_CSUM contract.

I'll make sure csum_start values below ETH_HLEN are rejected in v3.

> 
> > +
> > +		if (!(ndev->features & NETIF_F_RXCSUM) &&
> > +		    skb_checksum_help(skb))
> > +			goto rx_drop;
> > +	}
> >  	skb->protocol = eth_type_trans(skb, ndev);
> > -	skb->ip_summed = CHECKSUM_NONE;
> >  	skb_record_rx_queue(skb, q->qid);
> >  
> >  	if (netif_rx(skb) == NET_RX_DROP) {
> 
> [ ... ]
> 
> > @@ -252,13 +292,24 @@ 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 meta = NTB_NETDEV_META_CAP_CSUM;
> >  	int rc;
> >  
> >  	q = &dev->queues[qid];
> >  
> >  	ntb_netdev_maybe_stop_tx(ndev, q, tx_stop);
> >  
> > -	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);
> >  	if (rc)
> >  		goto err;
> 
> [Severity: High]
> This isn't a bug introduced by this patch, since the baseline already
> treated rc == 0 as a completed handoff with meta = 0, but does the
> rc == 0 path here leak the skb when the QP link is going down?
> 
> drivers/ntb/ntb_transport.c:ntb_transport_tx_enqueue() {
> 	/* If the qp link is down already, just ignore. */
> 	if (!qp->link_is_up)
> 		return 0;
> 
> 	entry = ntb_list_rm(&qp->ntb_tx_free_q_lock, &qp->tx_free_q);
> 	...
> 	entry->cb_data = cb;
> }
> 
> The early return happens before entry->cb_data = cb, so the transport
> never records the skb and ntb_netdev_tx_handler() -- the only place that
> calls dev_kfree_skb_any() for a transmitted skb -- is never invoked for
> it. start_xmit() then returns NETDEV_TX_OK without freeing.
> 
> The window looks reachable: qp->link_is_up is cleared in
> ntb_qp_link_context_reset() before ntb_netdev_event_handler() stops the
> subqueue, and both __ntb_netdev_maybe_stop_tx() and
> ntb_netdev_tx_timer() re-open the queue based on a check-then-act
> ntb_transport_link_query():
> 
> 		if (__netif_subqueue_stopped(ndev, q->qid) &&
> 		    ntb_transport_link_query(q->qp))
> 			netif_wake_subqueue(ndev, q->qid);
> 
> Would repeated peer link flaps then leak one sk_buff per racing packet?
> With this patch each leaked skb can additionally carry a freshly
> allocated linear buffer, because skb_checksum_help() may reallocate the
> head via pskb_expand_head() just before the enqueue.

The link-down skb leak is being handled by the separate net fix series:
https://patchwork.kernel.org/project/netdevbpf/cover/[email protected]/

Best regards,
Koichiro

> 
> [ ... ]
> -- 
> pw-bot: cr