Re: [PATCH v3 5/5] dma-iommu: account for min_align_mask

From: Robin Murphy
Date: Thu Aug 12 2021 - 05:57:45 EST


On 2021-08-12 02:45, David Stevens wrote:
On Thu, Aug 12, 2021 at 4:12 AM Robin Murphy <robin.murphy@xxxxxxx> wrote:

On 2021-08-11 03:42, David Stevens wrote:
From: David Stevens <stevensd@xxxxxxxxxxxx>

For devices which set min_align_mask, swiotlb preserves the offset of
the original physical address within that mask. Since __iommu_dma_map
accounts for non-aligned addresses, passing a non-aligned swiotlb
address with the swiotlb aligned size results in the offset being
accounted for twice in the size passed to iommu_map_atomic. The extra
page exposed to DMA is also not cleaned up by __iommu_dma_unmap, since
tht at function unmaps with the correct size. This causes mapping failures
if the iova gets reused, due to collisions in the iommu page tables.

To fix this, pass the original size to __iommu_dma_map, since that
function already handles alignment.

Additionally, when swiotlb returns non-aligned addresses, there is
padding at the start of the bounce buffer that needs to be cleared.

Fixes: 1f221a0d0dbf ("swiotlb: respect min_align_mask")
Signed-off-by: David Stevens <stevensd@xxxxxxxxxxxx>
---
drivers/iommu/dma-iommu.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 89b689bf801f..ffa7e8ef5db4 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -549,9 +549,8 @@ static dma_addr_t __iommu_dma_map_swiotlb(struct device *dev, phys_addr_t phys,
struct iommu_domain *domain = iommu_get_dma_domain(dev);
struct iommu_dma_cookie *cookie = domain->iova_cookie;
struct iova_domain *iovad = &cookie->iovad;
- size_t aligned_size = org_size;
- void *padding_start;
- size_t padding_size;
+ void *tlb_start;
+ size_t aligned_size, iova_off, mapping_end_off;
dma_addr_t iova;

/*
@@ -566,24 +565,26 @@ static dma_addr_t __iommu_dma_map_swiotlb(struct device *dev, phys_addr_t phys,
if (phys == DMA_MAPPING_ERROR)
return DMA_MAPPING_ERROR;

- /* Cleanup the padding area. */
- padding_start = phys_to_virt(phys);
- padding_size = aligned_size;
+ iova_off = iova_offset(iovad, phys);
+ tlb_start = phys_to_virt(phys - iova_off);

+ /* Cleanup the padding area. */
if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC) &&
(dir == DMA_TO_DEVICE ||
dir == DMA_BIDIRECTIONAL)) {
- padding_start += org_size;
- padding_size -= org_size;
+ mapping_end_off = iova_off + org_size;
+ memset(tlb_start, 0, iova_off);
+ memset(tlb_start + mapping_end_off, 0,
+ aligned_size - mapping_end_off);
+ } else {
+ memset(tlb_start, 0, aligned_size);
}
-
- memset(padding_start, 0, padding_size);
}

if (!coherent && !(attrs & DMA_ATTR_SKIP_CPU_SYNC))
arch_sync_dma_for_device(phys, org_size, dir);

- iova = __iommu_dma_map(dev, phys, aligned_size, prot, dma_mask);
+ iova = __iommu_dma_map(dev, phys, org_size, prot, dma_mask);

This doesn't feel right - what if the IOVA granule was equal to or
smaller than min_align_mask, wouldn't you potentially end up mapping the
padding rather than the data?

The phys value returned by swiotlb_tbl_map_single is the address of
the start of the data in the swiotlb buffer, so the range that needs
to be mapped is [phys, phys + org_size). __iommu_dma_map will handle
this the same as it handles a misaligned mapping in the non-swiotlb
case. It might map memory before/after the desired range, but it will
map the entire range and iova will be the mapped address of phys. Is
there something I'm missing there?

No, my bad - I overlooked that phys got rewritten, so that aspect is OK, but...

That said, considering that memory before phys might be mapped, I
think there is actually still a bug. The buffer allocated by swiotlb
needs to be aligned to the granule size to ensure that preceding
swiotlb slots aren't mapped. The swiotlb does align allocations larger
than a page to PAGE_SIZE, but if IO_TLB_SIZE < IOVA granule <
PAGE_SIZE, then there can be problems. That can't happen if PAGE_SIZE
is 4k, but it can for larger page sizes. I'll add a fix for that to
the next version of this series.

I was mainly thinking that we still need to map aligned_size (where that also accounts for min_align_mask) from tlb_start in order to guarantee that the buffer ends up at the right offset in IOVA space. I suppose technically we could be cleverer about only padding one thing or the other depending on the IOVA granule, but I'm not sure it's worth the bother of decoupling the IOMMU mapping from the IOVA allocation just for this niche case.

Robin.