Re: [PATCH v7 06/10] PCI: endpoint: pci-epf-vntb: Allow DMA and MW to share a BAR

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] PCI: pci-epf-vntb: wrong alignment reporting in vntb_epf_mw_get_align()
- [Medium] PCI: pci-epf-vntb: unaligned MW offset in epf_ntb_dw_edma_collect()
- [Medium] PCI: pci-epf-vntb: broken BAR validation in epf_ntb_dw_edma_collect()
--

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

PCI: endpoint: pci-epf-vntb: Allow DMA and MW to share a BAR

This commit allows the endpoint DMA BAR (dma_bar) to share a BAR with a
memory window by placing the DMA submap regions after the configured MW
region. It also updates MW alignment reporting and requires the host to
advertise MW/DMA boundary support during LINK_UP.

> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> index ac83af54122f7..721cd841a1dd8 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c

[ ... ]

> @@ -229,6 +230,22 @@ static bool epf_ntb_is_bar_used(struct epf_ntb *ntb,
>  	return false;
>  }
>  
> +static u64 epf_ntb_dma_bar_offset(struct epf_ntb *ntb, enum pci_barno barno)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < ntb->num_mws; i++)
> +		if (ntb->epf_ntb_bar[BAR_MW1 + i] == barno)
> +			return ntb->mws_size[i];
> +
> +	return 0;
> +}
> +
> +static bool epf_ntb_dma_shares_bar(struct epf_ntb *ntb, enum pci_barno barno)
> +{
> +	return ntb->dma && ntb->epf_ntb_bar[BAR_DMA] == barno;
> +}
> +
>  struct epf_ntb_dma_filter {
>  	struct device *dev;
>  	int chan_id;
> @@ -353,6 +370,7 @@ static int epf_ntb_dw_edma_collect(struct epf_ntb *ntb,
>  	unsigned int i;
>  	size_t align;
>  	u32 next = 0;
> +	u64 offset;
>  	int ret;
>  
>  	if (ctrl->u.dma_ctrl.reg_layout_data != EDMA_MF_EDMA_UNROLL)
> @@ -371,9 +389,30 @@ static int epf_ntb_dw_edma_collect(struct epf_ntb *ntb,
>  	if (!is_power_of_2(align))
>  		return -EINVAL;
>  
> -	if (epf_ntb_is_bar_used(ntb, barno) ||
> -	    pci_epc_get_next_free_bar(features, barno) != barno)
> +	if (barno == ntb->epf_ntb_bar[BAR_CONFIG] ||
> +	    barno == ntb->epf_ntb_bar[BAR_DB])
> +		return -EINVAL;
> +
> +	offset = epf_ntb_dma_bar_offset(ntb, barno);
> +	if (!offset &&
> +	    (epf_ntb_is_bar_used(ntb, barno) ||
> +	     pci_epc_get_next_free_bar(features, barno) != barno))
> +		return -EINVAL;

[Severity: Medium]
When a shared BAR is configured (offset != 0), gating
pci_epc_get_next_free_bar(features, barno) != barno under !offset skips the
EPC BAR validity check in epf_ntb_dw_edma_collect().

If both mwX_bar and dma_bar are explicitly configured to the same BAR number
via configfs, epf_ntb_find_bar() skips its loop because
ntb->epf_ntb_bar[bar] is already non-negative:

drivers/pci/endpoint/functions/pci-epf-vntb.c:epf_ntb_find_bar() {
	while (ntb->epf_ntb_bar[bar] < 0) {
		barno = pci_epc_get_next_free_bar(epc_features, barno);
		if (barno < 0)
			break; /* No more BAR available */

		if (ntb->epf_ntb_bar[BAR_DMA] != barno &&
		    !epf_ntb_is_bar_used(ntb, barno))
			ntb->epf_ntb_bar[bar] = barno;

		barno += 1;
	}
}

Because epf_ntb_mw_bar_init() also skips pci_epc_set_bar() when shared is
true, nothing verifies that barno is not BAR_RESERVED, BAR_DISABLED, or an
upper 32-bit slot of a preceding 64-bit BAR. In addition,
epf_ntb_dma_bar_offset() returns the first matching MW size without checking
whether multiple MWs were assigned to the same BAR, and epf_ntb_find_bar()
still skips ntb->epf_ntb_bar[BAR_DMA] when auto-assigning MW BARs.

Should pci_epc_get_next_free_bar(features, barno) != barno be checked
unconditionally in epf_ntb_dw_edma_collect(), and should duplicate MW BAR
assignments and auto-assignment in epf_ntb_find_bar() be handled for shared
BARs?

> +	if (offset > U32_MAX)
> +		return -EOVERFLOW;
> +	if (offset && !is_power_of_2(offset))
>  		return -EINVAL;
> +	dma->ctrl.submap.offset = offset;
> +	next = offset;
> +	if (next) {
> +		/*
> +		 * submap[0] covers the MW prefix. Keep its temporary
> +		 * target at address 0 until the MW translation is
> +		 * installed.
> +		 */
> +		dma->submap[0].size = next;
> +		dma->num_submap = 1;
> +	}

[Severity: Medium]
What happens if offset is a power of two that is smaller than align
(features->align, which is 16 KiB or 64 KiB on several DesignWare PCIe
endpoint controllers)?

In epf_ntb_dw_edma_collect(), offset is checked with is_power_of_2(offset),
but not with IS_ALIGNED(offset, align). If an MW size such as SZ_4K is
configured on an EPC with a 64 KiB region_align, dma->submap[0].size and the
starting BAR offset next for subsequent DMA submaps will not be aligned to
align, and backing_size in epf_ntb_dma_bar_init() will also be unaligned.

When epf_ntb_dma_set_bar() or vntb_epf_mw_set_trans() later calls
epf_ntb_dma_set_bar_locked() -> pci_epc_set_bar(),
dw_pcie_ep_validate_submap() rejects the submap array:

drivers/pci/controller/dwc/pcie-designware-ep.c:dw_pcie_ep_validate_submap() {
	for (i = 0; i < num_submap; i++) {
		size = submap[i].size;

		if (!size)
			return -EINVAL;

		if (!IS_ALIGNED(size, align) || !IS_ALIGNED(off, align))
			return -EINVAL;
}

Could epf_ntb_dw_edma_collect() also check !IS_ALIGNED(offset, align) before
accepting offset?

[ ... ]

> @@ -551,7 +592,7 @@ static int epf_ntb_dma_bar_init(struct epf_ntb *ntb)
>  		return -EOPNOTSUPP;
>  
>  	barno = ntb->epf_ntb_bar[BAR_DMA];
> -	mapped_size = dma->ctrl.submap.size;
> +	mapped_size = dma->ctrl.submap.offset + dma->ctrl.submap.size;
>  	/*
>  	 * Submaps cannot be installed until the host assigns the BAR address.
>  	 * Use address 0 for the temporary BAR Match Mode mapping, as is done

[ ... ]

> @@ -1936,6 +2008,16 @@ static int vntb_epf_mw_set_trans(struct ntb_dev *ndev, int pidx, int idx,
>  	dev = &ndev->dev;
>  	barno = ntb->epf_ntb_bar[BAR_MW1 + idx];
>  	epf_bar = &ntb->epf->bar[barno];
> +	if (epf_ntb_dma_shares_bar(ntb, barno)) {
> +		/* DMA submaps start after the configured MW size. */
> +		if (size != ntb->mws_size[idx])
> +			return -EINVAL;
> +
> +		guard(mutex)(&ntb->dma->lock);
> +
> +		return epf_ntb_dma_set_bar_locked(ntb, &addr);
> +	}
> +
>  	epf_bar->phys_addr = addr;
>  	epf_bar->barno = barno;
>  	epf_bar->size = size;
> @@ -2134,15 +2216,17 @@ static int vntb_epf_mw_get_align(struct ntb_dev *ndev, int pidx, int idx,
>  			resource_size_t *size_max)
>  {
>  	struct epf_ntb *ntb = ntb_ndev(ndev);
> +	enum pci_barno barno = ntb->epf_ntb_bar[BAR_MW1 + idx];
> +	u64 size = ntb->mws_size[idx];
>  
>  	if (addr_align)
>  		*addr_align = SZ_4K;
>  
>  	if (size_align)
> -		*size_align = 1;
> +		*size_align = epf_ntb_dma_shares_bar(ntb, barno) ? size : 1;
>  
>  	if (size_max)
> -		*size_max = ntb->mws_size[idx];
> +		*size_max = size;
>  
>  	return 0;
>  }

[Severity: High]
Can reporting *size_align = size while keeping *addr_align = SZ_4K in
vntb_epf_mw_get_align() cause an oversized inbound BAR mapping or an
unaligned submap physical address in vntb_epf_mw_set_trans()?

In drivers/ntb/ntb_transport.c:ntb_set_mw(), the translation size and the
allocated buffer size are rounded using different alignment values:

	rc = ntb_mw_get_align(nt->ndev, PIDX, num_mw, &xlat_align,
			      &xlat_align_size, NULL);
	if (rc)
		return rc;

	xlat_size = round_up(size, xlat_align_size);
	buff_size = round_up(size, xlat_align);

When the requested size (from the peer's MW size scratchpad or max_mw_size)
is smaller than ntb->mws_size[idx], ntb_set_mw() allocates only buff_size
(rounded to SZ_4K) but calls ntb_mw_set_trans() -> vntb_epf_mw_set_trans()
with xlat_size == ntb->mws_size[idx]. vntb_epf_mw_set_trans() then maps the
full ntb->mws_size[idx] window in dma->submap[0] starting at mw->dma_addr,
exposing memory past the end of the allocated buffer to the remote PCIe host.

Furthermore, when vntb_epf_mw_set_trans() installs addr into
dma->submap[0].phys_addr, dw_pcie_ep_validate_submap() requires
IS_ALIGNED(submap[0].phys_addr, align) where align is features->align. On
controllers where features->align > SZ_4K, buffers allocated by NTB clients
using *addr_align = SZ_4K can fail validation in pci_epc_set_bar() with
-EINVAL.

Should vntb_epf_mw_get_align() ensure *addr_align accounts for both
features->align and the shared MW size requirements?

[ ... ]

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