arm64: dma-mapping: clear buffers allocated with FORCE_CONTIGUOUS flag

PATCH New Marek Szyprowski 3 participants 5 messages Jun 11, 2018 – Jun 12, 2018 1 message not in the archive arm64

Share & tools

A message that is not in the archive (sent off-list or not yet received): CGME20180611051352eucas1p1f256a2a303da85a1e0923f012d709e39@eucas1p1.samsung.com

Marek Szyprowski

author

2018-06-11 05:13 UTC #1 lore raw

PATCH 1/1 arm64: dma-mapping: clear buffers allocated with FORCE_CONTIGUOUS flag

dma_alloc_*() buffers might be exposed to userspace via mmap() call, so they should be cleared on allocation. In case of IOMMU-based dma-mapping implementation such buffer clearing was missing in the code path for DMA_ATTR_FORCE_CONTIGUOUS flag handling. This patch fixes this issue. For more information on clearing buffers allocated by dma_alloc_* functions, see commit 44176bb38fa4 ("arm64: dma-mapping: always clear allocated buffers").

Fixes 44176bb38fa4 ("arm64: Add support for DMA_ATTR_FORCE_CONTIGUOUS to IOMMU")

Signed-off-by Marek Szyprowski <[email protected]>

---
 arch/arm64/mm/dma-mapping.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

arch/arm64/mm/dma-mapping.c +5 -4

index 632d32109755..aa0037a3185f 100644
@@ -629,13 +629,14 @@ static void *__iommu_alloc_attrs(struct device *dev, size_t size,
 						    size >> PAGE_SHIFT);
 			return NULL;
 		}
-		if (!coherent)
-			__dma_flush_area(page_to_virt(page), iosize);
-
 		addr = dma_common_contiguous_remap(page, size, VM_USERMAP,
 						   prot,
 						   __builtin_return_address(0));
-		if (!addr) {
+		if (addr) {
+			memset(addr, 0, size);
+			if (!coherent)
+				__dma_flush_area(page_to_virt(page), iosize);
+		} else {
 			iommu_dma_unmap_page(dev, *handle, iosize, 0, attrs);
 			dma_release_from_contiguous(dev, page,
 						    size >> PAGE_SHIFT);

Signature · 1 line

2.17.1

Geert Uytterhoeven

2018-06-11 07:55 UTC #2 lore raw

PATCH 1/1 arm64: dma-mapping: clear buffers allocated with FORCE_CONTIGUOUS flag

Hi Marek,

Thanks for your patch!

Quoted 4 lines · Marek Szyprowski

dma_alloc_*() buffers might be exposed to userspace via mmap() call, so they should be cleared on allocation. In case of IOMMU-based dma-mapping implementation such buffer clearing was missing in the code path for DMA_ATTR_FORCE_CONTIGUOUS flag handling. This patch fixes this issue. For

Is it? The memory is allocated using dma_alloc_from_contiguous(..., gfp), and __iommu_alloc_attrs() has

        /*
         * Some drivers rely on this, and we probably don't want the
         * possibility of stale kernel data being read by devices anyway.
         */
        gfp |= __GFP_ZERO;

at the top, before the allocation.

If cma_alloc() (called from dma_alloc_from_contiguous()) doesn't honor __GFP_ZERO, I think cma_alloc() should be fixed instead.

Quoted 2 lines

more information on clearing buffers allocated by dma_alloc_* functions, see commit 44176bb38fa4 ("arm64: dma-mapping: always clear allocated

6829e274a623

Quoted 4 lines

buffers").
Fixes 44176bb38fa4 ("arm64: Add support for DMA_ATTR_FORCE_CONTIGUOUS to IOMMU")
Signed-off-by Marek Szyprowski <[email protected]>

Gr{oetje,eeting}s,

Geert

Signature · 5 lines

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Will Deacon

2018-06-11 12:31 UTC #3 lore raw

PATCH 1/1 arm64: dma-mapping: clear buffers allocated with FORCE_CONTIGUOUS flag

Quoted 20 lines · Geert Uytterhoeven

Hi Marek,
Thanks for your patch!
Quoted 4 lines · Marek Szyprowski
dma_alloc_*() buffers might be exposed to userspace via mmap() call, so they should be cleared on allocation. In case of IOMMU-based dma-mapping implementation such buffer clearing was missing in the code path for DMA_ATTR_FORCE_CONTIGUOUS flag handling. This patch fixes this issue. For
Is it? The memory is allocated using dma_alloc_from_contiguous(..., gfp), and __iommu_alloc_attrs() has
        /*
         * Some drivers rely on this, and we probably don't want the
         * possibility of stale kernel data being read by devices anyway.
         */
        gfp |= __GFP_ZERO;

Quoted 4 lines

at the top, before the allocation.
If cma_alloc() (called from dma_alloc_from_contiguous()) doesn't honor __GFP_ZERO, I think cma_alloc() should be fixed instead.

Agreed. We tried to fix this in 7132813c3845 ("arm64: Honor __GFP_ZERO in dma allocations"). Has something broken that?

Will

Marek Szyprowski

author

2018-06-12 07:31 UTC #4 lore raw

PATCH 1/1 arm64: dma-mapping: clear buffers allocated with FORCE_CONTIGUOUS flag

Quoted 20 lines · Will Deacon

Quoted 19 lines · Geert Uytterhoeven
Hi Marek,
Thanks for your patch!
Quoted 4 lines · Marek Szyprowski
dma_alloc_*() buffers might be exposed to userspace via mmap() call, so they should be cleared on allocation. In case of IOMMU-based dma-mapping implementation such buffer clearing was missing in the code path for DMA_ATTR_FORCE_CONTIGUOUS flag handling. This patch fixes this issue. For
Is it? The memory is allocated using dma_alloc_from_contiguous(..., gfp), and __iommu_alloc_attrs() has
         /*
          * Some drivers rely on this, and we probably don't want the
          * possibility of stale kernel data being read by devices anyway.
          */
         gfp |= __GFP_ZERO;

Quoted 6 lines

Quoted 4 lines
at the top, before the allocation.
If cma_alloc() (called from dma_alloc_from_contiguous()) doesn't honor __GFP_ZERO, I think cma_alloc() should be fixed instead.
Agreed. We tried to fix this in 7132813c3845 ("arm64: Honor __GFP_ZERO in dma allocations"). Has something broken that?

The code for handling DMA_ATTR_FORCE_CONTIGUOUS flag in arm64 dma-mapping/iommu
implementation has been added later and assumed that __GFP_ZERO flag is handled by dma_alloc_from_contiguous(). This is sadly not true, so the buffer
allocated this way is not cleared.

In case of ARM (32bit) the newly allocated buffer is always cleared directly after calling dma_alloc_from_contiguous() function.

I agree that adding handling of __GFP_ZERO flag to dma_alloc_from_contiguous()
or rather cma_alloc() is a better idea, but might have some side effects, so such change is not a good candidate for -rc cycle. I will prepare such patch,
but for now this one is imho a bit less invasive approach.

Best regards

Signature · 2 lines

Marek Szyprowski, PhD
Samsung R&D Institute Poland

Geert Uytterhoeven

2018-06-12 07:36 UTC #5 lore raw

PATCH 1/1 arm64: dma-mapping: clear buffers allocated with FORCE_CONTIGUOUS flag

Hi Marek,

Quoted 31 lines · Marek Szyprowski

Quoted 16 lines · Will Deacon
Quoted 15 lines · Geert Uytterhoeven
Quoted 4 lines · Marek Szyprowski
dma_alloc_*() buffers might be exposed to userspace via mmap() call, so they should be cleared on allocation. In case of IOMMU-based dma-mapping implementation such buffer clearing was missing in the code path for DMA_ATTR_FORCE_CONTIGUOUS flag handling. This patch fixes this issue. For
Is it? The memory is allocated using dma_alloc_from_contiguous(..., gfp), and __iommu_alloc_attrs() has
         /*
          * Some drivers rely on this, and we probably don't want the
          * possibility of stale kernel data being read by devices anyway.
          */
         gfp |= __GFP_ZERO;
Quoted 6 lines
Quoted 4 lines
at the top, before the allocation.
If cma_alloc() (called from dma_alloc_from_contiguous()) doesn't honor __GFP_ZERO, I think cma_alloc() should be fixed instead.
Agreed. We tried to fix this in 7132813c3845 ("arm64: Honor __GFP_ZERO in dma allocations"). Has something broken that?
The code for handling DMA_ATTR_FORCE_CONTIGUOUS flag in arm64 dma-mapping/iommu
implementation has been added later and assumed that __GFP_ZERO flag is handled by dma_alloc_from_contiguous(). This is sadly not true, so the buffer
allocated this way is not cleared.

Quoted 2 lines

In case of ARM (32bit) the newly allocated buffer is always cleared directly after calling dma_alloc_from_contiguous() function.

JFTR, mips and generic dma_direct_alloc() also do that, extensa does not.

Quoted 6 lines

I agree that adding handling of __GFP_ZERO flag to dma_alloc_from_contiguous()
or rather cma_alloc() is a better idea, but might have some side effects, so such change is not a good candidate for -rc cycle. I will prepare such patch,
but for now this one is imho a bit less invasive approach.

OK, fine for me.
Thanks!

Gr{oetje,eeting}s,

Geert

Signature · 5 lines

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds