Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1337957 > unrolled thread

[PATCH 2/2] ARM: dma-mapping: fix alloc/free for coherent + CMA + gfp=0

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/2] ARM: dma-mapping: fix alloc/free for coherent + CMA + gfp=0 Rabin Vincent <rabin.vincent@axis.com> - 2016-02-19 09:20 +0100
    Re: [PATCH 2/2] ARM: dma-mapping: fix alloc/free for coherent + CMA + gfp=0 Michal Nazarewicz <mina86@mina86.com> - 2016-02-19 15:00 +0100
    Re: [PATCH 2/2] ARM: dma-mapping: fix alloc/free for coherent + CMA  + gfp=0 Russell King - ARM Linux <linux@arm.linux.org.uk> - 2016-02-19 15:10 +0100

#1337957 — [PATCH 2/2] ARM: dma-mapping: fix alloc/free for coherent + CMA + gfp=0

Given a device which uses arm_coherent_dma_ops and on which
dev_get_cma_area(dev) returns non-NULL, the following usage of the DMA
API with gfp=0 results in a memory leak and memory corruption.

 p = dma_alloc_coherent(dev, sz, &dma, 0);
 if (p)
 	dma_free_coherent(dev, sz, p, dma);

The memory leak is because the alloc allocates using
__alloc_simple_buffer() but the free attempts
dma_release_from_contiguous(), which does not do free anything since the
page is not in the CMA area.

The memory corruption is because the free calls __dma_remap() on a page
which is backed by only first level page tables.  The
apply_to_page_range() + __dma_update_pte() loop ends up interpreting the
section mapping as the address to a second level page table and writing
the new PTE to memory which is not used by page tables.

We don't have access to the GFP flags used for allocation in the free
function, so fix it by using the new in_cma() function to determine if a
buffer was allocated with CMA, similar to how we check for
__in_atomic_pool().

Fixes: 21caf3a7 ("ARM: 8398/1: arm DMA: Fix allocation from CMA for coherent DMA")
Signed-off-by: Rabin Vincent <rabin.vincent@axis.com>
---
 arch/arm/mm/dma-mapping.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 0eca381..a4592c7 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -749,16 +749,16 @@ static void __arm_dma_free(struct device *dev, size_t size, void *cpu_addr,
 		__dma_free_buffer(page, size);
 	} else if (!is_coherent && __free_from_pool(cpu_addr, size)) {
 		return;
-	} else if (!dev_get_cma_area(dev)) {
-		if (want_vaddr && !is_coherent)
-			__dma_free_remap(cpu_addr, size);
-		__dma_free_buffer(page, size);
-	} else {
+	} else if (in_cma(dev_get_cma_area(dev), page, size >> PAGE_SHIFT)) {
 		/*
 		 * Non-atomic allocations cannot be freed with IRQs disabled
 		 */
 		WARN_ON(irqs_disabled());
 		__free_from_contiguous(dev, page, cpu_addr, size, want_vaddr);
+	} else {
+		if (want_vaddr && !is_coherent)
+			__dma_free_remap(cpu_addr, size);
+		__dma_free_buffer(page, size);
 	}
 }
 
-- 
2.7.0

[toc] | [next] | [standalone]

#1338204

On Fri, Feb 19 2016, Rabin Vincent wrote:
> Given a device which uses arm_coherent_dma_ops and on which
> dev_get_cma_area(dev) returns non-NULL, the following usage of the DMA
> API with gfp=0 results in a memory leak and memory corruption.
>
>  p = dma_alloc_coherent(dev, sz, &dma, 0);
>  if (p)
>  	dma_free_coherent(dev, sz, p, dma);
>
> The memory leak is because the alloc allocates using
> __alloc_simple_buffer() but the free attempts
> dma_release_from_contiguous(), which does not do free anything since the
> page is not in the CMA area.
>
> The memory corruption is because the free calls __dma_remap() on a page
> which is backed by only first level page tables.  The
> apply_to_page_range() + __dma_update_pte() loop ends up interpreting the
> section mapping as the address to a second level page table and writing
> the new PTE to memory which is not used by page tables.
>
> We don't have access to the GFP flags used for allocation in the free
> function, so fix it by using the new in_cma() function to determine if a
> buffer was allocated with CMA, similar to how we check for
> __in_atomic_pool().
>
> Fixes: 21caf3a7 ("ARM: 8398/1: arm DMA: Fix allocation from CMA for coherent DMA")
> Signed-off-by: Rabin Vincent <rabin.vincent@axis.com>
> ---
>  arch/arm/mm/dma-mapping.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 0eca381..a4592c7 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -749,16 +749,16 @@ static void __arm_dma_free(struct device *dev, size_t size, void *cpu_addr,
>  		__dma_free_buffer(page, size);
>  	} else if (!is_coherent && __free_from_pool(cpu_addr, size)) {
>  		return;
> -	} else if (!dev_get_cma_area(dev)) {
> -		if (want_vaddr && !is_coherent)
> -			__dma_free_remap(cpu_addr, size);
> -		__dma_free_buffer(page, size);
> -	} else {
> +	} else if (in_cma(dev_get_cma_area(dev), page, size >> PAGE_SHIFT)) {
>  		/*
>  		 * Non-atomic allocations cannot be freed with IRQs disabled
>  		 */
>  		WARN_ON(irqs_disabled());
>  		__free_from_contiguous(dev, page, cpu_addr, size, want_vaddr);
> +	} else {
> +		if (want_vaddr && !is_coherent)
> +			__dma_free_remap(cpu_addr, size);
> +		__dma_free_buffer(page, size);
>  	}
>  }

I haven’t looked closely at the code, but why not:

	struct cma *cma = 
        if (!cma_release(dev_get_cma_area(dev), page, size >> PAGE_SHIFT)) {
		// ... do whatever other non-CMA free
	}

-- 
Best regards
Liege of Serenely Enlightened Majesty of Computer Science,
ミハウ “mina86” ナザレヴイツ  <mpn@google.com> <xmpp:mina86@jabber.org>

[toc] | [prev] | [next] | [standalone]

#1338211 — Re: [PATCH 2/2] ARM: dma-mapping: fix alloc/free for coherent + CMA + gfp=0

On Fri, Feb 19, 2016 at 09:12:04AM +0100, Rabin Vincent wrote:
> Given a device which uses arm_coherent_dma_ops and on which
> dev_get_cma_area(dev) returns non-NULL, the following usage of the DMA
> API with gfp=0 results in a memory leak and memory corruption.
> 
>  p = dma_alloc_coherent(dev, sz, &dma, 0);
>  if (p)
>  	dma_free_coherent(dev, sz, p, dma);
> 
> The memory leak is because the alloc allocates using
> __alloc_simple_buffer() but the free attempts
> dma_release_from_contiguous(), which does not do free anything since the
> page is not in the CMA area.

I'd really like to see a better solution to this problem: over the course
of the years, I've seen a number of patches that rearrange the test order
at allocation time because of some problem or the other.

What we need is a better way to ensure that we use the correct release
functionality - having two independent set of tests where the order
matters is really not very good.

Maybe someone can put some thought into this...

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web