linux-kernel - Re: [PATCHv2] staging: android: ion: use the manged version of DMA memory allocation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <56B1ECAD.203@arm.com>
Date:	Wed, 3 Feb 2016 12:03:57 +0000
From:	Robin Murphy <robin.murphy@....com>
To:	"Gujulan Elango, Hari Prasath (H.)" <hgujulan@...teon.com>
Cc:	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"arve@...roid.com" <arve@...roid.com>,
	"riandrews@...roid.com" <riandrews@...roid.com>,
	"tixy@...aro.org" <tixy@...aro.org>,
	"sriram@...irs.net.in" <sriram@...irs.net.in>,
	"devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Laura Abbott <labbott@...hat.com>
Subject: Re: [PATCHv2] staging: android: ion: use the manged version of DMA
 memory allocation

On 03/02/16 06:49, Gujulan Elango, Hari Prasath (H.) wrote:
> From: Hari Prasath Gujulan Elango <hgujulan@...teon.com>
>
> Use the managed version of the dma_alloc_coherent() i.e. the
> dmam_alloc_coherent() & accordingly cleanup the error handling
> part.Also,remove the references to dma_free_coherent

That last aspect looks a bit off to me - the heaps don't seem to be 
something that exist for the lifetime of the ION "device", given that 
these are specific runtime alloc and free calls, rather than the probe 
and remove routines. I don't know if CMA heaps are among those which ION 
creates and destroys frequently (enough that it apparently kicks off a 
whole background thread to manage the freeing), but this looks like a 
recipe for leaks. If the free call doesn't actually free the buffer, 
it's going to remain hanging around consuming precious CMA area until 
the ION device itself is torn down, which is likely never.

I wouldn't say it's necessarily inappropriate to use managed DMA 
resources here to cover unexpected failure cases for the ION device 
itself (I see the comment in ion_device_remove()), but that means still 
using dmam_free_coherent() when naturally releasing allocations for 
other reasons (i.e. both cases here). Think Java finalisers, rather than 
C++ destructors.

Robin.

> Signed-off-by: Hari Prasath Gujulan Elango <hgujulan@...teon.com>
> ---
> 	v2:kbuild test robot reported warnings on ununsed
> 	   variables.Those warnings are fixed.
> ---
>   drivers/staging/android/ion/ion_cma_heap.c | 11 ++---------
>   1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/android/ion/ion_cma_heap.c b/drivers/staging/android/ion/ion_cma_heap.c
> index a3446da..8cd720b 100644
> --- a/drivers/staging/android/ion/ion_cma_heap.c
> +++ b/drivers/staging/android/ion/ion_cma_heap.c
> @@ -61,7 +61,7 @@ static int ion_cma_allocate(struct ion_heap *heap, struct ion_buffer *buffer,
>   	if (!info)
>   		return ION_CMA_ALLOCATE_FAILED;
>
> -	info->cpu_addr = dma_alloc_coherent(dev, len, &(info->handle),
> +	info->cpu_addr = dmam_alloc_coherent(dev, len, &(info->handle),
>   						GFP_HIGHUSER | __GFP_ZERO);
>
>   	if (!info->cpu_addr) {
> @@ -71,7 +71,7 @@ static int ion_cma_allocate(struct ion_heap *heap, struct ion_buffer *buffer,
>
>   	info->table = kmalloc(sizeof(struct sg_table), GFP_KERNEL);
>   	if (!info->table)
> -		goto free_mem;
> +		goto err;
>
>   	if (dma_get_sgtable(dev, info->table, info->cpu_addr, info->handle,
>   			    len))
> @@ -83,8 +83,6 @@ static int ion_cma_allocate(struct ion_heap *heap, struct ion_buffer *buffer,
>
>   free_table:
>   	kfree(info->table);
> -free_mem:
> -	dma_free_coherent(dev, len, info->cpu_addr, info->handle);
>   err:
>   	kfree(info);
>   	return ION_CMA_ALLOCATE_FAILED;
> @@ -92,13 +90,8 @@ err:
>
>   static void ion_cma_free(struct ion_buffer *buffer)
>   {
> -	struct ion_cma_heap *cma_heap = to_cma_heap(buffer->heap);
> -	struct device *dev = cma_heap->dev;
>   	struct ion_cma_buffer_info *info = buffer->priv_virt;
>
> -	dev_dbg(dev, "Release buffer %p\n", buffer);
> -	/* release memory */
> -	dma_free_coherent(dev, buffer->size, info->cpu_addr, info->handle);
>   	/* release sg table */
>   	sg_free_table(info->table);
>   	kfree(info->table);
>