On 2023-01-18 18:00, Jason Gunthorpe wrote:
Change the sg_alloc_table_from_pages() allocation that was hardwired to
GFP_KERNEL to use the gfp parameter like the other allocations in this
function.
Auditing says this is never called from an atomic context, so it is safe
as is, but reads wrong.
I think the point may have been that the sgtable metadata is a
logically-distinct allocation from the buffer pages themselves. Much
like the allocation of the pages array itself further down in
__iommu_dma_alloc_pages(). I see these days it wouldn't be catastrophic
to pass GFP_HIGHMEM into __get_free_page() via sg_kmalloc(), but still,
allocating implementation-internal metadata with all the same
constraints as a DMA buffer has just as much smell of wrong about it IMO.
I'd say the more confusing thing about this particular context is why
we're using iommu_map_sg_atomic() further down - that seems to have been
an oversight in 781ca2de89ba, since this particular path has never
supported being called in atomic context.
Overall I'm starting to wonder if it might not be better to stick a "use
GFP_KERNEL_ACCOUNT if you allocate" flag in the domain for any level of
the API internals to pick up as appropriate, rather than propagate
per-call gfp flags everywhere. As it stands we're still missing
potential pagetable and other domain-related allocations by drivers in
.attach_dev and even (in probably-shouldn't-really-happen cases)
.unmap_pages...
Thanks,
Robin.
Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
---
drivers/iommu/dma-iommu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 8c2788633c1766..e4bf1bb159f7c7 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -822,7 +822,7 @@ static struct page **__iommu_dma_alloc_noncontiguous(struct device *dev,
if (!iova)
goto out_free_pages;
- if (sg_alloc_table_from_pages(sgt, pages, count, 0, size, GFP_KERNEL))
+ if (sg_alloc_table_from_pages(sgt, pages, count, 0, size, gfp))
goto out_free_iova;
if (!(ioprot & IOMMU_CACHE)) {
