[PATCH 1/2] media: videobuf2: use dmabuf size for length

[PATCH 1/2] media: videobuf2: use dmabuf size for length

[Helen Koike]

Always use dmabuf size when considering the length of the buffer.
Discard userspace provided length.
Fix length check error in _verify_length(), which was handling single and
multiplanar diferently, and also not catching the case where userspace
provides a bigger length and bytesused then the underlying buffer.

Suggested-by: Hans Verkuil <hverkuil@xs4all.nl>
Signed-off-by: Helen Koike <helen.koike@collabora.com>
---

Hello,

As discussed on
https://patchwork.linuxtv.org/project/linux-media/patch/gh5kef5bkeel3o6b2dkgc2dfagu9klj4c0@4ax.com/

This patch also helps the conversion layer of the Ext API patchset,
where we are not exposing the length field.

It was discussed that userspace might use a smaller length field to
limit the usage of the underlying buffer, but I'm not sure if this is
really usefull and just complicates things.

If this is usefull, then we should also expose a length field in the Ext
API, and document this feature properly.

What do you think?
---
 .../media/common/videobuf2/videobuf2-core.c   | 21 ++++++++++++++++---
 .../media/common/videobuf2/videobuf2-v4l2.c   |  8 +++----
 include/uapi/linux/videodev2.h                |  7 +++++--
 3 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 02281d13505f..2cbde14af051 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -1205,6 +1205,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
 
 	for (plane = 0; plane < vb->num_planes; ++plane) {
 		struct dma_buf *dbuf = dma_buf_get(planes[plane].m.fd);
+		unsigned int bytesused;
 
 		if (IS_ERR_OR_NULL(dbuf)) {
 			dprintk(q, 1, "invalid dmabuf fd for plane %d\n",
@@ -1213,9 +1214,23 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
 			goto err;
 		}
 
-		/* use DMABUF size if length is not provided */
-		if (planes[plane].length == 0)
-			planes[plane].length = dbuf->size;
+		planes[plane].length = dbuf->size;
+		bytesused = planes[plane].bytesused ?
+			    planes[plane].bytesused : dbuf->size;
+
+		if (planes[plane].bytesused > planes[plane].length) {
+			dprintk(q, 1, "bytesused is bigger then dmabuf length for plane %d\n",
+				plane);
+			ret = -EINVAL;
+			goto err;
+		}
+
+		if (planes[plane].data_offset >= bytesused) {
+			dprintk(q, 1, "data_offset >= bytesused for plane %d\n",
+				plane);
+			ret = -EINVAL;
+			goto err;
+		}
 
 		if (planes[plane].length < vb->planes[plane].min_length) {
 			dprintk(q, 1, "invalid dmabuf length %u for plane %d, minimum length %u\n",
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 7e96f67c60ba..ffc7ed46f74a 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -98,14 +98,14 @@ static int __verify_length(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 	unsigned int bytesused;
 	unsigned int plane;
 
-	if (V4L2_TYPE_IS_CAPTURE(b->type))
+	/* length check for dmabuf is performed in _prepare_dmabuf() */
+	if (V4L2_TYPE_IS_CAPTURE(b->type) || b->memory == VB2_MEMORY_DMABUF)
 		return 0;
 
 	if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
 		for (plane = 0; plane < vb->num_planes; ++plane) {
-			length = (b->memory == VB2_MEMORY_USERPTR ||
-				  b->memory == VB2_MEMORY_DMABUF)
-			       ? b->m.planes[plane].length
+			length = b->memory == VB2_MEMORY_USERPTR
+				? b->m.planes[plane].length
 				: vb->planes[plane].length;
 			bytesused = b->m.planes[plane].bytesused
 				  ? b->m.planes[plane].bytesused : length;
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 8d15f6ccc4b4..79b3b2893513 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -968,7 +968,9 @@ struct v4l2_requestbuffers {
 /**
  * struct v4l2_plane - plane info for multi-planar buffers
  * @bytesused:		number of bytes occupied by data in the plane (payload)
- * @length:		size of this plane (NOT the payload) in bytes
+ * @length:		size of this plane (NOT the payload) in bytes. Filled
+ *			by userspace for USERPTR and by the driver for DMABUF
+ *			and MMAP.
  * @mem_offset:		when memory in the associated struct v4l2_buffer is
  *			V4L2_MEMORY_MMAP, equals the offset from the start of
  *			the device memory for this plane (or is a "cookie" that
@@ -1025,7 +1027,8 @@ struct v4l2_plane {
  * @m:		union of @offset, @userptr, @planes and @fd
  * @length:	size in bytes of the buffer (NOT its payload) for single-plane
  *		buffers (when type != *_MPLANE); number of elements in the
- *		planes array for multi-plane buffers
+ *		planes array for multi-plane buffers. Filled by userspace for
+ *		USERPTR and by the driver for DMABUF and MMAP.
  * @reserved2:	drivers and applications must zero this field
  * @request_fd: fd of the request that this buffer should use
  * @reserved:	for backwards compatibility with applications that do not know
-- 
2.30.1

[PATCH 2/2] media: videobuf2: cleanup size argument from attach_dmabuf()

[Helen Koike]

Since we always use the size of the underlying buffer for dmabuf, remove
the size parameter from the attach_dmabuf() callback.

Suggested-by: Hans Verkuil <hverkuil@xs4all.nl>
Signed-off-by: Helen Koike <helen.koike@collabora.com>
---
 drivers/media/common/videobuf2/videobuf2-core.c       | 2 +-
 drivers/media/common/videobuf2/videobuf2-dma-contig.c | 7 ++-----
 drivers/media/common/videobuf2/videobuf2-dma-sg.c     | 7 ++-----
 drivers/media/common/videobuf2/videobuf2-vmalloc.c    | 7 ++-----
 include/media/videobuf2-core.h                        | 1 -
 5 files changed, 7 insertions(+), 17 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 2cbde14af051..86af4f3c72eb 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -1266,7 +1266,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
 		/* Acquire each plane's memory */
 		mem_priv = call_ptr_memop(vb, attach_dmabuf,
 				q->alloc_devs[plane] ? : q->dev,
-				dbuf, planes[plane].length, q->dma_dir);
+				dbuf, q->dma_dir);
 		if (IS_ERR(mem_priv)) {
 			dprintk(q, 1, "failed to attach dmabuf\n");
 			ret = PTR_ERR(mem_priv);
diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
index a7f61ba85440..a26aa52f954b 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
@@ -661,14 +661,11 @@ static void vb2_dc_detach_dmabuf(void *mem_priv)
 }
 
 static void *vb2_dc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
-	unsigned long size, enum dma_data_direction dma_dir)
+				  enum dma_data_direction dma_dir)
 {
 	struct vb2_dc_buf *buf;
 	struct dma_buf_attachment *dba;
 
-	if (dbuf->size < size)
-		return ERR_PTR(-EFAULT);
-
 	if (WARN_ON(!dev))
 		return ERR_PTR(-EINVAL);
 
@@ -686,7 +683,7 @@ static void *vb2_dc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
 	}
 
 	buf->dma_dir = dma_dir;
-	buf->size = size;
+	buf->size = dbuf->size;
 	buf->db_attach = dba;
 
 	return buf;
diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
index c5b06a509566..8c006f79bed4 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
@@ -606,7 +606,7 @@ static void vb2_dma_sg_detach_dmabuf(void *mem_priv)
 }
 
 static void *vb2_dma_sg_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
-	unsigned long size, enum dma_data_direction dma_dir)
+				      enum dma_data_direction dma_dir)
 {
 	struct vb2_dma_sg_buf *buf;
 	struct dma_buf_attachment *dba;
@@ -614,9 +614,6 @@ static void *vb2_dma_sg_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
 	if (WARN_ON(!dev))
 		return ERR_PTR(-EINVAL);
 
-	if (dbuf->size < size)
-		return ERR_PTR(-EFAULT);
-
 	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
 	if (!buf)
 		return ERR_PTR(-ENOMEM);
@@ -631,7 +628,7 @@ static void *vb2_dma_sg_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
 	}
 
 	buf->dma_dir = dma_dir;
-	buf->size = size;
+	buf->size = dmabuf->size;
 	buf->db_attach = dba;
 
 	return buf;
diff --git a/drivers/media/common/videobuf2/videobuf2-vmalloc.c b/drivers/media/common/videobuf2/videobuf2-vmalloc.c
index 83f95258ec8c..c2d41b375c10 100644
--- a/drivers/media/common/videobuf2/videobuf2-vmalloc.c
+++ b/drivers/media/common/videobuf2/videobuf2-vmalloc.c
@@ -404,20 +404,17 @@ static void vb2_vmalloc_detach_dmabuf(void *mem_priv)
 }
 
 static void *vb2_vmalloc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
-	unsigned long size, enum dma_data_direction dma_dir)
+				       enum dma_data_direction dma_dir)
 {
 	struct vb2_vmalloc_buf *buf;
 
-	if (dbuf->size < size)
-		return ERR_PTR(-EFAULT);
-
 	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
 	if (!buf)
 		return ERR_PTR(-ENOMEM);
 
 	buf->dbuf = dbuf;
 	buf->dma_dir = dma_dir;
-	buf->size = size;
+	buf->size = dbuf->size;
 
 	return buf;
 }
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 12955cb460d2..db07001cada8 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -134,7 +134,6 @@ struct vb2_mem_ops {
 
 	void		*(*attach_dmabuf)(struct device *dev,
 					  struct dma_buf *dbuf,
-					  unsigned long size,
 					  enum dma_data_direction dma_dir);
 	void		(*detach_dmabuf)(void *buf_priv);
 	int		(*map_dmabuf)(void *buf_priv);
-- 
2.30.1

Re: [PATCH 2/2] media: videobuf2: cleanup size argument from attach_dmabuf()

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3120 bytes --]

Hi Helen,

I love your patch! Yet something to improve:

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on next-20210324]
[cannot apply to v5.12-rc4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Helen-Koike/media-videobuf2-use-dmabuf-size-for-length/20210325-082047
base:   git://linuxtv.org/media_tree.git master
config: powerpc64-randconfig-r016-20210325 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 5d6b4aa80d6df62b924a12af030c5ded868ee4f1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install powerpc64 cross compiling tool for clang build
        # apt-get install binutils-powerpc64-linux-gnu
        # https://github.com/0day-ci/linux/commit/41e2cea31db8378b33e31785aec668a009d1355b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Helen-Koike/media-videobuf2-use-dmabuf-size-for-length/20210325-082047
        git checkout 41e2cea31db8378b33e31785aec668a009d1355b
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/media/common/videobuf2/videobuf2-dma-sg.c:631:14: error: use of undeclared identifier 'dmabuf'; did you mean 'dbuf'?
           buf->size = dmabuf->size;
                       ^~~~~~
                       dbuf
   drivers/media/common/videobuf2/videobuf2-dma-sg.c:608:75: note: 'dbuf' declared here
   static void *vb2_dma_sg_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
                                                                             ^
   1 error generated.


vim +631 drivers/media/common/videobuf2/videobuf2-dma-sg.c

   607	
   608	static void *vb2_dma_sg_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
   609					      enum dma_data_direction dma_dir)
   610	{
   611		struct vb2_dma_sg_buf *buf;
   612		struct dma_buf_attachment *dba;
   613	
   614		if (WARN_ON(!dev))
   615			return ERR_PTR(-EINVAL);
   616	
   617		buf = kzalloc(sizeof(*buf), GFP_KERNEL);
   618		if (!buf)
   619			return ERR_PTR(-ENOMEM);
   620	
   621		buf->dev = dev;
   622		/* create attachment for the dmabuf with the user device */
   623		dba = dma_buf_attach(dbuf, buf->dev);
   624		if (IS_ERR(dba)) {
   625			pr_err("failed to attach dmabuf\n");
   626			kfree(buf);
   627			return dba;
   628		}
   629	
   630		buf->dma_dir = dma_dir;
 > 631		buf->size = dmabuf->size;
   632		buf->db_attach = dba;
   633	
   634		return buf;
   635	}
   636	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 40444 bytes --]

Re: [PATCH 1/2] media: videobuf2: use dmabuf size for length

[John Cox]

Hi

>Always use dmabuf size when considering the length of the buffer.
>Discard userspace provided length.
>Fix length check error in _verify_length(), which was handling single and
>multiplanar diferently, and also not catching the case where userspace
>provides a bigger length and bytesused then the underlying buffer.
>
>Suggested-by: Hans Verkuil <hverkuil@xs4all.nl>
>Signed-off-by: Helen Koike <helen.koike@collabora.com>
>---
>
>Hello,
>
>As discussed on
>https://patchwork.linuxtv.org/project/linux-media/patch/gh5kef5bkeel3o6b2dkgc2dfagu9klj4c0@4ax.com/
>
>This patch also helps the conversion layer of the Ext API patchset,
>where we are not exposing the length field.
>
>It was discussed that userspace might use a smaller length field to
>limit the usage of the underlying buffer, but I'm not sure if this is
>really usefull and just complicates things.
>
>If this is usefull, then we should also expose a length field in the Ext
>API, and document this feature properly.
>
>What do you think?
>---
> .../media/common/videobuf2/videobuf2-core.c   | 21 ++++++++++++++++---
> .../media/common/videobuf2/videobuf2-v4l2.c   |  8 +++----
> include/uapi/linux/videodev2.h                |  7 +++++--
> 3 files changed, 27 insertions(+), 9 deletions(-)
>
>diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>index 02281d13505f..2cbde14af051 100644
>--- a/drivers/media/common/videobuf2/videobuf2-core.c
>+++ b/drivers/media/common/videobuf2/videobuf2-core.c
>@@ -1205,6 +1205,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
> 
> 	for (plane = 0; plane < vb->num_planes; ++plane) {
> 		struct dma_buf *dbuf = dma_buf_get(planes[plane].m.fd);
>+		unsigned int bytesused;
> 
> 		if (IS_ERR_OR_NULL(dbuf)) {
> 			dprintk(q, 1, "invalid dmabuf fd for plane %d\n",
>@@ -1213,9 +1214,23 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
> 			goto err;
> 		}
> 
>-		/* use DMABUF size if length is not provided */
>-		if (planes[plane].length == 0)
>-			planes[plane].length = dbuf->size;
>+		planes[plane].length = dbuf->size;
>+		bytesused = planes[plane].bytesused ?
>+			    planes[plane].bytesused : dbuf->size;
>+
>+		if (planes[plane].bytesused > planes[plane].length) {
>+			dprintk(q, 1, "bytesused is bigger then dmabuf length for plane %d\n",
>+				plane);
>+			ret = -EINVAL;
>+			goto err;
>+		}
>+
>+		if (planes[plane].data_offset >= bytesused) {
>+			dprintk(q, 1, "data_offset >= bytesused for plane %d\n",
>+				plane);
>+			ret = -EINVAL;
>+			goto err;
>+		}
> 
> 		if (planes[plane].length < vb->planes[plane].min_length) {
> 			dprintk(q, 1, "invalid dmabuf length %u for plane %d, minimum length %u\n",
>diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>index 7e96f67c60ba..ffc7ed46f74a 100644
>--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>@@ -98,14 +98,14 @@ static int __verify_length(struct vb2_buffer *vb, const struct v4l2_buffer *b)
> 	unsigned int bytesused;
> 	unsigned int plane;
> 
>-	if (V4L2_TYPE_IS_CAPTURE(b->type))
>+	/* length check for dmabuf is performed in _prepare_dmabuf() */
>+	if (V4L2_TYPE_IS_CAPTURE(b->type) || b->memory == VB2_MEMORY_DMABUF)
> 		return 0;
> 
> 	if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
> 		for (plane = 0; plane < vb->num_planes; ++plane) {
>-			length = (b->memory == VB2_MEMORY_USERPTR ||
>-				  b->memory == VB2_MEMORY_DMABUF)
>-			       ? b->m.planes[plane].length
>+			length = b->memory == VB2_MEMORY_USERPTR
>+				? b->m.planes[plane].length
> 				: vb->planes[plane].length;
> 			bytesused = b->m.planes[plane].bytesused
> 				  ? b->m.planes[plane].bytesused : length;
>diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>index 8d15f6ccc4b4..79b3b2893513 100644
>--- a/include/uapi/linux/videodev2.h
>+++ b/include/uapi/linux/videodev2.h
>@@ -968,7 +968,9 @@ struct v4l2_requestbuffers {
> /**
>  * struct v4l2_plane - plane info for multi-planar buffers
>  * @bytesused:		number of bytes occupied by data in the plane (payload)
>- * @length:		size of this plane (NOT the payload) in bytes
>+ * @length:		size of this plane (NOT the payload) in bytes. Filled
>+ *			by userspace for USERPTR and by the driver for DMABUF
>+ *			and MMAP.
>  * @mem_offset:		when memory in the associated struct v4l2_buffer is
>  *			V4L2_MEMORY_MMAP, equals the offset from the start of
>  *			the device memory for this plane (or is a "cookie" that
>@@ -1025,7 +1027,8 @@ struct v4l2_plane {
>  * @m:		union of @offset, @userptr, @planes and @fd
>  * @length:	size in bytes of the buffer (NOT its payload) for single-plane
>  *		buffers (when type != *_MPLANE); number of elements in the
>- *		planes array for multi-plane buffers
>+ *		planes array for multi-plane buffers. Filled by userspace for
>+ *		USERPTR and by the driver for DMABUF and MMAP.
>  * @reserved2:	drivers and applications must zero this field
>  * @request_fd: fd of the request that this buffer should use
>  * @reserved:	for backwards compatibility with applications that do not know

I think this does what I want. But I'm going to restate my usage desires
and check that you agree that it covers them.

I'm interested in passing compressed bitstreams to a decoder.  The size
of these buffers can be very variable and the worst case will nearly
always be much larger than the typical case and that size cannot be
known in advance of usage.  It can be very wasteful to have to allocate
buffers that are over an order of magnitude bigger than are likely to
ever be used.  If you have a fixed pool of fixed size buffers allocated
at the start of time this wastefulness is unavoidable, but dmabufs can
be dynamically sized to be as big as required and so there should be no
limitation on passing in buffers that are smaller than the maximum.  It
also seems plausible that dmabufs that are larger than the maximum
should be allowed as long as their bytesused is smaller or equal.

As an aside, even when using dynamically sized dmabufs they are often
way larger than the data they contain and forcing cache flushes or maps
of their entire length rather than just the used portion is also
wasteful.  This might be a use for the incoming size field.

Regards

John Cox

Re: [PATCH 1/2] media: videobuf2: use dmabuf size for length

[Laurent Pinchart]

Hi Helen,

On Wed, Mar 24, 2021 at 09:17:11PM -0300, Helen Koike wrote:
> Always use dmabuf size when considering the length of the buffer.
> Discard userspace provided length.
> Fix length check error in _verify_length(), which was handling single and
> multiplanar diferently, and also not catching the case where userspace
> provides a bigger length and bytesused then the underlying buffer.
> 
> Suggested-by: Hans Verkuil <hverkuil@xs4all.nl>
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> ---
> 
> Hello,
> 
> As discussed on
> https://patchwork.linuxtv.org/project/linux-media/patch/gh5kef5bkeel3o6b2dkgc2dfagu9klj4c0@4ax.com/
> 
> This patch also helps the conversion layer of the Ext API patchset,
> where we are not exposing the length field.
> 
> It was discussed that userspace might use a smaller length field to
> limit the usage of the underlying buffer, but I'm not sure if this is
> really usefull and just complicates things.
> 
> If this is usefull, then we should also expose a length field in the Ext
> API, and document this feature properly.
> 
> What do you think?

I think a limit could be useful, as a single dmabuf object could hold
multiple planes, which should be addressed by an offset from the
beginning of the buffer. Giving a length to the kernel could help
catching errors. As the existing API doesn't support offsets, a length
limit is likely not very useful at the moment, but should I believe be
included at least in the new API.

For the existing implementation, I'd say that we should be pragmatic. If
using the provided length as a maximum boundary makes the implementation
more complex for very little gain, let's not do it. But on the other
hand, considering existing userspace, would there be added value in
implementing such a mechanism ?

> ---
>  .../media/common/videobuf2/videobuf2-core.c   | 21 ++++++++++++++++---
>  .../media/common/videobuf2/videobuf2-v4l2.c   |  8 +++----
>  include/uapi/linux/videodev2.h                |  7 +++++--
>  3 files changed, 27 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 02281d13505f..2cbde14af051 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -1205,6 +1205,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
>  
>  	for (plane = 0; plane < vb->num_planes; ++plane) {
>  		struct dma_buf *dbuf = dma_buf_get(planes[plane].m.fd);
> +		unsigned int bytesused;
>  
>  		if (IS_ERR_OR_NULL(dbuf)) {
>  			dprintk(q, 1, "invalid dmabuf fd for plane %d\n",
> @@ -1213,9 +1214,23 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
>  			goto err;
>  		}
>  
> -		/* use DMABUF size if length is not provided */
> -		if (planes[plane].length == 0)
> -			planes[plane].length = dbuf->size;
> +		planes[plane].length = dbuf->size;
> +		bytesused = planes[plane].bytesused ?
> +			    planes[plane].bytesused : dbuf->size;
> +
> +		if (planes[plane].bytesused > planes[plane].length) {
> +			dprintk(q, 1, "bytesused is bigger then dmabuf length for plane %d\n",
> +				plane);
> +			ret = -EINVAL;
> +			goto err;
> +		}
> +
> +		if (planes[plane].data_offset >= bytesused) {
> +			dprintk(q, 1, "data_offset >= bytesused for plane %d\n",
> +				plane);
> +			ret = -EINVAL;
> +			goto err;
> +		}
>  
>  		if (planes[plane].length < vb->planes[plane].min_length) {
>  			dprintk(q, 1, "invalid dmabuf length %u for plane %d, minimum length %u\n",
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 7e96f67c60ba..ffc7ed46f74a 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -98,14 +98,14 @@ static int __verify_length(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>  	unsigned int bytesused;
>  	unsigned int plane;
>  
> -	if (V4L2_TYPE_IS_CAPTURE(b->type))
> +	/* length check for dmabuf is performed in _prepare_dmabuf() */
> +	if (V4L2_TYPE_IS_CAPTURE(b->type) || b->memory == VB2_MEMORY_DMABUF)
>  		return 0;
>  
>  	if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
>  		for (plane = 0; plane < vb->num_planes; ++plane) {
> -			length = (b->memory == VB2_MEMORY_USERPTR ||
> -				  b->memory == VB2_MEMORY_DMABUF)
> -			       ? b->m.planes[plane].length
> +			length = b->memory == VB2_MEMORY_USERPTR
> +				? b->m.planes[plane].length
>  				: vb->planes[plane].length;
>  			bytesused = b->m.planes[plane].bytesused
>  				  ? b->m.planes[plane].bytesused : length;
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 8d15f6ccc4b4..79b3b2893513 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -968,7 +968,9 @@ struct v4l2_requestbuffers {
>  /**
>   * struct v4l2_plane - plane info for multi-planar buffers
>   * @bytesused:		number of bytes occupied by data in the plane (payload)
> - * @length:		size of this plane (NOT the payload) in bytes
> + * @length:		size of this plane (NOT the payload) in bytes. Filled
> + *			by userspace for USERPTR and by the driver for DMABUF
> + *			and MMAP.
>   * @mem_offset:		when memory in the associated struct v4l2_buffer is
>   *			V4L2_MEMORY_MMAP, equals the offset from the start of
>   *			the device memory for this plane (or is a "cookie" that
> @@ -1025,7 +1027,8 @@ struct v4l2_plane {
>   * @m:		union of @offset, @userptr, @planes and @fd
>   * @length:	size in bytes of the buffer (NOT its payload) for single-plane
>   *		buffers (when type != *_MPLANE); number of elements in the
> - *		planes array for multi-plane buffers
> + *		planes array for multi-plane buffers. Filled by userspace for
> + *		USERPTR and by the driver for DMABUF and MMAP.
>   * @reserved2:	drivers and applications must zero this field
>   * @request_fd: fd of the request that this buffer should use
>   * @reserved:	for backwards compatibility with applications that do not know

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] media: videobuf2: use dmabuf size for length

[Helen Koike]

Hi John,

On 3/25/21 7:20 AM, John Cox wrote:
> Hi
> 
>> Always use dmabuf size when considering the length of the buffer.
>> Discard userspace provided length.
>> Fix length check error in _verify_length(), which was handling single and
>> multiplanar diferently, and also not catching the case where userspace
>> provides a bigger length and bytesused then the underlying buffer.
>>
>> Suggested-by: Hans Verkuil <hverkuil@xs4all.nl>
>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>> ---
>>
>> Hello,
>>
>> As discussed on
>> https://patchwork.linuxtv.org/project/linux-media/patch/gh5kef5bkeel3o6b2dkgc2dfagu9klj4c0@4ax.com/
>>
>> This patch also helps the conversion layer of the Ext API patchset,
>> where we are not exposing the length field.
>>
>> It was discussed that userspace might use a smaller length field to
>> limit the usage of the underlying buffer, but I'm not sure if this is
>> really usefull and just complicates things.
>>
>> If this is usefull, then we should also expose a length field in the Ext
>> API, and document this feature properly.
>>
>> What do you think?
>> ---
>> .../media/common/videobuf2/videobuf2-core.c   | 21 ++++++++++++++++---
>> .../media/common/videobuf2/videobuf2-v4l2.c   |  8 +++----
>> include/uapi/linux/videodev2.h                |  7 +++++--
>> 3 files changed, 27 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>> index 02281d13505f..2cbde14af051 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -1205,6 +1205,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
>>
>> 	for (plane = 0; plane < vb->num_planes; ++plane) {
>> 		struct dma_buf *dbuf = dma_buf_get(planes[plane].m.fd);
>> +		unsigned int bytesused;
>>
>> 		if (IS_ERR_OR_NULL(dbuf)) {
>> 			dprintk(q, 1, "invalid dmabuf fd for plane %d\n",
>> @@ -1213,9 +1214,23 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
>> 			goto err;
>> 		}
>>
>> -		/* use DMABUF size if length is not provided */
>> -		if (planes[plane].length == 0)
>> -			planes[plane].length = dbuf->size;
>> +		planes[plane].length = dbuf->size;
>> +		bytesused = planes[plane].bytesused ?
>> +			    planes[plane].bytesused : dbuf->size;
>> +
>> +		if (planes[plane].bytesused > planes[plane].length) {
>> +			dprintk(q, 1, "bytesused is bigger then dmabuf length for plane %d\n",
>> +				plane);
>> +			ret = -EINVAL;
>> +			goto err;
>> +		}
>> +
>> +		if (planes[plane].data_offset >= bytesused) {
>> +			dprintk(q, 1, "data_offset >= bytesused for plane %d\n",
>> +				plane);
>> +			ret = -EINVAL;
>> +			goto err;
>> +		}
>>
>> 		if (planes[plane].length < vb->planes[plane].min_length) {
>> 			dprintk(q, 1, "invalid dmabuf length %u for plane %d, minimum length %u\n",
>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> index 7e96f67c60ba..ffc7ed46f74a 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> @@ -98,14 +98,14 @@ static int __verify_length(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>> 	unsigned int bytesused;
>> 	unsigned int plane;
>>
>> -	if (V4L2_TYPE_IS_CAPTURE(b->type))
>> +	/* length check for dmabuf is performed in _prepare_dmabuf() */
>> +	if (V4L2_TYPE_IS_CAPTURE(b->type) || b->memory == VB2_MEMORY_DMABUF)
>> 		return 0;
>>
>> 	if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
>> 		for (plane = 0; plane < vb->num_planes; ++plane) {
>> -			length = (b->memory == VB2_MEMORY_USERPTR ||
>> -				  b->memory == VB2_MEMORY_DMABUF)
>> -			       ? b->m.planes[plane].length
>> +			length = b->memory == VB2_MEMORY_USERPTR
>> +				? b->m.planes[plane].length
>> 				: vb->planes[plane].length;
>> 			bytesused = b->m.planes[plane].bytesused
>> 				  ? b->m.planes[plane].bytesused : length;
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index 8d15f6ccc4b4..79b3b2893513 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -968,7 +968,9 @@ struct v4l2_requestbuffers {
>> /**
>>   * struct v4l2_plane - plane info for multi-planar buffers
>>   * @bytesused:		number of bytes occupied by data in the plane (payload)
>> - * @length:		size of this plane (NOT the payload) in bytes
>> + * @length:		size of this plane (NOT the payload) in bytes. Filled
>> + *			by userspace for USERPTR and by the driver for DMABUF
>> + *			and MMAP.
>>   * @mem_offset:		when memory in the associated struct v4l2_buffer is
>>   *			V4L2_MEMORY_MMAP, equals the offset from the start of
>>   *			the device memory for this plane (or is a "cookie" that
>> @@ -1025,7 +1027,8 @@ struct v4l2_plane {
>>   * @m:		union of @offset, @userptr, @planes and @fd
>>   * @length:	size in bytes of the buffer (NOT its payload) for single-plane
>>   *		buffers (when type != *_MPLANE); number of elements in the
>> - *		planes array for multi-plane buffers
>> + *		planes array for multi-plane buffers. Filled by userspace for
>> + *		USERPTR and by the driver for DMABUF and MMAP.
>>   * @reserved2:	drivers and applications must zero this field
>>   * @request_fd: fd of the request that this buffer should use
>>   * @reserved:	for backwards compatibility with applications that do not know
> 
> I think this does what I want. But I'm going to restate my usage desires
> and check that you agree that it covers them.
> 
> I'm interested in passing compressed bitstreams to a decoder.  The size
> of these buffers can be very variable and the worst case will nearly
> always be much larger than the typical case and that size cannot be
> known in advance of usage.  It can be very wasteful to have to allocate
> buffers that are over an order of magnitude bigger than are likely to
> ever be used.  If you have a fixed pool of fixed size buffers allocated
> at the start of time this wastefulness is unavoidable, but dmabufs can
> be dynamically sized to be as big as required and so there should be no
> limitation on passing in buffers that are smaller than the maximum.  It

Do you mean that the kernel should re-allocate the buffer dynamically
without userspace intervention?
I'm not entirely sure if this would be possible.

Regards,
Helen


> also seems plausible that dmabufs that are larger than the maximum
> should be allowed as long as their bytesused is smaller or equal.
> 
> As an aside, even when using dynamically sized dmabufs they are often
> way larger than the data they contain and forcing cache flushes or maps
> of their entire length rather than just the used portion is also
> wasteful.  This might be a use for the incoming size field.
> 
> Regards
> 
> John Cox
> 

Re: [PATCH 1/2] media: videobuf2: use dmabuf size for length

[Helen Koike]

Hi Laurent,

On 3/25/21 7:53 AM, Laurent Pinchart wrote:
> Hi Helen,
> 
> On Wed, Mar 24, 2021 at 09:17:11PM -0300, Helen Koike wrote:
>> Always use dmabuf size when considering the length of the buffer.
>> Discard userspace provided length.
>> Fix length check error in _verify_length(), which was handling single and
>> multiplanar diferently, and also not catching the case where userspace
>> provides a bigger length and bytesused then the underlying buffer.
>>
>> Suggested-by: Hans Verkuil <hverkuil@xs4all.nl>
>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>> ---
>>
>> Hello,
>>
>> As discussed on
>> https://patchwork.linuxtv.org/project/linux-media/patch/gh5kef5bkeel3o6b2dkgc2dfagu9klj4c0@4ax.com/
>>
>> This patch also helps the conversion layer of the Ext API patchset,
>> where we are not exposing the length field.
>>
>> It was discussed that userspace might use a smaller length field to
>> limit the usage of the underlying buffer, but I'm not sure if this is
>> really usefull and just complicates things.
>>
>> If this is usefull, then we should also expose a length field in the Ext
>> API, and document this feature properly.
>>
>> What do you think?
> 
> I think a limit could be useful, as a single dmabuf object could hold
> multiple planes, which should be addressed by an offset from the
> beginning of the buffer. Giving a length to the kernel could help
> catching errors. As the existing API doesn't support offsets, a length
> limit is likely not very useful at the moment, but should I believe be
> included at least in the new API.

For the new API, If there are no users, we can leave space to add it later
if such a feature becomes interesting for userspace in the future.

> 
> For the existing implementation, I'd say that we should be pragmatic. If
> using the provided length as a maximum boundary makes the implementation
> more complex for very little gain, let's not do it. But on the other
> hand, considering existing userspace, would there be added value in
> implementing such a mechanism ?

I'm guessing that userspace doesn't use this field as a boundary, since this
usage is not documented. So I'm guessing it makes things more complex for
little gain, so it doesn't seem we are adding much value to userspace.

And with this patch, it will be much easier to implement Ext API with a
conversion layer to/from the existing API.

Regards,
Helen

> 
>> ---
>>   .../media/common/videobuf2/videobuf2-core.c   | 21 ++++++++++++++++---
>>   .../media/common/videobuf2/videobuf2-v4l2.c   |  8 +++----
>>   include/uapi/linux/videodev2.h                |  7 +++++--
>>   3 files changed, 27 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>> index 02281d13505f..2cbde14af051 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -1205,6 +1205,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
>>   
>>   	for (plane = 0; plane < vb->num_planes; ++plane) {
>>   		struct dma_buf *dbuf = dma_buf_get(planes[plane].m.fd);
>> +		unsigned int bytesused;
>>   
>>   		if (IS_ERR_OR_NULL(dbuf)) {
>>   			dprintk(q, 1, "invalid dmabuf fd for plane %d\n",
>> @@ -1213,9 +1214,23 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
>>   			goto err;
>>   		}
>>   
>> -		/* use DMABUF size if length is not provided */
>> -		if (planes[plane].length == 0)
>> -			planes[plane].length = dbuf->size;
>> +		planes[plane].length = dbuf->size;
>> +		bytesused = planes[plane].bytesused ?
>> +			    planes[plane].bytesused : dbuf->size;
>> +
>> +		if (planes[plane].bytesused > planes[plane].length) {
>> +			dprintk(q, 1, "bytesused is bigger then dmabuf length for plane %d\n",
>> +				plane);
>> +			ret = -EINVAL;
>> +			goto err;
>> +		}
>> +
>> +		if (planes[plane].data_offset >= bytesused) {
>> +			dprintk(q, 1, "data_offset >= bytesused for plane %d\n",
>> +				plane);
>> +			ret = -EINVAL;
>> +			goto err;
>> +		}
>>   
>>   		if (planes[plane].length < vb->planes[plane].min_length) {
>>   			dprintk(q, 1, "invalid dmabuf length %u for plane %d, minimum length %u\n",
>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> index 7e96f67c60ba..ffc7ed46f74a 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> @@ -98,14 +98,14 @@ static int __verify_length(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>>   	unsigned int bytesused;
>>   	unsigned int plane;
>>   
>> -	if (V4L2_TYPE_IS_CAPTURE(b->type))
>> +	/* length check for dmabuf is performed in _prepare_dmabuf() */
>> +	if (V4L2_TYPE_IS_CAPTURE(b->type) || b->memory == VB2_MEMORY_DMABUF)
>>   		return 0;
>>   
>>   	if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
>>   		for (plane = 0; plane < vb->num_planes; ++plane) {
>> -			length = (b->memory == VB2_MEMORY_USERPTR ||
>> -				  b->memory == VB2_MEMORY_DMABUF)
>> -			       ? b->m.planes[plane].length
>> +			length = b->memory == VB2_MEMORY_USERPTR
>> +				? b->m.planes[plane].length
>>   				: vb->planes[plane].length;
>>   			bytesused = b->m.planes[plane].bytesused
>>   				  ? b->m.planes[plane].bytesused : length;
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index 8d15f6ccc4b4..79b3b2893513 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -968,7 +968,9 @@ struct v4l2_requestbuffers {
>>   /**
>>    * struct v4l2_plane - plane info for multi-planar buffers
>>    * @bytesused:		number of bytes occupied by data in the plane (payload)
>> - * @length:		size of this plane (NOT the payload) in bytes
>> + * @length:		size of this plane (NOT the payload) in bytes. Filled
>> + *			by userspace for USERPTR and by the driver for DMABUF
>> + *			and MMAP.
>>    * @mem_offset:		when memory in the associated struct v4l2_buffer is
>>    *			V4L2_MEMORY_MMAP, equals the offset from the start of
>>    *			the device memory for this plane (or is a "cookie" that
>> @@ -1025,7 +1027,8 @@ struct v4l2_plane {
>>    * @m:		union of @offset, @userptr, @planes and @fd
>>    * @length:	size in bytes of the buffer (NOT its payload) for single-plane
>>    *		buffers (when type != *_MPLANE); number of elements in the
>> - *		planes array for multi-plane buffers
>> + *		planes array for multi-plane buffers. Filled by userspace for
>> + *		USERPTR and by the driver for DMABUF and MMAP.
>>    * @reserved2:	drivers and applications must zero this field
>>    * @request_fd: fd of the request that this buffer should use
>>    * @reserved:	for backwards compatibility with applications that do not know
> 

Re: [PATCH 1/2] media: videobuf2: use dmabuf size for length

[John Cox]

Hi Helen

>Hi John,
>
>On 3/25/21 7:20 AM, John Cox wrote:
>> Hi
>> 
>>> Always use dmabuf size when considering the length of the buffer.
>>> Discard userspace provided length.
>>> Fix length check error in _verify_length(), which was handling single and
>>> multiplanar diferently, and also not catching the case where userspace
>>> provides a bigger length and bytesused then the underlying buffer.
>>>
>>> Suggested-by: Hans Verkuil <hverkuil@xs4all.nl>
>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>> ---
>>>
>>> Hello,
>>>
>>> As discussed on
>>> https://patchwork.linuxtv.org/project/linux-media/patch/gh5kef5bkeel3o6b2dkgc2dfagu9klj4c0@4ax.com/
>>>
>>> This patch also helps the conversion layer of the Ext API patchset,
>>> where we are not exposing the length field.
>>>
>>> It was discussed that userspace might use a smaller length field to
>>> limit the usage of the underlying buffer, but I'm not sure if this is
>>> really usefull and just complicates things.
>>>
>>> If this is usefull, then we should also expose a length field in the Ext
>>> API, and document this feature properly.
>>>
>>> What do you think?
>>> ---
>>> .../media/common/videobuf2/videobuf2-core.c   | 21 ++++++++++++++++---
>>> .../media/common/videobuf2/videobuf2-v4l2.c   |  8 +++----
>>> include/uapi/linux/videodev2.h                |  7 +++++--
>>> 3 files changed, 27 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>>> index 02281d13505f..2cbde14af051 100644
>>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>>> @@ -1205,6 +1205,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
>>>
>>> 	for (plane = 0; plane < vb->num_planes; ++plane) {
>>> 		struct dma_buf *dbuf = dma_buf_get(planes[plane].m.fd);
>>> +		unsigned int bytesused;
>>>
>>> 		if (IS_ERR_OR_NULL(dbuf)) {
>>> 			dprintk(q, 1, "invalid dmabuf fd for plane %d\n",
>>> @@ -1213,9 +1214,23 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
>>> 			goto err;
>>> 		}
>>>
>>> -		/* use DMABUF size if length is not provided */
>>> -		if (planes[plane].length == 0)
>>> -			planes[plane].length = dbuf->size;
>>> +		planes[plane].length = dbuf->size;
>>> +		bytesused = planes[plane].bytesused ?
>>> +			    planes[plane].bytesused : dbuf->size;
>>> +
>>> +		if (planes[plane].bytesused > planes[plane].length) {
>>> +			dprintk(q, 1, "bytesused is bigger then dmabuf length for plane %d\n",
>>> +				plane);
>>> +			ret = -EINVAL;
>>> +			goto err;
>>> +		}
>>> +
>>> +		if (planes[plane].data_offset >= bytesused) {
>>> +			dprintk(q, 1, "data_offset >= bytesused for plane %d\n",
>>> +				plane);
>>> +			ret = -EINVAL;
>>> +			goto err;
>>> +		}
>>>
>>> 		if (planes[plane].length < vb->planes[plane].min_length) {
>>> 			dprintk(q, 1, "invalid dmabuf length %u for plane %d, minimum length %u\n",
>>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>> index 7e96f67c60ba..ffc7ed46f74a 100644
>>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>> @@ -98,14 +98,14 @@ static int __verify_length(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>>> 	unsigned int bytesused;
>>> 	unsigned int plane;
>>>
>>> -	if (V4L2_TYPE_IS_CAPTURE(b->type))
>>> +	/* length check for dmabuf is performed in _prepare_dmabuf() */
>>> +	if (V4L2_TYPE_IS_CAPTURE(b->type) || b->memory == VB2_MEMORY_DMABUF)
>>> 		return 0;
>>>
>>> 	if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
>>> 		for (plane = 0; plane < vb->num_planes; ++plane) {
>>> -			length = (b->memory == VB2_MEMORY_USERPTR ||
>>> -				  b->memory == VB2_MEMORY_DMABUF)
>>> -			       ? b->m.planes[plane].length
>>> +			length = b->memory == VB2_MEMORY_USERPTR
>>> +				? b->m.planes[plane].length
>>> 				: vb->planes[plane].length;
>>> 			bytesused = b->m.planes[plane].bytesused
>>> 				  ? b->m.planes[plane].bytesused : length;
>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>> index 8d15f6ccc4b4..79b3b2893513 100644
>>> --- a/include/uapi/linux/videodev2.h
>>> +++ b/include/uapi/linux/videodev2.h
>>> @@ -968,7 +968,9 @@ struct v4l2_requestbuffers {
>>> /**
>>>   * struct v4l2_plane - plane info for multi-planar buffers
>>>   * @bytesused:		number of bytes occupied by data in the plane (payload)
>>> - * @length:		size of this plane (NOT the payload) in bytes
>>> + * @length:		size of this plane (NOT the payload) in bytes. Filled
>>> + *			by userspace for USERPTR and by the driver for DMABUF
>>> + *			and MMAP.
>>>   * @mem_offset:		when memory in the associated struct v4l2_buffer is
>>>   *			V4L2_MEMORY_MMAP, equals the offset from the start of
>>>   *			the device memory for this plane (or is a "cookie" that
>>> @@ -1025,7 +1027,8 @@ struct v4l2_plane {
>>>   * @m:		union of @offset, @userptr, @planes and @fd
>>>   * @length:	size in bytes of the buffer (NOT its payload) for single-plane
>>>   *		buffers (when type != *_MPLANE); number of elements in the
>>> - *		planes array for multi-plane buffers
>>> + *		planes array for multi-plane buffers. Filled by userspace for
>>> + *		USERPTR and by the driver for DMABUF and MMAP.
>>>   * @reserved2:	drivers and applications must zero this field
>>>   * @request_fd: fd of the request that this buffer should use
>>>   * @reserved:	for backwards compatibility with applications that do not know
>> 
>> I think this does what I want. But I'm going to restate my usage desires
>> and check that you agree that it covers them.
>> 
>> I'm interested in passing compressed bitstreams to a decoder.  The size
>> of these buffers can be very variable and the worst case will nearly
>> always be much larger than the typical case and that size cannot be
>> known in advance of usage.  It can be very wasteful to have to allocate
>> buffers that are over an order of magnitude bigger than are likely to
>> ever be used.  If you have a fixed pool of fixed size buffers allocated
>> at the start of time this wastefulness is unavoidable, but dmabufs can
>> be dynamically sized to be as big as required and so there should be no
>> limitation on passing in buffers that are smaller than the maximum.  It
>
>Do you mean that the kernel should re-allocate the buffer dynamically
>without userspace intervention?
>I'm not entirely sure if this would be possible.

No - I didn't mean that at all.  Any reallocation would be done by the
user.  I was just setting out why damabufs are different from (and more
useful than) MMAP buffers for bitstream-like purposes.

Regards

John Cox

>Regards,
>Helen
>
>
>> also seems plausible that dmabufs that are larger than the maximum
>> should be allowed as long as their bytesused is smaller or equal.
>> 
>> As an aside, even when using dynamically sized dmabufs they are often
>> way larger than the data they contain and forcing cache flushes or maps
>> of their entire length rather than just the used portion is also
>> wasteful.  This might be a use for the incoming size field.
>> 
>> Regards
>> 
>> John Cox
>> 

Re: [PATCH 1/2] media: videobuf2: use dmabuf size for length

[Helen Koike]

On 3/26/21 10:03 AM, John Cox wrote:
> Hi Helen
> 
>> Hi John,
>>
>> On 3/25/21 7:20 AM, John Cox wrote:
>>> Hi
>>>
>>>> Always use dmabuf size when considering the length of the buffer.
>>>> Discard userspace provided length.
>>>> Fix length check error in _verify_length(), which was handling single and
>>>> multiplanar diferently, and also not catching the case where userspace
>>>> provides a bigger length and bytesused then the underlying buffer.
>>>>
>>>> Suggested-by: Hans Verkuil <hverkuil@xs4all.nl>
>>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>>> ---
>>>>
>>>> Hello,
>>>>
>>>> As discussed on
>>>> https://patchwork.linuxtv.org/project/linux-media/patch/gh5kef5bkeel3o6b2dkgc2dfagu9klj4c0@4ax.com/
>>>>
>>>> This patch also helps the conversion layer of the Ext API patchset,
>>>> where we are not exposing the length field.
>>>>
>>>> It was discussed that userspace might use a smaller length field to
>>>> limit the usage of the underlying buffer, but I'm not sure if this is
>>>> really usefull and just complicates things.
>>>>
>>>> If this is usefull, then we should also expose a length field in the Ext
>>>> API, and document this feature properly.
>>>>
>>>> What do you think?
>>>> ---
>>>> .../media/common/videobuf2/videobuf2-core.c   | 21 ++++++++++++++++---
>>>> .../media/common/videobuf2/videobuf2-v4l2.c   |  8 +++----
>>>> include/uapi/linux/videodev2.h                |  7 +++++--
>>>> 3 files changed, 27 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>>>> index 02281d13505f..2cbde14af051 100644
>>>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>>>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>>>> @@ -1205,6 +1205,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
>>>>
>>>> 	for (plane = 0; plane < vb->num_planes; ++plane) {
>>>> 		struct dma_buf *dbuf = dma_buf_get(planes[plane].m.fd);
>>>> +		unsigned int bytesused;
>>>>
>>>> 		if (IS_ERR_OR_NULL(dbuf)) {
>>>> 			dprintk(q, 1, "invalid dmabuf fd for plane %d\n",
>>>> @@ -1213,9 +1214,23 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
>>>> 			goto err;
>>>> 		}
>>>>
>>>> -		/* use DMABUF size if length is not provided */
>>>> -		if (planes[plane].length == 0)
>>>> -			planes[plane].length = dbuf->size;
>>>> +		planes[plane].length = dbuf->size;
>>>> +		bytesused = planes[plane].bytesused ?
>>>> +			    planes[plane].bytesused : dbuf->size;
>>>> +
>>>> +		if (planes[plane].bytesused > planes[plane].length) {
>>>> +			dprintk(q, 1, "bytesused is bigger then dmabuf length for plane %d\n",
>>>> +				plane);
>>>> +			ret = -EINVAL;
>>>> +			goto err;
>>>> +		}
>>>> +
>>>> +		if (planes[plane].data_offset >= bytesused) {
>>>> +			dprintk(q, 1, "data_offset >= bytesused for plane %d\n",
>>>> +				plane);
>>>> +			ret = -EINVAL;
>>>> +			goto err;
>>>> +		}
>>>>
>>>> 		if (planes[plane].length < vb->planes[plane].min_length) {
>>>> 			dprintk(q, 1, "invalid dmabuf length %u for plane %d, minimum length %u\n",
>>>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>>> index 7e96f67c60ba..ffc7ed46f74a 100644
>>>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>>> @@ -98,14 +98,14 @@ static int __verify_length(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>>>> 	unsigned int bytesused;
>>>> 	unsigned int plane;
>>>>
>>>> -	if (V4L2_TYPE_IS_CAPTURE(b->type))
>>>> +	/* length check for dmabuf is performed in _prepare_dmabuf() */
>>>> +	if (V4L2_TYPE_IS_CAPTURE(b->type) || b->memory == VB2_MEMORY_DMABUF)
>>>> 		return 0;
>>>>
>>>> 	if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
>>>> 		for (plane = 0; plane < vb->num_planes; ++plane) {
>>>> -			length = (b->memory == VB2_MEMORY_USERPTR ||
>>>> -				  b->memory == VB2_MEMORY_DMABUF)
>>>> -			       ? b->m.planes[plane].length
>>>> +			length = b->memory == VB2_MEMORY_USERPTR
>>>> +				? b->m.planes[plane].length
>>>> 				: vb->planes[plane].length;
>>>> 			bytesused = b->m.planes[plane].bytesused
>>>> 				  ? b->m.planes[plane].bytesused : length;
>>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>>> index 8d15f6ccc4b4..79b3b2893513 100644
>>>> --- a/include/uapi/linux/videodev2.h
>>>> +++ b/include/uapi/linux/videodev2.h
>>>> @@ -968,7 +968,9 @@ struct v4l2_requestbuffers {
>>>> /**
>>>>    * struct v4l2_plane - plane info for multi-planar buffers
>>>>    * @bytesused:		number of bytes occupied by data in the plane (payload)
>>>> - * @length:		size of this plane (NOT the payload) in bytes
>>>> + * @length:		size of this plane (NOT the payload) in bytes. Filled
>>>> + *			by userspace for USERPTR and by the driver for DMABUF
>>>> + *			and MMAP.
>>>>    * @mem_offset:		when memory in the associated struct v4l2_buffer is
>>>>    *			V4L2_MEMORY_MMAP, equals the offset from the start of
>>>>    *			the device memory for this plane (or is a "cookie" that
>>>> @@ -1025,7 +1027,8 @@ struct v4l2_plane {
>>>>    * @m:		union of @offset, @userptr, @planes and @fd
>>>>    * @length:	size in bytes of the buffer (NOT its payload) for single-plane
>>>>    *		buffers (when type != *_MPLANE); number of elements in the
>>>> - *		planes array for multi-plane buffers
>>>> + *		planes array for multi-plane buffers. Filled by userspace for
>>>> + *		USERPTR and by the driver for DMABUF and MMAP.
>>>>    * @reserved2:	drivers and applications must zero this field
>>>>    * @request_fd: fd of the request that this buffer should use
>>>>    * @reserved:	for backwards compatibility with applications that do not know
>>>
>>> I think this does what I want. But I'm going to restate my usage desires
>>> and check that you agree that it covers them.
>>>
>>> I'm interested in passing compressed bitstreams to a decoder.  The size
>>> of these buffers can be very variable and the worst case will nearly
>>> always be much larger than the typical case and that size cannot be
>>> known in advance of usage.  It can be very wasteful to have to allocate
>>> buffers that are over an order of magnitude bigger than are likely to
>>> ever be used.  If you have a fixed pool of fixed size buffers allocated
>>> at the start of time this wastefulness is unavoidable, but dmabufs can
>>> be dynamically sized to be as big as required and so there should be no
>>> limitation on passing in buffers that are smaller than the maximum.  It
>>
>> Do you mean that the kernel should re-allocate the buffer dynamically
>> without userspace intervention?
>> I'm not entirely sure if this would be possible.
> 
> No - I didn't mean that at all.  Any reallocation would be done by the
> user.  I was just setting out why damabufs are different from (and more
> useful than) MMAP buffers for bitstream-like purposes.

Right, thanks for the clarification.

> 
> Regards
> 
> John Cox
> 
>> Regards,
>> Helen
>>
>>
>>> also seems plausible that dmabufs that are larger than the maximum
>>> should be allowed as long as their bytesused is smaller or equal.

If I understand correctly, the requirements would be:

(consider maximum being the length/boundary provided by userspace).

(1) bytesused <= maximum && bytesused <= dmabuf_length, this must always be true.
(2) maximum <= dmabuf_length is always ok.
(3) dmabuf_length <= maximum is ok as long (1) is still true.
if dmabuf_length <= maximum, but bytesused > maximum, then it is not ok.

Make sense?

We could save in vb2:
bytesused_max = maximum ? min(maximum, dmabuf_length) : dmabuf_length;

Then drivers could check if if bytesused <= bytesused_max,
and we don't need to check dma_length against the maximum value.

Or maybe there is little value in letting userspace define a maximum.

What do you think we should do? Remove the maximum (as implemented in this patch)?
Or just comparing against bytesused_max is enough (which would keeping the boundary
feature) ?

I would prefer to remove the maximum if there is no value for userspace, since
this would make things easier for the Ext API implementation.

>>>
>>> As an aside, even when using dynamically sized dmabufs they are often
>>> way larger than the data they contain and forcing cache flushes or maps
>>> of their entire length rather than just the used portion is also
>>> wasteful.  This might be a use for the incoming size field.

I guess this can be achieved using the bytesused field.

Regards,
Helen

>>>
>>> Regards
>>>
>>> John Cox
>>>

Re: [PATCH 1/2] media: videobuf2: use dmabuf size for length

[John Cox]

Hi Helen

>On 3/26/21 10:03 AM, John Cox wrote:
>> Hi Helen
>> 
>>> Hi John,
>>>
>>> On 3/25/21 7:20 AM, John Cox wrote:
>>>> Hi
>>>>
>>>>> Always use dmabuf size when considering the length of the buffer.
>>>>> Discard userspace provided length.
>>>>> Fix length check error in _verify_length(), which was handling single and
>>>>> multiplanar diferently, and also not catching the case where userspace
>>>>> provides a bigger length and bytesused then the underlying buffer.
>>>>>
>>>>> Suggested-by: Hans Verkuil <hverkuil@xs4all.nl>
>>>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>>>> ---
>>>>>
>>>>> Hello,
>>>>>
>>>>> As discussed on
>>>>> https://patchwork.linuxtv.org/project/linux-media/patch/gh5kef5bkeel3o6b2dkgc2dfagu9klj4c0@4ax.com/
>>>>>
>>>>> This patch also helps the conversion layer of the Ext API patchset,
>>>>> where we are not exposing the length field.
>>>>>
>>>>> It was discussed that userspace might use a smaller length field to
>>>>> limit the usage of the underlying buffer, but I'm not sure if this is
>>>>> really usefull and just complicates things.
>>>>>
>>>>> If this is usefull, then we should also expose a length field in the Ext
>>>>> API, and document this feature properly.
>>>>>
>>>>> What do you think?
>>>>> ---
>>>>> .../media/common/videobuf2/videobuf2-core.c   | 21 ++++++++++++++++---
>>>>> .../media/common/videobuf2/videobuf2-v4l2.c   |  8 +++----
>>>>> include/uapi/linux/videodev2.h                |  7 +++++--
>>>>> 3 files changed, 27 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>>>>> index 02281d13505f..2cbde14af051 100644
>>>>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>>>>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>>>>> @@ -1205,6 +1205,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
>>>>>
>>>>> 	for (plane = 0; plane < vb->num_planes; ++plane) {
>>>>> 		struct dma_buf *dbuf = dma_buf_get(planes[plane].m.fd);
>>>>> +		unsigned int bytesused;
>>>>>
>>>>> 		if (IS_ERR_OR_NULL(dbuf)) {
>>>>> 			dprintk(q, 1, "invalid dmabuf fd for plane %d\n",
>>>>> @@ -1213,9 +1214,23 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
>>>>> 			goto err;
>>>>> 		}
>>>>>
>>>>> -		/* use DMABUF size if length is not provided */
>>>>> -		if (planes[plane].length == 0)
>>>>> -			planes[plane].length = dbuf->size;
>>>>> +		planes[plane].length = dbuf->size;
>>>>> +		bytesused = planes[plane].bytesused ?
>>>>> +			    planes[plane].bytesused : dbuf->size;
>>>>> +
>>>>> +		if (planes[plane].bytesused > planes[plane].length) {
>>>>> +			dprintk(q, 1, "bytesused is bigger then dmabuf length for plane %d\n",
>>>>> +				plane);
>>>>> +			ret = -EINVAL;
>>>>> +			goto err;
>>>>> +		}
>>>>> +
>>>>> +		if (planes[plane].data_offset >= bytesused) {
>>>>> +			dprintk(q, 1, "data_offset >= bytesused for plane %d\n",
>>>>> +				plane);
>>>>> +			ret = -EINVAL;
>>>>> +			goto err;
>>>>> +		}
>>>>>
>>>>> 		if (planes[plane].length < vb->planes[plane].min_length) {
>>>>> 			dprintk(q, 1, "invalid dmabuf length %u for plane %d, minimum length %u\n",
>>>>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>>>> index 7e96f67c60ba..ffc7ed46f74a 100644
>>>>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>>>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>>>> @@ -98,14 +98,14 @@ static int __verify_length(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>>>>> 	unsigned int bytesused;
>>>>> 	unsigned int plane;
>>>>>
>>>>> -	if (V4L2_TYPE_IS_CAPTURE(b->type))
>>>>> +	/* length check for dmabuf is performed in _prepare_dmabuf() */
>>>>> +	if (V4L2_TYPE_IS_CAPTURE(b->type) || b->memory == VB2_MEMORY_DMABUF)
>>>>> 		return 0;
>>>>>
>>>>> 	if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
>>>>> 		for (plane = 0; plane < vb->num_planes; ++plane) {
>>>>> -			length = (b->memory == VB2_MEMORY_USERPTR ||
>>>>> -				  b->memory == VB2_MEMORY_DMABUF)
>>>>> -			       ? b->m.planes[plane].length
>>>>> +			length = b->memory == VB2_MEMORY_USERPTR
>>>>> +				? b->m.planes[plane].length
>>>>> 				: vb->planes[plane].length;
>>>>> 			bytesused = b->m.planes[plane].bytesused
>>>>> 				  ? b->m.planes[plane].bytesused : length;
>>>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>>>> index 8d15f6ccc4b4..79b3b2893513 100644
>>>>> --- a/include/uapi/linux/videodev2.h
>>>>> +++ b/include/uapi/linux/videodev2.h
>>>>> @@ -968,7 +968,9 @@ struct v4l2_requestbuffers {
>>>>> /**
>>>>>    * struct v4l2_plane - plane info for multi-planar buffers
>>>>>    * @bytesused:		number of bytes occupied by data in the plane (payload)
>>>>> - * @length:		size of this plane (NOT the payload) in bytes
>>>>> + * @length:		size of this plane (NOT the payload) in bytes. Filled
>>>>> + *			by userspace for USERPTR and by the driver for DMABUF
>>>>> + *			and MMAP.
>>>>>    * @mem_offset:		when memory in the associated struct v4l2_buffer is
>>>>>    *			V4L2_MEMORY_MMAP, equals the offset from the start of
>>>>>    *			the device memory for this plane (or is a "cookie" that
>>>>> @@ -1025,7 +1027,8 @@ struct v4l2_plane {
>>>>>    * @m:		union of @offset, @userptr, @planes and @fd
>>>>>    * @length:	size in bytes of the buffer (NOT its payload) for single-plane
>>>>>    *		buffers (when type != *_MPLANE); number of elements in the
>>>>> - *		planes array for multi-plane buffers
>>>>> + *		planes array for multi-plane buffers. Filled by userspace for
>>>>> + *		USERPTR and by the driver for DMABUF and MMAP.
>>>>>    * @reserved2:	drivers and applications must zero this field
>>>>>    * @request_fd: fd of the request that this buffer should use
>>>>>    * @reserved:	for backwards compatibility with applications that do not know
>>>>
>>>> I think this does what I want. But I'm going to restate my usage desires
>>>> and check that you agree that it covers them.
>>>>
>>>> I'm interested in passing compressed bitstreams to a decoder.  The size
>>>> of these buffers can be very variable and the worst case will nearly
>>>> always be much larger than the typical case and that size cannot be
>>>> known in advance of usage.  It can be very wasteful to have to allocate
>>>> buffers that are over an order of magnitude bigger than are likely to
>>>> ever be used.  If you have a fixed pool of fixed size buffers allocated
>>>> at the start of time this wastefulness is unavoidable, but dmabufs can
>>>> be dynamically sized to be as big as required and so there should be no
>>>> limitation on passing in buffers that are smaller than the maximum.  It
>>>
>>> Do you mean that the kernel should re-allocate the buffer dynamically
>>> without userspace intervention?
>>> I'm not entirely sure if this would be possible.
>> 
>> No - I didn't mean that at all.  Any reallocation would be done by the
>> user.  I was just setting out why damabufs are different from (and more
>> useful than) MMAP buffers for bitstream-like purposes.
>
>Right, thanks for the clarification.
>
>> 
>> Regards
>> 
>> John Cox
>> 
>>> Regards,
>>> Helen
>>>
>>>
>>>> also seems plausible that dmabufs that are larger than the maximum
>>>> should be allowed as long as their bytesused is smaller or equal.
>
>If I understand correctly, the requirements would be:
>
>(consider maximum being the length/boundary provided by userspace).
>
>(1) bytesused <= maximum && bytesused <= dmabuf_length, this must always be true.
>(2) maximum <= dmabuf_length is always ok.
>(3) dmabuf_length <= maximum is ok as long (1) is still true.
>if dmabuf_length <= maximum, but bytesused > maximum, then it is not ok.
>
>Make sense?
>
>We could save in vb2:
>bytesused_max = maximum ? min(maximum, dmabuf_length) : dmabuf_length;
>
>Then drivers could check if if bytesused <= bytesused_max,
>and we don't need to check dma_length against the maximum value.
>
>Or maybe there is little value in letting userspace define a maximum.
>
>What do you think we should do? Remove the maximum (as implemented in this patch)?
>Or just comparing against bytesused_max is enough (which would keeping the boundary
>feature) ?
>
>I would prefer to remove the maximum if there is no value for userspace, since
>this would make things easier for the Ext API implementation.

From my personal PoV, for an OUTPUT buffer, as long as the data fits in
the buffer i.e. bytesused <= dmabuf_length then that is all I really
care about. Other peoples mileage may vary!

Thanks

JC


>>>>
>>>> As an aside, even when using dynamically sized dmabufs they are often
>>>> way larger than the data they contain and forcing cache flushes or maps
>>>> of their entire length rather than just the used portion is also
>>>> wasteful.  This might be a use for the incoming size field.
>
>I guess this can be achieved using the bytesused field.
>
>Regards,
>Helen
>
>>>>
>>>> Regards
>>>>
>>>> John Cox
>>>>

Re: [PATCH 1/2] media: videobuf2: use dmabuf size for length

[Hans Verkuil]

Hi Helen & others,

I'm going through a bunch of (very) old patches in my patchwork TODO list
that for one reason or another I never processed. This patch series is one
of them. Given the discussion that followed the post of this series I've
decided not to merge this. I'll mark the series as 'Changes Requested'.

If someone wants to continue work on this (Helen left Collabora), then
please do so!

Regards,

	Hans


On 25/03/2021 01:17, Helen Koike wrote:
> Always use dmabuf size when considering the length of the buffer.
> Discard userspace provided length.
> Fix length check error in _verify_length(), which was handling single and
> multiplanar diferently, and also not catching the case where userspace
> provides a bigger length and bytesused then the underlying buffer.
> 
> Suggested-by: Hans Verkuil <hverkuil@xs4all.nl>
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> ---
> 
> Hello,
> 
> As discussed on
> https://patchwork.linuxtv.org/project/linux-media/patch/gh5kef5bkeel3o6b2dkgc2dfagu9klj4c0@4ax.com/
> 
> This patch also helps the conversion layer of the Ext API patchset,
> where we are not exposing the length field.
> 
> It was discussed that userspace might use a smaller length field to
> limit the usage of the underlying buffer, but I'm not sure if this is
> really usefull and just complicates things.
> 
> If this is usefull, then we should also expose a length field in the Ext
> API, and document this feature properly.
> 
> What do you think?
> ---
>  .../media/common/videobuf2/videobuf2-core.c   | 21 ++++++++++++++++---
>  .../media/common/videobuf2/videobuf2-v4l2.c   |  8 +++----
>  include/uapi/linux/videodev2.h                |  7 +++++--
>  3 files changed, 27 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 02281d13505f..2cbde14af051 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -1205,6 +1205,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
>  
>  	for (plane = 0; plane < vb->num_planes; ++plane) {
>  		struct dma_buf *dbuf = dma_buf_get(planes[plane].m.fd);
> +		unsigned int bytesused;
>  
>  		if (IS_ERR_OR_NULL(dbuf)) {
>  			dprintk(q, 1, "invalid dmabuf fd for plane %d\n",
> @@ -1213,9 +1214,23 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
>  			goto err;
>  		}
>  
> -		/* use DMABUF size if length is not provided */
> -		if (planes[plane].length == 0)
> -			planes[plane].length = dbuf->size;
> +		planes[plane].length = dbuf->size;
> +		bytesused = planes[plane].bytesused ?
> +			    planes[plane].bytesused : dbuf->size;
> +
> +		if (planes[plane].bytesused > planes[plane].length) {
> +			dprintk(q, 1, "bytesused is bigger then dmabuf length for plane %d\n",
> +				plane);
> +			ret = -EINVAL;
> +			goto err;
> +		}
> +
> +		if (planes[plane].data_offset >= bytesused) {
> +			dprintk(q, 1, "data_offset >= bytesused for plane %d\n",
> +				plane);
> +			ret = -EINVAL;
> +			goto err;
> +		}
>  
>  		if (planes[plane].length < vb->planes[plane].min_length) {
>  			dprintk(q, 1, "invalid dmabuf length %u for plane %d, minimum length %u\n",
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 7e96f67c60ba..ffc7ed46f74a 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -98,14 +98,14 @@ static int __verify_length(struct vb2_buffer *vb, const struct v4l2_buffer *b)
>  	unsigned int bytesused;
>  	unsigned int plane;
>  
> -	if (V4L2_TYPE_IS_CAPTURE(b->type))
> +	/* length check for dmabuf is performed in _prepare_dmabuf() */
> +	if (V4L2_TYPE_IS_CAPTURE(b->type) || b->memory == VB2_MEMORY_DMABUF)
>  		return 0;
>  
>  	if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
>  		for (plane = 0; plane < vb->num_planes; ++plane) {
> -			length = (b->memory == VB2_MEMORY_USERPTR ||
> -				  b->memory == VB2_MEMORY_DMABUF)
> -			       ? b->m.planes[plane].length
> +			length = b->memory == VB2_MEMORY_USERPTR
> +				? b->m.planes[plane].length
>  				: vb->planes[plane].length;
>  			bytesused = b->m.planes[plane].bytesused
>  				  ? b->m.planes[plane].bytesused : length;
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 8d15f6ccc4b4..79b3b2893513 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -968,7 +968,9 @@ struct v4l2_requestbuffers {
>  /**
>   * struct v4l2_plane - plane info for multi-planar buffers
>   * @bytesused:		number of bytes occupied by data in the plane (payload)
> - * @length:		size of this plane (NOT the payload) in bytes
> + * @length:		size of this plane (NOT the payload) in bytes. Filled
> + *			by userspace for USERPTR and by the driver for DMABUF
> + *			and MMAP.
>   * @mem_offset:		when memory in the associated struct v4l2_buffer is
>   *			V4L2_MEMORY_MMAP, equals the offset from the start of
>   *			the device memory for this plane (or is a "cookie" that
> @@ -1025,7 +1027,8 @@ struct v4l2_plane {
>   * @m:		union of @offset, @userptr, @planes and @fd
>   * @length:	size in bytes of the buffer (NOT its payload) for single-plane
>   *		buffers (when type != *_MPLANE); number of elements in the
> - *		planes array for multi-plane buffers
> + *		planes array for multi-plane buffers. Filled by userspace for
> + *		USERPTR and by the driver for DMABUF and MMAP.
>   * @reserved2:	drivers and applications must zero this field
>   * @request_fd: fd of the request that this buffer should use
>   * @reserved:	for backwards compatibility with applications that do not know