Re: [PATCH v9 22/25] iommu/arm-smmu-v3-kvm: Shadow the CPU stage-2 page table

[Mostafa Saleh <smostafa@google.com>]

On Thu, Oct 15, 2026 at 08:32:05PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] iommu/arm-smmu-v3: incorrect recovery from -EEXIST in smmu_host_stage2_idmap leaves holes in memory
> --
> 
> commit d265b84d43b0a3e480c297258952f561f0f39998
> Author: Mostafa Saleh <smostafa@google.com>
> 
> iommu/arm-smmu-v3-kvm: Shadow the CPU stage-2 page table
> 
> This commit mirrors changes to the host stage-2 page table into an identity
> mapped stage-2 for the SMMUv3. It handles memory mapped with PAGE_SIZE and
> MMIO mapped with the largest block that fits.
> 
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/pkvm/arm-smmu-v3-hyp.c b/drivers/iommu/arm/arm-smmu-v3/pkvm/arm-smmu-v3-hyp.c
> > index 943ffef0bb4aa..75b2b1477a4fa 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/pkvm/arm-smmu-v3-hyp.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/pkvm/arm-smmu-v3-hyp.c
> [ ... ]
> > @@ -1011,8 +1070,100 @@ static bool smmu_dabt_handler(struct user_pt_regs *regs, u64 esr, u64 addr)
> [ ... ]
> > +static int smmu_host_stage2_idmap(phys_addr_t start, phys_addr_t end, int prot)
> > +{
> > +	size_t pgsize = PAGE_SIZE, pgcount, size;
> > +	struct io_pgtable *pgtable = idmap_pgtable;
> > +	int ret = 0;
> > +
> > +	end = min(end, BIT(pgtable->cfg.oas));
> > +	if (start >= end)
> > +		return 0;
> > +
> > +	size = end - start;
> > +	if (prot) {
> > +		size_t mapped;
> > +
> > +		if (!(prot & IOMMU_MMIO))
> > +			prot |= IOMMU_CACHE;
> > +
> > +		while (size) {
> > +			mapped = 0;
> [ ... ]
> > +			if (prot & IOMMU_MMIO)
> > +				pgsize = smmu_pgsize_idmap(size, start, pgtable->cfg.pgsize_bitmap);
> > +
> > +			pgcount = size / pgsize;
> > +			ret = pgtable->ops.map_pages(&pgtable->ops, start, start,
> > +						     pgsize, pgcount, prot, 0, &mapped);
> > +			size -= mapped;
> > +			start += mapped;
> > +
> > +			if (ret == -EEXIST) {
> > +				/*
> > +				 * It is possible to get EEXIST when a VM dies with pages
> > +				 * in a shared state.
> > +				 */
> > +				ret = 0;
> > +				size -= pgsize;
> > +				start += pgsize;
> > +				continue;
> > +			}
> 
> [Severity: High]
> Does this error handling skip valid pages when a collision occurs later in
> the batch?
> 
> The io-pgtable-arm backend implementation of map_pages validates all entries
> in the batch before writing any PTEs:
> 
> drivers/iommu/io-pgtable-arm.c:arm_lpae_init_pte() {
> 	for (i = 0; i < num_entries; i++)
> 		if (iopte_leaf(ptep[i], lvl, data->iop.fmt)) {
> 			/* We require an unmap first */
> 			WARN_ON(!(data->iop.cfg.quirks & IO_PGTABLE_QUIRK_NO_WARN));
> 			return -EEXIST;
> 		}
> }
> 
> If a collision is found at any index (for example, the 5th page of the batch),
> map_pages returns -EEXIST immediately without mapping anything, leaving mapped
> at 0.
> 
> Because mapped is 0, this code assumes the very first page of the batch caused
> the collision, skips it by doing start += pgsize, and retries the loop. This
> process repeatedly skips earlier non-colliding pages without mapping them,
> leaving holes in the SMMU stage 2 identity map and causing translation faults
> during host DMA.
> 

mmm, yes, this only works now because idmap is called with a single
leaf. That logic needs some cleanup just in case that changes.

Thanks,
Mostafa

> > +			if (!mapped || ret)
> > +				break;
> > +		}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261015200903.2735565-1-smostafa@google.com?part=22