Re: [PATCH v8 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: endpoint: pci-epf-vntb: broken BAR state transitions in mw_fallback mode
- [High] PCI: endpoint: pci-epf-vntb: invalid MW size and alignment on shared DMA BAR
- [Medium] PCI: endpoint: pci-epf-vntb: missing align and BAR checks in dw_edma_collect()
--

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

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

This commit allows the DMA BAR in pci-epf-vntb to share a BAR with a memory
window by placing DMA subrange mappings after the configured MW prefix. It
also adds a fallback mode for older hosts that do not advertise MW/DMA
boundary support on link up, exposing the whole shared BAR as an MW.

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

[ ... ]

> @@ -371,10 +390,31 @@ 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;
> +	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]
In drivers/pci/endpoint/functions/pci-epf-vntb.c, could
epf_ntb_dw_edma_collect() accept a shared BAR configuration that later fails
at runtime or uses a reserved EPC BAR?

First, when offset is non-zero, offset is checked with is_power_of_2(offset),
but it is not checked with IS_ALIGNED(offset, align) where align is
features->align ?: 1. If the configured MW size is a power of two smaller
than features->align (for example, 4 KB on a DesignWare EPC with a 64 KB
region_align), epf_ntb_bind() succeeds because epf_ntb_dma_bar_init() sets up
the BAR with num_submap == 0. However, when epf_ntb_dma_set_bar() or
vntb_epf_mw_set_trans() later installs the submaps, dma->submap[0].size and
all subsequent submap offsets are unaligned to align, so
dw_pcie_ep_validate_submap() rejects the update:

drivers/pci/controller/dwc/pcie-designware-ep.c:dw_pcie_ep_validate_submap() {
	...
		if (!IS_ALIGNED(size, align) || !IS_ALIGNED(off, align))
			return -EINVAL;
	...
}

Second, because epf_ntb_find_bar() skips BAR numbers that were explicitly
assigned through configfs, sharing a BAR between an MW and BAR_DMA requires
configuring both in configfs. Gating pci_epc_get_next_free_bar(features,
barno) != barno behind !offset here (while epf_ntb_mw_bar_init() also skips
pci_epc_set_bar() when shared is true) means pci_epc_get_next_free_bar() is
never checked when offset != 0, allowing BAR_RESERVED, BAR_DISABLED, or the
upper half of a 64-bit BAR to bypass validation here.

Should pci_epc_get_next_free_bar(features, barno) != barno be checked
unconditionally, and should offset be checked with IS_ALIGNED(offset, align)?

[ ... ]

> @@ -607,23 +649,29 @@ static void epf_ntb_dma_bar_clear(struct epf_ntb *ntb)
>  	memset(bar, 0, sizeof(*bar));
>  }
>  
> -static int epf_ntb_dma_set_bar(struct epf_ntb *ntb)
> +static int epf_ntb_dma_set_bar_locked(struct epf_ntb *ntb, bool use_submap,
> +				      const dma_addr_t *mw_addr)
>  {
> -	struct pci_epf_bar_submap *old_submap;
>  	struct epf_ntb_dma *dma = ntb->dma;
> -	unsigned int old_num_submap;
> -	struct pci_epf_bar *bar;
> +	struct pci_epf_bar *bar, old_bar;
> +	dma_addr_t old_mw_addr;
>  	int restore, ret;
>  
> -	if (!dma)
> -		return 0;
> +	lockdep_assert_held(&dma->lock);
>  
>  	bar = &ntb->epf->bar[ntb->epf_ntb_bar[BAR_DMA]];
> -
> -	old_submap = bar->submap;
> -	old_num_submap = bar->num_submap;
> -	bar->submap = dma->submap;
> -	bar->num_submap = dma->num_submap;
> +	old_bar = *bar;
> +	old_mw_addr = dma->submap[0].phys_addr;
> +	if (use_submap) {
> +		if (mw_addr)
> +			dma->submap[0].phys_addr = *mw_addr;
> +		bar->submap = dma->submap;
> +		bar->num_submap = dma->num_submap;
> +	} else {
> +		bar->submap = NULL;
> +		bar->num_submap = 0;
> +		bar->phys_addr = mw_addr ? *mw_addr : 0;
> +	}
>  
>  	ret = pci_epc_set_bar(ntb->epf->epc, ntb->epf->func_no,
>  			      ntb->epf->vfunc_no, bar);
> @@ -631,8 +679,8 @@ static int epf_ntb_dma_set_bar(struct epf_ntb *ntb)
>  		return 0;
>  
>  	/* A failed dynamic update may have already removed the old mapping. */
> -	bar->submap = old_submap;
> -	bar->num_submap = old_num_submap;
> +	dma->submap[0].phys_addr = old_mw_addr;
> +	*bar = old_bar;
>  	restore = pci_epc_set_bar(ntb->epf->epc, ntb->epf->func_no,
>  				  ntb->epf->vfunc_no, bar);
>  	if (restore)
> @@ -642,6 +690,51 @@ static int epf_ntb_dma_set_bar(struct epf_ntb *ntb)
>  	return ret;
>  }
>  
> +static int epf_ntb_dma_set_bar(struct epf_ntb *ntb)
> +{
> +	struct epf_ntb_dma *dma = ntb->dma;
> +
> +	if (!dma)
> +		return 0;
> +
> +	guard(mutex)(&dma->lock);
> +
> +	return epf_ntb_dma_set_bar_locked(ntb, true, NULL);
> +}
> +
> +static int epf_ntb_dma_link_up(struct epf_ntb *ntb, u32 argument)
> +{
> +	struct epf_ntb_dma *dma = ntb->dma;
> +	struct pci_epf_bar *bar;
> +	bool fallback;
> +	int ret;
> +
> +	if (!dma || !dma->ctrl.submap.offset)
> +		return 0;
> +
> +	fallback = !(argument & EPF_NTB_LINK_UP_DMA_MW_SUPPORTED);
> +
> +	guard(mutex)(&dma->lock);
> +
> +	if (fallback == dma->mw_fallback)
> +		return 0;
> +
> +	if (fallback) {
> +		dev_warn(&ntb->epf->dev,
> +			 "Host does not support MW/DMA BAR sharing, exposing the whole BAR as an MW\n");
> +		/* Replace installed DMA submaps until the MW is set up. */
> +		bar = &ntb->epf->bar[ntb->epf_ntb_bar[BAR_DMA]];
> +		if (bar->num_submap) {
> +			ret = epf_ntb_dma_set_bar_locked(ntb, false, NULL);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +	dma->mw_fallback = fallback;
> +
> +	return 0;
> +}

[Severity: High]
How do epf_ntb_dma_set_bar_locked(), epf_ntb_dma_set_bar(),
epf_ntb_dma_link_up(), and vntb_epf_mw_set_trans() keep the BAR state and MW
translation address synchronized across mw_fallback transitions?

In epf_ntb_dma_set_bar_locked(), the MW translation address is stored in two
different places depending on use_submap: when use_submap is true, *mw_addr
updates dma->submap[0].phys_addr, whereas when use_submap is false, *mw_addr
updates bar->phys_addr (and resets bar->phys_addr to 0 when mw_addr is NULL).

This appears to cause several state regressions across command orderings:

1. If vntb_epf_mw_set_trans() configures the MW before an older host sends
EPF_NTB_COMMAND_LINK_UP (while dma->mw_fallback is still false), it calls
epf_ntb_dma_set_bar_locked(ntb, true, &addr), storing the MW address in
dma->submap[0].phys_addr while bar->phys_addr remains 0. When the older host
then sends EPF_NTB_COMMAND_LINK_UP without EPF_NTB_LINK_UP_DMA_MW_SUPPORTED,
epf_ntb_dma_link_up() calls:

	ret = epf_ntb_dma_set_bar_locked(ntb, false, NULL);

which sets bar->phys_addr = 0 and wipes out the already configured MW
translation.

2. In epf_ntb_dma_set_bar() (handling EPF_NTB_COMMAND_SETUP_DMA_BAR),
epf_ntb_dma_set_bar_locked(ntb, true, NULL) is called unconditionally without
checking or updating dma->mw_fallback. If dma->mw_fallback became true during
an earlier link-up and the host later re-runs EPF_NTB_COMMAND_SETUP_DMA_BAR
followed by EPF_NTB_COMMAND_LINK_UP with fallback == true,
epf_ntb_dma_set_bar() reinstalls bar->submap while leaving dma->mw_fallback
set to true. When epf_ntb_dma_link_up() then runs:

	if (fallback == dma->mw_fallback)
		return 0;

it returns early without removing the reinstalled DMA submaps. Similarly, if
EPF_NTB_COMMAND_SETUP_DMA_BAR arrives while dma->mw_fallback is true after
vntb_epf_mw_set_trans() configured bar->phys_addr, epf_ntb_dma_set_bar()
overwrites the BAR mapping with dma->submap where dma->submap[0].phys_addr is
0 or stale.

3. Conversely, when transitioning from dma->mw_fallback == true back to
fallback == false, if vntb_epf_mw_set_trans() ran while dma->mw_fallback was
true, it called epf_ntb_dma_set_bar_locked(ntb, false, &addr) which cleared
bar->submap and stored addr only in bar->phys_addr. When
epf_ntb_dma_link_up() then sees fallback == false, it only sets
dma->mw_fallback = false without reinstalling dma->submap or migrating
bar->phys_addr into dma->submap[0].phys_addr.

[ ... ]

> @@ -1936,6 +2037,21 @@ 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)) {
> +		guard(mutex)(&ntb->dma->lock);
> +
> +		if (ntb->dma->mw_fallback) {
> +			if (size > epf_bar->size)
> +				return -EINVAL;
> +		} else if (size != ntb->mws_size[idx]) {
> +			/* DMA submaps start after the configured MW size. */
> +			return -EINVAL;
> +		}
> +
> +		return epf_ntb_dma_set_bar_locked(ntb, !ntb->dma->mw_fallback,
> +						  &addr);
> +	}
> +
>  	epf_bar->phys_addr = addr;
>  	epf_bar->barno = barno;
>  	epf_bar->size = size;
> @@ -2134,15 +2250,29 @@ 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];
> +	resource_size_t salign = 1;
> +	u64 size = ntb->mws_size[idx];
> +
> +	if (epf_ntb_dma_shares_bar(ntb, barno)) {
> +		if (ntb->dma->mw_fallback)
> +			size = ntb->epf->bar[barno].size;
> +		else
> +			/*
> +			 * DMA submaps start after the configured MW size.
> +			 * Use size_align to require a full-size MW translation.
> +			 */
> +			salign = size;
> +	}
>  
>  	if (addr_align)
>  		*addr_align = SZ_4K;
>  
>  	if (size_align)
> -		*size_align = 1;
> +		*size_align = salign;
>  
>  	if (size_max)
> -		*size_max = ntb->mws_size[idx];
> +		*size_max = size;
>  
>  	return 0;
>  }

[Severity: High]
What happens in vntb_epf_mw_get_align() and vntb_epf_mw_set_trans() when an
NTB client allocates a memory window buffer smaller than the BAR or MW size
on a shared BAR?

1. In mw_fallback mode (ntb->dma->mw_fallback == true),
vntb_epf_mw_get_align() leaves salign = 1 and *addr_align = SZ_4K, while
vntb_epf_mw_set_trans() accepts any size <= epf_bar->size and calls
epf_ntb_dma_set_bar_locked(ntb, false, &addr). Unlike the non-shared path
(which updates epf_bar->size = size), epf_ntb_dma_set_bar_locked() keeps
bar->size at the full enumerated BAR size in BAR Match Mode (num_submap = 0).

In the DesignWare EPC driver, BAR Match Mode validates and maps the full
epf_bar->size:

drivers/pci/controller/dwc/pcie-designware.c:dw_pcie_prog_ep_inbound_atu() {
	...
	if (!IS_ALIGNED(parent_bus_addr, pci->region_align) ||
	    !IS_ALIGNED(parent_bus_addr, size))
		return -EINVAL;
	...
}

When an NTB client (such as ntb_tool, ntb_msi, ntb_perf, or ntb_transport
with max_mw_size) allocates a buffer with size < epf_bar->size, the DMA
address is only aligned to the smaller buffer size or SZ_4K, causing
pci_epc_set_bar() to fail with -EINVAL. If the buffer address happens to be
aligned to epf_bar->size, BAR Match Mode maps the full epf_bar->size window
over the smaller allocation, exposing adjacent unallocated endpoint memory to
the host.

2. In normal shared-BAR mode (ntb->dma->mw_fallback == false),
vntb_epf_mw_get_align() sets *size_align = ntb->mws_size[idx] while leaving
*addr_align = SZ_4K. In drivers/ntb/ntb_transport.c:ntb_set_mw():

	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);

If the requested size is smaller than ntb->mws_size[idx], ntb_set_mw()
allocates only buff_size (rounded to SZ_4K) but passes xlat_size (rounded up
to ntb->mws_size[idx]) to ntb_mw_set_trans(), mapping the full submap[0].size
over the smaller buff_size allocation. Also, if features->align > SZ_4K,
reporting *addr_align = SZ_4K allows clients to allocate DMA buffers that are
not aligned to features->align, which dw_pcie_ep_validate_submap() rejects
with -EINVAL.

титься

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