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

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