Re: [PATCH v2 05/11] ARM: dma-mapping: Bypass the legacy path for core-owned DMA API domains

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] arm: dma-mapping: off-by-one size causes mixed semantic in aperture check
--

commit 5535bede81dc83f33cc8029ff49a58ed02614672
Author: Mikko Perttunen <mperttunen@nvidia.com>

ARM: dma-mapping: Bypass the legacy path for core-owned DMA API domains

This commit updates the ARM DMA mapping code to bypass custom IOMMU domain
management when a DMA default domain is requested and owned by the core.
It adds checks in the setup and teardown routines to skip the legacy path
and ensures that the device's dma-ranges cover the IOMMU aperture.

> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index f2d3e8ec34872..d6315baeddb9c 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -1761,13 +1761,33 @@ EXPORT_SYMBOL_GPL(arm_iommu_detach_device);
>  
>  static void arm_setup_iommu_dma_ops(struct device *dev)
>  {
> -	struct dma_iommu_mapping *mapping;
> +	struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
>  	u64 dma_base = 0, size = 1ULL << 32;
>  
>  	if (dev->dma_range_map) {
>  		dma_base = dma_range_map_min(dev->dma_range_map);
>  		size = dma_range_map_max(dev->dma_range_map) - dma_base;
>  	}
> +
> +	/*
> +	 * An existing cookie means the core owns the domain. Only set the
> +	 * per-binding DMA ops. IOVAs are allocated from the whole aperture,
> +	 * so the device's dma-ranges must cover it.
> +	 */
> +	if (mapping) {
> +		const struct iommu_domain_geometry *geo =
> +			&mapping->domain->geometry;
> +
> +		if (dma_base > geo->aperture_start ||
> +		    dma_base + size < geo->aperture_end) {

[Severity: Medium]
Does the new aperture check incorrectly mix inclusive and exclusive size
semantics?

When the device has a dma_range_map, the size calculation relies on
dma_range_map_max(), which returns an inclusive maximum address:

include/linux/dma-direct.h:dma_range_map_max() {
    ...
    for (; map->size; map++)
        ret = max(ret, map->dma_start + map->size - 1);
    return ret;
}

Since dma_range_map_max() is inclusive, size = max - min results in a
value 1 byte too small. The new check (dma_base + size < geo->aperture_end)
appears to mathematically rely on this off-by-one size to evaluate correctly
against an inclusive aperture_end.

If the size calculation is ever corrected, would this incorrectly accept
dma-ranges that are 1 byte smaller than the aperture? Furthermore, when
dma_range_map is absent, size defaults to 1ULL << 32 (exclusive). If
aperture_end is exactly 0x100000000, will this check incorrectly fail?

> +			dev_err(dev, "dma-ranges do not cover the IOMMU aperture\n");
> +			return;
> +		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016-b4-arm32-iommu-dma-cookie-v2-0-b06842a01938@nvidia.com?part=5