media: videobuf2-vmalloc: get_userptr: buffers are always writable

In vb2_vmalloc_get_userptr() the framevector is created with the
'write' argument set to false when vb2_create_framevec() is called
for OUTPUT buffers. So the pages are marked as read-only.

However, userspace will write to these buffers since it will fill
in the data to output. Since get_userptr is only called if the userptr
of the queued buffer has changed since the last time that same buffer
was queued, this will fail when the buffer contents is updated and the
buffer is queued again.

E.g., userspace fills buffer 1 with the output video and queues it.
The first time get_userptr is called and the pages are grabbed and
pinned in memory and marked read-only. The second time buffer 1 is
filled with different video data and queued again. Since the userptr
hasn't changed the get_userptr() callback isn't called again. Since
the pages were marked as read-only the new contents isn't updated.

Just always call vb2_create_framevec() with FOLL_WRITE to always
allow writing to the buffers.

Using USERPTR streaming with OUTPUT devices is almost never done. And
when it is done it is via v4l2-compliance and a driver like vim2m. But
since v4l2-compliance doesn't actually inspect the capture buffer and
compare it to the original output buffer, this issue was never noticed.

But the vicodec driver actually needs to parse the bitstream in the
OUTPUT buffers and any errors there will be immediately noticed. So
this time v4l2-compliance failed the USERPTR streaming test.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>

diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
index ecbef266130b9fc7823f4858d46f148ce1f5253d..7d77e4d30c8ab987b6a35529968a2afb5505eabd 100644 (file)
--- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
@@ -475,8 +475,7 @@ static void *vb2_dc_get_userptr(struct device *dev, unsigned long vaddr,
        buf->dma_dir = dma_dir;
 
        offset = lower_32_bits(offset_in_page(vaddr));
-       vec = vb2_create_framevec(vaddr, size, dma_dir == DMA_FROM_DEVICE ||
-                                              dma_dir == DMA_BIDIRECTIONAL);
+       vec = vb2_create_framevec(vaddr, size);
        if (IS_ERR(vec)) {
                ret = PTR_ERR(vec);
                goto fail_buf;

diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
index 0f06f08346ba5d7ac8bf36a1e47d83c9b8a05772..ed706b2a263c57960b724439c1ab6057c65d21c9 100644 (file)
--- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
@@ -239,8 +239,7 @@ static void *vb2_dma_sg_get_userptr(struct device *dev, unsigned long vaddr,
        buf->offset = vaddr & ~PAGE_MASK;
        buf->size = size;
        buf->dma_sgt = &buf->sg_table;
-       vec = vb2_create_framevec(vaddr, size, dma_dir == DMA_FROM_DEVICE ||
-                                              dma_dir == DMA_BIDIRECTIONAL);
+       vec = vb2_create_framevec(vaddr, size);
        if (IS_ERR(vec))
                goto userptr_fail_pfnvec;
        buf->vec = vec;

diff --git a/drivers/media/common/videobuf2/videobuf2-memops.c b/drivers/media/common/videobuf2/videobuf2-memops.c
index c4a85be48ac25fe796fc339ada72f5e5f9dc9bde..6e9e05153f4e1ff57b886e3f4eef1e0bc9f38ef6 100644 (file)
--- a/drivers/media/common/videobuf2/videobuf2-memops.c
+++ b/drivers/media/common/videobuf2/videobuf2-memops.c
@@ -26,7 +26,6 @@
  * vb2_create_framevec() - map virtual addresses to pfns
  * @start:     Virtual user address where we start mapping
  * @length:    Length of a range to map
- * @write:     Should we map for writing into the area
  *
  * This function allocates and fills in a vector with pfns corresponding to
  * virtual address range passed in arguments. If pfns have corresponding pages,
@@ -35,17 +34,13 @@
  * failure. Returned vector needs to be freed via vb2_destroy_pfnvec().
  */
 struct frame_vector *vb2_create_framevec(unsigned long start,
-                                        unsigned long length,
-                                        bool write)
+                                        unsigned long length)
 {
        int ret;
        unsigned long first, last;
        unsigned long nr;
        struct frame_vector *vec;
-       unsigned int flags = FOLL_FORCE;
-
-       if (write)
-               flags |= FOLL_WRITE;
+       unsigned int flags = FOLL_FORCE | FOLL_WRITE;
 
        first = start >> PAGE_SHIFT;
        last = (start + length - 1) >> PAGE_SHIFT;

diff --git a/drivers/media/common/videobuf2/videobuf2-vmalloc.c b/drivers/media/common/videobuf2/videobuf2-vmalloc.c
index 1c6659f7c39442aeef6b59d39b7b3965e887a354..04d51ca6322399589fc62345044146882de643dd 100644 (file)
--- a/drivers/media/common/videobuf2/videobuf2-vmalloc.c
+++ b/drivers/media/common/videobuf2/videobuf2-vmalloc.c
@@ -87,8 +87,7 @@ static void *vb2_vmalloc_get_userptr(struct device *dev, unsigned long vaddr,
        buf->dma_dir = dma_dir;
        offset = vaddr & ~PAGE_MASK;
        buf->size = size;
-       vec = vb2_create_framevec(vaddr, size, dma_dir == DMA_FROM_DEVICE ||
-                                              dma_dir == DMA_BIDIRECTIONAL);
+       vec = vb2_create_framevec(vaddr, size);
        if (IS_ERR(vec)) {
                ret = PTR_ERR(vec);
                goto fail_pfnvec_create;

diff --git a/include/media/videobuf2-memops.h b/include/media/videobuf2-memops.h
index 4b5b84f93538ed4cc808dbc6cfdf18a448a5abab..cd4a463315313410a75b98b2a567d5f75925824e 100644 (file)
--- a/include/media/videobuf2-memops.h
+++ b/include/media/videobuf2-memops.h
@@ -34,8 +34,7 @@ struct vb2_vmarea_handler {
 extern const struct vm_operations_struct vb2_common_vm_ops;
 
 struct frame_vector *vb2_create_framevec(unsigned long start,
-                                        unsigned long length,
-                                        bool write);
+                                        unsigned long length);
 void vb2_destroy_framevec(struct frame_vector *vec);
 
 #endif