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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Loop variable `i` is overwritten by the `qp_count` loop, causing memory window leaks or out-of-bounds accesses during probe error teardown.
--

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

NTB: ntb_transport: Remove the device debugfs directory

This commit ensures the device debugfs directory is removed during teardown
and probe failure. This prevents -EEXIST errors and missing queue statistics
files on module rebind.

> 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--) {
>  		mw = &nt->mw_vec[i];
>  		iounmap(mw->vbase);

[Severity: High]
This is a pre-existing issue, but does this error path use the correct
value for 'i' to unmap memory windows?

Earlier in ntb_transport_probe(), 'i' is used to map up to 'mw_count'
windows:

    for (i = 0; i < mw_count; i++) {
        ...
        mw->vbase = ioremap_wc(mw->phys_addr, mw->phys_size);
    }

However, 'i' is subsequently overwritten when initializing queues:

    for (i = 0; i < qp_count; i++) {
        rc = ntb_transport_init_queue(nt, i);
        if (rc)
            goto err2;
    }

If probe fails during queue initialization or later, this cleanup loop uses
the overwritten value of 'i' (representing qp_count) rather than mw_count.

If qp_count < mw_count, the excess memory windows are leaked. If 
qp_count > mw_count (which can occur if max_num_clients bounds qp_count
larger than mw_count), this loop will read out of bounds of nt->mw_vec and
pass invalid pointers to iounmap().

Additionally, does ntb_transport_free() have a similar leak?

It uses the capped nt->mw_count to free windows:

ntb_transport_free() {
    ...
    for (i = nt->mw_count; i--; ) {
        ntb_free_mw(nt, i);
        iounmap(nt->mw_vec[i].vbase);
    }
}

This limits unmapping to nt->mw_count, which will leak any excess windows
that were mapped during probe if mw_count was originally greater than
nt->mw_count.

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