Re: [PATCH 06/11] PCI: endpoint: pci-epf-vntb: Add outbound MW group commands

[sashiko-bot@kernel.org]

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

New issues:
- [High] The cleanup path `epf_ntb_mw_bar_clear()` unconditionally calls `epf_ntb_teardown_mw_group(ntb, 0)` for packed MWs, leading to unmapping of physical address 0 if initialization fails.
- [High] The automatic ATU mapping teardown silently fails when `packed_mws == 1`, leading to ATU resource leaks and stale memory mappings.

Pre-existing issues:
- [High] Failure to unmap outbound ATU mappings for singleton (non-packed) memory windows during driver unbind or error cleanup leaves stale DMA translations active.
- [High] The command handler `epf_ntb_cmd_handler` reads `ctrl->command` and then `ctrl->argument` without a read memory barrier, risking processing of stale arguments.
--

--- Patch [6]: [PATCH 06/11] PCI: endpoint: pci-epf-vntb: Add outbound MW group commands ---
commit 6b0d64af683bb36f33f200f34cffed00c1c6d648
Author: Koichiro Den <den@valinux.co.jp>

PCI: endpoint: pci-epf-vntb: Add outbound MW group commands

Packed logical MWs share one contiguous EPC address range. Their
outbound translations can therefore use one mapping as well.

Add commands to map or unmap a complete group in one operation. Keep the
existing commands for singleton MWs.

> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> index aa8d304fe861f..526cc31d94351 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
[ ... ]
> @@ -319,6 +321,50 @@ static int epf_ntb_teardown_mw(struct epf_ntb *ntb, u32 mw)
[ ... ]
> +static int epf_ntb_teardown_mw_group(struct epf_ntb *ntb, u32 mw)
> +{
> +	unsigned int first, count;
> +	int ret;
> +
> +	ret = epf_ntb_get_mw_group(ntb, mw, &first, &count);
> +	if (ret)
> +		return ret;
> +	if (count <= 1)
> +		return -EOPNOTSUPP;

[Severity: High]
Does returning -EOPNOTSUPP here cause an ATU resource leak when a single
packed memory window is configured?

When packed_mws == 1, this returns -EOPNOTSUPP, which is then silently ignored
by callers like epf_ntb_mw_bar_clear() and COMMAND_LINK_DOWN.

This leaves the ATU hardware mappings active while the underlying physical EPC
memory is freed, which could lead to memory corruption if the physical address
is reallocated.

> +	if (mw != first)
> +		return -EINVAL;
> +
> +	pci_epc_unmap_addr(ntb->epf->epc, ntb->epf->func_no,
> +			   ntb->epf->vfunc_no, ntb->vpci_mw_phy[first]);
> +
> +	return 0;
> +}
[ ... ]
> @@ -378,6 +424,14 @@ static void epf_ntb_cmd_handler(struct work_struct *work)

[Severity: High]
This is a pre-existing issue, but does epf_ntb_cmd_handler() risk processing
stale arguments due to missing memory barriers?

The handler reads ctrl->command, checks it, and then reads ctrl->argument from
DMA-coherent memory.

Without a read memory barrier (e.g., smp_rmb()), control dependencies do not
prevent speculative reads on architectures with weak memory ordering.

Could the CPU speculatively read a stale argument before the command is fully
resolved?

>  		ret = epf_ntb_teardown_mw(ntb, argument);
>  		ctrl->command_status = ret ? COMMAND_STATUS_ERROR : COMMAND_STATUS_OK;
>  		break;
[ ... ]
> @@ -976,6 +1032,9 @@ static void epf_ntb_mw_bar_clear(struct epf_ntb *ntb, int num_mws)
>  	unsigned int count;
>  	int i;
>  
> +	if (ntb->packed_mws)
> +		epf_ntb_teardown_mw_group(ntb, 0);
> +

[Severity: High]
Is it possible to unmap physical address 0 if initialization fails?

If pci_epc_set_bar() fails for i=0 during epf_ntb_mw_bar_init(), it branches
to the error path and calls epf_ntb_mw_bar_clear(ntb, 0).

Because ntb->packed_mws is true, it unconditionally calls
epf_ntb_teardown_mw_group(ntb, 0).

Since the memory allocation was never reached for i=0, ntb->vpci_mw_phy[0] is
still 0, causing pci_epc_unmap_addr() to operate on uninitialized physical
address 0.

>  	for (i = 0; i < num_mws; i++) {
>  		barno = ntb->mw_layout[i].barno;

[Severity: High]
This is a pre-existing issue, but does this loop fail to unmap outbound ATU
mappings for standard (non-packed) memory windows?

During driver unbind or error cleanup, this loop frees the outbound EPC memory,
but it lacks a call to pci_epc_unmap_addr() for unpacked memory windows.

The physical EPC memory is freed while the ATU mapping remains active in
hardware, which could lead to silent memory corruption if re-allocated to
another endpoint function.

>  		if (!bar_cleared[barno]) {

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