[PATCH net-next v4 00/10] net: ntb_netdev: Preserve checksum offload across NTB

[PATCH net-next v4 00/10] 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 NTB in an ntb_netdev header.
A per-payload metadata bit in ntb_transport marks its presence. Receive
support is exchanged at QP link-up through scratchpads. Peers without
capability exchange continue to use software checksumming. The feature
remains disabled by default and must be enabled explicitly on trusted
links. RX checksum offload settings stay unchanged while the interface
is up.

Best regards,
Koichiro
---
Changes in v4:
  - Move offload metadata into an ntb_netdev header (Jakub)
  - Add an RX entry completion ordering fix (Sashiko)
  - Learn checksum support from link events, not RX callbacks, and keep
    it per QP (Sashiko)
    https://lore.kernel.org/r/xsaocp32a6siggahziu4jqbwgmdasmp6v64pd5aujfkoxhiqws@n5luestlntzc/
  - Advertise receive support only with RXCSUM enabled. Drop
    partial-checksum frames when it is disabled (Sashiko)
  - Keep local QP link requests separate from peer readiness
  - Reject short RX frames before parsing them
  - Factor out RX packet and byte accounting
  - Add a transport API to exchange client capabilities at QP link-up
  - Keep NETIF_F_RXCSUM unchanged while the interface is up

Changes in v3:
  - Rebase after the related ntb_netdev 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)

v3: https://lore.kernel.org/r/20260904052134.2970111-1-den@valinux.co.jp/
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 (10):
  NTB: ntb_transport: Order RX descriptor reads after completion
  NTB: ntb_transport: Use little-endian shared fields
  NTB: ntb_transport: Order RX entry completion
  NTB: ntb_transport: Keep local QP link requests separate
  NTB: ntb_transport: Exchange client capabilities at link-up
  NTB: ntb_transport: Add per-payload client metadata
  net: ntb_netdev: Reject short RX frames
  net: ntb_netdev: Factor out RX statistics update
  net: ntb_netdev: Introduce an optional packet header, ntb_netdev_hdr
  net: ntb_netdev: Preserve CHECKSUM_PARTIAL across NTB

 drivers/net/ntb_netdev.c      | 191 ++++++++++++++++++++++++++++------
 drivers/ntb/ntb_transport.c   | 177 +++++++++++++++++++++++--------
 include/linux/ntb_transport.h |  12 ++-
 3 files changed, 303 insertions(+), 77 deletions(-)


base-commit: 879e280b8486d4612ad1aa050d6fada2dd80cf1c
prerequisite-patch-id: 0000000000000000000000000000000000000000
-- 
2.51.0

[PATCH net-next v4 01/10] 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 v4:
  - No changes.

 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 v4 02/10] 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 v4:
  - No changes.

 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 v4 03/10] NTB: ntb_transport: Order RX entry completion

[Koichiro Den]

RX entries are added to rx_post_q before their fields are filled in.
The overflow path sets DONE without a write barrier, and
ntb_complete_rxc() has no read barrier after checking DONE. A concurrent
completion can therefore consume stale entry fields.

Publish DONE with release ordering and check it with acquire ordering in
ntb_complete_rxc(). Use the same publication rule in the copy callback.

Fixes: da2e5ae56164 ("NTB: Fix ntb_transport out-of-order RX update")
Cc: stable@vger.kernel.org
Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904052134.2970111-1-den%40valinux.co.jp?part=3
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v4:
  - New patch (Sashiko)

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

diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
index 3f497a62673f..b69e8ac8047d 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -1443,7 +1443,8 @@ static void ntb_complete_rxc(struct ntb_transport_qp *qp)
 	while (!list_empty(&qp->rx_post_q)) {
 		entry = list_first_entry(&qp->rx_post_q,
 					 struct ntb_queue_entry, entry);
-		if (!(entry->flags & DESC_DONE_FLAG))
+		/* DONE publishes the entry fields and copied data. */
+		if (!(smp_load_acquire(&entry->flags) & DESC_DONE_FLAG))
 			break;
 
 		entry->rx_hdr->flags = cpu_to_le32(0);
@@ -1496,7 +1497,8 @@ static void ntb_rx_copy_callback(void *data,
 		}
 	}
 
-	entry->flags |= DESC_DONE_FLAG;
+	/* Pair with the acquire load in ntb_complete_rxc(). */
+	smp_store_release(&entry->flags, entry->flags | DESC_DONE_FLAG);
 
 	ntb_complete_rxc(entry->qp);
 }
@@ -1664,7 +1666,8 @@ static int ntb_process_rxc(struct ntb_transport_qp *qp)
 		qp->rx_err_oflow++;
 
 		entry->len = -EIO;
-		entry->flags |= DESC_DONE_FLAG;
+		/* Pair with the acquire load in ntb_complete_rxc(). */
+		smp_store_release(&entry->flags, entry->flags | DESC_DONE_FLAG);
 
 		ntb_complete_rxc(qp);
 	} else {
-- 
2.51.0

[PATCH net-next v4 04/10] NTB: ntb_transport: Keep local QP link requests separate

[Koichiro Den]

The QP_LINKS handshake is a smart way for both sides to converge on
link-up without peer SPAD reads (MRd). But copying the peer's bitmap
back mixes up local requests and peer readiness when multiple QPs are
used.

When QPs are shut down back-to-back, each update reads the local
scratchpad and can restore a bit just cleared in the peer scratchpad.
Likewise, a peer's QP1 worker can echo our QP0 request before its own
QP0 worker runs, making us report QP0 up too early.

Keep our up requests in a bitmap in ntb_transport_ctx. Serialize bitmap
updates and peer writes under one lock. Clear it on transport cleanup
so each QP advertises itself again after reconnecting.

This bitmap handling dates back to commit fce8a7bb5b4b ("PCI-Express
Non-Transparent Bridge Support"), but ntb_netdev multi-queue support
exposed the problem in practice, hence the Fixes tag below.

Fixes: 24d9e73c7e00 ("net: ntb_netdev: Support ethtool channels for multi-queue")
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v4:
  - New patch for a pre-existing issue found while preparing v4.

 drivers/ntb/ntb_transport.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
index b69e8ac8047d..0b47285ef48b 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -244,6 +244,9 @@ struct ntb_transport_ctx {
 	unsigned int qp_count;
 	u64 qp_bitmap;
 	u64 qp_bitmap_free;
+	/* Serialize request updates and peer writes. */
+	spinlock_t up_request_lock;
+	u32 up_request;
 
 	bool use_msi;
 	unsigned int msi_spad_offset;
@@ -976,6 +979,9 @@ static void ntb_transport_link_cleanup(struct ntb_transport_ctx *nt)
 	if (!nt->link_is_up)
 		cancel_delayed_work_sync(&nt->link_work);
 
+	scoped_guard(spinlock, &nt->up_request_lock)
+		nt->up_request = 0;
+
 	for (i = 0; i < nt->mw_count; i++)
 		ntb_free_mw(nt, i);
 
@@ -1113,6 +1119,21 @@ static void ntb_transport_link_work(struct work_struct *work)
 				      msecs_to_jiffies(NTB_LINK_DOWN_TIMEOUT));
 }
 
+static void ntb_qp_up_request(struct ntb_transport_qp *qp, bool up)
+{
+	struct ntb_transport_ctx *nt = qp->transport;
+
+	guard(spinlock)(&nt->up_request_lock);
+
+	if (up)
+		nt->up_request |= BIT(qp->qp_num);
+	else
+		nt->up_request &= ~BIT(qp->qp_num);
+
+	/* Update the peer's view of our requests. */
+	ntb_peer_spad_write(nt->ndev, PIDX, QP_LINKS, nt->up_request);
+}
+
 static void ntb_qp_link_work(struct work_struct *work)
 {
 	struct ntb_transport_qp *qp = container_of(work,
@@ -1126,7 +1147,7 @@ static void ntb_qp_link_work(struct work_struct *work)
 
 	val = ntb_spad_read(nt->ndev, QP_LINKS);
 
-	ntb_peer_spad_write(nt->ndev, PIDX, QP_LINKS, val | BIT(qp->qp_num));
+	ntb_qp_up_request(qp, true);
 
 	/* query remote spad for qp ready bits */
 	dev_dbg_ratelimited(&pdev->dev, "Remote QP link status = %x\n", val);
@@ -1361,6 +1382,7 @@ static int ntb_transport_probe(struct ntb_client *self, struct ntb_dev *ndev)
 			goto err2;
 	}
 
+	spin_lock_init(&nt->up_request_lock);
 	mutex_init(&nt->link_event_lock);
 	INIT_DELAYED_WORK(&nt->link_work, ntb_transport_link_work);
 	INIT_WORK(&nt->link_cleanup, ntb_transport_link_cleanup_work);
@@ -2412,16 +2434,12 @@ EXPORT_SYMBOL_GPL(ntb_transport_link_up);
  */
 void ntb_transport_link_down(struct ntb_transport_qp *qp)
 {
-	int val;
-
 	if (!qp)
 		return;
 
 	qp->client_ready = false;
 
-	val = ntb_spad_read(qp->ndev, QP_LINKS);
-
-	ntb_peer_spad_write(qp->ndev, PIDX, QP_LINKS, val & ~BIT(qp->qp_num));
+	ntb_qp_up_request(qp, false);
 
 	if (qp->link_is_up)
 		ntb_send_link_down(qp);
-- 
2.51.0

Re: [PATCH net-next v4 04/10] NTB: ntb_transport: Keep local QP link requests separate

[netdev-bot+sashiko]

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

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

- [Medium] A cleared QP up-request can be re-asserted by the QP's own
  link_work, leaving a closed QP advertised as 'up' in the peer's…

--

NTB: ntb_transport: Keep local QP link requests separate

This moves the local QP up-request bitmap out of the local scratchpad into
a new nt->up_request field, serializing the read-modify-write and the peer
QP_LINKS write under a new nt->up_request_lock. The bitmap is cleared in
ntb_transport_link_cleanup() so each QP re-advertises after reconnecting.

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index b69e8ac8047df..0b47285ef48b6 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c

[ ... ]

> @@ -1113,6 +1119,21 @@ static void ntb_transport_link_work(struct work_struct *work)
>  				      msecs_to_jiffies(NTB_LINK_DOWN_TIMEOUT));
>  }
>  
> +static void ntb_qp_up_request(struct ntb_transport_qp *qp, bool up)
> +{
> +	struct ntb_transport_ctx *nt = qp->transport;
> +
> +	guard(spinlock)(&nt->up_request_lock);
> +
> +	if (up)
> +		nt->up_request |= BIT(qp->qp_num);
> +	else
> +		nt->up_request &= ~BIT(qp->qp_num);
> +
> +	/* Update the peer's view of our requests. */
> +	ntb_peer_spad_write(nt->ndev, PIDX, QP_LINKS, nt->up_request);
> +}
> +
>  static void ntb_qp_link_work(struct work_struct *work)
>  {
>  	struct ntb_transport_qp *qp = container_of(work,
> @@ -1126,7 +1147,7 @@ static void ntb_qp_link_work(struct work_struct *work)
>  
>  	val = ntb_spad_read(nt->ndev, QP_LINKS);
>  
> -	ntb_peer_spad_write(nt->ndev, PIDX, QP_LINKS, val | BIT(qp->qp_num));
> +	ntb_qp_up_request(qp, true);

[Severity: Medium]
The up/down decision is still made by the caller outside up_request_lock,
so the last writer wins.  Can a closed QP end up advertised as up again
here?

ntb_qp_link_work() calls ntb_qp_up_request(qp, true) unconditionally, with
no test of qp->client_ready, while ntb_transport_link_down() clears the bit
first and only cancels the worker in the qp->link_is_up == false branch:

drivers/ntb/ntb_transport.c:ntb_transport_link_down() {
	qp->client_ready = false;

	ntb_qp_up_request(qp, false);

	if (qp->link_is_up)
		ntb_send_link_down(qp);
	else
		cancel_delayed_work_sync(&qp->link_work);
}

So with qp->link_is_up true, a queued or in-flight qp->link_work can run
after the clear and re-set BIT(qp->qp_num) in nt->up_request, republishing
it to the peer's QP_LINKS.

There is also a repeating path through the cleanup worker:

drivers/ntb/ntb_transport.c:ntb_qp_link_cleanup_work() {
	ntb_qp_link_cleanup(qp);

	if (nt->link_is_up)
		schedule_delayed_work(&qp->link_work,
				      msecs_to_jiffies(NTB_LINK_DOWN_TIMEOUT));
}

nt->link_is_up is set false only in ntb_transport_probe(), so after the
peer's LINK_DOWN payload is consumed (ntb_process_rxc() -> ntb_qp_link_down()
-> schedule_work(&qp->link_cleanup)) the QP's link_work keeps re-arming every
NTB_LINK_DOWN_TIMEOUT and keeps re-advertising a QP whose client already went
down.  Once that happens, is there any remaining publisher that would clear
the bit again short of a transport-wide cleanup?

The peer would then declare its side of the QP up and transmit into a queue
whose client is closed, and those frames get dropped.

Would it be more robust to exclude qp->link_work unconditionally (cancel or
flush before clearing), or to evaluate qp->client_ready inside
ntb_qp_up_request() under up_request_lock, rather than only making the
read-modify-write atomic?

For reference, the later patch in this series ("NTB: ntb_transport: Exchange
client capabilities at link-up") adds

	if (!smp_load_acquire(&qp->client_ready))
		return;

before the ntb_qp_up_request(qp, true) call and moves the
cancel_delayed_work_sync() ahead of the clear, which removes the repeating
cleanup-worker path.  A worker that already passed that client_ready load
can still publish the bit after ntb_transport_link_down() cleared it when
qp->link_is_up is true, since that branch skips the cancel.

>  	/* query remote spad for qp ready bits */
>  	dev_dbg_ratelimited(&pdev->dev, "Remote QP link status = %x\n", val);

[ ... ]

> @@ -2412,16 +2434,12 @@ EXPORT_SYMBOL_GPL(ntb_transport_link_up);
>   */
>  void ntb_transport_link_down(struct ntb_transport_qp *qp)
>  {
> -	int val;
> -
>  	if (!qp)
>  		return;
>  
>  	qp->client_ready = false;
>  
> -	val = ntb_spad_read(qp->ndev, QP_LINKS);
> -
> -	ntb_peer_spad_write(qp->ndev, PIDX, QP_LINKS, val & ~BIT(qp->qp_num));
> +	ntb_qp_up_request(qp, false);
>  
>  	if (qp->link_is_up)
>  		ntb_send_link_down(qp);

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

[PATCH net-next v4 05/10] NTB: ntb_transport: Exchange client capabilities at link-up

[Koichiro Den]

Clients need to learn peer capabilities before sending data. Exchange
opaque 31-bit capabilities during QP link setup and pass them to the
client with the link-up event.

Use one scratchpad per QP after the MW sizes and MSI descriptors. The
top bit marks a valid advertisement. Write capabilities to the peer
before advertising QP readiness. Read peer capabilities from the local
scratchpad once the peer reports ready.

Clear the local capability scratchpad at probe, not on QP open or reset,
so early peer advertisements are not lost.
Withdraw advertisements on link-down. The existing transport link
cleanup clears all local scratchpads.

The transport version 4 layout is kept as-is, including MSI slots even
without local MSI. Report zero capabilities without extra retries for
legacy peers or QPs without spare scratchpads.

Adapt ntb_netdev to the API without using capabilities yet.

Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v4:
  - New patch to address Sashiko's feedback:
    https://lore.kernel.org/r/xsaocp32a6siggahziu4jqbwgmdasmp6v64pd5aujfkoxhiqws@n5luestlntzc/

For reviewers: we could also protect qp->client_ready and qp->local_caps
with ntb_rx_q_lock. I find the current approach easier to read, but I'm
happy to change it if preferred.

 drivers/net/ntb_netdev.c      |  8 ++---
 drivers/ntb/ntb_transport.c   | 60 +++++++++++++++++++++++++++++++----
 include/linux/ntb_transport.h |  7 ++--
 3 files changed, 62 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
index 7a0d5e892a1a..869fc9a7f9e8 100644
--- a/drivers/net/ntb_netdev.c
+++ b/drivers/net/ntb_netdev.c
@@ -101,7 +101,7 @@ static int ntb_netdev_queue_rx_fill(struct net_device *ndev,
 	return 0;
 }
 
-static void ntb_netdev_event_handler(void *data, int link_is_up)
+static void ntb_netdev_event_handler(void *data, int link_is_up, u32 peer_caps)
 {
 	struct ntb_netdev_queue *q = data;
 	struct ntb_netdev *dev = q->ntdev;
@@ -346,7 +346,7 @@ static int ntb_netdev_open(struct net_device *ndev)
 	netif_tx_stop_all_queues(ndev);
 
 	for (q = 0; q < dev->num_queues; q++)
-		ntb_transport_link_up(dev->queues[q].qp);
+		ntb_transport_link_up(dev->queues[q].qp, 0);
 
 	return 0;
 
@@ -430,7 +430,7 @@ static int ntb_netdev_change_mtu(struct net_device *ndev, int new_mtu)
 	WRITE_ONCE(ndev->mtu, new_mtu);
 
 	for (q = 0; q < dev->num_queues; q++)
-		ntb_transport_link_up(dev->queues[q].qp);
+		ntb_transport_link_up(dev->queues[q].qp, 0);
 
 	return 0;
 
@@ -538,7 +538,7 @@ static int ntb_inc_channels(struct net_device *ndev,
 
 	if (running)
 		for (q = old; q < new; q++)
-			ntb_transport_link_up(dev->queues[q].qp);
+			ntb_transport_link_up(dev->queues[q].qp, 0);
 
 	return 0;
 
diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
index 0b47285ef48b..ea89eb336472 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -145,6 +145,8 @@ struct ntb_transport_qp {
 	bool client_ready;
 	bool link_is_up;
 	bool active;
+	u32 local_caps;
+	unsigned int caps_spad;
 
 	u8 qp_num;	/* Only 64 QP's are allowed.  0-63 */
 	u64 qp_bit;
@@ -181,7 +183,7 @@ struct ntb_transport_qp {
 	dma_cookie_t last_cookie;
 	struct tasklet_struct rxc_db_work;
 
-	void (*event_handler)(void *data, int status);
+	void (*event_handler)(void *data, int status, u32 peer_caps);
 	struct delayed_work link_work;
 	struct work_struct link_cleanup;
 
@@ -282,6 +284,9 @@ enum {
 	MW0_SZ_LOW,
 };
 
+/* One per-QP scratchpad, with the remaining bits owned by the client. */
+#define QP_CAPS_VALID		BIT(31)
+
 #define dev_client_dev(__dev) \
 	container_of((__dev), struct ntb_transport_client_dev, dev)
 
@@ -937,7 +942,7 @@ static void ntb_qp_link_cleanup(struct ntb_transport_qp *qp)
 	ntb_qp_link_down_reset(qp);
 
 	if (qp->event_handler)
-		qp->event_handler(qp->cb_data, qp->link_is_up);
+		qp->event_handler(qp->cb_data, qp->link_is_up, 0);
 }
 
 static void ntb_qp_link_cleanup_work(struct work_struct *work)
@@ -1141,10 +1146,19 @@ static void ntb_qp_link_work(struct work_struct *work)
 						   link_work.work);
 	struct pci_dev *pdev = qp->ndev->pdev;
 	struct ntb_transport_ctx *nt = qp->transport;
+	u32 peer_caps = 0;
 	int val;
 
 	WARN_ON(!nt->link_is_up);
 
+	/* Pair with the release store in ntb_transport_link_up(). */
+	if (!smp_load_acquire(&qp->client_ready))
+		return;
+
+	/* Publish capabilities before QP readiness. */
+	if (qp->caps_spad)
+		ntb_peer_spad_write(nt->ndev, PIDX, qp->caps_spad,
+				    READ_ONCE(qp->local_caps) | QP_CAPS_VALID);
 	val = ntb_spad_read(nt->ndev, QP_LINKS);
 
 	ntb_qp_up_request(qp, true);
@@ -1154,12 +1168,26 @@ static void ntb_qp_link_work(struct work_struct *work)
 
 	/* See if the remote side is up */
 	if (val & BIT(qp->qp_num)) {
+		if (qp->caps_spad) {
+			u32 caps;
+
+			/*
+			 * Order the readiness read before the capability read
+			 * for memory-backed SPADs.
+			 */
+			dma_rmb();
+			caps = ntb_spad_read(nt->ndev, qp->caps_spad);
+
+			if (caps & QP_CAPS_VALID)
+				peer_caps = caps & ~QP_CAPS_VALID;
+		}
+
 		dev_info(&pdev->dev, "qp %d: Link Up\n", qp->qp_num);
 		qp->link_is_up = true;
 		qp->active = true;
 
 		if (qp->event_handler)
-			qp->event_handler(qp->cb_data, qp->link_is_up);
+			qp->event_handler(qp->cb_data, qp->link_is_up, peer_caps);
 
 		if (qp->active)
 			tasklet_schedule(&qp->rxc_db_work);
@@ -1189,6 +1217,12 @@ static int ntb_transport_init_queue(struct ntb_transport_ctx *nt,
 	qp->ndev = nt->ndev;
 	qp->client_ready = false;
 	qp->event_handler = NULL;
+	/* Reserve MSI slots even when only the peer might use them. */
+	qp->caps_spad = nt->msi_spad_offset + 2 * qp_count + qp_num;
+	if (qp->caps_spad >= ntb_spad_count(nt->ndev))
+		qp->caps_spad = 0;
+	else
+		ntb_spad_write(qp->ndev, qp->caps_spad, 0);
 	ntb_qp_link_context_reset(qp);
 
 	if (mw_num < qp_count % mw_count)
@@ -2409,15 +2443,22 @@ EXPORT_SYMBOL_GPL(ntb_transport_tx_enqueue);
 /**
  * ntb_transport_link_up - Notify NTB transport of client readiness to use queue
  * @qp: NTB transport layer queue to be enabled
+ * @local_caps: Opaque client capabilities in bits 0..30, unchanged until
+ *              ntb_transport_link_down()
  *
  * Notify NTB transport layer of client readiness to use queue
+ *
+ * Exchange capabilities before reporting link-up through event_handler.
+ * Report zero peer capabilities for legacy peers or insufficient scratchpads.
  */
-void ntb_transport_link_up(struct ntb_transport_qp *qp)
+void ntb_transport_link_up(struct ntb_transport_qp *qp, u32 local_caps)
 {
 	if (!qp)
 		return;
 
-	qp->client_ready = true;
+	WRITE_ONCE(qp->local_caps, local_caps & ~QP_CAPS_VALID);
+	/* Publish local_caps before QP link work sees client_ready. */
+	smp_store_release(&qp->client_ready, true);
 
 	if (qp->transport->link_is_up)
 		schedule_delayed_work(&qp->link_work, 0);
@@ -2439,12 +2480,17 @@ void ntb_transport_link_down(struct ntb_transport_qp *qp)
 
 	qp->client_ready = false;
 
+	if (!qp->link_is_up)
+		cancel_delayed_work_sync(&qp->link_work);
+
+	/* Stop advertising capabilities before withdrawing QP readiness. */
+	if (qp->caps_spad)
+		ntb_peer_spad_write(qp->ndev, PIDX, qp->caps_spad, 0);
+
 	ntb_qp_up_request(qp, false);
 
 	if (qp->link_is_up)
 		ntb_send_link_down(qp);
-	else
-		cancel_delayed_work_sync(&qp->link_work);
 }
 EXPORT_SYMBOL_GPL(ntb_transport_link_down);
 
diff --git a/include/linux/ntb_transport.h b/include/linux/ntb_transport.h
index 7243eb98a722..685dde629a48 100644
--- a/include/linux/ntb_transport.h
+++ b/include/linux/ntb_transport.h
@@ -48,6 +48,8 @@
  * Jon Mason <jon.mason@intel.com>
  */
 
+#include <linux/types.h>
+
 struct ntb_transport_qp;
 
 struct ntb_transport_client {
@@ -66,7 +68,8 @@ struct ntb_queue_handlers {
 			   void *data, int len);
 	void (*tx_handler)(struct ntb_transport_qp *qp, void *qp_data,
 			   void *data, int len);
-	void (*event_handler)(void *data, int status);
+	/* peer_caps is 31-bit, zero on link-down or without peer support. */
+	void (*event_handler)(void *data, int status, u32 peer_caps);
 };
 
 unsigned char ntb_transport_qp_num(struct ntb_transport_qp *qp);
@@ -80,7 +83,7 @@ int ntb_transport_rx_enqueue(struct ntb_transport_qp *qp, void *cb, void *data,
 int ntb_transport_tx_enqueue(struct ntb_transport_qp *qp, void *cb, void *data,
 			     unsigned int len);
 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_up(struct ntb_transport_qp *qp, u32 local_caps);
 void ntb_transport_link_down(struct ntb_transport_qp *qp);
 bool ntb_transport_link_query(struct ntb_transport_qp *qp);
 unsigned int ntb_transport_tx_free_entry(struct ntb_transport_qp *qp);
-- 
2.51.0

Re: [PATCH net-next v4 05/10] NTB: ntb_transport: Exchange client capabilities at link-up

[netdev-bot+sashiko]

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

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

- [Medium] ntb_transport_link_down() withdraws the per-QP capability word
  *before* it withdraws the QP readiness bit, which is the inverse of…
- [Medium] ntb_transport_link_down() can be raced by a queued/running
  ntb_qp_link_work() that re-publishes exactly what link_down just…

Pre-existing issues:
- [High] ntb_transport_free_queue() never synchronously cancels
  qp->link_cleanup, so that work — or the qp->link_work it re-arms — can…
- [Medium] Store-buffering race between ntb_transport_link_up() and
  ntb_transport_link_work() can leave qp->link_work permanently…

--

NTB: ntb_transport: Exchange client capabilities at link-up

The patch adds an opaque 31-bit per-QP capability word that is exchanged
through one spare scratchpad per QP during QP link setup, with bit 31 used
as a validity marker.  The capability value is handed to the client with
the link-up event, and ntb_transport_link_up() grows a local_caps argument
which ntb_netdev passes as zero for now.

A few questions below about the withdrawal ordering and about
synchronization against ntb_qp_link_work().

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index 0b47285ef48b6..ea89eb336472a 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c

[ ... ]

> @@ -937,7 +942,7 @@ static void ntb_qp_link_cleanup(struct ntb_transport_qp *qp)
>  	ntb_qp_link_down_reset(qp);
>  
>  	if (qp->event_handler)
> -		qp->event_handler(qp->cb_data, qp->link_is_up);
> +		qp->event_handler(qp->cb_data, qp->link_is_up, 0);
>  }
>  
>  static void ntb_qp_link_cleanup_work(struct work_struct *work)

[Severity: High]

This isn't a bug introduced by this patch, but while reading the
qp->link_cleanup path: can this work item outlive the QP?

ntb_qp_link_down(), called from ntb_process_rxc() on LINK_DOWN_FLAG in the
rx tasklet, only queues it:

static void ntb_qp_link_down(struct ntb_transport_qp *qp)
{
	schedule_work(&qp->link_cleanup);
}

ntb_transport_free_queue() then does tasklet_kill(&qp->rxc_db_work) and
cancel_delayed_work_sync(&qp->link_work), but no
cancel_work_sync(&qp->link_cleanup), and marks the QP free:

	qp->transport->qp_bitmap_free |= qp_bit;

A link_cleanup queued just before the tasklet was killed can then run
ntb_qp_link_cleanup_work() after free_queue() returned and re-arm
qp->link_work:

	if (nt->link_is_up)
		schedule_delayed_work(&qp->link_work,
				      msecs_to_jiffies(NTB_LINK_DOWN_TIMEOUT));

On transport teardown, ntb_transport_link_cleanup() only cancels per-QP
work for QPs still marked allocated:

	for (i = 0; i < nt->qp_count; i++)
		if (qp_bitmap_alloc & BIT_ULL(i)) {
			qp = &nt->qp_vec[i];
			ntb_qp_link_cleanup(qp);
			cancel_work_sync(&qp->link_cleanup);
			cancel_delayed_work_sync(&qp->link_work);
		}

so the released QP is skipped, and ntb_transport_free() then does
kfree(nt->qp_vec) with that work still pending.  Does this let the
pending link_cleanup/link_work dereference freed memory (the work_struct
list pointers, qp->transport, qp->ndev)?  Would adding
cancel_work_sync(&qp->link_cleanup) to ntb_transport_free_queue() close
this?

> @@ -1141,10 +1146,19 @@ static void ntb_qp_link_work(struct work_struct *work)
>  						   link_work.work);
>  	struct pci_dev *pdev = qp->ndev->pdev;
>  	struct ntb_transport_ctx *nt = qp->transport;
> +	u32 peer_caps = 0;
>  	int val;
>  
>  	WARN_ON(!nt->link_is_up);
>  
> +	/* Pair with the release store in ntb_transport_link_up(). */
> +	if (!smp_load_acquire(&qp->client_ready))
> +		return;
> +
> +	/* Publish capabilities before QP readiness. */
> +	if (qp->caps_spad)
> +		ntb_peer_spad_write(nt->ndev, PIDX, qp->caps_spad,
> +				    READ_ONCE(qp->local_caps) | QP_CAPS_VALID);
>  	val = ntb_spad_read(nt->ndev, QP_LINKS);
>  
>  	ntb_qp_up_request(qp, true);
> @@ -1154,12 +1168,26 @@ static void ntb_qp_link_work(struct work_struct *work)
>  
>  	/* See if the remote side is up */
>  	if (val & BIT(qp->qp_num)) {
> +		if (qp->caps_spad) {
> +			u32 caps;
> +
> +			/*
> +			 * Order the readiness read before the capability read
> +			 * for memory-backed SPADs.
> +			 */
> +			dma_rmb();
> +			caps = ntb_spad_read(nt->ndev, qp->caps_spad);
> +
> +			if (caps & QP_CAPS_VALID)
> +				peer_caps = caps & ~QP_CAPS_VALID;
> +		}
> +
>  		dev_info(&pdev->dev, "qp %d: Link Up\n", qp->qp_num);
>  		qp->link_is_up = true;
>  		qp->active = true;
>  
>  		if (qp->event_handler)
> -			qp->event_handler(qp->cb_data, qp->link_is_up);
> +			qp->event_handler(qp->cb_data, qp->link_is_up, peer_caps);
>  
>  		if (qp->active)
>  			tasklet_schedule(&qp->rxc_db_work);

[ ... ]

> @@ -2409,15 +2443,22 @@ EXPORT_SYMBOL_GPL(ntb_transport_tx_enqueue);
>  /**
>   * ntb_transport_link_up - Notify NTB transport of client readiness to use queue
>   * @qp: NTB transport layer queue to be enabled
> + * @local_caps: Opaque client capabilities in bits 0..30, unchanged until
> + *              ntb_transport_link_down()
>   *
>   * Notify NTB transport layer of client readiness to use queue
> + *
> + * Exchange capabilities before reporting link-up through event_handler.
> + * Report zero peer capabilities for legacy peers or insufficient scratchpads.
>   */
> -void ntb_transport_link_up(struct ntb_transport_qp *qp)
> +void ntb_transport_link_up(struct ntb_transport_qp *qp, u32 local_caps)
>  {
>  	if (!qp)
>  		return;
>  
> -	qp->client_ready = true;
> +	WRITE_ONCE(qp->local_caps, local_caps & ~QP_CAPS_VALID);
> +	/* Publish local_caps before QP link work sees client_ready. */
> +	smp_store_release(&qp->client_ready, true);
>  
>  	if (qp->transport->link_is_up)
>  		schedule_delayed_work(&qp->link_work, 0);

[Severity: Medium]

This is a pre-existing issue, but since the ordering on these exact lines
is now being reasoned about: can qp->link_work end up never being
scheduled?

Here the store to client_ready is followed by a load of
qp->transport->link_is_up, and ntb_transport_link_work() has the mirrored
order:

	nt->link_is_up = true;

	for (i = 0; i < nt->qp_count; i++) {
		...
		if (qp->client_ready)
			schedule_delayed_work(&qp->link_work, 0);
	}

Both sides are store-then-load with no full barrier and no common lock
(nt->link_work holds nt->link_event_lock, which ntb_transport_link_up()
does not take), so both loads can return the stale value: the client sees
the transport link down and does not schedule, the transport worker sees
client_ready as false and does not schedule either.
ntb_transport_link_work() does not reschedule itself on that path, so the
QP stays down until an unrelated NTB link event.

The smp_store_release() added here only orders the local_caps store ahead
of the client_ready store and gives no store-to-load ordering.  Would an
smp_mb() on both sides, or taking a common lock around the flag and the
link_is_up test, be needed to close this?

> @@ -2439,12 +2480,17 @@ void ntb_transport_link_down(struct ntb_transport_qp *qp)
>  
>  	qp->client_ready = false;
>  
> +	if (!qp->link_is_up)
> +		cancel_delayed_work_sync(&qp->link_work);
> +
> +	/* Stop advertising capabilities before withdrawing QP readiness. */
> +	if (qp->caps_spad)
> +		ntb_peer_spad_write(qp->ndev, PIDX, qp->caps_spad, 0);
> +
>  	ntb_qp_up_request(qp, false);
>  
>  	if (qp->link_is_up)
>  		ntb_send_link_down(qp);
> -	else
> -		cancel_delayed_work_sync(&qp->link_work);
>  }
>  EXPORT_SYMBOL_GPL(ntb_transport_link_down);

[Severity: Medium]

Is this the safe withdrawal order?  The up path in ntb_qp_link_work()
establishes "readiness implies a valid capability word" by writing the
capability word first and the QP_LINKS bit second, while here the
capability word is cleared before the readiness bit.

If the peer's ntb_qp_link_work() polls in that window:

peer ntb_qp_link_work()
	val = ntb_spad_read(nt->ndev, QP_LINKS);	/* our bit still set */
	...
	if (val & BIT(qp->qp_num)) {
		caps = ntb_spad_read(nt->ndev, qp->caps_spad);	/* already 0 */
		if (caps & QP_CAPS_VALID)			/* false */
			peer_caps = caps & ~QP_CAPS_VALID;
		...
		qp->link_is_up = true;
		qp->event_handler(qp->cb_data, qp->link_is_up, peer_caps);

the peer latches peer_caps as zero, sets qp->link_is_up and stops
rescheduling qp->link_work, so it never re-reads the capability slot and a
later re-advertisement is not observed.

In the common case where our qp->link_is_up was still false (we advertised
but the peer had not become ready yet), ntb_send_link_down() is not called
either, so nothing knocks the peer out of that state and peer_caps stays
zero for the rest of the session.

Would clearing readiness first and zeroing the capability word afterwards
avoid this, since the peer would then just retry?  The comment and the
commit message text "Write capabilities to the peer before advertising QP
readiness" plus "Withdraw advertisements on link-down" read as if the
withdrawal order here were the safe one.

[Severity: Medium]

Two questions about the synchronization against ntb_qp_link_work() in this
function.

The cancel now runs only when qp->link_is_up is false, but link_work can
be queued while qp->link_is_up is already true.  ntb_transport_link_work()
has no "already up" test:

	nt->link_is_up = true;

	for (i = 0; i < nt->qp_count; i++) {
		...
		if (qp->client_ready)
			schedule_delayed_work(&qp->link_work, 0);
	}

and ntb_qp_link_cleanup_work() re-arms it too.  Can that worker then undo
what this function just did?

CPU0 ntb_transport_link_down()
	qp->client_ready = false;		/* plain store */
	/* cancel skipped: qp->link_is_up == true */
	ntb_peer_spad_write(qp->ndev, PIDX, qp->caps_spad, 0);
	ntb_qp_up_request(qp, false);

CPU1 ntb_qp_link_work()		/* already past its client_ready check */
	ntb_peer_spad_write(nt->ndev, PIDX, qp->caps_spad,
			    READ_ONCE(qp->local_caps) | QP_CAPS_VALID);
	...
	ntb_qp_up_request(qp, true);
	...
	qp->link_is_up = true;
	qp->event_handler(qp->cb_data, qp->link_is_up, peer_caps);

After ntb_transport_link_down() returns, the peer would still see a ready
QP with a valid capability word, and the client receives a link-up event
after having asked for link-down (in ntb_netdev that re-wakes the subqueue
in the middle of an MTU change).

Second, qp->client_ready is cleared here with a plain store while
ntb_qp_link_work() now reads it with smp_load_acquire().  Should the clear
be a WRITE_ONCE() or smp_store_release() so the two accesses are paired,
and should the cancel be unconditional?

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

[PATCH net-next v4 06/10] 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 peers using
transport version 4 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>
---
Changes in v4:
  - Use the acquire-loaded flags for RX metadata and WRITE_ONCE() when
    initializing entry->flags (Sashiko)
  - Document the metadata range and legacy RX behavior (Sashiko)
  - Use GENMASK() and FIELD_*() helpers for metadata (Jakub)
  - Did not carry over Dave's R-b tag due to the changes. Would
    appreciate another look.

 drivers/net/ntb_netdev.c      |  4 ++--
 drivers/ntb/ntb_transport.c   | 29 ++++++++++++++++++++---------
 include/linux/ntb_transport.h |  5 +++--
 3 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
index 869fc9a7f9e8..78df47659d45 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, u32 peer_caps)
 }
 
 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 ea89eb336472..8ec798893ac4 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -47,6 +47,7 @@
  * Contact Information:
  * Jon Mason <jon.mason@intel.com>
  */
+#include <linux/bitfield.h>
 #include <linux/debugfs.h>
 #include <linux/delay.h>
 #include <linux/dmaengine.h>
@@ -169,7 +170,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;
@@ -269,6 +270,9 @@ enum {
 	LINK_DOWN_FLAG = BIT(1),
 };
 
+/* Reserve the low byte for transport flags. */
+#define DESC_META_MASK		GENMASK(31, 8)
+
 struct ntb_payload_header {
 	__le32 ver;
 	__le32 len;
@@ -1490,17 +1494,20 @@ static void ntb_transport_free(struct ntb_client *self, struct ntb_dev *ndev)
 static void ntb_complete_rxc(struct ntb_transport_qp *qp)
 {
 	struct ntb_queue_entry *entry;
-	void *cb_data;
-	unsigned int len;
 	unsigned long irqflags;
+	unsigned int flags;
+	unsigned int meta;
+	unsigned int len;
+	void *cb_data;
 
 	spin_lock_irqsave(&qp->ntb_rx_q_lock, irqflags);
 
 	while (!list_empty(&qp->rx_post_q)) {
 		entry = list_first_entry(&qp->rx_post_q,
 					 struct ntb_queue_entry, entry);
-		/* DONE publishes the entry fields and copied data. */
-		if (!(smp_load_acquire(&entry->flags) & DESC_DONE_FLAG))
+		/* DONE publishes the entry, payload and client metadata. */
+		flags = smp_load_acquire(&entry->flags);
+		if (!(flags & DESC_DONE_FLAG))
 			break;
 
 		entry->rx_hdr->flags = cpu_to_le32(0);
@@ -1508,13 +1515,14 @@ static void ntb_complete_rxc(struct ntb_transport_qp *qp)
 
 		cb_data = entry->cb_data;
 		len = entry->len;
+		meta = FIELD_GET(DESC_META_MASK, flags);
 
 		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);
 	}
@@ -1714,6 +1722,7 @@ static int ntb_process_rxc(struct ntb_transport_qp *qp)
 
 	entry->rx_hdr = hdr;
 	entry->rx_index = qp->rx_index;
+	WRITE_ONCE(entry->flags, flags & DESC_META_MASK);
 
 	if (len > entry->len) {
 		dev_dbg(&qp->ndev->pdev->dev,
@@ -2396,6 +2405,8 @@ 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: 24-bit client metadata to send.
+ *        Out-of-range values return -EINVAL.
  *
  * 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
@@ -2404,12 +2415,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 > FIELD_MAX(DESC_META_MASK))
 		return -EINVAL;
 
 	if (!qp->link_is_up)
@@ -2427,7 +2438,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 = FIELD_PREP(DESC_META_MASK, meta);
 	entry->errors = 0;
 	entry->tx_index = 0;
 
diff --git a/include/linux/ntb_transport.h b/include/linux/ntb_transport.h
index 685dde629a48..2eafb53c2c8d 100644
--- a/include/linux/ntb_transport.h
+++ b/include/linux/ntb_transport.h
@@ -64,8 +64,9 @@ int ntb_transport_register_client_dev(char *device_name);
 void ntb_transport_unregister_client_dev(char *device_name);
 
 struct ntb_queue_handlers {
+	/* meta is 24-bit client metadata, zero from legacy peers. */
 	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);
 	/* peer_caps is 31-bit, zero on link-down or without peer support. */
@@ -81,7 +82,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, u32 local_caps);
 void ntb_transport_link_down(struct ntb_transport_qp *qp);
-- 
2.51.0

[PATCH net-next v4 07/10] net: ntb_netdev: Reject short RX frames

[Koichiro Den]

ntb_netdev passes RX frames shorter than ETH_HLEN to eth_type_trans(),
which expects a complete Ethernet header.

Reject them as length errors.

Fixes: 548c237c0a99 ("net: Add support for NTB virtual ethernet device")
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v4:
  - New patch. Included here to fix the existing RX length check before
    adding checks for ntb_netdev_hdr later in the series.

 drivers/net/ntb_netdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
index 78df47659d45..84fdd81ebd16 100644
--- a/drivers/net/ntb_netdev.c
+++ b/drivers/net/ntb_netdev.c
@@ -140,7 +140,7 @@ static void ntb_netdev_rx_handler(struct ntb_transport_qp *qp, void *qp_data,
 
 	netdev_dbg(ndev, "%s: %d byte payload received\n", __func__, len);
 
-	if (len < 0) {
+	if (len < ETH_HLEN) {
 		DEV_STATS_INC(ndev, rx_errors);
 		DEV_STATS_INC(ndev, rx_length_errors);
 		goto enqueue_again;
-- 
2.51.0

[PATCH net-next v4 08/10] net: ntb_netdev: Factor out RX statistics update

[Koichiro Den]

Move RX packet and byte accounting into a helper for the packet-header
handling added by the next patch.

No functional change.

Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v4:
  - New patch.

For reviewers: I split this out for easier review, but I'm happy to fold
it into the next patch if preferred.

 drivers/net/ntb_netdev.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
index 84fdd81ebd16..cddfe8d63fc1 100644
--- a/drivers/net/ntb_netdev.c
+++ b/drivers/net/ntb_netdev.c
@@ -122,15 +122,25 @@ static void ntb_netdev_event_handler(void *data, int link_is_up, u32 peer_caps)
 	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,11 +156,7 @@ 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);
+	ntb_netdev_rx_stats_add(ndev, len);
 
 	new_skb = netdev_alloc_skb(ndev, ndev->mtu + ETH_HLEN);
 	if (!new_skb) {
-- 
2.51.0

[PATCH net-next v4 09/10] net: ntb_netdev: Introduce an optional packet header, ntb_netdev_hdr

[Koichiro Den]

Add an optional ntb_netdev_hdr for per-packet metadata, with a length
field for future extensions. One client metadata bit marks its presence.
Account for it in TX headroom, the MTU and RX buffers.

The flags field must be zero for now. The next patch adds checksum
metadata.

Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v4:
  - New patch. (Jakub)
    https://lore.kernel.org/r/20260910172047.167225eb@kernel.org/

 drivers/net/ntb_netdev.c | 69 ++++++++++++++++++++++++++++++++--------
 1 file changed, 55 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
index cddfe8d63fc1..67cfe0f1a49a 100644
--- a/drivers/net/ntb_netdev.c
+++ b/drivers/net/ntb_netdev.c
@@ -29,6 +29,14 @@ static unsigned int tx_stop = 5;
 #define NTB_NETDEV_MAX_QUEUES		64
 #define NTB_NETDEV_DEFAULT_QUEUES	1
 
+/* An ntb_netdev_hdr precedes the packet. */
+#define NTB_NETDEV_META_HDR		BIT(0)
+
+struct ntb_netdev_hdr {
+	__le16 len;		/* Header length in bytes, a multiple of 2. */
+	__le16 flags;
+};
+
 struct ntb_netdev;
 
 struct ntb_netdev_queue {
@@ -83,15 +91,16 @@ static int ntb_netdev_queue_rx_fill(struct net_device *ndev,
 				    struct ntb_netdev_queue *queue)
 {
 	struct sk_buff *skb;
+	unsigned int size;
 	int rc, i;
 
+	size = ndev->mtu + ETH_HLEN + sizeof(struct ntb_netdev_hdr);
 	for (i = 0; i < NTB_RXQ_SIZE; i++) {
-		skb = netdev_alloc_skb(ndev, ndev->mtu + ETH_HLEN);
+		skb = netdev_alloc_skb(ndev, size);
 		if (!skb)
 			return -ENOMEM;
 
-		rc = ntb_transport_rx_enqueue(queue->qp, skb, skb->data,
-					      ndev->mtu + ETH_HLEN);
+		rc = ntb_transport_rx_enqueue(queue->qp, skb, skb->data, size);
 		if (rc) {
 			dev_kfree_skb(skb);
 			return rc;
@@ -137,34 +146,57 @@ static void ntb_netdev_rx_stats_add(struct net_device *ndev,
 static void ntb_netdev_rx_handler(struct ntb_transport_qp *qp, void *qp_data,
 				  void *data, int len, unsigned int meta)
 {
+	const struct ntb_netdev_hdr *hdr = NULL;
 	struct ntb_netdev_queue *q = qp_data;
 	struct ntb_netdev *dev = q->ntdev;
+	unsigned int size, hdr_len = 0;
 	struct sk_buff *skb, *new_skb;
 	struct net_device *ndev;
 	int rc;
 
 	ndev = dev->ndev;
+	size = ndev->mtu + ETH_HLEN + sizeof(*hdr);
 	skb = data;
 	if (!skb)
 		return;
 
 	netdev_dbg(ndev, "%s: %d byte payload received\n", __func__, len);
 
+	/* Validate the frame and optional header lengths. */
 	if (len < ETH_HLEN) {
 		DEV_STATS_INC(ndev, rx_errors);
 		DEV_STATS_INC(ndev, rx_length_errors);
 		goto enqueue_again;
 	}
+	if (meta & NTB_NETDEV_META_HDR) {
+		hdr = (void *)skb->data;
+		hdr_len = le16_to_cpu(hdr->len);
+		if (hdr_len < sizeof(*hdr) || !IS_ALIGNED(hdr_len, 2) ||
+		    hdr_len > len - ETH_HLEN) {
+			DEV_STATS_INC(ndev, rx_errors);
+			DEV_STATS_INC(ndev, rx_length_errors);
+			goto enqueue_again;
+		}
+		len -= hdr_len;
+	}
 
-	ntb_netdev_rx_stats_add(ndev, len);
-
-	new_skb = netdev_alloc_skb(ndev, ndev->mtu + ETH_HLEN);
+	new_skb = netdev_alloc_skb(ndev, size);
 	if (!new_skb) {
+		ntb_netdev_rx_stats_add(ndev, len);
 		DEV_STATS_INC(ndev, rx_dropped);
 		goto enqueue_again;
 	}
 
-	skb_put(skb, len);
+	skb_put(skb, len + hdr_len);
+	if (hdr) {
+		u16 flags = le16_to_cpu(hdr->flags);
+
+		skb_pull(skb, hdr_len);
+		if (flags)
+			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);
@@ -174,12 +206,19 @@ static void ntb_netdev_rx_handler(struct ntb_transport_qp *qp, void *qp_data,
 	skb = new_skb;
 
 enqueue_again:
-	rc = ntb_transport_rx_enqueue(qp, skb, skb->data, ndev->mtu + ETH_HLEN);
+	rc = ntb_transport_rx_enqueue(qp, skb, skb->data, size);
 	if (rc) {
 		dev_kfree_skb_any(skb);
 		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,
@@ -390,9 +429,11 @@ static int ntb_netdev_change_mtu(struct net_device *ndev, int new_mtu)
 	struct ntb_netdev_queue *queue;
 	struct sk_buff *skb;
 	unsigned int q, i;
+	unsigned int size;
 	int len, rc = 0;
 
-	if (new_mtu > ntb_transport_max_size(dev->queues[0].qp) - ETH_HLEN)
+	size = new_mtu + ETH_HLEN + sizeof(struct ntb_netdev_hdr);
+	if (size > ntb_transport_max_size(dev->queues[0].qp))
 		return -EINVAL;
 
 	if (!netif_running(ndev)) {
@@ -414,8 +455,7 @@ static int ntb_netdev_change_mtu(struct net_device *ndev, int new_mtu)
 				dev_kfree_skb(skb);
 
 			for (; i; i--) {
-				skb = netdev_alloc_skb(ndev,
-						       new_mtu + ETH_HLEN);
+				skb = netdev_alloc_skb(ndev, size);
 				if (!skb) {
 					rc = -ENOMEM;
 					goto err;
@@ -423,8 +463,7 @@ static int ntb_netdev_change_mtu(struct net_device *ndev, int new_mtu)
 
 				rc = ntb_transport_rx_enqueue(queue->qp, skb,
 							      skb->data,
-							      new_mtu +
-							      ETH_HLEN);
+							      size);
 				if (rc) {
 					dev_kfree_skb(skb);
 					goto err;
@@ -673,6 +712,7 @@ static int ntb_netdev_probe(struct device *client_dev)
 
 	ndev->features = NETIF_F_HIGHDMA;
 	ndev->pcpu_stat_type = NETDEV_PCPU_STAT_TSTATS;
+	ndev->needed_headroom = sizeof(struct ntb_netdev_hdr);
 
 	ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
 
@@ -710,7 +750,8 @@ static int ntb_netdev_probe(struct device *client_dev)
 	if (rc)
 		goto err_free_qps;
 
-	ndev->mtu = ntb_transport_max_size(dev->queues[0].qp) - ETH_HLEN;
+	ndev->mtu = ntb_transport_max_size(dev->queues[0].qp) - ETH_HLEN -
+		    sizeof(struct ntb_netdev_hdr);
 
 	rc = register_netdev(ndev);
 	if (rc)
-- 
2.51.0

Re: [PATCH net-next v4 09/10] net: ntb_netdev: Introduce an optional packet header, ntb_netdev_hdr

[netdev-bot+sashiko]

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

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

- [Low] RX buffers are now posted with size = ndev->mtu + ETH_HLEN +
  sizeof(struct ntb_netdev_hdr) (drivers/net/ntb_netdev.c:97 and :158),…
- [Low] In ntb_netdev_rx_handler() the replacement-skb allocation
  (drivers/net/ntb_netdev.c:183) is attempted before hdr->flags is…

Pre-existing issues:
- [High] RX completion handler re-posts an already-allocated skb using a
  receive length recomputed from an unsynchronized read of ndev->mtu.

--

net: ntb_netdev: Introduce an optional packet header, ntb_netdev_hdr

This adds an optional 4-byte ntb_netdev_hdr in front of the packet, marked
by a new client metadata bit, and accounts for it in the TX headroom, the
MTU and the RX buffer sizes. The flags field must be zero for now, with
checksum metadata coming in the next patch.

A few questions on the RX path below.

> diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
> index cddfe8d63fc18..67cfe0f1a49a2 100644
> --- a/drivers/net/ntb_netdev.c
> +++ b/drivers/net/ntb_netdev.c
> @@ -29,6 +29,14 @@ static unsigned int tx_stop = 5;
>  #define NTB_NETDEV_MAX_QUEUES		64
>  #define NTB_NETDEV_DEFAULT_QUEUES	1
>  
> +/* An ntb_netdev_hdr precedes the packet. */
> +#define NTB_NETDEV_META_HDR		BIT(0)
> +
> +struct ntb_netdev_hdr {
> +	__le16 len;		/* Header length in bytes, a multiple of 2. */
> +	__le16 flags;
> +};
> +
>  struct ntb_netdev;
>  
>  struct ntb_netdev_queue {
> @@ -83,15 +91,16 @@ static int ntb_netdev_queue_rx_fill(struct net_device *ndev,
>  				    struct ntb_netdev_queue *queue)
>  {
>  	struct sk_buff *skb;
> +	unsigned int size;
>  	int rc, i;
>  
> +	size = ndev->mtu + ETH_HLEN + sizeof(struct ntb_netdev_hdr);
>  	for (i = 0; i < NTB_RXQ_SIZE; i++) {
> -		skb = netdev_alloc_skb(ndev, ndev->mtu + ETH_HLEN);
> +		skb = netdev_alloc_skb(ndev, size);
>  		if (!skb)
>  			return -ENOMEM;
>  
> -		rc = ntb_transport_rx_enqueue(queue->qp, skb, skb->data,
> -					      ndev->mtu + ETH_HLEN);
> +		rc = ntb_transport_rx_enqueue(queue->qp, skb, skb->data, size);
>  		if (rc) {
>  			dev_kfree_skb(skb);
>  			return rc;
> @@ -137,34 +146,57 @@ static void ntb_netdev_rx_stats_add(struct net_device *ndev,
>  static void ntb_netdev_rx_handler(struct ntb_transport_qp *qp, void *qp_data,
>  				  void *data, int len, unsigned int meta)
>  {
> +	const struct ntb_netdev_hdr *hdr = NULL;
>  	struct ntb_netdev_queue *q = qp_data;
>  	struct ntb_netdev *dev = q->ntdev;
> +	unsigned int size, hdr_len = 0;
>  	struct sk_buff *skb, *new_skb;
>  	struct net_device *ndev;
>  	int rc;
>  
>  	ndev = dev->ndev;
> +	size = ndev->mtu + ETH_HLEN + sizeof(*hdr);
>  	skb = data;
>  	if (!skb)
>  		return;
>  
>  	netdev_dbg(ndev, "%s: %d byte payload received\n", __func__, len);
>  
> +	/* Validate the frame and optional header lengths. */
>  	if (len < ETH_HLEN) {
>  		DEV_STATS_INC(ndev, rx_errors);
>  		DEV_STATS_INC(ndev, rx_length_errors);
>  		goto enqueue_again;
>  	}
> +	if (meta & NTB_NETDEV_META_HDR) {
> +		hdr = (void *)skb->data;
> +		hdr_len = le16_to_cpu(hdr->len);
> +		if (hdr_len < sizeof(*hdr) || !IS_ALIGNED(hdr_len, 2) ||
> +		    hdr_len > len - ETH_HLEN) {
> +			DEV_STATS_INC(ndev, rx_errors);
> +			DEV_STATS_INC(ndev, rx_length_errors);
> +			goto enqueue_again;
> +		}
> +		len -= hdr_len;
> +	}

[Severity: Low]
Now that ntb_netdev_queue_rx_fill() and ntb_netdev_rx_handler() post
buffers of ndev->mtu + ETH_HLEN + sizeof(struct ntb_netdev_hdr), and the
only length gate in ntb_netdev_rx_handler() is the len < ETH_HLEN test,
what rejects an over-MTU frame when the peer does not set
NTB_NETDEV_META_HDR?

Before this change the posted buffer was exactly mtu + ETH_HLEN, so
ntb_process_rxc() dropped such a frame:

	if (len > entry->len) {

and the driver counted rx_length_errors. With the extra reservation, a
frame of up to mtu + ETH_HLEN + sizeof(struct ntb_netdev_hdr) now fits the
buffer, so it is skb_put() and handed to netif_rx() as a good packet, i.e.
4 bytes over the configured MTU here and 8 bytes by the end of the series.
The len -= hdr_len subtraction that would re-bound the payload never runs
in that case.

The two sides of the link are configured independently, so a peer with a
larger MTU reaches this. Still present at the end of the series
(a68dfbeaf017).

>  
> -	ntb_netdev_rx_stats_add(ndev, len);
> -
> -	new_skb = netdev_alloc_skb(ndev, ndev->mtu + ETH_HLEN);
> +	new_skb = netdev_alloc_skb(ndev, size);
>  	if (!new_skb) {
> +		ntb_netdev_rx_stats_add(ndev, len);
>  		DEV_STATS_INC(ndev, rx_dropped);
>  		goto enqueue_again;
>  	}

[Severity: Low]
The replacement skb is allocated before hdr->flags is checked. When
netdev_alloc_skb() fails, is a frame carrying a non-zero flags value
counted as a successfully received packet?

This path calls ntb_netdev_rx_stats_add(ndev, len) plus rx_dropped and goes
straight to enqueue_again, skipping the validation added just below:

	if (hdr) {
		u16 flags = le16_to_cpu(hdr->flags);

		skb_pull(skb, hdr_len);
		if (flags)
			goto rx_drop;
	}

So whether a malformed frame ends up in rx_errors depends on whether the
atomic allocation happened to succeed. Would moving the flags check ahead
of the allocation keep the classification consistent? The same ordering
also bypasses the checksum metadata validation added later in the series
(a68dfbeaf017).

>  
> -	skb_put(skb, len);
> +	skb_put(skb, len + hdr_len);
> +	if (hdr) {
> +		u16 flags = le16_to_cpu(hdr->flags);
> +
> +		skb_pull(skb, hdr_len);
> +		if (flags)
> +			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);
> @@ -174,12 +206,19 @@ static void ntb_netdev_rx_handler(struct ntb_transport_qp *qp, void *qp_data,
>  	skb = new_skb;
>  
>  enqueue_again:
> -	rc = ntb_transport_rx_enqueue(qp, skb, skb->data, ndev->mtu + ETH_HLEN);
> +	rc = ntb_transport_rx_enqueue(qp, skb, skb->data, size);
>  	if (rc) {
>  		dev_kfree_skb_any(skb);
>  		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;
>  }

[Severity: High]
This isn't a bug introduced by this patch, since the baseline already
re-posted the original skb with a length derived from ndev->mtu, but the
patch touches these lines and adds new peer-triggerable ways to reach
enqueue_again with the original buffer (bad hdr_len, and the flags path).

Can the size computed at the top of ntb_netdev_rx_handler():

	size = ndev->mtu + ETH_HLEN + sizeof(*hdr);

describe a buffer larger than the skb actually being re-posted here?

ntb_netdev_change_mtu() drains only rx_pend_q through
ntb_transport_rx_remove():

	entry = ntb_list_rm(&qp->ntb_rx_q_lock, &qp->rx_pend_q);

while an entry whose copy is already in progress has been moved to
rx_post_q by ntb_process_rxc(), and ntb_complete_rxc() drops the RX queue
lock before invoking the client callback:

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

So after an MTU increase, a handler still holding an skb allocated for the
old, smaller MTU reads the new, larger ndev->mtu into size, and on any
enqueue_again path re-posts that small buffer while declaring the new
capacity. The transport bounds the next incoming frame only against the
declared entry->len in ntb_process_rxc():

	if (len > entry->len) {

Can a following frame then be copied past the end of that undersized skb,
and can skb_put(skb, len + hdr_len) in the handler push skb->tail beyond
skb->end?

The mirror case, an MTU decrease, declares a smaller length than the
buffer really provides and just drops otherwise valid frames.

Would it be better to take the re-post length from the skb itself
(skb_tailroom(), or a length recorded when the buffer was allocated) and
serialize the resize against RX callbacks? A plain READ_ONCE() of
ndev->mtu would not close this. Still present at the end of the series
(a68dfbeaf017).

> diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
> @@ -390,9 +429,11 @@ static int ntb_netdev_change_mtu(struct net_device *ndev, int new_mtu)
>  	struct ntb_netdev_queue *queue;
>  	struct sk_buff *skb;
>  	unsigned int q, i;
> +	unsigned int size;
>  	int len, rc = 0;
>  
> -	if (new_mtu > ntb_transport_max_size(dev->queues[0].qp) - ETH_HLEN)
> +	size = new_mtu + ETH_HLEN + sizeof(struct ntb_netdev_hdr);
> +	if (size > ntb_transport_max_size(dev->queues[0].qp))
>  		return -EINVAL;
>  
>  	if (!netif_running(ndev)) {

[ ... ]

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

[PATCH net-next v4 10/10] 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. On a trusted PCIe
fabric, we can skip that work.

Use the optional ntb_netdev header to carry CHECKSUM_PARTIAL with its
csum_start and csum_offset.

Exchange receive support at QP link-up and fall back to software if the
peer does not advertise it. This preserves netdev checksum semantics and
interoperability with existing transport version 4 peers.

Keep NETIF_F_RXCSUM unchanged while the interface is up so peers can
rely on the negotiated receive support. Drop partial-checksum frames
when it is disabled.

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 v4:
  - Move offload metadata into an ntb_netdev header (Jakub)
  - Exchange receive support at QP link-up, not on RX (Sashiko)
  - Keep peer checksum state per QP (Sashiko)
  - Advertise receive support only with RXCSUM enabled. Drop
    partial-checksum frames when it is disabled (Sashiko)
  - Keep RXCSUM unchanged while the interface is up

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

diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
index 67cfe0f1a49a..d889c8538be0 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,12 +30,19 @@ static unsigned int tx_stop = 5;
 #define NTB_NETDEV_MAX_QUEUES		64
 #define NTB_NETDEV_DEFAULT_QUEUES	1
 
+#define NTB_NETDEV_CAP_CSUM		BIT(0)
+
 /* An ntb_netdev_hdr precedes the packet. */
 #define NTB_NETDEV_META_HDR		BIT(0)
 
+#define NTB_NETDEV_HDR_F_CSUM		BIT(0)
+
 struct ntb_netdev_hdr {
 	__le16 len;		/* Header length in bytes, a multiple of 2. */
 	__le16 flags;
+	/* From the packet start, excluding this header. */
+	__le16 csum_start;
+	__le16 csum_offset;
 };
 
 struct ntb_netdev;
@@ -44,6 +52,7 @@ struct ntb_netdev_queue {
 	struct ntb_transport_qp *qp;
 	struct timer_list tx_timer;
 	u16 qid;
+	bool peer_csum;
 };
 
 struct ntb_netdev {
@@ -117,6 +126,7 @@ static void ntb_netdev_event_handler(void *data, int link_is_up, u32 peer_caps)
 	struct net_device *ndev;
 
 	ndev = dev->ndev;
+	WRITE_ONCE(q->peer_csum, link_is_up && (peer_caps & NTB_NETDEV_CAP_CSUM));
 
 	netdev_dbg(ndev, "Event %x, Link %x, qp %u\n", link_is_up,
 		   ntb_transport_link_query(q->qp), q->qid);
@@ -189,16 +199,29 @@ static void ntb_netdev_rx_handler(struct ntb_transport_qp *qp, void *qp_data,
 
 	skb_put(skb, len + hdr_len);
 	if (hdr) {
+		u16 offset = le16_to_cpu(hdr->csum_offset);
+		u16 start = le16_to_cpu(hdr->csum_start);
 		u16 flags = le16_to_cpu(hdr->flags);
 
 		skb_pull(skb, hdr_len);
-		if (flags)
+		if (flags & ~NTB_NETDEV_HDR_F_CSUM)
 			goto rx_drop;
+
+		if (flags & NTB_NETDEV_HDR_F_CSUM) {
+			if (!(ndev->features & NETIF_F_RXCSUM)) {
+				ntb_netdev_rx_stats_add(ndev, len);
+				DEV_STATS_INC(ndev, rx_dropped);
+				goto rx_free;
+			}
+
+			if (start < ETH_HLEN ||
+			    !skb_partial_csum_set(skb, start, offset))
+				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);
@@ -216,6 +239,7 @@ static void ntb_netdev_rx_handler(struct ntb_transport_qp *qp, void *qp_data,
 
 rx_drop:
 	DEV_STATS_INC(ndev, rx_errors);
+rx_free:
 	dev_kfree_skb_any(skb);
 	skb = new_skb;
 	goto enqueue_again;
@@ -316,6 +340,8 @@ 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 hdr_len = 0;
+	unsigned int meta = 0;
 	int rc;
 
 	q = &dev->queues[qid];
@@ -323,7 +349,29 @@ 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(q->peer_csum)) {
+			struct ntb_netdev_hdr hdr = {
+				.len = cpu_to_le16(sizeof(hdr)),
+				.flags = cpu_to_le16(NTB_NETDEV_HDR_F_CSUM),
+			};
+
+			if (skb_cow_head(skb, sizeof(hdr)))
+				goto drop;
+
+			hdr.csum_start = cpu_to_le16(skb_checksum_start_offset(skb));
+			hdr.csum_offset = cpu_to_le16(skb->csum_offset);
+			hdr_len = sizeof(hdr);
+			/* Keep skb->len unchanged for retries and byte accounting. */
+			memcpy(skb->data - hdr_len, &hdr, hdr_len);
+			meta = NTB_NETDEV_META_HDR;
+		} else if (skb_checksum_help(skb)) {
+			goto drop;
+		}
+	}
+
+	rc = ntb_transport_tx_enqueue(q->qp, skb, skb->data - hdr_len,
+				      skb->len + hdr_len, meta);
 	if (rc) {
 		if (rc == -EAGAIN || rc == -EBUSY) {
 			netif_stop_subqueue(ndev, q->qid);
@@ -346,6 +394,28 @@ 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)
+		features &= ~NETIF_F_CSUM_MASK;
+
+	return vlan_features_check(skb, features);
+}
+
+static netdev_features_t ntb_netdev_fix_features(struct net_device *ndev,
+						 netdev_features_t features)
+{
+	/* RX checksum support is exchanged at link-up. */
+	if (netif_running(ndev))
+		features = (features & ~NETIF_F_RXCSUM) |
+			   (ndev->features & NETIF_F_RXCSUM);
+
+	return features;
+}
+
 static void ntb_netdev_tx_timer(struct timer_list *t)
 {
 	struct ntb_netdev_queue *q = timer_container_of(q, t, tx_timer);
@@ -369,6 +439,16 @@ static void ntb_netdev_tx_timer(struct timer_list *t)
 	}
 }
 
+static void ntb_netdev_link_up(struct ntb_netdev_queue *q)
+{
+	u32 caps = 0;
+
+	WRITE_ONCE(q->peer_csum, false);
+	if (q->ntdev->ndev->features & NETIF_F_RXCSUM)
+		caps = NTB_NETDEV_CAP_CSUM;
+	ntb_transport_link_up(q->qp, caps);
+}
+
 static int ntb_netdev_open(struct net_device *ndev)
 {
 	struct ntb_netdev *dev = netdev_priv(ndev);
@@ -391,7 +471,7 @@ static int ntb_netdev_open(struct net_device *ndev)
 	netif_tx_stop_all_queues(ndev);
 
 	for (q = 0; q < dev->num_queues; q++)
-		ntb_transport_link_up(dev->queues[q].qp, 0);
+		ntb_netdev_link_up(&dev->queues[q]);
 
 	return 0;
 
@@ -420,6 +500,9 @@ static int ntb_netdev_close(struct net_device *ndev)
 		timer_delete_sync(&queue->tx_timer);
 	}
 
+	/* Apply RX checksum changes deferred while the interface was up. */
+	netdev_update_features(ndev);
+
 	return 0;
 }
 
@@ -475,7 +558,7 @@ static int ntb_netdev_change_mtu(struct net_device *ndev, int new_mtu)
 	WRITE_ONCE(ndev->mtu, new_mtu);
 
 	for (q = 0; q < dev->num_queues; q++)
-		ntb_transport_link_up(dev->queues[q].qp, 0);
+		ntb_netdev_link_up(&dev->queues[q]);
 
 	return 0;
 
@@ -496,6 +579,8 @@ 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_fix_features = ntb_netdev_fix_features,
 	.ndo_change_mtu = ntb_netdev_change_mtu,
 	.ndo_set_mac_address = eth_mac_addr,
 };
@@ -583,7 +668,7 @@ static int ntb_inc_channels(struct net_device *ndev,
 
 	if (running)
 		for (q = old; q < new; q++)
-			ntb_transport_link_up(dev->queues[q].qp, 0);
+			ntb_netdev_link_up(&dev->queues[q]);
 
 	return 0;
 
@@ -716,7 +801,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 v4 10/10] net: ntb_netdev: Preserve CHECKSUM_PARTIAL across NTB

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 3

- [Medium] When TX checksum offload is negotiated (skb->ip_summed ==
  CHECKSUM_PARTIAL and READ_ONCE(q->peer_csum) true),…
- [Low] Growing `struct ntb_netdev_hdr` from 4 to 8 bytes silently
  shrinks the usable MTU by 4 bytes for every user, including users who…
- [Low] ntb_netdev advertises NETIF_F_HW_CSUM in `ndev->hw_features` and
  installs an `ndo_features_check` that calls `vlan_features_check()`,…
- [Low] ntb_netdev_close() flips NETIF_F_RXCSUM (via
  netdev_update_features()) while the NTB transport RX callback…

--

net: ntb_netdev: Preserve CHECKSUM_PARTIAL across NTB

This adds an optional ntb_netdev header that carries csum_start and
csum_offset so CHECKSUM_PARTIAL survives the NTB hop, with receive support
exchanged at QP link-up and a software fallback when the peer does not
advertise it.  Both the TX and RX checksum features stay off by default and
NETIF_F_RXCSUM is frozen while the interface is up.

A few questions below.

> diff --git a/drivers/net/ntb_netdev.c b/drivers/net/ntb_netdev.c
> index 67cfe0f1a49a2..d889c8538be0c 100644
> --- a/drivers/net/ntb_netdev.c
> +++ b/drivers/net/ntb_netdev.c

[ ... ]

> @@ -29,12 +30,19 @@ static unsigned int tx_stop = 5;
>  #define NTB_NETDEV_MAX_QUEUES		64
>  #define NTB_NETDEV_DEFAULT_QUEUES	1
>  
> +#define NTB_NETDEV_CAP_CSUM		BIT(0)
> +
>  /* An ntb_netdev_hdr precedes the packet. */
>  #define NTB_NETDEV_META_HDR		BIT(0)
>  
> +#define NTB_NETDEV_HDR_F_CSUM		BIT(0)
> +
>  struct ntb_netdev_hdr {
>  	__le16 len;		/* Header length in bytes, a multiple of 2. */
>  	__le16 flags;
> +	/* From the packet start, excluding this header. */
> +	__le16 csum_start;
> +	__le16 csum_offset;
>  };

[Severity: Low]
Does growing struct ntb_netdev_hdr from 4 to 8 bytes shrink the usable MTU
for everyone, including users who never enable the new opt-in features?

Both the default MTU and the MTU ceiling are derived from sizeof(struct
ntb_netdev_hdr).  In ntb_netdev_probe():

	ndev->mtu = ntb_transport_max_size(dev->queues[0].qp) - ETH_HLEN -
		    sizeof(struct ntb_netdev_hdr);

and in ntb_netdev_change_mtu():

	size = new_mtu + ETH_HLEN + sizeof(struct ntb_netdev_hdr);
	if (size > ntb_transport_max_size(dev->queues[0].qp))
		return -EINVAL;

With the default transport_mtu of 0x10000, max_size is 65524, so the
default and maximum MTU move from 65506 to 65502.  Would an existing
"ip link set ntb0 mtu 65506" now fail with -EINVAL?  The RX buffer size and
ndev->needed_headroom also grow by 4 bytes unconditionally.

Could the commit message mention this, given it describes the change as
opt-in?

> @@ -189,16 +199,29 @@ static void ntb_netdev_rx_handler(struct ntb_transport_qp *qp, void *qp_data,
>  
>  	skb_put(skb, len + hdr_len);
>  	if (hdr) {
> +		u16 offset = le16_to_cpu(hdr->csum_offset);
> +		u16 start = le16_to_cpu(hdr->csum_start);
>  		u16 flags = le16_to_cpu(hdr->flags);
>  
>  		skb_pull(skb, hdr_len);
> -		if (flags)
> +		if (flags & ~NTB_NETDEV_HDR_F_CSUM)
>  			goto rx_drop;
> +
> +		if (flags & NTB_NETDEV_HDR_F_CSUM) {
> +			if (!(ndev->features & NETIF_F_RXCSUM)) {
> +				ntb_netdev_rx_stats_add(ndev, len);
> +				DEV_STATS_INC(ndev, rx_dropped);
> +				goto rx_free;
> +			}
> +
> +			if (start < ETH_HLEN ||
> +			    !skb_partial_csum_set(skb, start, offset))
> +				goto rx_drop;
> +		}
>  	}

[ ... ]

> @@ -316,6 +340,8 @@ 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 hdr_len = 0;
> +	unsigned int meta = 0;
>  	int rc;
>  
>  	q = &dev->queues[qid];
> @@ -323,7 +349,29 @@ 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(q->peer_csum)) {
> +			struct ntb_netdev_hdr hdr = {
> +				.len = cpu_to_le16(sizeof(hdr)),
> +				.flags = cpu_to_le16(NTB_NETDEV_HDR_F_CSUM),
> +			};
> +
> +			if (skb_cow_head(skb, sizeof(hdr)))
> +				goto drop;
> +
> +			hdr.csum_start = cpu_to_le16(skb_checksum_start_offset(skb));
> +			hdr.csum_offset = cpu_to_le16(skb->csum_offset);
> +			hdr_len = sizeof(hdr);
> +			/* Keep skb->len unchanged for retries and byte accounting. */
> +			memcpy(skb->data - hdr_len, &hdr, hdr_len);
> +			meta = NTB_NETDEV_META_HDR;
> +		} else if (skb_checksum_help(skb)) {
> +			goto drop;
> +		}
> +	}
> +
> +	rc = ntb_transport_tx_enqueue(q->qp, skb, skb->data - hdr_len,
> +				      skb->len + hdr_len, meta);

[Severity: Medium]
Should the header only be prepended when skb->len + sizeof(hdr) still fits
the transport frame?

The MTU accounting reserves exactly 8 bytes on top of MTU + ETH_HLEN, so the
on-wire budget equals ntb_transport_max_size() and the default MTU already
sits at that maximum.  If skb->len is larger than ndev->mtu + ETH_HLEN, the
enqueue is rejected in ntb_transport_tx_enqueue():

	if (len > qp->tx_max_frame - sizeof(struct ntb_payload_header))
		return -EMSGSIZE;

-EMSGSIZE is neither -EAGAIN nor -EBUSY, so it lands in the drop path:

drop:
	dev_kfree_skb_any(skb);
	DEV_STATS_INC(ndev, tx_dropped);
	return NETDEV_TX_OK;

Frames slightly over MTU + ETH_HLEN do reach ndo_start_xmit with
CHECKSUM_PARTIAL: __is_skb_forwardable() allows dev->mtu +
dev->hard_header_len + VLAN_HLEN for bridged traffic, and packet_snd()
allows len <= dev->mtu + reserve + VLAN_HLEN while virtio_net_hdr_to_skb()
marks the skb CHECKSUM_PARTIAL.  ntb_netdev_features_check() only clears
NETIF_F_CSUM_MASK for csum_start < ETH_HLEN, so those frames keep
CHECKSUM_PARTIAL and take the header path.

Before this patch such a frame was transmitted with a software checksum;
now it is freed and counted as tx_dropped.  Would falling back to
skb_checksum_help() when the header does not fit preserve the old
behaviour?

> @@ -346,6 +394,28 @@ 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)
> +		features &= ~NETIF_F_CSUM_MASK;
> +
> +	return vlan_features_check(skb, features);
> +}

[Severity: Low]
Can vlan_features_check() do anything useful here while ndev->vlan_features
stays at 0?

netif_skb_features() has already masked the features for any tagged skb
before ndo_features_check runs:

net/core/dev.c:netif_skb_features() {
	...
	if (skb_vlan_tagged(skb))
		features = netdev_intersect_features(features,
						     dev->vlan_features |
						     NETIF_F_HW_VLAN_CTAG_TX |
						     NETIF_F_HW_VLAN_STAG_TX);
	...
}

ntb_netdev_probe() only sets ndev->features and ndev->hw_features, so
NETIF_F_HW_CSUM is not in vlan_features and cannot be intersected back in.
vlan_dev_fix_features() derives the VLAN upper device features from
real_dev->vlan_features as well.

Does that mean VLAN traffic (on a VLAN upper device or as tagged frames on
ntb0) always takes the skb_checksum_help() path and never uses the new
header?  Should NETIF_F_HW_CSUM be added to ndev->vlan_features?

[ ... ]

> @@ -420,6 +500,9 @@ static int ntb_netdev_close(struct net_device *ndev)
>  		timer_delete_sync(&queue->tx_timer);
>  	}
>  
> +	/* Apply RX checksum changes deferred while the interface was up. */
> +	netdev_update_features(ndev);
> +
>  	return 0;
>  }

[Severity: Low]
Is ndev->features stable here with respect to the transport RX callback?

netdev_update_features() mutates NETIF_F_RXCSUM while
ntb_netdev_rx_handler() may be reading the same flag from the tasklet:

		if (flags & NTB_NETDEV_HDR_F_CSUM) {
			if (!(ndev->features & NETIF_F_RXCSUM)) {

Nothing in the close path appears to quiesce that tasklet.
ntb_transport_link_down() only clears client_ready, withdraws the QP
readiness bit and sends the link-down descriptor:

drivers/ntb/ntb_transport.c:ntb_transport_link_down() {
	...
	qp->client_ready = false;
	...
	ntb_qp_up_request(qp, false);

	if (qp->link_is_up)
		ntb_send_link_down(qp);
}

tasklet_kill(&qp->rxc_db_work) only appears in ntb_transport_free_queue(),
and the doorbell is not masked, so ntb_transport_rxc_db() ->
ntb_process_rxc() -> qp->rx_handler can still run while
ntb_netdev_close() flips the feature bit.  Frames in flight would then
either get an extra rx_dropped or be pushed to the stack with
CHECKSUM_PARTIAL after RXCSUM was cleared.

Would using the per-QP capability negotiated with the peer instead of the
device-global ndev->features in the RX path avoid this?

> @@ -716,7 +801,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);

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