[PATCH 00/11] NTB: ntb_transport: Miscellaneous fixes

[PATCH 00/11] 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-rc1.

[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


Koichiro Den (11):
  NTB: ntb_transport: Remove the device debugfs directory
  NTB: ntb_transport: Start TX offload thread after queue setup
  NTB: ntb_transport: Prepare remote RX info accesses for MW teardown
  NTB: ntb_transport: Clear QP pointers when freeing an MW
  NTB: ntb_transport: Avoid deadlock when cancelling link work
  NTB: ntb_transport: Publish link state after QP setup
  NTB: ntb_transport: Clear link state before QP cleanup
  NTB: ntb_transport: Abort link setup on QP MW allocation failure
  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

 drivers/ntb/ntb_transport.c | 190 ++++++++++++++++++++++++------------
 1 file changed, 128 insertions(+), 62 deletions(-)

base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
-- 
2.51.0

[PATCH 01/11] 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")
Signed-off-by: Koichiro Den <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 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 01/11] NTB: ntb_transport: Remove the device debugfs directory

[sashiko-bot]

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

Pre-existing issues:
- [High] The error handling path in `ntb_transport_probe()` incorrectly reuses the loop variable `i`, leading to an out-of-bounds `iounmap()` or a memory leak of mapped memory windows.
- [High] Use-after-free of `ntb_transport_ctx` due to cancelling background works before disabling new events in `ntb_transport_free`.
- [Medium] Incorrect dma_cookie_t used for waiting on TX DMA channel completion during queue teardown.
- [Medium] Memory windows are leaked because `ntb_transport_probe` allocates and maps `mw_count` elements but `ntb_transport_free` only unmaps `nt->mw_count` elements.
--

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

    NTB: ntb_transport: Remove the device debugfs directory
    
    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.

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

[Severity: High]
This isn't a bug introduced by this patch, but does this loop correctly
unmap the memory windows if probe fails?

In ntb_transport_probe(), 'i' is used to map memory windows up to
'mw_count', but then 'i' is reused as a loop index to initialize
'qp_count' queue pairs.

Since 'qp_count' can be clamped incorrectly and end up greater than
'mw_count', if an error occurs and execution falls through to 'err1',
'i' now represents the failed queue pair index (or 'qp_count'), rather
than 'mw_count'.

Could this cause an out-of-bounds read on nt->mw_vec if qp_count >
mw_count, or a memory leak if qp_count < mw_count?

>  		mw = &nt->mw_vec[i];
>  		iounmap(mw->vbase);
>  	}
>  	kfree(nt->mw_vec);

[ ... ]

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

[Severity: High]
This is a pre-existing issue, but does cancelling the link works here
leave a race window for a use-after-free?

The background works are cancelled here, but ntb_clear_ctx(ndev) isn't
called until further down in ntb_transport_free().

If a hardware link event occurs in this intermediate time window,
ntb_transport_event_callback() can still be invoked by the NTB core,
which re-schedules nt->link_cleanup or nt->link_work.

Since ntb_clear_ctx() doesn't cancel pending workqueue items, could the
rescheduled work execute after 'nt' is freed?

>  
>  	qp_bitmap_alloc = nt->qp_bitmap & ~nt->qp_bitmap_free;
>  
>  	/* verify that all the qp's are freed */
>  	for (i = 0; i < nt->qp_count; i++) {
>  		qp = &nt->qp_vec[i];
>  		if (qp_bitmap_alloc & BIT_ULL(i))
>  			ntb_transport_free_queue(qp);

[Severity: Medium]
This isn't a bug introduced by this patch, but does
ntb_transport_free_queue() wait on the correct DMA cookie?

When tearing down the queue, ntb_transport_free_queue() calls:
    dma_sync_wait(chan, qp->last_cookie);

However, qp->last_cookie only tracks RX submissions because
ntb_async_tx_submit() ignores the cookie returned by dmaengine_submit()
and doesn't update it.

Since last_cookie tracks RX submissions (or is uninitialized), does this
wait have any effect on the TX channel? Could this lead to premature
termination of in-flight TX transactions?

> -		debugfs_remove_recursive(qp->debugfs_dir);
>  	}
>  
>  	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);

[Severity: Medium]
This is a pre-existing issue, but does this loop correctly unmap all the
allocated memory windows?

In ntb_transport_probe(), 'mw_count' memory windows are allocated and
mapped. Later in the probe function, nt->mw_count is clamped to a smaller
value if scratchpads are limited.

When ntb_transport_free() tears down the transport, it loops only up to
nt->mw_count.

Doesn't this permanently leak the remaining (mw_count - nt->mw_count)
mapped IO windows and resources?

>  	}

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

[PATCH 02/11] 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")
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
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 02/11] NTB: ntb_transport: Start TX offload thread after queue setup

[sashiko-bot]

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

Pre-existing issues:
- [High] Potential DMA mapping leak on the error path because DMA address 0 is treated as invalid.
- [High] Concurrent accesses to `nt->qp_bitmap_free` are not synchronized, which can lead to QP assignment corruption.
- [High] Leaked queue entries and dangling pointer usage when stopping TX offload thread in ntb_transport_free_queue.
- [Medium] Unprotected modification of qp->tx_bytes and other statistics.
--

commit 7bd9e932183752e71eddb3f2bc51cf46a03b501b
Author: Koichiro Den <den@valinux.co.jp>
Date:   Mon Sep 7 14:24:20 2026 +0000

    NTB: ntb_transport: Start TX offload thread after queue setup
    
    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")
    Signed-off-by: Koichiro Den <den@valinux.co.jp>

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index 3389d6ca9ebd2..55a20ae9a85e6 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,

[Severity: High]
This isn't a bug introduced by this patch, but does this function have a race
condition when modifying the shared bitmap?

If multiple client drivers invoke ntb_transport_create_queue() concurrently,
they might observe the same free_queue bit:

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

Because these modifications to nt->qp_bitmap_free are done without holding any
locks, can this lead to assigning the same queue pair to multiple clients?

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

If dma_map_resource() succeeds but returns 0 (which is a valid DMA address on
some architectures), and a subsequent setup step fails, the error path does
this:

drivers/ntb/ntb_transport.c:ntb_transport_create_queue() {
    ...
err1:
    ...
    if (qp->tx_mw_dma_addr)
        dma_unmap_resource(qp->tx_dma_chan->device->dev,
                           qp->tx_mw_dma_addr, qp->tx_mw_size,
                           DMA_FROM_DEVICE, 0);
    ...
}

Since 0 evaluates to false, the mapping is skipped and leaked. Should this
check against DMA_MAPPING_ERROR or use a dedicated boolean flag instead?

>  	qp->tx_handler = handlers->tx_handler;
>  	qp->event_handler = handlers->event_handler;
>  
[ ... ]
>  	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,
[ ... ]
> +	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);

[Severity: High]
This isn't a bug introduced by this patch, but does the offload thread cleanup
in ntb_transport_free_queue() leave a dangling pointer?

When the thread is stopped, the pointer is set to NULL after the thread exits:

drivers/ntb/ntb_transport.c:ntb_transport_free_queue() {
    ...
    kthread_stop(qp->tx_offload_thread);
    qp->tx_offload_thread = NULL;
    ...
}

If a concurrent DMA transaction aborts before the DMA engine is fully
terminated, ntb_tx_copy_callback() might evaluate ntb_tx_offload_enabled():

drivers/ntb/ntb_transport.c:ntb_tx_offload_enabled() {
    return tx_memcpy_offload && qp && qp->tx_offload_thread;
}

Because the pointer is still non-NULL until kthread_stop() returns, the entry
is added to tx_offl_q.

Since the thread is dead, it will never process the queue, and the underlying
client data buffers (like network socket buffers) will be permanently leaked
when tx_offl_q entries are subsequently cleaned up.

[Severity: Medium]
This is a pre-existing issue, but does ntb_tx_copy_callback() have unprotected
modifications to statistics?

drivers/ntb/ntb_transport.c:ntb_tx_copy_callback() {
    ...
    qp->tx_bytes += len;
    ...
}

When a high volume of traffic triggers concurrent increments from multiple
contexts (e.g., DMA callbacks and the Tx offload kthread), can this 64-bit
statistic suffer from data races or torn updates on 32-bit platforms without
locking or u64_stats_sync protection?

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

[PATCH 03/11] 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.

Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
 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 55a20ae9a85e..944d38c7efd3 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -487,6 +487,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;
@@ -514,7 +515,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');
@@ -609,7 +612,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);
@@ -917,9 +920,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_qp_link_cleanup(struct ntb_transport_qp *qp)
@@ -2487,8 +2493,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

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Lockless NULL check of `qp->remote_rx_info` is insufficient to protect against concurrent memory window teardown, leading to Time-of-Check to Time-of-Use (TOCTOU) UAF/WAF vulnerabilities.
--

commit af53bb9020271a62d936b961c8c9d17ee66f2b23
Author: Koichiro Den <den@valinux.co.jp>
NTB: ntb_transport: Prepare remote RX info accesses for MW teardown

This commit attempts to protect against concurrent memory window teardown
by adding lockless READ_ONCE()/WRITE_ONCE() and NULL checks for
qp->remote_rx_info during debugfs reads, link down resets, and TX free
entry calculations.

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index 55a20ae9a85e6..944d38c7efd3f 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
[ ... ]
> @@ -514,7 +515,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 isn't a bug introduced by this patch, but does this lockless NULL check
fully protect against use-after-free here?

If a thread executing ntb_qp_debugfs_stats_show() reads qp->remote_rx_info
and passes the NULL check, but is then preempted before dereferencing it:

Thread 1
ntb_qp_debugfs_stats_show()
    remote_rx_info = READ_ONCE(qp->remote_rx_info);
    if (remote_rx_info)

Thread 2
ntb_qp_link_cleanup()
    ntb_free_mw() clears pointer and frees backing DMA memory

Could Thread 1 then resume and dereference the locally cached pointer,
accessing freed memory?

[ ... ]
> @@ -917,9 +920,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;

[Severity: High]
This is a pre-existing issue, but does this write expose a similar window
for a write-after-free?

If remote_rx_info is checked for NULL, but the backing memory is freed by
ntb_free_mw() before the assignment to remote_rx_info->entry, could this
corrupt reallocated memory?

[ ... ]
> @@ -2487,8 +2493,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;

[Severity: High]
This is a pre-existing issue, but is the lockless NULL check in
ntb_transport_tx_free_entry() sufficient to prevent a use-after-free
read during network TX?

Similar to the debugfs read, if this is called from the network fast path
(for example, ndo_start_xmit() calling ntb_transport_tx_enqueue()) and the
pointer is cached locally, could a concurrent link state change that frees
the DMA memory cause the subsequent dereference at remote_rx_info->entry
to access freed memory?

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

[PATCH 04/11] 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")
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
 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 944d38c7efd3..763e4bb175a3 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -764,10 +764,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

Re: [PATCH 04/11] NTB: ntb_transport: Clear QP pointers when freeing an MW

[sashiko-bot]

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

New issues:
- [High] Unsynchronized nullification of `qp->rx_buff` introduces a deterministic NULL pointer dereference in the asynchronous DMA callback.

Pre-existing issues:
- [High] The memory window (MW) DMA buffer is freed without terminating active RX DMA engine transactions.
- [High] Incomplete fix: The nullification of `qp->remote_rx_info` is inherently racy, leaving a Use-After-Free vulnerability unresolved.
- [High] If `max_num_clients` is specified and the peer NTB device has 0 memory windows, `ntb_transport_probe()` will crash with a division by zero.
- [High] `ntb_transport_free()` manually frees `nt->qp_vec` before the `devm`-managed MSI IRQ handler is unregistered, leading to a severe Use-After-Free.
--

commit 53bf2d568bcd99aba1ed7af64d8882df388ff574
Author: Koichiro Den <den@valinux.co.jp>
Subject: NTB: ntb_transport: Clear QP pointers when freeing an MW

The commit updates ntb_free_mw() to clear rx_buff and remote_rx_info pointers
for all QPs using a memory window when it is freed. This is intended to
prevent a use-after-free scenario where link-down reset routines write to
freed memory through stale remote_rx_info pointers.

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index 944d38c7efd3f..763e4bb175a3a 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -764,10 +764,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;

[Severity: High]
Does this code race with asynchronous DMA callbacks?
If ntb_rx_copy_callback() executes concurrently for a completed or aborted
DMA transfer, it appears to unconditionally use rx_buff without a null
check. Could this lead to a null pointer dereference in ntb_memcpy_rx() when
it calculates the offset from this newly cleared pointer?

> +		WRITE_ONCE(nt->qp_vec[i].remote_rx_info, NULL);

[Severity: High]
This isn't a bug introduced by this patch, but does nullifying this
pointer leave a Time-of-Check to Time-of-Use race unresolved?

If a concurrent thread in ntb_transport_tx_free_entry() locklessly
reads remote_rx_info and checks it for null, could this thread be
preempted, allowing ntb_free_mw() to nullify the pointer and free the
memory?

Would the preempted thread then dereference the freed pointer
when it resumes?

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

[Severity: High]
This is a pre-existing issue, but is the memory window DMA buffer freed
here without stopping active RX DMA engine transactions?

If the link goes down and ntb_transport_link_cleanup() calls ntb_free_mw(),
the active DMA engine is neither paused nor terminated. Could this
allow the hardware to continue reading from or writing to the physical
pages after they've been returned to the system allocator?

[ ... ]

Looking at ntb_transport_probe():

> 	if (max_num_clients && max_num_clients < qp_count)
> 		qp_count = max_num_clients;
> 	else if (nt->mw_count < qp_count)
> 		qp_count = nt->mw_count;

[Severity: High]
This is a pre-existing issue, but if the peer NTB device provides 0 memory
windows (so nt->mw_count is 0), and max_num_clients is specified, does
this override qp_count and bypass the safety check against nt->mw_count?

Since ntb_transport_init_queue() uses the QP_TO_MW macro:

> #define QP_TO_MW(nt, qp)	((qp) % nt->mw_count)

Could this result in a deterministic division by zero during module probe?

Looking at ntb_transport_free():

> 	kfree(nt->qp_vec);
> 	kfree(nt->mw_vec);
> 	kfree(nt);

[Severity: High]
This is a pre-existing issue, but does ntb_transport_free() manually free
nt->qp_vec before the devm-managed MSI IRQ handler is unregistered?

Since ntb_transport_setup_qp_msi() allocates the IRQ using a devm-managed
function tied to the parent device:

> 		qp->msi_irq = ntbm_msi_request_irq(qp->ndev, ntb_transport_isr,
> 						   KBUILD_MODNAME, qp,
> 						   &qp->msi_desc);

Could an interrupt fire after kfree(nt->qp_vec) is executed during unbind,
causing ntb_transport_isr() to access freed memory and corrupt the tasklet
list when scheduling qp->rxc_db_work?

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

Re: [PATCH 04/11] NTB: ntb_transport: Clear QP pointers when freeing an MW

[Koichiro Den]

On Mon, Sep 07, 2026 at 02:42:57PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> 
> New issues:
> - [High] Unsynchronized nullification of `qp->rx_buff` introduces a deterministic NULL pointer dereference in the asynchronous DMA callback.

In my opinion, the underlying RX DMA lifetime issue is pre-existing, although
clearing rx_buff introduces a new NULL pointer dereference path in the RX DMA
error fallback.

I was aware of this but I thought it would require a large restructuring, so I
thought it was better to fix the issue separately when needed, otherwise this
series would blow up in size. My patch 11's commit message says:

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

which is somewhat relevant. But as I said above, the possible NULL pointer
dereference can be seen as a regression, so now I'm planning to include the fix
in v2..

Best regards,
Koichiro

> 
> Pre-existing issues:
> - [High] The memory window (MW) DMA buffer is freed without terminating active RX DMA engine transactions.
> - [High] Incomplete fix: The nullification of `qp->remote_rx_info` is inherently racy, leaving a Use-After-Free vulnerability unresolved.
> - [High] If `max_num_clients` is specified and the peer NTB device has 0 memory windows, `ntb_transport_probe()` will crash with a division by zero.
> - [High] `ntb_transport_free()` manually frees `nt->qp_vec` before the `devm`-managed MSI IRQ handler is unregistered, leading to a severe Use-After-Free.
> --
> 
> commit 53bf2d568bcd99aba1ed7af64d8882df388ff574
> Author: Koichiro Den <den@valinux.co.jp>
> Subject: NTB: ntb_transport: Clear QP pointers when freeing an MW
> 
> The commit updates ntb_free_mw() to clear rx_buff and remote_rx_info pointers
> for all QPs using a memory window when it is freed. This is intended to
> prevent a use-after-free scenario where link-down reset routines write to
> freed memory through stale remote_rx_info pointers.
> 
> > diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> > index 944d38c7efd3f..763e4bb175a3a 100644
> > --- a/drivers/ntb/ntb_transport.c
> > +++ b/drivers/ntb/ntb_transport.c
> > @@ -764,10 +764,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;
> 
> [Severity: High]
> Does this code race with asynchronous DMA callbacks?
> If ntb_rx_copy_callback() executes concurrently for a completed or aborted
> DMA transfer, it appears to unconditionally use rx_buff without a null
> check. Could this lead to a null pointer dereference in ntb_memcpy_rx() when
> it calculates the offset from this newly cleared pointer?
> 
> > +		WRITE_ONCE(nt->qp_vec[i].remote_rx_info, NULL);
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does nullifying this
> pointer leave a Time-of-Check to Time-of-Use race unresolved?
> 
> If a concurrent thread in ntb_transport_tx_free_entry() locklessly
> reads remote_rx_info and checks it for null, could this thread be
> preempted, allowing ntb_free_mw() to nullify the pointer and free the
> memory?
> 
> Would the preempted thread then dereference the freed pointer
> when it resumes?
> 
> > +	}
> > +
> >  	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);
> 
> [Severity: High]
> This is a pre-existing issue, but is the memory window DMA buffer freed
> here without stopping active RX DMA engine transactions?
> 
> If the link goes down and ntb_transport_link_cleanup() calls ntb_free_mw(),
> the active DMA engine is neither paused nor terminated. Could this
> allow the hardware to continue reading from or writing to the physical
> pages after they've been returned to the system allocator?
> 
> [ ... ]
> 
> Looking at ntb_transport_probe():
> 
> > 	if (max_num_clients && max_num_clients < qp_count)
> > 		qp_count = max_num_clients;
> > 	else if (nt->mw_count < qp_count)
> > 		qp_count = nt->mw_count;
> 
> [Severity: High]
> This is a pre-existing issue, but if the peer NTB device provides 0 memory
> windows (so nt->mw_count is 0), and max_num_clients is specified, does
> this override qp_count and bypass the safety check against nt->mw_count?
> 
> Since ntb_transport_init_queue() uses the QP_TO_MW macro:
> 
> > #define QP_TO_MW(nt, qp)	((qp) % nt->mw_count)
> 
> Could this result in a deterministic division by zero during module probe?
> 
> Looking at ntb_transport_free():
> 
> > 	kfree(nt->qp_vec);
> > 	kfree(nt->mw_vec);
> > 	kfree(nt);
> 
> [Severity: High]
> This is a pre-existing issue, but does ntb_transport_free() manually free
> nt->qp_vec before the devm-managed MSI IRQ handler is unregistered?
> 
> Since ntb_transport_setup_qp_msi() allocates the IRQ using a devm-managed
> function tied to the parent device:
> 
> > 		qp->msi_irq = ntbm_msi_request_irq(qp->ndev, ntb_transport_isr,
> > 						   KBUILD_MODNAME, qp,
> > 						   &qp->msi_desc);
> 
> Could an interrupt fire after kfree(nt->qp_vec) is executed during unbind,
> causing ntb_transport_isr() to access freed memory and corrupt the tasklet
> list when scheduling qp->rxc_db_work?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260907142429.951930-1-den@valinux.co.jp?part=4

[PATCH 05/11] 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.")
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
 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 763e4bb175a3..c8e7fc774b4f 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -973,6 +973,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;
 
@@ -985,9 +994,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);
 
@@ -1005,7 +1011,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 05/11] NTB: ntb_transport: Avoid deadlock when cancelling link work

[sashiko-bot]

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

Pre-existing issues:
- [High] Race condition in ntb_transport_free() leading to use-after-free if a link event fires during removal.
--

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

NTB: ntb_transport: Avoid deadlock when cancelling link work

This commit prevents a deadlock in ntb_transport_link_cleanup by
conditionally cancelling link work outside of the link_event_lock.

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -973,6 +973,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]
This isn't a bug introduced by this patch, but while reviewing this work
cancellation, I noticed a potential race condition in ntb_transport_free()
that could lead to a use-after-free.

In ntb_transport_free(), the background works are cancelled, but this happens
before the hardware is stopped from generating link events via
ntb_link_disable() and ntb_clear_ctx():

	ntb_transport_link_cleanup(nt);
	cancel_work_sync(&nt->link_cleanup);
	cancel_delayed_work_sync(&nt->link_work);

	qp_bitmap_alloc = nt->qp_bitmap & ~nt->qp_bitmap_free;

	/* verify that all the qp's are freed */
	for (i = 0; i < nt->qp_count; i++) {
		qp = &nt->qp_vec[i];
		if (qp_bitmap_alloc & BIT_ULL(i))
			ntb_transport_free_queue(qp);
	}

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

	kfree(nt->qp_vec);
	kfree(nt->mw_vec);
	kfree(nt);

If a hardware interrupt triggers a link event during this window, can the
callback ntb_transport_event_callback() reschedule the just-cancelled works?

If so, would ntb_transport_free() then proceed to free the nt context via
kfree(nt), leaving the requeued work to execute against freed memory?

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

[PATCH 06/11] 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")
Link: https://lore.kernel.org/r/anyKbq3mpLG4y7rb@SMW015318
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
 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 c8e7fc774b4f..411017873a83 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -935,6 +935,16 @@ static void ntb_qp_link_down_reset(struct ntb_transport_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;
@@ -954,13 +964,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)
@@ -1102,16 +1109,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;
@@ -1159,9 +1169,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,
@@ -2405,8 +2416,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 06/11] NTB: ntb_transport: Publish link state after QP setup

[sashiko-bot]

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

New issues:
- [High] Store-Load memory ordering race between `nt->link_is_up` and `qp->client_ready` allows both threads to miss link scheduling, potentially deadlocking link establishment.

Pre-existing issues:
- [High] Workqueue teardown race leading to Use-After-Free during `ntb_transport_free()`.
- [High] Missing minimum size validation for memory windows causes an integer underflow when calculating tx_size and rx_size, leading to massive out-of-bounds MMIO writes and kernel heap memory corruption.
--

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

NTB: ntb_transport: Publish link state after QP setup

This commit delays publishing the transport link state until all QPs' MW and
peer MSI states are set up. It uses a release store for link state publication
and acquire loads before queuing QP link work to avoid premature execution.

Link: https://lore.kernel.org/r/anyKbq3mpLG4y7rb@SMW015318

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index c8e7fc774b4fd..411017873a83b 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c

[ ... ]

> @@ -1102,16 +1109,19 @@ static void ntb_transport_link_work(struct work_struct *work)
>  			goto out1;
>  	}
>  
>  	for (i = 0; i < nt->qp_count; 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);

[Severity: High]
Does this code have a Store-Load memory ordering race that could lead to a
deadlock?

In this thread, ntb_transport_link_work() performs a release store to
nt->link_is_up, followed by a plain load of qp->client_ready.

Meanwhile, ntb_transport_link_up() performs a plain store to qp->client_ready,
followed by an acquire load of nt->link_is_up via
ntb_transport_schedule_qp_link():

drivers/ntb/ntb_transport.c:ntb_transport_link_up() {
    ...
    qp->client_ready = true;
    ntb_transport_schedule_qp_link(qp, 0);
    ...
}

Since neither smp_store_release() nor smp_load_acquire() provides a full
memory barrier (Store-Load ordering), is it possible for both threads to delay
their stores in the CPU store buffer? If this happens, both threads could read
false and miss the wakeup, leaving the link permanently inactive.

[ ... ]

[Severity: High]
This is a pre-existing issue, but is there a use-after-free race during
workqueue teardown?

Looking at ntb_transport_free(), the delayed work is canceled before the
hardware events are neutralized:

drivers/ntb/ntb_transport.c:ntb_transport_free() {
    ...
    ntb_transport_link_cleanup(nt);
    cancel_work_sync(&nt->link_cleanup);
    cancel_delayed_work_sync(&nt->link_work);

    qp_bitmap_alloc = nt->qp_bitmap & ~nt->qp_bitmap_free;

    /* verify that all the qp's are freed */
    for (i = 0; i < nt->qp_count; i++) {
        qp = &nt->qp_vec[i];
        if (qp_bitmap_alloc & BIT_ULL(i))
            ntb_transport_free_queue(qp);
    }

    ntb_link_disable(ndev);
    ntb_clear_ctx(ndev);
    ...
}

If a hardware link event fires between cancel_delayed_work_sync() and
ntb_clear_ctx(), the interrupt handler could requeue the work item. After
ntb_transport_free() finishes freeing the context, couldn't the requeued work
execute and dereference the freed memory?

[Severity: High]
This is a pre-existing issue, but does this code lack minimum size validation
for memory windows, leading to a potential integer underflow?

In ntb_transport_init_queue(), the tx_size calculation unconditionally subtracts
the size of the struct:

drivers/ntb/ntb_transport.c:ntb_transport_init_queue() {
    ...
    tx_size -= sizeof(struct ntb_rx_info);
    ...
}

Similarly, in ntb_transport_setup_qp_mw():

drivers/ntb/ntb_transport.c:ntb_transport_setup_qp_mw() {
    ...
    rx_size -= sizeof(struct ntb_rx_info);
    ...
}

If tx_size or rx_size is less than 4 bytes, this will underflow. For tx_size,
setting the max_mw_size module parameter to a small value could trigger an
underflow. For rx_size, if a remote peer advertises a window size that
truncates to 0, could it underflow and lead to out-of-bounds MMIO writes
or memory corruption?

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

Re: [PATCH 06/11] NTB: ntb_transport: Publish link state after QP setup

[Koichiro Den]

On Mon, Sep 07, 2026 at 02:47:00PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> New issues:
> - [High] Store-Load memory ordering race between `nt->link_is_up` and `qp->client_ready` allows both threads to miss link scheduling, potentially deadlocking link establishment.
> 
> Pre-existing issues:
> - [High] Workqueue teardown race leading to Use-After-Free during `ntb_transport_free()`.
> - [High] Missing minimum size validation for memory windows causes an integer underflow when calculating tx_size and rx_size, leading to massive out-of-bounds MMIO writes and kernel heap memory corruption.
> --
> 
> commit 07de6dd89d3b67c6e867804f1d8d493ce3b736ff
> Author: Koichiro Den <den@valinux.co.jp>
> 
> NTB: ntb_transport: Publish link state after QP setup
> 
> This commit delays publishing the transport link state until all QPs' MW and
> peer MSI states are set up. It uses a release store for link state publication
> and acquire loads before queuing QP link work to avoid premature execution.
> 
> Link: https://lore.kernel.org/r/anyKbq3mpLG4y7rb@SMW015318
> 
> > diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> > index c8e7fc774b4fd..411017873a83b 100644
> > --- a/drivers/ntb/ntb_transport.c
> > +++ b/drivers/ntb/ntb_transport.c
> 
> [ ... ]
> 
> > @@ -1102,16 +1109,19 @@ static void ntb_transport_link_work(struct work_struct *work)
> >  			goto out1;
> >  	}
> >  
> >  	for (i = 0; i < nt->qp_count; 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);
> 
> [Severity: High]
> Does this code have a Store-Load memory ordering race that could lead to a
> deadlock?
> 
> In this thread, ntb_transport_link_work() performs a release store to
> nt->link_is_up, followed by a plain load of qp->client_ready.
> 
> Meanwhile, ntb_transport_link_up() performs a plain store to qp->client_ready,
> followed by an acquire load of nt->link_is_up via
> ntb_transport_schedule_qp_link():
> 
> drivers/ntb/ntb_transport.c:ntb_transport_link_up() {
>     ...
>     qp->client_ready = true;
>     ntb_transport_schedule_qp_link(qp, 0);
>     ...
> }
> 
> Since neither smp_store_release() nor smp_load_acquire() provides a full
> memory barrier (Store-Load ordering), is it possible for both threads to delay
> their stores in the CPU store buffer? If this happens, both threads could read
> false and miss the wakeup, leaving the link permanently inactive.

This store-buffering pattern does seem possible (with ntb_netdev), although the
race window is very narrow. A small patch can make this robust, so let me add
one patch in v2.

Best regards,
Koichiro

> 
> [ ... ]
> 
> [Severity: High]
> This is a pre-existing issue, but is there a use-after-free race during
> workqueue teardown?
> 
> Looking at ntb_transport_free(), the delayed work is canceled before the
> hardware events are neutralized:
> 
> drivers/ntb/ntb_transport.c:ntb_transport_free() {
>     ...
>     ntb_transport_link_cleanup(nt);
>     cancel_work_sync(&nt->link_cleanup);
>     cancel_delayed_work_sync(&nt->link_work);
> 
>     qp_bitmap_alloc = nt->qp_bitmap & ~nt->qp_bitmap_free;
> 
>     /* verify that all the qp's are freed */
>     for (i = 0; i < nt->qp_count; i++) {
>         qp = &nt->qp_vec[i];
>         if (qp_bitmap_alloc & BIT_ULL(i))
>             ntb_transport_free_queue(qp);
>     }
> 
>     ntb_link_disable(ndev);
>     ntb_clear_ctx(ndev);
>     ...
> }
> 
> If a hardware link event fires between cancel_delayed_work_sync() and
> ntb_clear_ctx(), the interrupt handler could requeue the work item. After
> ntb_transport_free() finishes freeing the context, couldn't the requeued work
> execute and dereference the freed memory?
> 
> [Severity: High]
> This is a pre-existing issue, but does this code lack minimum size validation
> for memory windows, leading to a potential integer underflow?
> 
> In ntb_transport_init_queue(), the tx_size calculation unconditionally subtracts
> the size of the struct:
> 
> drivers/ntb/ntb_transport.c:ntb_transport_init_queue() {
>     ...
>     tx_size -= sizeof(struct ntb_rx_info);
>     ...
> }
> 
> Similarly, in ntb_transport_setup_qp_mw():
> 
> drivers/ntb/ntb_transport.c:ntb_transport_setup_qp_mw() {
>     ...
>     rx_size -= sizeof(struct ntb_rx_info);
>     ...
> }
> 
> If tx_size or rx_size is less than 4 bytes, this will underflow. For tx_size,
> setting the max_mw_size module parameter to a small value could trigger an
> underflow. For rx_size, if a remote peer advertises a window size that
> truncates to 0, could it underflow and lead to out-of-bounds MMIO writes
> or memory corruption?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260907142429.951930-1-den@valinux.co.jp?part=6

[PATCH 07/11] 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")
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
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 411017873a83..4c0a29693ac2 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -990,6 +990,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 */
@@ -1149,7 +1151,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

Re: [PATCH 07/11] NTB: ntb_transport: Clear link state before QP cleanup

[sashiko-bot]

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

New issues:
- [Medium] Coding Pattern Violation: Mixing guard() and goto-based cleanup

Pre-existing issues:
- [High] Memory Ordering Race leading to lost wakeups
- [High] Data Race in ntb_qp_link_cleanup allowing concurrent execution
- [High] DMA Memory Corruption due to unterminated transactions
- [High] Missing read memory barrier (dma_rmb) when reading payload after checking DESC_DONE_FLAG
- [High] Stale Data Transmission due to missing flush on link down
--

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

    NTB: ntb_transport: Clear link state before QP cleanup

    Cleanup leaves the transport link marked up after releasing its MWs, allowing
    client link-up requests to start QP link work prematurely. Clear link_is_up
    before cancelling QP work and releasing MWs. Additionally, ensure 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 411017873a83b..4c0a29693ac23 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -990,6 +990,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 */

[Severity: High]
This is a pre-existing issue, but does this cleanup routine allow concurrent
execution?

Looking at ntb_transport_link_cleanup(), it calls ntb_qp_link_cleanup() on all
active QPs, and then synchronizes cancellation using cancel_work_sync():

drivers/ntb/ntb_transport.c:ntb_transport_link_cleanup() {
    ...
    for (i = 0; i < nt->qp_count; i++)
        if (nt->qp_bitmap_alloc & BIT_ULL(i)) {
            struct ntb_transport_qp *qp = &nt->qp_vec[i];
            ntb_qp_link_cleanup(qp);
            cancel_work_sync(&qp->link_cleanup);
        }
    ...
}

If the ntb_qp_link_cleanup_work() worker is concurrently executing, it will
also invoke ntb_qp_link_cleanup(). Since no lock synchronizes
ntb_qp_link_cleanup(), it can execute concurrently across threads. This could
lead to data races when resetting QP indices and cause duplicate concurrent
calls to the client's event_handler, potentially corrupting the client driver
state.

[Severity: High]
This is a pre-existing issue, but is it possible for active DMA transactions to
corrupt freed memory here?

Later in ntb_transport_link_cleanup(), it frees the backing DMA memory:

drivers/ntb/ntb_transport.c:ntb_transport_link_cleanup() {
    ...
    for (i = 0; i < nt->mw_count; i++)
        ntb_free_mw(nt, i);
    ...
}

ntb_free_mw() calls dma_free_attrs() without ensuring the DMA channel is
stopped. Prior to this, ntb_qp_link_cleanup() kills the RX tasklet with
tasklet_kill(&qp->rxc_db_work), but does not terminate active DMA engine
transactions (e.g., via dmaengine_terminate_all()).

If a DMA transfer was previously submitted, could it continue asynchronously in
hardware, writing into the freed memory pages and leading to IOMMU faults or
memory corruption?

[Severity: High]
This is a pre-existing issue, but does resetting the link context leave stale
data in the transmit queues?

When the link goes down, ntb_qp_link_down_reset() is called:

drivers/ntb/ntb_transport.c:ntb_qp_link_down_reset() {
    ...
    ntb_qp_link_context_reset(qp);
    remote_rx_info = READ_ONCE(qp->remote_rx_info);
    ...
}

The QP context and indices (like qp->tx_index) are reset to 0, but pending
entries in qp->tx_offl_q or other transmit queues are not flushed. These
pending entries already have their entry->tx_index populated with old index
values from before the link dropped.

When the tx_offload_thread processes them, could it write the data to the
old, stale index locations instead of starting correctly at index 0, causing
the remote receiver to hang or receive out-of-sequence packets?

[ ... ]

[Severity: Medium]
Does ntb_transport_link_work() mix scope-based guard macros with legacy
goto-based error cleanup?

The cleanup.md documentation explicitly mandates that usage of "goto" and
cleanup helpers must never be mixed in the same function.

drivers/ntb/ntb_transport.c:ntb_transport_link_work() {
    ...
    guard(mutex)(&nt->link_event_lock);
    ...
        rc = ntb_set_mw(nt, i, val64);
        if (rc)
            goto out1;
    ...
out1:
    for (i = 0; i < nt->mw_count; i++)
        ntb_free_mw(nt, i);
    ...
}

This function uses guard(mutex)(&nt->link_event_lock) at the beginning, but
continues to use goto out1; and goto out; for error handling and legacy cleanup
paths. Does this violate the established architectural cleanup pattern?

[ ... ]

> @@ -1149,7 +1151,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 is a pre-existing issue, but does this Dekker's algorithm pattern lack a
full memory barrier, potentially leading to lost wakeups?

In ntb_transport_link_work(), nt->link_is_up is stored and qp->client_ready is
read:

drivers/ntb/ntb_transport.c:ntb_transport_link_work() {
    ...
    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)
            ntb_transport_schedule_qp_link(qp, 0);
    }
    ...
}

Concurrently, ntb_transport_link_up() stores qp->client_ready and reads
nt->link_is_up:

drivers/ntb/ntb_transport.c:ntb_transport_link_up() {
    ...
    qp->client_ready = true;
    ntb_transport_schedule_qp_link(qp, 0);
    ...
}

Inside ntb_transport_schedule_qp_link(), it checks
smp_load_acquire(&nt->link_is_up). Because smp_store_release() allows
subsequent loads to be reordered before the store, and smp_load_acquire()
allows preceding stores to be reordered after the load, both threads can read
false for the counterpart flag.

Could this result in a lost wakeup where the QP link fails to initialize?

[ ... ]

[Severity: High]
This is a pre-existing issue, but is a read memory barrier required after
checking the DESC_DONE_FLAG?

In ntb_process_rxc(), the descriptor's done flag is read from the DMA-mapped
memory to verify packet completion:

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;
    }
    ...
}

However, there is no dma_rmb() issued after observing the flag. Could this
allow the CPU to speculatively read the subsequent header fields (e.g.,
hdr->ver, hdr->len) and the payload buffer before the done flag read
completes, resulting in stale data being processed?

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

Re: [PATCH 07/11] NTB: ntb_transport: Clear link state before QP cleanup

[Koichiro Den]

On Mon, Sep 07, 2026 at 02:43:02PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
> 
> New issues:
> - [Medium] Coding Pattern Violation: Mixing guard() and goto-based cleanup

I think this is a pre-existing issue, ie. this patch does not introduce any
regression around this. Correct me if I'm wrong.

Best regards,
Koichiro

> 
> Pre-existing issues:
> - [High] Memory Ordering Race leading to lost wakeups
> - [High] Data Race in ntb_qp_link_cleanup allowing concurrent execution
> - [High] DMA Memory Corruption due to unterminated transactions
> - [High] Missing read memory barrier (dma_rmb) when reading payload after checking DESC_DONE_FLAG
> - [High] Stale Data Transmission due to missing flush on link down
> --
> 
> commit 3aa5fe6a4883f5e1056224f4eb903786b4edfe5a
> Author: Koichiro Den <den@valinux.co.jp>
> 
>     NTB: ntb_transport: Clear link state before QP cleanup
> 
>     Cleanup leaves the transport link marked up after releasing its MWs, allowing
>     client link-up requests to start QP link work prematurely. Clear link_is_up
>     before cancelling QP work and releasing MWs. Additionally, ensure 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 411017873a83b..4c0a29693ac23 100644
> > --- a/drivers/ntb/ntb_transport.c
> > +++ b/drivers/ntb/ntb_transport.c
> > @@ -990,6 +990,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 */
> 
> [Severity: High]
> This is a pre-existing issue, but does this cleanup routine allow concurrent
> execution?
> 
> Looking at ntb_transport_link_cleanup(), it calls ntb_qp_link_cleanup() on all
> active QPs, and then synchronizes cancellation using cancel_work_sync():
> 
> drivers/ntb/ntb_transport.c:ntb_transport_link_cleanup() {
>     ...
>     for (i = 0; i < nt->qp_count; i++)
>         if (nt->qp_bitmap_alloc & BIT_ULL(i)) {
>             struct ntb_transport_qp *qp = &nt->qp_vec[i];
>             ntb_qp_link_cleanup(qp);
>             cancel_work_sync(&qp->link_cleanup);
>         }
>     ...
> }
> 
> If the ntb_qp_link_cleanup_work() worker is concurrently executing, it will
> also invoke ntb_qp_link_cleanup(). Since no lock synchronizes
> ntb_qp_link_cleanup(), it can execute concurrently across threads. This could
> lead to data races when resetting QP indices and cause duplicate concurrent
> calls to the client's event_handler, potentially corrupting the client driver
> state.
> 
> [Severity: High]
> This is a pre-existing issue, but is it possible for active DMA transactions to
> corrupt freed memory here?
> 
> Later in ntb_transport_link_cleanup(), it frees the backing DMA memory:
> 
> drivers/ntb/ntb_transport.c:ntb_transport_link_cleanup() {
>     ...
>     for (i = 0; i < nt->mw_count; i++)
>         ntb_free_mw(nt, i);
>     ...
> }
> 
> ntb_free_mw() calls dma_free_attrs() without ensuring the DMA channel is
> stopped. Prior to this, ntb_qp_link_cleanup() kills the RX tasklet with
> tasklet_kill(&qp->rxc_db_work), but does not terminate active DMA engine
> transactions (e.g., via dmaengine_terminate_all()).
> 
> If a DMA transfer was previously submitted, could it continue asynchronously in
> hardware, writing into the freed memory pages and leading to IOMMU faults or
> memory corruption?
> 
> [Severity: High]
> This is a pre-existing issue, but does resetting the link context leave stale
> data in the transmit queues?
> 
> When the link goes down, ntb_qp_link_down_reset() is called:
> 
> drivers/ntb/ntb_transport.c:ntb_qp_link_down_reset() {
>     ...
>     ntb_qp_link_context_reset(qp);
>     remote_rx_info = READ_ONCE(qp->remote_rx_info);
>     ...
> }
> 
> The QP context and indices (like qp->tx_index) are reset to 0, but pending
> entries in qp->tx_offl_q or other transmit queues are not flushed. These
> pending entries already have their entry->tx_index populated with old index
> values from before the link dropped.
> 
> When the tx_offload_thread processes them, could it write the data to the
> old, stale index locations instead of starting correctly at index 0, causing
> the remote receiver to hang or receive out-of-sequence packets?
> 
> [ ... ]
> 
> [Severity: Medium]
> Does ntb_transport_link_work() mix scope-based guard macros with legacy
> goto-based error cleanup?
> 
> The cleanup.md documentation explicitly mandates that usage of "goto" and
> cleanup helpers must never be mixed in the same function.
> 
> drivers/ntb/ntb_transport.c:ntb_transport_link_work() {
>     ...
>     guard(mutex)(&nt->link_event_lock);
>     ...
>         rc = ntb_set_mw(nt, i, val64);
>         if (rc)
>             goto out1;
>     ...
> out1:
>     for (i = 0; i < nt->mw_count; i++)
>         ntb_free_mw(nt, i);
>     ...
> }
> 
> This function uses guard(mutex)(&nt->link_event_lock) at the beginning, but
> continues to use goto out1; and goto out; for error handling and legacy cleanup
> paths. Does this violate the established architectural cleanup pattern?
> 
> [ ... ]
> 
> > @@ -1149,7 +1151,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 is a pre-existing issue, but does this Dekker's algorithm pattern lack a
> full memory barrier, potentially leading to lost wakeups?
> 
> In ntb_transport_link_work(), nt->link_is_up is stored and qp->client_ready is
> read:
> 
> drivers/ntb/ntb_transport.c:ntb_transport_link_work() {
>     ...
>     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)
>             ntb_transport_schedule_qp_link(qp, 0);
>     }
>     ...
> }
> 
> Concurrently, ntb_transport_link_up() stores qp->client_ready and reads
> nt->link_is_up:
> 
> drivers/ntb/ntb_transport.c:ntb_transport_link_up() {
>     ...
>     qp->client_ready = true;
>     ntb_transport_schedule_qp_link(qp, 0);
>     ...
> }
> 
> Inside ntb_transport_schedule_qp_link(), it checks
> smp_load_acquire(&nt->link_is_up). Because smp_store_release() allows
> subsequent loads to be reordered before the store, and smp_load_acquire()
> allows preceding stores to be reordered after the load, both threads can read
> false for the counterpart flag.
> 
> Could this result in a lost wakeup where the QP link fails to initialize?
> 
> [ ... ]
> 
> [Severity: High]
> This is a pre-existing issue, but is a read memory barrier required after
> checking the DESC_DONE_FLAG?
> 
> In ntb_process_rxc(), the descriptor's done flag is read from the DMA-mapped
> memory to verify packet completion:
> 
> 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;
>     }
>     ...
> }
> 
> However, there is no dma_rmb() issued after observing the flag. Could this
> allow the CPU to speculatively read the subsequent header fields (e.g.,
> hdr->ver, hdr->len) and the payload buffer before the done flag read
> completes, resulting in stale data being processed?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260907142429.951930-1-den@valinux.co.jp?part=7

[PATCH 08/11] 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")
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
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 4c0a29693ac2..a902fce4ba60 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -1112,7 +1112,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

Re: [PATCH 08/11] NTB: ntb_transport: Abort link setup on QP MW allocation failure

[sashiko-bot]

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

Pre-existing issues:
- [High] A malicious remote peer can cause an out-of-bounds read from the DMA memory window, leaking kernel memory to the local client.
- [High] Concurrent execution of `ntb_transport_tx_enqueue` and link teardown results in a Use-After-Free due to a TOCTOU race on `qp->remote_rx_info`.
--

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

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

This commit fixes an issue in ntb_transport_link_work() where a failure in
ntb_transport_setup_qp_mw() is ignored. It updates the error path to abort
link setup and properly release the memory windows if an allocation fails.

Link: https://lore.kernel.org/r/20260810165136.2292436-2-den@valinux.co.jp/

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index 4c0a29693ac23..a902fce4ba602 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but could this allow a malicious
remote peer to cause an out-of-bounds read from the memory window?

In ntb_process_rxc(), hdr->len is read directly from the memory window
(which is writable by the untrusted peer). If the client's buffer size
(entry->len) is larger than the locally negotiated frame size
(qp->rx_max_frame), a malicious peer could specify a payload length up to
entry->len:

drivers/ntb/ntb_transport.c:ntb_process_rxc() {
...
	if (hdr->len > entry->len) {
...
	} else {
		qp->rx_bytes += hdr->len;
		qp->rx_pkts++;
		entry->len = hdr->len;
		ntb_async_rx(entry, offset);
	}
...
}

Should hdr->len also be validated against the underlying frame buffer bounds
before initiating the memory copy?

[ ... ]

[Severity: High]
This is a pre-existing issue, but does this code have a time-of-check to
time-of-use race condition that could lead to a use-after-free?

ntb_transport_tx_free_entry() is invoked by clients without a lock that
serializes against ntb transport link cleanup:

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

	if (!remote_rx_info)
		return 0;

	tail = remote_rx_info->entry;
...
}

If the link goes down concurrently, the asynchronous workqueue executes
ntb_transport_link_cleanup(), which calls ntb_free_mw() and frees the
underlying memory window while setting the pointer to NULL. Since there is
no synchronization between the READ_ONCE check and the dereference of
remote_rx_info->entry, could the memory be freed in that tiny window and
result in a use-after-free read?

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

[PATCH 09/11] 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")
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
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 a902fce4ba60..d009bb5bb54b 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -1251,6 +1251,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);
@@ -2164,6 +2166,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);
 
@@ -2209,6 +2214,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) {
@@ -2256,8 +2265,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

Re: [PATCH 09/11] 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:

Pre-existing issues:
- [High] ntb_transport_free() cancels work items before clearing context, which can cause a use-after-free if the hardware triggers a link event during shutdown.
- [High] Data race between `ntb_transport_link_cleanup` and `ntb_transport_free_queue` causes invocation of a NULL or freed event_handler.
- [High] Using the deprecated `dmaengine_terminate_all()` fails to wait for running DMA callbacks, causing a race with list draining and freeing.
- [Medium] Unallocated queues leak rx_free_q memory entries when the module is unloaded.
- [Medium] Memory leak of TX queue entries if DMA transfers are aborted during teardown.
--

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

NTB: ntb_transport: Stop QP work before freeing a queue

This commit cancels the link cleanup and delayed link work items before freeing
a queue pair (QP) to prevent use-after-free issues. It transitions to using
disable_work_sync() and enable_work() to stop workers instead of just
canceling them.

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> index a902fce4ba602..d009bb5bb54bb 100644
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
[ ... ]
> @@ -2209,6 +2214,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) {
[ ... ]
> @@ -2256,8 +2265,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 there appears to be a data race when clearing
these handler pointers in ntb_transport_free_queue():

    qp->cb_data = NULL;
    qp->rx_handler = NULL;
    qp->tx_handler = NULL;
    qp->event_handler = NULL;

These pointers are cleared without holding any locks. If the physical link
goes down at the same time, the workqueue executes ntb_qp_link_cleanup(),
which checks qp->event_handler. Since ntb_transport_free_queue() clears
its allocation bit at the very end of the function, could the cleanup worker
race and call the event handler with a NULL pointer, or use qp->cb_data as
context after the client has freed the backing memory?

[Severity: Medium]
This is a pre-existing issue, but I noticed a potential leak of TX queue
entries in ntb_transport_free_queue() during DMA teardown:

    /* Try to be nice and wait for any queued DMA engine
     * transactions to process before smashing it with a rock
     */
    dma_sync_wait(chan, qp->last_cookie);
    dmaengine_terminate_all(chan);

If dma_sync_wait() times out or fails, dmaengine_terminate_all() will abort
pending transfers without invoking their completion callbacks. Since the TX
entries aren't tracked on a pending list like RX (where rx_post_q is properly
tracked and freed), will these bypassed entries be permanently leaked?

[Severity: High]
This is a pre-existing issue, but also in ntb_transport_free_queue() regarding
the DMA teardown:

    dma_sync_wait(chan, qp->last_cookie);
    dmaengine_terminate_all(chan);
    dma_release_channel(chan);

Using the deprecated dmaengine_terminate_all() fails to wait for running DMA
callbacks on other CPUs. Since a callback like ntb_rx_copy_callback() could
be running concurrently (e.g. in a tasklet) and accessing entries while
ntb_transport_free_queue() proceeds to drain and free the rx_post_q and
rx_free_q lists, could this lead to a use-after-free or list corruption?
Should we be using dmaengine_terminate_sync() instead to guarantee callbacks
have finished?

[Severity: High]
This is a pre-existing issue, but I noticed something in ntb_transport_free()
that might cause a use-after-free:

drivers/ntb/ntb_transport.c:ntb_transport_free() {
    ...
    cancel_work_sync(&nt->link_cleanup);
    cancel_delayed_work_sync(&nt->link_work);
    ...
    ntb_link_disable(ndev);
    ntb_clear_ctx(ndev);
    ...
}

If ntb_link_disable() triggers a hardware link down interrupt, and
ntb_clear_ctx() hasn't been called yet to unregister callbacks, the ISR
could invoke ntb_transport_event_callback(), which reschedules
nt->link_cleanup:

        schedule_work(&nt->link_cleanup);

Since nt is freed shortly after with kfree(nt), wouldn't the scheduled work
item remain on the workqueue and dereference the freed memory when executed?

[Severity: Medium]
This is a pre-existing issue, but there appears to be a memory leak related
to unallocated QPs during module unload.

When the link comes up, ntb_transport_setup_qp_mw() allocates entries for all
QPs up to nt->qp_count, regardless of active client allocations:

        for (i = qp->rx_alloc_entry; i < qp->rx_max_entry; i++) {
                entry = kzalloc_node(sizeof(*entry), GFP_KERNEL, node);
                ...
                ntb_list_add(&qp->ntb_rx_q_lock, &entry->entry,
                             &qp->rx_free_q);
                qp->rx_alloc_entry++;
        }

However, during teardown, ntb_transport_free() only explicitly frees
allocated QPs:

        /* verify that all the qp's are freed */
        for (i = 0; i < nt->qp_count; i++) {
                qp = &nt->qp_vec[i];
                if (qp_bitmap_alloc & BIT_ULL(i))
                        ntb_transport_free_queue(qp);
        }

By deliberately skipping unallocated QPs during cleanup, won't their
populated rx_free_q lists be completely leaked when the driver executes
kfree(nt->qp_vec)?

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

[PATCH 10/11] 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.")
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
 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 d009bb5bb54b..0257f3eeb5cd 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);
@@ -652,11 +654,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;
 }
@@ -905,7 +922,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;
@@ -1168,13 +1185,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));
@@ -1202,6 +1218,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)
@@ -1738,8 +1755,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));
@@ -1750,8 +1766,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);
 	}
 }
 
@@ -2218,7 +2233,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);
@@ -2260,11 +2279,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;
@@ -2359,8 +2373,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;
 }
@@ -2560,8 +2573,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 11/11] 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()")
Signed-off-by: Koichiro Den <den@valinux.co.jp>
---
 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 0257f3eeb5cd..dce7b1311406 100644
--- a/drivers/ntb/ntb_transport.c
+++ b/drivers/ntb/ntb_transport.c
@@ -969,11 +969,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

Re: [PATCH 11/11] 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] In-flight RX DMA transfers are not drained during link cleanup before freeing Memory Window (MW) buffers, leading to DMA Use-After-Free, Read-After-Free, and potential information leaks.
--

commit 0ffb444fbfb867ed6e5c68ad186229e763417ba5
Author: Koichiro Den <den@valinux.co.jp>
NTB: ntb_transport: Drain RX tasklets during link cleanup

This commit modifies the queue pair link cleanup process to explicitly drain
and kill the receive tasklet. It temporarily disables the queue pair link
work to prevent concurrent reactivations while resetting the queue pair.

> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
> --- a/drivers/ntb/ntb_transport.c
> +++ b/drivers/ntb/ntb_transport.c
> @@ -969,11 +969,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);

[Severity: High]
This isn't a bug introduced by this patch, but while we are draining the
tasklet here, does this code still leave in-flight RX DMA transfers active
when the memory window is subsequently freed?

When a link down event occurs, ntb_transport_link_cleanup() calls
ntb_qp_link_cleanup(), which now successfully kills the rxc_db_work tasklet.
However, it does not appear to wait for or terminate pending DMA transfers on
qp->rx_dma_chan. After this function returns, ntb_transport_link_cleanup()
proceeds to call ntb_free_mw(), which returns the physical memory to the
system via dma_free_attrs().

If the hardware DMA engine is executing asynchronously, couldn't it
continue reading from or writing to this freed physical memory, potentially
causing a DMA use-after-free or data leak?

Furthermore, when the DMA asynchronous transfer completes, the
ntb_rx_copy_callback() function could be invoked:

ntb_rx_copy_callback() {
	...
	offset = qp->rx_buff + qp->rx_max_frame * qp->rx_index;
	...
}

Since ntb_free_mw() sets qp->rx_buff to NULL, could this result in a NULL
pointer dereference? Additionally, on success, ntb_complete_rxc() attempts
to write to entry->rx_hdr, which points into the now-freed memory window,
which could cause a software use-after-free.

Is it possible we also need to drain or explicitly terminate the DMA channel
transfers before resetting the link context and returning?

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

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