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

[sashiko-bot@kernel.org]

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