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

[Jakub Kicinski <kuba@kernel.org>]

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

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

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

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

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