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

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

[Koichiro Den]

Hi,

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 series carries CHECKSUM_PARTIAL across the NTB link using opaque
per-payload metadata in ntb_transport. Peers without metadata support
continue to use software checksumming. The feature remains disabled by
default and must be enabled explicitly on trusted links.

Note: the first two fixes came from Sashiko's review of v1. They touch
the same ntb_transport path as the metadata patch, so keeping them here
avoids a cross-tree dependency. With review from the NTB side, I hope
the whole series can go through net-next.

Best regards,
Koichiro
---
Changes in v3:
  - Rebase after the related ntb_netdev/ntb_transport fixes landed in
    net-next
  - Avoid counting RX checksum failures as both errors and drops (Jakub)
  - Reject csum_start below ETH_HLEN on RX and fall back to software on
    TX (Jakub)
  - Use READ_ONCE() for hdr->ver and hdr->len (Sashiko)

Changes in v2:
  - Reset peer checksum capability on every link event (Sashiko)
  - Add prerequisite fixes for RX ordering and shared field endianness
    (Sashiko)

v2: https://lore.kernel.org/r/20260817064916.13278-1-den@valinux.co.jp/
v1: https://lore.kernel.org/r/20260814032913.3558500-1-den@valinux.co.jp/


Koichiro Den (4):
  NTB: ntb_transport: Order RX descriptor reads after completion
  NTB: ntb_transport: Use little-endian shared fields
  NTB: ntb_transport: Add per-payload client metadata
  net: ntb_netdev: Preserve CHECKSUM_PARTIAL across NTB

 drivers/net/ntb_netdev.c      | 95 ++++++++++++++++++++++++++++++-----
 drivers/ntb/ntb_transport.c   | 73 +++++++++++++++++----------
 include/linux/ntb_transport.h |  6 ++-
 3 files changed, 133 insertions(+), 41 deletions(-)

base-commit: 6ebcf5074cff0402730c6981d2397139fee6322d
-- 
2.51.0

[PATCH net-next v3 1/4] NTB: ntb_transport: Order RX descriptor reads after completion

[Koichiro Den]

The peer writes payloads and descriptors into a DMA-coherent memory
window. ntb_process_rxc() checks DESC_DONE_FLAG before consuming the
descriptor and payload, but coherent memory alone does not order those
reads on weakly ordered CPUs.

Read the completion word once and issue dma_rmb() after DONE is observed.
Use the saved word for subsequent transport flag checks.

Fixes: fce8a7bb5b4b ("PCI-Express Non-Transparent Bridge Support")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/r/20260815032932.151F11F000E9@smtp.kernel.org/
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v3:
  - Add Dave's Reviewed-by tag.

 drivers/ntb/ntb_transport.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
index f9caa1a653c5..74f4f8c1c7be 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -1609,21 +1609,25 @@ static int ntb_process_rxc(struct ntb_transport_qp *qp)
 {
 	struct ntb_payload_header *hdr;
 	struct ntb_queue_entry *entry;
+	unsigned int flags;
 	void *offset;
 
 	offset = qp->rx_buff + qp->rx_max_frame * qp->rx_index;
 	hdr = offset + qp->rx_max_frame - sizeof(struct ntb_payload_header);
 
-	dev_dbg(&qp->ndev->pdev->dev, "qp %d: RX ver %u len %d flags %x\n",
-		qp->qp_num, hdr->ver, hdr->len, hdr->flags);
-
-	if (!(hdr->flags & DESC_DONE_FLAG)) {
+	flags = READ_ONCE(hdr->flags);
+	if (!(flags & DESC_DONE_FLAG)) {
 		dev_dbg(&qp->ndev->pdev->dev, "done flag not set\n");
 		qp->rx_ring_empty++;
 		return -EAGAIN;
 	}
 
-	if (hdr->flags & LINK_DOWN_FLAG) {
+	dma_rmb();
+
+	dev_dbg(&qp->ndev->pdev->dev, "qp %d: RX ver %u len %d flags %x\n",
+		qp->qp_num, hdr->ver, hdr->len, flags);
+
+	if (flags & LINK_DOWN_FLAG) {
 		dev_dbg(&qp->ndev->pdev->dev, "link down flag set\n");
 		ntb_qp_link_down(qp);
 		hdr->flags = 0;
-- 
2.51.0

[PATCH net-next v3 2/4] NTB: ntb_transport: Use little-endian shared fields

[Koichiro Den]

ntb_transport writes payload headers and the RX ring tail with
iowrite32(), but reads peer-written copies from coherent memory as native
integers. The values are therefore byte-swapped when read on a big-endian
system.

Mark the shared fields as __le32 and convert coherent-memory accesses
accordingly. Read hdr->ver and hdr->len once so their checks and later
uses see the same values.

Fixes: 74465645cdb4 ("NTB: Fix Sparse Warnings")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/r/20260815032932.151F11F000E9@smtp.kernel.org/
Link: https://lore.kernel.org/r/20260818064951.7EA231F000E9@smtp.kernel.org/
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v3:
  - Use READ_ONCE() for hdr->ver and hdr->len (Sashiko)
  - Add Dave's Reviewed-by tag.

 drivers/ntb/ntb_transport.c | 47 +++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 20 deletions(-)

diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
index 74f4f8c1c7be..3f497a62673f 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -132,7 +132,7 @@ struct ntb_queue_entry {
 };
 
 struct ntb_rx_info {
-	unsigned int entry;
+	__le32 entry;
 };
 
 struct ntb_transport_qp {
@@ -265,9 +265,9 @@ enum {
 };
 
 struct ntb_payload_header {
-	unsigned int ver;
-	unsigned int len;
-	unsigned int flags;
+	__le32 ver;
+	__le32 len;
+	__le32 flags;
 };
 
 enum {
@@ -514,7 +514,8 @@ static int ntb_qp_debugfs_stats_show(struct seq_file *s, void *v)
 	seq_printf(s, "tx_err_no_buf - %llu\n", qp->tx_err_no_buf);
 	seq_printf(s, "tx_mw - \t0x%p\n", qp->tx_mw);
 	seq_printf(s, "tx_index (H) - \t%u\n", qp->tx_index);
-	seq_printf(s, "RRI (T) - \t%u\n", qp->remote_rx_info->entry);
+	seq_printf(s, "RRI (T) - \t%u\n",
+		   le32_to_cpu(qp->remote_rx_info->entry));
 	seq_printf(s, "tx_max_entry - \t%u\n", qp->tx_max_entry);
 	seq_printf(s, "free tx - \t%u\n", ntb_transport_tx_free_entry(qp));
 	seq_putc(s, '\n');
@@ -633,7 +634,7 @@ static int ntb_transport_setup_qp_mw(struct ntb_transport_ctx *nt,
 		qp->rx_alloc_entry++;
 	}
 
-	qp->remote_rx_info->entry = qp->rx_max_entry - 1;
+	qp->remote_rx_info->entry = cpu_to_le32(qp->rx_max_entry - 1);
 
 	/* setup the hdr offsets with 0's */
 	for (i = 0; i < qp->rx_max_entry; i++) {
@@ -919,7 +920,7 @@ static void ntb_qp_link_down_reset(struct ntb_transport_qp *qp)
 {
 	ntb_qp_link_context_reset(qp);
 	if (qp->remote_rx_info)
-		qp->remote_rx_info->entry = qp->rx_max_entry - 1;
+		qp->remote_rx_info->entry = cpu_to_le32(qp->rx_max_entry - 1);
 }
 
 static void ntb_qp_link_cleanup(struct ntb_transport_qp *qp)
@@ -1445,7 +1446,7 @@ static void ntb_complete_rxc(struct ntb_transport_qp *qp)
 		if (!(entry->flags & DESC_DONE_FLAG))
 			break;
 
-		entry->rx_hdr->flags = 0;
+		entry->rx_hdr->flags = cpu_to_le32(0);
 		iowrite32(entry->rx_index, &qp->rx_info->entry);
 
 		cb_data = entry->cb_data;
@@ -1609,13 +1610,15 @@ static int ntb_process_rxc(struct ntb_transport_qp *qp)
 {
 	struct ntb_payload_header *hdr;
 	struct ntb_queue_entry *entry;
-	unsigned int flags;
 	void *offset;
+	u32 flags;
+	u32 len;
+	u32 ver;
 
 	offset = qp->rx_buff + qp->rx_max_frame * qp->rx_index;
 	hdr = offset + qp->rx_max_frame - sizeof(struct ntb_payload_header);
 
-	flags = READ_ONCE(hdr->flags);
+	flags = le32_to_cpu(READ_ONCE(hdr->flags));
 	if (!(flags & DESC_DONE_FLAG)) {
 		dev_dbg(&qp->ndev->pdev->dev, "done flag not set\n");
 		qp->rx_ring_empty++;
@@ -1623,21 +1626,23 @@ static int ntb_process_rxc(struct ntb_transport_qp *qp)
 	}
 
 	dma_rmb();
+	ver = le32_to_cpu(READ_ONCE(hdr->ver));
+	len = le32_to_cpu(READ_ONCE(hdr->len));
 
 	dev_dbg(&qp->ndev->pdev->dev, "qp %d: RX ver %u len %d flags %x\n",
-		qp->qp_num, hdr->ver, hdr->len, flags);
+		qp->qp_num, ver, len, flags);
 
 	if (flags & LINK_DOWN_FLAG) {
 		dev_dbg(&qp->ndev->pdev->dev, "link down flag set\n");
 		ntb_qp_link_down(qp);
-		hdr->flags = 0;
+		hdr->flags = cpu_to_le32(0);
 		return -EAGAIN;
 	}
 
-	if (hdr->ver != (u32)qp->rx_pkts) {
+	if (ver != (u32)qp->rx_pkts) {
 		dev_dbg(&qp->ndev->pdev->dev,
 			"version mismatch, expected %llu - got %u\n",
-			qp->rx_pkts, hdr->ver);
+			qp->rx_pkts, ver);
 		qp->rx_err_ver++;
 		return -EIO;
 	}
@@ -1652,10 +1657,10 @@ static int ntb_process_rxc(struct ntb_transport_qp *qp)
 	entry->rx_hdr = hdr;
 	entry->rx_index = qp->rx_index;
 
-	if (hdr->len > entry->len) {
+	if (len > entry->len) {
 		dev_dbg(&qp->ndev->pdev->dev,
 			"receive buffer overflow! Wanted %d got %d\n",
-			hdr->len, entry->len);
+			len, entry->len);
 		qp->rx_err_oflow++;
 
 		entry->len = -EIO;
@@ -1665,12 +1670,12 @@ static int ntb_process_rxc(struct ntb_transport_qp *qp)
 	} else {
 		dev_dbg(&qp->ndev->pdev->dev,
 			"RX OK index %u ver %u size %d into buf size %d\n",
-			qp->rx_index, hdr->ver, hdr->len, entry->len);
+			qp->rx_index, ver, len, entry->len);
 
-		qp->rx_bytes += hdr->len;
+		qp->rx_bytes += len;
 		qp->rx_pkts++;
 
-		entry->len = hdr->len;
+		entry->len = len;
 
 		ntb_async_rx(entry, offset);
 	}
@@ -2490,7 +2495,9 @@ EXPORT_SYMBOL_GPL(ntb_transport_max_size);
 unsigned int ntb_transport_tx_free_entry(struct ntb_transport_qp *qp)
 {
 	unsigned int head = qp->tx_index;
-	unsigned int tail = qp->remote_rx_info->entry;
+	unsigned int tail;
+
+	tail = le32_to_cpu(READ_ONCE(qp->remote_rx_info->entry));
 
 	return tail >= head ? tail - head : qp->tx_max_entry + tail - head;
 }
-- 
2.51.0

[PATCH net-next v3 3/4] 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.

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v3:
  - Add Dave's Reviewed-by tag.

 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 2c04be6d61a8..d03cbda1637d 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;
@@ -278,7 +278,7 @@ 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);
+	rc = ntb_transport_tx_enqueue(q->qp, skb, skb->data, skb->len, 0);
 	if (rc) {
 		if (rc == -EAGAIN || rc == -EBUSY) {
 			netif_stop_subqueue(ndev, q->qid);
diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
index 3f497a62673f..197376376bff 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 {
 	__le32 ver;
 	__le32 len;
@@ -1436,6 +1440,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);
@@ -1451,13 +1456,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);
 	}
@@ -1656,6 +1662,7 @@ static int ntb_process_rxc(struct ntb_transport_qp *qp)
 
 	entry->rx_hdr = hdr;
 	entry->rx_index = qp->rx_index;
+	entry->flags = flags & DESC_META_MASK;
 
 	if (len > entry->len) {
 		dev_dbg(&qp->ndev->pdev->dev,
@@ -2337,6 +2344,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
@@ -2345,12 +2353,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 (!qp->link_is_up)
@@ -2368,7 +2376,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 v3 4/4] 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>
---
Changes in v3:
  - Count RX checksum failures as errors only (Jakub)
  - Reject csum_start below ETH_HLEN on RX and fall back to software on
    TX (Jakub, Sashiko)

 drivers/net/ntb_netdev.c | 93 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 82 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
index d03cbda1637d..de8c4f320614 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,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);
 
 	netdev_dbg(ndev, "Event %x, Link %x, qp %u\n", link_is_up,
 		   ntb_transport_link_query(q->qp), q->qid);
@@ -122,15 +140,25 @@ static void ntb_netdev_event_handler(void *data, int link_is_up)
 	ntb_netdev_update_carrier(dev);
 }
 
+static void ntb_netdev_rx_stats_add(struct net_device *ndev,
+				    unsigned int len)
+{
+	struct pcpu_sw_netstats *tstats = this_cpu_ptr(ndev->tstats);
+	unsigned long flags;
+
+	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);
+}
+
 static void ntb_netdev_rx_handler(struct ntb_transport_qp *qp, void *qp_data,
 				  void *data, int len, unsigned int meta)
 {
 	struct ntb_netdev_queue *q = qp_data;
 	struct ntb_netdev *dev = q->ntdev;
-	struct pcpu_sw_netstats *tstats;
 	struct sk_buff *skb, *new_skb;
 	struct net_device *ndev;
-	unsigned long flags;
 	int rc;
 
 	ndev = dev->ndev;
@@ -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);
+
+	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;
+	}
+
+	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);
@@ -174,6 +213,13 @@ static void ntb_netdev_rx_handler(struct ntb_transport_qp *qp, void *qp_data,
 		DEV_STATS_INC(ndev, rx_errors);
 		DEV_STATS_INC(ndev, rx_fifo_errors);
 	}
+	return;
+
+rx_drop:
+	DEV_STATS_INC(ndev, rx_errors);
+	dev_kfree_skb_any(skb);
+	skb = new_skb;
+	goto enqueue_again;
 }
 
 static int __ntb_netdev_maybe_stop_tx(struct net_device *netdev,
@@ -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);
 	if (rc) {
 		if (rc == -EAGAIN || rc == -EBUSY) {
 			netif_stop_subqueue(ndev, q->qid);
@@ -301,6 +358,18 @@ static netdev_tx_t ntb_netdev_start_xmit(struct sk_buff *skb,
 	return NETDEV_TX_OK;
 }
 
+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_checksum_start_offset(skb) < ETH_HLEN ||
+	     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);
@@ -451,6 +520,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,
 };
@@ -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;
 	ndev->watchdog_timeo = msecs_to_jiffies(NTB_TX_TIMEOUT_MS);
 
 	eth_random_addr(ndev->perm_addr);
-- 
2.51.0

Re: [PATCH net-next v3 1/4] NTB: ntb_transport: Order RX descriptor reads after completion

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Inbound memory windows are freed during link down cleanup without synchronizing with the receive tasklet or terminating in-flight DMA operations.
- [High] In-flight DMA transactions are not terminated on link down, causing stale DMA completions to corrupt the queue state of the subsequent link session.
- [High] An unhandled version mismatch in `ntb_process_rxc` causes a permanent stall of the receive queue.
- [High] In-flight DMA descriptors and their associated `ntb_queue_entry` structures are permanently leaked during QP teardown.
- [High] Broken DMA channel synchronization causes false timeouts and abandoned TX transactions during teardown.
- [High] Client data buffers (SKBs) are silently leaked when `tx_offl_q` and `rx_post_q` entries are freed during teardown.
- [High] DMA abort fallback copies the wrong memory payload due to improper index tracking.
- [Medium] Lockless modification of `entry->len` and `entry->flags` lacks memory barriers, potentially hiding overflow errors from the client.
--

commit 3308bb067cf3d0b798b94e6f70b47a7f9378fbae
Author: Koichiro Den <den@valinux.co.jp>
NTB: ntb_transport: Order RX descriptor reads after completion

This commit introduces a memory barrier after observing the DESC_DONE_FLAG in
ntb_process_rxc() to ensure that the descriptor and payload reads are properly
ordered on weakly ordered CPUs. It reads the completion word once and uses the
saved value for subsequent transport flag checks.

Link: https://lore.kernel.org/r/20260815032932.151F11F000E9@smtp.kernel.org/

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index f9caa1a653c5b..74f4f8c1c7be8 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -1609,21 +1609,25 @@ static int ntb_process_rxc(struct ntb_transport_qp *qp)
>  {
>  	struct ntb_payload_header *hdr;
>  	struct ntb_queue_entry *entry;
> +	unsigned int flags;
>  	void *offset;
>  
>  	offset = qp->rx_buff + qp->rx_max_frame * qp->rx_index;
>  	hdr = offset + qp->rx_max_frame - sizeof(struct ntb_payload_header);
>  
> -	dev_dbg(&qp->ndev->pdev->dev, "qp %d: RX ver %u len %d flags %x\n",
> -		qp->qp_num, hdr->ver, hdr->len, hdr->flags);
> -
> -	if (!(hdr->flags & DESC_DONE_FLAG)) {
> +	flags = READ_ONCE(hdr->flags);
> +	if (!(flags & DESC_DONE_FLAG)) {
>  		dev_dbg(&qp->ndev->pdev->dev, "done flag not set\n");
>  		qp->rx_ring_empty++;
>  		return -EAGAIN;
>  	}
>  
> -	if (hdr->flags & LINK_DOWN_FLAG) {
> +	dma_rmb();
> +
> +	dev_dbg(&qp->ndev->pdev->dev, "qp %d: RX ver %u len %d flags %x\n",
> +		qp->qp_num, hdr->ver, hdr->len, flags);
> +
> +	if (flags & LINK_DOWN_FLAG) {
>  		dev_dbg(&qp->ndev->pdev->dev, "link down flag set\n");
>  		ntb_qp_link_down(qp);
>  		hdr->flags = 0;

[Severity: High]
This isn't a bug introduced by this patch, but does an unhandled version
mismatch in ntb_process_rxc() cause a permanent stall?

In ntb_process_rxc():
	if (ver != (u32)qp->rx_pkts) {
		dev_dbg(&qp->ndev->pdev->dev,
			"version mismatch, expected %llu - got %u\n",
			qp->rx_pkts, ver);
		qp->rx_err_ver++;
		return -EIO;
	}

If this check fails, the function returns -EIO and the caller breaks out
of its loop without advancing qp->rx_index or clearing DESC_DONE_FLAG.
Will the next tasklet execution evaluate the exact same descriptor,
encounter the same error, and loop indefinitely?

[Severity: Medium]
This is a pre-existing issue, but do the lockless modifications of
entry->len and flags in ntb_process_rxc() require a memory barrier?

In ntb_process_rxc() during a buffer overflow event:
		entry->len = -EIO;
		entry->flags |= DESC_DONE_FLAG;

		ntb_complete_rxc(qp);

These fields are set locklessly on an entry that is already publicly
accessible in rx_post_q. Without an smp_wmb(), could a concurrent
execution of ntb_complete_rxc() observe the DESC_DONE_FLAG but read a
stale entry->len, falsely reporting a successful transfer to the client?

[Severity: High]
This is a pre-existing issue, but does ntb_qp_link_cleanup() fail to
terminate in-flight DMA operations during a link reset?

In ntb_qp_link_cleanup():
	cancel_delayed_work_sync(&qp->link_work);
	ntb_qp_link_down_reset(qp);

	if (qp->event_handler)

If a pending DMA transaction completes asynchronously after this reset, it
triggers the callback, appending to qp->rx_post_q and calling
ntb_complete_rxc():

	entry->rx_hdr->flags = cpu_to_le32(0);
	iowrite32(entry->rx_index, &qp->rx_info->entry);

Could this stale DMA completion overwrite the reset state with an old
index, corrupting the peer's calculation of available free space and
permanently halting communication?

[Severity: High]
This isn't a bug introduced by this patch, but are in-flight DMA descriptors
and their ntb_queue_entry structures permanently leaked during QP teardown?

In ntb_transport_free_queue():
		dma_sync_wait(chan, qp->last_cookie);
		dmaengine_terminate_all(chan);

When dmaengine_terminate_all() aborts pending transfers, the associated
callbacks are not executed. Since entries are removed from qp->tx_free_q
during submission and only restored by the callback, do these active queue
entries leak permanently?

[Severity: High]
This is a pre-existing issue, but does the TX channel teardown wait on the
wrong DMA cookie?

In ntb_transport_free_queue(), the code synchronizes on the TX channel:
		dma_sync_wait(chan, qp->last_cookie);

However, qp->last_cookie is exclusively updated in ntb_async_rx_submit():
	qp->last_cookie = cookie;

If the RX cookie exceeds the TX channel's highest cookie, will
dma_sync_wait() erroneously timeout and forcefully abort healthy TX
transactions?

[Severity: High]
This isn't a bug introduced by this patch, but does ntb_transport_free_queue()
silently leak client data buffers (like SKBs) when freeing entries?

	while ((entry = ntb_list_rm(&qp->ntb_rx_q_lock, &qp->rx_post_q))) {
		dev_warn(&pdev->dev, "Freeing item from non-empty rx_post_q\n");
		kfree(entry);
	}

And similarly for TX offload:
	while ((entry = ntb_list_rm(&qp->ntb_tx_offl_q_lock, &qp->tx_offl_q)))
		kfree(entry);

Since the driver nullifies the handlers before this loop, and freeing the
entry doesn't reclaim entry->cb_data, are these client buffers permanently
leaked?

[Severity: High]
This is a pre-existing issue, but does the CPU fallback copy the wrong
memory payload during a DMA abort?

In ntb_rx_copy_callback():
			void *offset = qp->rx_buff + qp->rx_max_frame *
					qp->rx_index;

This computes the source offset using the globally advanced qp->rx_index.
Because qp->rx_index is advanced globally in ntb_process_rxc() right
after DMA submission, won't it point to a future, unrelated slot by the
time an error callback fires? Should this use the saved entry->rx_index
instead?

[Severity: Critical]
This isn't a bug introduced by this patch, but are inbound memory windows
freed without synchronizing with the receive tasklet or terminating DMA?

In ntb_transport_link_cleanup():
	for (i = 0; i < nt->mw_count; i++)
		ntb_free_mw(nt, i);

This unmaps and frees the DMA coherent memory windows, but leaves the
receive tasklet active.

In ntb_transport_rxc_db():
	for (i = 0; i < qp->rx_max_entry; i++) {
		rc = ntb_process_rxc(qp);
		if (rc)
			break;
	}

This loop executes unconditionally, proceeding to dereference qp->rx_buff
in ntb_process_rxc() before checking qp->active. Can this cause a
Use-After-Free memory access or IOMMU faults if the link goes down while
tasklets or DMA are active?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904052134.2970111-1-den@valinux.co.jp?part=1

Re: [PATCH net-next v3 2/4] NTB: ntb_transport: Use little-endian shared fields

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Permanent RX queue stall on version mismatch error.
- [High] Missing bounds check against `rx_max_frame` allows an attacker-controlled peer to trigger an out-of-bounds read of kernel memory.
- [Medium] The queue size calculations do not guarantee that `rx_size` and `tx_size` are appropriately aligned, leading to unaligned MMIO writes and coherent memory accesses.
--

commit 8632f1194f420fac4999a48cdd0e2e9e59e54697
Author: Koichiro Den <den@valinux.co.jp>

NTB: ntb_transport: Use little-endian shared fields

ntb_transport writes payload headers and the RX ring tail with
iowrite32(), but reads peer-written copies from coherent memory as native
integers. The values are therefore byte-swapped when read on a big-endian
system.

Mark the shared fields as __le32 and convert coherent-memory accesses
accordingly. Read hdr->ver and hdr->len once so their checks and later
uses see the same values.

Link: https://lore.kernel.org/r/20260815032932.151F11F000E9@smtp.kernel.org/
Link: https://lore.kernel.org/r/20260818064951.7EA231F000E9@smtp.kernel.org/

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index 74f4f8c1c7be8..3f497a62673fd 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c

[ ... ]

> @@ -633,7 +634,7 @@ static int ntb_transport_setup_qp_mw(struct ntb_transport_ctx *nt,
>  		qp->rx_alloc_entry++;
>  	}
>  
> -	qp->remote_rx_info->entry = qp->rx_max_entry - 1;
> +	qp->remote_rx_info->entry = cpu_to_le32(qp->rx_max_entry - 1);

[Severity: Medium]
This isn't a bug introduced by this patch, but the local receive buffer slice
size rx_size is calculated earlier in ntb_transport_setup_qp_mw() as:
 
	rx_size = (unsigned int)mw->xlat_size / num_qps_mw;
 
If num_qps_mw does not cleanly divide the window size, the resulting rx_size
can be unaligned.
 
Does this misalign the qp->remote_rx_info pointer which resides at the end
of this buffer, as well as the frame boundaries qp->rx_max_frame? If so, on
strict architectures, could the unaligned READ_ONCE() and iowrite32()
accesses to the payload headers and shared fields trigger an alignment fault?

>  
>  	/* setup the hdr offsets with 0's */
>  	for (i = 0; i < qp->rx_max_entry; i++) {

[ ... ]

> @@ -1623,21 +1626,23 @@ static int ntb_process_rxc(struct ntb_transport_qp *qp)
>  	}
>  
>  	dma_rmb();
> +	ver = le32_to_cpu(READ_ONCE(hdr->ver));
> +	len = le32_to_cpu(READ_ONCE(hdr->len));
>  
>  	dev_dbg(&qp->ndev->pdev->dev, "qp %d: RX ver %u len %d flags %x\n",
> -		qp->qp_num, hdr->ver, hdr->len, flags);
> +		qp->qp_num, ver, len, flags);
>  
>  	if (flags & LINK_DOWN_FLAG) {
>  		dev_dbg(&qp->ndev->pdev->dev, "link down flag set\n");
>  		ntb_qp_link_down(qp);
> -		hdr->flags = 0;
> +		hdr->flags = cpu_to_le32(0);
>  		return -EAGAIN;
>  	}
>  
> -	if (hdr->ver != (u32)qp->rx_pkts) {
> +	if (ver != (u32)qp->rx_pkts) {
>  		dev_dbg(&qp->ndev->pdev->dev,
>  			"version mismatch, expected %llu - got %u\n",
> -			qp->rx_pkts, hdr->ver);
> +			qp->rx_pkts, ver);
>  		qp->rx_err_ver++;
>  		return -EIO;
>  	}

[Severity: High]
This is a pre-existing issue, but if the received payload header version does
not match the expected qp->rx_pkts in ntb_process_rxc(), the function
increments the error counter and returns -EIO without clearing the
DESC_DONE_FLAG or advancing qp->rx_index.

Since the caller aborts processing upon the error, the next time the queue is
processed, it will attempt to process the same packet at the same index, fail
the version check again, and return -EIO.

Can this lead to a permanent stall of the RX queue until the link is
physically reset?

> @@ -1652,10 +1657,10 @@ static int ntb_process_rxc(struct ntb_transport_qp *qp)
>  	entry->rx_hdr = hdr;
>  	entry->rx_index = qp->rx_index;
>  
> -	if (hdr->len > entry->len) {
> +	if (len > entry->len) {

[Severity: High]
This isn't a bug introduced by this patch, but this bounds check in
ntb_process_rxc() only validates that the received length len fits within the
client-provided buffer (entry->len).

Because entry->len is derived from the local memory window size
(tx_max_frame), it can be larger than the receive frame size (rx_max_frame)
negotiated by the peer.

If a malicious peer provides a large length that is greater than the
negotiated rx_max_frame but still less than entry->len, could this bypass the
check and cause ntb_memcpy_rx() or ntb_async_rx() below to copy memory
out-of-bounds from the qp->rx_buff DMA region, leaking adjacent kernel memory
to the network stack?

>  		dev_dbg(&qp->ndev->pdev->dev,
>  			"receive buffer overflow! Wanted %d got %d\n",
> -			hdr->len, entry->len);
> +			len, entry->len);
>  		qp->rx_err_oflow++;
>  
>  		entry->len = -EIO;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904052134.2970111-1-den@valinux.co.jp?part=2

Re: [PATCH net-next v3 3/4] NTB: ntb_transport: Add per-payload client metadata

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Unaligned frame sizes lead to unaligned I/O writes and memory reads of the payload header.
--

commit 91aff7efee260ec323eefdc5885fbc094f6643c8
Author: Koichiro Den <den@valinux.co.jp>

NTB: ntb_transport: Add per-payload client metadata

This commit extends ntb_transport to carry 24 bits of opaque client metadata
alongside payload bytes. The metadata is packed into the upper bits of the
payload header's flags field and exposed via the transmit and receive
interfaces. It also adapts ntb_netdev to use the new interfaces without
metadata.

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index 3f497a62673fd..197376376bff5 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c

[ ... ]

> @@ -1656,6 +1662,7 @@ static int ntb_process_rxc(struct ntb_transport_qp *qp)
>  
>  	entry->rx_hdr = hdr;
>  	entry->rx_index = qp->rx_index;
> +	entry->flags = flags & DESC_META_MASK;

[Severity: High]
This is a pre-existing issue, but does the calculation of the frame sizes
guarantee 4-byte alignment for the payload header?

In ntb_transport_setup_qp_mw(), qp->rx_max_frame and qp->tx_max_frame are
calculated using rx_size / 2 and tx_size / 2 without forcing alignment.

This means the hdr pointer in ntb_process_rxc() can be unaligned when
reading flags:

drivers/ntb/ntb_transport.c:ntb_process_rxc() {
	...
	offset = qp->rx_buff + qp->rx_max_frame * qp->rx_index;
	hdr = offset + qp->rx_max_frame - sizeof(struct ntb_payload_header);

	flags = le32_to_cpu(READ_ONCE(hdr->flags));
	...
}

Executing READ_ONCE() on an unaligned address can trigger alignment faults
(kernel panics) depending on the architecture.

Additionally, this unaligned frame size also leads to unaligned 32-bit PCIe
I/O writes in ntb_async_tx():

drivers/ntb/ntb_transport.c:ntb_async_tx() {
	...
	iowrite32(entry->len, &hdr->len);
	...
}

Unaligned memory-mapped PCIe addresses can trigger fatal hardware errors
(Unsupported Request). Can we ensure qp->rx_max_frame and qp->tx_max_frame
are properly aligned?

>  
>  	if (len > entry->len) {
>  		dev_dbg(&qp->ndev->pdev->dev,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904052134.2970111-1-den@valinux.co.jp?part=3

Re: [PATCH net-next v3 3/4] NTB: ntb_transport: Add per-payload client metadata

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 (1) · Medium: 0 · Low: 1

- [Low] The kernel-doc added for ntb_transport_tx_enqueue() documents the
  new parameter only as '@meta: client metadata to send with the buffer'…

Pre-existing issues:
- [High] Missing barrier / lockless publication of struct ntb_queue_entry
  state in ntb_process_rxc(): entry->rx_hdr, entry->rx_index,…

--

NTB: ntb_transport: Add per-payload client metadata

This adds opaque per-payload client metadata to ntb_transport by carrying
it in the upper 24 bits of the existing payload header flags field, exposed
through ntb_transport_tx_enqueue() and the rx_handler callback. The low
byte stays reserved for transport flags, and ntb_netdev is adapted to the
new interfaces without using metadata.

A couple of questions below, mostly about the receive-side publication of
struct ntb_queue_entry that the new metadata now rides along with.

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index 3f497a62673fd..197376376bff5 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c

[ ... ]

> @@ -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 {
>  	__le32 ver;
>  	__le32 len;
> @@ -1436,6 +1440,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);
> @@ -1451,13 +1456,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);
>  	}
> @@ -1656,6 +1662,7 @@ static int ntb_process_rxc(struct ntb_transport_qp *qp)
>  
>  	entry->rx_hdr = hdr;
>  	entry->rx_index = qp->rx_index;
> +	entry->flags = flags & DESC_META_MASK;
>  
>  	if (len > entry->len) {
>  		dev_dbg(&qp->ndev->pdev->dev,

[Severity: High]
This isn't a bug introduced by this patch, but the new entry->flags store
joins a group of fields in ntb_process_rxc() that are written after the
entry has already been published onto qp->rx_post_q:

	entry = ntb_list_mv(&qp->ntb_rx_q_lock, &qp->rx_pend_q, &qp->rx_post_q);
	...
	entry->rx_hdr = hdr;
	entry->rx_index = qp->rx_index;
	entry->flags = flags & DESC_META_MASK;

ntb_list_mv() drops qp->ntb_rx_q_lock before returning, so those three
stores run with no lock held. In the receive-overflow branch that follows,
the bit which publishes them is set with nothing in between:

		entry->len = -EIO;
		entry->flags |= DESC_DONE_FLAG;

		ntb_complete_rxc(qp);

Meanwhile ntb_complete_rxc() can already be executing on another CPU for a
preceding entry of the same qp, out of the RX DMA completion callback:

ntb_rx_copy_callback()
	entry->flags |= DESC_DONE_FLAG;
	ntb_complete_rxc(entry->qp);

That call takes qp->ntb_rx_q_lock, pops the completed entry and then looks
at the next entry on rx_post_q, i.e. the one the rxc tasklet is still
filling in, testing the done bit and consuming exactly those fields (now
including the new meta value):

		if (!(entry->flags & DESC_DONE_FLAG))
			break;

		entry->rx_hdr->flags = cpu_to_le32(0);
		iowrite32(entry->rx_index, &qp->rx_info->entry);

		cb_data = entry->cb_data;
		len = entry->len;
		meta = entry->flags >> DESC_META_SHIFT;

Can the DESC_DONE_FLAG store here become visible before the rx_hdr,
rx_index and len stores?  Note ntb_memcpy_rx() does have a wmb() before
reaching the done store:

	memcpy(buf, offset, len);

	/* Ensure that the data is fully copied out before clearing the flag */
	wmb();

	ntb_rx_copy_callback(entry, NULL);

while the overflow path in ntb_process_rxc() has no barrier at all.

Entries are allocated with kzalloc_node() in ntb_transport_create_queue()
and rx_hdr is never initialised there, so on an entry's first use a stale
read would be NULL, which would make

		entry->rx_hdr->flags = cpu_to_le32(0);

a NULL dereference, and iowrite32() would then push a stale rx_index into
qp->rx_info->entry, which the peer uses for ring flow control.

The concurrent completion context requires the RX DMA path (use_dma and a
payload at least copy_bytes), and the barrier-free done store is on the
peer-triggerable path where the peer sends a payload larger than the local
receive buffer.

Since the metadata bits share the flags word with DESC_DONE_FLAG, the read
of meta itself is ordered with the done test, so this patch does not
introduce the ordering gap; it adds one more store into, and one more
consumer of, an already unordered publication.

Would it make sense to fill in the entry fields before ntb_list_mv()
publishes the entry, or to add an explicit write barrier before the
DESC_DONE_FLAG store with a matching read barrier after the done test?

> @@ -2337,6 +2344,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

[Severity: Low]
This isn't a bug, but the kernel-doc for @meta in ntb_transport_tx_enqueue()
does not mention that values above NTB_TRANSPORT_MAX_META are rejected,
which the very next lines of the function enforce:

	if (!qp || !len || meta > NTB_TRANSPORT_MAX_META)
		return -EINVAL;

Similarly, the new meta argument of struct ntb_queue_handlers.rx_handler in
include/linux/ntb_transport.h carries no description of its width or of the
fact that peers which do not send metadata always deliver 0.  The
explanation of why the low byte is unavailable only exists as the private
comment in drivers/ntb/ntb_transport.c:

	/* Reserve the low byte for transport flags. */

which clients that only see the exported header do not get.  Could a short
note be added next to @meta and next to the rx_handler prototype?

> @@ -2345,12 +2353,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;

[ ... ]

> diff --git a/include/linux/ntb_transport.h b/include/linux/ntb_transport.h
> index 7243eb98a722e..9e807542b6c40 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);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904052134.2970111-1-den%40valinux.co.jp

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

[netdev-bot+sashiko]

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()).

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

Re: [PATCH net-next v3 3/4] NTB: ntb_transport: Add per-payload client metadata

[Jakub Kicinski]

On Fri,  4 Sep 2026 14:21:33 +0900 Koichiro Den wrote:
> +/* Reserve the low byte for transport flags. */
> +#define DESC_META_SHIFT		8
> +#define DESC_META_MASK		(~0U << DESC_META_SHIFT)

IMHO the code would be more readable if you defined this with 
one GENMASK and then used FILED_GET() / _PREP() / _MAX() on it

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

[Jakub Kicinski]

On Fri,  4 Sep 2026 14:21:34 +0900 Koichiro Den wrote:
> 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.

Would it make more sense to prepend something like a virtio_net_hdr
to the data so you can cheaply express more offloads?

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

[Koichiro Den]

On Thu, Sep 10, 2026 at 05:20:47PM -0700, Jakub Kicinski wrote:
> On Fri,  4 Sep 2026 14:21:34 +0900 Koichiro Den wrote:
> > 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.
> 
> Would it make more sense to prepend something like a virtio_net_hdr
> to the data so you can cheaply express more offloads?

Thanks for the suggestion. Yes, packing offload metadata into
ntb_payload_header::flags could become limiting in the future. We could prepend
a header like virtio_net_hdr to each packet and use one metadata bit to indicate
its presence.
(BTW, GSO hasn't been the focus here, given the large MTUs NTB can support, but
leaving room for it makes sense.)

Best regards,
Koichiro

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@kernel.org 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

Re: [PATCH net-next v3 3/4] NTB: ntb_transport: Add per-payload client metadata

[Koichiro Den]

On Thu, Sep 10, 2026 at 05:18:56PM -0700, Jakub Kicinski wrote:
> On Fri,  4 Sep 2026 14:21:33 +0900 Koichiro Den wrote:
> > +/* Reserve the low byte for transport flags. */
> > +#define DESC_META_SHIFT		8
> > +#define DESC_META_MASK		(~0U << DESC_META_SHIFT)
> 
> IMHO the code would be more readable if you defined this with 
> one GENMASK and then used FILED_GET() / _PREP() / _MAX() on it

Will do, thanks for the review!

Best regards,
Koichiro

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

[Paolo Abeni]

On 9/11/26 2:20 AM, Jakub Kicinski wrote:
> On Fri,  4 Sep 2026 14:21:34 +0900 Koichiro Den wrote:
>> 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.
> 
> Would it make more sense to prepend something like a virtio_net_hdr
> to the data so you can cheaply express more offloads?

FTR, I think it makes sense to consolidate similar code, with one possible
caveat: virtio_net is not spectacularly flexible in term of supporting
"new" stuff: if this use-case would need some special behavior, it could
be problematic.

AFAICS the intended usage is quite basic, so no big surprise expected, and
also I guess that only a serious attempt could tell...

/P