[PATCH 1/2] mm: cma: split out in_cma check to separate function

[PATCH 1/2] mm: cma: split out in_cma check to separate function

[Rabin Vincent]

Split out the logic in cma_release() which checks if the page is in the
contiguous area to a new function which can be called separately.  ARM
will use this.

Signed-off-by: Rabin Vincent <rabin.vincent@axis.com>
---
 include/linux/cma.h | 12 ++++++++++++
 mm/cma.c            | 27 +++++++++++++++++++--------
 2 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/include/linux/cma.h b/include/linux/cma.h
index 29f9e77..6e7fd2d 100644
--- a/include/linux/cma.h
+++ b/include/linux/cma.h
@@ -27,5 +27,17 @@ extern int cma_init_reserved_mem(phys_addr_t base, phys_addr_t size,
 					unsigned int order_per_bit,
 					struct cma **res_cma);
 extern struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align);
+
 extern bool cma_release(struct cma *cma, const struct page *pages, unsigned int count);
+#ifdef CONFIG_CMA
+extern bool in_cma(struct cma *cma, const struct page *pages,
+		   unsigned int count);
+#else
+static inline bool in_cma(struct cma *cma, const struct page *pages,
+			  unsigned int count)
+{
+	return false;
+}
+#endif
+
 #endif
diff --git a/mm/cma.c b/mm/cma.c
index ea506eb..55cda16 100644
--- a/mm/cma.c
+++ b/mm/cma.c
@@ -426,6 +426,23 @@ struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align)
 	return page;
 }
 
+bool in_cma(struct cma *cma, const struct page *pages, unsigned int count)
+{
+	unsigned long pfn;
+
+	if (!cma || !pages)
+		return false;
+
+	pfn = page_to_pfn(pages);
+
+	if (pfn < cma->base_pfn || pfn >= cma->base_pfn + cma->count)
+		return false;
+
+	VM_BUG_ON(pfn + count > cma->base_pfn + cma->count);
+
+	return true;
+}
+
 /**
  * cma_release() - release allocated pages
  * @cma:   Contiguous memory region for which the allocation is performed.
@@ -440,18 +457,12 @@ bool cma_release(struct cma *cma, const struct page *pages, unsigned int count)
 {
 	unsigned long pfn;
 
-	if (!cma || !pages)
-		return false;
-
 	pr_debug("%s(page %p)\n", __func__, (void *)pages);
 
-	pfn = page_to_pfn(pages);
-
-	if (pfn < cma->base_pfn || pfn >= cma->base_pfn + cma->count)
+	if (!in_cma(cma, pages, count))
 		return false;
 
-	VM_BUG_ON(pfn + count > cma->base_pfn + cma->count);
-
+	pfn = page_to_pfn(pages);
 	free_contig_range(pfn, count);
 	cma_clear_bitmap(cma, pfn, count);
 	trace_cma_release(pfn, pages, count);
-- 
2.7.0

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

[Rabin Vincent]

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

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

[Michal Nazarewicz]

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>

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

[Rabin Vincent]

On Fri, Feb 19, 2016 at 02:50:52PM +0100, Michal Nazarewicz wrote:
> 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
> 	}

The page tables changes need to be done before we release the area with
cma_release().  With the v2 patchset which I've sent to LAKML we won't
need a new in_cma() function since we'll now record how we allocated the
buffer and use this information in the free routine.

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

[Russell King - ARM Linux]

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.

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

[Rabin Vincent]

On Fri, Feb 19, 2016 at 02:06:00PM +0000, Russell King - ARM Linux wrote:
> 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.

I've sent a v2 of this series which refactors the code so that we no
longer have two independent sets of tests.

Re: [PATCH 1/2] mm: cma: split out in_cma check to separate function

[Michal Nazarewicz]

On Fri, Feb 19 2016, Rabin Vincent wrote:
> Split out the logic in cma_release() which checks if the page is in the
> contiguous area to a new function which can be called separately.  ARM
> will use this.
>
> Signed-off-by: Rabin Vincent <rabin.vincent@axis.com>
> ---
>  include/linux/cma.h | 12 ++++++++++++
>  mm/cma.c            | 27 +++++++++++++++++++--------
>  2 files changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/cma.h b/include/linux/cma.h
> index 29f9e77..6e7fd2d 100644
> --- a/include/linux/cma.h
> +++ b/include/linux/cma.h
> @@ -27,5 +27,17 @@ extern int cma_init_reserved_mem(phys_addr_t base, phys_addr_t size,
>  					unsigned int order_per_bit,
>  					struct cma **res_cma);
>  extern struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align);
> +
>  extern bool cma_release(struct cma *cma, const struct page *pages, unsigned int count);
> +#ifdef CONFIG_CMA
> +extern bool in_cma(struct cma *cma, const struct page *pages,
> +		   unsigned int count);
> +#else
> +static inline bool in_cma(struct cma *cma, const struct page *pages,
> +			  unsigned int count)
> +{
> +	return false;
> +}
> +#endif
> +
>  #endif
> diff --git a/mm/cma.c b/mm/cma.c
> index ea506eb..55cda16 100644
> --- a/mm/cma.c
> +++ b/mm/cma.c
> @@ -426,6 +426,23 @@ struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align)
>  	return page;
>  }
>  
> +bool in_cma(struct cma *cma, const struct page *pages, unsigned int count)

Should it instead take pfn as an argument instead of a page?  IIRC
page_to_pfn may be expensive on some architectures and with this patch,
cma_release will call it twice.

Or maybe in_cma could return a pfn, something like (error checking
stripped):

unsigned long pfn in_cma(struct cma *cma, const struct page *page,
			 unsgined count)
{
	unsigned long pfn = page_to_pfn(page);
	if (pfn < cma->base_pfn || pfn >= cma->base_pfn + cma->count)
		return 0;
	VM_BUG_ON(pfn + count > cma->base_pfn + cma->count);
	return pfn;
}

Is pfn == 0 guaranteed to be invalid?

> +{
> +	unsigned long pfn;
> +
> +	if (!cma || !pages)
> +		return false;
> +
> +	pfn = page_to_pfn(pages);
> +
> +	if (pfn < cma->base_pfn || pfn >= cma->base_pfn + cma->count)
> +		return false;
> +
> +	VM_BUG_ON(pfn + count > cma->base_pfn + cma->count);
> +
> +	return true;
> +}
> +
>  /**
>   * cma_release() - release allocated pages
>   * @cma:   Contiguous memory region for which the allocation is performed.
> @@ -440,18 +457,12 @@ bool cma_release(struct cma *cma, const struct page *pages, unsigned int count)
>  {
>  	unsigned long pfn;
>  
> -	if (!cma || !pages)
> -		return false;
> -
>  	pr_debug("%s(page %p)\n", __func__, (void *)pages);
>  
> -	pfn = page_to_pfn(pages);
> -
> -	if (pfn < cma->base_pfn || pfn >= cma->base_pfn + cma->count)
> +	if (!in_cma(cma, pages, count))
>  		return false;
>  
> -	VM_BUG_ON(pfn + count > cma->base_pfn + cma->count);
> -
> +	pfn = page_to_pfn(pages);
>  	free_contig_range(pfn, count);
>  	cma_clear_bitmap(cma, pfn, count);
>  	trace_cma_release(pfn, pages, count);
> -- 
> 2.7.0
>

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

Re: [PATCH 1/2] mm: cma: split out in_cma check to separate function

[Andrew Morton]

On Fri, 19 Feb 2016 09:12:03 +0100 Rabin Vincent <rabin.vincent@axis.com> wrote:

> Split out the logic in cma_release() which checks if the page is in the
> contiguous area to a new function which can be called separately.  ARM
> will use this.
> 
> ...
>
> --- a/include/linux/cma.h
> +++ b/include/linux/cma.h
> @@ -27,5 +27,17 @@ extern int cma_init_reserved_mem(phys_addr_t base, phys_addr_t size,
>  					unsigned int order_per_bit,
>  					struct cma **res_cma);
>  extern struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align);
> +
>  extern bool cma_release(struct cma *cma, const struct page *pages, unsigned int count);
> +#ifdef CONFIG_CMA
> +extern bool in_cma(struct cma *cma, const struct page *pages,
> +		   unsigned int count);
> +#else
> +static inline bool in_cma(struct cma *cma, const struct page *pages,
> +			  unsigned int count)
> +{
> +	return false;
> +}
> +#endif

Calling it "pages" is weird.  I immediately read it as a `struct page **'. 
Drop the 's' please.  Or call it `start_page' if you wish to retain the
"we're dealing with more than one page here" info.

And `nr_pages' is a better name than `count'.  

And `in_cma' seems rather ...  brief.  And it breaks the convention that
interface identifiers start with the name of the subsystem.  Look at the rest
of cma.h: cma_get_base(), cma_get_size() cma_declare_contiguous(), etc -
let's not break that.

>  #endif
> diff --git a/mm/cma.c b/mm/cma.c
> index ea506eb..55cda16 100644
> --- a/mm/cma.c
> +++ b/mm/cma.c
> @@ -426,6 +426,23 @@ struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align)
>  	return page;
>  }
>  
> +bool in_cma(struct cma *cma, const struct page *pages, unsigned int count)

A bit of documentation would be nice.

> +{
> +	unsigned long pfn;
> +
> +	if (!cma || !pages)
> +		return false;

Is this actually needed?  If there's no good reason for the test, let's leave
it out because it will just be hiding bugs in the caller.

> +	pfn = page_to_pfn(pages);
> +
> +	if (pfn < cma->base_pfn || pfn >= cma->base_pfn + cma->count)
> +		return false;
> +
> +	VM_BUG_ON(pfn + count > cma->base_pfn + cma->count);
> +
> +	return true;
> +}
> +
>  /**
>   * cma_release() - release allocated pages
>   * @cma:   Contiguous memory region for which the allocation is performed.

Apart from those cosmeticish things, no objections from me.