Re: [PATCH v2 4/5] PCI: endpoint: pci-epf-vntb: Export endpoint DMA channels

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Massive coherent DMA memory waste/failure when BAR_DMA shares an MW BAR.
- [High] epf_ntb_dma_set_active() overwrites the Memory Window (MW) outbound translation address with local DMA memory, breaking peer-to-peer data transfers.
- [High] Heap out-of-bounds write and memory leak when multiple memory windows share the DMA BAR.
- [High] Premature release of `ntb->dma` in `epf_ntb_unbind` leads to a Use-After-Free and data race with active NTB clients.
- [Medium] Mixing goto-based error handling and scope-based resource management (__free) in the same function.
--

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

PCI: endpoint: pci-epf-vntb: Export endpoint DMA channels

This commit allows a Root Complex to use endpoint-local DMA read channels 
by exporting them via a private control-region extension. It adds new 
configfs attributes and maps DMA resources through an optional DMA BAR.

> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> index d12d134ce5531..142799f64fe17 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
[ ... ]
> @@ -219,6 +268,445 @@ static bool epf_ntb_is_bar_used(struct epf_ntb *ntb,
[ ... ]
> +static int
> +epf_ntb_dw_edma_collect(struct epf_ntb *ntb,
> +			const struct pci_epc_aux_resource *ctrl,
> +			const struct pci_epc_aux_resource *resources,
> +			unsigned int count)
> +{
[ ... ]
> +	if (ctrl->u.dma_ctrl.ep_to_rc_ch_cnt > EDMA_MAX_WR_CH ||
> +	    !ctrl->u.dma_ctrl.rc_to_ep_ch_cnt ||
> +	    ctrl->u.dma_ctrl.rc_to_ep_ch_cnt > EDMA_MAX_RD_CH)
> +		return -EINVAL;
> +
> +	struct epf_ntb_dma *dma __free(kfree) =
> +		kzalloc(sizeof(*dma), GFP_KERNEL);

[Severity: Medium]
Is it permitted to mix scope-based cleanup (__free) with a manual goto label
in the same function here?

The kernel cleanup guidelines explicitly prohibit mixing goto with scope-based
cleanups in the same function, as it creates confusing ownership semantics and
can lead to double-free or resource leak bugs.

> +	if (!dma)
> +		return -ENOMEM;
[ ... ]
> +	for (i = 0; i < dma->rd_ch_cnt; i++) {
> +		u16 chan_id = ctrl->u.dma_ctrl.ep_to_rc_ch_cnt + i;
> +
> +		ret = epf_ntb_dw_edma_claim(dma_dev, chan_id, &dma->dchan[i]);
> +		if (ret)
> +			goto err_release;
> +	}
[ ... ]
> +	ntb->dma = no_free_ptr(dma);
> +	return 0;
> +
> +err_release:
> +	epf_ntb_dw_edma_release_channels(ntb, dma, false);
> +	return ret;
> +}
[ ... ]
> +static int epf_ntb_dma_set_bar_locked(struct epf_ntb *ntb, dma_addr_t addr,
> +				      bool submapped)
> +{
> +	struct epf_ntb_dma *dma = ntb->dma;
> +	struct pci_epf_bar *bar;
> +	dma_addr_t old_addr;
> +	bool old_submapped;
> +	int restore, ret;
> +
> +	lockdep_assert_held(&dma->lock);
> +
> +	bar = &ntb->epf->bar[ntb->epf_ntb_bar[BAR_DMA]];
> +	old_submapped = bar->num_submap;
> +	if (dma->ctrl.submap.offset) {
> +		old_addr = dma->submap[0].phys_addr;
> +		dma->submap[0].phys_addr = addr;

[Severity: High]
When epf_ntb_dma_set_active() calls epf_ntb_dma_set_bar_locked(ntb,
bar->phys_addr, active) to toggle DMA, does this overwrite the peer's memory
window outbound translation address?

If a virtual ntb driver has previously set the translation address via
vntb_epf_mw_set_trans(), which writes to dma->submap[0].phys_addr, toggling
the DMA link state seems to permanently overwrite this with the local dummy
memory's physical address. This appears to silently break peer-to-peer data
transfers.

> +	}
> +	bar->submap = submapped ? dma->submap : NULL;
> +	bar->num_submap = submapped ? dma->num_submap : 0;
[ ... ]
> @@ -738,6 +1238,46 @@ static int epf_ntb_db_bar_init(struct epf_ntb *ntb)
>  
>  static void epf_ntb_mw_bar_clear(struct epf_ntb *ntb, int num_mws);
>  
> +static int epf_ntb_dma_bar_init(struct epf_ntb *ntb)
> +{
> +	const struct pci_epc_features *features;
> +	struct epf_ntb_dma *dma = ntb->dma;
> +	struct pci_epf_bar *bar;
> +	enum pci_barno barno;
> +	u32 mapped_size;
> +	int ret;
> +
> +	features = pci_epc_get_features(ntb->epf->epc, ntb->epf->func_no,
> +					ntb->epf->vfunc_no);
> +	if (!features)
> +		return -EOPNOTSUPP;
> +
> +	barno = ntb->epf_ntb_bar[BAR_DMA];
> +	mapped_size = dma->ctrl.submap.offset + dma->ctrl.submap.size;
> +	if (!pci_epf_alloc_space(ntb->epf, mapped_size, barno, features,
> +				 PRIMARY_INTERFACE))

[Severity: High]
When the DMA BAR shares a memory window BAR, will this mapping attempt fail
or cause massive memory waste?

The mapped_size is calculated by adding the large memory window size (often 
1GB or more) to the small DMA descriptor size. Then pci_epf_alloc_space() 
tries to allocate this amount via dma_alloc_coherent(). Requesting gigabytes 
of contiguous DMA coherent memory typically exceeds the page allocator limits 
and fails.

[Severity: High]
If configfs allows multiple memory windows to map to the same BAR as dma_bar, 
could calling this function repeatedly in a loop in epf_ntb_mw_bar_init() 
cause a memory leak?

Since space is allocated for the same BAR without freeing prior allocations,
this appears to leak the previously allocated memory on each iteration.

> +		return -ENOMEM;
> +
> +	bar = &ntb->epf->bar[barno];
> +	if (bar->size > U32_MAX)
> +		return -EOVERFLOW;
> +
> +	if (dma->ctrl.submap.offset)
> +		dma->submap[0].phys_addr = bar->phys_addr;
> +	if (mapped_size < bar->size)
> +		dma->submap[dma->num_submap++] = (struct pci_epf_bar_submap) {

[Severity: High]
If epf_ntb_dma_bar_init() is called repeatedly for multiple memory windows,
can this overflow the dma->submap array?

If dma->num_submap exceeds the 11-element capacity, it creates a heap 
out-of-bounds write that corrupts adjacent fields like reg_submap.

> +			.phys_addr = bar->phys_addr + mapped_size,
> +			.size = bar->size - mapped_size,
> +		};
> +
> +	ret = pci_epc_set_bar(ntb->epf->epc, ntb->epf->func_no,
> +			      ntb->epf->vfunc_no, bar);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
[ ... ]
> @@ -833,20 +1388,43 @@ static int epf_ntb_mw_bar_init(struct epf_ntb *ntb)
>   */
>  static void epf_ntb_mw_bar_clear(struct epf_ntb *ntb, int num_mws)
>  {
[ ... ]
> +	if (ntb->dma && ntb->dma->num_submap) {
> +		barno = ntb->epf_ntb_bar[BAR_DMA];
> +		if (!ntb->epf->bar[barno].addr)
> +			return;
> +
> +		ntb->epf->bar[barno].submap = NULL;
> +		ntb->epf->bar[barno].num_submap = 0;

[Severity: High]
Is it safe to update the submap fields locklessly here?

This can race with concurrent accesses to ntb->dma, as ntb->dma->lock is not
acquired before modifying the submap and num_submap fields.

> +		if (!bar_cleared[barno])
> +			pci_epc_clear_bar(ntb->epf->epc, ntb->epf->func_no,
> +					  ntb->epf->vfunc_no,
> +					  &ntb->epf->bar[barno]);
> +		pci_epf_free_space(ntb->epf, ntb->epf->bar[barno].addr, barno,
> +				   PRIMARY_INTERFACE);
>  	}
>  }
[ ... ]
> @@ -1423,6 +2026,15 @@ static int vntb_epf_mw_set_trans(struct ntb_dev *ndev, int pidx, int idx,
>  	dev = &ntb->ntb.dev;
>  	barno = ntb->epf_ntb_bar[BAR_MW1 + idx];
>  	epf_bar = &ntb->epf->bar[barno];
> +	if (epf_ntb_dma_shares_bar(ntb, barno)) {
> +		if (size != ntb->mws_size[idx])
> +			return -EINVAL;
> +
> +		guard(mutex)(&ntb->dma->lock);

[Severity: High]
Can an active concurrent virtual ntb client trigger a use-after-free here 
during unbind?

In epf_ntb_unbind(), epf_ntb_dma_release(ntb, true) frees ntb->dma before 
pci_unregister_driver() stops the virtual ntb clients. An active client 
calling vntb_epf_mw_set_trans() might evaluate epf_ntb_dma_shares_bar() as 
true, get preempted, and then dereference the freed ntb->dma to acquire the 
lock.

> +
> +		return epf_ntb_dma_set_bar_locked(ntb, addr, true);
> +	}
> +
>  	epf_bar->phys_addr = addr;
>  	epf_bar->barno = barno;
>  	epf_bar->size = size;
[ ... ]
> @@ -1795,6 +2414,7 @@ static void epf_ntb_unbind(struct pci_epf *epf)
>  
>  	epf_ntb_epc_cleanup(ntb);
>  	epf_ntb_config_spad_bar_free(ntb);
> +	epf_ntb_dma_release(ntb, true);
>  
>  	pci_unregister_driver(&vntb_pci_driver);
>  }

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