[PATCH 01/10] udmabuf: sort headers, drop uapi/ path prefix

[PATCH 01/10] udmabuf: sort headers, drop uapi/ path prefix

[Gerd Hoffmann]

Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/dma-buf/udmabuf.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index 2e8502250a..e63d301bcb 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -1,17 +1,16 @@
 // SPDX-License-Identifier: GPL-2.0
-#include <linux/init.h>
-#include <linux/module.h>
+#include <linux/cred.h>
 #include <linux/device.h>
-#include <linux/kernel.h>
-#include <linux/slab.h>
-#include <linux/miscdevice.h>
 #include <linux/dma-buf.h>
 #include <linux/highmem.h>
-#include <linux/cred.h>
-#include <linux/shmem_fs.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
 #include <linux/memfd.h>
-
-#include <uapi/linux/udmabuf.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/shmem_fs.h>
+#include <linux/udmabuf.h>
 
 struct udmabuf {
 	u32 pagecount;
-- 
2.9.3

Re: [PATCH 01/10] udmabuf: sort headers, drop uapi/ path prefix

[Laurent Pinchart]

Hi Gerd,

Thank you for the patch.

On Tuesday, 11 September 2018 09:59:12 EEST Gerd Hoffmann wrote:
> Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  drivers/dma-buf/udmabuf.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index 2e8502250a..e63d301bcb 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -1,17 +1,16 @@
>  // SPDX-License-Identifier: GPL-2.0
> -#include <linux/init.h>
> -#include <linux/module.h>
> +#include <linux/cred.h>
>  #include <linux/device.h>
> -#include <linux/kernel.h>
> -#include <linux/slab.h>
> -#include <linux/miscdevice.h>
>  #include <linux/dma-buf.h>
>  #include <linux/highmem.h>
> -#include <linux/cred.h>
> -#include <linux/shmem_fs.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
>  #include <linux/memfd.h>
> -
> -#include <uapi/linux/udmabuf.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/shmem_fs.h>

Nearly there, l comes after h :-)

Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +#include <linux/udmabuf.h>
> 
>  struct udmabuf {
>  	u32 pagecount;


-- 
Regards,

Laurent Pinchart

[PATCH 02/10] udmabuf: improve map_udmabuf error handling

[Gerd Hoffmann]

Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/dma-buf/udmabuf.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index e63d301bcb..19bd918209 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -51,25 +51,24 @@ static struct sg_table *map_udmabuf(struct dma_buf_attachment *at,
 {
 	struct udmabuf *ubuf = at->dmabuf->priv;
 	struct sg_table *sg;
+	int ret;
 
 	sg = kzalloc(sizeof(*sg), GFP_KERNEL);
 	if (!sg)
-		goto err1;
-	if (sg_alloc_table_from_pages(sg, ubuf->pages, ubuf->pagecount,
-				      0, ubuf->pagecount << PAGE_SHIFT,
-				      GFP_KERNEL) < 0)
-		goto err2;
+		return ERR_PTR(-ENOMEM);
+	ret = sg_alloc_table_from_pages(sg, ubuf->pages, ubuf->pagecount,
+					0, ubuf->pagecount << PAGE_SHIFT,
+					GFP_KERNEL);
+	if (ret < 0)
+		goto err;
 	if (!dma_map_sg(at->dev, sg->sgl, sg->nents, direction))
-		goto err3;
-
+		goto err;
 	return sg;
 
-err3:
+err:
 	sg_free_table(sg);
-err2:
 	kfree(sg);
-err1:
-	return ERR_PTR(-ENOMEM);
+	return ERR_PTR(ret);
 }
 
 static void unmap_udmabuf(struct dma_buf_attachment *at,
-- 
2.9.3

Re: [PATCH 02/10] udmabuf: improve map_udmabuf error handling

[Laurent Pinchart]

Hi Gerd,

Thank you for the patch.

On Tuesday, 11 September 2018 09:59:13 EEST Gerd Hoffmann wrote:

A commit message would be nice, for all patches in this series.

> Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Reviewed-by: Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/dma-buf/udmabuf.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index e63d301bcb..19bd918209 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -51,25 +51,24 @@ static struct sg_table *map_udmabuf(struct
> dma_buf_attachment *at, {
>  	struct udmabuf *ubuf = at->dmabuf->priv;
>  	struct sg_table *sg;
> +	int ret;
> 
>  	sg = kzalloc(sizeof(*sg), GFP_KERNEL);
>  	if (!sg)
> -		goto err1;
> -	if (sg_alloc_table_from_pages(sg, ubuf->pages, ubuf->pagecount,
> -				      0, ubuf->pagecount << PAGE_SHIFT,
> -				      GFP_KERNEL) < 0)
> -		goto err2;
> +		return ERR_PTR(-ENOMEM);
> +	ret = sg_alloc_table_from_pages(sg, ubuf->pages, ubuf->pagecount,
> +					0, ubuf->pagecount << PAGE_SHIFT,
> +					GFP_KERNEL);
> +	if (ret < 0)
> +		goto err;
>  	if (!dma_map_sg(at->dev, sg->sgl, sg->nents, direction))
> -		goto err3;
> -
> +		goto err;
>  	return sg;
> 
> -err3:
> +err:
>  	sg_free_table(sg);
> -err2:
>  	kfree(sg);
> -err1:
> -	return ERR_PTR(-ENOMEM);
> +	return ERR_PTR(ret);
>  }
> 
>  static void unmap_udmabuf(struct dma_buf_attachment *at,

-- 
Regards,

Laurent Pinchart

[PATCH 03/10] udmabuf: use pgoff_t for pagecount

[Gerd Hoffmann]

Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/dma-buf/udmabuf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index 19bd918209..ec22f203b5 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -13,7 +13,7 @@
 #include <linux/udmabuf.h>
 
 struct udmabuf {
-	u32 pagecount;
+	pgoff_t pagecount;
 	struct page **pages;
 };
 
-- 
2.9.3

Re: [PATCH 03/10] udmabuf: use pgoff_t for pagecount

[Daniel Vetter]

On Tue, Sep 11, 2018 at 08:59:14AM +0200, Gerd Hoffmann wrote:
> Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

TIL pgoff_t stands for page cache offset. I think we're pretty bad at
using that within i915 :-)

On the entire series Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I did try to review in depth, but my brain is offline and coffee not
working :-) Hence just an ack.
-Daniel

> ---
>  drivers/dma-buf/udmabuf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index 19bd918209..ec22f203b5 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -13,7 +13,7 @@
>  #include <linux/udmabuf.h>
>  
>  struct udmabuf {
> -	u32 pagecount;
> +	pgoff_t pagecount;
>  	struct page **pages;
>  };
>  
> -- 
> 2.9.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 04/10] udmabuf: constify udmabuf_ops

[Gerd Hoffmann]

Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/dma-buf/udmabuf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index ec22f203b5..44c3c1bf20 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -104,7 +104,7 @@ static void kunmap_udmabuf(struct dma_buf *buf, unsigned long page_num,
 	kunmap(vaddr);
 }
 
-static struct dma_buf_ops udmabuf_ops = {
+static const struct dma_buf_ops udmabuf_ops = {
 	.map_dma_buf	  = map_udmabuf,
 	.unmap_dma_buf	  = unmap_udmabuf,
 	.release	  = release_udmabuf,
-- 
2.9.3

Re: [PATCH 04/10] udmabuf: constify udmabuf_ops

[Laurent Pinchart]

Hi Gerd,

Thank you for the patch.

On Tuesday, 11 September 2018 09:59:15 EEST Gerd Hoffmann wrote:
> Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/dma-buf/udmabuf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index ec22f203b5..44c3c1bf20 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -104,7 +104,7 @@ static void kunmap_udmabuf(struct dma_buf *buf, unsigned
> long page_num, kunmap(vaddr);
>  }
> 
> -static struct dma_buf_ops udmabuf_ops = {
> +static const struct dma_buf_ops udmabuf_ops = {
>  	.map_dma_buf	  = map_udmabuf,
>  	.unmap_dma_buf	  = unmap_udmabuf,
>  	.release	  = release_udmabuf,


-- 
Regards,

Laurent Pinchart

[PATCH 05/10] udmabuf: constify udmabuf_create args

[Gerd Hoffmann]

Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/dma-buf/udmabuf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index 44c3c1bf20..0cf7e85585 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -116,8 +116,8 @@ static const struct dma_buf_ops udmabuf_ops = {
 #define SEALS_WANTED (F_SEAL_SHRINK)
 #define SEALS_DENIED (F_SEAL_WRITE)
 
-static long udmabuf_create(struct udmabuf_create_list *head,
-			   struct udmabuf_create_item *list)
+static long udmabuf_create(struct const udmabuf_create_list *head,
+			   struct const udmabuf_create_item *list)
 {
 	DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
 	struct file *memfd = NULL;
-- 
2.9.3

Re: [PATCH 05/10] udmabuf: constify udmabuf_create args

[Laurent Pinchart]

Hi Gerd,

Thank you for the patch.

On Tuesday, 11 September 2018 09:59:16 EEST Gerd Hoffmann wrote:
> Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  drivers/dma-buf/udmabuf.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index 44c3c1bf20..0cf7e85585 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -116,8 +116,8 @@ static const struct dma_buf_ops udmabuf_ops = {
>  #define SEALS_WANTED (F_SEAL_SHRINK)
>  #define SEALS_DENIED (F_SEAL_WRITE)
> 
> -static long udmabuf_create(struct udmabuf_create_list *head,
> -			   struct udmabuf_create_item *list)
> +static long udmabuf_create(struct const udmabuf_create_list *head,
> +			   struct const udmabuf_create_item *list)

Shouldn't it be const struct, not struct const ?

With that fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  {
>  	DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
>  	struct file *memfd = NULL;

-- 
Regards,

Laurent Pinchart

[PATCH 06/10] udmabuf: add MEMFD_CREATE dependency

[Gerd Hoffmann]

udmabuf builds without it, but if userspace can not create memfd
handles in the first place it is rather pointless to include it.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/dma-buf/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig
index 338129eb12..fc3fe3f04e 100644
--- a/drivers/dma-buf/Kconfig
+++ b/drivers/dma-buf/Kconfig
@@ -34,6 +34,7 @@ config UDMABUF
 	bool "userspace dmabuf misc driver"
 	default n
 	depends on DMA_SHARED_BUFFER
+	depends on MEMFD_CREATE
 	help
 	  A driver to let userspace turn memfd regions into dma-bufs.
 	  Qemu can use this to create host dmabufs for guest framebuffers.
-- 
2.9.3

Re: [PATCH 06/10] udmabuf: add MEMFD_CREATE dependency

[Laurent Pinchart]

Hi Gerd,

Thank you for the patch.

On Tuesday, 11 September 2018 09:59:17 EEST Gerd Hoffmann wrote:
> udmabuf builds without it, but if userspace can not create memfd
> handles in the first place it is rather pointless to include it.

Except perhaps for compile test coverage ? How about

	depends on MEMFD_CREATE || COMPILE_TEST

?

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  drivers/dma-buf/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig
> index 338129eb12..fc3fe3f04e 100644
> --- a/drivers/dma-buf/Kconfig
> +++ b/drivers/dma-buf/Kconfig
> @@ -34,6 +34,7 @@ config UDMABUF
>  	bool "userspace dmabuf misc driver"
>  	default n
>  	depends on DMA_SHARED_BUFFER
> +	depends on MEMFD_CREATE
>  	help
>  	  A driver to let userspace turn memfd regions into dma-bufs.
>  	  Qemu can use this to create host dmabufs for guest framebuffers.


-- 
Regards,

Laurent Pinchart

[PATCH 07/10] udmabuf: rework limits

[Gerd Hoffmann]

Create variable for the list length limit.  Serves as documentation,
also allows to make it a module parameter if needed.

Also add a total size limit.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/dma-buf/udmabuf.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index 0cf7e85585..cb99a7886a 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -12,6 +12,9 @@
 #include <linux/shmem_fs.h>
 #include <linux/udmabuf.h>
 
+static int list_limit = 1024;  /* udmabuf_create_list->count limit */
+static int size_limit_mb = 64; /* total dmabuf size, in megabytes  */
+
 struct udmabuf {
 	pgoff_t pagecount;
 	struct page **pages;
@@ -123,7 +126,7 @@ static long udmabuf_create(struct const udmabuf_create_list *head,
 	struct file *memfd = NULL;
 	struct udmabuf *ubuf;
 	struct dma_buf *buf;
-	pgoff_t pgoff, pgcnt, pgidx, pgbuf;
+	pgoff_t pgoff, pgcnt, pgidx, pgbuf, pglimit;
 	struct page *page;
 	int seals, ret = -EINVAL;
 	u32 i, flags;
@@ -132,12 +135,15 @@ static long udmabuf_create(struct const udmabuf_create_list *head,
 	if (!ubuf)
 		return -ENOMEM;
 
+	pglimit = (size_limit_mb * 1024 * 1024) >> PAGE_SHIFT;
 	for (i = 0; i < head->count; i++) {
 		if (!IS_ALIGNED(list[i].offset, PAGE_SIZE))
 			goto err_free_ubuf;
 		if (!IS_ALIGNED(list[i].size, PAGE_SIZE))
 			goto err_free_ubuf;
 		ubuf->pagecount += list[i].size >> PAGE_SHIFT;
+		if (ubuf->pagecount > pglimit)
+			goto err_free_ubuf;
 	}
 	ubuf->pages = kmalloc_array(ubuf->pagecount, sizeof(struct page *),
 				    GFP_KERNEL);
@@ -227,7 +233,7 @@ static long udmabuf_ioctl_create_list(struct file *filp, unsigned long arg)
 
 	if (copy_from_user(&head, (void __user *)arg, sizeof(head)))
 		return -EFAULT;
-	if (head.count > 1024)
+	if (head.count > list_limit)
 		return -EINVAL;
 	lsize = sizeof(struct udmabuf_create_item) * head.count;
 	list = memdup_user((void __user *)(arg + sizeof(head)), lsize);
-- 
2.9.3

Re: [PATCH 07/10] udmabuf: rework limits

[Laurent Pinchart]

Hi Gerd,

Thank you for the patch.

On Tuesday, 11 September 2018 09:59:18 EEST Gerd Hoffmann wrote:
> Create variable for the list length limit.  Serves as documentation,
> also allows to make it a module parameter if needed.
> 
> Also add a total size limit.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  drivers/dma-buf/udmabuf.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index 0cf7e85585..cb99a7886a 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -12,6 +12,9 @@
>  #include <linux/shmem_fs.h>
>  #include <linux/udmabuf.h>
> 
> +static int list_limit = 1024;  /* udmabuf_create_list->count limit */
> +static int size_limit_mb = 64; /* total dmabuf size, in megabytes  */

static const int maybe ? Or size_t for the size limit and unsigned int for the 
limit on the number of entries, as they can't be negative ?

Why those specific values ?

>  struct udmabuf {
>  	pgoff_t pagecount;
>  	struct page **pages;
> @@ -123,7 +126,7 @@ static long udmabuf_create(struct const
> udmabuf_create_list *head, struct file *memfd = NULL;
>  	struct udmabuf *ubuf;
>  	struct dma_buf *buf;
> -	pgoff_t pgoff, pgcnt, pgidx, pgbuf;
> +	pgoff_t pgoff, pgcnt, pgidx, pgbuf, pglimit;
>  	struct page *page;
>  	int seals, ret = -EINVAL;
>  	u32 i, flags;
> @@ -132,12 +135,15 @@ static long udmabuf_create(struct const
> udmabuf_create_list *head, if (!ubuf)
>  		return -ENOMEM;
> 
> +	pglimit = (size_limit_mb * 1024 * 1024) >> PAGE_SHIFT;
>  	for (i = 0; i < head->count; i++) {
>  		if (!IS_ALIGNED(list[i].offset, PAGE_SIZE))
>  			goto err_free_ubuf;
>  		if (!IS_ALIGNED(list[i].size, PAGE_SIZE))
>  			goto err_free_ubuf;
>  		ubuf->pagecount += list[i].size >> PAGE_SHIFT;
> +		if (ubuf->pagecount > pglimit)
> +			goto err_free_ubuf;
>  	}
>  	ubuf->pages = kmalloc_array(ubuf->pagecount, sizeof(struct page *),
>  				    GFP_KERNEL);
> @@ -227,7 +233,7 @@ static long udmabuf_ioctl_create_list(struct file *filp,
> unsigned long arg)
> 
>  	if (copy_from_user(&head, (void __user *)arg, sizeof(head)))
>  		return -EFAULT;
> -	if (head.count > 1024)
> +	if (head.count > list_limit)
>  		return -EINVAL;
>  	lsize = sizeof(struct udmabuf_create_item) * head.count;
>  	list = memdup_user((void __user *)(arg + sizeof(head)), lsize);


-- 
Regards,

Laurent Pinchart

[PATCH 08/10] udmabuf: improve udmabuf_create error handling

[Gerd Hoffmann]

Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/dma-buf/udmabuf.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index cb99a7886a..ec46513a47 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -126,7 +126,7 @@ static long udmabuf_create(struct const udmabuf_create_list *head,
 	struct file *memfd = NULL;
 	struct udmabuf *ubuf;
 	struct dma_buf *buf;
-	pgoff_t pgoff, pgcnt, pgidx, pgbuf, pglimit;
+	pgoff_t pgoff, pgcnt, pgidx, pgbuf = 0, pglimit;
 	struct page *page;
 	int seals, ret = -EINVAL;
 	u32 i, flags;
@@ -138,32 +138,32 @@ static long udmabuf_create(struct const udmabuf_create_list *head,
 	pglimit = (size_limit_mb * 1024 * 1024) >> PAGE_SHIFT;
 	for (i = 0; i < head->count; i++) {
 		if (!IS_ALIGNED(list[i].offset, PAGE_SIZE))
-			goto err_free_ubuf;
+			goto err;
 		if (!IS_ALIGNED(list[i].size, PAGE_SIZE))
-			goto err_free_ubuf;
+			goto err;
 		ubuf->pagecount += list[i].size >> PAGE_SHIFT;
 		if (ubuf->pagecount > pglimit)
-			goto err_free_ubuf;
+			goto err;
 	}
 	ubuf->pages = kmalloc_array(ubuf->pagecount, sizeof(struct page *),
 				    GFP_KERNEL);
 	if (!ubuf->pages) {
 		ret = -ENOMEM;
-		goto err_free_ubuf;
+		goto err;
 	}
 
 	pgbuf = 0;
 	for (i = 0; i < head->count; i++) {
 		memfd = fget(list[i].memfd);
 		if (!memfd)
-			goto err_put_pages;
+			goto err;
 		if (!shmem_mapping(file_inode(memfd)->i_mapping))
-			goto err_put_pages;
+			goto err;
 		seals = memfd_fcntl(memfd, F_GET_SEALS, 0);
 		if (seals == -EINVAL ||
 		    (seals & SEALS_WANTED) != SEALS_WANTED ||
 		    (seals & SEALS_DENIED) != 0)
-			goto err_put_pages;
+			goto err;
 		pgoff = list[i].offset >> PAGE_SHIFT;
 		pgcnt = list[i].size   >> PAGE_SHIFT;
 		for (pgidx = 0; pgidx < pgcnt; pgidx++) {
@@ -171,13 +171,13 @@ static long udmabuf_create(struct const udmabuf_create_list *head,
 				file_inode(memfd)->i_mapping, pgoff + pgidx);
 			if (IS_ERR(page)) {
 				ret = PTR_ERR(page);
-				goto err_put_pages;
+				goto err;
 			}
 			ubuf->pages[pgbuf++] = page;
 		}
 		fput(memfd);
+		memfd = NULL;
 	}
-	memfd = NULL;
 
 	exp_info.ops  = &udmabuf_ops;
 	exp_info.size = ubuf->pagecount << PAGE_SHIFT;
@@ -186,7 +186,7 @@ static long udmabuf_create(struct const udmabuf_create_list *head,
 	buf = dma_buf_export(&exp_info);
 	if (IS_ERR(buf)) {
 		ret = PTR_ERR(buf);
-		goto err_put_pages;
+		goto err;
 	}
 
 	flags = 0;
@@ -194,10 +194,9 @@ static long udmabuf_create(struct const udmabuf_create_list *head,
 		flags |= O_CLOEXEC;
 	return dma_buf_fd(buf, flags);
 
-err_put_pages:
+err:
 	while (pgbuf > 0)
 		put_page(ubuf->pages[--pgbuf]);
-err_free_ubuf:
 	if (memfd)
 		fput(memfd);
 	kfree(ubuf->pages);
-- 
2.9.3

Re: [PATCH 08/10] udmabuf: improve udmabuf_create error handling

[Laurent Pinchart]

Hi Gerd,

Thank you for the patch.

On Tuesday, 11 September 2018 09:59:19 EEST Gerd Hoffmann wrote:
> Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/dma-buf/udmabuf.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index cb99a7886a..ec46513a47 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -126,7 +126,7 @@ static long udmabuf_create(struct const
> udmabuf_create_list *head, struct file *memfd = NULL;
>  	struct udmabuf *ubuf;
>  	struct dma_buf *buf;
> -	pgoff_t pgoff, pgcnt, pgidx, pgbuf, pglimit;
> +	pgoff_t pgoff, pgcnt, pgidx, pgbuf = 0, pglimit;
>  	struct page *page;
>  	int seals, ret = -EINVAL;
>  	u32 i, flags;
> @@ -138,32 +138,32 @@ static long udmabuf_create(struct const
> udmabuf_create_list *head, pglimit = (size_limit_mb * 1024 * 1024) >>
> PAGE_SHIFT;
>  	for (i = 0; i < head->count; i++) {
>  		if (!IS_ALIGNED(list[i].offset, PAGE_SIZE))
> -			goto err_free_ubuf;
> +			goto err;
>  		if (!IS_ALIGNED(list[i].size, PAGE_SIZE))
> -			goto err_free_ubuf;
> +			goto err;
>  		ubuf->pagecount += list[i].size >> PAGE_SHIFT;
>  		if (ubuf->pagecount > pglimit)
> -			goto err_free_ubuf;
> +			goto err;
>  	}
>  	ubuf->pages = kmalloc_array(ubuf->pagecount, sizeof(struct page *),
>  				    GFP_KERNEL);
>  	if (!ubuf->pages) {
>  		ret = -ENOMEM;
> -		goto err_free_ubuf;
> +		goto err;
>  	}
> 
>  	pgbuf = 0;
>  	for (i = 0; i < head->count; i++) {
>  		memfd = fget(list[i].memfd);
>  		if (!memfd)
> -			goto err_put_pages;
> +			goto err;
>  		if (!shmem_mapping(file_inode(memfd)->i_mapping))
> -			goto err_put_pages;
> +			goto err;
>  		seals = memfd_fcntl(memfd, F_GET_SEALS, 0);
>  		if (seals == -EINVAL ||
>  		    (seals & SEALS_WANTED) != SEALS_WANTED ||
>  		    (seals & SEALS_DENIED) != 0)
> -			goto err_put_pages;
> +			goto err;
>  		pgoff = list[i].offset >> PAGE_SHIFT;
>  		pgcnt = list[i].size   >> PAGE_SHIFT;
>  		for (pgidx = 0; pgidx < pgcnt; pgidx++) {
> @@ -171,13 +171,13 @@ static long udmabuf_create(struct const
> udmabuf_create_list *head, file_inode(memfd)->i_mapping, pgoff + pgidx);
>  			if (IS_ERR(page)) {
>  				ret = PTR_ERR(page);
> -				goto err_put_pages;
> +				goto err;
>  			}
>  			ubuf->pages[pgbuf++] = page;
>  		}
>  		fput(memfd);
> +		memfd = NULL;
>  	}
> -	memfd = NULL;
> 
>  	exp_info.ops  = &udmabuf_ops;
>  	exp_info.size = ubuf->pagecount << PAGE_SHIFT;
> @@ -186,7 +186,7 @@ static long udmabuf_create(struct const
> udmabuf_create_list *head, buf = dma_buf_export(&exp_info);
>  	if (IS_ERR(buf)) {
>  		ret = PTR_ERR(buf);
> -		goto err_put_pages;
> +		goto err;
>  	}
> 
>  	flags = 0;
> @@ -194,10 +194,9 @@ static long udmabuf_create(struct const
> udmabuf_create_list *head, flags |= O_CLOEXEC;
>  	return dma_buf_fd(buf, flags);
> 
> -err_put_pages:
> +err:
>  	while (pgbuf > 0)
>  		put_page(ubuf->pages[--pgbuf]);
> -err_free_ubuf:
>  	if (memfd)
>  		fput(memfd);
>  	kfree(ubuf->pages);


-- 
Regards,

Laurent Pinchart

[PATCH 09/10] udmabuf: use EBADFD in case we didn't got a memfd

[Gerd Hoffmann]

Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/dma-buf/udmabuf.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index ec46513a47..1862bb6bed 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -154,14 +154,17 @@ static long udmabuf_create(struct const udmabuf_create_list *head,
 
 	pgbuf = 0;
 	for (i = 0; i < head->count; i++) {
+		ret = -EBADFD;
 		memfd = fget(list[i].memfd);
 		if (!memfd)
 			goto err;
 		if (!shmem_mapping(file_inode(memfd)->i_mapping))
 			goto err;
 		seals = memfd_fcntl(memfd, F_GET_SEALS, 0);
-		if (seals == -EINVAL ||
-		    (seals & SEALS_WANTED) != SEALS_WANTED ||
+		if (seals == -EINVAL)
+			goto err;
+		ret = -EINVAL;
+		if ((seals & SEALS_WANTED) != SEALS_WANTED ||
 		    (seals & SEALS_DENIED) != 0)
 			goto err;
 		pgoff = list[i].offset >> PAGE_SHIFT;
-- 
2.9.3

[PATCH 10/10] udmabuf: use ENOTTY for invalid ioctls

[Gerd Hoffmann]

Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/dma-buf/udmabuf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index 1862bb6bed..a543fd6ea1 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -260,7 +260,7 @@ static long udmabuf_ioctl(struct file *filp, unsigned int ioctl,
 		ret = udmabuf_ioctl_create_list(filp, arg);
 		break;
 	default:
-		ret = -EINVAL;
+		ret = -ENOTTY;
 		break;
 	}
 	return ret;
-- 
2.9.3

Re: [PATCH 10/10] udmabuf: use ENOTTY for invalid ioctls

[Laurent Pinchart]

Hi Gerd,

Thank you for the patch.

On Tuesday, 11 September 2018 09:59:21 EEST Gerd Hoffmann wrote:
> Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/dma-buf/udmabuf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index 1862bb6bed..a543fd6ea1 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -260,7 +260,7 @@ static long udmabuf_ioctl(struct file *filp, unsigned
> int ioctl, ret = udmabuf_ioctl_create_list(filp, arg);
>  		break;
>  	default:
> -		ret = -EINVAL;
> +		ret = -ENOTTY;
>  		break;
>  	}
>  	return ret;

-- 
Regards,

Laurent Pinchart