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

[sashiko-bot@kernel.org]

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