Re: [PATCH 08/11] NTB: perf: Reject grouped memory windows

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Mismatch in memory window indices (`perf->gidx` vs `peer->gidx`) breaks test communication on multi-port configurations.
- [High] Multiple test threads executing concurrently map memory and overwrite a shared `peer->dma_dst_addr`, leading to DMA mapping leaks, IOMMU faults, and double-unmapping.
--

--- Patch [8]: [PATCH 08/11] NTB: perf: Reject grouped memory windows ---
commit 8a37bed871c2fb90c1fc72578c46b05070e8ad53
Author: Koichiro Den <den@valinux.co.jp>

NTB: perf: Reject grouped memory windows

NTB perf configures one MW per peer through the per-MW translation API.
It cannot configure one member of a larger translation group.
Reject such layouts at probe.

> diff --git a/drivers/ntb/test/ntb_perf.c b/drivers/ntb/test/ntb_perf.c
> index dfd175f79e8f0..50bfbec1e251e 100644
> --- a/drivers/ntb/test/ntb_perf.c
> +++ b/drivers/ntb/test/ntb_perf.c
[ ... ]
> @@ -1469,7 +1469,19 @@ static int perf_init_peers(struct perf_ctx *perf)
>  	}
>  
>  	for (pidx = 0; pidx < perf->pcnt; pidx++) {
> -		ret = perf_setup_peer_mw(&perf->peers[pidx]);
> +		peer = &perf->peers[pidx];
> +		ret = ntb_mw_get_trans_group(perf->ntb, peer->pidx,
> +					     peer->gidx, &first, &count);
> +		if (ret)
> +			return ret;
> +		if (first != peer->gidx || count != 1) {
> +			dev_err(&perf->ntb->dev,
> +				"Peer %d MW %d is not a standalone window\n",
> +				peer->pidx, peer->gidx);
> +			return -EOPNOTSUPP;
> +		}
> +
> +		ret = perf_setup_peer_mw(peer);

[Severity: High]
This is a pre-existing issue, but does perf_setup_peer_mw() incorrectly mix
index variables on multi-port configurations?

Looking at perf_setup_peer_mw(), it maps the physical address of the
outbound window using the local port's index (perf->gidx):

	ret = ntb_peer_mw_get_addr(perf->ntb, perf->gidx, &phys_addr,
				   &peer->outbuf_size);

However, perf_setup_outbuf() later configures the outbound translation using
the peer's index (peer->gidx):

	ret = ntb_peer_mw_set_trans(perf->ntb, peer->pidx, peer->gidx,
				    peer->outbuf_xlat, peer->outbuf_size);

When perf->gidx and peer->gidx differ, this mismatch causes writes to the
mapped memory to go to an unconfigured window.

The same index mismatch occurs for inbound windows between ntb_mw_get_align()
and ntb_mw_set_trans().

Could we use a consistent index?

[Severity: High]
This is a pre-existing issue, but can multiple concurrent test threads
overwrite peer->dma_dst_addr and leak DMA allocations?

When a user configures multiple test threads, they all execute
perf_init_test() concurrently without serialization.

Because they share the same perf->test_peer structure, they each call
dma_map_resource() and overwrite peer->dma_dst_addr:

drivers/ntb/test/ntb_perf.c:perf_init_test() {
    ...
	peer->dma_dst_addr =
		dma_map_resource(pthr->dma_chan->device->dev,
				 peer->out_phys_addr, peer->outbuf_size,
				 DMA_FROM_DEVICE, 0);
    ...
}

Later, threads may read an IOVA mapped for a completely different DMA device
in perf_copy_chunk():

	dst_dma_addr = peer->dma_dst_addr + (dst_vaddr - vbase);

During teardown, all threads call dma_unmap_resource() on the exact same
pointer in perf_clear_test(), causing a double-unmap:

	dma_unmap_resource(pthr->dma_chan->device->dev,
			   pthr->perf->test_peer->dma_dst_addr,
			   pthr->perf->test_peer->outbuf_size,
			   DMA_FROM_DEVICE, 0);

Does this sequence need additional locking or per-thread DMA mapping tracking?

>  		if (ret)
>  			return ret;
>  	}

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