[PATCH net-next 0/2] net: ntb_netdev: Preserve checksum offload across NTB

[PATCH net-next 0/2] net: ntb_netdev: Preserve checksum offload across NTB

[Koichiro Den]

ntb_netdev may be used on embedded systems, where CPU resources are
often limited. L4 checksum calculation can therefore become a bottleneck
even when traffic stays within a trusted PCIe fabric.

This small series carries CHECKSUM_PARTIAL state across the NTB link
using opaque per-payload metadata in ntb_transport. Existing peers
continue to use software checksumming. The feature remains disabled by
default and must be enabled explicitly for trusted links.

Best regards,
Koichiro

Koichiro Den (2):
  NTB: ntb_transport: Add per-payload client metadata
  net: ntb_netdev: Preserve CHECKSUM_PARTIAL across NTB

 drivers/net/ntb_netdev.c      | 78 +++++++++++++++++++++++++++++++++--
 drivers/ntb/ntb_transport.c   | 18 +++++---
 include/linux/ntb_transport.h |  6 ++-
 3 files changed, 91 insertions(+), 11 deletions(-)


base-commit: b3217bdb0091e52887e23896cd82483f7808914a
-- 
2.51.0

[PATCH net-next 1/2] NTB: ntb_transport: Add per-payload client metadata

[Koichiro Den]

ntb_transport currently carries only payload bytes, with no way for clients
to associate metadata with an individual payload.

The payload header has a 32-bit flags field, with only BIT(0) and BIT(1) in
use. Carry opaque client metadata in the upper 24 bits. Expose it through
the transmit enqueue interface and receive callback. Reject values that do
not fit. Keep the low byte for transport flags so future flags can continue
from BIT(2).

No protocol version bump is needed. Existing Linux version 4 peers ignore
the upper bits on receive and always transmit them as zero.

Adapt ntb_netdev to the new interfaces without using metadata.

Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
 drivers/net/ntb_netdev.c      |  4 ++--
 drivers/ntb/ntb_transport.c   | 18 +++++++++++++-----
 include/linux/ntb_transport.h |  6 ++++--
 3 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
index 029a4a532a10..5c7fe6883cb9 100644
--- a/drivers/net/ntb_netdev.c
+++ b/drivers/net/ntb_netdev.c
@@ -123,7 +123,7 @@ static void ntb_netdev_event_handler(void *data, int link_is_up)
 }
 
 static void ntb_netdev_rx_handler(struct ntb_transport_qp *qp, void *qp_data,
-				  void *data, int len)
+				  void *data, int len, unsigned int meta)
 {
 	struct ntb_netdev_queue *q = qp_data;
 	struct ntb_netdev *dev = q->ntdev;
@@ -258,7 +258,7 @@ static netdev_tx_t ntb_netdev_start_xmit(struct sk_buff *skb,
 
 	ntb_netdev_maybe_stop_tx(ndev, q, tx_stop);
 
-	rc = ntb_transport_tx_enqueue(q->qp, skb, skb->data, skb->len);
+	rc = ntb_transport_tx_enqueue(q->qp, skb, skb->data, skb->len, 0);
 	if (rc)
 		goto err;
 
diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
index f59f926d4bfa..ea97460c118a 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -167,7 +167,7 @@ struct ntb_transport_qp {
 	unsigned int tx_max_frame;
 
 	void (*rx_handler)(struct ntb_transport_qp *qp, void *qp_data,
-			   void *data, int len);
+			   void *data, int len, unsigned int meta);
 	struct list_head rx_post_q;
 	struct list_head rx_pend_q;
 	struct list_head rx_free_q;
@@ -264,6 +264,10 @@ enum {
 	LINK_DOWN_FLAG = BIT(1),
 };
 
+/* Reserve the low byte for transport flags. */
+#define DESC_META_SHIFT		8
+#define DESC_META_MASK		(~0U << DESC_META_SHIFT)
+
 struct ntb_payload_header {
 	unsigned int ver;
 	unsigned int len;
@@ -1435,6 +1439,7 @@ static void ntb_complete_rxc(struct ntb_transport_qp *qp)
 	struct ntb_queue_entry *entry;
 	void *cb_data;
 	unsigned int len;
+	unsigned int meta;
 	unsigned long irqflags;
 
 	spin_lock_irqsave(&qp->ntb_rx_q_lock, irqflags);
@@ -1450,13 +1455,14 @@ static void ntb_complete_rxc(struct ntb_transport_qp *qp)
 
 		cb_data = entry->cb_data;
 		len = entry->len;
+		meta = entry->flags >> DESC_META_SHIFT;
 
 		list_move_tail(&entry->entry, &qp->rx_free_q);
 
 		spin_unlock_irqrestore(&qp->ntb_rx_q_lock, irqflags);
 
 		if (qp->rx_handler && qp->client_ready)
-			qp->rx_handler(qp, qp->cb_data, cb_data, len);
+			qp->rx_handler(qp, qp->cb_data, cb_data, len, meta);
 
 		spin_lock_irqsave(&qp->ntb_rx_q_lock, irqflags);
 	}
@@ -1647,6 +1653,7 @@ static int ntb_process_rxc(struct ntb_transport_qp *qp)
 
 	entry->rx_hdr = hdr;
 	entry->rx_index = qp->rx_index;
+	entry->flags = hdr->flags & DESC_META_MASK;
 
 	if (hdr->len > entry->len) {
 		dev_dbg(&qp->ndev->pdev->dev,
@@ -2332,6 +2339,7 @@ EXPORT_SYMBOL_GPL(ntb_transport_rx_enqueue);
  * @cb: per buffer pointer for callback function to use
  * @data: pointer to data buffer that will be sent
  * @len: length of the data buffer
+ * @meta: client metadata to send with the buffer
  *
  * Enqueue a new transmit buffer onto the transport queue from which a NTB
  * payload will be transmitted.  This assumes that a lock is being held to
@@ -2340,12 +2348,12 @@ EXPORT_SYMBOL_GPL(ntb_transport_rx_enqueue);
  * RETURNS: An appropriate -ERRNO error value on error, or zero for success.
  */
 int ntb_transport_tx_enqueue(struct ntb_transport_qp *qp, void *cb, void *data,
-			     unsigned int len)
+			     unsigned int len, unsigned int meta)
 {
 	struct ntb_queue_entry *entry;
 	int rc;
 
-	if (!qp || !len)
+	if (!qp || !len || meta > NTB_TRANSPORT_MAX_META)
 		return -EINVAL;
 
 	/* If the qp link is down already, just ignore. */
@@ -2361,7 +2369,7 @@ int ntb_transport_tx_enqueue(struct ntb_transport_qp *qp, void *cb, void *data,
 	entry->cb_data = cb;
 	entry->buf = data;
 	entry->len = len;
-	entry->flags = 0;
+	entry->flags = meta << DESC_META_SHIFT;
 	entry->errors = 0;
 	entry->tx_index = 0;
 
diff --git a/include/linux/ntb_transport.h b/include/linux/ntb_transport.h
index 7243eb98a722..9e807542b6c4 100644
--- a/include/linux/ntb_transport.h
+++ b/include/linux/ntb_transport.h
@@ -50,6 +50,8 @@
 
 struct ntb_transport_qp;
 
+#define NTB_TRANSPORT_MAX_META	0x00ffffffU
+
 struct ntb_transport_client {
 	struct device_driver driver;
 	int (*probe)(struct device *client_dev);
@@ -63,7 +65,7 @@ void ntb_transport_unregister_client_dev(char *device_name);
 
 struct ntb_queue_handlers {
 	void (*rx_handler)(struct ntb_transport_qp *qp, void *qp_data,
-			   void *data, int len);
+			   void *data, int len, unsigned int meta);
 	void (*tx_handler)(struct ntb_transport_qp *qp, void *qp_data,
 			   void *data, int len);
 	void (*event_handler)(void *data, int status);
@@ -78,7 +80,7 @@ void ntb_transport_free_queue(struct ntb_transport_qp *qp);
 int ntb_transport_rx_enqueue(struct ntb_transport_qp *qp, void *cb, void *data,
 			     unsigned int len);
 int ntb_transport_tx_enqueue(struct ntb_transport_qp *qp, void *cb, void *data,
-			     unsigned int len);
+			     unsigned int len, unsigned int meta);
 void *ntb_transport_rx_remove(struct ntb_transport_qp *qp, unsigned int *len);
 void ntb_transport_link_up(struct ntb_transport_qp *qp);
 void ntb_transport_link_down(struct ntb_transport_qp *qp);
-- 
2.51.0

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

[Koichiro Den]

Calculating L4 checksums can limit ntb_netdev throughput especially on
embedded systems, where CPU resources are often limited. A trusted PCIe
fabric can avoid that work.

Carry CHECKSUM_PARTIAL with csum_start and csum_offset across the NTB link.
Advertise support in every frame and fall back to software until the peer
capability is seen. This preserves netdev checksum semantics and
interoperability with existing transport version 4 peers.

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.

Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
 drivers/net/ntb_netdev.c | 76 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 73 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
index 5c7fe6883cb9..b9a78ff695c8 100644
--- a/drivers/net/ntb_netdev.c
+++ b/drivers/net/ntb_netdev.c
@@ -4,6 +4,7 @@
  */
 #include <linux/etherdevice.h>
 #include <linux/ethtool.h>
+#include <linux/if_vlan.h>
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/ntb.h>
@@ -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);
+
+	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;
+
+		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) {
@@ -172,6 +204,14 @@ static void ntb_netdev_rx_handler(struct ntb_transport_qp *qp, void *qp_data,
 		ndev->stats.rx_errors++;
 		ndev->stats.rx_fifo_errors++;
 	}
+	return;
+
+rx_drop:
+	ndev->stats.rx_errors++;
+	ndev->stats.rx_dropped++;
+	dev_kfree_skb_any(skb);
+	skb = new_skb;
+	goto enqueue_again;
 }
 
 static int __ntb_netdev_maybe_stop_tx(struct net_device *netdev,
@@ -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;
 
@@ -267,12 +318,29 @@ static netdev_tx_t ntb_netdev_start_xmit(struct sk_buff *skb,
 
 	return NETDEV_TX_OK;
 
+drop:
+	dev_kfree_skb_any(skb);
+	ndev->stats.tx_dropped++;
+	ndev->stats.tx_errors++;
+	return NETDEV_TX_OK;
+
 err:
 	ndev->stats.tx_dropped++;
 	ndev->stats.tx_errors++;
 	return NETDEV_TX_BUSY;
 }
 
+static netdev_features_t ntb_netdev_features_check(struct sk_buff *skb,
+						   struct net_device *ndev,
+						   netdev_features_t features)
+{
+	if (skb->ip_summed == CHECKSUM_PARTIAL &&
+	    skb->csum_offset > NTB_NETDEV_META_CSUM_OFFSET_MASK)
+		features &= ~NETIF_F_CSUM_MASK;
+
+	return vlan_features_check(skb, features);
+}
+
 static void ntb_netdev_tx_timer(struct timer_list *t)
 {
 	struct ntb_netdev_queue *q = timer_container_of(q, t, tx_timer);
@@ -423,6 +491,7 @@ static const struct net_device_ops ntb_netdev_ops = {
 	.ndo_open = ntb_netdev_open,
 	.ndo_stop = ntb_netdev_close,
 	.ndo_start_xmit = ntb_netdev_start_xmit,
+	.ndo_features_check = ntb_netdev_features_check,
 	.ndo_change_mtu = ntb_netdev_change_mtu,
 	.ndo_set_mac_address = eth_mac_addr,
 };
@@ -642,7 +711,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;
 	ndev->watchdog_timeo = msecs_to_jiffies(NTB_TX_TIMEOUT_MS);
 
 	eth_random_addr(ndev->perm_addr);
-- 
2.51.0

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

[Jakub Kicinski]

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

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

[Koichiro Den]

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/20260817064916.13278-5-den@valinux.co.jp/

> 
> > +
> > +	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/20260817053519.4135287-1-den@valinux.co.jp/

Best regards,
Koichiro

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

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

[Jakub Kicinski]

On Fri, 14 Aug 2026 12:29:13 +0900 Koichiro Den wrote:
> +rx_drop:
> +	ndev->stats.rx_errors++;
> +	ndev->stats.rx_dropped++;

Error implies that the packet was dropped. Could you first
iron out the double counting this driver is doing?
Please see the kdoc on struct rtnl_link_stats64

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

[Koichiro Den]

On Tue, Aug 18, 2026 at 09:29:38AM -0700, Jakub Kicinski wrote:
> On Fri, 14 Aug 2026 12:29:13 +0900 Koichiro Den wrote:
> > +rx_drop:
> > +	ndev->stats.rx_errors++;
> > +	ndev->stats.rx_dropped++;
> 
> Error implies that the packet was dropped. Could you first
> iron out the double counting this driver is doing?
> Please see the kdoc on struct rtnl_link_stats64

You're right. I'll send the accounting fix to net first.
Also, apologies again for sending v2 while net-next was closed.

Best regards,
Koichiro