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;

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,

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

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,

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;

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.

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);

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);