Re: [PATCH net-next v4 03/13] net: microchip: fdma: add PCIe ATU support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Race condition and unsafe unmap ordering in the shared ATU region pool.
- [High] Missing ATU hardware initialization and REGION_CTRL1 configuration.
- [Medium] ATU translation window is hardcoded to the maximum region size, bypassing DMA isolation.
- [Medium] Dangling pointers left after DMA teardown risk double-free memory corruption.
--

commit 22984db44c5308e0df119c7e94e5241b187dad51
Author: Daniel Machon <daniel.machon@microchip.com>

net: microchip: fdma: add PCIe ATU support

When operating as a PCIe endpoint, the lan966x/lan969x FDMA engine needs to
map outbound DMA addresses through a PCIe Address Translation Unit (ATU).
This patch adds functions to allocate FDMA memory and map it into one of
six equally-sized ATU outbound regions.

> diff --git a/drivers/net/ethernet/microchip/fdma/fdma_api.c b/drivers/net/ethernet/microchip/fdma/fdma_api.c
> index e78c3590da9e6..e0c2b137afef7 100644
> --- a/drivers/net/ethernet/microchip/fdma/fdma_api.c
> +++ b/drivers/net/ethernet/microchip/fdma/fdma_api.c

[ ... ]

> +#if IS_ENABLED(CONFIG_MCHP_LAN966X_PCI)
> +/* Allocate coherent DMA memory and map it in the ATU. */
> +int fdma_alloc_coherent_and_map(struct device *dev, struct fdma *fdma,
> +				struct fdma_pci_atu *atu)
> +{
> +	struct fdma_pci_atu_region *region;
> +	int err;
> +
> +	err = fdma_alloc_coherent(dev, fdma);
> +	if (err)
> +		return err;
> +
> +	region = fdma_pci_atu_region_map(atu, fdma->dma, fdma->size);
> +	if (IS_ERR(region)) {
> +		fdma_free_coherent(dev, fdma);

Could leaving fdma->dcbs populated after calling fdma_free_coherent() cause
a double-free if the caller has a teardown routine that checks for non-NULL
pointers?

> +		return PTR_ERR(region);
> +	}
> +
> +	fdma->atu_region = region;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(fdma_alloc_coherent_and_map);
> +
> +/* Free coherent DMA memory and unmap the memory in the ATU. */
> +void fdma_free_coherent_and_unmap(struct device *dev, struct fdma *fdma)
> +{
> +	fdma_pci_atu_region_unmap(fdma->atu_region);
> +	fdma_free_coherent(dev, fdma);

Similarly, should fdma->atu_region and fdma->dcbs be cleared after being
freed to prevent dangling pointers?

> +}
> +EXPORT_SYMBOL_GPL(fdma_free_coherent_and_unmap);
> +#endif

[ ... ]

> diff --git a/drivers/net/ethernet/microchip/fdma/fdma_pci.c b/drivers/net/ethernet/microchip/fdma/fdma_pci.c
> new file mode 100644
> index 0000000000000..1bd41eaa58a4b
> --- /dev/null
> +++ b/drivers/net/ethernet/microchip/fdma/fdma_pci.c

[ ... ]

> +/* Configure the address translation in the ATU. */
> +static void
> +fdma_pci_atu_configure_translation(struct fdma_pci_atu_region *region)
> +{
> +	struct fdma_pci_atu *atu = region->atu;
> +	int idx = region->idx;

Does the ATU require REGION_CTRL1 (TLP Type at offset 0x0) to be configured
here?

If a bootloader previously configured an ATU region for Configuration or I/O
accesses, could the register retain that type and cause memory accesses to
be translated into invalid TLP types?

> +
> +	writel(lower_32_bits(region->base_addr),
> +	       fdma_pci_atu_addr_get(atu->addr,
> +				     FDMA_PCI_ATU_LWR_BASE_ADDR, idx));
> +
> +	writel(upper_32_bits(region->base_addr),
> +	       fdma_pci_atu_addr_get(atu->addr,
> +				     FDMA_PCI_ATU_UPP_BASE_ADDR, idx));
> +
> +	/* Upper limit register only needed with REGION_SIZE > 4GB. */
> +	writel(region->limit_addr,
> +	       fdma_pci_atu_addr_get(atu->addr, FDMA_PCI_ATU_LIMIT_ADDR, idx));

Since the LIMIT_ADDR register is always programmed to the maximum static
region size, does this allow the FDMA engine to read or write beyond its
intended buffer into adjacent host memory?

Should the limit register be dynamically programmed using the requested size
passed to fdma_pci_atu_region_map() instead?

> +
> +	writel(lower_32_bits(region->target_addr),
> +	       fdma_pci_atu_addr_get(atu->addr,
> +				     FDMA_PCI_ATU_LWR_TARGET_ADDR, idx));
> +
> +	writel(upper_32_bits(region->target_addr),
> +	       fdma_pci_atu_addr_get(atu->addr,
> +				     FDMA_PCI_ATU_UPP_TARGET_ADDR, idx));
> +}
> +
> +/* Find an unused ATU region. */
> +static struct fdma_pci_atu_region *
> +fdma_pci_atu_region_get_free(struct fdma_pci_atu *atu)
> +{
> +	struct fdma_pci_atu_region *regions = atu->regions;
> +
> +	for (int i = 0; i < FDMA_PCI_ATU_REGION_MAX; i++) {
> +		if (regions[i].in_use)
> +			continue;

Is there a race condition when accessing the shared atu->regions array?

Since there appears to be no locking, could concurrent allocations claim the
same region and overwrite each other's hardware registers?

> +
> +		return &regions[i];
> +	}
> +
> +	return ERR_PTR(-ENOSPC);
> +}
> +
> +/* Unmap an ATU region, clearing its translation and disabling it. */
> +void fdma_pci_atu_region_unmap(struct fdma_pci_atu_region *region)
> +{
> +	if (IS_ERR_OR_NULL(region))
> +		return;
> +
> +	region->target_addr = 0;
> +	region->in_use = false;
> +
> +	fdma_pci_atu_region_disable(region);

By setting region->in_use = false before disabling the region in hardware,
could a concurrent thread allocate and enable this region, only for this
unmap thread to proceed and clear the hardware translations, leaving the
new allocation disabled?

> +	fdma_pci_atu_configure_translation(region);
> +}
> +EXPORT_SYMBOL_GPL(fdma_pci_atu_region_unmap);

[ ... ]

> +/* Initialize ATU, dividing the OB space into equally sized regions. */
> +void fdma_pci_atu_init(struct fdma_pci_atu *atu, void __iomem *addr)
> +{
> +	struct fdma_pci_atu_region *regions = atu->regions;
> +	u32 region_size = fdma_pci_atu_region_size();
> +
> +	atu->addr = addr;
> +
> +	for (int i = 0; i < FDMA_PCI_ATU_REGION_MAX; i++) {
> +		regions[i].base_addr =
> +			FDMA_PCI_ATU_OB_START + (i * region_size);
> +		regions[i].limit_addr =
> +			regions[i].base_addr + region_size - 1;
> +		regions[i].idx = i;
> +		regions[i].atu = atu;
> +	}

Should the hardware regions be explicitly disabled during initialization?

If the bootloader left any regions enabled that overlap with the Outbound
space, could they silently intercept DMA traffic before the driver maps
them?

> +}
> +EXPORT_SYMBOL_GPL(fdma_pci_atu_init);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508-lan966x-pci-fdma-v4-0-14e0c89d8d63@microchip.com?part=3