Re: [PATCH v6 1/2] PCI: endpoint: pci-epf-vntb: Serialize virtual PCI bus scan

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Global static PCI driver and ID table are mutated and registered inside a per-device bind callback, preventing multi-instance support and corrupting global state.
- [High] Improper mixing of C struct accesses and I/O accessors (readl/writel) on a DMA coherent RAM pointer.
--

commit 52fb844432dfeb3fe13eef7dc35765e5cdc04cfa
Author: Koichiro Den <den@valinux.co.jp>

PCI: endpoint: pci-epf-vntb: Serialize virtual PCI bus scan

The virtual PCI bus is scanned without holding the PCI rescan/remove
lock. This can race with other PCI topology changes.

Hold the lock across the bus scan and device addition.

Fixes: e35f56bb0330 ("PCI: endpoint: Support NTB transfer between RC and EP")
Link: https://lore.kernel.org/r/3tgrcel5fl55ulf3zjya4fkz4t2ms7iwwjif5rnzn2rafsydd6@i4e5etm7uqvz/

> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> index c3caec927d748..412e8cc6fb1d2 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c

[ ... ]

While reviewing this file, I noticed a couple of pre-existing issues.

drivers/pci/endpoint/functions/pci-epf-vntb.c:epf_ntb_bind() {
    ...
	pci_space[0] = (ntb->vntb_pid << 16) | ntb->vntb_vid;
	pci_vntb_table[0].vendor = ntb->vntb_vid;
	pci_vntb_table[0].device = ntb->vntb_pid;

	ret = pci_register_driver(&vntb_pci_driver);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but does this code corrupt global
state if multiple instances of the endpoint function are bound?

If multiple endpoint functions are bound via configfs, a second call to
epf_ntb_bind() would overwrite the shared pci_vntb_table[0] and pci_space[0]
without synchronization, corrupting the first instance's state.

Also, won't calling pci_register_driver(&vntb_pci_driver) a second time fail
with -EEXIST, preventing the bind? Should this dynamic registration of a
global PCI driver be moved to the module initialization sequence instead?


A second issue occurs when allocating and accessing spad memory:

drivers/pci/endpoint/functions/pci-epf-vntb.c:epf_ntb_config_spad_bar_alloc() {
    ...
	base = pci_epf_alloc_space(epf, ctrl_size + spad_size,
				   barno, epc_features, 0);
    ...
	ntb->reg = base;

	ctrl = ntb->reg;
	ctrl->spad_offset = ctrl_size;
    ...
}

The exact same RAM pointer is then accessed via I/O accessors:

drivers/pci/endpoint/functions/pci-epf-vntb.c:vntb_epf_spad_read() {
    ...
	void __iomem *base = (void __iomem *)ntb->reg;

	val = readl(base + off + ct + idx * sizeof(u32));
    ...
}

[Severity: High]
This is a pre-existing issue, but does this code cause undefined behavior by
mixing C struct accesses and I/O accessors on a standard RAM pointer?

The driver allocates memory via pci_epf_alloc_space() (which uses
dma_alloc_coherent()) and assigns the returned standard virtual memory
pointer to ntb->reg.

Later, in vntb_epf_spad_read(), this standard RAM pointer is forcefully cast
to void __iomem * and accessed via readl().

On architectures with strictly separated I/O memory or strictly typed memory
domains, using MMIO instructions on standard cacheable memory can trigger
exceptions or alignment faults. Is it possible to avoid using MMIO accessors
on memory that isn't mapped as I/O memory?

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