linux-kernel - Re: [RFC PATCHv2 3/3] drm/vgem: Enable dmabuf import interfaces

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: <20170427073119.GW11432@nuc-i3427.alporthouse.com>
Date:   Thu, 27 Apr 2017 08:31:19 +0100
From:   Chris Wilson <chris@...is-wilson.co.uk>
To:     Laura Abbott <labbott@...hat.com>
Cc:     Daniel Vetter <daniel.vetter@...ll.ch>,
        Sean Paul <seanpaul@...omium.org>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>
Subject: Re: [RFC PATCHv2 3/3] drm/vgem: Enable dmabuf import interfaces

On Wed, Apr 26, 2017 at 02:12:30PM -0700, Laura Abbott wrote:
> Enable the GEM dma-buf import interfaces in addition to the export
> interfaces. This lets vgem be used as a test source for other allocators
> (e.g. Ion).
> 
> Signed-off-by: Laura Abbott <labbott@...hat.com>
> ---
> v2: Don't require vgem allocated buffers to existing in memory before importing.
> ---
>  drivers/gpu/drm/vgem/vgem_drv.c | 133 +++++++++++++++++++++++++++++++---------
>  drivers/gpu/drm/vgem/vgem_drv.h |   2 +
>  2 files changed, 106 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
> index 1b02e56..73a619a 100644
> --- a/drivers/gpu/drm/vgem/vgem_drv.c
> +++ b/drivers/gpu/drm/vgem/vgem_drv.c
> @@ -34,6 +34,9 @@
>  #include <linux/ramfs.h>
>  #include <linux/shmem_fs.h>
>  #include <linux/dma-buf.h>
> +
> +#include <drm/drmP.h>
> +
>  #include "vgem_drv.h"
>  
>  #define DRIVER_NAME	"vgem"
> @@ -46,6 +49,12 @@ static void vgem_gem_free_object(struct drm_gem_object *obj)
>  {
>  	struct drm_vgem_gem_object *vgem_obj = to_vgem_bo(obj);
>  
> +	if (vgem_obj->pages)
> +		drm_free_large(vgem_obj->pages);

Just drm_free_large(vgem_obj->pages); NULL -> kfree() which is NULL safe.

The series lgtm, doesn't compromise on any of the existing tests.

All 3,
Reviewed-by: Chris Wilson <chris@...is-wilson.co.uk>

You could always get Joonas to nitpick over style...

> +
> +	if (obj->import_attach)
> +		drm_prime_gem_destroy(obj, vgem_obj->table);
> +
>  	drm_gem_object_release(obj);
>  	kfree(vgem_obj);
>  }
> @@ -56,26 +65,48 @@ static int vgem_gem_fault(struct vm_fault *vmf)
>  	struct drm_vgem_gem_object *obj = vma->vm_private_data;
>  	/* We don't use vmf->pgoff since that has the fake offset */
>  	unsigned long vaddr = vmf->address;
> -	struct page *page;
> -
> -	page = shmem_read_mapping_page(file_inode(obj->base.filp)->i_mapping,
> -				       (vaddr - vma->vm_start) >> PAGE_SHIFT);
> -	if (!IS_ERR(page)) {
> -		vmf->page = page;
> -		return 0;
> -	} else switch (PTR_ERR(page)) {
> -		case -ENOSPC:
> -		case -ENOMEM:
> -			return VM_FAULT_OOM;
> -		case -EBUSY:
> -			return VM_FAULT_RETRY;
> -		case -EFAULT:
> -		case -EINVAL:
> -			return VM_FAULT_SIGBUS;
> -		default:
> -			WARN_ON_ONCE(PTR_ERR(page));
> -			return VM_FAULT_SIGBUS;
> +	int ret;
> +	loff_t num_pages;
> +	pgoff_t page_offset;
> +	page_offset = (vaddr - vma->vm_start) >> PAGE_SHIFT;
> +
> +	num_pages = DIV_ROUND_UP(obj->base.size, PAGE_SIZE);
> +
> +	if (page_offset > num_pages)
> +		return VM_FAULT_SIGBUS;
> +
> +	if (obj->pages) {
> +		get_page(obj->pages[page_offset]);
> +		vmf->page = obj->pages[page_offset];
> +		ret = 0;
> +	} else {
> +		struct page *page;
> +
> +		page = shmem_read_mapping_page(
> +					file_inode(obj->base.filp)->i_mapping,
> +					page_offset);
> +		if (!IS_ERR(page)) {
> +			vmf->page = page;
> +			ret = 0;
> +		} else switch (PTR_ERR(page)) {
> +			case -ENOSPC:
> +			case -ENOMEM:
> +				ret = VM_FAULT_OOM;
> +				break;
> +			case -EBUSY:
> +				ret = VM_FAULT_RETRY;
> +				break;
> +			case -EFAULT:
> +			case -EINVAL:
> +				ret = VM_FAULT_SIGBUS;
> +				break;
> +			default:
> +				WARN_ON(PTR_ERR(page));
> +				ret = VM_FAULT_SIGBUS;

break;
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre