[PATCH v2 00/14] NTB: ntb_transport: Miscellaneous fixes

[PATCH v2 00/14] NTB: ntb_transport: Miscellaneous fixes

[Koichiro Den]

Hi,

This series attempts to fix miscellaneous issues in ntb_transport. Some
of these were split from the direct TX/RX series v1 [1], as they stand
on their own. Based on v7.3-rc2.

Dave, could you please review this series and ack if it looks good to
you?

[1] https://lore.kernel.org/r/20260810165136.2292436-1-den@valinux.co.jp/
    (The v2 of [1] will be based off of this series to avoid conflicts.)

Best regards,
Koichiro
---
Changes in v2:
  - Address Sashiko feedback, including regressions introduced by v1,
    with new patches 5 and 10.
    Note that to keep the series manageable, this revision does not
    attempt to fix all the reported pre-existing issues.
  - Add a fix for client removal ordering (patch 14).
  - Reorder patches, moving v1 patch 4 after the fixes it depends on.
  - Add missing Cc: stable@vger.kernel.org.

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


Koichiro Den (14):
  NTB: ntb_transport: Remove the device debugfs directory
  NTB: ntb_transport: Start TX offload thread after queue setup
  NTB: ntb_transport: Avoid deadlock when cancelling link work
  NTB: ntb_transport: Publish link state after QP setup
  NTB: ntb_transport: Avoid losing QP link-up requests
  NTB: ntb_transport: Clear link state before QP cleanup
  NTB: ntb_transport: Stop QP work before freeing a queue
  NTB: ntb_transport: Stop RX tasklet scheduling before freeing a queue
  NTB: ntb_transport: Drain RX tasklets during link cleanup
  NTB: ntb_transport: Wait for RX completions before resetting a QP
  NTB: ntb_transport: Prepare remote RX info accesses for MW teardown
  NTB: ntb_transport: Clear QP pointers when freeing an MW
  NTB: ntb_transport: Abort link setup on QP MW allocation failure
  NTB: ntb_transport: Remove clients before freeing transport resources

 drivers/ntb/ntb_transport.c | 232 +++++++++++++++++++++++++-----------
 1 file changed, 163 insertions(+), 69 deletions(-)

base-commit: df2908090cda368b01ff43709f51890076c56157
-- 
2.51.0

[PATCH v2 01/14] NTB: ntb_transport: Remove the device debugfs directory

[Koichiro Den]

ntb_transport_free() removes QP debugfs directories but leaves the
device directory. On rebind, debugfs_create_dir() fails with -EEXIST
and QP statistics files are not recreated. Module unload masks this
by removing the entire debugfs tree.

To reproduce:

  # ls /sys/kernel/debug/ntb_transport/0001:10:00.0/
  qp0
  # echo 0001:10:00.0 > /sys/bus/ntb/drivers/ntb_transport/unbind
  # ls /sys/kernel/debug/ntb_transport/
  0001:10:00.0   <-- should not remain
  # echo 0001:10:00.0 > /sys/bus/ntb/drivers/ntb_transport/bind

  .. and then dmesg shows:
  debugfs: '0001:10:00.0' already exists in 'ntb_transport'

  # ls /sys/kernel/debug/ntb_transport/0001:10:00.0/
  (nothing)      <-- should be 'qp0'

Remove the device debugfs tree on teardown and probe failure.

Verified that unbind removes the directory and rebind recreates qp0.

Fixes: c8650fd03d32 ("NTB: Fix transport stats for multiple devices")
Cc: stable@vger.kernel.org
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v2:
  - No changes.

 drivers/ntb/ntb_transport.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
index f9caa1a653c5..3389d6ca9ebd 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -1382,6 +1382,7 @@ static int ntb_transport_probe(struct ntb_client *self, struct ntb_dev *ndev)
 err3:
 	ntb_clear_ctx(ndev);
 err2:
+	debugfs_remove_recursive(nt->debugfs_node_dir);
 	kfree(nt->qp_vec);
 err1:
 	while (i--) {
@@ -1401,6 +1402,8 @@ static void ntb_transport_free(struct ntb_client *self, struct ntb_dev *ndev)
 	u64 qp_bitmap_alloc;
 	int i;
 
+	debugfs_remove_recursive(nt->debugfs_node_dir);
+
 	ntb_transport_link_cleanup(nt);
 	cancel_work_sync(&nt->link_cleanup);
 	cancel_delayed_work_sync(&nt->link_work);
@@ -1412,7 +1415,6 @@ static void ntb_transport_free(struct ntb_client *self, struct ntb_dev *ndev)
 		qp = &nt->qp_vec[i];
 		if (qp_bitmap_alloc & BIT_ULL(i))
 			ntb_transport_free_queue(qp);
-		debugfs_remove_recursive(qp->debugfs_dir);
 	}
 
 	ntb_link_disable(ndev);
-- 
2.51.0

[PATCH v2 02/14] NTB: ntb_transport: Start TX offload thread after queue setup

[Koichiro Den]

ntb_transport_create_queue() starts the per-QP TX offload thread before
DMA mappings and queue entries are allocated. If later setup fails, the
error path returns the QP to the free bitmap without stopping the
thread. A retry can then reinitialize its waitqueue while the old thread
is still waiting on it.

Start the thread after queue setup.

Fixes: 322617a06c97 ("NTB: ntb_transport: Add 'tx_memcpy_offload' module option")
Cc: stable@vger.kernel.org
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v2:
  - No changes.

NOTE: Originally submitted as part of the direct TX/RX series v1:
https://lore.kernel.org/r/20260810165136.2292436-4-den@valinux.co.jp/

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

diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
index 3389d6ca9ebd..55a20ae9a85e 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -2055,20 +2055,6 @@ ntb_transport_create_queue(void *data, struct device *client_dev,
 	qp->tx_handler = handlers->tx_handler;
 	qp->event_handler = handlers->event_handler;
 
-	init_waitqueue_head(&qp->tx_offload_wq);
-	if (tx_memcpy_offload) {
-		qp->tx_offload_thread = kthread_run(ntb_tx_memcpy_kthread, qp,
-						    "ntb-txcpy/%s/%u",
-						    pci_name(ndev->pdev), qp->qp_num);
-		if (IS_ERR(qp->tx_offload_thread)) {
-			dev_warn(&nt->ndev->dev,
-				 "tx memcpy offload thread creation failed: %ld; falling back to inline copy\n",
-				 PTR_ERR(qp->tx_offload_thread));
-			qp->tx_offload_thread = NULL;
-		}
-	} else
-		qp->tx_offload_thread = NULL;
-
 	dma_cap_zero(dma_mask);
 	dma_cap_set(DMA_MEMCPY, dma_mask);
 
@@ -2129,6 +2115,20 @@ ntb_transport_create_queue(void *data, struct device *client_dev,
 			     &qp->tx_free_q);
 	}
 
+	init_waitqueue_head(&qp->tx_offload_wq);
+	qp->tx_offload_thread = NULL;
+	if (tx_memcpy_offload) {
+		qp->tx_offload_thread = kthread_run(ntb_tx_memcpy_kthread, qp,
+						    "ntb-txcpy/%s/%u",
+						    pci_name(ndev->pdev), qp->qp_num);
+		if (IS_ERR(qp->tx_offload_thread)) {
+			dev_warn(&nt->ndev->dev,
+				 "tx memcpy offload thread creation failed: %ld; falling back to inline copy\n",
+				 PTR_ERR(qp->tx_offload_thread));
+			qp->tx_offload_thread = NULL;
+		}
+	}
+
 	ntb_db_clear(qp->ndev, qp_bit);
 	ntb_db_clear_mask(qp->ndev, qp_bit);
 
-- 
2.51.0

[PATCH v2 03/14] NTB: ntb_transport: Avoid deadlock when cancelling link work

[Koichiro Den]

During initial link setup, ntb_transport_link_work() can retry with
nt->link_is_up still false. A retry can block on link_event_lock
while cleanup holds it and waits in cancel_delayed_work_sync(),
leading to deadlock.

Move the conditional cancellation outside link_event_lock, before
QP cleanup. Keep QP cleanup and MW release under the lock so link
work cannot restart QPs between them. Put the locking in
ntb_transport_link_cleanup() to cover both worker and remove paths.

Fixes: 3db835dd8f9a ("ntb: Add mutex to make link_event_callback executed linearly.")
Cc: stable@vger.kernel.org
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v2:
  - No changes.

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

diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
index 55a20ae9a85e..c77b173dca01 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -960,6 +960,15 @@ static void ntb_transport_link_cleanup(struct ntb_transport_ctx *nt)
 	struct ntb_transport_qp *qp;
 	u64 qp_bitmap_alloc;
 	unsigned int i, count;
+	bool cancel_link_work;
+
+	scoped_guard(mutex, &nt->link_event_lock)
+		cancel_link_work = !nt->link_is_up;
+
+	if (cancel_link_work)
+		cancel_delayed_work_sync(&nt->link_work);
+
+	guard(mutex)(&nt->link_event_lock);
 
 	qp_bitmap_alloc = nt->qp_bitmap & ~nt->qp_bitmap_free;
 
@@ -972,9 +981,6 @@ static void ntb_transport_link_cleanup(struct ntb_transport_ctx *nt)
 			cancel_delayed_work_sync(&qp->link_work);
 		}
 
-	if (!nt->link_is_up)
-		cancel_delayed_work_sync(&nt->link_work);
-
 	for (i = 0; i < nt->mw_count; i++)
 		ntb_free_mw(nt, i);
 
@@ -992,7 +998,6 @@ static void ntb_transport_link_cleanup_work(struct work_struct *work)
 	struct ntb_transport_ctx *nt =
 		container_of(work, struct ntb_transport_ctx, link_cleanup);
 
-	guard(mutex)(&nt->link_event_lock);
 	ntb_transport_link_cleanup(nt);
 }
 
-- 
2.51.0

[PATCH v2 04/14] NTB: ntb_transport: Publish link state after QP setup

[Koichiro Den]

ntb_transport_link_work() marks the transport link up before setting
up the QPs' MW and peer MSI state. A concurrent ntb_transport_link_up()
can then queue QP link work, which may enable RX and notify the client
before setup finishes.

Publish link_is_up with a release store after setting up all QPs,
and use acquire loads before queuing QP link work.

Fixes: fce8a7bb5b4b ("PCI-Express Non-Transparent Bridge Support")
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/r/anyKbq3mpLG4y7rb@SMW015318
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v2:
  - No changes.

 drivers/ntb/ntb_transport.c | 40 +++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 15 deletions(-)

diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
index c77b173dca01..75d4a2e021f4 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -922,6 +922,16 @@ static void ntb_qp_link_down_reset(struct ntb_transport_qp *qp)
 		qp->remote_rx_info->entry = qp->rx_max_entry - 1;
 }
 
+static void ntb_transport_schedule_qp_link(struct ntb_transport_qp *qp,
+					   unsigned long delay)
+{
+	struct ntb_transport_ctx *nt = qp->transport;
+
+	/* Pair with the link publication in ntb_transport_link_work(). */
+	if (smp_load_acquire(&nt->link_is_up))
+		schedule_delayed_work(&qp->link_work, delay);
+}
+
 static void ntb_qp_link_cleanup(struct ntb_transport_qp *qp)
 {
 	struct ntb_transport_ctx *nt = qp->transport;
@@ -941,13 +951,10 @@ static void ntb_qp_link_cleanup_work(struct work_struct *work)
 	struct ntb_transport_qp *qp = container_of(work,
 						   struct ntb_transport_qp,
 						   link_cleanup);
-	struct ntb_transport_ctx *nt = qp->transport;
 
 	ntb_qp_link_cleanup(qp);
-
-	if (nt->link_is_up)
-		schedule_delayed_work(&qp->link_work,
-				      msecs_to_jiffies(NTB_LINK_DOWN_TIMEOUT));
+	ntb_transport_schedule_qp_link(qp,
+				       msecs_to_jiffies(NTB_LINK_DOWN_TIMEOUT));
 }
 
 static void ntb_qp_link_down(struct ntb_transport_qp *qp)
@@ -1089,16 +1096,19 @@ static void ntb_transport_link_work(struct work_struct *work)
 			goto out1;
 	}
 
-	nt->link_is_up = true;
-
 	for (i = 0; i < nt->qp_count; i++) {
-		struct ntb_transport_qp *qp = &nt->qp_vec[i];
-
 		ntb_transport_setup_qp_mw(nt, i);
 		ntb_transport_setup_qp_peer_msi(nt, i);
+	}
+
+	/* Publish the link only after every QP has been set up. */
+	smp_store_release(&nt->link_is_up, true);
+
+	for (i = 0; i < nt->qp_count; i++) {
+		struct ntb_transport_qp *qp = &nt->qp_vec[i];
 
 		if (qp->client_ready)
-			schedule_delayed_work(&qp->link_work, 0);
+			ntb_transport_schedule_qp_link(qp, 0);
 	}
 
 	return;
@@ -1146,9 +1156,10 @@ static void ntb_qp_link_work(struct work_struct *work)
 
 		if (qp->active)
 			tasklet_schedule(&qp->rxc_db_work);
-	} else if (nt->link_is_up)
-		schedule_delayed_work(&qp->link_work,
-				      msecs_to_jiffies(NTB_LINK_DOWN_TIMEOUT));
+	} else {
+		ntb_transport_schedule_qp_link(qp,
+					       msecs_to_jiffies(NTB_LINK_DOWN_TIMEOUT));
+	}
 }
 
 static int ntb_transport_init_queue(struct ntb_transport_ctx *nt,
@@ -2392,8 +2403,7 @@ void ntb_transport_link_up(struct ntb_transport_qp *qp)
 
 	qp->client_ready = true;
 
-	if (qp->transport->link_is_up)
-		schedule_delayed_work(&qp->link_work, 0);
+	ntb_transport_schedule_qp_link(qp, 0);
 }
 EXPORT_SYMBOL_GPL(ntb_transport_link_up);
 
-- 
2.51.0

[PATCH v2 05/14] NTB: ntb_transport: Avoid losing QP link-up requests

[Koichiro Den]

ntb_netdev_open() can call ntb_transport_link_up() while the transport
worker is completing setup on another CPU. Concurrent transport setup
and a client link-up request can both read the other's flag as false and
leave QP link work unqueued. The QP then stays down until another link
event or client link-up request.

This is the store-buffering pattern described in
tools/memory-model/Documentation/recipes.txt ("Store buffering").

Add a full barrier between the store and load on each side, and
mark the client_ready accesses with READ_ONCE()/WRITE_ONCE().

Fixes: fce8a7bb5b4b ("PCI-Express Non-Transparent Bridge Support")
Cc: stable@vger.kernel.org
Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/r/20260907144701.702E41F00A3A@smtp.kernel.org/
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v2:
  - New patch (Sashiko)

 drivers/ntb/ntb_transport.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
index 75d4a2e021f4..1332d53bcfe7 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -1104,10 +1104,16 @@ static void ntb_transport_link_work(struct work_struct *work)
 	/* Publish the link only after every QP has been set up. */
 	smp_store_release(&nt->link_is_up, true);
 
+	/*
+	 * Prevent both sides from missing each other's flag. Pairs with
+	 * the barrier in ntb_transport_link_up().
+	 */
+	smp_mb();
+
 	for (i = 0; i < nt->qp_count; i++) {
 		struct ntb_transport_qp *qp = &nt->qp_vec[i];
 
-		if (qp->client_ready)
+		if (READ_ONCE(qp->client_ready))
 			ntb_transport_schedule_qp_link(qp, 0);
 	}
 
@@ -2401,7 +2407,10 @@ void ntb_transport_link_up(struct ntb_transport_qp *qp)
 	if (!qp)
 		return;
 
-	qp->client_ready = true;
+	WRITE_ONCE(qp->client_ready, true);
+
+	/* Pairs with the barrier in ntb_transport_link_work(). */
+	smp_mb();
 
 	ntb_transport_schedule_qp_link(qp, 0);
 }
-- 
2.51.0

[PATCH v2 06/14] NTB: ntb_transport: Clear link state before QP cleanup

[Koichiro Den]

Cleanup leaves the transport link marked up after releasing its MWs.
A subsequent client link-up request can therefore start QP link work
before the transport has been set up again.

Clear link_is_up before cancelling QP work and releasing the MWs.
Have QP link work return if the transport went down after it was
queued.

Fixes: e26a5843f7f5 ("NTB: Split ntb_hw_intel and ntb_transport drivers")
Cc: stable@vger.kernel.org
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v2:
  - No changes.

Note: this is a reworked version of my earlier, withdrawn patch:
https://lore.kernel.org/r/20260717061223.2203863-1-den@valinux.co.jp/

 drivers/ntb/ntb_transport.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
index 1332d53bcfe7..8dd1770aaaf1 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -977,6 +977,8 @@ static void ntb_transport_link_cleanup(struct ntb_transport_ctx *nt)
 
 	guard(mutex)(&nt->link_event_lock);
 
+	WRITE_ONCE(nt->link_is_up, false);
+
 	qp_bitmap_alloc = nt->qp_bitmap & ~nt->qp_bitmap_free;
 
 	/* Pass along the info to any clients */
@@ -1142,7 +1144,9 @@ static void ntb_qp_link_work(struct work_struct *work)
 	struct ntb_transport_ctx *nt = qp->transport;
 	int val;
 
-	WARN_ON(!nt->link_is_up);
+	/* Pair with the link publication in ntb_transport_link_work(). */
+	if (!smp_load_acquire(&nt->link_is_up))
+		return;
 
 	val = ntb_spad_read(nt->ndev, QP_LINKS);
 
-- 
2.51.0

[PATCH v2 07/14] NTB: ntb_transport: Stop QP work before freeing a queue

[Koichiro Den]

ntb_transport_free_queue() cancels qp->link_work but not qp->link_cleanup.
A peer link-down message can queue cleanup while ntb_netdev is freeing
the QP. Cleanup can then requeue link work after the queue resources
have been freed.

Disable and wait for cleanup, then link work, before freeing resources.
Unlike cancel, disable also prevents the RX tasklet and transport link
setup from queuing more work. Enable the works only after queue creation
succeeds.

Clear client_ready first so RX completions and transport link setup see
that the client is no longer ready. Clear link_is_up and active after
the workers stop, since link work can set both back to true.

Fixes: 7b4f2d3c3b82 ("NTB: No sleeping in interrupt context")
Cc: stable@vger.kernel.org
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v2:
  - No changes.

Reworked from the corresponding patch in the direct TX/RX series v1:
https://lore.kernel.org/r/20260810165136.2292436-5-den@valinux.co.jp/

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

diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
index 8dd1770aaaf1..e5599c7ca93f 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -1242,6 +1242,8 @@ static int ntb_transport_init_queue(struct ntb_transport_ctx *nt,
 
 	INIT_DELAYED_WORK(&qp->link_work, ntb_qp_link_work);
 	INIT_WORK(&qp->link_cleanup, ntb_qp_link_cleanup_work);
+	disable_delayed_work(&qp->link_work);
+	disable_work(&qp->link_cleanup);
 
 	spin_lock_init(&qp->ntb_rx_q_lock);
 	spin_lock_init(&qp->ntb_tx_free_q_lock);
@@ -2155,6 +2157,9 @@ ntb_transport_create_queue(void *data, struct device *client_dev,
 		}
 	}
 
+	enable_work(&qp->link_cleanup);
+	enable_delayed_work(&qp->link_work);
+
 	ntb_db_clear(qp->ndev, qp_bit);
 	ntb_db_clear_mask(qp->ndev, qp_bit);
 
@@ -2200,6 +2205,10 @@ void ntb_transport_free_queue(struct ntb_transport_qp *qp)
 
 	pdev = qp->ndev->pdev;
 
+	qp->client_ready = false;
+	disable_work_sync(&qp->link_cleanup);
+	disable_delayed_work_sync(&qp->link_work);
+	qp->link_is_up = false;
 	qp->active = false;
 
 	if (qp->tx_offload_thread) {
@@ -2247,8 +2256,6 @@ void ntb_transport_free_queue(struct ntb_transport_qp *qp)
 	ntb_db_set_mask(qp->ndev, qp_bit);
 	tasklet_kill(&qp->rxc_db_work);
 
-	cancel_delayed_work_sync(&qp->link_work);
-
 	qp->cb_data = NULL;
 	qp->rx_handler = NULL;
 	qp->tx_handler = NULL;
-- 
2.51.0

[PATCH v2 08/14] NTB: ntb_transport: Stop RX tasklet scheduling before freeing a queue

[Koichiro Den]

A caller can read qp->active before teardown clears it, then schedule
the RX tasklet after tasklet_kill() returns. The MSI handler does not
check active at all. Teardown also releases DMA channels before
draining the tasklet.

Protect active updates and the check-and-schedule sequence with
rx_sched_lock, including the MSI path. Clear active under the lock,
then drain the tasklet before releasing DMA channels or queue entries.
QP link work is already disabled, so it cannot reactivate RX. Use a
separate lock to avoid contention with RX list operations.

Fixes: e902133162af ("ntb: stop tasklet from spinning forever during shutdown.")
Cc: stable@vger.kernel.org
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v2:
  - No changes.

 drivers/ntb/ntb_transport.c | 50 +++++++++++++++++++++++--------------
 1 file changed, 31 insertions(+), 19 deletions(-)

diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
index e5599c7ca93f..45d4365becac 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -179,6 +179,8 @@ struct ntb_transport_qp {
 	unsigned int rx_max_frame;
 	unsigned int rx_alloc_entry;
 	dma_cookie_t last_cookie;
+	/* Protect active and RX tasklet scheduling. */
+	spinlock_t rx_sched_lock;
 	struct tasklet_struct rxc_db_work;
 
 	void (*event_handler)(void *data, int status);
@@ -649,11 +651,26 @@ static int ntb_transport_setup_qp_mw(struct ntb_transport_ctx *nt,
 	return 0;
 }
 
+static void ntb_transport_set_qp_active(struct ntb_transport_qp *qp, bool active)
+{
+	guard(spinlock_irqsave)(&qp->rx_sched_lock);
+
+	qp->active = active;
+}
+
+static void ntb_transport_schedule_rxc(struct ntb_transport_qp *qp)
+{
+	guard(spinlock_irqsave)(&qp->rx_sched_lock);
+
+	if (qp->active)
+		tasklet_schedule(&qp->rxc_db_work);
+}
+
 static irqreturn_t ntb_transport_isr(int irq, void *dev)
 {
 	struct ntb_transport_qp *qp = dev;
 
-	tasklet_schedule(&qp->rxc_db_work);
+	ntb_transport_schedule_rxc(qp);
 
 	return IRQ_HANDLED;
 }
@@ -895,7 +912,7 @@ static int ntb_set_mw(struct ntb_transport_ctx *nt, int num_mw,
 static void ntb_qp_link_context_reset(struct ntb_transport_qp *qp)
 {
 	qp->link_is_up = false;
-	qp->active = false;
+	ntb_transport_set_qp_active(qp, false);
 
 	qp->tx_index = 0;
 	qp->rx_index = 0;
@@ -1159,13 +1176,12 @@ static void ntb_qp_link_work(struct work_struct *work)
 	if (val & BIT(qp->qp_num)) {
 		dev_info(&pdev->dev, "qp %d: Link Up\n", qp->qp_num);
 		qp->link_is_up = true;
-		qp->active = true;
+		ntb_transport_set_qp_active(qp, true);
 
 		if (qp->event_handler)
 			qp->event_handler(qp->cb_data, qp->link_is_up);
 
-		if (qp->active)
-			tasklet_schedule(&qp->rxc_db_work);
+		ntb_transport_schedule_rxc(qp);
 	} else {
 		ntb_transport_schedule_qp_link(qp,
 					       msecs_to_jiffies(NTB_LINK_DOWN_TIMEOUT));
@@ -1193,6 +1209,7 @@ static int ntb_transport_init_queue(struct ntb_transport_ctx *nt,
 	qp->ndev = nt->ndev;
 	qp->client_ready = false;
 	qp->event_handler = NULL;
+	spin_lock_init(&qp->rx_sched_lock);
 	ntb_qp_link_context_reset(qp);
 
 	if (mw_num < qp_count % mw_count)
@@ -1729,8 +1746,7 @@ static void ntb_transport_rxc_db(unsigned long data)
 
 	if (i == qp->rx_max_entry) {
 		/* there is more work to do */
-		if (qp->active)
-			tasklet_schedule(&qp->rxc_db_work);
+		ntb_transport_schedule_rxc(qp);
 	} else if (ntb_db_read(qp->ndev) & BIT_ULL(qp->qp_num)) {
 		/* the doorbell bit is set: clear it */
 		ntb_db_clear(qp->ndev, BIT_ULL(qp->qp_num));
@@ -1741,8 +1757,7 @@ static void ntb_transport_rxc_db(unsigned long data)
 		 * ntb_process_rxc and clearing the doorbell bit:
 		 * there might be some more work to do.
 		 */
-		if (qp->active)
-			tasklet_schedule(&qp->rxc_db_work);
+		ntb_transport_schedule_rxc(qp);
 	}
 }
 
@@ -2209,7 +2224,11 @@ void ntb_transport_free_queue(struct ntb_transport_qp *qp)
 	disable_work_sync(&qp->link_cleanup);
 	disable_delayed_work_sync(&qp->link_work);
 	qp->link_is_up = false;
-	qp->active = false;
+	ntb_transport_set_qp_active(qp, false);
+
+	qp_bit = BIT_ULL(qp->qp_num);
+	ntb_db_set_mask(qp->ndev, qp_bit);
+	tasklet_kill(&qp->rxc_db_work);
 
 	if (qp->tx_offload_thread) {
 		kthread_stop(qp->tx_offload_thread);
@@ -2251,11 +2270,6 @@ void ntb_transport_free_queue(struct ntb_transport_qp *qp)
 		dma_release_channel(chan);
 	}
 
-	qp_bit = BIT_ULL(qp->qp_num);
-
-	ntb_db_set_mask(qp->ndev, qp_bit);
-	tasklet_kill(&qp->rxc_db_work);
-
 	qp->cb_data = NULL;
 	qp->rx_handler = NULL;
 	qp->tx_handler = NULL;
@@ -2350,8 +2364,7 @@ int ntb_transport_rx_enqueue(struct ntb_transport_qp *qp, void *cb, void *data,
 
 	ntb_list_add(&qp->ntb_rx_q_lock, &entry->entry, &qp->rx_pend_q);
 
-	if (qp->active)
-		tasklet_schedule(&qp->rxc_db_work);
+	ntb_transport_schedule_rxc(qp);
 
 	return 0;
 }
@@ -2548,8 +2561,7 @@ static void ntb_transport_doorbell_callback(void *data, int vector)
 		qp_num = __ffs(db_bits);
 		qp = &nt->qp_vec[qp_num];
 
-		if (qp->active)
-			tasklet_schedule(&qp->rxc_db_work);
+		ntb_transport_schedule_rxc(qp);
 
 		db_bits &= ~BIT_ULL(qp_num);
 	}
-- 
2.51.0

[PATCH v2 09/14] NTB: ntb_transport: Drain RX tasklets during link cleanup

[Koichiro Den]

ntb_qp_link_cleanup() cancels QP link work but does not wait for the RX
tasklet. The tasklet can still be processing the ring while cleanup
resets the QP, and transport link cleanup can free the MW before the
tasklet finishes.

Clear active under rx_sched_lock and drain the tasklet before resetting
the QP. Temporarily disable QP link work so a concurrent client link-up
request cannot reactivate RX during cleanup, then re-enable it for the
existing link setup paths.

This does not drain RX DMA transfers or their completion callbacks.

Fixes: 9143595a7e05 ("NTB: ntb_transport: Free MWs in ntb_transport_link_cleanup()")
Cc: stable@vger.kernel.org
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v2:
  - No changes.

 drivers/ntb/ntb_transport.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
index 45d4365becac..36797ea3ff45 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -956,11 +956,16 @@ static void ntb_qp_link_cleanup(struct ntb_transport_qp *qp)
 
 	dev_info(&pdev->dev, "qp %d: Link Cleanup\n", qp->qp_num);
 
-	cancel_delayed_work_sync(&qp->link_work);
+	disable_delayed_work_sync(&qp->link_work);
+	ntb_transport_set_qp_active(qp, false);
+	tasklet_kill(&qp->rxc_db_work);
+
 	ntb_qp_link_down_reset(qp);
 
 	if (qp->event_handler)
 		qp->event_handler(qp->cb_data, qp->link_is_up);
+
+	enable_delayed_work(&qp->link_work);
 }
 
 static void ntb_qp_link_cleanup_work(struct work_struct *work)
-- 
2.51.0

[PATCH v2 10/14] NTB: ntb_transport: Wait for RX completions before resetting a QP

[Koichiro Den]

Transport link cleanup can free an MW still used by RX DMA or its
completion path. QP-only cleanup retains the MW, but can restart RX
on ring slots whose old completion callbacks have not yet cleared
the headers.

Wait for rx_post_q to empty before resetting the QP. ntb_complete_rxc()
finishes its MW accesses before removing each entry under ntb_rx_q_lock,
so cleanup can free the MW without racing with these RX accesses.
Using dmaengine_terminate_sync() instead would not work with drivers
such as IOAT that lack the required ops. Cookie-based waits would
not work with DMA_COMPLETION_NO_ORDER either.

DMA teardown in ntb_transport_free_queue() is unchanged.

Fixes: 9143595a7e05 ("NTB: ntb_transport: Free MWs in ntb_transport_link_cleanup()")
Cc: stable@vger.kernel.org
Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/r/20260907144257.767281F00A3A@smtp.kernel.org/
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v2:
  - New patch (Sashiko).

 drivers/ntb/ntb_transport.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
index 36797ea3ff45..7ccba2c04f54 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -949,6 +949,13 @@ static void ntb_transport_schedule_qp_link(struct ntb_transport_qp *qp,
 		schedule_delayed_work(&qp->link_work, delay);
 }
 
+static bool ntb_transport_rx_idle(struct ntb_transport_qp *qp)
+{
+	guard(spinlock_irqsave)(&qp->ntb_rx_q_lock);
+
+	return list_empty(&qp->rx_post_q);
+}
+
 static void ntb_qp_link_cleanup(struct ntb_transport_qp *qp)
 {
 	struct ntb_transport_ctx *nt = qp->transport;
@@ -959,6 +966,17 @@ static void ntb_qp_link_cleanup(struct ntb_transport_qp *qp)
 	disable_delayed_work_sync(&qp->link_work);
 	ntb_transport_set_qp_active(qp, false);
 	tasklet_kill(&qp->rxc_db_work);
+	/*
+	 * Some DMA engines lack terminate/synchronize ops (e.g. IOAT), and
+	 * DMA_COMPLETION_NO_ORDER rules out cookie-based waits.
+	 *
+	 * Waiting for rx_post_q to empty suffices: ntb_complete_rxc() finishes
+	 * its MW accesses before removing each entry under ntb_rx_q_lock.
+	 * qp->active is false and rxc_db_work is stopped, so no new RX DMA
+	 * can be submitted.
+	 */
+	while (!ntb_transport_rx_idle(qp))
+		fsleep(1000);
 
 	ntb_qp_link_down_reset(qp);
 
-- 
2.51.0

[PATCH v2 11/14] NTB: ntb_transport: Prepare remote RX info accesses for MW teardown

[Koichiro Den]

The next patch clears remote_rx_info when freeing its MW.
ntb_transport_tx_free_entry() and debugfs stats reads can run during
link cleanup, so make them handle a NULL pointer.

The pointer is accessed locklessly. Use READ_ONCE() and WRITE_ONCE()
to prevent compiler-induced tearing, and retain the read value so
the NULL check and dereference use the same pointer.

Cc: stable@vger.kernel.org
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v2:
  - No changes.

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

diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
index 7ccba2c04f54..b949f36a4f2d 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -489,6 +489,7 @@ EXPORT_SYMBOL_GPL(ntb_transport_unregister_client);
 static int ntb_qp_debugfs_stats_show(struct seq_file *s, void *v)
 {
 	struct ntb_transport_qp *qp = s->private;
+	struct ntb_rx_info *remote_rx_info;
 
 	if (!qp || !qp->link_is_up)
 		return 0;
@@ -516,7 +517,9 @@ 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);
+	remote_rx_info = READ_ONCE(qp->remote_rx_info);
+	if (remote_rx_info)
+		seq_printf(s, "RRI (T) - \t%u\n", 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');
@@ -611,7 +614,7 @@ static int ntb_transport_setup_qp_mw(struct ntb_transport_ctx *nt,
 	qp->rx_buff = mw->virt_addr + rx_size * (qp_num / mw_count);
 	rx_size -= sizeof(struct ntb_rx_info);
 
-	qp->remote_rx_info = qp->rx_buff + rx_size;
+	WRITE_ONCE(qp->remote_rx_info, qp->rx_buff + rx_size);
 
 	/* Due to housekeeping, there must be atleast 2 buffs */
 	qp->rx_max_frame = min(transport_mtu, rx_size / 2);
@@ -934,9 +937,12 @@ static void ntb_qp_link_context_reset(struct ntb_transport_qp *qp)
 
 static void ntb_qp_link_down_reset(struct ntb_transport_qp *qp)
 {
+	struct ntb_rx_info *remote_rx_info;
+
 	ntb_qp_link_context_reset(qp);
-	if (qp->remote_rx_info)
-		qp->remote_rx_info->entry = qp->rx_max_entry - 1;
+	remote_rx_info = READ_ONCE(qp->remote_rx_info);
+	if (remote_rx_info)
+		remote_rx_info->entry = qp->rx_max_entry - 1;
 }
 
 static void ntb_transport_schedule_qp_link(struct ntb_transport_qp *qp,
@@ -2558,8 +2564,14 @@ EXPORT_SYMBOL_GPL(ntb_transport_max_size);
 
 unsigned int ntb_transport_tx_free_entry(struct ntb_transport_qp *qp)
 {
+	struct ntb_rx_info *remote_rx_info = READ_ONCE(qp->remote_rx_info);
 	unsigned int head = qp->tx_index;
-	unsigned int tail = qp->remote_rx_info->entry;
+	unsigned int tail;
+
+	if (!remote_rx_info)
+		return 0;
+
+	tail = remote_rx_info->entry;
 
 	return tail >= head ? tail - head : qp->tx_max_entry + tail - head;
 }
-- 
2.51.0

[PATCH v2 12/14] NTB: ntb_transport: Clear QP pointers when freeing an MW

[Koichiro Den]

ntb_transport_link_cleanup() frees MW buffers but leaves rx_buff and
remote_rx_info pointing into them. With a QP still allocated, another
link-down notification or transport unbind before MW setup runs again
can make ntb_qp_link_down_reset() write to freed memory through
remote_rx_info.

Clear both pointers in ntb_free_mw() for all QPs using that MW,
including those without a client. This also covers link-setup failures.

How to reproduce:

  1. Load ntb_transport and ntb_netdev on both sides and establish the
     transport/QP links once. Stop traffic, but leave ntb_netdev loaded
     on VHOST so its QPs remain allocated throughout the test.

  2. On HOST, unload ntb_netdev and ntb_transport, leaving ntb_hw_epf
     bound:

     modprobe -r ntb_netdev ntb_transport

     Transport removal sends COMMAND_LINK_DOWN to VHOST. Wait for
     ntb_transport_link_cleanup_work() to return on VHOST, using a
     function-graph trace. The "Link Cleanup" message is printed before
     MW release and is not sufficient to establish completion. Do not
     bring the link back up before the next step.

  3-(A). UAF via repeated link-down notification

     Use ntb_tool on HOST to send another link-down request:

     HOST# modprobe ntb_tool
     HOST# echo N > "/sys/kernel/debug/ntb_tool/$ntb_host_dev/link"

     ==================================================================
     BUG: KASAN: vmalloc-out-of-bounds in ntb_qp_link_down_reset+0x2c0..
     ...
     Call trace:
      ...
      __asan_report_store4_noabort+0x1c/0x28
      ntb_qp_link_down_reset+0x2c0/0x2e0 [ntb_transport]
      ntb_qp_link_cleanup+0xc4/0x148 [ntb_transport]
      ntb_transport_link_cleanup+0x314/0x350 [ntb_transport]
      ntb_transport_link_cleanup_work+0x2c/0x50 [ntb_transport]
      process_one_work+0x5b8/0x12f0
      ...

  3-(B). UAF via transport removal after link-down

     VHOST# echo "$ntb_vhost_dev" > \
         /sys/bus/ntb/drivers/ntb_transport/unbind

     ==================================================================
     BUG: KASAN: vmalloc-out-of-bounds in ntb_qp_link_down_reset+0x2c0..
     ...
     Call trace:
      ...
      __asan_report_store4_noabort+0x1c/0x28
      ntb_qp_link_down_reset+0x2c0/0x2e0 [ntb_transport]
      ntb_qp_link_cleanup+0xc4/0x148 [ntb_transport]
      ntb_transport_link_cleanup+0x314/0x350 [ntb_transport]
      ntb_transport_free+0x68/0x588 [ntb_transport]
      ntb_remove+0x5c/0xa0 [ntb]

Verified that neither test triggers a KASAN report with this patch.

Fixes: cc79bd2738c2 ("ntb: Clean up tx tail index on link down")
Cc: stable@vger.kernel.org
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v2:
  - No changes.

 drivers/ntb/ntb_transport.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
index b949f36a4f2d..096be87e5ede 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -781,10 +781,17 @@ static void ntb_free_mw(struct ntb_transport_ctx *nt, int num_mw)
 {
 	struct ntb_transport_mw *mw = &nt->mw_vec[num_mw];
 	struct device *dma_dev = ntb_get_dma_dev(nt->ndev);
+	unsigned int i;
 
 	if (!mw->virt_addr)
 		return;
 
+	/* Drop references from every QP using this MW. */
+	for (i = num_mw; i < nt->qp_count; i += nt->mw_count) {
+		nt->qp_vec[i].rx_buff = NULL;
+		WRITE_ONCE(nt->qp_vec[i].remote_rx_info, NULL);
+	}
+
 	ntb_mw_clear_trans(nt->ndev, PIDX, num_mw);
 	dma_free_attrs(dma_dev, mw->alloc_size, mw->alloc_addr,
 		       mw->original_dma_addr, DMA_ATTR_FORCE_CONTIGUOUS);
-- 
2.51.0

[PATCH v2 13/14] NTB: ntb_transport: Abort link setup on QP MW allocation failure

[Koichiro Den]

ntb_transport_setup_qp_mw() can fail while growing a QP's RX entry pool,
but the link worker ignores that error. The worker can consequently
publish a QP whose memory-window state is only partly initialized.

Abort on the first QP setup error and release the MWs through the
existing error path instead of publishing the transport link.

Fixes: a754a8fcaf38 ("NTB: allocate number transport entries depending on size of ring size")
Cc: stable@vger.kernel.org
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v2:
  - No changes.

Originally submitted as part of the direct TX/RX series v1:
https://lore.kernel.org/r/20260810165136.2292436-2-den@valinux.co.jp/

 drivers/ntb/ntb_transport.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
index 096be87e5ede..c1e3a077c45e 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -1152,7 +1152,9 @@ static void ntb_transport_link_work(struct work_struct *work)
 	}
 
 	for (i = 0; i < nt->qp_count; i++) {
-		ntb_transport_setup_qp_mw(nt, i);
+		rc = ntb_transport_setup_qp_mw(nt, i);
+		if (rc)
+			goto out1;
 		ntb_transport_setup_qp_peer_msi(nt, i);
 	}
 
-- 
2.51.0

[PATCH v2 14/14] NTB: ntb_transport: Remove clients before freeing transport resources

[Koichiro Den]

Unbinding ntb_transport can call ntb_transport_free() while ntb_netdev
is still bound. The transport frees MWs and QP resources before
unregistering the clients, so the netdev's transmit path and timer can
access freed memory. Its remove callback also calls
ntb_transport_free_queue() on a QP whose resources have already been
released. This teardown order is unsafe and somewhat unintuitive.

The crash can be reproduced with an intensive TX load, during which you
unbind the NTB device. The following is a KASAN report from my
VHOST/HOST setup using vNTB.

  VHOST# sudo iperf3 -ub0 -c $HOST -l 100 -P 100 &
  VHOST# echo $VHOST_NTB_DEV > /sys/bus/ntb/drivers/ntb_transport/unbind

  ==================================================================
  BUG: KASAN: vmalloc-out-of-bounds in ntb_transport_tx_free_entry+0xf0
  ...
  Call trace:
   ...
   __asan_report_load4_noabort+0x1c/0x30
   ntb_transport_tx_free_entry+0xf0/0x130 [ntb_transport]
   ntb_netdev_tx_timer+0x78/0x260 [ntb_netdev]
   ...

Disable and drain transport link work first, then unregister the clients
so they stop using and release their QPs. After that, free any QPs left
over before running transport link cleanup. Disabling the work keeps
link events from restarting setup or cleanup during client removal.

Fixes: fce8a7bb5b4b ("PCI-Express Non-Transparent Bridge Support")
Cc: stable@vger.kernel.org
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
Changes in v2:
  - New patch.

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

diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
index c1e3a077c45e..f24555b38d1b 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -1487,9 +1487,11 @@ static void ntb_transport_free(struct ntb_client *self, struct ntb_dev *ndev)
 
 	debugfs_remove_recursive(nt->debugfs_node_dir);
 
-	ntb_transport_link_cleanup(nt);
-	cancel_work_sync(&nt->link_cleanup);
-	cancel_delayed_work_sync(&nt->link_work);
+	/* Stop transport work before clients release their QPs. */
+	disable_delayed_work_sync(&nt->link_work);
+	disable_work_sync(&nt->link_cleanup);
+
+	ntb_bus_remove(nt);
 
 	qp_bitmap_alloc = nt->qp_bitmap & ~nt->qp_bitmap_free;
 
@@ -1500,11 +1502,10 @@ static void ntb_transport_free(struct ntb_client *self, struct ntb_dev *ndev)
 			ntb_transport_free_queue(qp);
 	}
 
+	ntb_transport_link_cleanup(nt);
 	ntb_link_disable(ndev);
 	ntb_clear_ctx(ndev);
 
-	ntb_bus_remove(nt);
-
 	for (i = nt->mw_count; i--; ) {
 		ntb_free_mw(nt, i);
 		iounmap(nt->mw_vec[i].vbase);
-- 
2.51.0

Re: [PATCH v2 01/14] NTB: ntb_transport: Remove the device debugfs directory

[sashiko-bot]

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

Pre-existing issues:
- [High] Loop variable `i` is overwritten by the `qp_count` loop, causing memory window leaks or out-of-bounds accesses during probe error teardown.
--

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

NTB: ntb_transport: Remove the device debugfs directory

This commit ensures the device debugfs directory is removed during teardown
and probe failure. This prevents -EEXIST errors and missing queue statistics
files on module rebind.

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index f9caa1a653c5b..3389d6ca9ebd2 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -1382,6 +1382,7 @@ static int ntb_transport_probe(struct ntb_client *self, struct ntb_dev *ndev)
>  err3:
>  	ntb_clear_ctx(ndev);
>  err2:
> +	debugfs_remove_recursive(nt->debugfs_node_dir);
>  	kfree(nt->qp_vec);
>  err1:
>  	while (i--) {
>  		mw = &nt->mw_vec[i];
>  		iounmap(mw->vbase);

[Severity: High]
This is a pre-existing issue, but does this error path use the correct
value for 'i' to unmap memory windows?

Earlier in ntb_transport_probe(), 'i' is used to map up to 'mw_count'
windows:

    for (i = 0; i < mw_count; i++) {
        ...
        mw->vbase = ioremap_wc(mw->phys_addr, mw->phys_size);
    }

However, 'i' is subsequently overwritten when initializing queues:

    for (i = 0; i < qp_count; i++) {
        rc = ntb_transport_init_queue(nt, i);
        if (rc)
            goto err2;
    }

If probe fails during queue initialization or later, this cleanup loop uses
the overwritten value of 'i' (representing qp_count) rather than mw_count.

If qp_count < mw_count, the excess memory windows are leaked. If 
qp_count > mw_count (which can occur if max_num_clients bounds qp_count
larger than mw_count), this loop will read out of bounds of nt->mw_vec and
pass invalid pointers to iounmap().

Additionally, does ntb_transport_free() have a similar leak?

It uses the capped nt->mw_count to free windows:

ntb_transport_free() {
    ...
    for (i = nt->mw_count; i--; ) {
        ntb_free_mw(nt, i);
        iounmap(nt->mw_vec[i].vbase);
    }
}

This limits unmapping to nt->mw_count, which will leak any excess windows
that were mapped during probe if mw_count was originally greater than
nt->mw_count.

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

Re: [PATCH v2 09/14] NTB: ntb_transport: Drain RX tasklets during link cleanup

[sashiko-bot]

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

Pre-existing issues:
- [High] TOCTOU race in ntb_qp_link_cleanup() on qp->event_handler leading to NULL pointer dereference or execution of unloaded module
--

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

NTB: ntb_transport: Drain RX tasklets during link cleanup

This commit drains the RX tasklets during QP link cleanup to prevent them
from continuing to process the ring while the QP is reset. It uses
disable_delayed_work_sync() and tasklet_kill() before bringing down the QP
link, then re-enables the delayed work.

Cc: stable@vger.kernel.org

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index 45d4365becac5..36797ea3ff45c 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -956,11 +956,16 @@ static void ntb_qp_link_cleanup(struct ntb_transport_qp *qp)
>  
>  	dev_info(&pdev->dev, "qp %d: Link Cleanup\n", qp->qp_num);
>  
> -	cancel_delayed_work_sync(&qp->link_work);
> +	disable_delayed_work_sync(&qp->link_work);
> +	ntb_transport_set_qp_active(qp, false);
> +	tasklet_kill(&qp->rxc_db_work);
> +
>  	ntb_qp_link_down_reset(qp);
>  
>  	if (qp->event_handler)
>  		qp->event_handler(qp->cb_data, qp->link_is_up);

[Severity: High]
This is a pre-existing issue, but is this check of qp->event_handler
vulnerable to a TOCTOU race with client driver unbinding?

Looking at ntb_qp_link_cleanup() called directly from
ntb_transport_link_cleanup_work(), it evaluates if (qp->event_handler)
without any transport-level locks.

Concurrently, if a user unbinds the client driver, ntb_transport_free_queue()
runs and nullifies these fields:

drivers/ntb/ntb_transport.c:ntb_transport_free_queue() {
    ...
    qp->cb_data = NULL;
    qp->rx_handler = NULL;
    qp->tx_handler = NULL;
    qp->event_handler = NULL;
    ...
}

If the nullification in ntb_transport_free_queue() happens after the
if (qp->event_handler) check but before the call in ntb_qp_link_cleanup(),
could this execute a NULL pointer or unloaded module code?

> +
> +	enable_delayed_work(&qp->link_work);
>  }

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

Re: [PATCH v2 07/14] NTB: ntb_transport: Stop QP work before freeing a queue

[sashiko-bot]

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

New issues:
- [Low] Data race on `qp->client_ready` due to missing `WRITE_ONCE()` barrier.

Pre-existing issues:
- [High] Concurrent execution of global link cleanup and queue freeing causes a TOCTOU NULL pointer dereference or UAF of the client event handler.
- [High] Missing `dmaengine_terminate_sync()` causes potential Use-After-Free of `ntb_queue_entry` by pending DMA callbacks.
- [High] Lockless read-modify-write of `nt->qp_bitmap_free` allows concurrent queue allocations to race and corrupt state.
- [Medium] DMA mapping is leaked on error paths if the mapped address is exactly 0.
--

commit 5792da2527dba6f7348b49cf7d71537d78957df9
Author: Koichiro Den <den@valinux.co.jp>
Date:   Thu Sep 10 04:08:29 2026 +0000

    NTB: ntb_transport: Stop QP work before freeing a queue

This patch disables and waits for qp->link_cleanup and qp->link_work to
finish before freeing queue resources in ntb_transport_free_queue(). It also
delays enabling these works until queue creation succeeds to prevent tasks
from running concurrently with teardown or setup.

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index 8dd1770aaaf15..e5599c7ca93f2 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
[ ... ]
> @@ -2155,6 +2157,9 @@ ntb_transport_create_queue(void *data, struct device *client_dev,
>  		}
>  	}
>  
> +	enable_work(&qp->link_cleanup);
> +	enable_delayed_work(&qp->link_work);
> +
>  	ntb_db_clear(qp->ndev, qp_bit);
>  	ntb_db_clear_mask(qp->ndev, qp_bit);

[Severity: High]
This is a pre-existing issue, but earlier in ntb_transport_create_queue(),
does the lockless allocation of transport queues allow concurrency issues?

    free_queue = ffs(nt->qp_bitmap_free);
    if (!free_queue)
        goto err;
    free_queue--;
    qp = &nt->qp_vec[free_queue];
    qp_bit = BIT_ULL(qp->qp_num);
    nt->qp_bitmap_free &= ~qp_bit;

Could multiple concurrent clients probe and hijack the same transport queue
index, corrupting the bitmap?

[Severity: Medium]
This is a pre-existing issue, but does the error path in this function leak
the DMA mapping if the mapped address is 0?

In ntb_transport_create_queue(), the cleanup code checks:

    if (qp->tx_mw_dma_addr)
        dma_unmap_resource(qp->tx_dma_chan->device->dev, ...);

Since 0 can be a valid DMA address on some architectures, and
dma_map_resource() returns DMA_MAPPING_ERROR on failure rather than 0,
would treating 0 as invalid bypass unmapping and permanently leak it?

[ ... ]
> @@ -2200,6 +2205,10 @@ void ntb_transport_free_queue(struct ntb_transport_qp *qp)
>  
>  	pdev = qp->ndev->pdev;
>  
> +	qp->client_ready = false;

[Severity: Low]
Is there a data race on qp->client_ready?

Since ntb_transport_link_work() concurrently reads this flag using
READ_ONCE(), should this plain store be updated to use WRITE_ONCE() to
prevent compiler caching or tearing?

> +	disable_work_sync(&qp->link_cleanup);
> +	disable_delayed_work_sync(&qp->link_work);
> +	qp->link_is_up = false;
>  	qp->active = false;

[Severity: High]
This is a pre-existing issue, but does queue teardown safely wait for
in-flight DMA callbacks to finish?

Further down in ntb_transport_free_queue(), the code calls:

    dmaengine_terminate_all(chan);

This stops new DMA but doesn't synchronously wait. If a callback like
ntb_rx_copy_callback() is executing concurrently:

drivers/ntb/ntb_transport.c:ntb_rx_copy_callback() {
    ...
    entry->flags |= DESC_DONE_FLAG;
    ntb_complete_rxc(entry->qp);
}

Could it access the entry after the queue teardown loops free it via kfree(),
causing a use-after-free? Should this use dmaengine_terminate_sync() instead?

[ ... ]
> @@ -2247,8 +2256,6 @@ void ntb_transport_free_queue(struct ntb_transport_qp *qp)
>  	ntb_db_set_mask(qp->ndev, qp_bit);
>  	tasklet_kill(&qp->rxc_db_work);
>  
> -	cancel_delayed_work_sync(&qp->link_work);
> -
>  	qp->cb_data = NULL;
>  	qp->rx_handler = NULL;
>  	qp->tx_handler = NULL;

[Severity: High]
This is a pre-existing issue, but is there a race condition between clearing
these handler pointers and ntb_qp_link_cleanup()?

When ntb_transport_free_queue() clears qp->event_handler (immediately below
this snippet):

    qp->event_handler = NULL;

ntb_qp_link_cleanup() can execute concurrently on a workqueue from a hardware
link down event:

drivers/ntb/ntb_transport.c:ntb_qp_link_cleanup() {
    ...
    if (qp->event_handler)
        qp->event_handler(qp->cb_data, qp->link_is_up);
}

If the handler is cleared between the check and the call, could this result
in a NULL pointer dereference?

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

Re: [PATCH v2 03/14] NTB: ntb_transport: Avoid deadlock when cancelling link work

[sashiko-bot]

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

New issues:
- [High] Dropping `link_event_lock` around `cancel_delayed_work_sync()` introduces a race condition that can permanently tear down a successfully established link.

Pre-existing issues:
- [High] Separate work items for link-up and link-down events can execute out of order, leading to a permanent link failure.
--

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

NTB: ntb_transport: Avoid deadlock when cancelling link work

This commit changes ntb_transport_link_cleanup() to cancel the link_work
delayed work item outside of the link_event_lock. This avoids a deadlock
where a retry in link_work blocks on the lock held by the cleanup function.

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index 55a20ae9a85e6..c77b173dca01f 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -960,6 +960,15 @@ static void ntb_transport_link_cleanup(struct ntb_transport_ctx *nt)
>  	struct ntb_transport_qp *qp;
>  	u64 qp_bitmap_alloc;
>  	unsigned int i, count;
> +	bool cancel_link_work;
> +
> +	scoped_guard(mutex, &nt->link_event_lock)
> +		cancel_link_work = !nt->link_is_up;
> +
> +	if (cancel_link_work)
> +		cancel_delayed_work_sync(&nt->link_work);
> +
> +	guard(mutex)(&nt->link_event_lock);

[Severity: High]
Does dropping the link_event_lock around cancel_delayed_work_sync() create a
race condition that could permanently tear down the link?

If a hardware link flap occurs, ntb_transport_event_callback() can queue both
link_work and link_cleanup. If link_cleanup executes, it evaluates
!nt->link_is_up under the scoped_guard, drops the lock, and calls
cancel_delayed_work_sync().

During this unlocked window, a pending link_work can acquire the lock,
successfully establish the NTB link, set nt->link_is_up = true, and complete.

When link_cleanup reacquires the lock, it unconditionally destroys the QPs
and MWs that were just successfully established, without re-evaluating the
link state.

Can we re-check the link state after reacquiring the lock to prevent this
permanent link failure?

[ ... ]

As an additional observation regarding the event handling:

drivers/ntb/ntb_transport.c:ntb_transport_event_callback() {
...
	if (ntb_link_is_up(nt->ndev, NULL, NULL) == 1)
		schedule_delayed_work(&nt->link_work, 0);
	else
		schedule_work(&nt->link_cleanup);
...
}

[Severity: High]
This isn't a bug introduced by this patch, but does using separate work items
for link-up and link-down events allow them to execute out of order?

Since link_work and link_cleanup are queued to the global workqueue as
distinct work items, rapid hardware flapping (DOWN then UP) can queue both
simultaneously.

If they execute out of order, or if link_cleanup executes last and cancels an
un-run link_work, the software state could be torn down permanently even
though the physical hardware state is UP.

Could a single work item or a state machine handle both events to enforce
ordering?

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

Re: [PATCH v2 10/14] NTB: ntb_transport: Wait for RX completions before resetting a QP

[sashiko-bot]

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

Pre-existing issues:
- [High] Data corruption during fallback RX memory copy.
- [High] Use-After-Free of `ntb_queue_entry` elements during transport queue teardown.
--

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

NTB: ntb_transport: Wait for RX completions before resetting a QP

This commit fixes a race condition where transport link cleanup can free a
memory window still in use by RX DMA. It adds a wait loop for the RX post
queue to become idle before resetting the QP.

Link: https://lore.kernel.org/r/20260907144257.767281F00A3A@smtp.kernel.org/

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index 36797ea3ff45c..7ccba2c04f548 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c

[ ... ]

> @@ -959,6 +966,17 @@ static void ntb_qp_link_cleanup(struct ntb_transport_qp *qp)
>  	disable_delayed_work_sync(&qp->link_work);
>  	ntb_transport_set_qp_active(qp, false);
>  	tasklet_kill(&qp->rxc_db_work);
> +	/*
> +	 * Some DMA engines lack terminate/synchronize ops (e.g. IOAT), and
> +	 * DMA_COMPLETION_NO_ORDER rules out cookie-based waits.
> +	 *
> +	 * Waiting for rx_post_q to empty suffices: ntb_complete_rxc() finishes
> +	 * its MW accesses before removing each entry under ntb_rx_q_lock.
> +	 * qp->active is false and rxc_db_work is stopped, so no new RX DMA
> +	 * can be submitted.
> +	 */
> +	while (!ntb_transport_rx_idle(qp))
> +		fsleep(1000);
>  
>  	ntb_qp_link_down_reset(qp);

[Severity: High]
This is a pre-existing issue, but does ntb_transport_free_queue() also need
this same wait loop to prevent a use-after-free?

When tearing down a queue, ntb_transport_free_queue() calls
dmaengine_terminate_all() and dma_release_channel(). However, as noted in the
patch comment here, some DMA engines lack terminate/synchronize ops. The
cleanup routine then immediately frees the rx_post_q entries:

drivers/ntb/ntb_transport.c:ntb_transport_free_queue() {
    ...
	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);
	}
    ...
}

If a DMA callback is pending or executing asynchronously while the queue is
being torn down, won't it dereference the freed entry pointer after
kfree(entry) is called?

[Severity: High]
This is another pre-existing issue, but can the fallback copy corrupt data
during a DMA error?

If a DMA transfer fails and ntb_rx_copy_callback() falls back to a software
copy, it calculates the offset using the globally incremented qp->rx_index:

drivers/ntb/ntb_transport.c:ntb_rx_copy_callback() {
    ...
		case DMA_TRANS_ABORTED:
		{
			struct ntb_transport_qp *qp = entry->qp;
			void *offset = qp->rx_buff + qp->rx_max_frame *
					qp->rx_index;

			ntb_memcpy_rx(entry, offset);
			qp->rx_memcpy++;
			return;
		}
    ...
}

Because qp->rx_index was already advanced to the next available ring buffer
frame when the transaction was originally submitted in ntb_process_rxc(),
won't this offset calculation point to the wrong frame and copy the wrong
payload? Should this calculation use the entry-specific index
(entry->rx_index) instead?

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

Re: [PATCH v2 05/14] NTB: ntb_transport: Avoid losing QP link-up requests

[sashiko-bot]

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

Pre-existing issues:
- [High] Race condition between `ntb_qp_link_work` and `ntb_transport_link_down` causes spurious link-up and breaks queue teardown synchronization.
- [High] Missing `dma_rmb()` after checking `DESC_DONE_FLAG` allows speculative reading of stale DMA data.
--

commit 99af086b145ca7c747420cbf914a391c3b3216f2
Author: Koichiro Den <den@valinux.co.jp>

NTB: ntb_transport: Avoid losing QP link-up requests

ntb_netdev_open() can call ntb_transport_link_up() while the transport
worker is completing setup on another CPU. Concurrent transport setup
and a client link-up request can both read the other's flag as false and
leave QP link work unqueued. The QP then stays down until another link
event or client link-up request.

Link: https://lore.kernel.org/r/20260907144701.702E41F00A3A@smtp.kernel.org/

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index 75d4a2e021f4e..1332d53bcfe7d 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -1104,10 +1104,16 @@ static void ntb_transport_link_work(struct work_struct *work)
>  	/* Publish the link only after every QP has been set up. */
>  	smp_store_release(&nt->link_is_up, true);
>  
> +	/*
> +	 * Prevent both sides from missing each other's flag. Pairs with
> +	 * the barrier in ntb_transport_link_up().
> +	 */
> +	smp_mb();
> +
>  	for (i = 0; i < nt->qp_count; i++) {
>  		struct ntb_transport_qp *qp = &nt->qp_vec[i];
>  
> -		if (qp->client_ready)
> +		if (READ_ONCE(qp->client_ready))
>  			ntb_transport_schedule_qp_link(qp, 0);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but does the check for
qp->client_ready in ntb_qp_link_work() safely prevent a spurious link-up if
the client simultaneously tears down the link?

If CPU 1 calls ntb_transport_link_down(), clearing qp->client_ready and
calling cancel_delayed_work_sync(&qp->link_work):

drivers/ntb/ntb_transport.c:ntb_transport_link_down() {
	...
	if (qp->link_is_up)
		ntb_send_link_down(qp);
	else
		cancel_delayed_work_sync(&qp->link_work);
}

Could CPU 0, executing ntb_qp_link_work() concurrently, fail to see
qp->client_ready as false before writing to the remote SPAD and firing the
link-up event handler?

drivers/ntb/ntb_transport.c:ntb_qp_link_work() {
	...
	if (val & BIT(qp->qp_num)) {
		dev_info(&pdev->dev, "qp %d: Link Up\n", qp->qp_num);
		qp->link_is_up = true;
	...
}

This sequence bypasses the client's teardown intent, and forcing the link
back up after teardown may lead to use-after-free or data corruption.

[ ... ]

> @@ -2401,7 +2407,10 @@ void ntb_transport_link_up(struct ntb_transport_qp *qp)
>  	if (!qp)
>  		return;
>  
> -	qp->client_ready = true;
> +	WRITE_ONCE(qp->client_ready, true);
> +
> +	/* Pairs with the barrier in ntb_transport_link_work(). */
> +	smp_mb();
>  
>  	ntb_transport_schedule_qp_link(qp, 0);
>  }

[Severity: High]
This is a pre-existing issue, but does ntb_process_rxc() need a dma_rmb()
barrier after checking DESC_DONE_FLAG?

When processing received packets, the done flag is checked:

drivers/ntb/ntb_transport.c:ntb_process_rxc() {
	...
	if (!(hdr->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) {
	...
}

Without a dma_rmb() here, can weak memory architectures (like ARM64 or
PowerPC) speculatively fetch subsequent header fields and payload data
(such as hdr->ver or hdr->len) before the flag check completes, leading to
processing stale DMA data?

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

Re: [PATCH v2 06/14] NTB: ntb_transport: Clear link state before QP cleanup

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing read memory barrier in ntb_process_rxc() allows out-of-order reads of packet headers and payload.
- [High] ntb_qp_link_work() and ntb_transport_link_down() incorrectly echo the peer's QP link state back to the peer instead of transmitting the local state.
--

commit c193be6a6eafe61af2db594c3da12d870adbadab
Author: Koichiro Den <den@valinux.co.jp>
NTB: ntb_transport: Clear link state before QP cleanup

This commit clears the transport link state before QP cleanup to prevent
subsequent client link-up requests from starting QP link work prematurely.
It also ensures QP link work returns if the transport went down after it
was queued.

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index 1332d53bcfe7d..8dd1770aaaf15 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
[ ... ]
> @@ -1142,7 +1144,9 @@ static void ntb_qp_link_work(struct work_struct *work)
>  	struct ntb_transport_ctx *nt = qp->transport;
>  	int val;
>  
> -	WARN_ON(!nt->link_is_up);
> +	/* Pair with the link publication in ntb_transport_link_work(). */
> +	if (!smp_load_acquire(&nt->link_is_up))
> +		return;
>  
>  	val = ntb_spad_read(nt->ndev, QP_LINKS);
>  

[Severity: High]
This isn't a bug introduced by this patch, but do ntb_qp_link_work() and
ntb_transport_link_down() incorrectly echo the peer's QP link state back to
the peer instead of transmitting the local state?

When the local side brings up a QP, ntb_qp_link_work() reads the local
scratchpad via ntb_spad_read(). Because the peer writes its state to our
local scratchpad, this reads the peer's state. It then ORs its own QP bit
into this value and writes it back to the peer via ntb_peer_spad_write():

	val = ntb_spad_read(nt->ndev, QP_LINKS);

	ntb_peer_spad_write(nt->ndev, PIDX, QP_LINKS, val | BIT(qp->qp_num));

This same flawed echo logic applies in ntb_transport_link_down(), echoing
the peer's state back to the peer:

	val = ntb_spad_read(qp->ndev, QP_LINKS);

	ntb_peer_spad_write(qp->ndev, PIDX, QP_LINKS, val & ~BIT(qp->qp_num));

Is this echo loop intentional, or does it corrupt the independent QP link
state management by causing the peer to incorrectly believe that the local
side has brought up QPs that the peer itself brought up?

[Severity: High]
This is also a pre-existing issue, but is there a missing read memory
barrier in ntb_process_rxc() that allows out-of-order reads of packet
headers and payload?

In drivers/ntb/ntb_transport.c:ntb_process_rxc():

	if (!(hdr->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) {
		dev_dbg(&qp->ndev->pdev->dev, "link down flag set\n");

Is a dma_rmb() required after validating the DESC_DONE_FLAG to ensure
subsequent memory reads (like hdr->ver, hdr->len, and payload) are not
reordered before the flag check completes?

If the CPU speculatively reads hdr->ver, hdr->len, or the payload data before
the DESC_DONE_FLAG check is retired, and the DMA transfer hasn't fully arrived
at the CPU cache yet, could stale data be processed, leading to data
corruption?

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

Re: [PATCH v2 11/14] NTB: ntb_transport: Prepare remote RX info accesses for MW teardown

[sashiko-bot]

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

New issues:
- [Medium] Returning 0 from `ntb_transport_tx_free_entry()` when `remote_rx_info` is NULL can trigger a spurious `WARN_ON_ONCE` in `ntb_async_tx()`.

Pre-existing issues:
- [High] Lockless Time-of-Check to Time-of-Use (TOCTOU) race leading to a Use-After-Free on `remote_rx_info` during Memory Window (MW) teardown.
--

commit 82666f622eff311c2e1a37c138e32f49823001fc
Author: Koichiro Den <den@valinux.co.jp>

NTB: ntb_transport: Prepare remote RX info accesses for MW teardown

The commit uses READ_ONCE() and WRITE_ONCE() for remote_rx_info to handle
lockless accesses during link cleanup and MW freeing. It also adds a NULL
check before dereferencing the pointer.

Cc: stable@vger.kernel.org

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index 7ccba2c04f548..b949f36a4f2d9 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -489,6 +489,7 @@ EXPORT_SYMBOL_GPL(ntb_transport_unregister_client);
>  static int ntb_qp_debugfs_stats_show(struct seq_file *s, void *v)
>  {
>  	struct ntb_transport_qp *qp = s->private;
> +	struct ntb_rx_info *remote_rx_info;
>  
>  	if (!qp || !qp->link_is_up)
>  		return 0;
> @@ -516,7 +517,9 @@ 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);
> +	remote_rx_info = READ_ONCE(qp->remote_rx_info);
> +	if (remote_rx_info)
> +		seq_printf(s, "RRI (T) - \t%u\n", remote_rx_info->entry);

[Severity: High]
This is a pre-existing issue, but does checking the pointer for NULL
actually prevent a use-after-free here?

If a reader thread reads a non-NULL pointer and gets preempted, a concurrent
link-down event could trigger ntb_free_mw(), synchronously unmapping and
freeing the DMA memory.

When the reader resumes, wouldn't dereferencing its local remote_rx_info
pointer result in a use-after-free on kernel DMA memory since the underlying
Memory Window (MW) is no longer valid?

[ ... ]

> @@ -2558,8 +2564,14 @@ EXPORT_SYMBOL_GPL(ntb_transport_max_size);
>  
>  unsigned int ntb_transport_tx_free_entry(struct ntb_transport_qp *qp)
>  {
> +	struct ntb_rx_info *remote_rx_info = READ_ONCE(qp->remote_rx_info);
>  	unsigned int head = qp->tx_index;
> -	unsigned int tail = qp->remote_rx_info->entry;
> +	unsigned int tail;
> +
> +	if (!remote_rx_info)
> +		return 0;

[Severity: Medium]
Could this new return path trigger a spurious WARN_ON_ONCE in ntb_async_tx()?

In ntb_process_tx(), the code verifies ntb_transport_tx_free_entry() space
before executing ntb_async_tx(). If a concurrent MW teardown sets
qp->remote_rx_info to NULL right at this moment, the secondary check inside
ntb_async_tx() would hit this new return 0 path.

Since it evaluates to zero unexpectedly, would this trip the WARN_ON_ONCE
even though there was no actual queue exhaustion or invalid usage?

> +
> +	tail = remote_rx_info->entry;

[Severity: High]
This is a pre-existing issue, but is it possible for a time-of-check to
time-of-use (TOCTOU) race to occur here during Memory Window (MW) teardown?

Just like in ntb_qp_debugfs_stats_show(), if a concurrent link-down event
frees the MW after the NULL check but before this dereference, wouldn't
this lead to a use-after-free?

>  
>  	return tail >= head ? tail - head : qp->tx_max_entry + tail - head;
>  }

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

Re: [PATCH v2 12/14] NTB: ntb_transport: Clear QP pointers when freeing an MW

[sashiko-bot]

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

Pre-existing issues:
- [High] The patch addresses a serial Use-After-Free but leaves a concurrent TOCTOU (Time-Of-Check to Time-Of-Use) Use-After-Free unmitigated, as `WRITE_ONCE()` does not protect concurrent readers from accessing freed memory.
--

commit 83701cf77653ce575ecc537dc3582d73e7b2ebcf
Author: Koichiro Den <den@valinux.co.jp>

NTB: ntb_transport: Clear QP pointers when freeing an MW

This commit clears the rx_buff and remote_rx_info pointers for all QPs using
an MW when the MW is freed. This addresses a serial use-after-free where a
link-down event could cause ntb_qp_link_down_reset() to write to previously
freed MW memory.

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index b949f36a4f2d9..096be87e5ede9 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -781,10 +781,17 @@ static void ntb_free_mw(struct ntb_transport_ctx *nt, int num_mw)
>  {
>  	struct ntb_transport_mw *mw = &nt->mw_vec[num_mw];
>  	struct device *dma_dev = ntb_get_dma_dev(nt->ndev);
> +	unsigned int i;
>  
>  	if (!mw->virt_addr)
>  		return;
>  
> +	/* Drop references from every QP using this MW. */
> +	for (i = num_mw; i < nt->qp_count; i += nt->mw_count) {
> +		nt->qp_vec[i].rx_buff = NULL;
> +		WRITE_ONCE(nt->qp_vec[i].remote_rx_info, NULL);
> +	}
> +

[Severity: High]
This is a pre-existing issue, but does this clearing mechanism leave a
concurrent Time-Of-Check to Time-Of-Use (TOCTOU) regression unmitigated?

While this patch addresses a serial use-after-free, WRITE_ONCE() does not
protect concurrent readers from accessing freed memory. If a client drops the
link concurrently with a hardware link-down event triggering
ntb_transport_link_cleanup(), the client thread can fetch the remote_rx_info
pointer locklessly:

drivers/ntb/ntb_transport.c:ntb_qp_link_down_reset() {
	...
	remote_rx_info = READ_ONCE(qp->remote_rx_info);
	if (remote_rx_info)
		remote_rx_info->entry = qp->rx_max_entry - 1;
	...
}

If the client thread is preempted after reading the pointer, and the cleanup
thread proceeds to free the memory in ntb_free_mw():

drivers/ntb/ntb_transport.c:ntb_free_mw() {
	...
	ntb_mw_clear_trans(nt->ndev, PIDX, num_mw);
	dma_free_attrs(dma_dev, mw->alloc_size, mw->alloc_addr,
		       mw->original_dma_addr, DMA_ATTR_FORCE_CONTIGUOUS);
	...
}

When the client thread resumes, wouldn't it write to the freed DMA memory?

This same lockless TOCTOU vulnerability appears to be reachable on the
transmit hot-path in ntb_transport_tx_free_entry() and via debugfs in
ntb_qp_debugfs_stats_show().

>  	ntb_mw_clear_trans(nt->ndev, PIDX, num_mw);
>  	dma_free_attrs(dma_dev, mw->alloc_size, mw->alloc_addr,
>  		       mw->original_dma_addr, DMA_ATTR_FORCE_CONTIGUOUS);

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

Re: [PATCH v2 14/14] NTB: ntb_transport: Remove clients before freeing transport resources

[sashiko-bot]

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

New issues:
- [Critical] Skipping ntb_qp_link_cleanup() during transport teardown fails to drain rx_post_q, causing freed memory to be accessed by in-flight IOAT DMA.

Pre-existing issues:
- [Critical] Double-fetch of `hdr->len` from shared memory allows buffer overflow via TOCTOU race.
- [High] Racy read-modify-write of `qp_bitmap_free` bitmask allows queue state corruption.
--

commit 68ca6cad9a88600dd4607f3c4ccd6bd32385b0e7
Author: Koichiro Den <den@valinux.co.jp>
Subject: NTB: ntb_transport: Remove clients before freeing transport resources

Unbinding ntb_transport can call ntb_transport_free() while ntb_netdev
is still bound. This patch reorders the teardown process to unregister
clients before freeing resources, intending to fix unsafe memory accesses
during intensive TX loads.

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index c1e3a077c45ec..f24555b38d1b5 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -1487,9 +1487,11 @@ static void ntb_transport_free(struct ntb_client *self, struct ntb_dev *ndev)
>  
>  	debugfs_remove_recursive(nt->debugfs_node_dir);
>  
> -	ntb_transport_link_cleanup(nt);
> -	cancel_work_sync(&nt->link_cleanup);
> -	cancel_delayed_work_sync(&nt->link_work);
> +	/* Stop transport work before clients release their QPs. */
> +	disable_delayed_work_sync(&nt->link_work);
> +	disable_work_sync(&nt->link_cleanup);
> +
> +	ntb_bus_remove(nt);

[Severity: Critical]
By moving ntb_bus_remove() before ntb_transport_link_cleanup(), does this
introduce a DMA use-after-free regression?

Calling ntb_bus_remove() first prompts client drivers to free their QPs via
ntb_transport_free_queue(), which blindly frees all entries in rx_post_q
without polling:

drivers/ntb/ntb_transport.c:ntb_transport_free_queue() {
    ...
	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);
	}
    ...
}

Because IOAT DMA engines could still be actively copying into those buffers,
freeing them allows the hardware DMA engine to write to freed kernel memory.
Furthermore, the DMA drain wait in ntb_qp_link_cleanup() is subsequently
skipped entirely because qp_bitmap_alloc evaluates to 0 during the later
call to ntb_transport_link_cleanup().

[ ... ]

> @@ -1736,17 +1736,17 @@ static void ntb_process_rxc(struct ntb_transport_qp *qp)
>  
>  	if (hdr->len > entry->len) {
>  		dev_dbg(&qp->ndev->pdev->dev,
>  			"receive buffer overflow! Wanted %d got %d\n",
>  			hdr->len, entry->len);
>  		qp->rx_err_oflow++;
>  
>  		entry->len = -EIO;
>  		entry->flags |= DESC_DONE_FLAG;
>  
>  		ntb_complete_rxc(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_bytes += hdr->len;
>  		qp->rx_pkts++;
>  
>  		entry->len = hdr->len;

[Severity: Critical]
This is a pre-existing issue, but is there a time-of-check to time-of-use
race condition in this bounds check on hdr->len?

Since hdr points to a payload header located inside a memory window writable
by the remote NTB peer (shared memory), hdr->len is read directly from
shared memory twice (once for the bounds check, once for the assignment).

Can a remote peer swap hdr->len to a much larger value immediately after
the bounds check but before the assignment, bypassing the bounds check and
causing a buffer overflow when copied?

[ ... ]

> @@ -2130,7 +2130,7 @@ struct ntb_transport_qp *
>  ntb_transport_create_queue(void *data, struct device *client_dev,
>  			   const struct ntb_queue_handlers *handlers)
>  {
[ ... ]
>  	free_queue--;
>  
>  	qp = &nt->qp_vec[free_queue];
>  	qp_bit = BIT_ULL(qp->qp_num);
>  
>  	nt->qp_bitmap_free &= ~qp_bit;

[Severity: High]
This isn't a bug introduced by this patch, but could this read-modify-write
of nt->qp_bitmap_free lead to queue state corruption?

The qp_bitmap_free bitmask is modified here using non-atomic bitwise
operations without holding any lock. It is also modified concurrently during
driver unbind:

drivers/ntb/ntb_transport.c:ntb_transport_free_queue() {
    ...
	qp->transport->qp_bitmap_free |= qp_bit;
    ...
}

Since these are exported functions called by NTB client drivers, could
concurrent probe and remove operations race and lose bit updates, leading
to a leaked queue or two clients being assigned the same queue?

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

Re: [PATCH v2 13/14] NTB: ntb_transport: Abort link setup on QP MW allocation failure

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Lockless access to `remote_rx_info` can lead to Use-After-Free during link down.
- [Critical] Pending TX transactions and DMA operations are not flushed on link down, causing data corruption across link sessions or PCIe AER faults.
- [High] Aborting link setup on error path `out1` leaves the remote peer with an initialized `VERSION` scratchpad, desynchronizing the link state and potentially causing PCIe AER faults.
--

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

NTB: ntb_transport: Abort link setup on QP MW allocation failure

This patch updates ntb_transport_link_work() to abort link setup if QP memory
window allocation fails. It ensures that the worker does not publish a QP with
only a partially initialized memory-window state.

Fixes: a754a8fcaf38 ("NTB: allocate number transport entries depending on size of ring size")
Cc: stable@vger.kernel.org

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index 096be87e5ede9..c1e3a077c45ec 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -1152,7 +1152,9 @@ static void ntb_transport_link_work(struct work_struct *work)
>  	}
>  
>  	for (i = 0; i < nt->qp_count; i++) {
> -		ntb_transport_setup_qp_mw(nt, i);
> +		rc = ntb_transport_setup_qp_mw(nt, i);
> +		if (rc)
> +			goto out1;

[Severity: High]
This isn't a bug introduced by this patch, but does jumping to out1 here
leave the remote peer with an initialized VERSION scratchpad?

Earlier in ntb_transport_link_work(), readiness is signaled to the remote peer:

    ntb_peer_spad_write(ndev, PIDX, VERSION, NTB_TRANSPORT_VERSION);
    /* Query the remote side for its info */

If ntb_transport_setup_qp_mw() fails, the error path cleans up local
translations but returns immediately without rescinding the version scratchpad:

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

    /* if there's an actual failure, we should just bail */
    if (rc < 0)
        return;

Could this leave the peer stranded in an invalid 'up' state, transmitting
memory-mapped I/O that hits unconfigured BARs and generates PCIe AER faults?

>  		ntb_transport_setup_qp_peer_msi(nt, i);
>  	}
>  

[Severity: Critical]
This is a pre-existing issue, but can lockless access to remote_rx_info
lead to a use-after-free regression during link down?

The ntb_transport_tx_free_entry() function reads the pointer locklessly:

drivers/ntb/ntb_transport.c:ntb_transport_tx_free_entry() {
    struct ntb_rx_info *remote_rx_info = READ_ONCE(qp->remote_rx_info);
    unsigned int head = qp->tx_index;
    unsigned int tail;

    if (!remote_rx_info)
        return 0;

    tail = remote_rx_info->entry;
    ...
}

When the link goes down, ntb_transport_link_cleanup() invokes ntb_free_mw()
to free the DMA memory window:

drivers/ntb/ntb_transport.c:ntb_free_mw() {
    ...
    for (i = num_mw; i < nt->qp_count; i += nt->mw_count) {
        nt->qp_vec[i].rx_buff = NULL;
        WRITE_ONCE(nt->qp_vec[i].remote_rx_info, NULL);
    }

    ntb_mw_clear_trans(nt->ndev, PIDX, num_mw);
    dma_free_attrs(dma_dev, mw->alloc_size, mw->alloc_addr,
                   mw->original_dma_addr, DMA_ATTR_FORCE_CONTIGUOUS);
}

Since the memory is freed immediately without waiting for concurrent readers
or an RCU grace period, could a reader preempted after reading the non-NULL
pointer dereference it after it is freed?

[Severity: Critical]
This is a pre-existing issue, but are pending TX transactions and DMA
operations properly flushed on link down?

When the link goes down, ntb_qp_link_cleanup() waits for RX to idle and resets
the QP state:

drivers/ntb/ntb_transport.c:ntb_qp_link_cleanup() {
    ...
    while (!ntb_transport_rx_idle(qp))
        fsleep(1000);

    ntb_qp_link_down_reset(qp);
    ...
}

However, it does not appear to synchronize, wait for, or cancel the TX offload
thread. The background thread can continue processing old packets:

drivers/ntb/ntb_transport.c:ntb_tx_memcpy_kthread() {
    ...
    list_for_each_entry_safe(entry, tmp, &local_list, entry) {
        list_del(&entry->entry);
        offset = qp->tx_mw + qp->tx_max_frame * entry->tx_index;
        ntb_memcpy_tx_on_stack(entry, offset);
    ...
}

Could delayed transactions from the old session execute and copy their
payloads to the remote memory window at their old tx_index, potentially
causing data corruption on the remote peer or PCIe AER faults if the
remote memory window is unmapped?

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

Re: [PATCH v2 14/14] NTB: ntb_transport: Remove clients before freeing transport resources

[Koichiro Den]

On Thu, Sep 10, 2026 at 04:36:13AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> New issues:
> - [Critical] Skipping ntb_qp_link_cleanup() during transport teardown fails to drain rx_post_q, causing freed memory to be accessed by in-flight IOAT DMA.

I believe this is a false positive. See my inline comment below.

> 
> Pre-existing issues:
> - [Critical] Double-fetch of `hdr->len` from shared memory allows buffer overflow via TOCTOU race.
> - [High] Racy read-modify-write of `qp_bitmap_free` bitmask allows queue state corruption.
> --
> 
> commit 68ca6cad9a88600dd4607f3c4ccd6bd32385b0e7
> Author: Koichiro Den <den@valinux.co.jp>
> Subject: NTB: ntb_transport: Remove clients before freeing transport resources
> 
> Unbinding ntb_transport can call ntb_transport_free() while ntb_netdev
> is still bound. This patch reorders the teardown process to unregister
> clients before freeing resources, intending to fix unsafe memory accesses
> during intensive TX loads.
> 
> > diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> > index c1e3a077c45ec..f24555b38d1b5 100644
> > --- a/drivers/ntb/ntb_transport.c
> > +++ b/drivers/ntb/ntb_transport.c
> > @@ -1487,9 +1487,11 @@ static void ntb_transport_free(struct ntb_client *self, struct ntb_dev *ndev)
> >  
> >  	debugfs_remove_recursive(nt->debugfs_node_dir);
> >  
> > -	ntb_transport_link_cleanup(nt);
> > -	cancel_work_sync(&nt->link_cleanup);
> > -	cancel_delayed_work_sync(&nt->link_work);
> > +	/* Stop transport work before clients release their QPs. */
> > +	disable_delayed_work_sync(&nt->link_work);
> > +	disable_work_sync(&nt->link_cleanup);
> > +
> > +	ntb_bus_remove(nt);
> 
> [Severity: Critical]
> By moving ntb_bus_remove() before ntb_transport_link_cleanup(), does this
> introduce a DMA use-after-free regression?
> 
> Calling ntb_bus_remove() first prompts client drivers to free their QPs via
> ntb_transport_free_queue(), which blindly frees all entries in rx_post_q
> without polling:
> 
> drivers/ntb/ntb_transport.c:ntb_transport_free_queue() {
>     ...
> 	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);
> 	}
>     ...
> }

Yes, but ntb_transport_free_queue() calls dma_release_channel() before freeing
those entries. For IOAT, this goes through ioat_free_chan_resources(), which
calls ioat_stop() and ioat_reset_hw() to synchronize callbacks and stop the DMA
channel.

I believe Jon and Dave know IOAT well too, so I'd appreciate any comments from
them. I guess the mention on IOAT in an earlier patch in this series prompted
Sashiko to focus on it here.

Best regards,
Koichiro

> 
> Because IOAT DMA engines could still be actively copying into those buffers,
> freeing them allows the hardware DMA engine to write to freed kernel memory.
> Furthermore, the DMA drain wait in ntb_qp_link_cleanup() is subsequently
> skipped entirely because qp_bitmap_alloc evaluates to 0 during the later
> call to ntb_transport_link_cleanup().
> 
> [ ... ]
> 
> > @@ -1736,17 +1736,17 @@ static void ntb_process_rxc(struct ntb_transport_qp *qp)
> >  
> >  	if (hdr->len > entry->len) {
> >  		dev_dbg(&qp->ndev->pdev->dev,
> >  			"receive buffer overflow! Wanted %d got %d\n",
> >  			hdr->len, entry->len);
> >  		qp->rx_err_oflow++;
> >  
> >  		entry->len = -EIO;
> >  		entry->flags |= DESC_DONE_FLAG;
> >  
> >  		ntb_complete_rxc(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_bytes += hdr->len;
> >  		qp->rx_pkts++;
> >  
> >  		entry->len = hdr->len;
> 
> [Severity: Critical]
> This is a pre-existing issue, but is there a time-of-check to time-of-use
> race condition in this bounds check on hdr->len?
> 
> Since hdr points to a payload header located inside a memory window writable
> by the remote NTB peer (shared memory), hdr->len is read directly from
> shared memory twice (once for the bounds check, once for the assignment).
> 
> Can a remote peer swap hdr->len to a much larger value immediately after
> the bounds check but before the assignment, bypassing the bounds check and
> causing a buffer overflow when copied?
> 
> [ ... ]
> 
> > @@ -2130,7 +2130,7 @@ struct ntb_transport_qp *
> >  ntb_transport_create_queue(void *data, struct device *client_dev,
> >  			   const struct ntb_queue_handlers *handlers)
> >  {
> [ ... ]
> >  	free_queue--;
> >  
> >  	qp = &nt->qp_vec[free_queue];
> >  	qp_bit = BIT_ULL(qp->qp_num);
> >  
> >  	nt->qp_bitmap_free &= ~qp_bit;
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but could this read-modify-write
> of nt->qp_bitmap_free lead to queue state corruption?
> 
> The qp_bitmap_free bitmask is modified here using non-atomic bitwise
> operations without holding any lock. It is also modified concurrently during
> driver unbind:
> 
> drivers/ntb/ntb_transport.c:ntb_transport_free_queue() {
>     ...
> 	qp->transport->qp_bitmap_free |= qp_bit;
>     ...
> }
> 
> Since these are exported functions called by NTB client drivers, could
> concurrent probe and remove operations race and lose bit updates, leading
> to a leaked queue or two clients being assigned the same queue?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260910040836.3792333-1-den@valinux.co.jp?part=14

Re: [PATCH v2 01/14] NTB: ntb_transport: Remove the device debugfs directory

[Frank Li]

On Thu, Sep 10, 2026 at 01:08:23PM +0900, Koichiro Den wrote:
> ntb_transport_free() removes QP debugfs directories but leaves the
> device directory. On rebind, debugfs_create_dir() fails with -EEXIST
> and QP statistics files are not recreated. Module unload masks this
> by removing the entire debugfs tree.
>
> To reproduce:
>
>   # ls /sys/kernel/debug/ntb_transport/0001:10:00.0/
>   qp0
>   # echo 0001:10:00.0 > /sys/bus/ntb/drivers/ntb_transport/unbind
>   # ls /sys/kernel/debug/ntb_transport/
>   0001:10:00.0   <-- should not remain
>   # echo 0001:10:00.0 > /sys/bus/ntb/drivers/ntb_transport/bind
>
>   .. and then dmesg shows:
>   debugfs: '0001:10:00.0' already exists in 'ntb_transport'
>
>   # ls /sys/kernel/debug/ntb_transport/0001:10:00.0/
>   (nothing)      <-- should be 'qp0'
>
> Remove the device debugfs tree on teardown and probe failure.
>
> Verified that unbind removes the directory and rebind recreates qp0.
>
> Fixes: c8650fd03d32 ("NTB: Fix transport stats for multiple devices")
> Cc: stable@vger.kernel.org
> Signed-off-by: Koichiro Den <den@valinux.co.jp>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> Changes in v2:
>   - No changes.
>
>  drivers/ntb/ntb_transport.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index f9caa1a653c5..3389d6ca9ebd 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -1382,6 +1382,7 @@ static int ntb_transport_probe(struct ntb_client *self, struct ntb_dev *ndev)
>  err3:
>  	ntb_clear_ctx(ndev);
>  err2:
> +	debugfs_remove_recursive(nt->debugfs_node_dir);
>  	kfree(nt->qp_vec);
>  err1:
>  	while (i--) {
> @@ -1401,6 +1402,8 @@ static void ntb_transport_free(struct ntb_client *self, struct ntb_dev *ndev)
>  	u64 qp_bitmap_alloc;
>  	int i;
>
> +	debugfs_remove_recursive(nt->debugfs_node_dir);
> +
>  	ntb_transport_link_cleanup(nt);
>  	cancel_work_sync(&nt->link_cleanup);
>  	cancel_delayed_work_sync(&nt->link_work);
> @@ -1412,7 +1415,6 @@ static void ntb_transport_free(struct ntb_client *self, struct ntb_dev *ndev)
>  		qp = &nt->qp_vec[i];
>  		if (qp_bitmap_alloc & BIT_ULL(i))
>  			ntb_transport_free_queue(qp);
> -		debugfs_remove_recursive(qp->debugfs_dir);
>  	}
>
>  	ntb_link_disable(ndev);
> --
> 2.51.0
>

Re: [PATCH v2 14/14] NTB: ntb_transport: Remove clients before freeing transport resources

[Dave Jiang]

On 9/10/26 1:48 AM, Koichiro Den wrote:
> On Thu, Sep 10, 2026 at 04:36:13AM +0000, sashiko-bot@kernel.org wrote:
>> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>>
>> New issues:
>> - [Critical] Skipping ntb_qp_link_cleanup() during transport teardown fails to drain rx_post_q, causing freed memory to be accessed by in-flight IOAT DMA.
> 
> I believe this is a false positive. See my inline comment below.
> 
>>
>> Pre-existing issues:
>> - [Critical] Double-fetch of `hdr->len` from shared memory allows buffer overflow via TOCTOU race.
>> - [High] Racy read-modify-write of `qp_bitmap_free` bitmask allows queue state corruption.
>> --
>>
>> commit 68ca6cad9a88600dd4607f3c4ccd6bd32385b0e7
>> Author: Koichiro Den <den@valinux.co.jp>
>> Subject: NTB: ntb_transport: Remove clients before freeing transport resources
>>
>> Unbinding ntb_transport can call ntb_transport_free() while ntb_netdev
>> is still bound. This patch reorders the teardown process to unregister
>> clients before freeing resources, intending to fix unsafe memory accesses
>> during intensive TX loads.
>>
>>> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
>>> index c1e3a077c45ec..f24555b38d1b5 100644
>>> --- a/drivers/ntb/ntb_transport.c
>>> +++ b/drivers/ntb/ntb_transport.c
>>> @@ -1487,9 +1487,11 @@ static void ntb_transport_free(struct ntb_client *self, struct ntb_dev *ndev)
>>>  
>>>  	debugfs_remove_recursive(nt->debugfs_node_dir);
>>>  
>>> -	ntb_transport_link_cleanup(nt);
>>> -	cancel_work_sync(&nt->link_cleanup);
>>> -	cancel_delayed_work_sync(&nt->link_work);
>>> +	/* Stop transport work before clients release their QPs. */
>>> +	disable_delayed_work_sync(&nt->link_work);
>>> +	disable_work_sync(&nt->link_cleanup);
>>> +
>>> +	ntb_bus_remove(nt);
>>
>> [Severity: Critical]
>> By moving ntb_bus_remove() before ntb_transport_link_cleanup(), does this
>> introduce a DMA use-after-free regression?
>>
>> Calling ntb_bus_remove() first prompts client drivers to free their QPs via
>> ntb_transport_free_queue(), which blindly frees all entries in rx_post_q
>> without polling:
>>
>> drivers/ntb/ntb_transport.c:ntb_transport_free_queue() {
>>     ...
>> 	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);
>> 	}
>>     ...
>> }
> 
> Yes, but ntb_transport_free_queue() calls dma_release_channel() before freeing
> those entries. For IOAT, this goes through ioat_free_chan_resources(), which
> calls ioat_stop() and ioat_reset_hw() to synchronize callbacks and stop the DMA
> channel.

The refute looks reasonable to me for ioat. Any concerns for other host DMA engines? 

DJ

> 
> I believe Jon and Dave know IOAT well too, so I'd appreciate any comments from
> them. I guess the mention on IOAT in an earlier patch in this series prompted
> Sashiko to focus on it here.
> 
> Best regards,
> Koichiro
> 
>>
>> Because IOAT DMA engines could still be actively copying into those buffers,
>> freeing them allows the hardware DMA engine to write to freed kernel memory.
>> Furthermore, the DMA drain wait in ntb_qp_link_cleanup() is subsequently
>> skipped entirely because qp_bitmap_alloc evaluates to 0 during the later
>> call to ntb_transport_link_cleanup().
>>
>> [ ... ]
>>
>>> @@ -1736,17 +1736,17 @@ static void ntb_process_rxc(struct ntb_transport_qp *qp)
>>>  
>>>  	if (hdr->len > entry->len) {
>>>  		dev_dbg(&qp->ndev->pdev->dev,
>>>  			"receive buffer overflow! Wanted %d got %d\n",
>>>  			hdr->len, entry->len);
>>>  		qp->rx_err_oflow++;
>>>  
>>>  		entry->len = -EIO;
>>>  		entry->flags |= DESC_DONE_FLAG;
>>>  
>>>  		ntb_complete_rxc(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_bytes += hdr->len;
>>>  		qp->rx_pkts++;
>>>  
>>>  		entry->len = hdr->len;
>>
>> [Severity: Critical]
>> This is a pre-existing issue, but is there a time-of-check to time-of-use
>> race condition in this bounds check on hdr->len?
>>
>> Since hdr points to a payload header located inside a memory window writable
>> by the remote NTB peer (shared memory), hdr->len is read directly from
>> shared memory twice (once for the bounds check, once for the assignment).
>>
>> Can a remote peer swap hdr->len to a much larger value immediately after
>> the bounds check but before the assignment, bypassing the bounds check and
>> causing a buffer overflow when copied?
>>
>> [ ... ]
>>
>>> @@ -2130,7 +2130,7 @@ struct ntb_transport_qp *
>>>  ntb_transport_create_queue(void *data, struct device *client_dev,
>>>  			   const struct ntb_queue_handlers *handlers)
>>>  {
>> [ ... ]
>>>  	free_queue--;
>>>  
>>>  	qp = &nt->qp_vec[free_queue];
>>>  	qp_bit = BIT_ULL(qp->qp_num);
>>>  
>>>  	nt->qp_bitmap_free &= ~qp_bit;
>>
>> [Severity: High]
>> This isn't a bug introduced by this patch, but could this read-modify-write
>> of nt->qp_bitmap_free lead to queue state corruption?
>>
>> The qp_bitmap_free bitmask is modified here using non-atomic bitwise
>> operations without holding any lock. It is also modified concurrently during
>> driver unbind:
>>
>> drivers/ntb/ntb_transport.c:ntb_transport_free_queue() {
>>     ...
>> 	qp->transport->qp_bitmap_free |= qp_bit;
>>     ...
>> }
>>
>> Since these are exported functions called by NTB client drivers, could
>> concurrent probe and remove operations race and lose bit updates, leading
>> to a leaked queue or two clients being assigned the same queue?
>>
>> -- 
>> Sashiko AI review · https://sashiko.dev/#/patchset/20260910040836.3792333-1-den@valinux.co.jp?part=14
> 

Re: [PATCH v2 02/14] NTB: ntb_transport: Start TX offload thread after queue setup

[Frank Li]

On Thu, Sep 10, 2026 at 01:08:24PM +0900, Koichiro Den wrote:
> ntb_transport_create_queue() starts the per-QP TX offload thread before
> DMA mappings and queue entries are allocated. If later setup fails, the
> error path returns the QP to the free bitmap without stopping the
> thread. A retry can then reinitialize its waitqueue while the old thread
> is still waiting on it.
>
> Start the thread after queue setup.
>
> Fixes: 322617a06c97 ("NTB: ntb_transport: Add 'tx_memcpy_offload' module option")
> Cc: stable@vger.kernel.org
> Signed-off-by: Koichiro Den <den@valinux.co.jp>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> Changes in v2:
>   - No changes.
>
> NOTE: Originally submitted as part of the direct TX/RX series v1:
> https://lore.kernel.org/r/20260810165136.2292436-4-den@valinux.co.jp/
>
>  drivers/ntb/ntb_transport.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index 3389d6ca9ebd..55a20ae9a85e 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -2055,20 +2055,6 @@ ntb_transport_create_queue(void *data, struct device *client_dev,
>  	qp->tx_handler = handlers->tx_handler;
>  	qp->event_handler = handlers->event_handler;
>
> -	init_waitqueue_head(&qp->tx_offload_wq);
> -	if (tx_memcpy_offload) {
> -		qp->tx_offload_thread = kthread_run(ntb_tx_memcpy_kthread, qp,
> -						    "ntb-txcpy/%s/%u",
> -						    pci_name(ndev->pdev), qp->qp_num);
> -		if (IS_ERR(qp->tx_offload_thread)) {
> -			dev_warn(&nt->ndev->dev,
> -				 "tx memcpy offload thread creation failed: %ld; falling back to inline copy\n",
> -				 PTR_ERR(qp->tx_offload_thread));
> -			qp->tx_offload_thread = NULL;
> -		}
> -	} else
> -		qp->tx_offload_thread = NULL;
> -
>  	dma_cap_zero(dma_mask);
>  	dma_cap_set(DMA_MEMCPY, dma_mask);
>
> @@ -2129,6 +2115,20 @@ ntb_transport_create_queue(void *data, struct device *client_dev,
>  			     &qp->tx_free_q);
>  	}
>
> +	init_waitqueue_head(&qp->tx_offload_wq);
> +	qp->tx_offload_thread = NULL;
> +	if (tx_memcpy_offload) {
> +		qp->tx_offload_thread = kthread_run(ntb_tx_memcpy_kthread, qp,
> +						    "ntb-txcpy/%s/%u",
> +						    pci_name(ndev->pdev), qp->qp_num);
> +		if (IS_ERR(qp->tx_offload_thread)) {
> +			dev_warn(&nt->ndev->dev,
> +				 "tx memcpy offload thread creation failed: %ld; falling back to inline copy\n",
> +				 PTR_ERR(qp->tx_offload_thread));
> +			qp->tx_offload_thread = NULL;
> +		}
> +	}
> +
>  	ntb_db_clear(qp->ndev, qp_bit);
>  	ntb_db_clear_mask(qp->ndev, qp_bit);
>
> --
> 2.51.0
>

Re: [PATCH v2 03/14] NTB: ntb_transport: Avoid deadlock when cancelling link work

[Frank Li]

On Thu, Sep 10, 2026 at 01:08:25PM +0900, Koichiro Den wrote:
> During initial link setup, ntb_transport_link_work() can retry with
> nt->link_is_up still false. A retry can block on link_event_lock
> while cleanup holds it and waits in cancel_delayed_work_sync(),
> leading to deadlock.
>
> Move the conditional cancellation outside link_event_lock, before
> QP cleanup. Keep QP cleanup and MW release under the lock so link
> work cannot restart QPs between them. Put the locking in
> ntb_transport_link_cleanup() to cover both worker and remove paths.
>
> Fixes: 3db835dd8f9a ("ntb: Add mutex to make link_event_callback executed linearly.")
> Cc: stable@vger.kernel.org
> Signed-off-by: Koichiro Den <den@valinux.co.jp>
> ---
> Changes in v2:
>   - No changes.
>
>  drivers/ntb/ntb_transport.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index 55a20ae9a85e..c77b173dca01 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -960,6 +960,15 @@ static void ntb_transport_link_cleanup(struct ntb_transport_ctx *nt)
>  	struct ntb_transport_qp *qp;
>  	u64 qp_bitmap_alloc;
>  	unsigned int i, count;
> +	bool cancel_link_work;
> +
> +	scoped_guard(mutex, &nt->link_event_lock)
> +		cancel_link_work = !nt->link_is_up;
> +
> +	if (cancel_link_work)
> +		cancel_delayed_work_sync(&nt->link_work);

what happen if schedule new work here?

Frank
> +
> +	guard(mutex)(&nt->link_event_lock);
>
>  	qp_bitmap_alloc = nt->qp_bitmap & ~nt->qp_bitmap_free;
>
> @@ -972,9 +981,6 @@ static void ntb_transport_link_cleanup(struct ntb_transport_ctx *nt)
>  			cancel_delayed_work_sync(&qp->link_work);
>  		}
>
> -	if (!nt->link_is_up)
> -		cancel_delayed_work_sync(&nt->link_work);
> -
>  	for (i = 0; i < nt->mw_count; i++)
>  		ntb_free_mw(nt, i);
>
> @@ -992,7 +998,6 @@ static void ntb_transport_link_cleanup_work(struct work_struct *work)
>  	struct ntb_transport_ctx *nt =
>  		container_of(work, struct ntb_transport_ctx, link_cleanup);
>
> -	guard(mutex)(&nt->link_event_lock);
>  	ntb_transport_link_cleanup(nt);
>  }
>
> --
> 2.51.0
>

Re: [PATCH v2 04/14] NTB: ntb_transport: Publish link state after QP setup

[Frank Li]

On Thu, Sep 10, 2026 at 01:08:26PM +0900, Koichiro Den wrote:
> ntb_transport_link_work() marks the transport link up before setting
> up the QPs' MW and peer MSI state. A concurrent ntb_transport_link_up()
> can then queue QP link work, which may enable RX and notify the client
> before setup finishes.
>
> Publish link_is_up with a release store after setting up all QPs,
> and use acquire loads before queuing QP link work.
>
> Fixes: fce8a7bb5b4b ("PCI-Express Non-Transparent Bridge Support")
> Cc: stable@vger.kernel.org
> Link: https://lore.kernel.org/r/anyKbq3mpLG4y7rb@SMW015318
> Signed-off-by: Koichiro Den <den@valinux.co.jp>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> Changes in v2:
>   - No changes.
>
>  drivers/ntb/ntb_transport.c | 40 +++++++++++++++++++++++--------------
>  1 file changed, 25 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index c77b173dca01..75d4a2e021f4 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -922,6 +922,16 @@ static void ntb_qp_link_down_reset(struct ntb_transport_qp *qp)
>  		qp->remote_rx_info->entry = qp->rx_max_entry - 1;
>  }
>
> +static void ntb_transport_schedule_qp_link(struct ntb_transport_qp *qp,
> +					   unsigned long delay)
> +{
> +	struct ntb_transport_ctx *nt = qp->transport;
> +
> +	/* Pair with the link publication in ntb_transport_link_work(). */
> +	if (smp_load_acquire(&nt->link_is_up))
> +		schedule_delayed_work(&qp->link_work, delay);
> +}
> +
>  static void ntb_qp_link_cleanup(struct ntb_transport_qp *qp)
>  {
>  	struct ntb_transport_ctx *nt = qp->transport;
> @@ -941,13 +951,10 @@ static void ntb_qp_link_cleanup_work(struct work_struct *work)
>  	struct ntb_transport_qp *qp = container_of(work,
>  						   struct ntb_transport_qp,
>  						   link_cleanup);
> -	struct ntb_transport_ctx *nt = qp->transport;
>
>  	ntb_qp_link_cleanup(qp);
> -
> -	if (nt->link_is_up)
> -		schedule_delayed_work(&qp->link_work,
> -				      msecs_to_jiffies(NTB_LINK_DOWN_TIMEOUT));
> +	ntb_transport_schedule_qp_link(qp,
> +				       msecs_to_jiffies(NTB_LINK_DOWN_TIMEOUT));
>  }
>
>  static void ntb_qp_link_down(struct ntb_transport_qp *qp)
> @@ -1089,16 +1096,19 @@ static void ntb_transport_link_work(struct work_struct *work)
>  			goto out1;
>  	}
>
> -	nt->link_is_up = true;
> -
>  	for (i = 0; i < nt->qp_count; i++) {
> -		struct ntb_transport_qp *qp = &nt->qp_vec[i];
> -
>  		ntb_transport_setup_qp_mw(nt, i);
>  		ntb_transport_setup_qp_peer_msi(nt, i);
> +	}
> +
> +	/* Publish the link only after every QP has been set up. */
> +	smp_store_release(&nt->link_is_up, true);
> +
> +	for (i = 0; i < nt->qp_count; i++) {
> +		struct ntb_transport_qp *qp = &nt->qp_vec[i];
>
>  		if (qp->client_ready)
> -			schedule_delayed_work(&qp->link_work, 0);
> +			ntb_transport_schedule_qp_link(qp, 0);
>  	}
>
>  	return;
> @@ -1146,9 +1156,10 @@ static void ntb_qp_link_work(struct work_struct *work)
>
>  		if (qp->active)
>  			tasklet_schedule(&qp->rxc_db_work);
> -	} else if (nt->link_is_up)
> -		schedule_delayed_work(&qp->link_work,
> -				      msecs_to_jiffies(NTB_LINK_DOWN_TIMEOUT));
> +	} else {
> +		ntb_transport_schedule_qp_link(qp,
> +					       msecs_to_jiffies(NTB_LINK_DOWN_TIMEOUT));
> +	}
>  }
>
>  static int ntb_transport_init_queue(struct ntb_transport_ctx *nt,
> @@ -2392,8 +2403,7 @@ void ntb_transport_link_up(struct ntb_transport_qp *qp)
>
>  	qp->client_ready = true;
>
> -	if (qp->transport->link_is_up)
> -		schedule_delayed_work(&qp->link_work, 0);
> +	ntb_transport_schedule_qp_link(qp, 0);
>  }
>  EXPORT_SYMBOL_GPL(ntb_transport_link_up);
>
> --
> 2.51.0
>

Re: [PATCH v2 05/14] NTB: ntb_transport: Avoid losing QP link-up requests

[Frank Li]

On Thu, Sep 10, 2026 at 01:08:27PM +0900, Koichiro Den wrote:
> ntb_netdev_open() can call ntb_transport_link_up() while the transport
> worker is completing setup on another CPU. Concurrent transport setup
> and a client link-up request can both read the other's flag as false and
> leave QP link work unqueued. The QP then stays down until another link
> event or client link-up request.
>
> This is the store-buffering pattern described in
> tools/memory-model/Documentation/recipes.txt ("Store buffering").
>
> Add a full barrier between the store and load on each side, and
> mark the client_ready accesses with READ_ONCE()/WRITE_ONCE().
>
> Fixes: fce8a7bb5b4b ("PCI-Express Non-Transparent Bridge Support")
> Cc: stable@vger.kernel.org
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Link: https://lore.kernel.org/r/20260907144701.702E41F00A3A@smtp.kernel.org/
> Signed-off-by: Koichiro Den <den@valinux.co.jp>
> ---
> Changes in v2:
>   - New patch (Sashiko)
>
>  drivers/ntb/ntb_transport.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index 75d4a2e021f4..1332d53bcfe7 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -1104,10 +1104,16 @@ static void ntb_transport_link_work(struct work_struct *work)
>  	/* Publish the link only after every QP has been set up. */
>  	smp_store_release(&nt->link_is_up, true);
>
> +	/*
> +	 * Prevent both sides from missing each other's flag. Pairs with
> +	 * the barrier in ntb_transport_link_up().
> +	 */
> +	smp_mb();
> +
>  	for (i = 0; i < nt->qp_count; i++) {
>  		struct ntb_transport_qp *qp = &nt->qp_vec[i];
>
> -		if (qp->client_ready)
> +		if (READ_ONCE(qp->client_ready))

I think it'd better change to use atomic variable for client_ready to avoid
manual handle smp sync.

Frank

>  			ntb_transport_schedule_qp_link(qp, 0);
>  	}
>
> @@ -2401,7 +2407,10 @@ void ntb_transport_link_up(struct ntb_transport_qp *qp)
>  	if (!qp)
>  		return;
>
> -	qp->client_ready = true;
> +	WRITE_ONCE(qp->client_ready, true);
> +
> +	/* Pairs with the barrier in ntb_transport_link_work(). */
> +	smp_mb();
>
>  	ntb_transport_schedule_qp_link(qp, 0);
>  }
> --
> 2.51.0
>

Re: [PATCH v2 03/14] NTB: ntb_transport: Avoid deadlock when cancelling link work

[Koichiro Den]

On Fri, Sep 11, 2026 at 11:21:43AM -0500, Frank Li wrote:
> On Thu, Sep 10, 2026 at 01:08:25PM +0900, Koichiro Den wrote:
> > During initial link setup, ntb_transport_link_work() can retry with
> > nt->link_is_up still false. A retry can block on link_event_lock
> > while cleanup holds it and waits in cancel_delayed_work_sync(),
> > leading to deadlock.
> >
> > Move the conditional cancellation outside link_event_lock, before
> > QP cleanup. Keep QP cleanup and MW release under the lock so link
> > work cannot restart QPs between them. Put the locking in
> > ntb_transport_link_cleanup() to cover both worker and remove paths.
> >
> > Fixes: 3db835dd8f9a ("ntb: Add mutex to make link_event_callback executed linearly.")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Koichiro Den <den@valinux.co.jp>
> > ---
> > Changes in v2:
> >   - No changes.
> >
> >  drivers/ntb/ntb_transport.c | 13 +++++++++----
> >  1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> > index 55a20ae9a85e..c77b173dca01 100644
> > --- a/drivers/ntb/ntb_transport.c
> > +++ b/drivers/ntb/ntb_transport.c
> > @@ -960,6 +960,15 @@ static void ntb_transport_link_cleanup(struct ntb_transport_ctx *nt)
> >  	struct ntb_transport_qp *qp;
> >  	u64 qp_bitmap_alloc;
> >  	unsigned int i, count;
> > +	bool cancel_link_work;
> > +
> > +	scoped_guard(mutex, &nt->link_event_lock)
> > +		cancel_link_work = !nt->link_is_up;
> > +
> > +	if (cancel_link_work)
> > +		cancel_delayed_work_sync(&nt->link_work);
> 
> what happen if schedule new work here?

Both link_work and link_cleanup can race to take link_event_lock.

- If link_work wins and completes setup, cleanup can tear the link down again.
  The old code also allowed setup to run before a pending cleanup though. In
  that sense, although Sashiko flagged it as a new issue, I believe the ordering
  issue itself is pre-existing.
- If link_cleanup wins, link_work just waits for cleanup to finish before
  starting setup.

This patch fixes the deadlock, not the event ordering. I would prefer to handle
the ordering issue separately.

Best regards,
Koichiro

> 
> Frank
> > +
> > +	guard(mutex)(&nt->link_event_lock);
> >
> >  	qp_bitmap_alloc = nt->qp_bitmap & ~nt->qp_bitmap_free;
> >
> > @@ -972,9 +981,6 @@ static void ntb_transport_link_cleanup(struct ntb_transport_ctx *nt)
> >  			cancel_delayed_work_sync(&qp->link_work);
> >  		}
> >
> > -	if (!nt->link_is_up)
> > -		cancel_delayed_work_sync(&nt->link_work);
> > -
> >  	for (i = 0; i < nt->mw_count; i++)
> >  		ntb_free_mw(nt, i);
> >
> > @@ -992,7 +998,6 @@ static void ntb_transport_link_cleanup_work(struct work_struct *work)
> >  	struct ntb_transport_ctx *nt =
> >  		container_of(work, struct ntb_transport_ctx, link_cleanup);
> >
> > -	guard(mutex)(&nt->link_event_lock);
> >  	ntb_transport_link_cleanup(nt);
> >  }
> >
> > --
> > 2.51.0
> >

Re: [PATCH v2 05/14] NTB: ntb_transport: Avoid losing QP link-up requests

[Koichiro Den]

On Fri, Sep 11, 2026 at 11:53:37AM -0500, Frank Li wrote:
> On Thu, Sep 10, 2026 at 01:08:27PM +0900, Koichiro Den wrote:
> > ntb_netdev_open() can call ntb_transport_link_up() while the transport
> > worker is completing setup on another CPU. Concurrent transport setup
> > and a client link-up request can both read the other's flag as false and
> > leave QP link work unqueued. The QP then stays down until another link
> > event or client link-up request.
> >
> > This is the store-buffering pattern described in
> > tools/memory-model/Documentation/recipes.txt ("Store buffering").
> >
> > Add a full barrier between the store and load on each side, and
> > mark the client_ready accesses with READ_ONCE()/WRITE_ONCE().
> >
> > Fixes: fce8a7bb5b4b ("PCI-Express Non-Transparent Bridge Support")
> > Cc: stable@vger.kernel.org
> > Reported-by: Sashiko <sashiko-bot@kernel.org>
> > Link: https://lore.kernel.org/r/20260907144701.702E41F00A3A@smtp.kernel.org/
> > Signed-off-by: Koichiro Den <den@valinux.co.jp>
> > ---
> > Changes in v2:
> >   - New patch (Sashiko)
> >
> >  drivers/ntb/ntb_transport.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> > index 75d4a2e021f4..1332d53bcfe7 100644
> > --- a/drivers/ntb/ntb_transport.c
> > +++ b/drivers/ntb/ntb_transport.c
> > @@ -1104,10 +1104,16 @@ static void ntb_transport_link_work(struct work_struct *work)
> >  	/* Publish the link only after every QP has been set up. */
> >  	smp_store_release(&nt->link_is_up, true);
> >
> > +	/*
> > +	 * Prevent both sides from missing each other's flag. Pairs with
> > +	 * the barrier in ntb_transport_link_up().
> > +	 */
> > +	smp_mb();
> > +
> >  	for (i = 0; i < nt->qp_count; i++) {
> >  		struct ntb_transport_qp *qp = &nt->qp_vec[i];
> >
> > -		if (qp->client_ready)
> > +		if (READ_ONCE(qp->client_ready))
> 
> I think it'd better change to use atomic variable for client_ready to avoid
> manual handle smp sync.

AFAICT converting client_ready to atomic_t and using atomic_set()/atomic_read()
instead of WRITE_ONCE()/READ_ONCE() would not free us from the smp sync, as
those operations are unordered.
So to illustrate this in litmus test like form:

   # L = nt->link_is_up
   # R = qp->client_ready
   # Both initially false

   Transport setup                  Client link-up
   ---------------                  --------------
    smp_store_release(&L, true);     WRITE_ONCE(R, true);
    smp_mb(); /* added */            smp_mb(); /* added */
    r0 = READ_ONCE(R);               r1 = smp_load_acquire(&L);

   Both reads return false?
    Before: allowed
    After:  forbidden

I might be misunderstanding your suggestion though. If you have something
different in mind, please let me know.

Thanks for the review.
Koichiro

> 
> Frank
> 
> >  			ntb_transport_schedule_qp_link(qp, 0);
> >  	}
> >
> > @@ -2401,7 +2407,10 @@ void ntb_transport_link_up(struct ntb_transport_qp *qp)
> >  	if (!qp)
> >  		return;
> >
> > -	qp->client_ready = true;
> > +	WRITE_ONCE(qp->client_ready, true);
> > +
> > +	/* Pairs with the barrier in ntb_transport_link_work(). */
> > +	smp_mb();
> >
> >  	ntb_transport_schedule_qp_link(qp, 0);
> >  }
> > --
> > 2.51.0
> >

Re: [PATCH v2 05/14] NTB: ntb_transport: Avoid losing QP link-up requests

[Koichiro Den]

On Sat, Sep 12, 2026 at 03:04:21AM +0900, Koichiro Den wrote:
> On Fri, Sep 11, 2026 at 11:53:37AM -0500, Frank Li wrote:
> > On Thu, Sep 10, 2026 at 01:08:27PM +0900, Koichiro Den wrote:
> > > ntb_netdev_open() can call ntb_transport_link_up() while the transport
> > > worker is completing setup on another CPU. Concurrent transport setup
> > > and a client link-up request can both read the other's flag as false and
> > > leave QP link work unqueued. The QP then stays down until another link
> > > event or client link-up request.
> > >
> > > This is the store-buffering pattern described in
> > > tools/memory-model/Documentation/recipes.txt ("Store buffering").
> > >
> > > Add a full barrier between the store and load on each side, and
> > > mark the client_ready accesses with READ_ONCE()/WRITE_ONCE().
> > >
> > > Fixes: fce8a7bb5b4b ("PCI-Express Non-Transparent Bridge Support")
> > > Cc: stable@vger.kernel.org
> > > Reported-by: Sashiko <sashiko-bot@kernel.org>
> > > Link: https://lore.kernel.org/r/20260907144701.702E41F00A3A@smtp.kernel.org/
> > > Signed-off-by: Koichiro Den <den@valinux.co.jp>
> > > ---
> > > Changes in v2:
> > >   - New patch (Sashiko)
> > >
> > >  drivers/ntb/ntb_transport.c | 13 +++++++++++--
> > >  1 file changed, 11 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> > > index 75d4a2e021f4..1332d53bcfe7 100644
> > > --- a/drivers/ntb/ntb_transport.c
> > > +++ b/drivers/ntb/ntb_transport.c
> > > @@ -1104,10 +1104,16 @@ static void ntb_transport_link_work(struct work_struct *work)
> > >  	/* Publish the link only after every QP has been set up. */
> > >  	smp_store_release(&nt->link_is_up, true);
> > >
> > > +	/*
> > > +	 * Prevent both sides from missing each other's flag. Pairs with
> > > +	 * the barrier in ntb_transport_link_up().
> > > +	 */
> > > +	smp_mb();
> > > +
> > >  	for (i = 0; i < nt->qp_count; i++) {
> > >  		struct ntb_transport_qp *qp = &nt->qp_vec[i];
> > >
> > > -		if (qp->client_ready)
> > > +		if (READ_ONCE(qp->client_ready))
> > 
> > I think it'd better change to use atomic variable for client_ready to avoid
> > manual handle smp sync.
> 
> AFAICT converting client_ready to atomic_t and using atomic_set()/atomic_read()
> instead of WRITE_ONCE()/READ_ONCE() would not free us from the smp sync, as
> those operations are unordered.
> So to illustrate this in litmus test like form:
> 
>    # L = nt->link_is_up
>    # R = qp->client_ready
>    # Both initially false
> 
>    Transport setup                  Client link-up
>    ---------------                  --------------
>     smp_store_release(&L, true);     WRITE_ONCE(R, true);
>     smp_mb(); /* added */            smp_mb(); /* added */
>     r0 = READ_ONCE(R);               r1 = smp_load_acquire(&L);
> 
>    Both reads return false?
>     Before: allowed
>     After:  forbidden
> 
> I might be misunderstanding your suggestion though. If you have something
> different in mind, please let me know.

Just for the record, smp_store_release/smp_load_acquire depicted above, instead
of WRITE_ONCE()/READ_ONCE(), is intentional. They are for MP ordering to publish
the QP setup, which is needed by an earlier patch:
https://lore.kernel.org/r/20260910040836.3792333-5-den@valinux.co.jp/

Best regards,
Koichiro

> 
> Thanks for the review.
> Koichiro
> 
> > 
> > Frank
> > 
> > >  			ntb_transport_schedule_qp_link(qp, 0);
> > >  	}
> > >
> > > @@ -2401,7 +2407,10 @@ void ntb_transport_link_up(struct ntb_transport_qp *qp)
> > >  	if (!qp)
> > >  		return;
> > >
> > > -	qp->client_ready = true;
> > > +	WRITE_ONCE(qp->client_ready, true);
> > > +
> > > +	/* Pairs with the barrier in ntb_transport_link_work(). */
> > > +	smp_mb();
> > >
> > >  	ntb_transport_schedule_qp_link(qp, 0);
> > >  }
> > > --
> > > 2.51.0
> > >

Re: [PATCH v2 05/14] NTB: ntb_transport: Avoid losing QP link-up requests

[Frank Li]

On Sat, Sep 12, 2026 at 03:21:08AM +0900, Koichiro Den wrote:
> On Sat, Sep 12, 2026 at 03:04:21AM +0900, Koichiro Den wrote:
> > On Fri, Sep 11, 2026 at 11:53:37AM -0500, Frank Li wrote:
> > > On Thu, Sep 10, 2026 at 01:08:27PM +0900, Koichiro Den wrote:
> > > > ntb_netdev_open() can call ntb_transport_link_up() while the transport
> > > > worker is completing setup on another CPU. Concurrent transport setup
> > > > and a client link-up request can both read the other's flag as false and
> > > > leave QP link work unqueued. The QP then stays down until another link
> > > > event or client link-up request.
> > > >
> > > > This is the store-buffering pattern described in
> > > > tools/memory-model/Documentation/recipes.txt ("Store buffering").
> > > >
> > > > Add a full barrier between the store and load on each side, and
> > > > mark the client_ready accesses with READ_ONCE()/WRITE_ONCE().
> > > >
> > > > Fixes: fce8a7bb5b4b ("PCI-Express Non-Transparent Bridge Support")
> > > > Cc: stable@vger.kernel.org
> > > > Reported-by: Sashiko <sashiko-bot@kernel.org>
> > > > Link: https://lore.kernel.org/r/20260907144701.702E41F00A3A@smtp.kernel.org/
> > > > Signed-off-by: Koichiro Den <den@valinux.co.jp>
> > > > ---
> > > > Changes in v2:
> > > >   - New patch (Sashiko)
> > > >
> > > >  drivers/ntb/ntb_transport.c | 13 +++++++++++--
> > > >  1 file changed, 11 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> > > > index 75d4a2e021f4..1332d53bcfe7 100644
> > > > --- a/drivers/ntb/ntb_transport.c
> > > > +++ b/drivers/ntb/ntb_transport.c
> > > > @@ -1104,10 +1104,16 @@ static void ntb_transport_link_work(struct work_struct *work)
> > > >  	/* Publish the link only after every QP has been set up. */
> > > >  	smp_store_release(&nt->link_is_up, true);
> > > >
> > > > +	/*
> > > > +	 * Prevent both sides from missing each other's flag. Pairs with
> > > > +	 * the barrier in ntb_transport_link_up().
> > > > +	 */
> > > > +	smp_mb();
> > > > +
> > > >  	for (i = 0; i < nt->qp_count; i++) {
> > > >  		struct ntb_transport_qp *qp = &nt->qp_vec[i];
> > > >
> > > > -		if (qp->client_ready)
> > > > +		if (READ_ONCE(qp->client_ready))
> > >
> > > I think it'd better change to use atomic variable for client_ready to avoid
> > > manual handle smp sync.
> >
> > AFAICT converting client_ready to atomic_t and using atomic_set()/atomic_read()
> > instead of WRITE_ONCE()/READ_ONCE() would not free us from the smp sync, as
> > those operations are unordered.

there are acquire version for atomic

atomic_set_release()

atomic_read_acquire()

My key point is use existing higher level sync APIs to avoid consider
barrier problem, which need more brain cell to think it.

this is just sync state, which don't impact performance.

Frank

> > So to illustrate this in litmus test like form:
> >
> >    # L = nt->link_is_up
> >    # R = qp->client_ready
> >    # Both initially false
> >
> >    Transport setup                  Client link-up
> >    ---------------                  --------------
> >     smp_store_release(&L, true);     WRITE_ONCE(R, true);
> >     smp_mb(); /* added */            smp_mb(); /* added */
> >     r0 = READ_ONCE(R);               r1 = smp_load_acquire(&L);
> >
> >    Both reads return false?
> >     Before: allowed
> >     After:  forbidden
> >
> > I might be misunderstanding your suggestion though. If you have something
> > different in mind, please let me know.
>
> Just for the record, smp_store_release/smp_load_acquire depicted above, instead
> of WRITE_ONCE()/READ_ONCE(), is intentional. They are for MP ordering to publish
> the QP setup, which is needed by an earlier patch:
> https://lore.kernel.org/r/20260910040836.3792333-5-den@valinux.co.jp/
>
> Best regards,
> Koichiro
>
> >
> > Thanks for the review.
> > Koichiro
> >
> > >
> > > Frank
> > >
> > > >  			ntb_transport_schedule_qp_link(qp, 0);
> > > >  	}
> > > >
> > > > @@ -2401,7 +2407,10 @@ void ntb_transport_link_up(struct ntb_transport_qp *qp)
> > > >  	if (!qp)
> > > >  		return;
> > > >
> > > > -	qp->client_ready = true;
> > > > +	WRITE_ONCE(qp->client_ready, true);
> > > > +
> > > > +	/* Pairs with the barrier in ntb_transport_link_work(). */
> > > > +	smp_mb();
> > > >
> > > >  	ntb_transport_schedule_qp_link(qp, 0);
> > > >  }
> > > > --
> > > > 2.51.0
> > > >

Re: [PATCH v2 05/14] NTB: ntb_transport: Avoid losing QP link-up requests

[Koichiro Den]

On Fri, Sep 11, 2026 at 10:20:34PM -0500, Frank Li wrote:
> On Sat, Sep 12, 2026 at 03:21:08AM +0900, Koichiro Den wrote:
> > On Sat, Sep 12, 2026 at 03:04:21AM +0900, Koichiro Den wrote:
> > > On Fri, Sep 11, 2026 at 11:53:37AM -0500, Frank Li wrote:
> > > > On Thu, Sep 10, 2026 at 01:08:27PM +0900, Koichiro Den wrote:
> > > > > ntb_netdev_open() can call ntb_transport_link_up() while the transport
> > > > > worker is completing setup on another CPU. Concurrent transport setup
> > > > > and a client link-up request can both read the other's flag as false and
> > > > > leave QP link work unqueued. The QP then stays down until another link
> > > > > event or client link-up request.
> > > > >
> > > > > This is the store-buffering pattern described in
> > > > > tools/memory-model/Documentation/recipes.txt ("Store buffering").
> > > > >
> > > > > Add a full barrier between the store and load on each side, and
> > > > > mark the client_ready accesses with READ_ONCE()/WRITE_ONCE().
> > > > >
> > > > > Fixes: fce8a7bb5b4b ("PCI-Express Non-Transparent Bridge Support")
> > > > > Cc: stable@vger.kernel.org
> > > > > Reported-by: Sashiko <sashiko-bot@kernel.org>
> > > > > Link: https://lore.kernel.org/r/20260907144701.702E41F00A3A@smtp.kernel.org/
> > > > > Signed-off-by: Koichiro Den <den@valinux.co.jp>
> > > > > ---
> > > > > Changes in v2:
> > > > >   - New patch (Sashiko)
> > > > >
> > > > >  drivers/ntb/ntb_transport.c | 13 +++++++++++--
> > > > >  1 file changed, 11 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> > > > > index 75d4a2e021f4..1332d53bcfe7 100644
> > > > > --- a/drivers/ntb/ntb_transport.c
> > > > > +++ b/drivers/ntb/ntb_transport.c
> > > > > @@ -1104,10 +1104,16 @@ static void ntb_transport_link_work(struct work_struct *work)
> > > > >  	/* Publish the link only after every QP has been set up. */
> > > > >  	smp_store_release(&nt->link_is_up, true);
> > > > >
> > > > > +	/*
> > > > > +	 * Prevent both sides from missing each other's flag. Pairs with
> > > > > +	 * the barrier in ntb_transport_link_up().
> > > > > +	 */
> > > > > +	smp_mb();
> > > > > +
> > > > >  	for (i = 0; i < nt->qp_count; i++) {
> > > > >  		struct ntb_transport_qp *qp = &nt->qp_vec[i];
> > > > >
> > > > > -		if (qp->client_ready)
> > > > > +		if (READ_ONCE(qp->client_ready))
> > > >
> > > > I think it'd better change to use atomic variable for client_ready to avoid
> > > > manual handle smp sync.
> > >
> > > AFAICT converting client_ready to atomic_t and using atomic_set()/atomic_read()
> > > instead of WRITE_ONCE()/READ_ONCE() would not free us from the smp sync, as
> > > those operations are unordered.
> 
> there are acquire version for atomic
> 
> atomic_set_release()
> 
> atomic_read_acquire()
> 
> My key point is use existing higher level sync APIs to avoid consider
> barrier problem, which need more brain cell to think it.
> 
> this is just sync state, which don't impact performance.

I agree with the idea of changing both flags to atomic_t for cleaner code, but
I'm not sure if it frees us from "the smp sync" or the "barrier problem". We'd
still need both smp_mb()s and still have to think about *_release/*_acquire when
revisiting the code and maybe scratching our heads, right?

BTW, I once wondered whether it might be worth consolidating
link_work/link_cleanup into a single state machine worker per transport or QP.
That could simplify synchronization between setup and cleanup, which aren't
really performance-critical, as you say. However, that would possibly require a
larger rework, so for now I've put together this fix series as a small step. I
would be interested to hear what NTB maintainers think about the possibility of
such a rework.

Best regards,
Koichiro

> 
> Frank
> 
> > > So to illustrate this in litmus test like form:
> > >
> > >    # L = nt->link_is_up
> > >    # R = qp->client_ready
> > >    # Both initially false
> > >
> > >    Transport setup                  Client link-up
> > >    ---------------                  --------------
> > >     smp_store_release(&L, true);     WRITE_ONCE(R, true);
> > >     smp_mb(); /* added */            smp_mb(); /* added */
> > >     r0 = READ_ONCE(R);               r1 = smp_load_acquire(&L);
> > >
> > >    Both reads return false?
> > >     Before: allowed
> > >     After:  forbidden
> > >
> > > I might be misunderstanding your suggestion though. If you have something
> > > different in mind, please let me know.
> >
> > Just for the record, smp_store_release/smp_load_acquire depicted above, instead
> > of WRITE_ONCE()/READ_ONCE(), is intentional. They are for MP ordering to publish
> > the QP setup, which is needed by an earlier patch:
> > https://lore.kernel.org/r/20260910040836.3792333-5-den@valinux.co.jp/
> >
> > Best regards,
> > Koichiro
> >
> > >
> > > Thanks for the review.
> > > Koichiro
> > >
> > > >
> > > > Frank
> > > >
> > > > >  			ntb_transport_schedule_qp_link(qp, 0);
> > > > >  	}
> > > > >
> > > > > @@ -2401,7 +2407,10 @@ void ntb_transport_link_up(struct ntb_transport_qp *qp)
> > > > >  	if (!qp)
> > > > >  		return;
> > > > >
> > > > > -	qp->client_ready = true;
> > > > > +	WRITE_ONCE(qp->client_ready, true);
> > > > > +
> > > > > +	/* Pairs with the barrier in ntb_transport_link_work(). */
> > > > > +	smp_mb();
> > > > >
> > > > >  	ntb_transport_schedule_qp_link(qp, 0);
> > > > >  }
> > > > > --
> > > > > 2.51.0
> > > > >