Re: [Linaro-mm-sig] [PATCH v3] staging: android: ion: Zero CMA allocated memory

[Laura Abbott]

On 01/26/2018 06:04 PM, Chen Feng wrote:
> On 2018/1/27 1:48, Liam Mark wrote:
> > Since commit 204f672255c2 ("staging: android: ion: Use CMA APIs directly")
> > the CMA API is now used directly and therefore the allocated memory is no
> > longer automatically zeroed.
> >
> > Explicitly zero CMA allocated memory to ensure that no data is exposed to
> > userspace.
> >
> > Fixes: 204f672255c2 ("staging: android: ion: Use CMA APIs directly")
> > Signed-off-by: Liam Mark <lmark@xxxxxxxxxxxxxx>
> > ---
> > Changes in v2:
> >    - Clean up the commit message.
> >    - Add 'Fixes:'
> >
> > Changes in v3:
> >   - Add support for highmem pages
> >
> >   drivers/staging/android/ion/ion_cma_heap.c | 17 +++++++++++++++++
> >   1 file changed, 17 insertions(+)
> >
> > diff --git a/drivers/staging/android/ion/ion_cma_heap.c b/drivers/staging/android/ion/ion_cma_heap.c
> > index 86196ffd2faf..fa3e4b7e0c9f 100644
> > --- a/drivers/staging/android/ion/ion_cma_heap.c
> > +++ b/drivers/staging/android/ion/ion_cma_heap.c
> > @@ -21,6 +21,7 @@
> >   #include <linux/err.h>
> >   #include <linux/cma.h>
> >   #include <linux/scatterlist.h>
> > +#include <linux/highmem.h>
> >   
> >   #include "ion.h"
> >   
> > @@ -51,6 +52,22 @@ static int ion_cma_allocate(struct ion_heap *heap, struct ion_buffer *buffer,
> >   	if (!pages)
> >   		return -ENOMEM;
> >   
> > +	if (PageHighMem(pages)) {
> > +		unsigned long nr_clear_pages = nr_pages;
> > +		struct page *page = pages;
> > +
> > +		while (nr_clear_pages > 0) {
> > +			void *vaddr = kmap_atomic(page);
> > +
> > +			memset(vaddr, 0, PAGE_SIZE);
> > +			kunmap_atomic(vaddr);
>
> Here. This way may cause performance latency at mapping-memset-umap page one bye one.
>
> Take a look at ion_heap_pages_zero.
>
> Not very critical, arm64 always have linear mapping.

This is under a PageHighMem check so arm64 isn't affected. It's also
the same algorithm arm32 dma-mapping.c uses so I'd like to see some
data about the performance improvement before we go changing things
too much.

Thanks,
Laura

> > +			page++;
> > +			nr_clear_pages--;
> > +		}
> > +	} else {
> > +		memset(page_address(pages), 0, size);
> > +	}
> > +
> >   	table = kmalloc(sizeof(*table), GFP_KERNEL);
> >   	if (!table)
> >   		goto err;