Hi Robin,
thanks for looking at this.
On 2026-06-25 at 15:29:10 +0100, Robin Murphy wrote:
> On 25/06/2026 2:43 pm, Krzysztof Karas wrote:
> > Currently, if iommu maps fewer bytes than requested (iova_len),
> > it proceeds to free the iova, but never tries to unmap already
> > touched bytes. This behavior may cause memory hogging down the
> > line.
>
> Huh? iommu_map_sg() has always unmapped internally upon any error - can you
> clarify how you've seen it returning a short mapping in a non-error case?
Yes. I applied some debug logging:
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 381b60d9e7ce..f166cd31d68f 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -1515,8 +1515,10 @@ int iommu_dma_map_sg(struct device *dev, struct
scatterlist *sg, int nents,
* implementation - it knows better than we do.
*/
ret = iommu_map_sg(domain, iova, sg, nents, prot, GFP_ATOMIC);
- if (ret < 0 || ret < iova_len)
+ if (ret < 0 || ret < iova_len) {
+ printk("%s: ret = %zd, iova_len = %lu\n", __func__, ret,
iova_len);
goto out_free_iova;
+ }
return __finalise_sg(dev, sg, nents, iova);
@@ -1525,8 +1527,10 @@ int iommu_dma_map_sg(struct device *dev, struct
scatterlist *sg, int nents,
out_restore_sg:
__invalidate_sg(sg, nents);
out:
- if (ret != -ENOMEM && ret != -EREMOTEIO)
+ if (ret != -ENOMEM && ret != -EREMOTEIO) {
+ printk("%s: returning -EINVAL\n", __func__);
return -EINVAL;
+ }
return ret;
}
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index d1a9e713d3a0..59163ad0bce3 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2900,6 +2900,7 @@ ssize_t iommu_map_sg(struct iommu_domain *domain,
unsigned long iova,
return mapped;
out_err:
+ printk("%s: Calling iommu_unmap()\n", __func__);
/* undo mappings already done */
iommu_unmap(domain, iova, mapped);
and ran "gem_exec_big" with subtest "single" from igt-gpu-tools
(./igt-gpu-tools/build/tests/gem_exec_big --run-subtest single)
on a Tiger Lake platform. I observed:
[ 77.494513] [IGT] gem_exec_big: executing
[ 77.513162] [IGT] gem_exec_big: starting subtest single
[ 77.513739] gem_exec_big (2863): drop_caches: 4
[ 79.912199] i915 0000:00:02.0: Using 39-bit DMA addresses
[ 79.915571] iommu_dma_map_sg: ret = 7138717696, iova_len = 20023619584
<<===
[ 79.915581] iommu_dma_map_sg: returning -EINVAL
[ 82.680323] [IGT] gem_exec_big: finished subtest single, SUCCESS
[ 82.682692] [IGT] gem_exec_big: exiting, ret=0
on a first test execution and then subsequent runs would result
DMA remap failures:
[ 1053.077620] [IGT] gem_exec_big: executing
[ 1053.086272] [IGT] gem_exec_big: starting subtest single
[ 1055.220807] iommu_dma_map_sg: ret = 10581184512, iova_len = 19171119104
[ 1055.220821] iommu_dma_map_sg: returning -EINVAL
[ 1055.343891] iommu_map_sg: Calling iommu_unmap()
[ 1055.343895] iommu_dma_map_sg: ret = -98, iova_len = 15817768960
[ 1055.356710] iommu_dma_map_sg: returning -EINVAL
[ 1055.356807] i915 0000:00:02.0: Failed to DMA remap 3861760 pages
[ 1055.509975] iommu_map_sg: Calling iommu_unmap()
[ 1055.509981] iommu_dma_map_sg: ret = -98, iova_len = 19171119104
[ 1055.509990] iommu_dma_map_sg: returning -EINVAL
[ 1055.636194] iommu_map_sg: Calling iommu_unmap()
[ 1055.636198] iommu_dma_map_sg: ret = -98, iova_len = 15817768960
[ 1055.648844] iommu_dma_map_sg: returning -EINVAL
[ 1055.648955] i915 0000:00:02.0: Failed to DMA remap 3861760 pages
[ 1055.800664] iommu_map_sg: Calling iommu_unmap()
[ 1055.800670] iommu_dma_map_sg: ret = -98, iova_len = 19171119104
[ 1055.800679] iommu_dma_map_sg: returning -EINVAL
[ 1055.924869] iommu_map_sg: Calling iommu_unmap()
[ 1055.924873] iommu_dma_map_sg: ret = -98, iova_len = 15817768960
[ 1055.937634] iommu_dma_map_sg: returning -EINVAL
[ 1055.937729] i915 0000:00:02.0: Failed to DMA remap 3861760 pages
[ 1056.042152] [IGT] gem_exec_big: finished subtest single, FAIL
[ 1056.042736] [IGT] gem_exec_big: exiting, ret=98
which would cause the test to fail. Rebooting the machine is the
only way I could get the test to pass again.
>
> Thanks,
> Robin.
>
> > Correct that by unmapping before exiting.
> >
> > Signed-off-by: Krzysztof Karas <[email protected]>
> > ---
> > drivers/iommu/dma-iommu.c | 10 ++++++++--
> > 1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> > index 381b60d9e7ce..c4c058ba07ef 100644
> > --- a/drivers/iommu/dma-iommu.c
> > +++ b/drivers/iommu/dma-iommu.c
> > @@ -1515,8 +1515,14 @@ int iommu_dma_map_sg(struct device *dev, struct
> > scatterlist *sg, int nents,
> > * implementation - it knows better than we do.
> > */
> > ret = iommu_map_sg(domain, iova, sg, nents, prot, GFP_ATOMIC);
> > - if (ret < 0 || ret < iova_len)
> > + if (ret < 0 || ret < iova_len) {
> > + if (ret > 0) {
> > + /* Unmap partially mapped bytes before freeing IOVA */
> > + if (iommu_unmap(domain, iova, ret) != ret)
> > + ret = -EIO;
> > + }
> > goto out_free_iova;
> > + }
> > return __finalise_sg(dev, sg, nents, iova);
> > @@ -1525,7 +1531,7 @@ int iommu_dma_map_sg(struct device *dev, struct
> > scatterlist *sg, int nents,
> > out_restore_sg:
> > __invalidate_sg(sg, nents);
> > out:
> > - if (ret != -ENOMEM && ret != -EREMOTEIO)
> > + if (ret != -ENOMEM && ret != -EREMOTEIO && ret != -EIO)
> > return -EINVAL;
> > return ret;
> > }
>
--
Best Regards,
Krzysztof
