Hi,
On 2026-07-06 at 11:45:46 -0300, Jason Gunthorpe wrote:
> On Mon, Jul 06, 2026 at 02:08:14PM +0100, Robin Murphy wrote:
>
> > modern use-cases anyway, and is being replaced, so limitations of a "legacy"
> > API that don't have any meaningful impact to its existing users are hardly
> > something to panic about. If DRM does want to be able to *reliably* map
> > massive amounts of RAM then it can adopt the new IOMMU API, for this and all
> > the other reasons that that new API was promised to be "better".
>
> Yes the new API does solve these issues, and it avoids padding
> destroying the iova contiguity.
>
> > > So I wouldn't be quite so dismissive that this is not something a real
> > > user can hit.
> >
> > I'm not being dismissive - clearly it can be hit. My point is that anyone
> > who *does* hit it can only expect it to fail (as indeed this particular IGT
> > test seems to), because it has never worked.
>
> Today you can allocate memory from hugetlbfs and map it through
> RDMA. The user can set 1G hugetlbfs page size and allocate 16G of
> memory, and RDMA map it. Databases and HPC, for example, love to do
> work loads like this.
>
> So there is an unlucky hugetlbfs FD that has an internal memory layout
> that will build a scatterlist that cannot be mapped by the iommu.
>
> The user's application that normally works will fail randomly and
> infrequently for no fault of their own. This isn't "can only expect it
> to fail".
>
> > So yes, limiting any individual segment to <=2GB would end up avoiding both
> > those conditions, but it would also impact plenty of cases that *do*
> > currently work fine, e.g. 1GB+3GB+3GB. The limitation is really that you
> > can't have two consecutive segments where the first starts exactly on a 4GB
> > boundary and the sum of both their sizes >=4GB.
>
> Yeah, but that's hard to express.
>
> My feeling was supporting >2GB without a split is not really so
> important. You are (righly) arguing these huge sizes are usually rare.
> Places like RDMA where they are not rare people already added
> re-assembly logic to join SGLs so HW optimizations for >= 4G mappings
> can be used, so smaller splitting doesn't really matter.
>
> > > API wise I expect any arbitary input to sg_alloc_table_from_pages() to
> > > result in a scatterlist that iommu_dma_map_sg() will map. This
> > > patch highlights there are cornere cases where that isn't true, it
> > > should be fixed..
> >
> > Technically sg_alloc_table_from_pages() carries no such assumption,
> > only
>
> There are lots of callers that just do sg_alloc_table_from_pages()
> then dma_map_sg(). Callers can use this simplified API when they don't
> need any segmentation logic in the DMA API.
>
> > sg_alloc_table_from_pages_segment() (or __sg_alloc_table_from_pages()) with
> > the correct dma_seg_boundary value for the given device.
>
> _segment is the segment size, not boundary. The sg_alloc_table
> functions don't support the seg_boundary limitation.
>
I do not have any strong feelings for or against any of your
suggestions. We could do one of them or both, as long as you and
community are happy, I'm happy too.
Current fix adjusted to Jason's suggestion:
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 381b60d9e7ce..1a36fd9bf10b 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -1493,7 +1493,18 @@ int iommu_dma_map_sg(struct device *dev, struct
scatterlist *sg, int nents,
* time through here (i.e. before it has a meaningful value).
*/
if (pad_len && pad_len < s_length - 1) {
- prev->length += pad_len;
+ unsigned int new_pad_len;
+ /*
+ * For large mappings spanning multiple GBs we
+ * may not be able to fit all needed padding into
+ * sg->length.
+ */
+ if (check_add_overflow(prev->length, pad_len,
&new_pad_len)) {
+ ret = -EOVERFLOW;
+ goto out_restore_sg;
+ }
+
+ prev->length = new_pad_len;
iova_len += pad_len;
}
My interpretation of what should be done according to Jason's
point of view:
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 381b60d9e7ce..4d06010216bb 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -33,6 +33,7 @@
#include <linux/spinlock.h>
#include <linux/swiotlb.h>
#include <linux/vmalloc.h>
+#include <linux/scatterlist.h>
#include <trace/events/swiotlb.h>
#include "dma-iommu.h"
@@ -1477,6 +1478,10 @@ int iommu_dma_map_sg(struct device *dev, struct
scatterlist *sg, int nents,
sg_dma_len(s) = s_length;
s->offset -= s_iova_off;
s_length = iova_align(iovad, s_length + s_iova_off);
+
+ if (WARN_ON_ONCE(s_length > SG_MAX_LENGTH))
+ goto out_restore_sg;
+
s->length = s_length;
/*
diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index 6de1a2434299..04be2fa10359 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -472,6 +472,12 @@ int sg_alloc_table_from_pages_segment(struct sg_table
*sgt, struct page **pages,
unsigned long size,
unsigned int max_segment, gfp_t gfp_mask);
+
+/*
+ * Keep enough headroom for segment-merging in DMA/IOMMU paths.
+ */
+#define SG_MAX_LENGTH (UINT_MAX / 2U)
+
/**
* sg_alloc_table_from_pages - Allocate and initialize an sg table from
* an array of pages
@@ -499,7 +505,7 @@ static inline int sg_alloc_table_from_pages(struct sg_table
*sgt,
unsigned long size, gfp_t gfp_mask)
{
return sg_alloc_table_from_pages_segment(sgt, pages, n_pages, offset,
- size, UINT_MAX, gfp_mask);
+ size, SG_MAX_LENGTH, gfp_mask);
}
#ifdef CONFIG_SGL_ALLOC
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index b7fe91ef35b8..a788345b93a1 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -468,6 +468,7 @@ int sg_alloc_append_table_from_pages(struct sg_append_table
*sgt_append,
* The algorithm below requires max_segment to be aligned to PAGE_SIZE
* otherwise it can overshoot.
*/
+ max_segment = min(max_segment, SG_MAX_LENGTH);
max_segment = ALIGN_DOWN(max_segment, PAGE_SIZE);
if (WARN_ON(max_segment < PAGE_SIZE))
return -EINVAL;
@Robin @Jason I value your input and would like to reach an
agreement on how to proceed.
If scatterlists are going away soon-ish, then we could go with
the quick-fix version as to not involve too many people.
--
Best Regards,
Krzysztof
