linux-kernel - Re: [PATCH] staging: android: ion: refactory ion

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]

Date:   Fri, 29 Mar 2019 06:27:17 -0700
From:   Laura Abbott <labbott@...hat.com>
To:     Zeng Tao <prime.zeng@...ilicon.com>, sumit.semwal@...aro.org
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Arve Hjønnevåg <arve@...roid.com>,
        Todd Kjos <tkjos@...roid.com>,
        Martijn Coenen <maco@...roid.com>,
        Joel Fernandes <joel@...lfernandes.org>,
        Christian Brauner <christian@...uner.io>,
        devel@...verdev.osuosl.org, dri-devel@...ts.freedesktop.org,
        linaro-mm-sig@...ts.linaro.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: android: ion: refactory ion_alloc for kernel
 driver use

On 3/29/19 11:40 AM, Zeng Tao wrote:
> There are two reasons for this patch:
> 1. There are some potential requirements for ion_alloc in kernel space,
> some media drivers need to allocate media buffers from ion instead of
> buddy or dma framework, this is more convient and clean very for media
> drivers. And In that case, ion is the only media buffer provider, it's
> more easier to maintain.
> 2. Fd is only needed by user processes, not the kernel space, so dma_buf
> should be returned instead of fd for kernel space, and dma_buf_fd should
> be called only for userspace api.
> 

I really want to just NAK this because it doesn't seem like something
that's necessary. The purpose of Ion is to provide buffers to userspace
because there's no other way for userspace to get access to the memory.
The kernel already has other APIs to access the memory. This also
complicates the re-work that's been happening where the requirement
is only userspace.

Can you be more detailed about which media drivers you are referring
to and why they can't just use other APIs?


> Signed-off-by: Zeng Tao <prime.zeng@...ilicon.com>
> ---
>   drivers/staging/android/ion/ion.c | 32 +++++++++++++++++---------------
>   1 file changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> index 92c2914..e93fb49 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -387,13 +387,13 @@ static const struct dma_buf_ops dma_buf_ops = {
>   	.unmap = ion_dma_buf_kunmap,
>   };
>   
> -static int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int flags)
> +struct dma_buf *ion_alloc(size_t len, unsigned int heap_id_mask,
> +			  unsigned int flags)
>   {
>   	struct ion_device *dev = internal_dev;
>   	struct ion_buffer *buffer = NULL;
>   	struct ion_heap *heap;
>   	DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
> -	int fd;
>   	struct dma_buf *dmabuf;
>   
>   	pr_debug("%s: len %zu heap_id_mask %u flags %x\n", __func__,
> @@ -407,7 +407,7 @@ static int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int flags)
>   	len = PAGE_ALIGN(len);
>   
>   	if (!len)
> -		return -EINVAL;
> +		return ERR_PTR(-EINVAL);
>   
>   	down_read(&dev->lock);
>   	plist_for_each_entry(heap, &dev->heaps, node) {
> @@ -421,10 +421,10 @@ static int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int flags)
>   	up_read(&dev->lock);
>   
>   	if (!buffer)
> -		return -ENODEV;
> +		return ERR_PTR(-ENODEV);
>   
>   	if (IS_ERR(buffer))
> -		return PTR_ERR(buffer);
> +		return ERR_PTR(PTR_ERR(buffer));
>   
>   	exp_info.ops = &dma_buf_ops;
>   	exp_info.size = buffer->size;
> @@ -432,17 +432,12 @@ static int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int flags)
>   	exp_info.priv = buffer;
>   
>   	dmabuf = dma_buf_export(&exp_info);
> -	if (IS_ERR(dmabuf)) {
> +	if (IS_ERR(dmabuf))
>   		_ion_buffer_destroy(buffer);
> -		return PTR_ERR(dmabuf);
> -	}
>   
> -	fd = dma_buf_fd(dmabuf, O_CLOEXEC);
> -	if (fd < 0)
> -		dma_buf_put(dmabuf);
> -
> -	return fd;
> +	return dmabuf;
>   }
> +EXPORT_SYMBOL(ion_alloc);
>   
>   static int ion_query_heaps(struct ion_heap_query *query)
>   {
> @@ -539,12 +534,19 @@ static long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>   	case ION_IOC_ALLOC:
>   	{
>   		int fd;
> +		struct dma_buf *dmabuf;
>   
> -		fd = ion_alloc(data.allocation.len,
> +		dmabuf = ion_alloc(data.allocation.len,
>   			       data.allocation.heap_id_mask,
>   			       data.allocation.flags);
> -		if (fd < 0)
> +		if (IS_ERR(dmabuf))
> +			return PTR_ERR(dmabuf);
> +
> +		fd = dma_buf_fd(dmabuf, O_CLOEXEC);
> +		if (fd < 0) {
> +			dma_buf_put(dmabuf);
>   			return fd;
> +		}
>   
>   		data.allocation.fd = fd;
>   
>