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

[sashiko-bot@kernel.org]

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

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

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

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

    NTB: ntb_transport: Stop QP work before freeing a queue

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

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

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

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

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

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

In ntb_transport_create_queue(), the cleanup code checks:

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

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

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

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

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

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

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

Further down in ntb_transport_free_queue(), the code calls:

    dmaengine_terminate_all(chan);

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

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

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

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

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

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

    qp->event_handler = NULL;

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

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

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

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